From 9fe7c8955bcb1edbb5aa4fe6bfb8bb6d93d381de Mon Sep 17 00:00:00 2001 From: Xiaojun Hu Date: Tue, 18 May 2021 13:31:03 -0400 Subject: [PATCH] add fail-open audit logs to validating and mutating admission webhook --- .../plugin/webhook/mutating/dispatcher.go | 46 +++++++++++++------ .../plugin/webhook/testing/testcase.go | 22 ++++++++- .../plugin/webhook/validating/dispatcher.go | 20 +++++++- 3 files changed, 71 insertions(+), 17 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go index fc636601b5d..3b35bb31ece 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go +++ b/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) } } 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 d6ae57247bd..88911d747b1 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 @@ -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", 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 7a24d52ce57..250bb02a783 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 @@ -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)