diff --git a/checks.md b/checks.md index 186ae539..2143e526 100644 --- a/checks.md +++ b/checks.md @@ -207,6 +207,82 @@ spec: ## Admission Controller Webhook - Name: `admission-controller-webhook` +- Groups: `basic` + +Admission control webhooks can disrupt normal cluster operations. Specifically, this happens when an admission control webhook: +* targets a service that does not exist, +* targets a service in a namespace that does not exist. + +### Example + +```yaml +# Error: Configure a webhook pointing at a service that does not exist +apiVersion: admissionregistration.k8s.io/v1beta1 +kind: ValidatingWebhookConfiguration +metadata: + name: sample-webhook.example.com +webhooks: +- name: sample-webhook.example.com + rules: + - apiGroups: + - "" + apiVersions: + - v1 + operations: + - CREATE + resources: + - pods + scope: "Namespaced" + clientConfig: + service: + namespace: webhook + name: missing-webhook-server + path: /pods + admissionReviewVersions: + - v1beta1 + timeoutSeconds: 1 + failurePolicy: Fail +``` + +### How to Fix + +Point the webhook at the correct service. + +```yaml +apiVersion: admissionregistration.k8s.io/v1beta1 +kind: ValidatingWebhookConfiguration +metadata: + name: sample-webhook.example.com +webhooks: +- name: sample-webhook.example.com + rules: + - apiGroups: + - "" + apiVersions: + - v1 + operations: + - CREATE + resources: + - pods + scope: "Namespaced" + clientConfig: + service: + namespace: webhook + name: webhook-server + path: /pods + admissionReviewVersions: + - v1beta1 + timeoutSeconds: 1 + failurePolicy: Fail + namespaceSelector: + matchExpressions: + - key: "skip-webhooks" + operator: "DoesNotExist" +``` + +## Admission Controller Webhook Replacement + +- Name: `admission-controller-webhook-replacement` - Groups: `doks` Admission control webhooks can disrupt upgrade and node replacement operations by preventing system components from starting. Specifically, this happens when an admission control webhook: diff --git a/checks/basic/admission_controller_webhook.go b/checks/basic/admission_controller_webhook.go new file mode 100644 index 00000000..26e402e3 --- /dev/null +++ b/checks/basic/admission_controller_webhook.go @@ -0,0 +1,131 @@ +/* +Copyright 2019 DigitalOcean + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package basic + +import ( + "fmt" + + "github.com/digitalocean/clusterlint/checks" + "github.com/digitalocean/clusterlint/kube" + v1 "k8s.io/api/core/v1" +) + +func init() { + checks.Register(&webhookCheck{}) +} + +type webhookCheck struct{} + +// Name returns a unique name for this check. +func (w *webhookCheck) Name() string { + return "admission-controller-webhook" +} + +// Groups returns a list of group names this check should be part of. +func (w *webhookCheck) Groups() []string { + return []string{"basic"} +} + +// Description returns a detailed human-readable description of what this check +// does. +func (w *webhookCheck) Description() string { + return "Check for admission control webhooks" +} + +// Run runs this check on a set of Kubernetes objects. +func (w *webhookCheck) Run(objects *kube.Objects) ([]checks.Diagnostic, error) { + const apiserverServiceName = "kubernetes" + + var diagnostics []checks.Diagnostic + + for _, config := range objects.ValidatingWebhookConfigurations.Items { + for _, wh := range config.Webhooks { + if wh.ClientConfig.Service != nil { + // Ensure that the service (and its namespace) that is configure actually exists. + + if !namespaceExists(objects.Namespaces, wh.ClientConfig.Service.Namespace) { + diagnostics = append(diagnostics, checks.Diagnostic{ + Severity: checks.Error, + Message: fmt.Sprintf("Validating webhook %s is configured against a service in a namespace that does not exist.", wh.Name), + Kind: checks.ValidatingWebhookConfiguration, + Object: &config.ObjectMeta, + Owners: config.ObjectMeta.GetOwnerReferences(), + }) + continue + } + + if !serviceExists(objects.Services, wh.ClientConfig.Service.Name, wh.ClientConfig.Service.Namespace) { + diagnostics = append(diagnostics, checks.Diagnostic{ + Severity: checks.Error, + Message: fmt.Sprintf("Validating webhook %s is configured against a service that does not exist.", wh.Name), + Kind: checks.ValidatingWebhookConfiguration, + Object: &config.ObjectMeta, + Owners: config.ObjectMeta.GetOwnerReferences(), + }) + } + } + } + } + + for _, config := range objects.MutatingWebhookConfigurations.Items { + for _, wh := range config.Webhooks { + if wh.ClientConfig.Service != nil { + // Ensure that the service (and its namespace) that is configure actually exists. + + if !namespaceExists(objects.Namespaces, wh.ClientConfig.Service.Namespace) { + diagnostics = append(diagnostics, checks.Diagnostic{ + Severity: checks.Error, + Message: fmt.Sprintf("Mutating webhook %s is configured against a service in a namespace that does not exist.", wh.Name), + Kind: checks.MutatingWebhookConfiguration, + Object: &config.ObjectMeta, + Owners: config.ObjectMeta.GetOwnerReferences(), + }) + continue + } + + if !serviceExists(objects.Services, wh.ClientConfig.Service.Name, wh.ClientConfig.Service.Namespace) { + diagnostics = append(diagnostics, checks.Diagnostic{ + Severity: checks.Error, + Message: fmt.Sprintf("Mutating webhook %s is configured against a service that does not exist.", wh.Name), + Kind: checks.MutatingWebhookConfiguration, + Object: &config.ObjectMeta, + Owners: config.ObjectMeta.GetOwnerReferences(), + }) + } + } + } + } + return diagnostics, nil +} + +func namespaceExists(namespaceList *v1.NamespaceList, namespace string) bool { + for _, ns := range namespaceList.Items { + if ns.Name == namespace { + return true + } + } + return false +} + +func serviceExists(serviceList *v1.ServiceList, service, namespace string) bool { + for _, svc := range serviceList.Items { + if svc.Name == service && svc.Namespace == namespace { + return true + } + } + return false +} diff --git a/checks/basic/admission_controller_webhook_test.go b/checks/basic/admission_controller_webhook_test.go new file mode 100644 index 00000000..53bf1274 --- /dev/null +++ b/checks/basic/admission_controller_webhook_test.go @@ -0,0 +1,266 @@ +/* +Copyright 2019 DigitalOcean + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package basic + +import ( + "testing" + + "github.com/digitalocean/clusterlint/checks" + "github.com/digitalocean/clusterlint/kube" + "github.com/stretchr/testify/assert" + ar "k8s.io/api/admissionregistration/v1beta1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestWebhookCheckMeta(t *testing.T) { + webhookCheck := webhookCheck{} + assert.Equal(t, "admission-controller-webhook", webhookCheck.Name()) + assert.Equal(t, []string{"basic"}, webhookCheck.Groups()) + assert.NotEmpty(t, webhookCheck.Description()) +} + +func TestWebhookCheckRegistration(t *testing.T) { + webhookCheck := &webhookCheck{} + check, err := checks.Get("admission-controller-webhook") + assert.NoError(t, err) + assert.Equal(t, check, webhookCheck) +} + +func TestWebHookRun(t *testing.T) { + emptyNamespaceList := &corev1.NamespaceList{ + Items: []corev1.Namespace{}, + } + emptyServiceList := &corev1.ServiceList{ + Items: []corev1.Service{}, + } + + baseMWC := ar.MutatingWebhookConfiguration{ + TypeMeta: metav1.TypeMeta{Kind: "MutatingWebhookConfiguration", APIVersion: "v1beta1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "mwc_foo", + }, + Webhooks: []ar.MutatingWebhook{}, + } + baseMW := ar.MutatingWebhook{ + Name: "mw_foo", + } + + baseVWC := ar.ValidatingWebhookConfiguration{ + TypeMeta: metav1.TypeMeta{Kind: "ValidatingWebhookConfiguration", APIVersion: "v1beta1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "vwc_foo", + }, + Webhooks: []ar.ValidatingWebhook{}, + } + baseVW := ar.ValidatingWebhook{ + Name: "vw_foo", + } + + tests := []struct { + name string + objs *kube.Objects + expected []checks.Diagnostic + }{ + { + name: "no webhook configuration", + objs: &kube.Objects{ + MutatingWebhookConfigurations: &ar.MutatingWebhookConfigurationList{}, + ValidatingWebhookConfigurations: &ar.ValidatingWebhookConfigurationList{}, + SystemNamespace: &corev1.Namespace{}, + }, + expected: nil, + }, + { + name: "direct url webhooks", + objs: &kube.Objects{ + Namespaces: emptyNamespaceList, + MutatingWebhookConfigurations: &ar.MutatingWebhookConfigurationList{ + Items: []ar.MutatingWebhookConfiguration{ + func() ar.MutatingWebhookConfiguration { + mwc := baseMWC + mw := baseMW + mw.ClientConfig = ar.WebhookClientConfig{ + URL: strPtr("http://webhook.com"), + } + mwc.Webhooks = append(mwc.Webhooks, mw) + return mwc + }(), + }, + }, + ValidatingWebhookConfigurations: &ar.ValidatingWebhookConfigurationList{ + Items: []ar.ValidatingWebhookConfiguration{ + func() ar.ValidatingWebhookConfiguration { + vwc := baseVWC + vw := baseVW + vw.ClientConfig = ar.WebhookClientConfig{ + URL: strPtr("http://webhook.com"), + } + vwc.Webhooks = append(vwc.Webhooks, vw) + return vwc + }(), + }, + }, + SystemNamespace: &corev1.Namespace{}, + }, + expected: nil, + }, + { + name: "namespace does not exist", + objs: &kube.Objects{ + Namespaces: emptyNamespaceList, + Services: &corev1.ServiceList{ + Items: []corev1.Service{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "service", + }, + }, + }, + }, + MutatingWebhookConfigurations: &ar.MutatingWebhookConfigurationList{ + Items: []ar.MutatingWebhookConfiguration{ + func() ar.MutatingWebhookConfiguration { + mwc := baseMWC + mw := baseMW + mw.ClientConfig = ar.WebhookClientConfig{ + Service: &ar.ServiceReference{ + Namespace: "missing", + Name: "service", + }, + } + mwc.Webhooks = append(mwc.Webhooks, mw) + return mwc + }(), + }, + }, + ValidatingWebhookConfigurations: &ar.ValidatingWebhookConfigurationList{ + Items: []ar.ValidatingWebhookConfiguration{ + func() ar.ValidatingWebhookConfiguration { + vwc := baseVWC + vw := baseVW + vw.ClientConfig = ar.WebhookClientConfig{ + Service: &ar.ServiceReference{ + Namespace: "missing", + Name: "service", + }, + } + vwc.Webhooks = append(vwc.Webhooks, vw) + return vwc + }(), + }, + }, + SystemNamespace: &corev1.Namespace{}, + }, + expected: []checks.Diagnostic{ + { + Severity: checks.Error, + Message: "Validating webhook vw_foo is configured against a service in a namespace that does not exist.", + Kind: checks.ValidatingWebhookConfiguration, + }, + { + Severity: checks.Error, + Message: "Mutating webhook mw_foo is configured against a service in a namespace that does not exist.", + Kind: checks.MutatingWebhookConfiguration, + }, + }, + }, + { + name: "service does not exist", + objs: &kube.Objects{ + Namespaces: &corev1.NamespaceList{ + Items: []corev1.Namespace{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "webhook", + }, + }, + }, + }, + Services: emptyServiceList, + MutatingWebhookConfigurations: &ar.MutatingWebhookConfigurationList{ + Items: []ar.MutatingWebhookConfiguration{ + func() ar.MutatingWebhookConfiguration { + mwc := baseMWC + mw := baseMW + mw.ClientConfig = ar.WebhookClientConfig{ + Service: &ar.ServiceReference{ + Namespace: "webhook", + Name: "service", + }, + } + mwc.Webhooks = append(mwc.Webhooks, mw) + return mwc + }(), + }, + }, + ValidatingWebhookConfigurations: &ar.ValidatingWebhookConfigurationList{ + Items: []ar.ValidatingWebhookConfiguration{ + func() ar.ValidatingWebhookConfiguration { + vwc := baseVWC + vw := baseVW + vw.ClientConfig = ar.WebhookClientConfig{ + Service: &ar.ServiceReference{ + Namespace: "webhook", + Name: "service", + }, + } + vwc.Webhooks = append(vwc.Webhooks, vw) + return vwc + }(), + }, + }, + SystemNamespace: &corev1.Namespace{}, + }, + expected: []checks.Diagnostic{ + { + Severity: checks.Error, + Message: "Validating webhook vw_foo is configured against a service that does not exist.", + Kind: checks.ValidatingWebhookConfiguration, + }, + { + Severity: checks.Error, + Message: "Mutating webhook mw_foo is configured against a service that does not exist.", + Kind: checks.MutatingWebhookConfiguration, + }, + }, + }, + } + + webhookCheck := webhookCheck{} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + diagnostics, err := webhookCheck.Run(test.objs) + assert.NoError(t, err) + + // skip checking object and owner since for this checker it just uses the object being checked. + var strippedDiagnostics []checks.Diagnostic + for _, d := range diagnostics { + d.Object = nil + d.Owners = nil + strippedDiagnostics = append(strippedDiagnostics, d) + } + + assert.ElementsMatch(t, test.expected, strippedDiagnostics) + }) + } +} + +func strPtr(s string) *string { + return &s +} diff --git a/checks/doks/admission_controller_webhook.go b/checks/doks/admission_controller_webhook_replacement.go similarity index 87% rename from checks/doks/admission_controller_webhook.go rename to checks/doks/admission_controller_webhook_replacement.go index c71a854c..360e9b2a 100644 --- a/checks/doks/admission_controller_webhook.go +++ b/checks/doks/admission_controller_webhook_replacement.go @@ -17,8 +17,6 @@ limitations under the License. package doks import ( - "errors" - "github.com/digitalocean/clusterlint/checks" "github.com/digitalocean/clusterlint/kube" ar "k8s.io/api/admissionregistration/v1beta1" @@ -28,29 +26,29 @@ import ( ) func init() { - checks.Register(&webhookCheck{}) + checks.Register(&webhookReplaementCheck{}) } -type webhookCheck struct{} +type webhookReplaementCheck struct{} // Name returns a unique name for this check. -func (w *webhookCheck) Name() string { - return "admission-controller-webhook" +func (w *webhookReplaementCheck) Name() string { + return "admission-controller-webhook-replacement" } // Groups returns a list of group names this check should be part of. -func (w *webhookCheck) Groups() []string { +func (w *webhookReplaementCheck) Groups() []string { return []string{"doks"} } // Description returns a detailed human-readable description of what this check // does. -func (w *webhookCheck) Description() string { - return "Check for admission control webhooks that could cause problems during upgrades" +func (w *webhookReplaementCheck) Description() string { + return "Check for admission control webhooks that could cause problems during upgrades or node replacement" } // Run runs this check on a set of Kubernetes objects. -func (w *webhookCheck) Run(objects *kube.Objects) ([]checks.Diagnostic, error) { +func (w *webhookReplaementCheck) Run(objects *kube.Objects) ([]checks.Diagnostic, error) { const apiserverServiceName = "kubernetes" var diagnostics []checks.Diagnostic @@ -80,10 +78,9 @@ func (w *webhookCheck) Run(objects *kube.Objects) ([]checks.Diagnostic, error) { svcNamespace = &ns } } - if svcNamespace == nil { - return nil, errors.New("webhook refers to service in non-existent namespace") - } - if !selectorMatchesNamespace(wh.NamespaceSelector, svcNamespace) && len(objects.Nodes.Items) > 1 { + if svcNamespace != nil && + !selectorMatchesNamespace(wh.NamespaceSelector, svcNamespace) && + len(objects.Nodes.Items) > 1 { // Webhooks that don't apply to their own namespace are fine, as // long as there's more than one node in the cluster. continue @@ -130,10 +127,9 @@ func (w *webhookCheck) Run(objects *kube.Objects) ([]checks.Diagnostic, error) { svcNamespace = &ns } } - if svcNamespace == nil { - return nil, errors.New("webhook refers to service in non-existent namespace") - } - if !selectorMatchesNamespace(wh.NamespaceSelector, svcNamespace) && len(objects.Nodes.Items) > 1 { + if svcNamespace != nil && + !selectorMatchesNamespace(wh.NamespaceSelector, svcNamespace) && + len(objects.Nodes.Items) > 1 { // Webhooks that don't apply to their own namespace are fine, as // long as there's more than one node in the cluster. continue diff --git a/checks/doks/admission_controller_webhook_test.go b/checks/doks/admission_controller_webhook_test.go index c355edfa..95c463fb 100644 --- a/checks/doks/admission_controller_webhook_test.go +++ b/checks/doks/admission_controller_webhook_test.go @@ -30,15 +30,15 @@ import ( var webhookURL = "https://example.com/webhook" func TestWebhookCheckMeta(t *testing.T) { - webhookCheck := webhookCheck{} - assert.Equal(t, "admission-controller-webhook", webhookCheck.Name()) + webhookCheck := webhookReplaementCheck{} + assert.Equal(t, "admission-controller-webhook-replacement", webhookCheck.Name()) assert.Equal(t, []string{"doks"}, webhookCheck.Groups()) assert.NotEmpty(t, webhookCheck.Description()) } func TestWebhookCheckRegistration(t *testing.T) { - webhookCheck := &webhookCheck{} - check, err := checks.Get("admission-controller-webhook") + webhookCheck := &webhookReplaementCheck{} + check, err := checks.Get("admission-controller-webhook-replacement") assert.NoError(t, err) assert.Equal(t, check, webhookCheck) } @@ -236,7 +236,7 @@ func TestWebhookError(t *testing.T) { }, } - webhookCheck := webhookCheck{} + webhookCheck := webhookReplaementCheck{} for _, test := range tests { t.Run(test.name, func(t *testing.T) {