Merge pull request #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

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

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