Repository: redhat-certification/chart-verifier
Pull request #261 (2022-05-09 10:21:31)

Add timeout option in the verify cmd
This PR will cover story: https://issues.redhat.com/browse/HELM-324

If the chart contains hooks that takes more time to finish installation and test a release, timeout is implemented (with default value of 30 mins) as total time chart-verifier should wait till the chart install and test finishes.

mmulholla: @tisutisu 
Please also update the README file to include the new option and some words about it.

Also would be good if the message said what the timeout was set to and advise user to consider increasing the default timeout by setting the new option, if more time is needed. 

Patch:
diff --git a/cmd/verify.go b/cmd/verify.go
index e71c10e..cddaee2 100644
--- a/cmd/verify.go
+++ b/cmd/verify.go
@@ -19,6 +19,8 @@ package cmd
 import (
 	"encoding/json"
 	"fmt"
+	"time"
+
 	"github.com/redhat-certification/chart-verifier/pkg/chartverifier/checks"
 	"github.com/redhat-certification/chart-verifier/pkg/chartverifier/profiles"
 	"github.com/redhat-certification/chart-verifier/pkg/chartverifier/utils"
@@ -60,6 +62,8 @@ var (
 	suppressErrorLog bool
 	// provider controls chart deliver mechanism.
 	providerDelivery bool
+	//client timeout
+	clientTimeout time.Duration
 )
 
 func filterChecks(set profiles.FilteredRegistry, subset []string, setEnabled bool, subsetEnabled bool) (chartverifier.FilteredRegistry, error) {
@@ -132,6 +136,7 @@ func NewVerifyCmd(config *viper.Viper) *cobra.Command {
 
 			utils.LogInfo(fmt.Sprintf("Chart Verifer %s.", Version))
 			utils.LogInfo(fmt.Sprintf("Verify : %s", args[0]))
+			utils.LogInfo(fmt.Sprintf("Client timeout: %s", clientTimeout))
 
 			// vals is a resulting map considering all the options the user has given.
 			vals, err := opts.MergeValues(getter.All(settings))
@@ -156,6 +161,7 @@ func NewVerifyCmd(config *viper.Viper) *cobra.Command {
 				SetToolVersion(Version).
 				SetOpenShiftVersion(openshiftVersionFlag).
 				SetProviderDelivery(providerDelivery).
+				SetTimeout(clientTimeout).
 				Build()
 
 			if err != nil {
@@ -211,6 +217,7 @@ func NewVerifyCmd(config *viper.Viper) *cobra.Command {
 
 	cmd.Flags().StringSliceVarP(&verifyOpts.ValueFiles, "set-values", "f", nil, "specify application and check configuration values in a YAML file or a URL (can specify multiple)")
 	cmd.Flags().StringVarP(&openshiftVersionFlag, "openshift-version", "V", "", "version of OpenShift used in the cluster")
+	cmd.Flags().DurationVar(&clientTimeout, "timeout", 30*time.Minute, "time to wait for completion of chart install and test")
 	cmd.Flags().BoolVarP(&reportToFile, "write-to-file", "w", false, "write report to ./chartverifier/report.yaml (default: stdout)")
 	cmd.Flags().BoolVarP(&suppressErrorLog, "suppress-error-log", "E", false, "suppress the error log (default: written to ./chartverifier/verifier-<timestamp>.log)")
 	cmd.Flags().BoolVarP(&providerDelivery, "provider-delivery", "d", false, "chart provider will provide the chart delivery mechanism (default: false)")
diff --git a/docs/helm-chart-checks.md b/docs/helm-chart-checks.md
index e19e165..2b2fb72 100644
--- a/docs/helm-chart-checks.md
+++ b/docs/helm-chart-checks.md
@@ -27,7 +27,7 @@ Helm chart checks are a set of checks against which the Red Hat Helm chart-verif
 - The checks are configurable. For example, if a chart requires additional values to be compliant with the checks, configure the values using the available options. The options are similar to those used by the `helm lint` and `helm template` commands.
 - When there are no error messages, the `helm-lint` check passes the verification and is successful. Messages such as `Warning` and `info` do not cause the check to fail.
 - Profiles define the checks needed based on the chart type: partner, redhat or community.
-    - Profiles are versioned. Each new version may include updated checks, new checks, new annotations or chnaged annotations.
+    - Profiles are versioned. Each new version may include updated checks, new checks, new annotations or changed annotations.
 - The generated report is written to stdout but can optionally be written to a file.
 - An error log is created for all verify commands but can be optionally suppressed.
 - You can indicate that a chart is not to be published in the OpenShift catalogue 
@@ -142,6 +142,7 @@ This section provides help on the basic usage of Helm chart checks with the podm
     -s, --set strings                 overrides a configuration, e.g: dummy.ok=false
     -f, --set-values strings          specify application and check configuration values in a YAML file or a URL (can specify multiple)
     -E, --suppress-error-log          suppress the error log (default: written to ./chartverifier/verifier-<timestamp>.log)
+        --timeout duration            time to wait for completion of chart install and test (default 30m0s)
     -w, --write-to-file               write report to ./chartverifier/report.yaml (default: stdout)
   Global Flags:
         --config string   config file (default is $HOME/.chart-verifier.yaml)
@@ -189,6 +190,21 @@ This section provides help on the basic usage of Helm chart checks with the podm
           <chart-uri>
   ```
 
+### Timeout Option
+
+Increase the timeout value if chart-testing is going to take more time, default value is 30m.
+
+  ```
+  $ podman run --rm -i                                  \
+          -e KUBECONFIG=/.kube/config                   \
+          -v "${HOME}/.kube":/.kube                     \
+          -v $(pwd):/values                             \
+          "quay.io/redhat-certification/chart-verifier" \
+          verify --timeout 40m                          \
+          <chart-uri>
+  ```
+Note: In case chart-testing takes more time, it is advised to submit the report for certification since the certification process will use the default value of 30m. 
+
 ### Saving the report
 
 By default the report is written to stdout which can be redirected to a file. For example:
diff --git a/pkg/chartverifier/checks/registry.go b/pkg/chartverifier/checks/registry.go
index 198c5d7..89fb230 100644
--- a/pkg/chartverifier/checks/registry.go
+++ b/pkg/chartverifier/checks/registry.go
@@ -17,6 +17,8 @@
 package checks
 
 import (
+	"time"
+
 	"github.com/spf13/viper"
 	helmcli "helm.sh/helm/v3/pkg/cli"
 )
@@ -104,6 +106,8 @@ type CheckOptions struct {
 	HelmEnvSettings *helmcli.EnvSettings
 	// AnnotationHolder provides and API to set the OpenShift Version
 	AnnotationHolder AnnotationHolder
+	// client timeout
+	Timeout time.Duration
 }
 
 type CheckFunc func(options *CheckOptions) (Result, error)
diff --git a/pkg/chartverifier/helmverifier.go b/pkg/chartverifier/helmverifier.go
index 6d6e80b..37b3484 100644
--- a/pkg/chartverifier/helmverifier.go
+++ b/pkg/chartverifier/helmverifier.go
@@ -17,6 +17,8 @@
 package chartverifier
 
 import (
+	"time"
+
 	"github.com/redhat-certification/chart-verifier/pkg/chartverifier/checks"
 	"github.com/spf13/viper"
 	"helm.sh/helm/v3/pkg/cli"
@@ -32,6 +34,7 @@ type VerifierBuilder interface {
 	SetToolVersion(string) VerifierBuilder
 	SetOpenShiftVersion(string) VerifierBuilder
 	SetProviderDelivery(bool) VerifierBuilder
+	SetTimeout(time.Duration) VerifierBuilder
 	SetSettings(settings *cli.EnvSettings) VerifierBuilder
 	Build() (Verifier, error)
 }
diff --git a/pkg/chartverifier/verifier.go b/pkg/chartverifier/verifier.go
index 9023fa6..7a6c7ec 100644
--- a/pkg/chartverifier/verifier.go
+++ b/pkg/chartverifier/verifier.go
@@ -17,6 +17,8 @@
 package chartverifier
 
 import (
+	"time"
+
 	"github.com/redhat-certification/chart-verifier/pkg/chartverifier/checks"
 	"github.com/redhat-certification/chart-verifier/pkg/chartverifier/profiles"
 	"github.com/spf13/viper"
@@ -65,6 +67,7 @@ type verifier struct {
 	profile          *profiles.Profile
 	openshiftVersion string
 	providerDelivery bool
+	timeout          time.Duration
 	values           map[string]interface{}
 }
 
@@ -104,6 +107,7 @@ func (c *verifier) Verify(uri string) (*Report, error) {
 			Values:           c.values,
 			ViperConfig:      c.subConfig(string(check.CheckId.Name)),
 			AnnotationHolder: &holder,
+			Timeout:          c.timeout,
 		})
 
 		if checkErr != nil {
diff --git a/pkg/chartverifier/verifierbuilder.go b/pkg/chartverifier/verifierbuilder.go
index 50369c8..adc51a5 100644
--- a/pkg/chartverifier/verifierbuilder.go
+++ b/pkg/chartverifier/verifierbuilder.go
@@ -19,6 +19,7 @@ package chartverifier
 import (
 	"errors"
 	"strings"
+	"time"
 
 	"github.com/redhat-certification/chart-verifier/pkg/chartverifier/profiles"
 
@@ -63,6 +64,7 @@ type verifierBuilder struct {
 	openshiftVersion            string
 	suppportedOpenshiftVersions string
 	providerDelivery            bool
+	timeout                     time.Duration
 	values                      map[string]interface{}
 	settings                    *cli.EnvSettings
 }
@@ -117,6 +119,11 @@ func (b *verifierBuilder) SetProviderDelivery(providerDelivery bool) VerifierBui
 	return b
 }
 
+func (b *verifierBuilder) SetTimeout(timeout time.Duration) VerifierBuilder {
+	b.timeout = timeout
+	return b
+}
+
 func (b *verifierBuilder) GetConfig() *viper.Viper {
 	return b.config
 }
@@ -155,6 +162,7 @@ func (b *verifierBuilder) Build() (Verifier, error) {
 		profile:          profile,
 		openshiftVersion: b.openshiftVersion,
 		providerDelivery: b.providerDelivery,
+		timeout:          b.timeout,
 		values:           b.values,
 	}, nil
 }
diff --git a/pkg/tool/helm.go b/pkg/tool/helm.go
index 099ef86..2cb83b0 100644
--- a/pkg/tool/helm.go
+++ b/pkg/tool/helm.go
@@ -1,6 +1,8 @@
 package tool
 
 import (
+	"context"
+	"errors"
 	"fmt"
 	"os"
 	"time"
@@ -33,7 +35,7 @@ func NewHelm(envSettings *cli.EnvSettings, args map[string]interface{}) (*Helm,
 	return helm, nil
 }
 
-func (h Helm) Install(namespace, chart, release, valuesFile string) error {
+func (h Helm) Install(ctx context.Context, namespace, chart, release, valuesFile string) error {
 	utils.LogInfo(fmt.Sprintf("Execute helm install. namespace: %s, release: %s chart: %s", namespace, release, chart))
 	client := action.NewInstall(h.config)
 	client.Namespace = namespace
@@ -88,7 +90,7 @@ func (h Helm) Install(namespace, chart, release, valuesFile string) error {
 	}
 
 	// TODO: support other options if required
-	_, err = client.Run(c, vals)
+	_, err = client.RunWithContext(ctx, c, vals)
 	if err != nil {
 		utils.LogError(fmt.Sprintf("Error running chart install: %v", err))
 		return err
@@ -98,12 +100,16 @@ func (h Helm) Install(namespace, chart, release, valuesFile string) error {
 	return nil
 }
 
-func (h Helm) Test(namespace, release string) error {
+func (h Helm) Test(ctx context.Context, namespace, release string) error {
 	utils.LogInfo(fmt.Sprintf("Execute helm test. namespace: %s, release: %s, args: %+v", namespace, release, h.args))
+	deadline, _ := ctx.Deadline()
 	client := action.NewReleaseTesting(h.config)
 	client.Namespace = namespace
-	client.Timeout = 30 * time.Minute
+	client.Timeout = deadline.Sub(time.Now())
 
+	if client.Timeout <= 0 {
+		return errors.New("Helm test error : timeout has expired, please consider increasing the timeout using the chart-verifier timeout flag")
+	}
 	// TODO: support filter
 	_, err := client.Run(release)
 	if err != nil {
@@ -130,7 +136,7 @@ func (h Helm) Uninstall(namespace, release string) error {
 	return nil
 }
 
-func (h Helm) Upgrade(namespace, chart, release string) error {
+func (h Helm) Upgrade(ctx context.Context, namespace, chart, release string) error {
 	utils.LogInfo(fmt.Sprintf("Execute helm upgrade. namespace: %s, release: %s chart: %s", namespace, release, chart))
 	client := action.NewUpgrade(h.config)
 	client.Namespace = namespace
@@ -158,7 +164,7 @@ func (h Helm) Upgrade(namespace, chart, release string) error {
 	}
 
 	// TODO: support other options if required
-	_, err = client.Run(release, c, vals)
+	_, err = client.RunWithContext(ctx, release, c, vals)
 	if err != nil {
 		utils.LogError(fmt.Sprintf("Error running chart upgrade: %v", err))
 		return err


Test patch:
diff --git a/pkg/chartverifier/checks/charttesting.go b/pkg/chartverifier/checks/charttesting.go
index d423cda..cb3dfc4 100644
--- a/pkg/chartverifier/checks/charttesting.go
+++ b/pkg/chartverifier/checks/charttesting.go
@@ -106,6 +106,9 @@ func ChartTesting(opts *CheckOptions) (Result, error) {
 
 	utils.LogInfo("Start chart install and test check")
 
+	ctx, cancel := context.WithTimeout(context.Background(), opts.Timeout)
+	defer cancel()
+
 	cfg := buildChartTestingConfiguration(opts)
 	helm, err := tool.NewHelm(opts.HelmEnvSettings, opts.Values)
 	if err != nil {
@@ -157,14 +160,14 @@ func ChartTesting(opts *CheckOptions) (Result, error) {
 			utils.LogError(fmt.Sprintf("End chart install and test check with BreakingChangeAllowed error: %v", err))
 			return NewResult(false, err.Error()), nil
 		}
-		result := upgradeAndTestChart(cfg, oldChrt, chrt, helm, kubectl, configRelease)
+		result := upgradeAndTestChart(ctx, cfg, oldChrt, chrt, helm, kubectl, configRelease)
 
 		if result.Error != nil {
 			utils.LogError(fmt.Sprintf("End chart install and test check with upgradeAndTestChart error: %v", result.Error))
 			return NewResult(false, result.Error.Error()), nil
 		}
 	} else {
-		result := installAndTestChartRelease(cfg, chrt, helm, kubectl, opts.Values, configRelease)
+		result := installAndTestChartRelease(ctx, cfg, chrt, helm, kubectl, opts.Values, configRelease)
 		if result.Error != nil {
 			utils.LogError(fmt.Sprintf("End chart install and test check with installAndTestChartRelease error: %v", result.Error))
 			return NewResult(false, result.Error.Error()), nil
@@ -218,15 +221,16 @@ func generateInstallConfig(
 
 // testRelease tests a release.
 func testRelease(
+	ctx context.Context,
 	helm *tool.Helm,
 	kubectl *tool.Kubectl,
 	release, namespace, releaseSelector string,
 	cleanupHelmTests bool,
 ) error {
-	if err := kubectl.WaitForDeployments(context.TODO(), namespace, releaseSelector); err != nil {
+	if err := kubectl.WaitForDeployments(ctx, namespace, releaseSelector); err != nil {
 		return err
 	}
-	if err := helm.Test(namespace, release); err != nil {
+	if err := helm.Test(ctx, namespace, release); err != nil {
 		return err
 	}
 	return nil
@@ -243,6 +247,7 @@ func getChartPreviousVersion(chrt *chart.Chart) (*chart.Chart, error) {
 // upgradeAndTestChart performs the installation of the given oldChrt,
 // and attempts to perform an upgrade from that state.
 func upgradeAndTestChart(
+	ctx context.Context,
 	cfg config.Configuration,
 	oldChrt, chrt *chart.Chart,
 	helm *tool.Helm,
@@ -276,18 +281,18 @@ func upgradeAndTestChart(
 			defer cleanup()
 
 			// Install previous version of chart. If installation fails, ignore this release.
-			if err := helm.Install(namespace, oldChrt.Path(), release, valuesFile); err != nil {
+			if err := helm.Install(ctx, namespace, oldChrt.Path(), release, valuesFile); err != nil {
 				return fmt.Errorf("Upgrade testing for release '%s' skipped because of previous revision installation error: %w", release, err)
 			}
-			if err := testRelease(helm, kubectl, release, namespace, releaseSelector, true); err != nil {
+			if err := testRelease(ctx, helm, kubectl, release, namespace, releaseSelector, true); err != nil {
 				return fmt.Errorf("Upgrade testing for release '%s' skipped because of previous revision testing error", release)
 			}
 
-			if err := helm.Upgrade(namespace, oldChrt.Path(), release); err != nil {
+			if err := helm.Upgrade(ctx, namespace, oldChrt.Path(), release); err != nil {
 				return err
 			}
 
-			return testRelease(helm, kubectl, release, namespace, releaseSelector, false)
+			return testRelease(ctx, helm, kubectl, release, namespace, releaseSelector, false)
 		}
 
 		if err := fun(); err != nil {
@@ -379,6 +384,7 @@ func newTempValuesFileWithOverrides(filename string, valuesOverrides map[string]
 
 // installAndTestChartRelease installs and tests a chart release.
 func installAndTestChartRelease(
+	ctx context.Context,
 	cfg config.Configuration,
 	chrt *chart.Chart,
 	helm *tool.Helm,
@@ -416,10 +422,10 @@ func installAndTestChartRelease(
 			namespace, release, releaseSelector, releaseCleanup := generateInstallConfig(cfg, chrt, helm, kubectl, configRelease)
 			defer releaseCleanup()
 
-			if err := helm.Install(namespace, chrt.Path(), release, tmpValuesFile); err != nil {
+			if err := helm.Install(ctx, namespace, chrt.Path(), release, tmpValuesFile); err != nil {
 				return errors.New(fmt.Sprintf("Chart Install failure: %v", err))
 			}
-			if err = testRelease(helm, kubectl, release, namespace, releaseSelector, false); err != nil {
+			if err = testRelease(ctx, helm, kubectl, release, namespace, releaseSelector, false); err != nil {
 				return errors.New(fmt.Sprintf("Chart test failure: %v", err))
 			}
 			return nil
diff --git a/pkg/tool/helm_test.go b/pkg/tool/helm_test.go
index ffff9a0..c81ebd7 100644
--- a/pkg/tool/helm_test.go
+++ b/pkg/tool/helm_test.go
@@ -1,8 +1,10 @@
 package tool
 
 import (
+	"context"
 	"io/ioutil"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/require"
 	"helm.sh/helm/v3/pkg/action"
@@ -20,16 +22,19 @@ func TestInstall(t *testing.T) {
 		releaseName string
 		chartPath   string
 		expected    string
+		timeout     time.Duration
 	}{
 		{
 			releaseName: "valid chart",
 			chartPath:   "../chartverifier/checks/psql-service-0.1.7",
 			expected:    "",
+			timeout:     10 * time.Second,
 		},
 		{
 			releaseName: "invalid chart",
 			chartPath:   "../chartverifier/checks/psql-service-9.9.9",
 			expected:    "path \"../chartverifier/checks/psql-service-9.9.9\" not found",
+			timeout:     10 * time.Second,
 		},
 	}
 	for _, tt := range tests {
@@ -45,7 +50,13 @@ func TestInstall(t *testing.T) {
 				args:        map[string]interface{}{"set": "k8Project=default"},
 				envSettings: &cli.EnvSettings{},
 			}
-			err := helm.Install("default", tt.chartPath, tt.releaseName, "")
+
+			ctx, cancel := context.WithTimeout(context.Background(), tt.timeout)
+			defer cancel()
+
+			before_install_time := time.Now()
+			err := helm.Install(ctx, "default", tt.chartPath, tt.releaseName, "")
+			require.WithinDuration(t, before_install_time, time.Now(), tt.timeout)
 			if err == nil {
 				require.Equal(t, tt.expected, "")
 			} else {
@@ -126,6 +137,7 @@ func TestUpgrade(t *testing.T) {
 		chartPath string
 		release   *release.Release
 		expected  string
+		timeout   time.Duration
 	}{
 		{
 			name:      "successful release upgrade should not return error",
@@ -139,6 +151,7 @@ func TestUpgrade(t *testing.T) {
 				Chart:     &chart.Chart{Values: testValues},
 			},
 			expected: "",
+			timeout:  10 * time.Second,
 		},
 		{
 			name:      "upgrade non-existent release should result in error",
@@ -152,6 +165,7 @@ func TestUpgrade(t *testing.T) {
 				Chart:     &chart.Chart{Values: testValues},
 			},
 			expected: "\"test-release-invalid\" has no deployed releases",
+			timeout:  10 * time.Second,
 		},
 	}
 
@@ -175,7 +189,11 @@ func TestUpgrade(t *testing.T) {
 					t.Error(err)
 				}
 			}
-			err := helm.Upgrade("default", tt.chartPath, tt.release.Name)
+			ctx, cancel := context.WithTimeout(context.Background(), tt.timeout)
+			defer cancel()
+			before_upgrade_time := time.Now()
+			err := helm.Upgrade(ctx, "default", tt.chartPath, tt.release.Name)
+			require.WithinDuration(t, before_upgrade_time, time.Now(), tt.timeout)
 			if err == nil {
 				require.Equal(t, tt.expected, "")
 			} else {
@@ -206,6 +224,7 @@ func TestReleaseTesting(t *testing.T) {
 		chartPath string
 		release   *release.Release
 		expected  string
+		timeout   time.Duration
 	}{
 		{
 			name:      "successful release test should not return error",
@@ -219,6 +238,7 @@ func TestReleaseTesting(t *testing.T) {
 				Hooks:     testHooks,
 			},
 			expected: "",
+			timeout:  10 * time.Second,
 		},
 		{
 			name:      "release test on non-existent release should result in error",
@@ -232,6 +252,21 @@ func TestReleaseTesting(t *testing.T) {
 				Hooks:     testHooks,
 			},
 			expected: "release: not found",
+			timeout:  10 * time.Second,
+		},
+		{
+			name:      "release test with a zero or negative value should result in error",
+			chartPath: "../chartverifier/checks/psql-service-0.1.7",
+			release: &release.Release{
+				Name: "test-release-invalid-timeout",
+				Info: &release.Info{
+					Status: release.StatusDeployed,
+				},
+				Namespace: "default",
+				Hooks:     testHooks,
+			},
+			expected: "Helm test error : timeout has expired, please consider increasing the timeout using the chart-verifier timeout flag",
+			timeout:  -1 * time.Second,
 		},
 	}
 
@@ -255,7 +290,15 @@ func TestReleaseTesting(t *testing.T) {
 					t.Error(err)
 				}
 			}
-			err := helm.Test("default", tt.release.Name)
+			ctx, cancel := context.WithTimeout(context.Background(), tt.timeout)
+			defer cancel()
+			before_test_time := time.Now()
+			err := helm.Test(ctx, "default", tt.release.Name)
+			if tt.timeout <= 0 {
+				require.WithinDuration(t, before_test_time, time.Now(), 1*time.Second)
+			} else {
+				require.WithinDuration(t, before_test_time, time.Now(), tt.timeout)
+			}
 			if err == nil {
 				require.Equal(t, tt.expected, "")
 			} else {