Merge pull request #99 from varshavaradarajan/varsha/webhook-check-up…

…date

webhook-replacement: ensure that the webhook rules are applicable to v1, apps/*

checks.md

@@ -289,6 +289,7 @@ Admission control webhooks can disrupt upgrade and node replacement operations b
 * has failurePolicy set to Fail,
 * targets a service other than the Kubernetes apiserver, and
 * applies to both kube-system and the namespace of the targeted service.
+* has rules applicable to `v1`, `apps/v1`, `apps/v1beta1` or `apps/v1beta2` resources.
 
 ### Example
 
@@ -302,9 +303,9 @@ webhooks:
 - name: sample-webhook.example.com
   rules:
   - apiGroups:
-    - ""
+    - "*"
     apiVersions:
-    - v1
+    - "*"
     operations:
     - CREATE
     resources:
@@ -328,6 +329,8 @@ There are a few options:
 2. Use an apiserver extension as your webhook service.
 3. Explicitly exclude the kube-system namespace.
 4. Explicitly exclude the webhook service's namespace.
+5. Explicitly include the resource api group and version in the rules.
+If you have configured webhooks for CRDs, we recommend that you explicitly specify the rules instead of generally applying them to all resources.
 
 ```yaml
 # Recommended: Exclude objects in the `webhook` namespace by explicitly specifying a namespaceSelector.

checks/doks/admission_controller_webhook_replacement.go

@@ -57,6 +57,10 @@ func (w *webhookReplacementCheck) Run(objects *kube.Objects) ([]checks.Diagnosti
 		config := config
 		for _, wh := range config.Webhooks {
 			wh := wh
+			if !applicable(wh.Rules) {
+				// Webhooks that do not apply to core/v1, apps/v1, apps/v1beta1, apps/v1beta2 resources are fine
+				continue
+			}
 			if *wh.FailurePolicy == ar.Ignore {
 				// Webhooks with failurePolicy: Ignore are fine.
 				continue
@@ -109,6 +113,10 @@ func (w *webhookReplacementCheck) Run(objects *kube.Objects) ([]checks.Diagnosti
 		config := config
 		for _, wh := range config.Webhooks {
 			wh := wh
+			if !applicable(wh.Rules) {
+				// Webhooks that do not apply to core/v1, apps/v1, apps/v1beta1, apps/v1beta2 resources are fine
+				continue
+			}
 			if *wh.FailurePolicy == ar.Ignore {
 				// Webhooks with failurePolicy: Ignore are fine.
 				continue
@@ -159,6 +167,33 @@ func (w *webhookReplacementCheck) Run(objects *kube.Objects) ([]checks.Diagnosti
 	return diagnostics, nil
 }
 
+func applicable(rules []ar.RuleWithOperations) bool {
+	for _, r := range rules {
+		if apiVersions(r.APIVersions) {
+			// applies to "apiVersions: v1"
+			if len(r.APIGroups) == 0 {
+				return true
+			}
+			// applies to "apiVersion: v1", "apiVersion: apps/v1", "apiVersion: apps/v1beta1", "apiVersion: apps/v1beta2"
+			for _, g := range r.APIGroups {
+				if g == "" || g == "*" || g == "apps" {
+					return true
+				}
+			}
+		}
+	}
+	return false
+}
+
+func apiVersions(versions []string) bool {
+	for _, v := range versions {
+		if v == "*" || v == "v1" || v == "v1beta1" || v == "v1beta2" {
+			return true
+		}
+	}
+	return false
+}
+
 func selectorMatchesNamespace(selector *metav1.LabelSelector, namespace *corev1.Namespace) bool {
 	if selector.Size() == 0 {
 		return true

checks/doks/admission_controller_webhook_replacement_test.go

@@ -70,6 +70,8 @@ func TestWebhookError(t *testing.T) {
 					},
 				},
 				2,
+				[]string{"*"},
+				[]string{"*"},
 			),
 			expected: nil,
 		},
@@ -82,6 +84,8 @@ func TestWebhookError(t *testing.T) {
 					URL: &webhookURL,
 				},
 				2,
+				[]string{"*"},
+				[]string{"*"},
 			),
 			expected: nil,
 		},
@@ -97,6 +101,8 @@ func TestWebhookError(t *testing.T) {
 					},
 				},
 				2,
+				[]string{"*"},
+				[]string{"*"},
 			),
 			expected: nil,
 		},
@@ -114,6 +120,8 @@ func TestWebhookError(t *testing.T) {
 					},
 				},
 				2,
+				[]string{"*"},
+				[]string{"*"},
 			),
 			expected: nil,
 		},
@@ -131,6 +139,8 @@ func TestWebhookError(t *testing.T) {
 					},
 				},
 				2,
+				[]string{"*"},
+				[]string{"*"},
 			),
 			expected: nil,
 		},
@@ -148,6 +158,8 @@ func TestWebhookError(t *testing.T) {
 					},
 				},
 				2,
+				[]string{"*"},
+				[]string{"*"},
 			),
 			expected: nil,
 		},
@@ -165,6 +177,8 @@ func TestWebhookError(t *testing.T) {
 					},
 				},
 				2,
+				[]string{"*"},
+				[]string{"*"},
 			),
 			expected: nil,
 		},
@@ -182,6 +196,8 @@ func TestWebhookError(t *testing.T) {
 					},
 				},
 				2,
+				[]string{"*"},
+				[]string{"*"},
 			),
 			expected: nil,
 		},
@@ -199,6 +215,8 @@ func TestWebhookError(t *testing.T) {
 					},
 				},
 				2,
+				[]string{"*"},
+				[]string{"*"},
 			),
 			expected: nil,
 		},
@@ -216,6 +234,8 @@ func TestWebhookError(t *testing.T) {
 					},
 				},
 				1,
+				[]string{"*"},
+				[]string{"*"},
 			),
 			expected: webhookErrors(),
 		},
@@ -231,9 +251,113 @@ func TestWebhookError(t *testing.T) {
 					},
 				},
 				2,
+				[]string{"*"},
+				[]string{"*"},
 			),
 			expected: webhookErrors(),
 		},
+		{
+			name: "error: webhook applies to core/v1 group",
+			objs: webhookTestObjects(
+				ar.Fail,
+				&metav1.LabelSelector{},
+				ar.WebhookClientConfig{
+					Service: &ar.ServiceReference{
+						Namespace: "webhook",
+						Name:      "webhook-service",
+					},
+				},
+				2,
+				[]string{""},
+				[]string{"v1"},
+			),
+			expected: webhookErrors(),
+		},
+		{
+			name: "error: webhook applies to apps/v1 group",
+			objs: webhookTestObjects(
+				ar.Fail,
+				&metav1.LabelSelector{},
+				ar.WebhookClientConfig{
+					Service: &ar.ServiceReference{
+						Namespace: "webhook",
+						Name:      "webhook-service",
+					},
+				},
+				2,
+				[]string{"apps"},
+				[]string{"v1"},
+			),
+			expected: webhookErrors(),
+		},
+		{
+			name: "error: webhook applies to apps/v1beta1 group",
+			objs: webhookTestObjects(
+				ar.Fail,
+				&metav1.LabelSelector{},
+				ar.WebhookClientConfig{
+					Service: &ar.ServiceReference{
+						Namespace: "webhook",
+						Name:      "webhook-service",
+					},
+				},
+				2,
+				[]string{"apps"},
+				[]string{"v1beta1"},
+			),
+			expected: webhookErrors(),
+		},
+		{
+			name: "error: webhook applies to apps/v1beta2 group",
+			objs: webhookTestObjects(
+				ar.Fail,
+				&metav1.LabelSelector{},
+				ar.WebhookClientConfig{
+					Service: &ar.ServiceReference{
+						Namespace: "webhook",
+						Name:      "webhook-service",
+					},
+				},
+				2,
+				[]string{"apps"},
+				[]string{"v1beta2"},
+			),
+			expected: webhookErrors(),
+		},
+		{
+			name: "error: webhook applies to *",
+			objs: webhookTestObjects(
+				ar.Fail,
+				&metav1.LabelSelector{},
+				ar.WebhookClientConfig{
+					Service: &ar.ServiceReference{
+						Namespace: "webhook",
+						Name:      "webhook-service",
+					},
+				},
+				2,
+				[]string{"*"},
+				[]string{"*"},
+			),
+			expected: webhookErrors(),
+		},
+		{
+			name: "no error: webhook applies to batch/v1",
+			objs: webhookTestObjects(
+				ar.Fail,
+				&metav1.LabelSelector{},
+				ar.WebhookClientConfig{
+					Service: &ar.ServiceReference{
+						Namespace: "webhook",
+						Name:      "webhook-service",
+					},
+				},
+				2,
+				[]string{"batch"},
+				[]string{"*"},
+			),
+			expected: nil,
+		},
 	}
 
 	webhookCheck := webhookReplacementCheck{}
@@ -260,6 +384,8 @@ func webhookTestObjects(
 	nsSelector *metav1.LabelSelector,
 	clientConfig ar.WebhookClientConfig,
 	numNodes int,
+	groups []string,
+	versions []string,
 ) *kube.Objects {
 	objs := &kube.Objects{
 		SystemNamespace: &corev1.Namespace{
@@ -300,6 +426,14 @@ func webhookTestObjects(
 							FailurePolicy:     &failurePolicyType,
 							NamespaceSelector: nsSelector,
 							ClientConfig:      clientConfig,
+							Rules: []ar.RuleWithOperations{
+								{
+									Rule: ar.Rule{
+										APIGroups:   groups,
+										APIVersions: versions,
+									},
+								},
+							},
 						},
 					},
 				},
@@ -318,6 +452,14 @@ func webhookTestObjects(
 							FailurePolicy:     &failurePolicyType,
 							NamespaceSelector: nsSelector,
 							ClientConfig:      clientConfig,
+							Rules: []ar.RuleWithOperations{
+								{
+									Rule: ar.Rule{
+										APIGroups:   groups,
+										APIVersions: versions,
+									},
+								},
+							},
 						},
 					},
 				},
@@ -333,7 +475,7 @@ func webhookTestObjects(
 }
 
 func webhookErrors() []checks.Diagnostic {
-	objs := webhookTestObjects(ar.Fail, nil, ar.WebhookClientConfig{}, 0)
+	objs := webhookTestObjects(ar.Fail, nil, ar.WebhookClientConfig{}, 0, []string{"*"}, []string{"*"})
 	validatingConfig := objs.ValidatingWebhookConfigurations.Items[0]
 	mutatingConfig := objs.MutatingWebhookConfigurations.Items[0]