diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/authentication_info_resolver.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/authentication_info_resolver.go index c06f6d826a8..4a2b0de10e9 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/authentication_info_resolver.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/authentication_info_resolver.go @@ -61,3 +61,22 @@ func (a *authenticationInfoResolver) ClientConfigForService(serviceName, service atomic.AddInt32(a.cacheMisses, 1) return a.restConfig, nil } + +// NewPanickingAuthenticationInfoResolver creates a fake AuthenticationInfoResolver that panics +func NewPanickingAuthenticationInfoResolver(panicMessage string) webhook.AuthenticationInfoResolver { + return &panickingAuthenticationInfoResolver{ + panicMessage: panicMessage, + } +} + +type panickingAuthenticationInfoResolver struct { + panicMessage string +} + +func (a *panickingAuthenticationInfoResolver) ClientConfigFor(hostPort string) (*rest.Config, error) { + panic(a.panicMessage) +} + +func (a *panickingAuthenticationInfoResolver) ClientConfigForService(serviceName, serviceNamespace string, servicePort int) (*rest.Config, error) { + panic(a.panicMessage) +} diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/service_resolver.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/service_resolver.go index 97c2e9a521f..526ffcc9d99 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/service_resolver.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/service_resolver.go @@ -40,3 +40,16 @@ func (f serviceResolver) ResolveEndpoint(namespace, name string, port int32) (*u u := f.base return &u, nil } + +type panickingResolver struct { + panicMessage string +} + +// NewPanickingServiceResolver returns a static service resolver that panics. +func NewPanickingServiceResolver(panicMessage string) webhook.ServiceResolver { + return &panickingResolver{panicMessage} +} + +func (f panickingResolver) ResolveEndpoint(namespace, name string, port int32) (*url.URL, error) { + panic(f.panicMessage) +} diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go index caa1e2b79d1..539fbfd538f 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go @@ -691,6 +691,98 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { } } +// NewNonMutatingPanicTestCases returns test cases with a given base url. +// All test cases in NewNonMutatingTestCases have no Patch set in +// AdmissionResponse. The expected responses are set for panic handling. +func NewNonMutatingPanicTestCases(url *url.URL) []ValidatingTest { + policyIgnore := registrationv1.Ignore + policyFail := registrationv1.Fail + + return []ValidatingTest{ + { + Name: "match & allow, but panic", + Webhooks: []registrationv1.ValidatingWebhook{{ + Name: "allow.example.com", + ClientConfig: ccfgSVC("allow"), + Rules: matchEverythingRules, + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + AdmissionReviewVersions: []string{"v1beta1"}, + }}, + ExpectStatusCode: http.StatusForbidden, + ErrorContains: "ValidatingAdmissionWebhook/allow.example.com has panicked: Start panicking!", + ExpectAnnotations: map[string]string{}, + }, + { + Name: "match & fail (but allow because fail open)", + Webhooks: []registrationv1.ValidatingWebhook{{ + Name: "internalErr A", + ClientConfig: ccfgSVC("internalErr"), + Rules: matchEverythingRules, + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + FailurePolicy: &policyIgnore, + AdmissionReviewVersions: []string{"v1beta1"}, + }, { + Name: "internalErr B", + ClientConfig: ccfgSVC("internalErr"), + Rules: matchEverythingRules, + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + FailurePolicy: &policyIgnore, + AdmissionReviewVersions: []string{"v1beta1"}, + }, { + Name: "internalErr C", + ClientConfig: ccfgSVC("internalErr"), + Rules: matchEverythingRules, + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + FailurePolicy: &policyIgnore, + AdmissionReviewVersions: []string{"v1beta1"}, + }}, + + SkipBenchmark: true, + ExpectAllow: true, + ExpectAnnotations: map[string]string{ + "failed-open.validating.webhook.admission.k8s.io/round_0_index_0": "internalErr A", + "failed-open.validating.webhook.admission.k8s.io/round_0_index_1": "internalErr B", + "failed-open.validating.webhook.admission.k8s.io/round_0_index_2": "internalErr C", + }, + }, + { + Name: "match & fail (but fail because fail closed)", + Webhooks: []registrationv1.ValidatingWebhook{{ + Name: "internalErr A", + ClientConfig: ccfgSVC("internalErr"), + Rules: matchEverythingRules, + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + FailurePolicy: &policyFail, + AdmissionReviewVersions: []string{"v1beta1"}, + }, { + Name: "internalErr B", + ClientConfig: ccfgSVC("internalErr"), + Rules: matchEverythingRules, + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + FailurePolicy: &policyFail, + AdmissionReviewVersions: []string{"v1beta1"}, + }, { + Name: "internalErr C", + ClientConfig: ccfgSVC("internalErr"), + Rules: matchEverythingRules, + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + FailurePolicy: &policyFail, + AdmissionReviewVersions: []string{"v1beta1"}, + }}, + ExpectStatusCode: http.StatusInternalServerError, + ExpectAllow: false, + ErrorContains: " has panicked: Start panicking!", + }, + } +} + func mutationAnnotationValue(configuration, webhook string, mutated bool) string { return fmt.Sprintf(`{"configuration":"%s","webhook":"%s","mutated":%t}`, configuration, webhook, mutated) } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go index a11da8b94c0..01dae9dc304 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go @@ -100,20 +100,52 @@ func (d *validatingDispatcher) Dispatch(ctx context.Context, attr admission.Attr } wg := sync.WaitGroup{} - errCh := make(chan error, len(relevantHooks)) + errCh := make(chan error, 2*len(relevantHooks)) // double the length to handle extra errors for panics in the gofunc wg.Add(len(relevantHooks)) for i := range relevantHooks { go func(invocation *generic.WebhookInvocation, idx int) { + ignoreClientCallFailures := false + hookName := "unknown" + versionedAttr := versionedAttrs[invocation.Kind] + // The ordering of these two defers is critical. The wg.Done will release the parent go func to close the errCh + // that is used by the second defer to report errors. The recovery and error reporting must be done first. defer wg.Done() + defer func() { + // HandleCrash has already called the crash handlers and it has been configured to utilruntime.ReallyCrash + // This block prevents the second panic from failing our process. + // This failure mode for the handler functions properly using the channel below. + recover() + }() + defer utilruntime.HandleCrash( + func(r interface{}) { + if r == nil { + return + } + if ignoreClientCallFailures { + // if failures are supposed to ignored, ignore it + klog.Warningf("Panic calling webhook, failing open %v: %v", hookName, r) + admissionmetrics.Metrics.ObserveWebhookFailOpen(ctx, hookName, "validating") + key := fmt.Sprintf("%sround_0_index_%d", ValidatingAuditAnnotationFailedOpenKeyPrefix, idx) + value := hookName + if err := versionedAttr.Attributes.AddAnnotation(key, value); err != nil { + klog.Warningf("Failed to set admission audit annotation %s to %s for validating webhook %s: %v", key, value, hookName, err) + } + return + } + // this ensures that the admission request fails and a message is provided. + errCh <- apierrors.NewInternalError(fmt.Errorf("ValidatingAdmissionWebhook/%v has panicked: %v", hookName, r)) + }, + ) + hook, ok := invocation.Webhook.GetValidatingWebhook() if !ok { utilruntime.HandleError(fmt.Errorf("validating webhook dispatch requires v1.ValidatingWebhook, but got %T", hook)) return } - versionedAttr := versionedAttrs[invocation.Kind] + hookName = hook.Name + ignoreClientCallFailures = hook.FailurePolicy != nil && *hook.FailurePolicy == v1.Ignore t := time.Now() err := d.callHook(ctx, hook, invocation, versionedAttr) - ignoreClientCallFailures := hook.FailurePolicy != nil && *hook.FailurePolicy == v1.Ignore rejected := false if err != nil { switch err := err.(type) { diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/plugin_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/plugin_test.go index f4d6b3df7bf..b9ea6662d9c 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/plugin_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/plugin_test.go @@ -281,3 +281,69 @@ func TestValidateWebhookDuration(ts *testing.T) { }) } } + +// TestValidatePanicHandling tests that panics should not escape the dispatcher +func TestValidatePanicHandling(t *testing.T) { + testServer := webhooktesting.NewTestServer(t) + testServer.StartTLS() + defer testServer.Close() + + objectInterfaces := webhooktesting.NewObjectInterfacesForTest() + + serverURL, err := url.ParseRequestURI(testServer.URL) + if err != nil { + t.Fatalf("this should never happen? %v", err) + } + + stopCh := make(chan struct{}) + defer close(stopCh) + + for _, tt := range webhooktesting.NewNonMutatingPanicTestCases(serverURL) { + wh, err := NewValidatingAdmissionWebhook(nil) + if err != nil { + t.Errorf("%s: failed to create validating webhook: %v", tt.Name, err) + continue + } + + ns := "webhook-test" + client, informer := webhooktesting.NewFakeValidatingDataSource(ns, tt.Webhooks, stopCh) + + wh.SetAuthenticationInfoResolverWrapper(webhooktesting.Wrapper(webhooktesting.NewPanickingAuthenticationInfoResolver("Start panicking!"))) // see Aladdin, it's awesome + wh.SetServiceResolver(webhooktesting.NewServiceResolver(*serverURL)) + wh.SetExternalKubeClientSet(client) + wh.SetExternalKubeInformerFactory(informer) + + informer.Start(stopCh) + informer.WaitForCacheSync(stopCh) + + if err = wh.ValidateInitialization(); err != nil { + t.Errorf("%s: failed to validate initialization: %v", tt.Name, err) + continue + } + + attr := webhooktesting.NewAttribute(ns, nil, tt.IsDryRun) + err = wh.Validate(context.TODO(), attr, objectInterfaces) + if tt.ExpectAllow != (err == nil) { + t.Errorf("%s: expected allowed=%v, but got err=%v", tt.Name, tt.ExpectAllow, err) + } + // ErrWebhookRejected is not an error for our purposes + if tt.ErrorContains != "" { + if err == nil || !strings.Contains(err.Error(), tt.ErrorContains) { + t.Errorf("%s: expected an error saying %q, but got %v", tt.Name, tt.ErrorContains, err) + } + } + if _, isStatusErr := err.(*errors.StatusError); err != nil && !isStatusErr { + t.Errorf("%s: expected a StatusError, got %T", tt.Name, err) + } + fakeAttr, ok := attr.(*webhooktesting.FakeAttributes) + if !ok { + t.Errorf("Unexpected error, failed to convert attr to webhooktesting.FakeAttributes") + continue + } + if len(tt.ExpectAnnotations) == 0 { + assert.Empty(t, fakeAttr.GetAnnotations(auditinternal.LevelMetadata), tt.Name+": annotations not set as expected.") + } else { + assert.Equal(t, tt.ExpectAnnotations, fakeAttr.GetAnnotations(auditinternal.LevelMetadata), tt.Name+": annotations not set as expected.") + } + } +}