Merge pull request #92739 from cnphil/fail-open-audit-log

Add fail-open audit logs to validating admission webhook

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go

@@ -55,6 +55,10 @@ const (
 	PatchAuditAnnotationPrefix = "patch.webhook.admission.k8s.io/"
 	// MutationAuditAnnotationPrefix is a prefix for presisting webhook mutation existence in audit annotation.
 	MutationAuditAnnotationPrefix = "mutation.webhook.admission.k8s.io/"
+	// MutationAnnotationFailedOpenKeyPrefix in an annotation indicates
+	// the mutating webhook failed open when the webhook backend connection
+	// failed or returned an internal server error.
+	MutationAuditAnnotationFailedOpenKeyPrefix string = "failed-open." + MutationAuditAnnotationPrefix
 )
 
 var encodingjson = json.CaseSensitiveJSONIterator()
@@ -134,7 +138,9 @@ func (a *mutatingDispatcher) Dispatch(ctx context.Context, attr admission.Attrib
 		if reinvokeCtx.IsReinvoke() {
 			round = 1
 		}
-		changed, err := a.callAttrMutatingHook(ctx, hook, invocation, versionedAttr, o, round, i)
+
+		annotator := newWebhookAnnotator(versionedAttr, round, i, hook.Name, invocation.Webhook.GetConfigurationName())
+		changed, err := a.callAttrMutatingHook(ctx, hook, invocation, versionedAttr, annotator, o, round, i)
 		ignoreClientCallFailures := hook.FailurePolicy != nil && *hook.FailurePolicy == admissionregistrationv1.Ignore
 		rejected := false
 		if err != nil {
@@ -168,6 +174,9 @@ func (a *mutatingDispatcher) Dispatch(ctx context.Context, attr admission.Attrib
 		if callErr, ok := err.(*webhookutil.ErrCallingWebhook); ok {
 			if ignoreClientCallFailures {
 				klog.Warningf("Failed calling webhook, failing open %v: %v", hook.Name, callErr)
+
+				annotator.addFailedOpenAnnotation()
+
 				utilruntime.HandleError(callErr)
 
 				select {
@@ -198,9 +207,8 @@ func (a *mutatingDispatcher) Dispatch(ctx context.Context, attr admission.Attrib
 
 // note that callAttrMutatingHook updates attr
 
-func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, h *admissionregistrationv1.MutatingWebhook, invocation *generic.WebhookInvocation, attr *generic.VersionedAttributes, o admission.ObjectInterfaces, round, idx int) (bool, error) {
+func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, h *admissionregistrationv1.MutatingWebhook, invocation *generic.WebhookInvocation, attr *generic.VersionedAttributes, annotator *webhookAnnotator, o admission.ObjectInterfaces, round, idx int) (bool, error) {
 	configurationName := invocation.Webhook.GetConfigurationName()
-	annotator := newWebhookAnnotator(attr, round, idx, h.Name, configurationName)
 	changed := false
 	defer func() { annotator.addMutationAnnotation(changed) }()
 	if attr.Attributes.IsDryRun() {
@@ -338,20 +346,32 @@ func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, h *admiss
 }
 
 type webhookAnnotator struct {
-	attr                  *generic.VersionedAttributes
-	patchAnnotationKey    string
-	mutationAnnotationKey string
-	webhook               string
-	configuration         string
+	attr                    *generic.VersionedAttributes
+	failedOpenAnnotationKey string
+	patchAnnotationKey      string
+	mutationAnnotationKey   string
+	webhook                 string
+	configuration           string
 }
 
 func newWebhookAnnotator(attr *generic.VersionedAttributes, round, idx int, webhook, configuration string) *webhookAnnotator {
 	return &webhookAnnotator{
-		attr:                  attr,
-		patchAnnotationKey:    fmt.Sprintf("%sround_%d_index_%d", PatchAuditAnnotationPrefix, round, idx),
-		mutationAnnotationKey: fmt.Sprintf("%sround_%d_index_%d", MutationAuditAnnotationPrefix, round, idx),
-		webhook:               webhook,
-		configuration:         configuration,
+		attr:                    attr,
+		failedOpenAnnotationKey: fmt.Sprintf("%sround_%d_index_%d", MutationAuditAnnotationFailedOpenKeyPrefix, round, idx),
+		patchAnnotationKey:      fmt.Sprintf("%sround_%d_index_%d", PatchAuditAnnotationPrefix, round, idx),
+		mutationAnnotationKey:   fmt.Sprintf("%sround_%d_index_%d", MutationAuditAnnotationPrefix, round, idx),
+		webhook:                 webhook,
+		configuration:           configuration,
+	}
+}
+
+func (w *webhookAnnotator) addFailedOpenAnnotation() {
+	if w.attr == nil || w.attr.Attributes == nil {
+		return
+	}
+	value := w.webhook
+	if err := w.attr.Attributes.AddAnnotation(w.failedOpenAnnotationKey, value); err != nil {
+		klog.Warningf("failed to set failed open annotation for mutating webhook key %s to %s: %v", w.failedOpenAnnotationKey, value, err)
 	}
 }
 

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go

@@ -264,6 +264,18 @@ func ConvertToMutatingTestCases(tests []ValidatingTest, configurationName string
 				break
 			}
 		}
+		// Change annotation keys for Validating's fail open to Mutating's fail open.
+		failOpenAnnotations := map[string]string{}
+		for key, value := range t.ExpectAnnotations {
+			if strings.HasPrefix(key, "failed-open.validating.webhook.admission.k8s.io/") {
+				failOpenAnnotations[key] = value
+			}
+		}
+		for key, value := range failOpenAnnotations {
+			newKey := strings.Replace(key, "failed-open.validating.webhook.admission.k8s.io/", "failed-open.mutation.webhook.admission.k8s.io/", 1)
+			t.ExpectAnnotations[newKey] = value
+			delete(t.ExpectAnnotations, key)
+		}
 		r[i] = MutatingTest{t.Name, ConvertToMutatingWebhooks(t.Webhooks), t.Path, t.IsCRD, t.IsDryRun, t.AdditionalLabels, t.SkipBenchmark, t.ExpectLabels, t.ExpectAllow, t.ErrorContains, t.ExpectAnnotations, t.ExpectStatusCode, t.ExpectReinvokeWebhooks}
 	}
 	return r
@@ -408,6 +420,11 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest {
 
 			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 disallow because fail close on nil FailurePolicy)",
@@ -502,8 +519,9 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest {
 				ObjectSelector:          &metav1.LabelSelector{},
 				AdmissionReviewVersions: []string{"v1beta1"},
 			}},
-			SkipBenchmark: true,
-			ExpectAllow:   true,
+			SkipBenchmark:     true,
+			ExpectAllow:       true,
+			ExpectAnnotations: map[string]string{"failed-open.validating.webhook.admission.k8s.io/round_0_index_0": "nilResponse"},
 		},
 		{
 			Name: "absent response and fail closed",

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go

@@ -38,6 +38,16 @@ import (
 	utiltrace "k8s.io/utils/trace"
 )
 
+const (
+	// ValidatingAuditAnnotationPrefix is a prefix for keeping noteworthy
+	// validating audit annotations.
+	ValidatingAuditAnnotationPrefix = "validating.webhook.admission.k8s.io/"
+	// ValidatingAuditAnnotationFailedOpenKeyPrefix in an annotation indicates
+	// the validating webhook failed open when the webhook backend connection
+	// failed or returned an internal server error.
+	ValidatingAuditAnnotationFailedOpenKeyPrefix = "failed-open." + ValidatingAuditAnnotationPrefix
+)
+
 type validatingDispatcher struct {
 	cm     *webhookutil.ClientManager
 	plugin *Plugin
@@ -92,7 +102,7 @@ func (d *validatingDispatcher) Dispatch(ctx context.Context, attr admission.Attr
 	errCh := make(chan error, len(relevantHooks))
 	wg.Add(len(relevantHooks))
 	for i := range relevantHooks {
-		go func(invocation *generic.WebhookInvocation) {
+		go func(invocation *generic.WebhookInvocation, idx int) {
 			defer wg.Done()
 			hook, ok := invocation.Webhook.GetValidatingWebhook()
 			if !ok {
@@ -127,6 +137,12 @@ func (d *validatingDispatcher) Dispatch(ctx context.Context, attr admission.Attr
 			if callErr, ok := err.(*webhookutil.ErrCallingWebhook); ok {
 				if ignoreClientCallFailures {
 					klog.Warningf("Failed calling webhook, failing open %v: %v", hook.Name, callErr)
+
+					key := fmt.Sprintf("%sround_0_index_%d", ValidatingAuditAnnotationFailedOpenKeyPrefix, idx)
+					value := hook.Name
+					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, hook.Name, err)
+					}
 					utilruntime.HandleError(callErr)
 					return
 				}
@@ -141,7 +157,7 @@ func (d *validatingDispatcher) Dispatch(ctx context.Context, attr admission.Attr
 			}
 			klog.Warningf("rejected by webhook %q: %#v", hook.Name, err)
 			errCh <- err
-		}(relevantHooks[i])
+		}(relevantHooks[i], i)
 	}
 	wg.Wait()
 	close(errCh)