From f81b6004dedfa928dc7fece5de6fa365ac8e4ea4 Mon Sep 17 00:00:00 2001 From: David Eads Date: Thu, 12 Oct 2017 15:59:17 -0400 Subject: [PATCH] allow fail close webhook admission --- .../validation/validation.go | 10 ++- .../validation/validation_test.go | 6 +- plugin/pkg/admission/webhook/admission.go | 28 +++++-- .../pkg/admission/webhook/admission_test.go | 73 ++++++++++++++++--- 4 files changed, 91 insertions(+), 26 deletions(-) diff --git a/pkg/apis/admissionregistration/validation/validation.go b/pkg/apis/admissionregistration/validation/validation.go index 07c0bb1b19..3c4068f24c 100644 --- a/pkg/apis/admissionregistration/validation/validation.go +++ b/pkg/apis/admissionregistration/validation/validation.go @@ -179,13 +179,17 @@ func validateExternalAdmissionHook(hook *admissionregistration.ExternalAdmission for i, rule := range hook.Rules { allErrors = append(allErrors, validateRuleWithOperations(&rule, fldPath.Child("rules").Index(i))...) } - // TODO: relax the validation rule when admissionregistration is beta. - if hook.FailurePolicy != nil && *hook.FailurePolicy != admissionregistration.Ignore { - allErrors = append(allErrors, field.NotSupported(fldPath.Child("failurePolicy"), *hook.FailurePolicy, []string{string(admissionregistration.Ignore)})) + if hook.FailurePolicy != nil && !supportedFailurePolicies.Has(string(*hook.FailurePolicy)) { + allErrors = append(allErrors, field.NotSupported(fldPath.Child("failurePolicy"), *hook.FailurePolicy, supportedFailurePolicies.List())) } return allErrors } +var supportedFailurePolicies = sets.NewString( + string(admissionregistration.Ignore), + string(admissionregistration.Fail), +) + var supportedOperations = sets.NewString( string(admissionregistration.OperationAll), string(admissionregistration.Create), diff --git a/pkg/apis/admissionregistration/validation/validation_test.go b/pkg/apis/admissionregistration/validation/validation_test.go index de36f18065..a842c47281 100644 --- a/pkg/apis/admissionregistration/validation/validation_test.go +++ b/pkg/apis/admissionregistration/validation/validation_test.go @@ -469,18 +469,18 @@ func TestValidateExternalAdmissionHookConfiguration(t *testing.T) { expectedError: `externalAdmissionHooks[0].rules[0].resources: Invalid value: []string{"*/*", "a"}: if '*/*' is present, must not specify other resources`, }, { - name: "FailurePolicy can only be \"Ignore\"", + name: "FailurePolicy can only be \"Ignore\" or \"Fail\"", config: getExternalAdmissionHookConfiguration( []admissionregistration.ExternalAdmissionHook{ { Name: "webhook.k8s.io", FailurePolicy: func() *admissionregistration.FailurePolicyType { - r := admissionregistration.Fail + r := admissionregistration.FailurePolicyType("other") return &r }(), }, }), - expectedError: `failurePolicy: Unsupported value: "Fail": supported values: "Ignore"`, + expectedError: `externalAdmissionHooks[0].failurePolicy: Unsupported value: "other": supported values: "Fail", "Ignore"`, }, } for _, test := range tests { diff --git a/plugin/pkg/admission/webhook/admission.go b/plugin/pkg/admission/webhook/admission.go index efcd534179..97a55dc828 100644 --- a/plugin/pkg/admission/webhook/admission.go +++ b/plugin/pkg/admission/webhook/admission.go @@ -192,16 +192,28 @@ func (a *GenericAdmissionWebhook) Admit(attr admission.Attributes) error { for i := range hooks { go func(hook *v1alpha1.ExternalAdmissionHook) { defer wg.Done() - if err := a.callHook(ctx, hook, attr); err == nil { + + err := a.callHook(ctx, hook, attr) + if err == nil { return - } else if callErr, ok := err.(*ErrCallingWebhook); ok { - glog.Warningf("Failed calling webhook %v: %v", hook.Name, callErr) - utilruntime.HandleError(callErr) - // Since we are failing open to begin with, we do not send an error down the channel - } else { - glog.Warningf("rejected by webhook %v %t: %v", hook.Name, err, err) - errCh <- err } + + ignoreClientCallFailures := hook.FailurePolicy == nil || *hook.FailurePolicy == v1alpha1.Ignore + if callErr, ok := err.(*ErrCallingWebhook); ok { + if ignoreClientCallFailures { + glog.Warningf("Failed calling webhook, failing open %v: %v", hook.Name, callErr) + utilruntime.HandleError(callErr) + // Since we are failing open to begin with, we do not send an error down the channel + return + } + + glog.Warningf("Failed calling webhook, failing closed %v: %v", hook.Name, err) + errCh <- err + return + } + + glog.Warningf("rejected by webhook %v %t: %v", hook.Name, err, err) + errCh <- err }(&hooks[i]) } wg.Wait() diff --git a/plugin/pkg/admission/webhook/admission_test.go b/plugin/pkg/admission/webhook/admission_test.go index 1e66fb4a46..71076d0a81 100644 --- a/plugin/pkg/admission/webhook/admission_test.go +++ b/plugin/pkg/admission/webhook/admission_test.go @@ -144,6 +144,9 @@ func TestAdmit(t *testing.T) { }, }} + policyFail := registrationv1alpha1.Fail + policyIgnore := registrationv1alpha1.Ignore + table := map[string]test{ "no match": { hookSource: fakeHookSource{ @@ -192,6 +195,28 @@ func TestAdmit(t *testing.T) { errorContains: "you shall not pass", }, "match & fail (but allow because fail open)": { + hookSource: fakeHookSource{ + hooks: []registrationv1alpha1.ExternalAdmissionHook{{ + Name: "internalErr A", + ClientConfig: ccfg, + Rules: matchEverythingRules, + FailurePolicy: &policyIgnore, + }, { + Name: "internalErr B", + ClientConfig: ccfg, + Rules: matchEverythingRules, + FailurePolicy: &policyIgnore, + }, { + Name: "internalErr C", + ClientConfig: ccfg, + Rules: matchEverythingRules, + FailurePolicy: &policyIgnore, + }}, + }, + path: "internalErr", + expectAllow: true, + }, + "match & fail (but allow because fail open on nil)": { hookSource: fakeHookSource{ hooks: []registrationv1alpha1.ExternalAdmissionHook{{ Name: "internalErr A", @@ -210,23 +235,47 @@ func TestAdmit(t *testing.T) { path: "internalErr", expectAllow: true, }, + "match & fail (but fail because fail closed)": { + hookSource: fakeHookSource{ + hooks: []registrationv1alpha1.ExternalAdmissionHook{{ + Name: "internalErr A", + ClientConfig: ccfg, + Rules: matchEverythingRules, + FailurePolicy: &policyFail, + }, { + Name: "internalErr B", + ClientConfig: ccfg, + Rules: matchEverythingRules, + FailurePolicy: &policyFail, + }, { + Name: "internalErr C", + ClientConfig: ccfg, + Rules: matchEverythingRules, + FailurePolicy: &policyFail, + }}, + }, + path: "internalErr", + expectAllow: false, + }, } for name, tt := range table { - wh.hookSource = &tt.hookSource - wh.serviceResolver = fakeServiceResolver{base: *serverURL, path: tt.path} - wh.SetScheme(legacyscheme.Scheme) + t.Run(name, func(t *testing.T) { + wh.hookSource = &tt.hookSource + wh.serviceResolver = fakeServiceResolver{base: *serverURL, path: tt.path} + wh.SetScheme(legacyscheme.Scheme) - err = wh.Admit(admission.NewAttributesRecord(&object, &oldObject, kind, namespace, name, resource, subResource, operation, &userInfo)) - if tt.expectAllow != (err == nil) { - t.Errorf("%q: expected allowed=%v, but got err=%v", 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("%q: expected an error saying %q, but got %v", name, tt.errorContains, err) + err = wh.Admit(admission.NewAttributesRecord(&object, &oldObject, kind, namespace, name, resource, subResource, operation, &userInfo)) + if tt.expectAllow != (err == nil) { + t.Errorf("expected allowed=%v, but got err=%v", 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(" expected an error saying %q, but got %v", tt.errorContains, err) + } + } + }) } }