Merge pull request kubernetes#53823 from deads2k/admission-01-allow-fail

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

allow fail close webhook admission

Webhook admission needs to allow failing closed.  Even in an alpha state, I don't want to be one DDOS away from having an exposed cluster.

/assign caesarxuchao
/assign sttts

Author: Kubernetes Submit Queue

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

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 {

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

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