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")) } 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 99202e2009f..063533fa2a5 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()