From e51320f69d92e4d08bc25eec5a4b7a58d23184ab Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Tue, 4 Jun 2019 14:19:26 -0700 Subject: [PATCH] Flake fix: poll for webhook registration to complete in reinvocation integration tests --- .../pkg/admission/plugin/webhook/BUILD | 13 +- .../pkg/admission/plugin/webhook/accessors.go | 2 +- .../plugin/webhook/accessors_test.go | 111 +++++++++++++++ .../plugin/webhook/mutating/plugin_test.go | 3 +- .../admissionwebhook/reinvocation_test.go | 131 +++++++++++++----- 5 files changed, 220 insertions(+), 40 deletions(-) create mode 100644 staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors_test.go diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/BUILD b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/BUILD index c7d1cfa4556..7bfb60fc731 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/BUILD @@ -1,4 +1,4 @@ -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", + ], +) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go index b44c72ebfc9..f0cbf4f338e 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go +++ b/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 diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors_test.go new file mode 100644 index 00000000000..fb6338de3c9 --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors_test.go @@ -0,0 +1,111 @@ +/* +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)) + } + }) + } +} diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/plugin_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/plugin_test.go index b618e309149..db178eeca23 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/plugin_test.go +++ b/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) } diff --git a/test/integration/apiserver/admissionwebhook/reinvocation_test.go b/test/integration/apiserver/admissionwebhook/reinvocation_test.go index 417f616eca1..8da030e63e2 100644 --- a/test/integration/apiserver/admissionwebhook/reinvocation_test.go +++ b/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", + }}, + }, +}