Flake fix: poll for webhook registration to complete in reinvocation integration tests

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/BUILD

-load("@io_bazel_rules_go//go:def.bzl", "go_library")
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
     name = "go_default_library",
@@ -40,3 +40,14 @@ filegroup(
     tags = ["automanaged"],
     visibility = ["//visibility:public"],
 )
+
+go_test(
+    name = "go_default_test",
+    srcs = ["accessors_test.go"],
+    embed = [":go_default_library"],
+    deps = [
+        "//staging/src/k8s.io/api/admissionregistration/v1beta1:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library",
+        "//vendor/github.com/google/gofuzz:go_default_library",
+    ],
+)

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

@@ -66,7 +66,7 @@ type mutatingWebhookAccessor struct {
 }
 
 func (m mutatingWebhookAccessor) GetUID() string {
-	return m.Name
+	return m.uid
 }
 func (m mutatingWebhookAccessor) GetName() string {
 	return m.Name

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors_test.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 webhook
+
+import (
+	"fmt"
+	"reflect"
+	"testing"
+
+	fuzz "github.com/google/gofuzz"
+	"k8s.io/api/admissionregistration/v1beta1"
+	"k8s.io/apimachinery/pkg/util/diff"
+)
+
+func TestMutatingWebhookAccessor(t *testing.T) {
+	f := fuzz.New()
+	for i := 0; i < 100; i++ {
+		t.Run(fmt.Sprintf("Run %d/100", i), func(t *testing.T) {
+			orig := &v1beta1.MutatingWebhook{}
+			f.Fuzz(orig)
+
+			// zero out any accessor type specific fields not included in the accessor
+			orig.ReinvocationPolicy = nil
+
+			uid := fmt.Sprintf("test.configuration.admission/%s/0", orig.Name)
+			accessor := NewMutatingWebhookAccessor(uid, orig)
+			if uid != accessor.GetUID() {
+				t.Errorf("expected GetUID to return %s, but got %s", accessor.GetUID(), uid)
+			}
+			m, ok := accessor.GetMutatingWebhook()
+			if !ok {
+				t.Errorf("expected GetMutatingWebhook to return ok for mutating webhook accessor")
+			}
+			if !reflect.DeepEqual(orig, m) {
+				t.Errorf("expected GetMutatingWebhook to return original webhook, diff:\n%s", diff.ObjectReflectDiff(orig, m))
+			}
+			if _, ok := accessor.GetValidatingWebhook(); ok {
+				t.Errorf("expected GetValidatingWebhook to be nil for mutating webhook accessor")
+			}
+			copy := &v1beta1.MutatingWebhook{
+				Name:                    accessor.GetName(),
+				ClientConfig:            accessor.GetClientConfig(),
+				Rules:                   accessor.GetRules(),
+				FailurePolicy:           accessor.GetFailurePolicy(),
+				MatchPolicy:             accessor.GetMatchPolicy(),
+				NamespaceSelector:       accessor.GetNamespaceSelector(),
+				ObjectSelector:          accessor.GetObjectSelector(),
+				SideEffects:             accessor.GetSideEffects(),
+				TimeoutSeconds:          accessor.GetTimeoutSeconds(),
+				AdmissionReviewVersions: accessor.GetAdmissionReviewVersions(),
+			}
+			if !reflect.DeepEqual(orig, copy) {
+				t.Errorf("expected mutatingWebhook to round trip through WebhookAccessor, diff:\n%s", diff.ObjectReflectDiff(orig, copy))
+			}
+		})
+	}
+}
+
+func TestValidatingWebhookAccessor(t *testing.T) {
+	f := fuzz.New()
+	for i := 0; i < 100; i++ {
+		t.Run(fmt.Sprintf("Run %d/100", i), func(t *testing.T) {
+			orig := &v1beta1.ValidatingWebhook{}
+			f.Fuzz(orig)
+			uid := fmt.Sprintf("test.configuration.admission/%s/0", orig.Name)
+			accessor := NewValidatingWebhookAccessor(uid, orig)
+			if uid != accessor.GetUID() {
+				t.Errorf("expected GetUID to return %s, but got %s", accessor.GetUID(), uid)
+			}
+			m, ok := accessor.GetValidatingWebhook()
+			if !ok {
+				t.Errorf("expected GetValidatingWebhook to return ok for validating webhook accessor")
+			}
+			if !reflect.DeepEqual(orig, m) {
+				t.Errorf("expected GetValidatingWebhook to return original webhook, diff:\n%s", diff.ObjectReflectDiff(orig, m))
+			}
+			if _, ok := accessor.GetMutatingWebhook(); ok {
+				t.Errorf("expected GetMutatingWebhook to be nil for validating webhook accessor")
+			}
+			copy := &v1beta1.ValidatingWebhook{
+				Name:                    accessor.GetName(),
+				ClientConfig:            accessor.GetClientConfig(),
+				Rules:                   accessor.GetRules(),
+				FailurePolicy:           accessor.GetFailurePolicy(),
+				MatchPolicy:             accessor.GetMatchPolicy(),
+				NamespaceSelector:       accessor.GetNamespaceSelector(),
+				ObjectSelector:          accessor.GetObjectSelector(),
+				SideEffects:             accessor.GetSideEffects(),
+				TimeoutSeconds:          accessor.GetTimeoutSeconds(),
+				AdmissionReviewVersions: accessor.GetAdmissionReviewVersions(),
+			}
+			if !reflect.DeepEqual(orig, copy) {
+				t.Errorf("expected validatingWebhook to round trip through WebhookAccessor, diff:\n%s", diff.ObjectReflectDiff(orig, copy))
+			}
+		})
+	}
+}

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

@@ -17,6 +17,7 @@ limitations under the License.
 package mutating
 
 import (
+	"fmt"
 	"net/url"
 	"reflect"
 	"strings"
@@ -114,7 +115,7 @@ func TestAdmit(t *testing.T) {
 			reinvocationCtx := fakeAttr.Attributes.GetReinvocationContext()
 			reinvocationCtx.SetIsReinvoke()
 			for webhook, expectReinvoke := range tt.ExpectReinvokeWebhooks {
-				shouldReinvoke := reinvocationCtx.Value(PluginName).(*webhookReinvokeContext).ShouldReinvokeWebhook(webhook)
+				shouldReinvoke := reinvocationCtx.Value(PluginName).(*webhookReinvokeContext).ShouldReinvokeWebhook(fmt.Sprintf("test-webhooks/%s/0", webhook))
 				if expectReinvoke != shouldReinvoke {
 					t.Errorf("expected reinvocationContext.ShouldReinvokeWebhook(%s)=%t, but got %t", webhook, expectReinvoke, shouldReinvoke)
 				}

test/integration/apiserver/admissionwebhook/reinvocation_test.go

@@ -28,6 +28,7 @@ import (
 	"strings"
 	"sync"
 	"testing"
+	"time"
 
 	"k8s.io/api/admission/v1beta1"
 	admissionv1beta1 "k8s.io/api/admissionregistration/v1beta1"
@@ -36,6 +37,8 @@ import (
 	v1 "k8s.io/api/core/v1"
 	schedulingv1 "k8s.io/api/scheduling/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/wait"
 	clientset "k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/rest"
 	kubeapiservertesting "k8s.io/kubernetes/cmd/kube-apiserver/app/testing"
@@ -194,17 +197,68 @@ func TestWebhookReinvocationPolicy(t *testing.T) {
 		}
 	}
 
+	_, err = client.CoreV1().Pods("default").Create(reinvocationMarkerFixture)
+	if err != nil {
+		t.Fatal(err)
+	}
+
 	for i, tt := range testCases {
 		t.Run(tt.name, func(t *testing.T) {
-			recorder.Reset()
+			upCh := recorder.Reset()
 			ns := fmt.Sprintf("reinvoke-%d", i)
 			_, err = client.CoreV1().Namespaces().Create(&v1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}})
 			if err != nil {
 				t.Fatal(err)
 			}
 
-			for i, webhook := range tt.webhooks {
-				defer registerWebhook(t, client, fmt.Sprintf("admission.integration.test%d", i), webhookServer.URL+webhook.path, webhook.policy, webhook.objectSelector)()
+			webhooks := []admissionv1beta1.MutatingWebhook{}
+			for j, webhook := range tt.webhooks {
+				name := fmt.Sprintf("admission.integration.test.%d.%s", j, strings.TrimPrefix(webhook.path, "/"))
+				fail := admissionv1beta1.Fail
+				endpoint := webhookServer.URL + webhook.path
+				webhooks = append(webhooks, admissionv1beta1.MutatingWebhook{
+					Name: name,
+					ClientConfig: admissionv1beta1.WebhookClientConfig{
+						URL:      &endpoint,
+						CABundle: localhostCert,
+					},
+					Rules: []admissionv1beta1.RuleWithOperations{{
+						Operations: []admissionv1beta1.OperationType{admissionv1beta1.OperationAll},
+						Rule:       admissionv1beta1.Rule{APIGroups: []string{""}, APIVersions: []string{"v1"}, Resources: []string{"pods"}},
+					}},
+					ObjectSelector:          webhook.objectSelector,
+					FailurePolicy:           &fail,
+					ReinvocationPolicy:      webhook.policy,
+					AdmissionReviewVersions: []string{"v1beta1"},
+				})
+			}
+
+			cfg, err := client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Create(&admissionv1beta1.MutatingWebhookConfiguration{
+				ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("admission.integration.test-%d", i)},
+				Webhooks:   webhooks,
+			})
+			if err != nil {
+				t.Fatal(err)
+			}
+			defer func() {
+				err := client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Delete(cfg.GetName(), &metav1.DeleteOptions{})
+				if err != nil {
+					t.Fatal(err)
+				}
+			}()
+
+			// wait until new webhook is called the first time
+			if err := wait.PollImmediate(time.Millisecond*5, wait.ForeverTestTimeout, func() (bool, error) {
+				_, err = client.CoreV1().Pods("default").Patch(reinvocationMarkerFixture.Name, types.JSONPatchType, []byte("[]"))
+				select {
+				case <-upCh:
+					return true, nil
+				default:
+					t.Logf("Waiting for webhook to become effective, getting marker object: %v", err)
+					return false, nil
+				}
+			}); err != nil {
+				t.Fatal(err)
 			}
 
 			pod := &corev1.Pod{
@@ -259,48 +313,30 @@ func TestWebhookReinvocationPolicy(t *testing.T) {
 	}
 }
 
-func registerWebhook(t *testing.T, client clientset.Interface, name, endpoint string, reinvocationPolicy *registrationv1beta1.ReinvocationPolicyType, objectSelector *metav1.LabelSelector) func() {
-	fail := admissionv1beta1.Fail
-	hook, err := client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Create(&admissionv1beta1.MutatingWebhookConfiguration{
-		ObjectMeta: metav1.ObjectMeta{Name: name},
-		Webhooks: []admissionv1beta1.MutatingWebhook{{
-			Name: name,
-			ClientConfig: admissionv1beta1.WebhookClientConfig{
-				URL:      &endpoint,
-				CABundle: localhostCert,
-			},
-			Rules: []admissionv1beta1.RuleWithOperations{{
-				Operations: []admissionv1beta1.OperationType{admissionv1beta1.OperationAll},
-				Rule:       admissionv1beta1.Rule{APIGroups: []string{"*"}, APIVersions: []string{"*"}, Resources: []string{"*/*"}},
-			}},
-			ObjectSelector:          objectSelector,
-			FailurePolicy:           &fail,
-			ReinvocationPolicy:      reinvocationPolicy,
-			AdmissionReviewVersions: []string{"v1beta1"},
-		}},
-	})
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	tearDown := func() {
-		err := client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Delete(hook.GetName(), &metav1.DeleteOptions{})
-		if err != nil {
-			t.Fatal(err)
-		}
-	}
-	return tearDown
-}
-
 type invocationRecorder struct {
 	mu     sync.Mutex
+	upCh   chan struct{}
+	upOnce sync.Once
 	counts map[string]int
 }
 
-func (i *invocationRecorder) Reset() {
+// Reset zeros out all counts and returns a channel that is closed when the first admission of the
+// marker object is received.
+func (i *invocationRecorder) Reset() chan struct{} {
 	i.mu.Lock()
 	defer i.mu.Unlock()
 	i.counts = map[string]int{}
+	i.upCh = make(chan struct{})
+	i.upOnce = sync.Once{}
+	return i.upCh
+}
+
+func (i *invocationRecorder) MarkerReceived() {
+	i.mu.Lock()
+	defer i.mu.Unlock()
+	i.upOnce.Do(func() {
+		close(i.upCh)
+	})
 }
 
 func (i *invocationRecorder) GetCount(path string) int {
@@ -359,6 +395,14 @@ func newReinvokeWebhookHandler(recorder *invocationRecorder) http.Handler {
 			http.Error(w, err.Error(), 400)
 		}
 
+		// When resetting between tests, a marker object is patched until this webhook
+		// observes it, at which point it is considered ready.
+		if pod.Namespace == reinvocationMarkerFixture.Namespace && pod.Name == reinvocationMarkerFixture.Name {
+			recorder.MarkerReceived()
+			allow(w)
+			return
+		}
+
 		recorder.IncrementCount(r.URL.Path)
 
 		switch r.URL.Path {
@@ -399,3 +443,16 @@ func newReinvokeWebhookHandler(recorder *invocationRecorder) http.Handler {
 		}
 	})
 }
+
+var reinvocationMarkerFixture = &corev1.Pod{
+	ObjectMeta: metav1.ObjectMeta{
+		Namespace: "default",
+		Name:      "marker",
+	},
+	Spec: corev1.PodSpec{
+		Containers: []v1.Container{{
+			Name:  "fake-name",
+			Image: "fakeimage",
+		}},
+	},
+}