Merge pull request kubernetes#108746 from deads2k/proof-2

Handle panic during validating admission webhook admission

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)
+}

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)
+}

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)
 }

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) {

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.")
+		}
+	}
+}