Merge pull request #115145 from alexzielenski/apiserver/policy/typed-params

ValidatingAdmissionPolicy: use natively-typed informer for params

staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go

@@ -26,6 +26,7 @@ import (
 
 	"github.com/stretchr/testify/require"
 	"k8s.io/api/admissionregistration/v1alpha1"
+	v1 "k8s.io/api/core/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -59,6 +60,11 @@ var (
 		if err := v1alpha1.AddToScheme(res); err != nil {
 			panic(err)
 		}
+
+		if err := fake.AddToScheme(res); err != nil {
+			panic(err)
+		}
+
 		return res
 	}()
 	paramsGVK schema.GroupVersionKind = schema.GroupVersionKind{
@@ -78,6 +84,7 @@ var (
 		res.Add(paramsGVK, meta.RESTScopeNamespace)
 		res.Add(definitionGVK, meta.RESTScopeRoot)
 		res.Add(bindingGVK, meta.RESTScopeRoot)
+		res.Add(v1.SchemeGroupVersion.WithKind("ConfigMap"), meta.RESTScopeNamespace)
 		return res
 	}()
 
@@ -142,9 +149,9 @@ var (
 // So that we can test the controller without pulling in any CEL functionality
 type fakeCompiler struct {
 	DefaultMatch         bool
-	CompileFuncs         map[string]func(*v1alpha1.ValidatingAdmissionPolicy) Validator
+	CompileFuncs         map[namespacedName]func(*v1alpha1.ValidatingAdmissionPolicy) Validator
-	DefinitionMatchFuncs map[string]func(*v1alpha1.ValidatingAdmissionPolicy, admission.Attributes) bool
+	DefinitionMatchFuncs map[namespacedName]func(*v1alpha1.ValidatingAdmissionPolicy, admission.Attributes) bool
-	BindingMatchFuncs    map[string]func(*v1alpha1.ValidatingAdmissionPolicyBinding, admission.Attributes) bool
+	BindingMatchFuncs    map[namespacedName]func(*v1alpha1.ValidatingAdmissionPolicyBinding, admission.Attributes) bool
 }
 
 var _ ValidatorCompiler = &fakeCompiler{}
@@ -161,7 +168,10 @@ func (f *fakeCompiler) ValidateInitialization() error {
 // resource request
 func (f *fakeCompiler) DefinitionMatches(a admission.Attributes, o admission.ObjectInterfaces, definition *v1alpha1.ValidatingAdmissionPolicy) (bool, schema.GroupVersionKind, error) {
 	namespace, name := definition.Namespace, definition.Name
-	key := namespace + "/" + name
+	key := namespacedName{
+		name:      name,
+		namespace: namespace,
+	}
 	if fun, ok := f.DefinitionMatchFuncs[key]; ok {
 		return fun(definition, a), schema.GroupVersionKind{}, nil
 	}
@@ -174,7 +184,10 @@ func (f *fakeCompiler) DefinitionMatches(a admission.Attributes, o admission.Obj
 // resource request
 func (f *fakeCompiler) BindingMatches(a admission.Attributes, o admission.ObjectInterfaces, binding *v1alpha1.ValidatingAdmissionPolicyBinding) (bool, error) {
 	namespace, name := binding.Namespace, binding.Name
-	key := namespace + "/" + name
+	key := namespacedName{
+		name:      name,
+		namespace: namespace,
+	}
 	if fun, ok := f.BindingMatchFuncs[key]; ok {
 		return fun(binding, a), nil
 	}
@@ -187,8 +200,10 @@ func (f *fakeCompiler) Compile(
 	definition *v1alpha1.ValidatingAdmissionPolicy,
 ) Validator {
 	namespace, name := definition.Namespace, definition.Name
-
+	key := namespacedName{
-	key := namespace + "/" + name
+		name:      name,
+		namespace: namespace,
+	}
 	if fun, ok := f.CompileFuncs[key]; ok {
 		return fun(definition)
 	}
@@ -198,18 +213,21 @@ func (f *fakeCompiler) Compile(
 
 func (f *fakeCompiler) RegisterDefinition(definition *v1alpha1.ValidatingAdmissionPolicy, compileFunc func(*v1alpha1.ValidatingAdmissionPolicy) Validator, matchFunc func(*v1alpha1.ValidatingAdmissionPolicy, admission.Attributes) bool) {
 	namespace, name := definition.Namespace, definition.Name
-	key := namespace + "/" + name
+	key := namespacedName{
+		name:      name,
+		namespace: namespace,
+	}
 	if compileFunc != nil {
 
 		if f.CompileFuncs == nil {
-			f.CompileFuncs = make(map[string]func(*v1alpha1.ValidatingAdmissionPolicy) Validator)
+			f.CompileFuncs = make(map[namespacedName]func(*v1alpha1.ValidatingAdmissionPolicy) Validator)
 		}
 		f.CompileFuncs[key] = compileFunc
 	}
 
 	if matchFunc != nil {
 		if f.DefinitionMatchFuncs == nil {
-			f.DefinitionMatchFuncs = make(map[string]func(*v1alpha1.ValidatingAdmissionPolicy, admission.Attributes) bool)
+			f.DefinitionMatchFuncs = make(map[namespacedName]func(*v1alpha1.ValidatingAdmissionPolicy, admission.Attributes) bool)
 		}
 		f.DefinitionMatchFuncs[key] = matchFunc
 	}
@@ -217,11 +235,14 @@ func (f *fakeCompiler) RegisterDefinition(definition *v1alpha1.ValidatingAdmissi
 
 func (f *fakeCompiler) RegisterBinding(binding *v1alpha1.ValidatingAdmissionPolicyBinding, matchFunc func(*v1alpha1.ValidatingAdmissionPolicyBinding, admission.Attributes) bool) {
 	namespace, name := binding.Namespace, binding.Name
-	key := namespace + "/" + name
+	key := namespacedName{
+		name:      name,
+		namespace: namespace,
+	}
 
 	if matchFunc != nil {
 		if f.BindingMatchFuncs == nil {
-			f.BindingMatchFuncs = make(map[string]func(*v1alpha1.ValidatingAdmissionPolicyBinding, admission.Attributes) bool)
+			f.BindingMatchFuncs = make(map[namespacedName]func(*v1alpha1.ValidatingAdmissionPolicyBinding, admission.Attributes) bool)
 		}
 		f.BindingMatchFuncs[key] = matchFunc
 	}
@@ -374,30 +395,6 @@ func (c *celAdmissionController) getCurrentObject(obj runtime.Object) (runtime.O
 	defer c.policyController.mutex.RUnlock()
 
 	switch obj.(type) {
-	case *unstructured.Unstructured:
-		paramSourceGVK := obj.GetObjectKind().GroupVersionKind()
-		paramKind := v1alpha1.ParamKind{
-			APIVersion: paramSourceGVK.GroupVersion().String(),
-			Kind:       paramSourceGVK.Kind,
-		}
-		var paramInformer generic.Informer[*unstructured.Unstructured]
-		if paramInfo, ok := c.policyController.paramsCRDControllers[paramKind]; ok {
-			paramInformer = paramInfo.controller.Informer()
-		} else {
-			// Treat unknown CRD the same as not found
-			return nil, nil
-		}
-
-		// Param type. Just check informer for its GVK
-		item, err := paramInformer.Get(accessor.GetName())
-		if err != nil {
-			if k8serrors.IsNotFound(err) {
-				return nil, nil
-			}
-			return nil, err
-		}
-
-		return item, nil
 	case *v1alpha1.ValidatingAdmissionPolicyBinding:
 		nn := getNamespaceName(accessor.GetNamespace(), accessor.GetName())
 		info, ok := c.policyController.bindingInfos[nn]
@@ -415,7 +412,32 @@ func (c *celAdmissionController) getCurrentObject(obj runtime.Object) (runtime.O
 
 		return info.lastReconciledValue, nil
 	default:
-		panic(fmt.Errorf("unhandled object type: %T", obj))
+		// If test isn't trying to fetch a policy or binding, assume it is
+		// fetching a param
+		paramSourceGVK := obj.GetObjectKind().GroupVersionKind()
+		paramKind := v1alpha1.ParamKind{
+			APIVersion: paramSourceGVK.GroupVersion().String(),
+			Kind:       paramSourceGVK.Kind,
+		}
+
+		var paramInformer generic.Informer[runtime.Object]
+		if paramInfo, ok := c.policyController.paramsCRDControllers[paramKind]; ok {
+			paramInformer = paramInfo.controller.Informer()
+		} else {
+			// Treat unknown CRD the same as not found
+			return nil, nil
+		}
+
+		// Param type. Just check informer for its GVK
+		item, err := paramInformer.Get(accessor.GetName())
+		if err != nil {
+			if k8serrors.IsNotFound(err) {
+				return nil, nil
+			}
+			return nil, err
+		}
+
+		return item, nil
 	}
 }
 
@@ -1209,3 +1231,86 @@ func TestMultiplePoliciesSharedParamType(t *testing.T) {
 	require.EqualValues(t, 1, compiles2.Load())
 	require.EqualValues(t, 2, evaluations2.Load())
 }
+
+// Shows that we can refer to native-typed params just fine
+// (as opposed to CRD params)
+func TestNativeTypeParam(t *testing.T) {
+	testContext, testContextCancel := context.WithCancel(context.Background())
+	defer testContextCancel()
+	compiler := &fakeCompiler{
+		// Match everything by default
+		DefaultMatch: true,
+	}
+	handler, _, tracker, controller := setupFakeTest(t, compiler)
+
+	compiles := atomic.Int64{}
+	evaluations := atomic.Int64{}
+
+	// Use ConfigMap native-typed param
+	nativeTypeParamPolicy := *denyPolicy
+	nativeTypeParamPolicy.Spec.ParamKind = &v1alpha1.ParamKind{
+		APIVersion: "v1",
+		Kind:       "ConfigMap",
+	}
+
+	compiler.RegisterDefinition(&nativeTypeParamPolicy, func(vap *v1alpha1.ValidatingAdmissionPolicy) Validator {
+		compiles.Add(1)
+
+		return validatorFunc(func(a admission.Attributes, o admission.ObjectInterfaces, params runtime.Object, matchKind schema.GroupVersionKind) ([]policyDecision, error) {
+			evaluations.Add(1)
+
+			// show that the passed params was a ConfigMap native type
+			if _, ok := params.(*v1.ConfigMap); ok {
+				return []policyDecision{
+					{
+						action:  actionDeny,
+						message: "correct type",
+					},
+				}, nil
+			}
+			return []policyDecision{
+				{
+					action:  actionDeny,
+					message: "Incorrect param type",
+				},
+			}, nil
+		})
+	}, nil)
+
+	configMapParam := &v1.ConfigMap{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: "v1",
+			Kind:       "ConfigMap",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:            "replicas-test.example.com",
+			Namespace:       "",
+			ResourceVersion: "1",
+		},
+		Data: map[string]string{
+			"coolkey": "coolvalue",
+		},
+	}
+	require.NoError(t, tracker.Create(definitionsGVR, &nativeTypeParamPolicy, nativeTypeParamPolicy.Namespace))
+	require.NoError(t, tracker.Create(bindingsGVR, denyBinding, denyBinding.Namespace))
+	require.NoError(t, tracker.Add(configMapParam))
+
+	// Wait for controller to reconcile given objects
+	require.NoError(t,
+		waitForReconcile(
+			testContext, controller,
+			denyBinding, denyPolicy, configMapParam))
+
+	err := handler.Validate(
+		testContext,
+		// Object is irrelevant/unchecked for this test. Just test that
+		// the evaluator is executed, and returns admit meaning the params
+		// passed was a configmap
+		attributeRecord(nil, fakeParams, admission.Create),
+		&admission.RuntimeObjectInterfaces{},
+	)
+
+	require.ErrorContains(t, err, "correct type")
+	require.EqualValues(t, 1, compiles.Load())
+	require.EqualValues(t, 1, evaluations.Load())
+}

staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go

@@ -25,13 +25,12 @@ import (
 	"time"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/matching"
 
 	"k8s.io/api/admissionregistration/v1alpha1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -65,7 +64,7 @@ type celAdmissionController struct {
 // against all of its registered bindings.
 type policyData struct {
 	definitionInfo
-	paramController generic.Controller[*unstructured.Unstructured]
+	paramController generic.Controller[runtime.Object]
 	bindings        []bindingInfo
 }
 
@@ -96,7 +95,7 @@ type bindingInfo struct {
 
 type paramInfo struct {
 	// Controller which is watching this param CRD
-	controller generic.Controller[*unstructured.Unstructured]
+	controller generic.Controller[runtime.Object]
 
 	// Function to call to stop the informer and clean up the controller
 	stop func()
@@ -116,6 +115,7 @@ func NewAdmissionController(
 		definitions: atomic.Value{},
 		policyController: newPolicyController(
 			restMapper,
+			client,
 			dynamicClient,
 			&CELValidatorCompiler{
 				Matcher: matching.NewMatcher(informerFactory.Core().V1().Namespaces().Lister(), client),
@@ -241,7 +241,7 @@ func (c *celAdmissionController) Validate(
 				continue
 			}
 
-			var param *unstructured.Unstructured
+			var param runtime.Object
 
 			// If definition has paramKind, paramRef is required in binding.
 			// If definition has no paramKind, paramRef set in binding will be ignored.
@@ -292,6 +292,7 @@ func (c *celAdmissionController) Validate(
 					continue
 				}
 			}
+
 			decisions, err := bindingInfo.validator.Validate(a, o, param, matchKind)
 			if err != nil {
 				// runtime error. Apply failure policy

staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go

@@ -25,13 +25,16 @@ import (
 	"k8s.io/api/admissionregistration/v1alpha1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/sets"
 	celmetrics "k8s.io/apiserver/pkg/admission/cel"
 	"k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/internal/generic"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/dynamic/dynamicinformer"
+	"k8s.io/client-go/informers"
+	"k8s.io/client-go/kubernetes"
+	k8sscheme "k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/tools/cache"
 )
 
@@ -74,10 +77,13 @@ type policyController struct {
 	// All keys must have at least one dependent binding
 	// All binding names MUST exist as a key bindingInfos
 	definitionsToBindings map[namespacedName]sets.Set[namespacedName]
+
+	client kubernetes.Interface
 }
 
 func newPolicyController(
 	restMapper meta.RESTMapper,
+	client kubernetes.Interface,
 	dynamicClient dynamic.Interface,
 	validatorCompiler ValidatorCompiler,
 	policiesInformer generic.Informer[*v1alpha1.ValidatingAdmissionPolicy],
@@ -108,6 +114,7 @@ func newPolicyController(
 		),
 		restMapper:    restMapper,
 		dynamicClient: dynamicClient,
+		client:        client,
 	}
 	return res
 }
@@ -237,18 +244,75 @@ func (c *policyController) reconcilePolicyDefinition(namespace, name string, def
 	} else {
 		instanceContext, instanceCancel := context.WithCancel(c.context)
 
-		// Watch for new instances of this policy
+		var informer cache.SharedIndexInformer
-		informer := dynamicinformer.NewFilteredDynamicInformer(
+
-			c.dynamicClient,
+		// Informer Factory is optional
-			paramsGVR.Resource,
+		if c.client != nil {
-			corev1.NamespaceAll,
+			// Create temporary informer factory
-			30*time.Second, // TODO: do we really need to ever resync these?
+			// Cannot use the k8s shared informer factory for dynamic params informer.
-			cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc},
+			// Would leak unnecessary informers when we are done since we would have to
-			nil,
+			// call informerFactory.Start() with a longer-lived stopCh than necessary.
-		)
+			// SharedInformerFactory does not support temporary usage.
+			dynamicFactory := informers.NewSharedInformerFactory(c.client, 10*time.Minute)
+
+			// Look for a typed informer. If it does not exist
+			genericInformer, err := dynamicFactory.ForResource(paramsGVR.Resource)
+
+			// Ignore error. We fallback to dynamic informer if there is no
+			// typed informer
+			if err != nil {
+				informer = nil
+			} else {
+				informer = genericInformer.Informer()
+
+				// Set transformer on the informer to workaround inconsistency
+				// where typed objects have TypeMeta wiped out but dynamic
+				// objects keep kind/apiVersion fields
+				informer.SetTransform(func(i interface{}) (interface{}, error) {
+					// Ensure param is populated with its GVK for consistency
+					// (CRD dynamic informer always returns objects with kind/apiversion,
+					// but native types do not include populated TypeMeta.
+					if param := i.(runtime.Object); param != nil {
+						if param.GetObjectKind().GroupVersionKind().Empty() {
+							// https://github.com/kubernetes/client-go/issues/413#issue-324586398
+							gvks, _, _ := k8sscheme.Scheme.ObjectKinds(param)
+							for _, gvk := range gvks {
+								if len(gvk.Kind) == 0 {
+									continue
+								}
+								if len(gvk.Version) == 0 || gvk.Version == runtime.APIVersionInternal {
+									continue
+								}
+								param.GetObjectKind().SetGroupVersionKind(gvk)
+								break
+							}
+						}
+					}
+
+					return i, nil
+				})
+			}
+		}
+
+		if informer == nil {
+			// Dynamic JSON informer fallback.
+			// Cannot use shared dynamic informer since it would be impossible
+			// to clean CRD informers properly with multiple dependents
+			// (cannot start ahead of time, and cannot track dependencies via stopCh)
+			informer = dynamicinformer.NewFilteredDynamicInformer(
+				c.dynamicClient,
+				paramsGVR.Resource,
+				corev1.NamespaceAll,
+				// Use same interval as is used for k8s typed sharedInformerFactory
+				// https://github.com/kubernetes/kubernetes/blob/7e0923899fed622efbc8679cca6b000d43633e38/cmd/kube-apiserver/app/server.go#L430
+				10*time.Minute,
+				cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc},
+				nil,
+			).Informer()
+		}
 
 		controller := generic.NewController(
-			generic.NewInformer[*unstructured.Unstructured](informer.Informer()),
+			generic.NewInformer[runtime.Object](informer),
 			c.reconcileParams,
 			generic.ControllerOptions{
 				Workers: 1,
@@ -262,8 +326,8 @@ func (c *policyController) reconcilePolicyDefinition(namespace, name string, def
 			dependentDefinitions: sets.New(nn),
 		}
 
-		go informer.Informer().Run(instanceContext.Done())
 		go controller.Run(instanceContext)
+		go informer.Run(instanceContext.Done())
 	}
 
 	return nil
@@ -329,7 +393,7 @@ func (c *policyController) reconcilePolicyBinding(namespace, name string, bindin
 	return nil
 }
 
-func (c *policyController) reconcileParams(namespace, name string, params *unstructured.Unstructured) error {
+func (c *policyController) reconcileParams(namespace, name string, params runtime.Object) error {
 	// Do nothing.
 	// When we add informational type checking we will need to compile in the
 	// reconcile loops instead of lazily so we can add compiler errors / type
@@ -365,7 +429,7 @@ func (c *policyController) latestPolicyData() []policyData {
 			bindingInfos = append(bindingInfos, *bindingInfo)
 		}
 
-		var paramController generic.Controller[*unstructured.Unstructured]
+		var paramController generic.Controller[runtime.Object]
 		if paramKind := definitionInfo.lastReconciledValue.Spec.ParamKind; paramKind != nil {
 			if info, ok := c.paramsCRDControllers[*paramKind]; ok {
 				paramController = info.controller