Merge pull request #74562 from roycaihw/feat/admission-webhook-timeout

api/openapi-spec/swagger.json

@@ -223,6 +223,11 @@
         "sideEffects": {
           "description": "SideEffects states whether this webhookk has side effects. Acceptable values are: Unknown, None, Some, NoneOnDryRun Webhooks with side effects MUST implement a reconciliation system, since a request may be rejected by a future step in the admission change and the side effects therefore need to be undone. Requests with the dryRun attribute will be auto-rejected if they match a webhook with sideEffects == Unknown or Some. Defaults to Unknown.",
           "type": "string"
+        },
+        "timeoutSeconds": {
+          "description": "TimeoutSeconds specifies the timeout for this webhook. After the timeout passes, the webhook call will be ignored or the API call will fail based on the failure policy. The timeout value must be between 1 and 30 seconds. Default to 30 seconds.",
+          "format": "int32",
+          "type": "integer"
         }
       },
       "required": [

pkg/apis/admissionregistration/fuzzer/fuzzer.go

@@ -32,6 +32,10 @@ var Funcs = func(codecs runtimeserializer.CodecFactory) []interface{} {
 			obj.FailurePolicy = &p
 			s := admissionregistration.SideEffectClassUnknown
 			obj.SideEffects = &s
+			if obj.TimeoutSeconds == nil {
+				i := int32(30)
+				obj.TimeoutSeconds = &i
+			}
 		},
 	}
 }

pkg/apis/admissionregistration/types.go

@@ -208,6 +208,13 @@ type Webhook struct {
 	// sideEffects == Unknown or Some. Defaults to Unknown.
 	// +optional
 	SideEffects *SideEffectClass
+
+	// TimeoutSeconds specifies the timeout for this webhook. After the timeout passes,
+	// the webhook call will be ignored or the API call will fail based on the
+	// failure policy.
+	// The timeout value must be between 1 and 30 seconds.
+	// +optional
+	TimeoutSeconds *int32
 }
 
 // RuleWithOperations is a tuple of Operations and Resources. It is recommended to make

pkg/apis/admissionregistration/v1beta1/defaults.go

@@ -40,4 +40,8 @@ func SetDefaults_Webhook(obj *admissionregistrationv1beta1.Webhook) {
 		unknown := admissionregistrationv1beta1.SideEffectClassUnknown
 		obj.SideEffects = &unknown
 	}
+	if obj.TimeoutSeconds == nil {
+		obj.TimeoutSeconds = new(int32)
+		*obj.TimeoutSeconds = 30
+	}
 }

pkg/apis/admissionregistration/v1beta1/zz_generated.conversion.go

@@ -301,6 +301,7 @@ func autoConvert_v1beta1_Webhook_To_admissionregistration_Webhook(in *v1beta1.We
 	out.FailurePolicy = (*admissionregistration.FailurePolicyType)(unsafe.Pointer(in.FailurePolicy))
 	out.NamespaceSelector = (*v1.LabelSelector)(unsafe.Pointer(in.NamespaceSelector))
 	out.SideEffects = (*admissionregistration.SideEffectClass)(unsafe.Pointer(in.SideEffects))
+	out.TimeoutSeconds = (*int32)(unsafe.Pointer(in.TimeoutSeconds))
 	return nil
 }
 
@@ -318,6 +319,7 @@ func autoConvert_admissionregistration_Webhook_To_v1beta1_Webhook(in *admissionr
 	out.FailurePolicy = (*v1beta1.FailurePolicyType)(unsafe.Pointer(in.FailurePolicy))
 	out.NamespaceSelector = (*v1.LabelSelector)(unsafe.Pointer(in.NamespaceSelector))
 	out.SideEffects = (*v1beta1.SideEffectClass)(unsafe.Pointer(in.SideEffects))
+	out.TimeoutSeconds = (*int32)(unsafe.Pointer(in.TimeoutSeconds))
 	return nil
 }
 

pkg/apis/admissionregistration/validation/validation.go

@@ -171,6 +171,9 @@ func validateWebhook(hook *admissionregistration.Webhook, fldPath *field.Path) f
 	if hook.SideEffects != nil && !supportedSideEffectClasses.Has(string(*hook.SideEffects)) {
 		allErrors = append(allErrors, field.NotSupported(fldPath.Child("sideEffects"), *hook.SideEffects, supportedSideEffectClasses.List()))
 	}
+	if hook.TimeoutSeconds != nil && (*hook.TimeoutSeconds > 30 || *hook.TimeoutSeconds < 1) {
+		allErrors = append(allErrors, field.Invalid(fldPath.Child("timeoutSeconds"), *hook.TimeoutSeconds, "the timeout value must be between 1 and 30 seconds"))
+	}
 
 	if hook.NamespaceSelector != nil {
 		allErrors = append(allErrors, metav1validation.ValidateLabelSelector(hook.NamespaceSelector, fldPath.Child("namespaceSelector"))...)

pkg/apis/admissionregistration/validation/validation_test.go

@@ -26,6 +26,8 @@ import (
 
 func strPtr(s string) *string { return &s }
 
+func int32Ptr(i int32) *int32 { return &i }
+
 func newValidatingWebhookConfiguration(hooks []admissionregistration.Webhook) *admissionregistration.ValidatingWebhookConfiguration {
 	return &admissionregistration.ValidatingWebhookConfiguration{
 		ObjectMeta: metav1.ObjectMeta{
@@ -544,6 +546,63 @@ func TestValidateValidatingWebhookConfiguration(t *testing.T) {
 				}),
 			expectedError: `clientConfig.service.path: Invalid value: "/apis/foo.bar/v1alpha1/--bad": segment[3]: a DNS-1123 subdomain`,
 		},
+		{
+			name: "timeout seconds cannot be greater than 30",
+			config: newValidatingWebhookConfiguration(
+				[]admissionregistration.Webhook{
+					{
+						Name:           "webhook.k8s.io",
+						ClientConfig:   validClientConfig,
+						TimeoutSeconds: int32Ptr(31),
+					},
+				}),
+			expectedError: `webhooks[0].timeoutSeconds: Invalid value: 31: the timeout value must be between 1 and 30 seconds`,
+		},
+		{
+			name: "timeout seconds cannot be smaller than 1",
+			config: newValidatingWebhookConfiguration(
+				[]admissionregistration.Webhook{
+					{
+						Name:           "webhook.k8s.io",
+						ClientConfig:   validClientConfig,
+						TimeoutSeconds: int32Ptr(0),
+					},
+				}),
+			expectedError: `webhooks[0].timeoutSeconds: Invalid value: 0: the timeout value must be between 1 and 30 seconds`,
+		},
+		{
+			name: "timeout seconds must be positive",
+			config: newValidatingWebhookConfiguration(
+				[]admissionregistration.Webhook{
+					{
+						Name:           "webhook.k8s.io",
+						ClientConfig:   validClientConfig,
+						TimeoutSeconds: int32Ptr(-1),
+					},
+				}),
+			expectedError: `webhooks[0].timeoutSeconds: Invalid value: -1: the timeout value must be between 1 and 30 seconds`,
+		},
+		{
+			name: "valid timeout seconds",
+			config: newValidatingWebhookConfiguration(
+				[]admissionregistration.Webhook{
+					{
+						Name:           "webhook.k8s.io",
+						ClientConfig:   validClientConfig,
+						TimeoutSeconds: int32Ptr(1),
+					},
+					{
+						Name:           "webhook2.k8s.io",
+						ClientConfig:   validClientConfig,
+						TimeoutSeconds: int32Ptr(15),
+					},
+					{
+						Name:           "webhook3.k8s.io",
+						ClientConfig:   validClientConfig,
+						TimeoutSeconds: int32Ptr(30),
+					},
+				}),
+		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {

pkg/apis/admissionregistration/zz_generated.deepcopy.go

@@ -257,6 +257,11 @@ func (in *Webhook) DeepCopyInto(out *Webhook) {
 		*out = new(SideEffectClass)
 		**out = **in
 	}
+	if in.TimeoutSeconds != nil {
+		in, out := &in.TimeoutSeconds, &out.TimeoutSeconds
+		*out = new(int32)
+		**out = **in
+	}
 	return
 }
 

staging/src/k8s.io/api/admissionregistration/v1beta1/generated.proto

@@ -217,6 +217,14 @@ message Webhook {
   // sideEffects == Unknown or Some. Defaults to Unknown.
   // +optional
   optional string sideEffects = 6;
+
+  // TimeoutSeconds specifies the timeout for this webhook. After the timeout passes,
+  // the webhook call will be ignored or the API call will fail based on the
+  // failure policy.
+  // The timeout value must be between 1 and 30 seconds.
+  // Default to 30 seconds.
+  // +optional
+  optional int32 timeoutSeconds = 7;
 }
 
 // WebhookClientConfig contains the information to make a TLS

staging/src/k8s.io/api/admissionregistration/v1beta1/types.go

@@ -216,6 +216,14 @@ type Webhook struct {
 	// sideEffects == Unknown or Some. Defaults to Unknown.
 	// +optional
 	SideEffects *SideEffectClass `json:"sideEffects,omitempty" protobuf:"bytes,6,opt,name=sideEffects,casttype=SideEffectClass"`
+
+	// TimeoutSeconds specifies the timeout for this webhook. After the timeout passes,
+	// the webhook call will be ignored or the API call will fail based on the
+	// failure policy.
+	// The timeout value must be between 1 and 30 seconds.
+	// Default to 30 seconds.
+	// +optional
+	TimeoutSeconds *int32 `json:"timeoutSeconds,omitempty" protobuf:"varint,7,opt,name=timeoutSeconds"`
 }
 
 // RuleWithOperations is a tuple of Operations and Resources. It is recommended to make

staging/src/k8s.io/api/admissionregistration/v1beta1/types_swagger_doc_generated.go

@@ -106,6 +106,7 @@ var map_Webhook = map[string]string{
 	"failurePolicy":     "FailurePolicy defines how unrecognized errors from the admission endpoint are handled - allowed values are Ignore or Fail. Defaults to Ignore.",
 	"namespaceSelector": "NamespaceSelector decides whether to run the webhook on an object based on whether the namespace for that object matches the selector. If the object itself is a namespace, the matching is performed on object.metadata.labels. If the object is another cluster scoped resource, it never skips the webhook.\n\nFor example, to run the webhook on any objects whose namespace is not associated with \"runlevel\" of \"0\" or \"1\";  you will set the selector as follows: \"namespaceSelector\": {\n  \"matchExpressions\": [\n    {\n      \"key\": \"runlevel\",\n      \"operator\": \"NotIn\",\n      \"values\": [\n        \"0\",\n        \"1\"\n      ]\n    }\n  ]\n}\n\nIf instead you want to only run the webhook on any objects whose namespace is associated with the \"environment\" of \"prod\" or \"staging\"; you will set the selector as follows: \"namespaceSelector\": {\n  \"matchExpressions\": [\n    {\n      \"key\": \"environment\",\n      \"operator\": \"In\",\n      \"values\": [\n        \"prod\",\n        \"staging\"\n      ]\n    }\n  ]\n}\n\nSee https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/ for more examples of label selectors.\n\nDefault to the empty LabelSelector, which matches everything.",
 	"sideEffects":       "SideEffects states whether this webhookk has side effects. Acceptable values are: Unknown, None, Some, NoneOnDryRun Webhooks with side effects MUST implement a reconciliation system, since a request may be rejected by a future step in the admission change and the side effects therefore need to be undone. Requests with the dryRun attribute will be auto-rejected if they match a webhook with sideEffects == Unknown or Some. Defaults to Unknown.",
+	"timeoutSeconds":    "TimeoutSeconds specifies the timeout for this webhook. After the timeout passes, the webhook call will be ignored or the API call will fail based on the failure policy. The timeout value must be between 1 and 30 seconds. Default to 30 seconds.",
 }
 
 func (Webhook) SwaggerDoc() map[string]string {

staging/src/k8s.io/api/admissionregistration/v1beta1/zz_generated.deepcopy.go

@@ -257,6 +257,11 @@ func (in *Webhook) DeepCopyInto(out *Webhook) {
 		*out = new(SideEffectClass)
 		**out = **in
 	}
+	if in.TimeoutSeconds != nil {
+		in, out := &in.TimeoutSeconds, &out.TimeoutSeconds
+		*out = new(int32)
+		**out = **in
+	}
 	return
 }
 

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go

@@ -101,7 +101,11 @@ func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, h *v1beta
 		return &webhook.ErrCallingWebhook{WebhookName: h.Name, Reason: err}
 	}
 	response := &admissionv1beta1.AdmissionReview{}
-	if err := client.Post().Context(ctx).Body(&request).Do().Into(response); err != nil {
+	r := client.Post().Context(ctx).Body(&request)
+	if h.TimeoutSeconds != nil {
+		r = r.Timeout(time.Duration(*h.TimeoutSeconds) * time.Second)
+	}
+	if err := r.Do().Into(response); err != nil {
 		return &webhook.ErrCallingWebhook{WebhookName: h.Name, Reason: err}
 	}
 

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go

@@ -115,7 +115,11 @@ func (d *validatingDispatcher) callHook(ctx context.Context, h *v1beta1.Webhook,
 		return &webhook.ErrCallingWebhook{WebhookName: h.Name, Reason: err}
 	}
 	response := &admissionv1beta1.AdmissionReview{}
-	if err := client.Post().Context(ctx).Body(&request).Do().Into(response); err != nil {
+	r := client.Post().Context(ctx).Body(&request)
+	if h.TimeoutSeconds != nil {
+		r = r.Timeout(time.Duration(*h.TimeoutSeconds) * time.Second)
+	}
+	if err := r.Do().Into(response); err != nil {
 		return &webhook.ErrCallingWebhook{WebhookName: h.Name, Reason: err}
 	}
 

test/e2e/apimachinery/webhook.go

@@ -64,6 +64,7 @@ const (
 	dummyValidatingWebhookConfigName       = "e2e-test-dummy-validating-webhook-config"
 	dummyMutatingWebhookConfigName         = "e2e-test-dummy-mutating-webhook-config"
 	crdWebhookConfigName                   = "e2e-test-webhook-config-crd"
+	slowWebhookConfigName                  = "e2e-test-webhook-config-slow"
 
 	skipNamespaceLabelKey   = "skip-webhook-admission"
 	skipNamespaceLabelValue = "yes"
@@ -201,6 +202,31 @@ var _ = SIGDescribe("AdmissionWebhook", func() {
 		testCRDDenyWebhook(f)
 	})
 
+	It("Should honor timeout", func() {
+		policyFail := v1beta1.Fail
+		policyIgnore := v1beta1.Ignore
+
+		By("Setting timeout (1s) shorter than webhook latency (5s)")
+		slowWebhookCleanup := registerSlowWebhook(f, context, &policyFail, int32Ptr(1))
+		testSlowWebhookTimeoutFailEarly(f)
+		slowWebhookCleanup()
+
+		By("Having no error when timeout is shorter than webhook latency and failure policy is ignore")
+		slowWebhookCleanup = registerSlowWebhook(f, context, &policyIgnore, int32Ptr(1))
+		testSlowWebhookTimeoutNoError(f)
+		slowWebhookCleanup()
+
+		By("Having no error when timeout is longer than webhook latency")
+		slowWebhookCleanup = registerSlowWebhook(f, context, &policyFail, int32Ptr(10))
+		testSlowWebhookTimeoutNoError(f)
+		slowWebhookCleanup()
+
+		By("Having no error when timeout is empty (defaulted to 10s in v1beta1)")
+		slowWebhookCleanup = registerSlowWebhook(f, context, &policyFail, nil)
+		testSlowWebhookTimeoutNoError(f)
+		slowWebhookCleanup()
+	})
+
 	// TODO: add more e2e tests for mutating webhooks
 	// 1. mutating webhook that mutates pod
 	// 2. mutating webhook that sends empty patch
@@ -357,6 +383,8 @@ func deployWebhookAndService(f *framework.Framework, image string, context *cert
 
 func strPtr(s string) *string { return &s }
 
+func int32Ptr(i int32) *int32 { return &i }
+
 func registerWebhook(f *framework.Framework, context *certContext) func() {
 	client := f.ClientSet
 	By("Registering the webhook via the AdmissionRegistration API")
@@ -1445,3 +1473,69 @@ func testCRDDenyWebhook(f *framework.Framework) {
 		framework.Failf("expect error contains %q, got %q", expectedErrMsg, err.Error())
 	}
 }
+
+func registerSlowWebhook(f *framework.Framework, context *certContext, policy *v1beta1.FailurePolicyType, timeout *int32) func() {
+	client := f.ClientSet
+	By("Registering slow webhook via the AdmissionRegistration API")
+
+	namespace := f.Namespace.Name
+	configName := slowWebhookConfigName
+
+	_, err := client.AdmissionregistrationV1beta1().ValidatingWebhookConfigurations().Create(&v1beta1.ValidatingWebhookConfiguration{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: configName,
+		},
+		Webhooks: []v1beta1.Webhook{
+			{
+				Name: "allow-configmap-with-delay-webhook.k8s.io",
+				Rules: []v1beta1.RuleWithOperations{{
+					Operations: []v1beta1.OperationType{v1beta1.Create},
+					Rule: v1beta1.Rule{
+						APIGroups:   []string{""},
+						APIVersions: []string{"v1"},
+						Resources:   []string{"configmaps"},
+					},
+				}},
+				ClientConfig: v1beta1.WebhookClientConfig{
+					Service: &v1beta1.ServiceReference{
+						Namespace: namespace,
+						Name:      serviceName,
+						Path:      strPtr("/always-allow-delay-5s"),
+					},
+					CABundle: context.signingCert,
+				},
+				FailurePolicy:  policy,
+				TimeoutSeconds: timeout,
+			},
+		},
+	})
+	framework.ExpectNoError(err, "registering slow webhook config %s with namespace %s", configName, namespace)
+
+	// The webhook configuration is honored in 10s.
+	time.Sleep(10 * time.Second)
+
+	return func() {
+		client.AdmissionregistrationV1beta1().ValidatingWebhookConfigurations().Delete(configName, nil)
+	}
+}
+
+func testSlowWebhookTimeoutFailEarly(f *framework.Framework) {
+	By("Request fails when timeout (1s) is shorter than slow webhook latency (5s)")
+	client := f.ClientSet
+	name := "e2e-test-slow-webhook-configmap"
+	_, err := client.CoreV1().ConfigMaps(f.Namespace.Name).Create(&v1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: name}})
+	Expect(err).To(HaveOccurred(), "create configmap in namespace %s should have timed-out reaching slow webhook", f.Namespace.Name)
+	expectedErrMsg := `/always-allow-delay-5s?timeout=1s: context deadline exceeded`
+	if !strings.Contains(err.Error(), expectedErrMsg) {
+		framework.Failf("expect error contains %q, got %q", expectedErrMsg, err.Error())
+	}
+}
+
+func testSlowWebhookTimeoutNoError(f *framework.Framework) {
+	client := f.ClientSet
+	name := "e2e-test-slow-webhook-configmap"
+	_, err := client.CoreV1().ConfigMaps(f.Namespace.Name).Create(&v1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: name}})
+	Expect(err).To(BeNil())
+	err = client.CoreV1().ConfigMaps(f.Namespace.Name).Delete(name, &metav1.DeleteOptions{})
+	Expect(err).To(BeNil())
+}

test/images/webhook/BUILD

@@ -4,6 +4,7 @@ go_library(
     name = "go_default_library",
     srcs = [
         "addlabel.go",
+        "alwaysallow.go",
         "alwaysdeny.go",
         "config.go",
         "configmap.go",

test/images/webhook/VERSION

-1.13v1
+1.14v1

test/images/webhook/alwaysallow.go

+/*
+Copyright 2019 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package main
+
+import (
+	"time"
+
+	"k8s.io/api/admission/v1beta1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/klog"
+)
+
+// alwaysAllowDelayFiveSeconds sleeps for five seconds and allows all requests made to this function.
+func alwaysAllowDelayFiveSeconds(ar v1beta1.AdmissionReview) *v1beta1.AdmissionResponse {
+	klog.V(2).Info("always-allow-with-delay sleeping for 5 seconds")
+	time.Sleep(5 * time.Second)
+	klog.V(2).Info("calling always-allow")
+	reviewResponse := v1beta1.AdmissionResponse{}
+	reviewResponse.Allowed = true
+	reviewResponse.Result = &metav1.Status{Message: "this webhook allows all requests"}
+	return &reviewResponse
+}

test/images/webhook/main.go

@@ -91,6 +91,10 @@ func serve(w http.ResponseWriter, r *http.Request, admit admitFunc) {
 	}
 }
 
+func serveAlwaysAllowDelayFiveSeconds(w http.ResponseWriter, r *http.Request) {
+	serve(w, r, alwaysAllowDelayFiveSeconds)
+}
+
 func serveAlwaysDeny(w http.ResponseWriter, r *http.Request) {
 	serve(w, r, alwaysDeny)
 }
@@ -132,10 +136,12 @@ func serveCRD(w http.ResponseWriter, r *http.Request) {
 }
 
 func main() {
+	klog.InitFlags(nil)
 	var config Config
 	config.addFlags()
 	flag.Parse()
 
+	http.HandleFunc("/always-allow-delay-5s", serveAlwaysAllowDelayFiveSeconds)
 	http.HandleFunc("/always-deny", serveAlwaysDeny)
 	http.HandleFunc("/add-label", serveAddLabel)
 	http.HandleFunc("/pods", servePods)

test/utils/image/manifest.go

@@ -93,7 +93,7 @@ var (
 // Preconfigured image configs
 var (
 	CRDConversionWebhook     = Config{e2eRegistry, "crd-conversion-webhook", "1.13rev2"}
-	AdmissionWebhook         = Config{e2eRegistry, "webhook", "1.13v1"}
+	AdmissionWebhook         = Config{e2eRegistry, "webhook", "1.14v1"}
 	APIServer                = Config{e2eRegistry, "sample-apiserver", "1.10"}
 	AppArmorLoader           = Config{e2eRegistry, "apparmor-loader", "1.0"}
 	BusyBox                  = Config{dockerLibraryRegistry, "busybox", "1.29"}