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

Add fail-open audit logs to validating admission webhook

Kubernetes-commit: ebd61572c579476a6537b9002d90c978f24dda4e

Godeps/Godeps.json

@@ -924,7 +924,7 @@
 		},
 		{
 			"ImportPath": "k8s.io/api",
-			"Rev": "191e7be037d4"
+			"Rev": "a16591c7eddc"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery",
@@ -932,7 +932,7 @@
 		},
 		{
 			"ImportPath": "k8s.io/client-go",
-			"Rev": "d1fa200aef5b"
+			"Rev": "228dada99554"
 		},
 		{
 			"ImportPath": "k8s.io/component-base",

go.mod

@@ -38,9 +38,9 @@ require (
 	google.golang.org/grpc v1.27.1
 	gopkg.in/natefinch/lumberjack.v2 v2.0.0
 	gopkg.in/square/go-jose.v2 v2.2.2
-	k8s.io/api v0.0.0-20210521070909-191e7be037d4
+	k8s.io/api v0.0.0-20210523150857-a16591c7eddc
 	k8s.io/apimachinery v0.0.0-20210521150646-cfc896c115eb
-	k8s.io/client-go v0.0.0-20210521071212-d1fa200aef5b
+	k8s.io/client-go v0.0.0-20210524151206-228dada99554
 	k8s.io/component-base v0.0.0-20210521071829-2a2fe9bbf92f
 	k8s.io/klog/v2 v2.8.0
 	k8s.io/kube-openapi v0.0.0-20210421082810-95288971da7e
@@ -51,8 +51,8 @@ require (
 )
 
 replace (
-	k8s.io/api => k8s.io/api v0.0.0-20210521070909-191e7be037d4
+	k8s.io/api => k8s.io/api v0.0.0-20210523150857-a16591c7eddc
 	k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20210521150646-cfc896c115eb
-	k8s.io/client-go => k8s.io/client-go v0.0.0-20210521071212-d1fa200aef5b
+	k8s.io/client-go => k8s.io/client-go v0.0.0-20210524151206-228dada99554
 	k8s.io/component-base => k8s.io/component-base v0.0.0-20210521071829-2a2fe9bbf92f
 )

go.sum

@@ -684,12 +684,12 @@ honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWh
 honnef.co/go/tools v0.0.1-2019.2.3/go.mod h1:a3bituU0lyd329TUQxRnasdCoJDkEUEAqEt0JzvZhAg=
 honnef.co/go/tools v0.0.1-2020.1.3 h1:sXmLre5bzIR6ypkjXCDI3jHPssRhc8KD/Ome589sc3U=
 honnef.co/go/tools v0.0.1-2020.1.3/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k=
-k8s.io/api v0.0.0-20210521070909-191e7be037d4 h1:qgCkzN01a1u8h5bYNgyqdKqGL+K19wvkp1JLOv3Oj2E=
+k8s.io/api v0.0.0-20210523150857-a16591c7eddc h1:MY1s8HL0z3c7g23mc5sawXXsIBL/kp89slIVNKz9mZ8=
-k8s.io/api v0.0.0-20210521070909-191e7be037d4/go.mod h1:GfPTpPBzvUagWJY1n2kt8oncShcczHwc4trHJ+04pNE=
+k8s.io/api v0.0.0-20210523150857-a16591c7eddc/go.mod h1:OYWcx23Cf4qX54GQBQLeSSUlvOAvbgMucgB0TrADRHU=
 k8s.io/apimachinery v0.0.0-20210521150646-cfc896c115eb h1:kxJH/Hf/+EnvbMiymbBbJ5nVuiQjcHZjfgI57rCtSDc=
 k8s.io/apimachinery v0.0.0-20210521150646-cfc896c115eb/go.mod h1:8Ay3sPKuJtD/dIjPSlQVgJeuW/8JmLTCqDeAv6NSCxU=
-k8s.io/client-go v0.0.0-20210521071212-d1fa200aef5b h1:mcl3yABrLVtYYWiFDkScO9n8op2wxzwEzReyjO4WEPY=
+k8s.io/client-go v0.0.0-20210524151206-228dada99554 h1:1dDOjwqRZGPWA+A9/dSuZ7KVJcGwDkabQf/N3Hdwyyk=
-k8s.io/client-go v0.0.0-20210521071212-d1fa200aef5b/go.mod h1:fSZsfZrJMyBRs3783ddvdNtsZlZbJDL4O3F1ROhjdIY=
+k8s.io/client-go v0.0.0-20210524151206-228dada99554/go.mod h1:EWkJNCU4qOSWrtM+poIXCY937ArhI5w0ACG3zkw0qXE=
 k8s.io/component-base v0.0.0-20210521071829-2a2fe9bbf92f h1:5WGHsufGjyHFfwMXj0R/nilyN7HzOrb1XqGO6NErjDY=
 k8s.io/component-base v0.0.0-20210521071829-2a2fe9bbf92f/go.mod h1:PLrHIJKBKJZB+zZsUAAQNbGrps/afIih9Oj/oqdQtsc=
 k8s.io/gengo v0.0.0-20200413195148-3a45101e95ac/go.mod h1:ezvh/TsK7cY6rbqRK0oQQ8IAqLxYwwyPxAX1Pzy0ii0=

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
+	attr                    *generic.VersionedAttributes
-	patchAnnotationKey    string
+	failedOpenAnnotationKey string
-	mutationAnnotationKey string
+	patchAnnotationKey      string
-	webhook               string
+	mutationAnnotationKey   string
-	configuration         string
+	webhook                 string
+	configuration           string
 }
 
 func newWebhookAnnotator(attr *generic.VersionedAttributes, round, idx int, webhook, configuration string) *webhookAnnotator {
 	return &webhookAnnotator{
-		attr:                  attr,
+		attr:                    attr,
-		patchAnnotationKey:    fmt.Sprintf("%sround_%d_index_%d", PatchAuditAnnotationPrefix, round, idx),
+		failedOpenAnnotationKey: fmt.Sprintf("%sround_%d_index_%d", MutationAuditAnnotationFailedOpenKeyPrefix, round, idx),
-		mutationAnnotationKey: fmt.Sprintf("%sround_%d_index_%d", MutationAuditAnnotationPrefix, round, idx),
+		patchAnnotationKey:      fmt.Sprintf("%sround_%d_index_%d", PatchAuditAnnotationPrefix, round, idx),
-		webhook:               webhook,
+		mutationAnnotationKey:   fmt.Sprintf("%sround_%d_index_%d", MutationAuditAnnotationPrefix, round, idx),
-		configuration:         configuration,
+		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)
 	}
 }
 

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,
+			SkipBenchmark:     true,
-			ExpectAllow:   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",

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)