From acf1d850c6153aae10f26ef3d3e21fa8a63b20e0 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Tue, 20 Feb 2024 09:22:35 -0800 Subject: [PATCH] refactor: handle paramKind directly remove hacks that might conceal errors --- .../plugin/policy/generic/accessor.go | 3 +- .../plugin/policy/generic/policy_source.go | 38 +++++++++++++------ .../policy/generic/policy_source_test.go | 12 +++--- .../plugin/policy/validating/accessor.go | 29 +------------- 4 files changed, 35 insertions(+), 47 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/accessor.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/accessor.go index fc8a7b77aeb..ae6d069a35c 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/accessor.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/accessor.go @@ -18,14 +18,13 @@ package generic import ( "k8s.io/api/admissionregistration/v1beta1" - "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" ) type PolicyAccessor interface { GetName() string GetNamespace() string - GetParamKind() *schema.GroupVersionKind + GetParamKind() *v1beta1.ParamKind GetMatchConstraints() *v1beta1.MatchResources } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_source.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_source.go index 4f52e406404..9b2e2146a8f 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_source.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_source.go @@ -83,8 +83,11 @@ type compiledPolicyEntry[E Evaluator] struct { } type PolicyHook[P runtime.Object, B runtime.Object, E Evaluator] struct { - Policy P - Bindings []B + Policy P + Bindings []B + + // ParamInformer is the informer for the param CRD for this policy, or nil if + // there is no param or if there was a configuration error ParamInformer informers.GenericInformer ParamScope meta.RESTScope @@ -157,7 +160,7 @@ func (s *policySource[P, B, E]) Run(ctx context.Context) error { } defer func() { if err := s.policyInformer.RemoveEventHandler(handle); err != nil { - utilruntime.HandleError(fmt.Errorf("failed to remove policy event handler: %v", err)) + utilruntime.HandleError(fmt.Errorf("failed to remove policy event handler: %w", err)) } }() @@ -167,7 +170,7 @@ func (s *policySource[P, B, E]) Run(ctx context.Context) error { } defer func() { if err := s.bindingInformer.RemoveEventHandler(bindingHandle); err != nil { - utilruntime.HandleError(fmt.Errorf("failed to remove binding event handler: %v", err)) + utilruntime.HandleError(fmt.Errorf("failed to remove binding event handler: %w", err)) } }() @@ -231,7 +234,7 @@ func (s *policySource[P, B, E]) refreshPolicies() { if err != nil { // An error was generated while syncing policies. Mark it as dirty again // so we can retry later - utilruntime.HandleError(fmt.Errorf("encountered error syncing policies: %v. Rescheduling policy sync", err)) + utilruntime.HandleError(fmt.Errorf("encountered error syncing policies: %w. Rescheduling policy sync", err)) s.notify() } } @@ -299,8 +302,26 @@ func (s *policySource[P, B, E]) calculatePolicyData() ([]PolicyHook[P, B, E], er continue } + var parsedParamKind *schema.GroupVersionKind policyAccessor := s.newPolicyAccessor(policySpec) - paramInformer, paramScope, configurationError := s.ensureParamsForPolicyLocked(policyAccessor.GetParamKind()) + + if paramKind := policyAccessor.GetParamKind(); paramKind != nil { + groupVersion, err := schema.ParseGroupVersion(paramKind.APIVersion) + if err != nil { + errs = append(errs, fmt.Errorf("failed to parse paramKind APIVersion: %w", err)) + continue + } + parsedParamKind = &schema.GroupVersionKind{ + Group: groupVersion.Group, + Version: groupVersion.Version, + Kind: paramKind.Kind, + } + + // TEMPORARY UNTIL WE HAVE SHARED PARAM INFORMERS + usedParams[*parsedParamKind] = struct{}{} + } + + paramInformer, paramScope, configurationError := s.ensureParamsForPolicyLocked(parsedParamKind) result = append(result, PolicyHook[P, B, E]{ Policy: policySpec, Bindings: bindingSpecs, @@ -310,11 +331,6 @@ func (s *policySource[P, B, E]) calculatePolicyData() ([]PolicyHook[P, B, E], er ConfigurationError: configurationError, }) - // TEMPORARY UNTIL WE HAVE SHARED PARAM INFORMERS - if paramKind := policyAccessor.GetParamKind(); paramKind != nil { - usedParams[*paramKind] = struct{}{} - } - // Should queue a re-sync for policy sync error. If our shared param // informer can notify us when CRD discovery changes we can remove this // and just rely on the informer to notify us when the CRDs change diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_source_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_source_test.go index bbc98c6a620..7aae1be052a 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_source_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_source_test.go @@ -23,7 +23,6 @@ import ( "k8s.io/api/admissionregistration/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/apiserver/pkg/admission/plugin/policy/generic" "k8s.io/apiserver/pkg/admission/plugin/policy/matching" @@ -111,10 +110,9 @@ func TestPolicySourceHasSyncedInitialList(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "policy2", }, - ParamKind: &schema.GroupVersionKind{ - Group: "policy.example.com", - Version: "v1", - Kind: "FakeParam", + ParamKind: &v1beta1.ParamKind{ + APIVersion: "policy.example.com/v1", + Kind: "FakeParam", }, }, )) @@ -179,7 +177,7 @@ type FakePolicy struct { metav1.TypeMeta metav1.ObjectMeta - ParamKind *schema.GroupVersionKind + ParamKind *v1beta1.ParamKind } var _ generic.PolicyAccessor = &FakePolicy{} @@ -201,7 +199,7 @@ func (fp *FakePolicy) GetNamespace() string { return fp.Namespace } -func (fp *FakePolicy) GetParamKind() *schema.GroupVersionKind { +func (fp *FakePolicy) GetParamKind() *v1beta1.ParamKind { return fp.ParamKind } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/accessor.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/accessor.go index 2627e4d63c0..6af2857598d 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/accessor.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/accessor.go @@ -18,7 +18,6 @@ package validating import ( "k8s.io/api/admissionregistration/v1beta1" - "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/apiserver/pkg/admission/plugin/policy/generic" ) @@ -47,32 +46,8 @@ func (v *validatingAdmissionPolicyAccessor) GetName() string { return v.Name } -func (v *validatingAdmissionPolicyAccessor) GetParamKind() *schema.GroupVersionKind { - paramKind := v.Spec.ParamKind - if paramKind == nil { - return nil - } - - groupVersion, err := schema.ParseGroupVersion(paramKind.APIVersion) - if err != nil { - // A validatingadmissionpolicy which passes validation should have - // a parseable APIVersion for its ParamKind, so this should never happen - // if the policy is valid. - // - // Return a bogus but non-nil GVK that will throw an error about the - // invalid APIVersion when the param is looked up. - return &schema.GroupVersionKind{ - Group: paramKind.APIVersion, - Version: "", - Kind: paramKind.Kind, - } - } - - return &schema.GroupVersionKind{ - Group: groupVersion.Group, - Version: groupVersion.Version, - Kind: paramKind.Kind, - } +func (v *validatingAdmissionPolicyAccessor) GetParamKind() *v1beta1.ParamKind { + return v.Spec.ParamKind } func (v *validatingAdmissionPolicyAccessor) GetMatchConstraints() *v1beta1.MatchResources {