Merge pull request #88 from MorrisLaw/fix-nil-timeout-error

Update timeout check to account for nil TimeoutSeconds value
digitalocean · Jul 7, 2020 · 3555522 · 3555522
2 parents c571ab5 + 161c7a1
commit 3555522
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 22 deletions.
diff --git a/checks/doks/admission_controller_webhook_timeout.go b/checks/doks/admission_controller_webhook_timeout.go
@@ -49,35 +49,45 @@ func (w *webhookTimeoutCheck) Run(objects *kube.Objects) ([]checks.Diagnostic, e
 
 	for _, config := range objects.ValidatingWebhookConfigurations.Items {
 		for _, wh := range config.Webhooks {
-			if *wh.TimeoutSeconds >= int32(1) && *wh.TimeoutSeconds < int32(30) {
-				// Webhooks with TimeoutSeconds set: between 1 and 30 is fine.
+			if wh.TimeoutSeconds == nil {
+				// TimeoutSeconds value should be set to a non-nil value (greater than or equal to 1 and less than 30).
+				// If the TimeoutSeconds value is set to nil and the cluster version is 1.13.*, users are
+				// unable to configure the TimeoutSeconds value and this value will stay at nil, breaking
+				// upgrades. It's only for versions >= 1.14 that the value will default to 30 seconds.
 				continue
+			} else if *wh.TimeoutSeconds < int32(1) || *wh.TimeoutSeconds >= int32(30) {
+				// Webhooks with TimeoutSeconds set: less than 1 or greater than or equal to 30 is bad.
+				d := checks.Diagnostic{
+					Severity: checks.Error,
+					Message:  "Validating webhook with a TimeoutSeconds value greater than 29 seconds will block upgrades.",
+					Kind:     checks.ValidatingWebhookConfiguration,
+					Object:   &config.ObjectMeta,
+					Owners:   config.ObjectMeta.GetOwnerReferences(),
+				}
+				diagnostics = append(diagnostics, d)
 			}
-			d := checks.Diagnostic{
-				Severity: checks.Error,
-				Message:  "Validating webhook with a TimeoutSeconds value greater than 30 seconds will block upgrades.",
-				Kind:     checks.ValidatingWebhookConfiguration,
-				Object:   &config.ObjectMeta,
-				Owners:   config.ObjectMeta.GetOwnerReferences(),
-			}
-			diagnostics = append(diagnostics, d)
 		}
 	}
 
 	for _, config := range objects.MutatingWebhookConfigurations.Items {
 		for _, wh := range config.Webhooks {
-			if *wh.TimeoutSeconds >= int32(1) && *wh.TimeoutSeconds < int32(30) {
-				// Webhooks with TimeoutSeconds set: between 1 and 30 is fine.
+			if wh.TimeoutSeconds == nil {
+				// TimeoutSeconds value should be set to a non-nil value (greater than or equal to 1 and less than 30).
+				// If the TimeoutSeconds value is set to nil and the cluster version is 1.13.*, users are
+				// unable to configure the TimeoutSeconds value and this value will stay at nil, breaking
+				// upgrades. It's only for versions >= 1.14 that the value will default to 30 seconds.
 				continue
+			} else if *wh.TimeoutSeconds < int32(1) || *wh.TimeoutSeconds >= int32(30) {
+				// Webhooks with TimeoutSeconds set: less than 1 or greater than or equal to 30 is bad.
+				d := checks.Diagnostic{
+					Severity: checks.Error,
+					Message:  "Mutating webhook with a TimeoutSeconds value greater than 29 seconds will block upgrades.",
+					Kind:     checks.MutatingWebhookConfiguration,
+					Object:   &config.ObjectMeta,
+					Owners:   config.ObjectMeta.GetOwnerReferences(),
+				}
+				diagnostics = append(diagnostics, d)
 			}
-			d := checks.Diagnostic{
-				Severity: checks.Error,
-				Message:  "Mutating webhook with a TimeoutSeconds value greater than 30 seconds will block upgrades.",
-				Kind:     checks.MutatingWebhookConfiguration,
-				Object:   &config.ObjectMeta,
-				Owners:   config.ObjectMeta.GetOwnerReferences(),
-			}
-			diagnostics = append(diagnostics, d)
 		}
 	}
 	return diagnostics, nil

diff --git a/checks/doks/admission_controller_webhook_timeout_test.go b/checks/doks/admission_controller_webhook_timeout_test.go
@@ -111,6 +111,20 @@ func TestWebhookTimeoutError(t *testing.T) {
 			),
 			expected: webhookTimeoutErrors(),
 		},
+		{
+			name: "TimeoutSeconds value is set to nil",
+			objs: webhookTimeoutTestObjects(
+				ar.WebhookClientConfig{
+					Service: &ar.ServiceReference{
+						Namespace: "webhook",
+						Name:      "webhook-service",
+					},
+				},
+				nil,
+				2,
+			),
+			expected: nil,
+		},
 	}
 
 	webhookCheck := webhookTimeoutCheck{}
@@ -206,14 +220,14 @@ func webhookTimeoutErrors() []checks.Diagnostic {
 	diagnostics := []checks.Diagnostic{
 		{
 			Severity: checks.Error,
-			Message:  "Validating webhook with a TimeoutSeconds value greater than 30 seconds will block upgrades.",
+			Message:  "Validating webhook with a TimeoutSeconds value greater than 29 seconds will block upgrades.",
 			Kind:     checks.ValidatingWebhookConfiguration,
 			Object:   &validatingConfig.ObjectMeta,
 			Owners:   validatingConfig.ObjectMeta.GetOwnerReferences(),
 		},
 		{
 			Severity: checks.Error,
-			Message:  "Mutating webhook with a TimeoutSeconds value greater than 30 seconds will block upgrades.",
+			Message:  "Mutating webhook with a TimeoutSeconds value greater than 29 seconds will block upgrades.",
 			Kind:     checks.MutatingWebhookConfiguration,
 			Object:   &mutatingConfig.ObjectMeta,
 			Owners:   mutatingConfig.ObjectMeta.GetOwnerReferences(),