From df315f347c911c5cc189d14f6dc70a23da52e57d Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Tue, 8 Nov 2022 13:07:42 -0800 Subject: [PATCH 1/2] use existing admissionHandler readyfunc to wait for sync is what other plugins do, and should decrease verbosity in logs --- .../pkg/admission/plugin/cel/admission.go | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/admission.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/admission.go index 405cc11b5a1..a2e14542400 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/admission.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/admission.go @@ -21,17 +21,16 @@ import ( "errors" "fmt" "io" + "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apiserver/pkg/features" "k8s.io/client-go/dynamic" "k8s.io/component-base/featuregate" - "time" "k8s.io/apiserver/pkg/admission" "k8s.io/apiserver/pkg/admission/initializer" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes" - "k8s.io/client-go/tools/cache" ) //////////////////////////////////////////////////////////////////////////////// @@ -60,6 +59,7 @@ func Register(plugins *admission.Plugins) { //////////////////////////////////////////////////////////////////////////////// type celAdmissionPlugin struct { + *admission.Handler evaluator CELPolicyEvaluator inspectedFeatureGates bool @@ -83,8 +83,9 @@ var _ admission.InitializationValidator = &celAdmissionPlugin{} var _ admission.ValidationInterface = &celAdmissionPlugin{} func NewPlugin() (admission.Interface, error) { - result := &celAdmissionPlugin{} - return result, nil + return &celAdmissionPlugin{ + Handler: admission.NewHandler(admission.Connect, admission.Create, admission.Delete, admission.Update), + }, nil } func (c *celAdmissionPlugin) SetExternalKubeInformerFactory(f informers.SharedInformerFactory) { @@ -142,6 +143,7 @@ func (c *celAdmissionPlugin) ValidateInitialization() error { return err } + c.SetReadyFunc(c.evaluator.HasSynced) go c.evaluator.Run(c.stopCh) return nil } @@ -163,16 +165,13 @@ func (c *celAdmissionPlugin) Validate( return nil } - deadlined, cancel := context.WithTimeout(ctx, 2*time.Second) - defer cancel() - // isPolicyResource determines if an admission.Attributes object is describing // the admission of a ValidatingAdmissionPolicy or a ValidatingAdmissionPolicyBinding if isPolicyResource(a) { return } - if !cache.WaitForNamedCacheSync("cel-admission-plugin", deadlined.Done(), c.evaluator.HasSynced) { + if !c.WaitForReady() { return admission.NewForbidden(a, fmt.Errorf("not yet ready to handle request")) } From acf571fcbed6e762a2a654bfbe6c415e668dfed3 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Wed, 9 Nov 2022 15:28:37 -0800 Subject: [PATCH 2/2] add test for error when informers are not ready --- .../admission/plugin/cel/admission_test.go | 53 +++++++++++++++++-- 1 file changed, 48 insertions(+), 5 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/admission_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/admission_test.go index 75c79cc97d4..a610cda68c5 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/admission_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/admission_test.go @@ -227,7 +227,7 @@ func (f *fakeCompiler) RegisterBinding(binding *v1alpha1.ValidatingAdmissionPoli } func setupFakeTest(t *testing.T, comp *fakeCompiler) (plugin admission.ValidationInterface, paramTracker, policyTracker clienttesting.ObjectTracker, controller *celAdmissionController) { - return setupTestCommon(t, comp) + return setupTestCommon(t, comp, true) } // Starts CEL admission controller and sets up a plugin configured with it as well @@ -237,7 +237,9 @@ func setupFakeTest(t *testing.T, comp *fakeCompiler) (plugin admission.Validatio // support multiple types of params this function needs to be augmented // // PolicyTracker expects FakePolicyDefinition and FakePolicyBinding types -func setupTestCommon(t *testing.T, compiler ValidatorCompiler) (plugin admission.ValidationInterface, paramTracker, policyTracker clienttesting.ObjectTracker, controller *celAdmissionController) { +// !TODO: refactor this test/framework to remove startInformers argument and +// clean up the return args, and in general make it more accessible. +func setupTestCommon(t *testing.T, compiler ValidatorCompiler, shouldStartInformers bool) (plugin admission.ValidationInterface, paramTracker, policyTracker clienttesting.ObjectTracker, controller *celAdmissionController) { testContext, testContextCancel := context.WithCancel(context.Background()) t.Cleanup(testContextCancel) @@ -259,7 +261,11 @@ func setupTestCommon(t *testing.T, compiler ValidatorCompiler) (plugin admission panic("Unexpected error.") } - handler := &celAdmissionPlugin{enabled: true} + plug, err := NewPlugin() + require.NoError(t, err) + + handler := plug.(*celAdmissionPlugin) + handler.enabled = true genericInitializer := initializer.New(fakeClient, dynamicClient, fakeInformerFactory, nil, featureGate, testContext.Done()) genericInitializer.Initialize(handler) @@ -272,14 +278,19 @@ func setupTestCommon(t *testing.T, compiler ValidatorCompiler) (plugin admission controller = handler.evaluator.(*celAdmissionController) controller.validatorCompiler = compiler - // Make sure to start the fake informers - fakeInformerFactory.Start(testContext.Done()) t.Cleanup(func() { testContextCancel() // wait for informer factory to shutdown fakeInformerFactory.Shutdown() }) + if !shouldStartInformers { + return handler, dynamicClient.Tracker(), fakeClient.Tracker(), controller + } + + // Make sure to start the fake informers + fakeInformerFactory.Start(testContext.Done()) + // Wait for admission controller to begin its object watches // This is because there is a very rare (0.05% on my machine) race doing the // initial List+Watch if an object is added after the list, but before the @@ -532,6 +543,38 @@ func must3[T any, I any](val T, _ I, err error) T { // Functionality Tests //////////////////////////////////////////////////////////////////////////////// +func TestPluginNotReady(t *testing.T) { + compiler := &fakeCompiler{ + // Match everything by default + DefaultMatch: true, + } + + // Show that an unstarted informer (or one that has failed its listwatch) + // will show proper error from plugin + handler, _, _, _ := setupTestCommon(t, compiler, false) + err := handler.Validate( + context.Background(), + // Object is irrelevant/unchecked for this test. Just test that + // the evaluator is executed, and returns a denial + attributeRecord(nil, fakeParams, admission.Create), + &admission.RuntimeObjectInterfaces{}, + ) + + require.ErrorContains(t, err, "not yet ready to handle request") + + // Show that by now starting the informer, the error is dissipated + handler, _, _, _ = setupTestCommon(t, compiler, true) + err = handler.Validate( + context.Background(), + // Object is irrelevant/unchecked for this test. Just test that + // the evaluator is executed, and returns a denial + attributeRecord(nil, fakeParams, admission.Create), + &admission.RuntimeObjectInterfaces{}, + ) + + require.NoError(t, err) +} + func TestBasicPolicyDefinitionFailure(t *testing.T) { testContext, testContextCancel := context.WithCancel(context.Background()) defer testContextCancel()