From 2edf737cb023f283df397a943f25591abbd95684 Mon Sep 17 00:00:00 2001 From: Varsha Varadarajan Date: Wed, 4 Nov 2020 13:42:21 -0800 Subject: [PATCH] webhook-replacement: ensure that the webhook rules are applicable to v1, apps/* --- checks.md | 7 +- ...dmission_controller_webhook_replacement.go | 35 +++++ ...ion_controller_webhook_replacement_test.go | 144 +++++++++++++++++- 3 files changed, 183 insertions(+), 3 deletions(-) diff --git a/checks.md b/checks.md index c2889fa8..283172ad 100644 --- a/checks.md +++ b/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. diff --git a/checks/doks/admission_controller_webhook_replacement.go b/checks/doks/admission_controller_webhook_replacement.go index fc939284..a64b8986 100644 --- a/checks/doks/admission_controller_webhook_replacement.go +++ b/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 diff --git a/checks/doks/admission_controller_webhook_replacement_test.go b/checks/doks/admission_controller_webhook_replacement_test.go index 6398371b..283f97b6 100644 --- a/checks/doks/admission_controller_webhook_replacement_test.go +++ b/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]