From 0c495cb429e54a6d25e9252aca3e32fd9f0aef6b Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Thu, 19 Jan 2023 10:04:46 -0800 Subject: [PATCH 1/5] use namespacedName for keys in fakeCompiler --- .../admission_test.go | 38 +++++++++++++------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go index 3e41a9684c3..3e0e9c34bc1 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go @@ -142,9 +142,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 - DefinitionMatchFuncs map[string]func(*v1alpha1.ValidatingAdmissionPolicy, admission.Attributes) bool - BindingMatchFuncs map[string]func(*v1alpha1.ValidatingAdmissionPolicyBinding, admission.Attributes) bool + CompileFuncs map[namespacedName]func(*v1alpha1.ValidatingAdmissionPolicy) Validator + DefinitionMatchFuncs map[namespacedName]func(*v1alpha1.ValidatingAdmissionPolicy, admission.Attributes) bool + BindingMatchFuncs map[namespacedName]func(*v1alpha1.ValidatingAdmissionPolicyBinding, admission.Attributes) bool } var _ ValidatorCompiler = &fakeCompiler{} @@ -161,7 +161,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 +177,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 +193,10 @@ func (f *fakeCompiler) Compile( definition *v1alpha1.ValidatingAdmissionPolicy, ) Validator { namespace, name := definition.Namespace, definition.Name - - key := namespace + "/" + name + key := namespacedName{ + name: name, + namespace: namespace, + } if fun, ok := f.CompileFuncs[key]; ok { return fun(definition) } @@ -198,18 +206,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 +228,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 } From b969dfec9fd33f8bfff47e54f2995a4865839ea6 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Thu, 19 Jan 2023 10:04:52 -0800 Subject: [PATCH 2/5] use typedinformer if available reduces memory and cpu when things like configmap are used as a param cannot be shared due to limitatoins of sharedinformerfactory --- .../admission_test.go | 141 ++++++++++++++---- .../validatingadmissionpolicy/controller.go | 9 +- .../controller_reconcile.go | 65 ++++++-- 3 files changed, 172 insertions(+), 43 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go index 3e0e9c34bc1..7edb06f5f3b 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go +++ b/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 }() @@ -388,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] @@ -429,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 } } @@ -1223,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()) +} diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go index 6767c7d11fb..cd5745fe986 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go @@ -25,13 +25,13 @@ 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 +65,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 +96,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 +116,7 @@ func NewAdmissionController( definitions: atomic.Value{}, policyController: newPolicyController( restMapper, + client, dynamicClient, &CELValidatorCompiler{ Matcher: matching.NewMatcher(informerFactory.Core().V1().Namespaces().Lister(), client), @@ -241,7 +242,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. diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go index bb2289f1b63..9a376507a77 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go @@ -25,13 +25,15 @@ 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" "k8s.io/client-go/tools/cache" ) @@ -74,10 +76,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 +113,7 @@ func newPolicyController( ), restMapper: restMapper, dynamicClient: dynamicClient, + client: client, } return res } @@ -237,18 +243,50 @@ func (c *policyController) reconcilePolicyDefinition(namespace, name string, def } else { instanceContext, instanceCancel := context.WithCancel(c.context) - // Watch for new instances of this policy - informer := dynamicinformer.NewFilteredDynamicInformer( - c.dynamicClient, - paramsGVR.Resource, - corev1.NamespaceAll, - 30*time.Second, // TODO: do we really need to ever resync these? - cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}, - nil, - ) + var informer informers.GenericInformer + + // Informer Factory is optional + if c.client != nil { + // Create temporary informer factory + // Cannot use the k8s shared informer factory for dynamic params informer. + // Would leak unnecessary informers when we are done since we would have to + // 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 + informer, err = dynamicFactory.ForResource(paramsGVR.Resource) + + // Ignore error. We fallback to dynamic informer if there is no + // typed informer + if err != nil { + informer = nil + } else { + dynamicFactory.Start(instanceContext.Done()) + } + } + + 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, + ) + + go informer.Informer().Run(instanceContext.Done()) + } controller := generic.NewController( - generic.NewInformer[*unstructured.Unstructured](informer.Informer()), + generic.NewInformer[runtime.Object](informer.Informer()), c.reconcileParams, generic.ControllerOptions{ Workers: 1, @@ -262,7 +300,6 @@ func (c *policyController) reconcilePolicyDefinition(namespace, name string, def dependentDefinitions: sets.New(nn), } - go informer.Informer().Run(instanceContext.Done()) go controller.Run(instanceContext) } @@ -329,7 +366,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 +402,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 From 1554e50be43660bc9f03d97cc26b235ad4f94d6c Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Tue, 24 Jan 2023 12:00:05 -0800 Subject: [PATCH 3/5] fix integration test by working around #3030 test uses kind field which is not populated for native types --- .../validatingadmissionpolicy/controller.go | 27 ++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go index cd5745fe986..b4d580adaa6 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go @@ -26,8 +26,8 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/matching" + k8sscheme "k8s.io/client-go/kubernetes/scheme" "k8s.io/api/admissionregistration/v1alpha1" k8serrors "k8s.io/apimachinery/pkg/api/errors" @@ -293,6 +293,31 @@ func (c *celAdmissionController) Validate( continue } } + + // 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 != nil { + if paramGVK := param.GetObjectKind(); paramGVK.GroupVersionKind().Empty() { + // https://github.com/kubernetes/client-go/issues/413#issue-324586398 + gvks, _, err := k8sscheme.Scheme.ObjectKinds(param) + if err != nil { + return fmt.Errorf("missing apiVersion or kind and cannot assign it; %w", err) + } + + for _, gvk := range gvks { + if len(gvk.Kind) == 0 { + continue + } + if len(gvk.Version) == 0 || gvk.Version == runtime.APIVersionInternal { + continue + } + paramGVK.SetGroupVersionKind(gvk) + break + } + } + } + decisions, err := bindingInfo.validator.Validate(a, o, param, matchKind) if err != nil { // runtime error. Apply failure policy From 65513eac3ab67f08745197d8af469532284b797e Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Tue, 24 Jan 2023 14:46:35 -0800 Subject: [PATCH 4/5] add unfortunate deepcopy --- .../plugin/validatingadmissionpolicy/controller.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go index b4d580adaa6..65e46137cbf 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go @@ -298,7 +298,11 @@ func (c *celAdmissionController) Validate( // (CRD dynamic informer always returns objects with kind/apiversion, // but native types do not include populated TypeMeta. if param != nil { - if paramGVK := param.GetObjectKind(); paramGVK.GroupVersionKind().Empty() { + if param.GetObjectKind().GroupVersionKind().Empty() { + // Very unfortunate. Unfortunately object is shared and cannot + // modify GVK async + param = param.DeepCopyObject() + // https://github.com/kubernetes/client-go/issues/413#issue-324586398 gvks, _, err := k8sscheme.Scheme.ObjectKinds(param) if err != nil { @@ -312,7 +316,7 @@ func (c *celAdmissionController) Validate( if len(gvk.Version) == 0 || gvk.Version == runtime.APIVersionInternal { continue } - paramGVK.SetGroupVersionKind(gvk) + param.GetObjectKind().SetGroupVersionKind(gvk) break } } From 24fb6b89812ac86622a536dba861729ed5a20b74 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Thu, 26 Jan 2023 12:14:14 -0800 Subject: [PATCH 5/5] use transformer to set gvk back --- .../validatingadmissionpolicy/controller.go | 29 ------------- .../controller_reconcile.go | 41 +++++++++++++++---- 2 files changed, 34 insertions(+), 36 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go index 65e46137cbf..795dbad5b9c 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go @@ -27,7 +27,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/matching" - k8sscheme "k8s.io/client-go/kubernetes/scheme" "k8s.io/api/admissionregistration/v1alpha1" k8serrors "k8s.io/apimachinery/pkg/api/errors" @@ -294,34 +293,6 @@ func (c *celAdmissionController) Validate( } } - // 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 != nil { - if param.GetObjectKind().GroupVersionKind().Empty() { - // Very unfortunate. Unfortunately object is shared and cannot - // modify GVK async - param = param.DeepCopyObject() - - // https://github.com/kubernetes/client-go/issues/413#issue-324586398 - gvks, _, err := k8sscheme.Scheme.ObjectKinds(param) - if err != nil { - return fmt.Errorf("missing apiVersion or kind and cannot assign it; %w", err) - } - - 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 - } - } - } - decisions, err := bindingInfo.validator.Validate(a, o, param, matchKind) if err != nil { // runtime error. Apply failure policy diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go index 9a376507a77..2952640eb3e 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go @@ -34,6 +34,7 @@ import ( "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" ) @@ -243,7 +244,7 @@ func (c *policyController) reconcilePolicyDefinition(namespace, name string, def } else { instanceContext, instanceCancel := context.WithCancel(c.context) - var informer informers.GenericInformer + var informer cache.SharedIndexInformer // Informer Factory is optional if c.client != nil { @@ -255,14 +256,41 @@ func (c *policyController) reconcilePolicyDefinition(namespace, name string, def dynamicFactory := informers.NewSharedInformerFactory(c.client, 10*time.Minute) // Look for a typed informer. If it does not exist - informer, err = dynamicFactory.ForResource(paramsGVR.Resource) + 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 { - dynamicFactory.Start(instanceContext.Done()) + 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 + }) } } @@ -280,13 +308,11 @@ func (c *policyController) reconcilePolicyDefinition(namespace, name string, def 10*time.Minute, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}, nil, - ) - - go informer.Informer().Run(instanceContext.Done()) + ).Informer() } controller := generic.NewController( - generic.NewInformer[runtime.Object](informer.Informer()), + generic.NewInformer[runtime.Object](informer), c.reconcileParams, generic.ControllerOptions{ Workers: 1, @@ -301,6 +327,7 @@ func (c *policyController) reconcilePolicyDefinition(namespace, name string, def } go controller.Run(instanceContext) + go informer.Run(instanceContext.Done()) } return nil