Set a request timeout on kubectl commands (#360)

smlx · web-flow · commit 048064130dba · 2022-07-26T17:16:31.000+02:00
* feat: add kubectl timeout value to configuration

Signed-off-by: Scott Leggett &lt;scott@sl.id.au&gt;

* fix: set a request timeout on kubectl commands

Simple kubectl commands such as get, describe, and logs should return
quickly, but kubectl does not set a timeout on requests by default.

This can mean kubectl hangs forever if the kubernetes API stops
responding during a request, which is quite possible in CI environments
where jobs may cancelled and kubernetes clusters torn down while ct is
running.

This change sets a configurable timeout on such kubectl commands, with a
default value of 30s.

Signed-off-by: Scott Leggett &lt;scott@sl.id.au&gt;

* chore: update integration test for new kubectl API

Signed-off-by: Scott Leggett &lt;scott@sl.id.au&gt;
diff --git a/pkg/chart/chart.go b/pkg/chart/chart.go
@@ -263,7 +263,7 @@ func NewTesting(config config.Configuration, extraSetArgs string) (Testing, erro
 		config:           config,
 		helm:             tool.NewHelm(procExec, extraArgs, strings.Fields(extraSetArgs)),
 		git:              tool.NewGit(procExec),
-		kubectl:          tool.NewKubectl(procExec),
+		kubectl:          tool.NewKubectl(procExec, config.KubectlTimeout),
 		linter:           tool.NewLinter(procExec),
 		cmdExecutor:      tool.NewCmdTemplateExecutor(procExec),
 		accountValidator: tool.AccountValidator{},
diff --git a/pkg/chart/integration_test.go b/pkg/chart/integration_test.go
@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/helm/chart-testing/v3/pkg/config"
 	"github.com/helm/chart-testing/v3/pkg/exec"
@@ -41,7 +42,7 @@ func newTestingHelmIntegration(cfg config.Configuration, extraSetArgs string) Te
 		accountValidator: fakeAccountValidator{},
 		linter:           fakeMockLinter,
 		helm:             tool.NewHelm(procExec, extraArgs, strings.Fields(extraSetArgs)),
-		kubectl:          tool.NewKubectl(procExec),
+		kubectl:          tool.NewKubectl(procExec, 30*time.Second),
 	}
 }
 
diff --git a/pkg/config/config.go b/pkg/config/config.go
@@ -20,6 +20,7 @@ import (
 	"path/filepath"
 	"reflect"
 	"strings"
+	"time"
 
 	"github.com/mitchellh/go-homedir"
 
@@ -41,36 +42,39 @@ var (
 )
 
 type Configuration struct {
-	Remote                  string   `mapstructure:"remote"`
-	TargetBranch            string   `mapstructure:"target-branch"`
-	Since                   string   `mapstructure:"since"`
-	BuildId                 string   `mapstructure:"build-id"`
-	LintConf                string   `mapstructure:"lint-conf"`
-	ChartYamlSchema         string   `mapstructure:"chart-yaml-schema"`
-	ValidateMaintainers     bool     `mapstructure:"validate-maintainers"`
-	ValidateChartSchema     bool     `mapstructure:"validate-chart-schema"`
-	ValidateYaml            bool     `mapstructure:"validate-yaml"`
-	AdditionalCommands      []string `mapstructure:"additional-commands"`
-	CheckVersionIncrement   bool     `mapstructure:"check-version-increment"`
-	ProcessAllCharts        bool     `mapstructure:"all"`
-	Charts                  []string `mapstructure:"charts"`
-	ChartRepos              []string `mapstructure:"chart-repos"`
-	ChartDirs               []string `mapstructure:"chart-dirs"`
-	ExcludedCharts          []string `mapstructure:"excluded-charts"`
-	HelmExtraArgs           string   `mapstructure:"helm-extra-args"`
-	HelmRepoExtraArgs       []string `mapstructure:"helm-repo-extra-args"`
-	HelmDependencyExtraArgs []string `mapstructure:"helm-dependency-extra-args"`
-	Debug                   bool     `mapstructure:"debug"`
-	Upgrade                 bool     `mapstructure:"upgrade"`
-	SkipMissingValues       bool     `mapstructure:"skip-missing-values"`
-	Namespace               string   `mapstructure:"namespace"`
-	ReleaseLabel            string   `mapstructure:"release-label"`
-	ExcludeDeprecated       bool     `mapstructure:"exclude-deprecated"`
+	Remote                  string        `mapstructure:"remote"`
+	TargetBranch            string        `mapstructure:"target-branch"`
+	Since                   string        `mapstructure:"since"`
+	BuildId                 string        `mapstructure:"build-id"`
+	LintConf                string        `mapstructure:"lint-conf"`
+	ChartYamlSchema         string        `mapstructure:"chart-yaml-schema"`
+	ValidateMaintainers     bool          `mapstructure:"validate-maintainers"`
+	ValidateChartSchema     bool          `mapstructure:"validate-chart-schema"`
+	ValidateYaml            bool          `mapstructure:"validate-yaml"`
+	AdditionalCommands      []string      `mapstructure:"additional-commands"`
+	CheckVersionIncrement   bool          `mapstructure:"check-version-increment"`
+	ProcessAllCharts        bool          `mapstructure:"all"`
+	Charts                  []string      `mapstructure:"charts"`
+	ChartRepos              []string      `mapstructure:"chart-repos"`
+	ChartDirs               []string      `mapstructure:"chart-dirs"`
+	ExcludedCharts          []string      `mapstructure:"excluded-charts"`
+	HelmExtraArgs           string        `mapstructure:"helm-extra-args"`
+	HelmRepoExtraArgs       []string      `mapstructure:"helm-repo-extra-args"`
+	HelmDependencyExtraArgs []string      `mapstructure:"helm-dependency-extra-args"`
+	Debug                   bool          `mapstructure:"debug"`
+	Upgrade                 bool          `mapstructure:"upgrade"`
+	SkipMissingValues       bool          `mapstructure:"skip-missing-values"`
+	Namespace               string        `mapstructure:"namespace"`
+	ReleaseLabel            string        `mapstructure:"release-label"`
+	ExcludeDeprecated       bool          `mapstructure:"exclude-deprecated"`
+	KubectlTimeout          time.Duration `mapstructure:"kubectl-timeout"`
 }
 
 func LoadConfiguration(cfgFile string, cmd *cobra.Command, printConfig bool) (*Configuration, error) {
 	v := viper.New()
 
+	v.SetDefault("kubectl-timeout", time.Duration(30*time.Second))
+
 	cmd.Flags().VisitAll(func(flag *flag.Flag) {
 		flagName := flag.Name
 		if flagName != "config" && flagName != "help" {
diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go
@@ -18,6 +18,7 @@ import (
 	"os"
 	"path/filepath"
 	"testing"
+	"time"
 
 	"github.com/spf13/cobra"
 	"github.com/stretchr/testify/assert"
@@ -57,6 +58,7 @@ func loadAndAssertConfigFromFile(t *testing.T, configFile string) {
 	require.Equal(t, "default", cfg.Namespace)
 	require.Equal(t, "release", cfg.ReleaseLabel)
 	require.Equal(t, true, cfg.ExcludeDeprecated)
+	require.Equal(t, 120*time.Second, cfg.KubectlTimeout)
 }
 
 func Test_findConfigFile(t *testing.T) {
diff --git a/pkg/config/test_config.json b/pkg/config/test_config.json
@@ -29,5 +29,6 @@
     "skip-missing-values": true,
     "namespace": "default",
     "release-label": "release",
-    "exclude-deprecated": true
+    "exclude-deprecated": true,
+    "kubectl-timeout": "120s"
 }
diff --git a/pkg/config/test_config.yaml b/pkg/config/test_config.yaml
@@ -25,3 +25,4 @@ skip-missing-values: true
 namespace: default
 release-label: release
 exclude-deprecated: true
+kubectl-timeout: 120s
diff --git a/pkg/tool/kubectl.go b/pkg/tool/kubectl.go
@@ -14,35 +14,46 @@ import (
 )
 
 type Kubectl struct {
-	exec exec.ProcessExecutor
+	exec    exec.ProcessExecutor
+	timeout time.Duration
 }
 
-func NewKubectl(exec exec.ProcessExecutor) Kubectl {
+func NewKubectl(exec exec.ProcessExecutor, timeout time.Duration) Kubectl {
 	return Kubectl{
-		exec: exec,
+		exec:    exec,
+		timeout: timeout,
 	}
 }
 
 // CreateNamespace creates a new namespace with the given name.
 func (k Kubectl) CreateNamespace(namespace string) error {
 	fmt.Printf("Creating namespace '%s'...\n", namespace)
-	return k.exec.RunProcess("kubectl", "create", "namespace", namespace)
+	return k.exec.RunProcess("kubectl",
+		fmt.Sprintf("--request-timeout=%s", k.timeout),
+		"create", "namespace", namespace)
 }
 
 // DeleteNamespace deletes the specified namespace. If the namespace does not terminate within 120s, pods running in the
 // namespace and, eventually, the namespace itself are force-deleted.
 func (k Kubectl) DeleteNamespace(namespace string) {
 	fmt.Printf("Deleting namespace '%s'...\n", namespace)
 	timeoutSec := "180s"
-	if err := k.exec.RunProcess("kubectl", "delete", "namespace", namespace, "--timeout", timeoutSec); err != nil {
+	err := k.exec.RunProcess("kubectl",
+		fmt.Sprintf("--request-timeout=%s", k.timeout),
+		"delete", "namespace", namespace, "--timeout", timeoutSec)
+	if err != nil {
 		fmt.Printf("Namespace '%s' did not terminate after %s.\n", namespace, timeoutSec)
 	}
 
 	if k.getNamespace(namespace) {
 		fmt.Printf("Namespace '%s' did not terminate after %s.\n", namespace, timeoutSec)
 
 		fmt.Println("Force-deleting everything...")
-		if err := k.exec.RunProcess("kubectl", "delete", "all", "--namespace", namespace, "--all", "--force", "--grace-period=0"); err != nil {
+		err = k.exec.RunProcess("kubectl",
+			fmt.Sprintf("--request-timeout=%s", k.timeout),
+			"delete", "all", "--namespace", namespace, "--all", "--force",
+			"--grace-period=0")
+		if err != nil {
 			fmt.Printf("Error deleting everything in the namespace %v: %v", namespace, err)
 		}
 
@@ -59,7 +70,9 @@ func (k Kubectl) DeleteNamespace(namespace string) {
 
 func (k Kubectl) forceNamespaceDeletion(namespace string) error {
 	// Getting the namespace json to remove the finalizer
-	cmdOutput, err := k.exec.RunProcessAndCaptureOutput("kubectl", "get", "namespace", namespace, "--output=json")
+	cmdOutput, err := k.exec.RunProcessAndCaptureOutput("kubectl",
+		fmt.Sprintf("--request-timeout=%s", k.timeout),
+		"get", "namespace", namespace, "--output=json")
 	if err != nil {
 		fmt.Println("Error getting namespace json:", err)
 		return err
@@ -111,13 +124,20 @@ func (k Kubectl) forceNamespaceDeletion(namespace string) error {
 	time.Sleep(5 * time.Second)
 
 	// Check again
-	if _, err := k.exec.RunProcessAndCaptureOutput("kubectl", "get", "namespace", namespace); err != nil {
+	_, err = k.exec.RunProcessAndCaptureOutput("kubectl",
+		fmt.Sprintf("--request-timeout=%s", k.timeout),
+		"get", "namespace", namespace)
+	if err != nil {
 		fmt.Printf("Namespace '%s' terminated.\n", namespace)
 		return nil
 	}
 
 	fmt.Printf("Force-deleting namespace '%s'...\n", namespace)
-	if err := k.exec.RunProcess("kubectl", "delete", "namespace", namespace, "--force", "--grace-period=0", "--ignore-not-found=true"); err != nil {
+	err = k.exec.RunProcess("kubectl",
+		fmt.Sprintf("--request-timeout=%s", k.timeout),
+		"delete", "namespace", namespace, "--force", "--grace-period=0",
+		"--ignore-not-found=true")
+	if err != nil {
 		fmt.Println("Error deleting namespace:", err)
 		return err
 	}
@@ -126,16 +146,20 @@ func (k Kubectl) forceNamespaceDeletion(namespace string) error {
 }
 
 func (k Kubectl) WaitForDeployments(namespace string, selector string) error {
-	output, err := k.exec.RunProcessAndCaptureOutput(
-		"kubectl", "get", "deployments", "--namespace", namespace, "--selector", selector, "--output", "jsonpath={.items[*].metadata.name}")
+	output, err := k.exec.RunProcessAndCaptureOutput("kubectl",
+		fmt.Sprintf("--request-timeout=%s", k.timeout),
+		"get", "deployments", "--namespace", namespace, "--selector", selector,
+		"--output", "jsonpath={.items[*].metadata.name}")
 	if err != nil {
 		return err
 	}
 
 	deployments := strings.Fields(output)
 	for _, deployment := range deployments {
 		deployment = strings.Trim(deployment, "'")
-		err := k.exec.RunProcess("kubectl", "rollout", "status", "deployment", deployment, "--namespace", namespace)
+		err = k.exec.RunProcess("kubectl",
+			fmt.Sprintf("--request-timeout=%s", k.timeout),
+			"rollout", "status", "deployment", deployment, "--namespace", namespace)
 		if err != nil {
 			return err
 		}
@@ -145,7 +169,9 @@ func (k Kubectl) WaitForDeployments(namespace string, selector string) error {
 		//
 		// Just after rollout, pods from the previous deployment revision may still be in a
 		// terminating state.
-		unavailable, err := k.exec.RunProcessAndCaptureOutput("kubectl", "get", "deployment", deployment, "--namespace", namespace, "--output",
+		unavailable, err := k.exec.RunProcessAndCaptureOutput("kubectl",
+			fmt.Sprintf("--request-timeout=%s", k.timeout),
+			"get", "deployment", deployment, "--namespace", namespace, "--output",
 			`jsonpath={.status.unavailableReplicas}`)
 		if err != nil {
 			return err
@@ -159,7 +185,9 @@ func (k Kubectl) WaitForDeployments(namespace string, selector string) error {
 }
 
 func (k Kubectl) GetPodsforDeployment(namespace string, deployment string) ([]string, error) {
-	jsonString, _ := k.exec.RunProcessAndCaptureOutput("kubectl", "get", "deployment", deployment, "--namespace", namespace, "--output=json")
+	jsonString, _ := k.exec.RunProcessAndCaptureOutput("kubectl",
+		fmt.Sprintf("--request-timeout=%s", k.timeout),
+		"get", "deployment", deployment, "--namespace", namespace, "--output=json")
 	var deploymentMap map[string]interface{}
 	err := json.Unmarshal([]byte(jsonString), &deploymentMap)
 	if err != nil {
@@ -183,23 +211,30 @@ func (k Kubectl) GetPodsforDeployment(namespace string, deployment string) ([]st
 func (k Kubectl) GetPods(args ...string) ([]string, error) {
 	kubectlArgs := []string{"get", "pods"}
 	kubectlArgs = append(kubectlArgs, args...)
-	pods, err := k.exec.RunProcessAndCaptureOutput("kubectl", kubectlArgs)
+	pods, err := k.exec.RunProcessAndCaptureOutput("kubectl",
+		fmt.Sprintf("--request-timeout=%s", k.timeout), kubectlArgs)
 	if err != nil {
 		return nil, err
 	}
 	return strings.Fields(pods), nil
 }
 
 func (k Kubectl) GetEvents(namespace string) error {
-	return k.exec.RunProcess("kubectl", "get", "events", "--output", "wide", "--namespace", namespace)
+	return k.exec.RunProcess("kubectl",
+		fmt.Sprintf("--request-timeout=%s", k.timeout),
+		"get", "events", "--output", "wide", "--namespace", namespace)
 }
 
 func (k Kubectl) DescribePod(namespace string, pod string) error {
-	return k.exec.RunProcess("kubectl", "describe", "pod", pod, "--namespace", namespace)
+	return k.exec.RunProcess("kubectl",
+		fmt.Sprintf("--request-timeout=%s", k.timeout),
+		"describe", "pod", pod, "--namespace", namespace)
 }
 
 func (k Kubectl) Logs(namespace string, pod string, container string) error {
-	return k.exec.RunProcess("kubectl", "logs", pod, "--namespace", namespace, "--container", container)
+	return k.exec.RunProcess("kubectl",
+		fmt.Sprintf("--request-timeout=%s", k.timeout),
+		"logs", pod, "--namespace", namespace, "--container", container)
 }
 
 func (k Kubectl) GetInitContainers(namespace string, pod string) ([]string, error) {
@@ -211,7 +246,10 @@ func (k Kubectl) GetContainers(namespace string, pod string) ([]string, error) {
 }
 
 func (k Kubectl) getNamespace(namespace string) bool {
-	if _, err := k.exec.RunProcessAndCaptureOutput("kubectl", "get", "namespace", namespace); err != nil {
+	_, err := k.exec.RunProcessAndCaptureOutput("kubectl",
+		fmt.Sprintf("--request-timeout=%s", k.timeout),
+		"get", "namespace", namespace)
+	if err != nil {
 		fmt.Printf("Namespace '%s' terminated.\n", namespace)
 		return false
 	}

Original file line number	Diff line number	Diff line change
`@@ -21,6 +21,7 @@ import (`
`21`	`21`	`"fmt"`
`22`	`22`	`"strings"`
`23`	`23`	`"testing"`
	`24`	`+ "time"`
`24`	`25`
`25`	`26`	`"github.com/helm/chart-testing/v3/pkg/config"`
`26`	`27`	`"github.com/helm/chart-testing/v3/pkg/exec"`
`@@ -41,7 +42,7 @@ func newTestingHelmIntegration(cfg config.Configuration, extraSetArgs string) Te`
`41`	`42`	`accountValidator: fakeAccountValidator{},`
`42`	`43`	`linter: fakeMockLinter,`
`43`	`44`	`helm: tool.NewHelm(procExec, extraArgs, strings.Fields(extraSetArgs)),`
`44`		`- kubectl: tool.NewKubectl(procExec),`
	`45`	`+ kubectl: tool.NewKubectl(procExec, 30*time.Second),`
`45`	`46`	`}`
`46`	`47`	`}`
`47`	`48`
Original file line number	Diff line number	Diff line change
`@@ -29,5 +29,6 @@`
`29`	`29`	`"skip-missing-values": true,`
`30`	`30`	`"namespace": "default",`
`31`	`31`	`"release-label": "release",`
`32`		`- "exclude-deprecated": true`
	`32`	`+ "exclude-deprecated": true,`
	`33`	`+ "kubectl-timeout": "120s"`
`33`	`34`	`}`