Add caching of structural schema generation

Author: JoelSpeed

pkg/controllers/crdcompatibility/objectpruning/validator_unit_test.go

@@ -0,0 +1,135 @@
+/*
+Copyright 2025 Red Hat, Inc.
+
+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 objectpruning
+
+import (
+	"context"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	apiextensionsv1alpha1 "github.com/openshift/api/apiextensions/v1alpha1"
+	"github.com/openshift/cluster-capi-operator/pkg/controllers/crdcompatibility/index"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/serializer"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+)
+
+var _ = Describe("validator", func() {
+	var (
+		ctx                      context.Context
+		testCRD                  *apiextensionsv1.CustomResourceDefinition
+		compatibilityRequirement *apiextensionsv1alpha1.CompatibilityRequirement
+	)
+
+	BeforeEach(func() {
+		ctx = context.Background()
+
+		// Create a test CRD with some properties for testing
+		testCRD = createStrictCRDSchema()
+
+		// Create a corresponding compatibility requirement
+		compatibilityRequirement = createCompatibilityRequirement(testCRD)
+	})
+
+	Describe("getStructuralSchema", func() {
+		Context("when CompatibilityRequirement exists", func() {
+			var validator *validator
+
+			BeforeEach(func() {
+				validator = createValidatorWithFakeClient([]client.Object{compatibilityRequirement})
+			})
+
+			It("should return structural schema", func() {
+				schema, err := validator.getStructuralSchema(ctx, compatibilityRequirement.Name, "v1")
+
+				Expect(err).NotTo(HaveOccurred())
+				Expect(schema).NotTo(BeNil())
+			})
+
+			It("should cache structural schema", func() {
+				// First call should create and cache the schema
+				_, err := validator.getStructuralSchema(ctx, compatibilityRequirement.Name, "v1")
+				Expect(err).NotTo(HaveOccurred())
+
+				// Check that cache now contains an entry
+				validator.structuralSchemaCacheLock.RLock()
+				cacheSize := len(validator.structuralSchemaCache)
+				validator.structuralSchemaCacheLock.RUnlock()
+
+				Expect(cacheSize).To(Equal(1))
+			})
+
+			It("should use cached schema on subsequent calls", func() {
+				// First call
+				schema1, err1 := validator.getStructuralSchema(ctx, compatibilityRequirement.Name, "v1")
+				Expect(err1).NotTo(HaveOccurred())
+
+				// Second call should return the same schema instance
+				schema2, err2 := validator.getStructuralSchema(ctx, compatibilityRequirement.Name, "v1")
+				Expect(err2).NotTo(HaveOccurred())
+
+				// Should be the exact same object (cached)
+				// This uses reflect.DeepEqual to compare the two schemas.
+				Expect(schema1).To(Equal(schema2))
+			})
+		})
+
+		Context("when CompatibilityRequirement does not exist", func() {
+			It("should return error", func() {
+				validator := createValidatorWithFakeClient([]client.Object{}) // No objects
+
+				_, err := validator.getStructuralSchema(ctx, "non-existent", "v1")
+
+				Expect(err).To(MatchError("failed to get CompatibilityRequirement \"non-existent\": compatibilityrequirements.apiextensions.openshift.io \"non-existent\" not found"))
+			})
+		})
+
+		Context("when CompatibilityRequirement has invalid CRD YAML", func() {
+			It("should return error", func() {
+				brokenCompatibilityRequirement := createCompatibilityRequirement(testCRD)
+				brokenCompatibilityRequirement.Spec.CompatibilitySchema.CustomResourceDefinition.Data = "invalid: yaml: content: ["
+
+				validator := createValidatorWithFakeClient([]client.Object{brokenCompatibilityRequirement})
+
+				_, err := validator.getStructuralSchema(ctx, brokenCompatibilityRequirement.Name, "v1")
+
+				Expect(err).To(MatchError(ContainSubstring("failed to get structural schema: failed to decode compatibility schema data for CompatibilityRequirement \"\": yaml: mapping values are not allowed in this context")))
+			})
+		})
+	})
+})
+
+// Helper function to create a validator with fake client for testing
+func createValidatorWithFakeClient(objects []client.Object) *validator {
+	scheme := runtime.NewScheme()
+	scheme.AddKnownTypes(apiextensionsv1alpha1.SchemeGroupVersion, &apiextensionsv1alpha1.CompatibilityRequirement{})
+	scheme.AddKnownTypes(apiextensionsv1.SchemeGroupVersion, &apiextensionsv1.CustomResourceDefinition{})
+
+	return &validator{
+		universalDeserializer: serializer.NewCodecFactory(scheme).UniversalDeserializer(),
+		client: fake.NewClientBuilder().
+			WithObjects(objects...).
+			WithIndex(&apiextensionsv1alpha1.CompatibilityRequirement{},
+				index.FieldCRDByName,
+				index.CRDByName).
+			Build(),
+		structuralSchemaCache: make(map[structuralSchemaCacheKey]structuralSchemaCacheValue),
+	}
+}

pkg/controllers/crdcompatibility/objectpruning/webhook.go

@@ -18,6 +18,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"sync"
 
 	apiextensionsv1alpha1 "github.com/openshift/api/apiextensions/v1alpha1"
 	apiextensionshelpers "k8s.io/apiextensions-apiserver/pkg/apihelpers"
@@ -28,6 +29,7 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/serializer"
+	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -45,19 +47,40 @@ const (
 	WebhookPrefix = "/compatibility-requirement-object-mutation/"
 )
 
+type structuralSchemaCacheKey struct {
+	// The name of the CompatibilityRequirement.
+	compatibilityRequirementName string
+	// The API version of the schema we are caching.
+	version string
+}
+
+type structuralSchemaCacheValue struct {
+	schema *structuralschema.Structural
+
+	// The UID of the CompatibilityRequirement.
+	uid types.UID
+
+	// The generation of the CompatibilityRequirement.
+	generation int64
+}
+
 // validator implements the admission.Handler to have a custom Handle function which is able to
 // validate arbitrary objects against CompatibilityRequirements by leveraging unstructured.
 type validator struct {
 	client                client.Reader
 	decoder               admission.Decoder
 	universalDeserializer runtime.Decoder
+
+	structuralSchemaCacheLock sync.RWMutex
+	structuralSchemaCache     map[structuralSchemaCacheKey]structuralSchemaCacheValue
 }
 
 // NewValidator returns a partially initialized ObjectValidator.
 func NewValidator() *validator {
 	return &validator{
 		// This decoder is only used to decode to unstructured and for CompatibilityRequirements.
-		decoder: admission.NewDecoder(runtime.NewScheme()),
+		decoder:               admission.NewDecoder(runtime.NewScheme()),
+		structuralSchemaCache: make(map[structuralSchemaCacheKey]structuralSchemaCacheValue),
 	}
 }
 
@@ -83,7 +106,7 @@ func (v *validator) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opts
 
 // handleObjectPruning handles the pruning of an object.
 func (v *validator) handleObjectPruning(ctx context.Context, compatibilityRequirementName string, obj *unstructured.Unstructured) error {
-	schema, err := v.getCopmatibilityRequirementStructuralSchema(ctx, compatibilityRequirementName, obj.GroupVersionKind().Version)
+	schema, err := v.getStructuralSchema(ctx, compatibilityRequirementName, obj.GroupVersionKind().Version)
 	if err != nil {
 		return fmt.Errorf("failed to get schema for CompatibilityRequirement %q: %w", compatibilityRequirementName, err)
 	}
@@ -93,12 +116,72 @@ func (v *validator) handleObjectPruning(ctx context.Context, compatibilityRequir
 	return nil
 }
 
-func (v *validator) getCopmatibilityRequirementStructuralSchema(ctx context.Context, compatibilityRequirementName string, version string) (*structuralschema.Structural, error) {
+func (v *validator) getStructuralSchema(ctx context.Context, compatibilityRequirementName string, version string) (*structuralschema.Structural, error) {
 	compatibilityRequirement := &apiextensionsv1alpha1.CompatibilityRequirement{}
 	if err := v.client.Get(ctx, client.ObjectKey{Name: compatibilityRequirementName}, compatibilityRequirement); err != nil {
 		return nil, fmt.Errorf("failed to get CompatibilityRequirement %q: %w", compatibilityRequirementName, err)
 	}
 
+	cacheKey := getStructuralSchemaCacheKey(compatibilityRequirement, version)
+
+	schema, ok := v.getStructuralSchemaFromCache(compatibilityRequirement, cacheKey)
+	if ok {
+		return schema, nil
+	}
+
+	v.structuralSchemaCacheLock.Lock()
+	defer v.structuralSchemaCacheLock.Unlock()
+
+	// Check the cache again under the write lock in case another thread populated the cache
+	// while we were waiting for the write lock.
+	schemaValue, ok := v.structuralSchemaCache[cacheKey]
+	if ok && isCacheEntryValid(compatibilityRequirement, schemaValue) {
+		return schemaValue.schema, nil
+	}
+
+	schema, err := v.getCompatibilityRequirementStructuralSchema(ctx, compatibilityRequirement, version)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get structural schema: %w", err)
+	}
+
+	v.storeStructuralSchemaInCache(compatibilityRequirement, cacheKey, schema)
+
+	return schema, nil
+}
+
+func getStructuralSchemaCacheKey(compatibilityRequirement *apiextensionsv1alpha1.CompatibilityRequirement, version string) structuralSchemaCacheKey {
+	return structuralSchemaCacheKey{
+		compatibilityRequirementName: compatibilityRequirement.Name,
+		version:                      version,
+	}
+}
+
+func isCacheEntryValid(compatibilityRequirement *apiextensionsv1alpha1.CompatibilityRequirement, schema structuralSchemaCacheValue) bool {
+	return compatibilityRequirement.Generation == schema.generation && compatibilityRequirement.UID == schema.uid
+}
+
+func (v *validator) getStructuralSchemaFromCache(compatibilityRequirement *apiextensionsv1alpha1.CompatibilityRequirement, cacheKey structuralSchemaCacheKey) (*structuralschema.Structural, bool) {
+	v.structuralSchemaCacheLock.RLock()
+	defer v.structuralSchemaCacheLock.RUnlock()
+
+	schema, ok := v.structuralSchemaCache[cacheKey]
+	if !ok || !isCacheEntryValid(compatibilityRequirement, schema) {
+		return nil, false
+	}
+
+	return schema.schema, true
+}
+
+func (v *validator) storeStructuralSchemaInCache(compatibilityRequirement *apiextensionsv1alpha1.CompatibilityRequirement, cacheKey structuralSchemaCacheKey, schema *structuralschema.Structural) {
+	// No locking here as we take the lock when constructing the new schema.
+	v.structuralSchemaCache[cacheKey] = structuralSchemaCacheValue{
+		schema:     schema,
+		uid:        compatibilityRequirement.UID,
+		generation: compatibilityRequirement.Generation,
+	}
+}
+
+func (v *validator) getCompatibilityRequirementStructuralSchema(ctx context.Context, compatibilityRequirement *apiextensionsv1alpha1.CompatibilityRequirement, version string) (*structuralschema.Structural, error) {
 	// Extract the CRD so we can use the schema.
 	// Use a universal deserializer as it correctly handles YAML and JSON decoding based on the expected key formatting for CRDs.
 	// N.B. DO NOT switch this to a YAML library - they do not correctly handle the OpenAPIV3Schema casing within the CRD version schema.