Merge pull request #119109 from jiahuif-forks/feature/validating-admission-policy/crd-typechecking

ValidatingAdmissionPolicy - Type Checking for API Expensions types

cmd/kube-controller-manager/app/validatingadmissionpolicystatus.go

@@ -19,10 +19,11 @@ package app
 import (
 	"context"
 
+	apiextensionsscheme "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/scheme"
 	pluginvalidatingadmissionpolicy "k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy"
 	"k8s.io/apiserver/pkg/cel/openapi/resolver"
 	genericfeatures "k8s.io/apiserver/pkg/features"
-	"k8s.io/client-go/kubernetes/scheme"
+	k8sscheme "k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/component-base/featuregate"
 	"k8s.io/controller-manager/controller"
 	"k8s.io/kubernetes/cmd/kube-controller-manager/names"
@@ -42,8 +43,12 @@ func newValidatingAdmissionPolicyStatusControllerDescriptor() *ControllerDescrip
 
 func startValidatingAdmissionPolicyStatusController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) {
 	// KCM won't start the controller without the feature gate set.
+
+	schemaResolver := resolver.NewDefinitionsSchemaResolver(openapi.GetOpenAPIDefinitions, k8sscheme.Scheme, apiextensionsscheme.Scheme).
+		Combine(&resolver.ClientDiscoveryResolver{Discovery: controllerContext.ClientBuilder.DiscoveryClientOrDie(names.ValidatingAdmissionPolicyStatusController)})
+
 	typeChecker := &pluginvalidatingadmissionpolicy.TypeChecker{
-		SchemaResolver: resolver.NewDefinitionsSchemaResolver(scheme.Scheme, openapi.GetOpenAPIDefinitions),
+		SchemaResolver: schemaResolver,
 		RestMapper:     controllerContext.RESTMapper,
 	}
 	c, err := validatingadmissionpolicystatus.NewController(

pkg/controller/validatingadmissionpolicystatus/controller_test.go

@@ -103,7 +103,7 @@ func TestTypeChecking(t *testing.T) {
 			client := fake.NewSimpleClientset(policy)
 			informerFactory := informers.NewSharedInformerFactory(client, 0)
 			typeChecker := &validatingadmissionpolicy.TypeChecker{
-				SchemaResolver: resolver.NewDefinitionsSchemaResolver(scheme.Scheme, openapi.GetOpenAPIDefinitions),
+				SchemaResolver: resolver.NewDefinitionsSchemaResolver(openapi.GetOpenAPIDefinitions, scheme.Scheme),
 				RestMapper:     testrestmapper.TestOnlyStaticRESTMapper(scheme.Scheme),
 			}
 			controller, err := NewController(

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

@@ -238,7 +238,7 @@ func (c *TypeChecker) typesToCheck(p *v1beta1.ValidatingAdmissionPolicy) []schem
 	if p.Spec.MatchConstraints == nil || len(p.Spec.MatchConstraints.ResourceRules) == 0 {
 		return nil
 	}
-
+	restMapperRefreshAttempted := false // at most once per policy, refresh RESTMapper and retry resolution.
 	for _, rule := range p.Spec.MatchConstraints.ResourceRules {
 		groups := extractGroups(&rule.Rule)
 		if len(groups) == 0 {
@@ -268,7 +268,16 @@ func (c *TypeChecker) typesToCheck(p *v1beta1.ValidatingAdmissionPolicy) []schem
 					}
 					resolved, err := c.RestMapper.KindsFor(gvr)
 					if err != nil {
-						continue
+						if restMapperRefreshAttempted {
+							// RESTMapper refresh happens at most once per policy
+							continue
+						}
+						c.tryRefreshRESTMapper()
+						restMapperRefreshAttempted = true
+						resolved, err = c.RestMapper.KindsFor(gvr)
+						if err != nil {
+							continue
+						}
 					}
 					for _, r := range resolved {
 						if !r.Empty() {
@@ -344,6 +353,13 @@ func sortGVKList(list []schema.GroupVersionKind) []schema.GroupVersionKind {
 	return list
 }
 
+// tryRefreshRESTMapper refreshes the RESTMapper if it supports refreshing.
+func (c *TypeChecker) tryRefreshRESTMapper() {
+	if r, ok := c.RestMapper.(meta.ResettableRESTMapper); ok {
+		r.Reset()
+	}
+}
+
 func buildEnv(hasParams bool, hasAuthorizer bool, types typeOverwrite) (*cel.Env, error) {
 	baseEnv := environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion())
 	requestType := plugincel.BuildRequestType()

staging/src/k8s.io/apiserver/pkg/cel/openapi/resolver/combined.go

@@ -0,0 +1,45 @@
+/*
+Copyright 2023 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package resolver
+
+import (
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/kube-openapi/pkg/validation/spec"
+)
+
+// Combine combines the DefinitionsSchemaResolver with a secondary schema resolver.
+// The resulting schema resolver uses the DefinitionsSchemaResolver for a GVK that DefinitionsSchemaResolver knows,
+// and the secondary otherwise.
+func (d *DefinitionsSchemaResolver) Combine(secondary SchemaResolver) SchemaResolver {
+	return &combinedSchemaResolver{definitions: d, secondary: secondary}
+}
+
+type combinedSchemaResolver struct {
+	definitions *DefinitionsSchemaResolver
+	secondary   SchemaResolver
+}
+
+// ResolveSchema takes a GroupVersionKind (GVK) and returns the OpenAPI schema
+// identified by the GVK.
+// If the DefinitionsSchemaResolver knows the gvk, the DefinitionsSchemaResolver handles the resolution,
+// otherwise, the secondary does.
+func (r *combinedSchemaResolver) ResolveSchema(gvk schema.GroupVersionKind) (*spec.Schema, error) {
+	if _, ok := r.definitions.gvkToSchema[gvk]; ok {
+		return r.definitions.ResolveSchema(gvk)
+	}
+	return r.secondary.ResolveSchema(gvk)
+}

staging/src/k8s.io/apiserver/pkg/cel/openapi/resolver/definitions.go

@@ -35,11 +35,11 @@ type DefinitionsSchemaResolver struct {
 
 // NewDefinitionsSchemaResolver creates a new DefinitionsSchemaResolver.
 // An example working setup:
-// scheme         = "k8s.io/client-go/kubernetes/scheme".Scheme
 // getDefinitions = "k8s.io/kubernetes/pkg/generated/openapi".GetOpenAPIDefinitions
-func NewDefinitionsSchemaResolver(scheme *runtime.Scheme, getDefinitions common.GetOpenAPIDefinitions) *DefinitionsSchemaResolver {
+// scheme         = "k8s.io/client-go/kubernetes/scheme".Scheme
+func NewDefinitionsSchemaResolver(getDefinitions common.GetOpenAPIDefinitions, schemes ...*runtime.Scheme) *DefinitionsSchemaResolver {
 	gvkToSchema := make(map[schema.GroupVersionKind]*spec.Schema)
-	namer := openapi.NewDefinitionNamer(scheme)
+	namer := openapi.NewDefinitionNamer(schemes...)
 	defs := getDefinitions(func(path string) spec.Ref {
 		return spec.MustCreateRef(path)
 	})

test/e2e/apimachinery/validatingadmissionpolicy.go

@@ -28,11 +28,15 @@ import (
 	admissionregistrationv1beta1 "k8s.io/api/admissionregistration/v1beta1"
 	appsv1 "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	apiextensionsclientset "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/apiserver/pkg/features"
 	clientset "k8s.io/client-go/kubernetes"
+	"k8s.io/client-go/openapi3"
 	"k8s.io/kubernetes/test/e2e/framework"
 	admissionapi "k8s.io/pod-security-admission/api"
 )
@@ -42,6 +46,7 @@ var _ = SIGDescribe("ValidatingAdmissionPolicy [Privileged:ClusterAdmin]", frame
 	f.NamespacePodSecurityLevel = admissionapi.LevelBaseline
 
 	var client clientset.Interface
+	var extensionsClient apiextensionsclientset.Interface
 
 	ginkgo.BeforeEach(func() {
 		var err error
@@ -52,6 +57,8 @@ var _ = SIGDescribe("ValidatingAdmissionPolicy [Privileged:ClusterAdmin]", frame
 			// TODO: feature check should fail after GA graduation
 			ginkgo.Skip(fmt.Sprintf("server does not support ValidatingAdmissionPolicy v1beta1: %v, feature gate not enabled?", err))
 		}
+		extensionsClient, err = apiextensionsclientset.NewForConfig(f.ClientConfig())
+		framework.ExpectNoError(err, "initializing api-extensions client")
 	})
 
 	ginkgo.BeforeEach(func(ctx context.Context) {
@@ -256,6 +263,98 @@ var _ = SIGDescribe("ValidatingAdmissionPolicy [Privileged:ClusterAdmin]", frame
 			framework.ExpectNoError(err, "create non-replicated ReplicaSet")
 		})
 	})
+
+	ginkgo.It("should type check a CRD", func(ctx context.Context) {
+		crd := crontabExampleCRD()
+		crd.Spec.Group = "stable." + f.UniqueName
+		crd.Name = crd.Spec.Names.Plural + "." + crd.Spec.Group
+		var policy *admissionregistrationv1beta1.ValidatingAdmissionPolicy
+		ginkgo.By("creating the CRD", func() {
+			var err error
+			crd, err = extensionsClient.ApiextensionsV1().CustomResourceDefinitions().Create(ctx, crd, metav1.CreateOptions{})
+			framework.ExpectNoError(err, "create CRD")
+			err = wait.PollUntilContextCancel(ctx, 100*time.Millisecond, true, func(ctx context.Context) (done bool, err error) {
+				// wait for the CRD to be published.
+				root := openapi3.NewRoot(client.Discovery().OpenAPIV3())
+				_, err = root.GVSpec(schema.GroupVersion{Group: crd.Spec.Group, Version: "v1"})
+				return err == nil, nil
+			})
+			framework.ExpectNoError(err, "wait for CRD.")
+			ginkgo.DeferCleanup(func(ctx context.Context, name string) error {
+				return extensionsClient.ApiextensionsV1().CustomResourceDefinitions().Delete(ctx, name, metav1.DeleteOptions{})
+			}, crd.Name)
+		})
+		ginkgo.By("creating a vaild policy for crontabs", func() {
+			policy = newValidatingAdmissionPolicyBuilder(f.UniqueName+".correct-crd-policy.example.com").
+				MatchUniqueNamespace(f.UniqueName).
+				StartResourceRule().
+				MatchResource([]string{crd.Spec.Group}, []string{"v1"}, []string{"crontabs"}).
+				EndResourceRule().
+				WithValidation(admissionregistrationv1beta1.Validation{
+					Expression: "object.spec.replicas > 1",
+				}).
+				Build()
+			policy, err := client.AdmissionregistrationV1beta1().ValidatingAdmissionPolicies().Create(ctx, policy, metav1.CreateOptions{})
+			framework.ExpectNoError(err, "create policy")
+			ginkgo.DeferCleanup(func(ctx context.Context, name string) error {
+				return client.AdmissionregistrationV1alpha1().ValidatingAdmissionPolicies().Delete(ctx, name, metav1.DeleteOptions{})
+			}, policy.Name)
+		})
+		ginkgo.By("waiting for the type check to finish without warnings", func() {
+			err := wait.PollUntilContextCancel(ctx, 100*time.Millisecond, true, func(ctx context.Context) (done bool, err error) {
+				policy, err = client.AdmissionregistrationV1beta1().ValidatingAdmissionPolicies().Get(ctx, policy.Name, metav1.GetOptions{})
+				if err != nil {
+					return false, err
+				}
+				if policy.Status.TypeChecking != nil {
+					return true, nil
+				}
+				return false, nil
+			})
+			framework.ExpectNoError(err, "wait for type checking")
+			gomega.Expect(policy.Status.TypeChecking.ExpressionWarnings).To(gomega.BeEmpty(), "expect no warnings")
+		})
+		ginkgo.By("creating a policy with type-confused expressions for crontabs", func() {
+			policy = newValidatingAdmissionPolicyBuilder(f.UniqueName+".confused-crd-policy.example.com").
+				MatchUniqueNamespace(f.UniqueName).
+				StartResourceRule().
+				MatchResource([]string{crd.Spec.Group}, []string{"v1"}, []string{"crontabs"}).
+				EndResourceRule().
+				WithValidation(admissionregistrationv1beta1.Validation{
+					Expression: "object.spec.replicas > '1'", // type confusion
+				}).
+				WithValidation(admissionregistrationv1beta1.Validation{
+					Expression: "object.spec.maxRetries < 10", // not yet existing field
+				}).
+				Build()
+			policy, err := client.AdmissionregistrationV1beta1().ValidatingAdmissionPolicies().Create(ctx, policy, metav1.CreateOptions{})
+			framework.ExpectNoError(err, "create policy")
+			ginkgo.DeferCleanup(func(ctx context.Context, name string) error {
+				return client.AdmissionregistrationV1alpha1().ValidatingAdmissionPolicies().Delete(ctx, name, metav1.DeleteOptions{})
+			}, policy.Name)
+		})
+		ginkgo.By("waiting for the type check to finish with warnings", func() {
+			err := wait.PollUntilContextCancel(ctx, 100*time.Millisecond, true, func(ctx context.Context) (done bool, err error) {
+				policy, err = client.AdmissionregistrationV1beta1().ValidatingAdmissionPolicies().Get(ctx, policy.Name, metav1.GetOptions{})
+				if err != nil {
+					return false, err
+				}
+				if policy.Status.TypeChecking != nil {
+					return true, nil
+				}
+				return false, nil
+			})
+			framework.ExpectNoError(err, "wait for type checking")
+
+			gomega.Expect(policy.Status.TypeChecking.ExpressionWarnings).To(gomega.HaveLen(2))
+			warning := policy.Status.TypeChecking.ExpressionWarnings[0]
+			gomega.Expect(warning.FieldRef).To(gomega.Equal("spec.validations[0].expression"))
+			gomega.Expect(warning.Warning).To(gomega.ContainSubstring("found no matching overload for '_>_' applied to '(int, string)'"))
+			warning = policy.Status.TypeChecking.ExpressionWarnings[1]
+			gomega.Expect(warning.FieldRef).To(gomega.Equal("spec.validations[1].expression"))
+			gomega.Expect(warning.Warning).To(gomega.ContainSubstring("undefined field 'maxRetries'"))
+		})
+	})
 })
 
 func createBinding(bindingName string, uniqueLabel string, policyName string) *admissionregistrationv1beta1.ValidatingAdmissionPolicyBinding {
@@ -408,3 +507,48 @@ func (b *validatingAdmissionPolicyBuilder) WithVariable(variable admissionregist
 func (b *validatingAdmissionPolicyBuilder) Build() *admissionregistrationv1beta1.ValidatingAdmissionPolicy {
 	return b.policy
 }
+
+func crontabExampleCRD() *apiextensionsv1.CustomResourceDefinition {
+	return &apiextensionsv1.CustomResourceDefinition{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "crontabs.stable.example.com",
+		},
+		Spec: apiextensionsv1.CustomResourceDefinitionSpec{
+			Group: "stable.example.com",
+			Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
+				{
+					Name:    "v1",
+					Served:  true,
+					Storage: true,
+					Schema: &apiextensionsv1.CustomResourceValidation{
+						OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
+							Type: "object",
+							Properties: map[string]apiextensionsv1.JSONSchemaProps{
+								"spec": {
+									Type: "object",
+									Properties: map[string]apiextensionsv1.JSONSchemaProps{
+										"cronSpec": {
+											Type: "string",
+										},
+										"image": {
+											Type: "string",
+										},
+										"replicas": {
+											Type: "integer",
+										},
+									},
+								},
+							},
+						}},
+				},
+			},
+			Scope: apiextensionsv1.NamespaceScoped,
+			Names: apiextensionsv1.CustomResourceDefinitionNames{
+				Plural:     "crontabs",
+				Singular:   "crontab",
+				Kind:       "CronTab",
+				ShortNames: []string{"ct"},
+			},
+		},
+	}
+}

test/integration/apiserver/cel/typeresolution_test.go

@@ -79,7 +79,7 @@ func TestTypeResolver(t *testing.T) {
 		}
 	}(crd)
 	discoveryResolver := &resolver.ClientDiscoveryResolver{Discovery: client.Discovery()}
-	definitionsResolver := resolver.NewDefinitionsSchemaResolver(k8sscheme.Scheme, openapi.GetOpenAPIDefinitions)
+	definitionsResolver := resolver.NewDefinitionsSchemaResolver(openapi.GetOpenAPIDefinitions, k8sscheme.Scheme)
 	// wait until the CRD schema is published at the OpenAPI v3 endpoint
 	err = wait.PollImmediate(time.Second, time.Minute, func() (done bool, err error) {
 		p, err := client.OpenAPIV3().Paths()
@@ -330,7 +330,7 @@ func TestBuiltinResolution(t *testing.T) {
 	}{
 		{
 			name:     "definitions",
-			resolver: resolver.NewDefinitionsSchemaResolver(k8sscheme.Scheme, openapi.GetOpenAPIDefinitions),
+			resolver: resolver.NewDefinitionsSchemaResolver(openapi.GetOpenAPIDefinitions, k8sscheme.Scheme),
 			scheme:   buildTestScheme(),
 		},
 		{