From d71d2f9b6b600e03fe863a33b19143542dc009b8 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Thu, 5 Mar 2026 10:14:24 +0000 Subject: [PATCH 1/7] Implement Warn Action for Object Admission --- .../objectvalidation/handle_test.go | 418 ++++++++++++++++++ .../objectvalidation/suite_test.go | 2 + .../objectvalidation/validator_unit_test.go | 29 +- .../objectvalidation/webhook.go | 88 +++- pkg/test/crdbuilder.go | 3 + pkg/test/warninghandler.go | 60 +++ 6 files changed, 560 insertions(+), 40 deletions(-) create mode 100644 pkg/test/warninghandler.go diff --git a/pkg/controllers/crdcompatibility/objectvalidation/handle_test.go b/pkg/controllers/crdcompatibility/objectvalidation/handle_test.go index 49973217a..7f05e167d 100644 --- a/pkg/controllers/crdcompatibility/objectvalidation/handle_test.go +++ b/pkg/controllers/crdcompatibility/objectvalidation/handle_test.go @@ -26,6 +26,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" apierrors "k8s.io/apimachinery/pkg/api/errors" + "sigs.k8s.io/controller-runtime/pkg/client" apiextensionsv1alpha1 "github.com/openshift/api/apiextensions/v1alpha1" "github.com/openshift/cluster-capi-operator/pkg/test" @@ -52,6 +53,16 @@ func createValidatingWebhookConfig(compatibilityRequirement *apiextensionsv1alph return validatingWebhookConfig } +// createWarningCompatibilityRequirement creates a CompatibilityRequirement with Warn action +// and minimal configuration to avoid selector validation issues. +func createWarningCompatibilityRequirement(crd *apiextensionsv1.CustomResourceDefinition) *apiextensionsv1alpha1.CompatibilityRequirement { + compatibilityRequirement := test.GenerateTestCompatibilityRequirement(crd) + compatibilityRequirement.Spec.CustomResourceDefinitionSchemaValidation.Action = apiextensionsv1alpha1.CRDAdmitActionWarn + compatibilityRequirement.Spec.ObjectSchemaValidation.Action = apiextensionsv1alpha1.CRDAdmitActionWarn + + return compatibilityRequirement +} + var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOnFailure, func() { var ( namespace string @@ -832,6 +843,413 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn }, defaultNodeTimeout) }) }) + + Describe("Warning Mode Validation", func() { + var ( + warningHandler *test.WarningHandler + warningClient client.Client + ) + + BeforeEach(func() { + // Create a new client that collects warnings in the test warning handler. + var err error + + warningHandler = test.NewTestWarningHandler() + warningConfig := *cfg + warningConfig.WarningHandlerWithContext = warningHandler + warningClient, err = client.New(&warningConfig, client.Options{}) + Expect(err).ToNot(HaveOccurred()) + }) + + Context("when ObjectSchemaValidation Action is set to Warn", func() { + var ( + warningCompatibilityRequirement *apiextensionsv1alpha1.CompatibilityRequirement + ) + + BeforeEach(func(ctx context.Context) { + // Create a CompatibilityRequirement with Warn action using utility function + warningCompatibilityRequirement = createWarningCompatibilityRequirement(compatibilityCRD.DeepCopy()) + + Expect(warningClient.Create(ctx, warningCompatibilityRequirement)).To(Succeed()) + + // Create ValidatingWebhookConfiguration for the warning requirement + warningWebhookConfig := createValidatingWebhookConfig(warningCompatibilityRequirement, baseCRD) + warningWebhookConfig.Name = fmt.Sprintf("test-warning-validation-%s", warningCompatibilityRequirement.Name) + Expect(warningClient.Create(ctx, warningWebhookConfig)).To(Succeed()) + + DeferCleanup(func(ctx context.Context) { + Expect(test.CleanupAndWait(ctx, warningClient, warningWebhookConfig, warningCompatibilityRequirement)).To(Succeed()) + }) + }, defaultNodeTimeout) + + It("should allow objects missing required fields but generate warnings", func(ctx context.Context) { + gvk := schema.GroupVersionKind{ + Group: baseCRD.Spec.Group, + Version: baseCRD.Spec.Versions[0].Name, + Kind: baseCRD.Spec.Names.Kind, + } + + invalidObj := test.NewTestObject(gvk). + WithNamespace(namespace). + WithField("requiredField", "value"). + WithNestedField("spec.replicas", int64(150)). // Above maximum of 100 + Build() + + // This should succeed despite validation failure, with warnings + err := warningClient.Create(ctx, invalidObj) + Expect(err).ToNot(HaveOccurred()) + + DeferCleanup(func(ctx context.Context) { + Expect(test.CleanupAndWait(ctx, warningClient, invalidObj)).To(Succeed()) + }) + + // Verify object was created successfully + Eventually(kWithCtx(ctx).Get(invalidObj)).WithContext(ctx).Should(Succeed()) + + Expect(warningHandler.Messages()).To(ConsistOf(ContainSubstring("Warning: spec.replicas: Invalid value: 150: spec.replicas in body should be less than or equal to 100"))) + }, defaultNodeTimeout) + + It("should allow updates changing a field to be invalid but generate warnings", func(ctx context.Context) { + gvk := schema.GroupVersionKind{ + Group: baseCRD.Spec.Group, + Version: baseCRD.Spec.Versions[0].Name, + Kind: baseCRD.Spec.Names.Kind, + } + + // First create a valid object + validObj := test.NewTestObject(gvk). + WithNamespace(namespace). + WithField("requiredField", "value"). + WithNestedField("spec.replicas", int64(100)). // At maximum of 100 + Build() + + Expect(warningClient.Create(ctx, validObj)).To(Succeed()) + + DeferCleanup(func(ctx context.Context) { + Expect(test.CleanupAndWait(ctx, warningClient, validObj)).To(Succeed()) + }) + + // Wait for object to be created + Eventually(kWithCtx(ctx).Get(validObj)).WithContext(ctx).Should(Succeed()) + Expect(warningHandler.Messages()).To(BeEmpty()) + + // Update to remove required field - should generate warning but succeed + invalidUpdate := validObj.DeepCopy() + Expect(unstructured.SetNestedField(invalidUpdate.Object, int64(150), "spec", "replicas")).To(Succeed()) + + Expect(warningClient.Update(ctx, invalidUpdate)).To(Succeed()) + + // Verify update was applied + Eventually(kWithCtx(ctx).Object(validObj)).WithContext(ctx).Should( + HaveField("Object", HaveKeyWithValue("spec", HaveKeyWithValue("replicas", int64(150)))), + ) + Expect(warningHandler.Messages()).To(ConsistOf(ContainSubstring("Warning: spec.replicas: Invalid value: 150: spec.replicas in body should be less than or equal to 100"))) + }, defaultNodeTimeout) + }) + + Context("when ObjectSchemaValidation Action is Warn for status subresource", func() { + var ( + warningStatusCompatibilityRequirement *apiextensionsv1alpha1.CompatibilityRequirement + ) + + BeforeEach(func(ctx context.Context) { + liveCRD := compatibilityCRD.DeepCopy() + Eventually(kWithCtx(ctx).Update(liveCRD, func() { + liveCRD.Spec.Versions[0].Subresources.Scale = nil + })).Should(Succeed()) + + statusCRD := compatibilityCRD.DeepCopy() + // Disable the scale subresource for these test cases + statusCRD.Spec.Versions[0].Subresources.Scale = nil + + // Create a CompatibilityRequirement with Warn action using utility function + warningStatusCompatibilityRequirement = createWarningCompatibilityRequirement(statusCRD) + Expect(warningClient.Create(ctx, warningStatusCompatibilityRequirement)).To(Succeed()) + + // Create ValidatingWebhookConfiguration for the warning requirement + warningStatusWebhookConfig := createValidatingWebhookConfig(warningStatusCompatibilityRequirement, baseCRD) + warningStatusWebhookConfig.Name = fmt.Sprintf("test-warning-status-validation-%s", warningStatusCompatibilityRequirement.Name) + Expect(warningClient.Create(ctx, warningStatusWebhookConfig)).To(Succeed()) + + DeferCleanup(func(ctx context.Context) { + Expect(test.CleanupAndWait(ctx, warningClient, warningStatusWebhookConfig, warningStatusCompatibilityRequirement)).To(Succeed()) + }) + }, defaultNodeTimeout) + + It("should allow status updates with invalid enum values but generate warnings", func(ctx context.Context) { + gvk := schema.GroupVersionKind{ + Group: baseCRD.Spec.Group, + Version: baseCRD.Spec.Versions[0].Name, + Kind: baseCRD.Spec.Names.Kind, + } + + // First create the object without status + baseObj := test.NewTestObject(gvk). + WithNamespace(namespace). + WithField("requiredField", "value"). + WithField("testField", "test-value"). + Build() + + Expect(cl.Create(ctx, baseObj)).To(Succeed()) + + DeferCleanup(func(ctx context.Context) { + Expect(test.CleanupAndWait(ctx, cl, baseObj)).To(Succeed()) + }, defaultNodeTimeout) + + // Wait for object to be created + Eventually(kWithCtx(ctx).Get(baseObj)).WithContext(ctx).Should(Succeed()) + + // Update status with invalid enum value - should generate warning but succeed + statusUpdate := baseObj.DeepCopy() + statusUpdate.Object["status"] = map[string]interface{}{ + "phase": "InvalidPhase", // Not in allowed enum values + } + + err := warningClient.Status().Update(ctx, statusUpdate) + Expect(err).ToNot(HaveOccurred()) // Should succeed despite validation failure + + // Verify status was updated despite being invalid + Eventually(kWithCtx(ctx).Object(baseObj)).WithContext(ctx).Should( + HaveField("Object", HaveKeyWithValue("status", HaveKeyWithValue("phase", "InvalidPhase"))), + ) + Expect(warningHandler.Messages()).To(ConsistOf(ContainSubstring("Warning: status.phase: Unsupported value: \"InvalidPhase\": supported values: \"Ready\", \"Pending\", \"Failed\""))) + }, defaultNodeTimeout) + + It("should allow status updates with invalid nested structures but generate warnings", func(ctx context.Context) { + gvk := schema.GroupVersionKind{ + Group: baseCRD.Spec.Group, + Version: baseCRD.Spec.Versions[0].Name, + Kind: baseCRD.Spec.Names.Kind, + } + + // First create the object without status + baseObj := test.NewTestObject(gvk). + WithNamespace(namespace). + WithField("requiredField", "value"). + WithField("testField", "test-value"). + Build() + + Expect(cl.Create(ctx, baseObj)).To(Succeed()) + + DeferCleanup(func(ctx context.Context) { + Expect(test.CleanupAndWait(ctx, cl, baseObj)).To(Succeed()) + }, defaultNodeTimeout) + + // Wait for object to be created + Eventually(kWithCtx(ctx).Get(baseObj)).WithContext(ctx).Should(Succeed()) + + // Update status with invalid nested structure - should generate warning but succeed + statusUpdate := baseObj.DeepCopy() + statusUpdate.Object["status"] = map[string]interface{}{ + "phase": "Ready", + "conditions": []interface{}{ + map[string]interface{}{ + "type": "Available", + // Missing required "status" field in condition + }, + }, + } + + err := warningClient.Status().Update(ctx, statusUpdate) + Expect(err).ToNot(HaveOccurred()) // Should succeed despite validation failure + + // Verify status was updated despite being invalid + Eventually(kWithCtx(ctx).Object(baseObj)).WithContext(ctx).Should( + HaveField("Object", HaveKeyWithValue("status", HaveKeyWithValue("phase", "Ready"))), + ) + Expect(warningHandler.Messages()).To(ConsistOf(ContainSubstring("Warning: status.conditions[0].status: Required value"))) + }, defaultNodeTimeout) + + It("should allow status updates with negative readyReplicas but generate warnings", func(ctx context.Context) { + gvk := schema.GroupVersionKind{ + Group: baseCRD.Spec.Group, + Version: baseCRD.Spec.Versions[0].Name, + Kind: baseCRD.Spec.Names.Kind, + } + + // First create the object without status + baseObj := test.NewTestObject(gvk). + WithNamespace(namespace). + WithField("requiredField", "value"). + WithField("testField", "test-value"). + Build() + + Expect(warningClient.Create(ctx, baseObj)).To(Succeed()) + + DeferCleanup(func(ctx context.Context) { + Expect(test.CleanupAndWait(ctx, warningClient, baseObj)).To(Succeed()) + }) + + // Wait for object to be created + Eventually(kWithCtx(ctx).Get(baseObj)).WithContext(ctx).Should(Succeed()) + + // Update status with negative readyReplicas - should generate warning but succeed + statusUpdate := baseObj.DeepCopy() + statusUpdate.Object["status"] = map[string]interface{}{ + "phase": "Ready", + "readyReplicas": int64(-1), // Below minimum value + } + + err := warningClient.Status().Update(ctx, statusUpdate) + Expect(err).ToNot(HaveOccurred()) // Should succeed despite validation failure + + // Verify status was updated despite being invalid + Eventually(kWithCtx(ctx).Object(baseObj)).WithContext(ctx).Should( + HaveField("Object", HaveKeyWithValue("status", HaveKeyWithValue("readyReplicas", int64(-1)))), + ) + Expect(warningHandler.Messages()).To(ConsistOf(ContainSubstring("Warning: status.readyReplicas: Invalid value: -1: status.readyReplicas in body should be greater than or equal to 0"))) + }, defaultNodeTimeout) + }) + + Context("when ObjectSchemaValidation Action is Warn for scale subresource", func() { + var ( + warningScaleCompatibilityRequirement *apiextensionsv1alpha1.CompatibilityRequirement + ) + + BeforeEach(func(ctx context.Context) { + liveCRD := compatibilityCRD.DeepCopy() + Eventually(kWithCtx(ctx).Update(liveCRD, func() { + // Disable the live CRD scale subresource else the objects will be rejected + // and we won't be able to check the warnings. + liveCRD.Spec.Versions[0].Subresources.Scale = nil + })).WithContext(ctx).Should(Succeed()) + + scaleCRD := compatibilityCRD.DeepCopy() + + // Create a CompatibilityRequirement with Warn action using utility function + warningScaleCompatibilityRequirement = createWarningCompatibilityRequirement(scaleCRD) + Expect(warningClient.Create(ctx, warningScaleCompatibilityRequirement)).To(Succeed()) + + // Create ValidatingWebhookConfiguration for the warning requirement + warningScaleWebhookConfig := createValidatingWebhookConfig(warningScaleCompatibilityRequirement, baseCRD) + warningScaleWebhookConfig.Name = fmt.Sprintf("test-warning-scale-validation-%s", warningScaleCompatibilityRequirement.Name) + Expect(warningClient.Create(ctx, warningScaleWebhookConfig)).To(Succeed()) + + DeferCleanup(func(ctx context.Context) { + Expect(test.CleanupAndWait(ctx, cl, warningScaleWebhookConfig, warningScaleCompatibilityRequirement)).To(Succeed()) + }) + }, defaultNodeTimeout) + + It("should allow objects with replica count above maximum but generate warnings", func(ctx context.Context) { + gvk := schema.GroupVersionKind{ + Group: baseCRD.Spec.Group, + Version: baseCRD.Spec.Versions[0].Name, + Kind: baseCRD.Spec.Names.Kind, + } + + objWithTooManyReplicas := test.NewTestObject(gvk). + WithNamespace(namespace). + WithField("requiredField", "value"). + WithField("testField", "test-value"). + WithField("spec", map[string]interface{}{ + "replicas": int64(150), // Above maximum of 100 + "selector": map[string]interface{}{ + "matchLabels": map[string]interface{}{ + "app": "test-app", + }, + }, + }). + Build() + + err := warningClient.Create(ctx, objWithTooManyReplicas) + Expect(err).ToNot(HaveOccurred()) // Should succeed despite validation failure + + DeferCleanup(func(ctx context.Context) { + Expect(test.CleanupAndWait(ctx, warningClient, objWithTooManyReplicas)).To(Succeed()) + }) + + // Verify object was created despite being invalid + Eventually(kWithCtx(ctx).Get(objWithTooManyReplicas)).WithContext(ctx).Should(Succeed()) + Expect(warningHandler.Messages()).To(ConsistOf(ContainSubstring("Warning: spec.replicas: Invalid value: 150: spec.replicas in body should be less than or equal to 100"))) + }, defaultNodeTimeout) + + It("should allow objects with negative replica count but generate warnings", func(ctx context.Context) { + gvk := schema.GroupVersionKind{ + Group: baseCRD.Spec.Group, + Version: baseCRD.Spec.Versions[0].Name, + Kind: baseCRD.Spec.Names.Kind, + } + + objWithNegativeReplicas := test.NewTestObject(gvk). + WithNamespace(namespace). + WithField("requiredField", "value"). + WithField("testField", "test-value"). + WithField("spec", map[string]interface{}{ + "replicas": int64(-1), // Below minimum of 0 + "selector": map[string]interface{}{ + "matchLabels": map[string]interface{}{ + "app": "test-app", + }, + }, + }). + Build() + + err := warningClient.Create(ctx, objWithNegativeReplicas) + Expect(err).ToNot(HaveOccurred()) // Should succeed despite validation failure + + DeferCleanup(func(ctx context.Context) { + Expect(test.CleanupAndWait(ctx, warningClient, objWithNegativeReplicas)).To(Succeed()) + }) + + // Verify object was created despite being invalid + Eventually(kWithCtx(ctx).Get(objWithNegativeReplicas)).WithContext(ctx).Should(Succeed()) + Expect(warningHandler.Messages()).To(ConsistOf( + ContainSubstring("Warning: spec.replicas: Invalid value: -1: spec.replicas in body should be greater than or equal to 0"), // Minimum validation + ContainSubstring("Warning: .spec.replicas: Invalid value: -1: should be a non-negative integer"), // Scale subresource validation + )) + }, defaultNodeTimeout) + + It("should allow status updates with negative readyReplicas but generate warnings", func(ctx context.Context) { + gvk := schema.GroupVersionKind{ + Group: baseCRD.Spec.Group, + Version: baseCRD.Spec.Versions[0].Name, + Kind: baseCRD.Spec.Names.Kind, + } + + // First create the object with valid spec + baseObj := test.NewTestObject(gvk). + WithNamespace(namespace). + WithField("requiredField", "value"). + WithField("testField", "test-value"). + WithField("spec", map[string]interface{}{ + "replicas": int64(3), + "selector": map[string]interface{}{ + "matchLabels": map[string]interface{}{ + "app": "test-app", + }, + }, + }). + Build() + + Expect(warningClient.Create(ctx, baseObj)).To(Succeed()) + + DeferCleanup(func(ctx context.Context) { + Expect(test.CleanupAndWait(ctx, warningClient, baseObj)).To(Succeed()) + }) + + // Wait for object to be created + Eventually(kWithCtx(ctx).Get(baseObj)).WithContext(ctx).Should(Succeed()) + + // Update status with negative readyReplicas - should generate warning but succeed + statusUpdate := baseObj.DeepCopy() + statusUpdate.Object["status"] = map[string]interface{}{ + "readyReplicas": int64(-1), // Below minimum of 0 + } + + err := warningClient.Status().Update(ctx, statusUpdate) + Expect(err).ToNot(HaveOccurred()) // Should succeed despite validation failure + + // Verify status was updated despite being invalid + Eventually(kWithCtx(ctx).Object(baseObj)).WithContext(ctx).Should( + HaveField("Object", HaveKeyWithValue("status", HaveKeyWithValue("readyReplicas", int64(-1)))), + ) + Expect(warningHandler.Messages()).To(ConsistOf( + ContainSubstring("Warning: status.readyReplicas: Invalid value: -1: status.readyReplicas in body should be greater than or equal to 0"), // Minimum validation + ContainSubstring("Warning: .status.readyReplicas: Invalid value: -1: should be a non-negative integer"), // Scale subresource validation + )) + }, defaultNodeTimeout) + }) + }) }) func TestCompatibilityRequirementContext(t *testing.T) { diff --git a/pkg/controllers/crdcompatibility/objectvalidation/suite_test.go b/pkg/controllers/crdcompatibility/objectvalidation/suite_test.go index be43d8fc1..6fb8c27a6 100644 --- a/pkg/controllers/crdcompatibility/objectvalidation/suite_test.go +++ b/pkg/controllers/crdcompatibility/objectvalidation/suite_test.go @@ -31,6 +31,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/envtest" "sigs.k8s.io/controller-runtime/pkg/envtest/komega" logf "sigs.k8s.io/controller-runtime/pkg/log" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" "sigs.k8s.io/controller-runtime/pkg/webhook" "github.com/openshift/cluster-capi-operator/pkg/test" @@ -97,6 +98,7 @@ func initValidator(ctx context.Context, cfg *rest.Config, scheme *runtime.Scheme Port: testEnv.WebhookInstallOptions.LocalServingPort, Host: testEnv.WebhookInstallOptions.LocalServingHost, }), + Metrics: metricsserver.Options{BindAddress: "0"}, }) Expect(err).ToNot(HaveOccurred(), "Manager should be created") diff --git a/pkg/controllers/crdcompatibility/objectvalidation/validator_unit_test.go b/pkg/controllers/crdcompatibility/objectvalidation/validator_unit_test.go index 7c5f17fcc..f2965d3a5 100644 --- a/pkg/controllers/crdcompatibility/objectvalidation/validator_unit_test.go +++ b/pkg/controllers/crdcompatibility/objectvalidation/validator_unit_test.go @@ -17,8 +17,6 @@ limitations under the License. package objectvalidation import ( - "context" - . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -34,14 +32,11 @@ import ( 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 = test.NewCRDSchemaBuilder(). WithStringProperty("testField"). @@ -62,7 +57,7 @@ var _ = Describe("validator", func() { }) It("should return validation strategy", func() { - strategy, err := validator.getValidationStrategy(ctx, compatibilityRequirement.Name, "v1") + strategy, err := validator.getValidationStrategy(compatibilityRequirement, "v1") Expect(err).NotTo(HaveOccurred()) Expect(strategy).NotTo(BeNil()) @@ -70,7 +65,7 @@ var _ = Describe("validator", func() { It("should cache validation strategy", func() { // First call should create and cache the strategy - _, err := validator.getValidationStrategy(ctx, compatibilityRequirement.Name, "v1") + _, err := validator.getValidationStrategy(compatibilityRequirement, "v1") Expect(err).NotTo(HaveOccurred()) // Check that cache now contains an entry @@ -83,11 +78,11 @@ var _ = Describe("validator", func() { It("should use cached strategy on subsequent calls", func() { // First call - strategy1, err1 := validator.getValidationStrategy(ctx, compatibilityRequirement.Name, "v1") + strategy1, err1 := validator.getValidationStrategy(compatibilityRequirement, "v1") Expect(err1).NotTo(HaveOccurred()) // Second call should return the same strategy instance - strategy2, err2 := validator.getValidationStrategy(ctx, compatibilityRequirement.Name, "v1") + strategy2, err2 := validator.getValidationStrategy(compatibilityRequirement, "v1") Expect(err2).NotTo(HaveOccurred()) // Should be the exact same object (cached) @@ -97,30 +92,20 @@ var _ = Describe("validator", func() { It("should invalidate cache when generation changes", func() { // First call - strategy1, err1 := validator.getValidationStrategy(ctx, compatibilityRequirement.Name, "v1") + strategy1, err1 := validator.getValidationStrategy(compatibilityRequirement, "v1") Expect(err1).NotTo(HaveOccurred()) compatibilityRequirement.Generation++ validator.client = createValidatorWithFakeClient([]client.Object{compatibilityRequirement}).client // Second call should return the same strategy instance - strategy2, err2 := validator.getValidationStrategy(ctx, compatibilityRequirement.Name, "v1") + strategy2, err2 := validator.getValidationStrategy(compatibilityRequirement, "v1") Expect(err2).NotTo(HaveOccurred()) Expect(strategy1).NotTo(Equal(strategy2)) }) }) - Context("when CompatibilityRequirement does not exist", func() { - It("should return error", func() { - validator := createValidatorWithFakeClient([]client.Object{}) // No objects - - _, err := validator.getValidationStrategy(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 := test.GenerateTestCompatibilityRequirement(testCRD) @@ -128,7 +113,7 @@ var _ = Describe("validator", func() { validator := createValidatorWithFakeClient([]client.Object{brokenCompatibilityRequirement}) - _, err := validator.getValidationStrategy(ctx, brokenCompatibilityRequirement.Name, "v1") + _, err := validator.getValidationStrategy(brokenCompatibilityRequirement, "v1") Expect(err).To(MatchError("failed to create validation strategy: failed to decode compatibility schema data for CompatibilityRequirement \"\": yaml: mapping values are not allowed in this context")) }) diff --git a/pkg/controllers/crdcompatibility/objectvalidation/webhook.go b/pkg/controllers/crdcompatibility/objectvalidation/webhook.go index cb9ffef75..a433a677e 100644 --- a/pkg/controllers/crdcompatibility/objectvalidation/webhook.go +++ b/pkg/controllers/crdcompatibility/objectvalidation/webhook.go @@ -38,6 +38,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/registry/rest" "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" @@ -54,8 +55,9 @@ const ( ) var ( - errObjectValidator = errors.New("failed to create the object validator") - errUnexpectedObjectType = errors.New("unexpected object type") + errObjectValidator = errors.New("failed to create the object validator") + errUnexpectedObjectType = errors.New("unexpected object type") + errUnknownCRDAdmitAction = errors.New("unknown CRD admit action") ) var _ admission.Handler = &validator{} @@ -119,7 +121,18 @@ func (v *validator) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opts // ValidateCreate validates the creation of an object. func (v *validator) ValidateCreate(ctx context.Context, compatibilityRequirementName string, obj *unstructured.Unstructured) (admission.Warnings, error) { - strategy, err := v.getValidationStrategy(ctx, compatibilityRequirementName, obj.GroupVersionKind().Version) + 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) + } + + if !isObjectValidationWebhookEnabled(compatibilityRequirement) { + // The webhook should not be configured, so the controller should remove the VWC and we should no longer + // receive requests. Before it gets there, ignore any requests we do receive. + return nil, nil + } + + strategy, err := v.getValidationStrategy(compatibilityRequirement, obj.GroupVersionKind().Version) if err != nil { return nil, fmt.Errorf("failed to get validation strategy: %w", err) } @@ -127,16 +140,37 @@ func (v *validator) ValidateCreate(ctx context.Context, compatibilityRequirement errs := strategy.Validate(ctx, obj) warnings := strategy.WarningsOnCreate(ctx, obj) - if len(errs) > 0 { - return warnings, apierrors.NewInvalid(obj.GroupVersionKind().GroupKind(), obj.GetName(), errs) - } + switch compatibilityRequirement.Spec.ObjectSchemaValidation.Action { + case apiextensionsv1alpha1.CRDAdmitActionWarn: + warnings = append(warnings, util.SliceMap(errs, errorToString)...) - return warnings, nil + return warnings, nil + case apiextensionsv1alpha1.CRDAdmitActionDeny: + if len(errs) > 0 { + return warnings, apierrors.NewInvalid(obj.GroupVersionKind().GroupKind(), obj.GetName(), errs) + } + + return warnings, nil + default: + // This should be impossible as validation on the action is enforced by openapi as an enum. + return nil, fmt.Errorf("%w: %q", errUnknownCRDAdmitAction, compatibilityRequirement.Spec.ObjectSchemaValidation.Action) + } } // ValidateUpdate validates the update of an object. func (v *validator) ValidateUpdate(ctx context.Context, compatibilityRequirementName string, oldObj, obj *unstructured.Unstructured) (admission.Warnings, error) { - strategy, err := v.getValidationStrategy(ctx, compatibilityRequirementName, obj.GroupVersionKind().Version) + 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) + } + + if !isObjectValidationWebhookEnabled(compatibilityRequirement) { + // The webhook should not be configured, so the controller should remove the VWC and we should no longer + // receive requests. Before it gets there, ignore any requests we do receive. + return nil, nil + } + + strategy, err := v.getValidationStrategy(compatibilityRequirement, obj.GroupVersionKind().Version) if err != nil { return nil, fmt.Errorf("failed to get validation strategy: %w", err) } @@ -144,11 +178,21 @@ func (v *validator) ValidateUpdate(ctx context.Context, compatibilityRequirement errs := strategy.ValidateUpdate(ctx, obj, oldObj) warnings := strategy.WarningsOnUpdate(ctx, obj, oldObj) - if len(errs) > 0 { - return warnings, apierrors.NewInvalid(obj.GroupVersionKind().GroupKind(), obj.GetName(), errs) - } + switch compatibilityRequirement.Spec.ObjectSchemaValidation.Action { + case apiextensionsv1alpha1.CRDAdmitActionWarn: + warnings = append(warnings, util.SliceMap(errs, errorToString)...) + + return warnings, nil + case apiextensionsv1alpha1.CRDAdmitActionDeny: + if len(errs) > 0 { + return warnings, apierrors.NewInvalid(obj.GroupVersionKind().GroupKind(), obj.GetName(), errs) + } - return warnings, nil + return warnings, nil + default: + // This should be impossible as validation on the action is enforced by openapi as an enum. + return nil, fmt.Errorf("%w: %q", errUnknownCRDAdmitAction, compatibilityRequirement.Spec.ObjectSchemaValidation.Action) + } } // ValidateDelete validates the deletion of an object. @@ -156,12 +200,7 @@ func (v *validator) ValidateDelete(ctx context.Context, compatibilityRequirement return nil, nil } -func (v *validator) getValidationStrategy(ctx context.Context, compatibilityRequirementName string, version string) (rest.RESTCreateUpdateStrategy, 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) - } - +func (v *validator) getValidationStrategy(compatibilityRequirement *apiextensionsv1alpha1.CompatibilityRequirement, version string) (rest.RESTCreateUpdateStrategy, error) { cacheKey := getValidationStrategyCacheKey(compatibilityRequirement, version) strategy, ok := v.getValidationStrategyFromCache(compatibilityRequirement, cacheKey) @@ -412,3 +451,16 @@ func ValidatingWebhookConfigurationFor(obj *apiextensionsv1alpha1.CompatibilityR return vwc } + +func errorToString(err *field.Error) string { + return err.Error() +} + +func isObjectValidationWebhookEnabled(obj *apiextensionsv1alpha1.CompatibilityRequirement) bool { + osv := obj.Spec.ObjectSchemaValidation + return osv.Action != "" || len(osv.MatchConditions) > 0 || !labelSelectorIsEmpty(osv.NamespaceSelector) || !labelSelectorIsEmpty(osv.ObjectSelector) +} + +func labelSelectorIsEmpty(ls metav1.LabelSelector) bool { + return len(ls.MatchLabels) == 0 && len(ls.MatchExpressions) == 0 +} diff --git a/pkg/test/crdbuilder.go b/pkg/test/crdbuilder.go index f8151a9b0..109b17822 100644 --- a/pkg/test/crdbuilder.go +++ b/pkg/test/crdbuilder.go @@ -245,6 +245,9 @@ func GenerateTestCompatibilityRequirement(testCRD *apiextensionsv1.CustomResourc CustomResourceDefinitionSchemaValidation: apiextensionsv1alpha1.CustomResourceDefinitionSchemaValidation{ Action: apiextensionsv1alpha1.CRDAdmitActionDeny, }, + ObjectSchemaValidation: apiextensionsv1alpha1.ObjectSchemaValidation{ + Action: apiextensionsv1alpha1.CRDAdmitActionDeny, + }, }, Status: apiextensionsv1alpha1.CompatibilityRequirementStatus{ CRDName: testCRD.GetName(), diff --git a/pkg/test/warninghandler.go b/pkg/test/warninghandler.go new file mode 100644 index 000000000..96109555c --- /dev/null +++ b/pkg/test/warninghandler.go @@ -0,0 +1,60 @@ +/* +Copyright 2026 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 test + +import ( + "bytes" + "context" + "strings" + + "k8s.io/client-go/rest" +) + +// WarningHandler is a test implementation of the rest.WarningHandler interface. +type WarningHandler struct { + inner rest.WarningHandler + + buf *bytes.Buffer +} + +// NewTestWarningHandler creates a new WarningHandler. +func NewTestWarningHandler() *WarningHandler { + var buf bytes.Buffer + + return &WarningHandler{ + inner: rest.NewWarningWriter(&buf, rest.WarningWriterOptions{}), + buf: &buf, + } +} + +// HandleWarningHeader is a test implementation of the rest.WarningHandler.HandleWarningHeader method. +func (w *WarningHandler) HandleWarningHeader(code int, agent string, text string) { + w.inner.HandleWarningHeader(code, agent, text) +} + +// HandleWarningHeaderWithContext is a test implementation of the rest.WarningHandler.HandleWarningHeaderWithContext method. +func (w *WarningHandler) HandleWarningHeaderWithContext(_ context.Context, code int, agent string, text string) { + w.inner.HandleWarningHeader(code, agent, text) +} + +// Messages returns the messages from the warning handler. +func (w *WarningHandler) Messages() []string { + if w.buf.Len() == 0 { + return nil + } + + return strings.Split(strings.TrimSuffix(w.buf.String(), "\n"), "\n") +} From 4e9492ae4747a79b38cbe50c306e6552df85b6e1 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Fri, 6 Mar 2026 14:14:08 +0000 Subject: [PATCH 2/7] Implement warn behaviour in object pruning webhook --- .../objectpruning/handle_test.go | 72 ++++++++++++++++--- .../objectpruning/validator_unit_test.go | 25 ++----- .../crdcompatibility/objectpruning/webhook.go | 33 +++++++-- 3 files changed, 93 insertions(+), 37 deletions(-) diff --git a/pkg/controllers/crdcompatibility/objectpruning/handle_test.go b/pkg/controllers/crdcompatibility/objectpruning/handle_test.go index 343df5d5c..e79332da4 100644 --- a/pkg/controllers/crdcompatibility/objectpruning/handle_test.go +++ b/pkg/controllers/crdcompatibility/objectpruning/handle_test.go @@ -95,9 +95,9 @@ var _ = Describe("Object Pruning Integration", func() { By("Creating object through API server (should be pruned by webhook)") // Set the namespace and ensure object matches the CRD GVK gvk := liveCRD.Spec.Versions[0].Name - inputObject := &unstructured.Unstructured{ + inputObject := (&unstructured.Unstructured{ Object: scenario.InputObject, - } + }).DeepCopy() inputObject.SetAPIVersion(fmt.Sprintf("%s/%s", liveCRD.Spec.Group, gvk)) inputObject.SetKind(liveCRD.Spec.Names.Kind) inputObject.SetNamespace(namespace) @@ -126,16 +126,65 @@ var _ = Describe("Object Pruning Integration", func() { Expect(cl.Status().Update(ctx, inputObject)).To(Succeed()) } - By("Verifying the object was pruned correctly") + By("Verifying the object was pruned correctly", func() { + retrievedObj := &unstructured.Unstructured{} + retrievedObj.SetGroupVersionKind(inputObject.GroupVersionKind()) + retrievedObj.SetName(inputObject.GetName()) + retrievedObj.SetNamespace(inputObject.GetNamespace()) + + Eventually(kWithCtx(ctx).Get(retrievedObj)).WithContext(ctx).Should(Succeed()) + + Expect(retrievedObj.Object).To(test.IgnoreFields([]string{"apiVersion", "kind", "metadata"}, Equal(scenario.ExpectedObject)), "Expected object to be pruned correctly") + }) + + By("Attempting to update the object, should prune the object again", func() { + inputObject.Object["spec"] = scenario.InputObject["spec"] + Expect(cl.Update(ctx, inputObject)).To(Succeed()) + }) + + // Write the status through the status subresource. + if hasStatus { + inputObject.Object["status"] = status + Expect(cl.Status().Update(ctx, inputObject)).To(Succeed()) + } + + By("Verifying the object was pruned correctly", func() { + retrievedObj := &unstructured.Unstructured{} + retrievedObj.SetGroupVersionKind(inputObject.GroupVersionKind()) + retrievedObj.SetName(inputObject.GetName()) + retrievedObj.SetNamespace(inputObject.GetNamespace()) - retrievedObj := &unstructured.Unstructured{} - retrievedObj.SetGroupVersionKind(inputObject.GroupVersionKind()) - retrievedObj.SetName(inputObject.GetName()) - retrievedObj.SetNamespace(inputObject.GetNamespace()) + Eventually(kWithCtx(ctx).Get(retrievedObj)).WithContext(ctx).Should(Succeed()) - Eventually(kWithCtx(ctx).Get(retrievedObj)).WithContext(ctx).Should(Succeed()) + Expect(retrievedObj.Object).To(test.IgnoreFields([]string{"apiVersion", "kind", "metadata"}, Equal(scenario.ExpectedObject)), "Expected object to be pruned correctly") + }) - Expect(retrievedObj.Object).To(test.IgnoreFields([]string{"apiVersion", "kind", "metadata"}, Equal(scenario.ExpectedObject))) + By("Updating the compatibility requirement to warn action") + Eventually(kWithCtx(ctx).Update(scenario.CompatibilityRequirement, func() { + scenario.CompatibilityRequirement.Spec.ObjectSchemaValidation.Action = apiextensionsv1alpha1.CRDAdmitActionWarn + })).WithContext(ctx).Should(Succeed()) + + By("Updating the object again, should not be pruned", func() { + inputObject.Object["spec"] = scenario.InputObject["spec"] + Expect(cl.Update(ctx, inputObject)).To(Succeed()) + }) + + // Write the status through the status subresource. + if hasStatus { + inputObject.Object["status"] = status + Expect(cl.Status().Update(ctx, inputObject)).To(Succeed()) + } + + By("Verifying the object was not pruned", func() { + retrievedObj := &unstructured.Unstructured{} + retrievedObj.SetGroupVersionKind(inputObject.GroupVersionKind()) + retrievedObj.SetName(inputObject.GetName()) + retrievedObj.SetNamespace(inputObject.GetNamespace()) + + Eventually(kWithCtx(ctx).Get(retrievedObj)).WithContext(ctx).Should(Succeed()) + + Expect(retrievedObj.Object).To(test.IgnoreFields([]string{"apiVersion", "kind", "metadata"}, Equal(scenario.InputObject)), "Expected object to be not pruned") + }) }, Entry("object with unknown fields pruned by strict compatibility requirement", pruningTestScenario{ @@ -261,7 +310,7 @@ var _ = Describe("Object Pruning Integration", func() { "spec": map[string]interface{}{ "field1": "removeThis", "field2": "alsoRemove", - "field3": 42, + "field3": int64(42), }, "status": map[string]interface{}{ "phase": "Running", @@ -526,6 +575,9 @@ func createCompatibilityRequirement(crd *apiextensionsv1.CustomResourceDefinitio DefaultSelection: apiextensionsv1alpha1.APIVersionSetTypeAllServed, }, }, + ObjectSchemaValidation: apiextensionsv1alpha1.ObjectSchemaValidation{ + Action: apiextensionsv1alpha1.CRDAdmitActionDeny, + }, }, } } diff --git a/pkg/controllers/crdcompatibility/objectpruning/validator_unit_test.go b/pkg/controllers/crdcompatibility/objectpruning/validator_unit_test.go index 09de5e9cc..7fe4aedb5 100644 --- a/pkg/controllers/crdcompatibility/objectpruning/validator_unit_test.go +++ b/pkg/controllers/crdcompatibility/objectpruning/validator_unit_test.go @@ -17,8 +17,6 @@ limitations under the License. package objectpruning import ( - "context" - . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -33,14 +31,11 @@ import ( 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() @@ -57,7 +52,7 @@ var _ = Describe("validator", func() { }) It("should return structural schema", func() { - schema, err := validator.getStructuralSchema(ctx, compatibilityRequirement.Name, "v1") + schema, err := validator.getStructuralSchema(compatibilityRequirement, "v1") Expect(err).NotTo(HaveOccurred()) Expect(schema).NotTo(BeNil()) @@ -65,7 +60,7 @@ var _ = Describe("validator", func() { It("should cache structural schema", func() { // First call should create and cache the schema - _, err := validator.getStructuralSchema(ctx, compatibilityRequirement.Name, "v1") + _, err := validator.getStructuralSchema(compatibilityRequirement, "v1") Expect(err).NotTo(HaveOccurred()) // Check that cache now contains an entry @@ -78,11 +73,11 @@ var _ = Describe("validator", func() { It("should use cached schema on subsequent calls", func() { // First call - schema1, err1 := validator.getStructuralSchema(ctx, compatibilityRequirement.Name, "v1") + schema1, err1 := validator.getStructuralSchema(compatibilityRequirement, "v1") Expect(err1).NotTo(HaveOccurred()) // Second call should return the same schema instance - schema2, err2 := validator.getStructuralSchema(ctx, compatibilityRequirement.Name, "v1") + schema2, err2 := validator.getStructuralSchema(compatibilityRequirement, "v1") Expect(err2).NotTo(HaveOccurred()) // Should be the exact same object (cached) @@ -91,16 +86,6 @@ var _ = Describe("validator", func() { }) }) - 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) @@ -108,7 +93,7 @@ var _ = Describe("validator", func() { validator := createValidatorWithFakeClient([]client.Object{brokenCompatibilityRequirement}) - _, err := validator.getStructuralSchema(ctx, brokenCompatibilityRequirement.Name, "v1") + _, err := validator.getStructuralSchema(brokenCompatibilityRequirement, "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"))) }) diff --git a/pkg/controllers/crdcompatibility/objectpruning/webhook.go b/pkg/controllers/crdcompatibility/objectpruning/webhook.go index ef5ff9000..02caaef8e 100644 --- a/pkg/controllers/crdcompatibility/objectpruning/webhook.go +++ b/pkg/controllers/crdcompatibility/objectpruning/webhook.go @@ -111,7 +111,22 @@ 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.getStructuralSchema(ctx, compatibilityRequirementName, obj.GroupVersionKind().Version) + compatibilityRequirement := &apiextensionsv1alpha1.CompatibilityRequirement{} + if err := v.client.Get(ctx, client.ObjectKey{Name: compatibilityRequirementName}, compatibilityRequirement); err != nil { + return fmt.Errorf("failed to get CompatibilityRequirement %q: %w", compatibilityRequirementName, err) + } + + switch { + case !isObjectValidationWebhookEnabled(compatibilityRequirement): + // The webhook should not be configured, so the controller should remove the MWC and we should no longer + // receive requests. Before it gets there, ignore any requests we do receive. + return nil + case compatibilityRequirement.Spec.ObjectSchemaValidation.Action == apiextensionsv1alpha1.CRDAdmitActionWarn: + // When set to warn, we do not expect to mutate the object, so we return early without pruning. + return nil + } + + schema, err := v.getStructuralSchema(compatibilityRequirement, obj.GroupVersionKind().Version) if err != nil { return fmt.Errorf("failed to get schema for CompatibilityRequirement %q: %w", compatibilityRequirementName, err) } @@ -121,12 +136,7 @@ func (v *validator) handleObjectPruning(ctx context.Context, compatibilityRequir return nil } -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) - } - +func (v *validator) getStructuralSchema(compatibilityRequirement *apiextensionsv1alpha1.CompatibilityRequirement, version string) (*structuralschema.Structural, error) { cacheKey := getStructuralSchemaCacheKey(compatibilityRequirement, version) schema, ok := v.getStructuralSchemaFromCache(compatibilityRequirement, cacheKey) @@ -288,3 +298,12 @@ func MutatingWebhookConfigurationFor(obj *apiextensionsv1alpha1.CompatibilityReq return vwc } + +func isObjectValidationWebhookEnabled(obj *apiextensionsv1alpha1.CompatibilityRequirement) bool { + osv := obj.Spec.ObjectSchemaValidation + return osv.Action != "" || len(osv.MatchConditions) > 0 || !labelSelectorIsEmpty(osv.NamespaceSelector) || !labelSelectorIsEmpty(osv.ObjectSelector) +} + +func labelSelectorIsEmpty(ls metav1.LabelSelector) bool { + return len(ls.MatchLabels) == 0 && len(ls.MatchExpressions) == 0 +} From 9e5745b52adc4ae555c13572c1b5e94b2d77cb21 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Fri, 6 Mar 2026 17:03:31 +0000 Subject: [PATCH 3/7] Fixup existing reconcile tests --- .../crdcompatibility/reconcile_test.go | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/pkg/controllers/crdcompatibility/reconcile_test.go b/pkg/controllers/crdcompatibility/reconcile_test.go index 2d9bc9774..187f2a301 100644 --- a/pkg/controllers/crdcompatibility/reconcile_test.go +++ b/pkg/controllers/crdcompatibility/reconcile_test.go @@ -46,17 +46,18 @@ var _ = Describe("CompatibilityRequirement", Ordered, ContinueOnFailure, func() var ( // testCRDClean is a CRD generated by generateTestCRD. - // testCRDWorking is a copy of testCRDClean with runtime metadata. testCRDClean, testCRDWorking *apiextensionsv1.CustomResourceDefinition ) BeforeEach(func() { testCRDClean = test.GenerateTestCRD() - testCRDWorking = testCRDClean.DeepCopy() }) Context("When creating a CompatibilityRequirement", func() { + var testCRDWorking *apiextensionsv1.CustomResourceDefinition + BeforeEach(func(ctx context.Context) { + testCRDWorking = testCRDClean.DeepCopy() createTestObject(ctx, testCRDWorking, "CRD") }, defaultNodeTimeout) @@ -190,7 +191,7 @@ var _ = Describe("CompatibilityRequirement", Ordered, ContinueOnFailure, func() requirement = test.GenerateTestCompatibilityRequirement(incompatibleCRD) createTestObject(ctx, requirement, "CompatibilityRequirement") - }) + }, defaultNodeTimeout) It("Should set not Compatible", func(ctx context.Context) { By("Checking that the CompatibilityRequirement is admitted but not compatible") @@ -296,7 +297,7 @@ var _ = Describe("CompatibilityRequirement", Ordered, ContinueOnFailure, func() // We leverage a function in the objectvalidation package to create the validating webhook configuration. // Smoke test here that the right name and service details are used, the full spec is tested with the objectvalidation package. - Eventually(kWithCtx(ctx).Object(validatingWebhookConfig)).Should(SatisfyAll( + Eventually(kWithCtx(ctx).Object(validatingWebhookConfig)).WithContext(ctx).Should(SatisfyAll( HaveField("ObjectMeta.Name", BeEquivalentTo(requirement.Name)), HaveField("Webhooks", ConsistOf(SatisfyAll( HaveField("Name", BeEquivalentTo("compatibilityrequirement.operator.openshift.io")), @@ -304,7 +305,7 @@ var _ = Describe("CompatibilityRequirement", Ordered, ContinueOnFailure, func() HaveField("ClientConfig.Service.Namespace", BeEquivalentTo("openshift-compatibility-requirements-operator")), ))), )) - }) + }, defaultNodeTimeout) It("Should create a mutating webhook configuration to implement the compatibility requirement", func(ctx context.Context) { mutatingWebhookConfig := &admissionregistrationv1.MutatingWebhookConfiguration{ @@ -315,7 +316,7 @@ var _ = Describe("CompatibilityRequirement", Ordered, ContinueOnFailure, func() // We leverage a function in the objectpruning package to create the mutating webhook configuration. // Smoke test here that the right name and service details are used, the full spec is tested with the objectpruning package. - Eventually(kWithCtx(ctx).Object(mutatingWebhookConfig)).Should(SatisfyAll( + Eventually(kWithCtx(ctx).Object(mutatingWebhookConfig)).WithContext(ctx).Should(SatisfyAll( HaveField("ObjectMeta.Name", BeEquivalentTo(requirement.Name)), HaveField("Webhooks", ConsistOf(SatisfyAll( HaveField("Name", BeEquivalentTo("compatibilityrequirement.operator.openshift.io")), @@ -323,7 +324,7 @@ var _ = Describe("CompatibilityRequirement", Ordered, ContinueOnFailure, func() HaveField("ClientConfig.Service.Namespace", BeEquivalentTo("openshift-compatibility-requirements-operator")), ))), )) - }) + }, defaultNodeTimeout) It("Should delete the validating webhook configuration when the CompatibilityRequirement is deleted", func(ctx context.Context) { validatingWebhookConfig := &admissionregistrationv1.ValidatingWebhookConfiguration{ @@ -333,8 +334,8 @@ var _ = Describe("CompatibilityRequirement", Ordered, ContinueOnFailure, func() } Expect(cl.Delete(ctx, requirement)).To(Succeed()) - Eventually(kWithCtx(ctx).Get(validatingWebhookConfig)).Should(test.BeK8SNotFound()) - }) + Eventually(kWithCtx(ctx).Get(validatingWebhookConfig)).WithContext(ctx).Should(test.BeK8SNotFound()) + }, defaultNodeTimeout) It("Should delete the mutating webhook configuration when the CompatibilityRequirement is deleted", func(ctx context.Context) { mutatingWebhookConfig := &admissionregistrationv1.MutatingWebhookConfiguration{ @@ -344,8 +345,8 @@ var _ = Describe("CompatibilityRequirement", Ordered, ContinueOnFailure, func() } // No need to delete as we are in an ordered context and the previous test removes the compatibility requirement. - Eventually(kWithCtx(ctx).Get(mutatingWebhookConfig)).Should(test.BeK8SNotFound()) - }) + Eventually(kWithCtx(ctx).Get(mutatingWebhookConfig)).WithContext(ctx).Should(test.BeK8SNotFound()) + }, defaultNodeTimeout) }) Context("When creating or modifying a CRD", func() { @@ -384,7 +385,7 @@ var _ = Describe("CompatibilityRequirement", Ordered, ContinueOnFailure, func() ) BeforeEach(func(ctx context.Context) { - createTestObject(ctx, testCRDWorking, "CRD") + createTestObject(ctx, testCRDClean.DeepCopy(), "CRD") requirement = test.GenerateTestCompatibilityRequirement(testCRDClean) createTestObject(ctx, requirement, "CompatibilityRequirement") From a83a3af22e4634b56412a9f4c040af3d6f90f21f Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Fri, 6 Mar 2026 17:11:15 +0000 Subject: [PATCH 4/7] Ensure reconciliation removes VWC/MWC when not applicable --- .../crdcompatibility/objectpruning/webhook.go | 1 + .../objectvalidation/webhook.go | 1 + pkg/controllers/crdcompatibility/reconcile.go | 44 +++++++++++++-- .../crdcompatibility/reconcile_test.go | 56 +++++++++++++++++++ 4 files changed, 97 insertions(+), 5 deletions(-) diff --git a/pkg/controllers/crdcompatibility/objectpruning/webhook.go b/pkg/controllers/crdcompatibility/objectpruning/webhook.go index 02caaef8e..8b7292f3a 100644 --- a/pkg/controllers/crdcompatibility/objectpruning/webhook.go +++ b/pkg/controllers/crdcompatibility/objectpruning/webhook.go @@ -245,6 +245,7 @@ func MutatingWebhookConfigurationFor(obj *apiextensionsv1alpha1.CompatibilityReq Annotations: map[string]string{ "service.beta.openshift.io/inject-cabundle": "true", }, + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(obj, apiextensionsv1alpha1.GroupVersion.WithKind("CompatibilityRequirement"))}, }, Webhooks: []admissionregistrationv1.MutatingWebhook{ { diff --git a/pkg/controllers/crdcompatibility/objectvalidation/webhook.go b/pkg/controllers/crdcompatibility/objectvalidation/webhook.go index a433a677e..47f5fcc80 100644 --- a/pkg/controllers/crdcompatibility/objectvalidation/webhook.go +++ b/pkg/controllers/crdcompatibility/objectvalidation/webhook.go @@ -398,6 +398,7 @@ func ValidatingWebhookConfigurationFor(obj *apiextensionsv1alpha1.CompatibilityR Annotations: map[string]string{ "service.beta.openshift.io/inject-cabundle": "true", }, + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(obj, apiextensionsv1alpha1.GroupVersion.WithKind("CompatibilityRequirement"))}, }, Webhooks: []admissionregistrationv1.ValidatingWebhook{ { diff --git a/pkg/controllers/crdcompatibility/reconcile.go b/pkg/controllers/crdcompatibility/reconcile.go index 1b979a760..6670b6248 100644 --- a/pkg/controllers/crdcompatibility/reconcile.go +++ b/pkg/controllers/crdcompatibility/reconcile.go @@ -32,6 +32,7 @@ import ( "k8s.io/utils/clock" ctrl "sigs.k8s.io/controller-runtime" logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/yaml" apiextensionsv1alpha1 "github.com/openshift/api/apiextensions/v1alpha1" @@ -47,6 +48,10 @@ const ( terminalErrorReasonConfigurationError string = "ConfigurationError" ) +var ( + errWebhookConfigNotControlledByCompatibilityRequirement = reconcile.TerminalError(errors.New("webhook config is not controlled by CompatibilityRequirement")) //nolint:err113 +) + type reconcileState struct { *CompatibilityRequirementReconciler @@ -192,13 +197,22 @@ func (r *reconcileState) reconcileDelete(ctx context.Context, obj *apiextensions return ctrl.Result{}, nil } +//nolint:dupl // This and the ensureObjectPruningWebhook function look very similar, but are populating different objects. func (r *reconcileState) ensureObjectValidationWebhook(ctx context.Context, obj *apiextensionsv1alpha1.CompatibilityRequirement) error { - if isObjectValidationWebhookEnabled(obj) || r.compatibilityCRD == nil { - return nil + if !isObjectValidationWebhookEnabled(obj) || r.compatibilityCRD == nil { + // Ensure that the webhook is removed in case we previously created it. + return r.removeObjectValidationWebhook(ctx, obj) } webhookConfig := objectvalidation.ValidatingWebhookConfigurationFor(obj, r.compatibilityCRD) + existingWebhookConfig := &admissionregistrationv1.ValidatingWebhookConfiguration{} + if err := r.client.Get(ctx, types.NamespacedName{Name: webhookConfig.Name}, existingWebhookConfig); err != nil && !apierrors.IsNotFound(err) { + return fmt.Errorf("failed to get ValidatingWebhookConfiguration %s: %w", webhookConfig.Name, err) + } else if err == nil && !metav1.IsControlledBy(existingWebhookConfig, obj) { + return fmt.Errorf("%w: %s", errWebhookConfigNotControlledByCompatibilityRequirement, webhookConfig.Name) + } + if _, _, err := resourceapply.ApplyValidatingWebhookConfigurationImproved( ctx, r.kubeClient.AdmissionregistrationV1(), @@ -230,6 +244,11 @@ func (r *reconcileState) removeObjectValidationWebhook(ctx context.Context, obj return fmt.Errorf("failed to get ValidatingWebhookConfiguration %s: %w", webhookConfig.Name, err) } + // If we don't own the webhook config, we should not be deleting it. + if !metav1.IsControlledBy(webhookConfig, obj) { + return nil + } + if err := r.client.Delete(ctx, webhookConfig); err != nil { return fmt.Errorf("failed to delete ValidatingWebhookConfiguration %s: %w", webhookConfig.Name, err) } @@ -237,13 +256,23 @@ func (r *reconcileState) removeObjectValidationWebhook(ctx context.Context, obj return nil } +//nolint:dupl // This and the ensureObjectValidationWebhook function look very similar, but are populating different objects. func (r *reconcileState) ensureObjectPruningWebhook(ctx context.Context, obj *apiextensionsv1alpha1.CompatibilityRequirement) error { - if isObjectValidationWebhookEnabled(obj) || r.compatibilityCRD == nil { - return nil + if !isObjectValidationWebhookEnabled(obj) || r.compatibilityCRD == nil { + // Ensure that the webhook is removed in case we previously created it. + return r.removeObjectPruningWebhook(ctx, obj) } webhookConfig := objectpruning.MutatingWebhookConfigurationFor(obj, r.compatibilityCRD) + existingWebhookConfig := &admissionregistrationv1.MutatingWebhookConfiguration{} + if err := r.client.Get(ctx, types.NamespacedName{Name: webhookConfig.Name}, existingWebhookConfig); err != nil && !apierrors.IsNotFound(err) { + return fmt.Errorf("failed to get MutatingWebhookConfiguration %s: %w", webhookConfig.Name, err) + } else if err == nil && !metav1.IsControlledBy(existingWebhookConfig, obj) { + return fmt.Errorf("%w: %s", errWebhookConfigNotControlledByCompatibilityRequirement, webhookConfig.Name) + } + + // If we don't own the webhook config, we should not be overwriting it. if _, _, err := resourceapply.ApplyMutatingWebhookConfigurationImproved( ctx, r.kubeClient.AdmissionregistrationV1(), @@ -275,6 +304,11 @@ func (r *reconcileState) removeObjectPruningWebhook(ctx context.Context, obj *ap return fmt.Errorf("failed to get MutatingWebhookConfiguration %s: %w", webhookConfig.Name, err) } + // If we don't own the webhook config, we should not be deleting it. + if !metav1.IsControlledBy(webhookConfig, obj) { + return nil + } + if err := r.client.Delete(ctx, webhookConfig); err != nil { return fmt.Errorf("failed to delete MutatingWebhookConfiguration %s: %w", webhookConfig.Name, err) } @@ -284,7 +318,7 @@ func (r *reconcileState) removeObjectPruningWebhook(ctx context.Context, obj *ap func isObjectValidationWebhookEnabled(obj *apiextensionsv1alpha1.CompatibilityRequirement) bool { osv := obj.Spec.ObjectSchemaValidation - return osv.Action == "" && osv.MatchConditions == nil && labelSelectorIsEmpty(osv.NamespaceSelector) && labelSelectorIsEmpty(osv.ObjectSelector) + return osv.Action != "" || len(osv.MatchConditions) > 0 || !labelSelectorIsEmpty(osv.NamespaceSelector) || !labelSelectorIsEmpty(osv.ObjectSelector) } func labelSelectorIsEmpty(ls metav1.LabelSelector) bool { diff --git a/pkg/controllers/crdcompatibility/reconcile_test.go b/pkg/controllers/crdcompatibility/reconcile_test.go index 187f2a301..dd03c0b6d 100644 --- a/pkg/controllers/crdcompatibility/reconcile_test.go +++ b/pkg/controllers/crdcompatibility/reconcile_test.go @@ -18,6 +18,7 @@ package crdcompatibility import ( "context" + "fmt" "time" . "github.com/onsi/ginkgo/v2" @@ -347,6 +348,61 @@ var _ = Describe("CompatibilityRequirement", Ordered, ContinueOnFailure, func() // No need to delete as we are in an ordered context and the previous test removes the compatibility requirement. Eventually(kWithCtx(ctx).Get(mutatingWebhookConfig)).WithContext(ctx).Should(test.BeK8SNotFound()) }, defaultNodeTimeout) + + It("Should only have webhook configurations when the compatibility requirement sets object schema validation", func(ctx context.Context) { + Expect(requirement.Spec.ObjectSchemaValidation).ToNot(BeZero(), "Later test setup relies on this being non-zero") + + By("Creating a CompatibilityRequirement without object schema validation") + + requirementWithoutObjectSchemaValidation := test.GenerateTestCompatibilityRequirement(testCRDClean) + requirementWithoutObjectSchemaValidation.Spec.ObjectSchemaValidation = (apiextensionsv1alpha1.ObjectSchemaValidation{}) + + createTestObject(ctx, requirementWithoutObjectSchemaValidation, "CompatibilityRequirement") + + By(fmt.Sprintf("Compatibility requirement %s created", requirementWithoutObjectSchemaValidation.Name)) + // Check that the CompatibilityRequirement is admitted + Eventually(kWithCtx(ctx).Object(requirementWithoutObjectSchemaValidation)).WithContext(ctx).Should(HaveField("Status.Conditions", SatisfyAll( + test.HaveCondition("Progressing"). + WithStatus(metav1.ConditionFalse). + WithReason(apiextensionsv1alpha1.CompatibilityRequirementUpToDateReason), + test.HaveCondition("Admitted"). + WithStatus(metav1.ConditionTrue). + WithReason(apiextensionsv1alpha1.CompatibilityRequirementAdmittedReason), + ))) + + noObjectSchemaValidatingWebhook := &admissionregistrationv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: requirementWithoutObjectSchemaValidation.Name, + }, + } + noObjectSchemaMutatingWebhook := &admissionregistrationv1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: requirementWithoutObjectSchemaValidation.Name, + }, + } + + By("Checking that the webhook configurations are not present") + Expect(kWithCtx(ctx).Get(noObjectSchemaValidatingWebhook)()).To(MatchError(ContainSubstring("not found"))) + Expect(kWithCtx(ctx).Get(noObjectSchemaMutatingWebhook)()).To(MatchError(ContainSubstring("not found"))) + + By("Adding object schema validation back to the CompatibilityRequirement") + Eventually(kWithCtx(ctx).Update(requirementWithoutObjectSchemaValidation, func() { + requirementWithoutObjectSchemaValidation.Spec.ObjectSchemaValidation = requirement.Spec.ObjectSchemaValidation + })).WithContext(ctx).Should(Succeed()) + + By("Checking that the webhook configurations are now present") + Eventually(kWithCtx(ctx).Get(noObjectSchemaValidatingWebhook)).WithContext(ctx).Should(Succeed(), "The validating webhook configuration should be created") + Eventually(kWithCtx(ctx).Get(noObjectSchemaMutatingWebhook)).WithContext(ctx).Should(Succeed(), "The mutating webhook configuration should be created") + + By("Removing the object schema validation from the CompatibilityRequirement") + Eventually(kWithCtx(ctx).Update(requirementWithoutObjectSchemaValidation, func() { + requirementWithoutObjectSchemaValidation.Spec.ObjectSchemaValidation = (apiextensionsv1alpha1.ObjectSchemaValidation{}) + })).WithContext(ctx).Should(Succeed()) + + By("Checking that the webhook configurations are now removed") + Eventually(kWithCtx(ctx).Get(noObjectSchemaValidatingWebhook)).WithContext(ctx).Should(MatchError(ContainSubstring("not found"))) + Eventually(kWithCtx(ctx).Get(noObjectSchemaMutatingWebhook)).WithContext(ctx).Should(MatchError(ContainSubstring("not found"))) + }, defaultNodeTimeout) }) Context("When creating or modifying a CRD", func() { From 7f406a05aaf5342fb5c484cebeec14778d807a79 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Fri, 13 Mar 2026 15:45:41 +0000 Subject: [PATCH 5/7] Switch to pre-creating CRD in suite setup --- .../objectvalidation/handle_test.go | 340 ++++++++---------- .../objectvalidation/suite_test.go | 100 +++++- 2 files changed, 252 insertions(+), 188 deletions(-) diff --git a/pkg/controllers/crdcompatibility/objectvalidation/handle_test.go b/pkg/controllers/crdcompatibility/objectvalidation/handle_test.go index 7f05e167d..e966af824 100644 --- a/pkg/controllers/crdcompatibility/objectvalidation/handle_test.go +++ b/pkg/controllers/crdcompatibility/objectvalidation/handle_test.go @@ -65,11 +65,10 @@ func createWarningCompatibilityRequirement(crd *apiextensionsv1.CustomResourceDe var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOnFailure, func() { var ( - namespace string - baseCRD *apiextensionsv1.CustomResourceDefinition - compatibilityCRD *apiextensionsv1.CustomResourceDefinition + namespace = "default" compatibilityRequirement *apiextensionsv1alpha1.CompatibilityRequirement startWebhookServer func() + compatibilityCRD *apiextensionsv1.CustomResourceDefinition ) BeforeAll(func() { @@ -79,82 +78,18 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn }) BeforeEach(func(ctx context.Context) { - namespace = "default" - - // Create base CRD with test fields and subresources and install it - specReplicasPath := ".spec.replicas" - statusReplicasPath := ".status.readyReplicas" - labelSelectorPath := ".status.selector" - - // Define status properties using schema builder pattern - statusProperties := map[string]apiextensionsv1.JSONSchemaProps{ - "phase": *test.NewStringSchema(). - WithStringEnum("Ready", "Pending", "Failed"). - Build(), - "readyReplicas": *test.NewIntegerSchema(). - WithMinimum(0). - Build(), - "selector": *test.NewStringSchema(). - Build(), - "conditions": *test.NewArraySchema(). - WithObjectItems( - test.NewObjectSchema(). - WithRequiredStringProperty("type"). - WithRequiredStringProperty("status"), - ). - Build(), - } - - // Define spec properties using schema builder pattern - specProperties := map[string]apiextensionsv1.JSONSchemaProps{ - "replicas": *test.NewIntegerSchema(). - WithMinimum(0). - WithMaximum(100). - Build(), - "selector": *test.NewObjectSchema(). - WithObjectProperty("matchLabels", - test.NewObjectSchema(). - WithAdditionalPropertiesSchema(test.NewStringSchema()), - ). - Build(), - } - - compatibilityCRD = test.NewCRDSchemaBuilder(). - WithStringProperty("testField"). - WithRequiredStringProperty("requiredField"). - WithIntegerProperty("optionalNumber"). - WithStatusSubresource(statusProperties). - WithScaleSubresource(&specReplicasPath, &statusReplicasPath, &labelSelectorPath). - WithObjectProperty("spec", specProperties). - WithObjectProperty("status", statusProperties). - Build() - - // Deepcopy here as when we use the baseCRD for create/read it wipes the type meta. - // Set spec and status to empty schemas with preserve unknown fields so that the only validation applied is the compatibility requirement. - baseCRD = compatibilityCRD.DeepCopy() - baseCRD.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["spec"] = *test.NewObjectSchema().WithXPreserveUnknownFields(true).Build() - baseCRD.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["status"] = *test.NewObjectSchema().WithXPreserveUnknownFields(true).Build() - - // Install the CRD in the test environment - Expect(cl.Create(ctx, baseCRD.DeepCopy())).To(Succeed()) - - DeferCleanup(func(ctx context.Context) { - Expect(test.CleanupAndWait(ctx, cl, baseCRD)).To(Succeed()) - }) - - // Wait for CRD to be established - Eventually(kWithCtx(ctx).Object(baseCRD)).WithContext(ctx).Should(HaveField("Status.Conditions", test.HaveCondition("Established").WithStatus(apiextensionsv1.ConditionTrue))) - }, defaultNodeTimeout) + compatibilityCRD = suiteCompatibilityCRD() + }) Describe("Schema Matching Scenarios", func() { Context("when schemas match exactly", func() { BeforeEach(func(ctx context.Context) { // Create and store the compatibility requirement - compatibilityRequirement = test.GenerateTestCompatibilityRequirement(compatibilityCRD.DeepCopy()) + compatibilityRequirement = test.GenerateTestCompatibilityRequirement(compatibilityCRD) Expect(cl.Create(ctx, compatibilityRequirement)).To(Succeed()) // Create ValidatingWebhookConfiguration to enable end-to-end testing - webhookConfig := createValidatingWebhookConfig(compatibilityRequirement, baseCRD) + webhookConfig := createValidatingWebhookConfig(compatibilityRequirement, compatibilityCRD) Expect(cl.Create(ctx, webhookConfig)).To(Succeed()) DeferCleanup(func(ctx context.Context) { @@ -167,9 +102,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should allow valid objects through API server", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } validObj := test.NewTestObject(gvk). @@ -192,9 +127,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should reject invalid objects through API server", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } invalidObj := test.NewTestObject(gvk). @@ -243,9 +178,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should reject objects missing newly required fields through API server", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } objMissingField := test.NewTestObject(gvk). @@ -263,9 +198,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should allow objects with all required fields through API server", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } completeObj := test.NewTestObject(gvk). @@ -317,9 +252,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should allow objects matching tighter validation through API server", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } objWithExtraProperty := test.NewTestObject(gvk). @@ -338,9 +273,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should not allow objects without required fields through API server", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } objMissingFormerlyRequired := test.NewTestObject(gvk). @@ -363,9 +298,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn BeforeEach(func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } existingObj = test.NewTestObject(gvk). @@ -374,42 +309,23 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn WithField("testField", "initial-test"). Build() - Expect(cl.Create(ctx, existingObj)).To(Succeed()) + Expect(kWithCtx(ctx).ObjectList(&admissionv1.ValidatingWebhookConfigurationList{})()).Should(HaveField("Items", HaveLen(0))) + Expect(kWithCtx(ctx).ObjectList(&admissionv1.ValidatingAdmissionPolicyList{})()).Should(HaveField("Items", HaveLen(0))) + + By("Creating object", func() { + Expect(cl.Create(ctx, existingObj)).To(Succeed()) + }) - // Wait for object to be created before proceeding - Eventually(kWithCtx(ctx).Get(existingObj)).WithContext(ctx).Should(Succeed()) + By("Waiting for object to be created", func() { + // Wait for object to be created before proceeding + Eventually(kWithCtx(ctx).Get(existingObj)).WithContext(ctx).Should(Succeed()) + }) DeferCleanup(func(ctx context.Context) { Expect(test.CleanupAndWait(ctx, cl, existingObj)).To(Succeed()) }, defaultNodeTimeout) }) - Context("basic update validation", func() { - It("should allow valid updates through API server", func(ctx context.Context) { - // Update with valid changes - updatedObj := existingObj.DeepCopy() - updatedObj.Object["testField"] = "updated-test" - - Expect(cl.Update(ctx, updatedObj)).To(Succeed()) - - // Verify update was applied - Eventually(kWithCtx(ctx).Object(existingObj)).WithContext(ctx).Should( - HaveField("Object", HaveKeyWithValue("testField", "updated-test")), - ) - }, defaultNodeTimeout) - - It("should reject invalid updates through API server", func(ctx context.Context) { - // Update to remove required field - invalidUpdate := existingObj.DeepCopy() - delete(invalidUpdate.Object, "requiredField") // Remove required field - - err := cl.Update(ctx, invalidUpdate) - Expect(err).To(HaveOccurred()) - Expect(apierrors.IsInvalid(err)).To(BeTrue()) - Expect(err.Error()).To(ContainSubstring("required")) - }, defaultNodeTimeout) - }) - Context("tighter validation on updates (CompatibilityRequirement stricter than live CRD)", func() { var ( tighterCRD *apiextensionsv1.CustomResourceDefinition @@ -424,14 +340,20 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn WithRequiredField("optionalNumber"). Build() - tighterCompatibilityRequirement = test.GenerateTestCompatibilityRequirement(tighterCRD) - tighterCompatibilityRequirement.Name = fmt.Sprintf("tighter-%s", baseCRD.Name) - Expect(cl.Create(ctx, tighterCompatibilityRequirement)).To(Succeed()) + By("Creating tighter compatibility requirement", func() { + tighterCompatibilityRequirement = test.GenerateTestCompatibilityRequirement(tighterCRD) + tighterCompatibilityRequirement.Name = fmt.Sprintf("tighter-%s", compatibilityCRD.Name) + + Expect(cl.Create(ctx, tighterCompatibilityRequirement)).To(Succeed()) + }) // Create separate webhook config for tighter validation - tighterWebhookConfig = createValidatingWebhookConfig(tighterCompatibilityRequirement, baseCRD) - tighterWebhookConfig.Name = fmt.Sprintf("test-tighter-validation-%s", tighterCompatibilityRequirement.Name) - Expect(cl.Create(ctx, tighterWebhookConfig)).To(Succeed()) + By("Creating tighter webhook config", func() { + tighterWebhookConfig = createValidatingWebhookConfig(tighterCompatibilityRequirement, compatibilityCRD) + tighterWebhookConfig.Name = fmt.Sprintf("test-tighter-validation-%s", tighterCompatibilityRequirement.Name) + + Expect(cl.Create(ctx, tighterWebhookConfig)).To(Succeed()) + }) DeferCleanup(func(ctx context.Context) { Expect(test.CleanupAndWait(ctx, cl, tighterWebhookConfig, tighterCompatibilityRequirement)).To(Succeed()) @@ -469,9 +391,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn Describe("Delete Operations", func() { It("should allow deletion through API server", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } objToDelete := test.NewTestObject(gvk). @@ -499,21 +421,33 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn BeforeEach(func(ctx context.Context) { liveCRD := compatibilityCRD.DeepCopy() + + // Capture the existing scale subresource, remove it for the duration of this test, + // and then restore it to put the live CRD back to its original state. + var existingScaleSubresource *apiextensionsv1.CustomResourceSubresourceScale + Eventually(kWithCtx(ctx).Update(liveCRD, func() { + existingScaleSubresource = liveCRD.Spec.Versions[0].Subresources.Scale liveCRD.Spec.Versions[0].Subresources.Scale = nil })).WithContext(ctx).Should(Succeed()) + DeferCleanup(func(ctx context.Context) { + Eventually(kWithCtx(ctx).Update(liveCRD, func() { + liveCRD.Spec.Versions[0].Subresources.Scale = existingScaleSubresource + })).WithContext(ctx).Should(Succeed()) + }, defaultNodeTimeout) + statusCRD := compatibilityCRD.DeepCopy() // Disable the scale subresource for these test cases statusCRD.Spec.Versions[0].Subresources.Scale = nil // The baseCRD already has status subresource, so we can create a compatibility requirement directly statusCompatibilityRequirement = test.GenerateTestCompatibilityRequirement(statusCRD) - statusCompatibilityRequirement.Name = fmt.Sprintf("status-%s", baseCRD.Name) + statusCompatibilityRequirement.Name = fmt.Sprintf("status-%s", compatibilityCRD.Name) Expect(cl.Create(ctx, statusCompatibilityRequirement)).To(Succeed()) // Create ValidatingWebhookConfiguration for the compatibility requirement - statusWebhookConfig := createValidatingWebhookConfig(statusCompatibilityRequirement, baseCRD) + statusWebhookConfig := createValidatingWebhookConfig(statusCompatibilityRequirement, compatibilityCRD) statusWebhookConfig.Name = fmt.Sprintf("test-status-validation-%s", statusCompatibilityRequirement.Name) Expect(cl.Create(ctx, statusWebhookConfig)).To(Succeed()) @@ -524,9 +458,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should allow valid status updates when status validation is enabled", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } // First create the object without status @@ -568,9 +502,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should reject status updates with invalid enum values when status validation is enabled", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } // First create the object without status @@ -601,9 +535,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should reject status updates with invalid nested structure when status validation is enabled", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } // First create the object without status @@ -640,9 +574,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should reject status updates with negative readyReplicas when status validation is enabled", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } // First create the object without status @@ -682,24 +616,36 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn BeforeEach(func(ctx context.Context) { liveCRD := compatibilityCRD.DeepCopy() + + // Capture the existing scale subresource, remove it for the duration of this test, + // and then restore it to put the live CRD back to its original state. + var existingScaleSubresource *apiextensionsv1.CustomResourceSubresourceScale + Eventually(kWithCtx(ctx).Update(liveCRD, func() { + existingScaleSubresource = liveCRD.Spec.Versions[0].Subresources.Scale // Disable the scale subresource for these test cases // This means the scale validation is being implemented by the compatibility requirement, // rather than the live CRD. liveCRD.Spec.Versions[0].Subresources.Scale = nil })).WithContext(ctx).Should(Succeed()) + DeferCleanup(func(ctx context.Context) { + Eventually(kWithCtx(ctx).Update(liveCRD, func() { + liveCRD.Spec.Versions[0].Subresources.Scale = existingScaleSubresource + })).WithContext(ctx).Should(Succeed()) + }, defaultNodeTimeout) + scaleCRD := compatibilityCRD.DeepCopy() // Disable the status subresource for these test cases scaleCRD.Spec.Versions[0].Subresources.Status = nil // The baseCRD already has scale subresource, so we can create a compatibility requirement directly scaleCompatibilityRequirement = test.GenerateTestCompatibilityRequirement(scaleCRD) - scaleCompatibilityRequirement.Name = fmt.Sprintf("scale-%s", baseCRD.Name) + scaleCompatibilityRequirement.Name = fmt.Sprintf("scale-%s", compatibilityCRD.Name) Expect(cl.Create(ctx, scaleCompatibilityRequirement)).To(Succeed()) // Create ValidatingWebhookConfiguration for the compatibility requirement - scaleWebhookConfig := createValidatingWebhookConfig(scaleCompatibilityRequirement, baseCRD) + scaleWebhookConfig := createValidatingWebhookConfig(scaleCompatibilityRequirement, compatibilityCRD) scaleWebhookConfig.Name = fmt.Sprintf("test-scale-validation-%s", scaleCompatibilityRequirement.Name) Expect(cl.Create(ctx, scaleWebhookConfig)).To(Succeed()) @@ -710,9 +656,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should allow objects with valid scale-related fields when scale validation is enabled", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } validScaledObj := test.NewTestObject(gvk). @@ -753,9 +699,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should reject objects with replica count above maximum when scale validation is enabled", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } objWithTooManyReplicas := test.NewTestObject(gvk). @@ -778,9 +724,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should reject objects with negative replica count when scale validation is enabled", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } objWithNegativeReplicas := test.NewTestObject(gvk). @@ -803,9 +749,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should reject status updates with negative readyReplicas when scale validation is enabled", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } // First create the object with valid spec @@ -873,7 +819,7 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn Expect(warningClient.Create(ctx, warningCompatibilityRequirement)).To(Succeed()) // Create ValidatingWebhookConfiguration for the warning requirement - warningWebhookConfig := createValidatingWebhookConfig(warningCompatibilityRequirement, baseCRD) + warningWebhookConfig := createValidatingWebhookConfig(warningCompatibilityRequirement, compatibilityCRD) warningWebhookConfig.Name = fmt.Sprintf("test-warning-validation-%s", warningCompatibilityRequirement.Name) Expect(warningClient.Create(ctx, warningWebhookConfig)).To(Succeed()) @@ -884,9 +830,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should allow objects missing required fields but generate warnings", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } invalidObj := test.NewTestObject(gvk). @@ -911,9 +857,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should allow updates changing a field to be invalid but generate warnings", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } // First create a valid object @@ -954,10 +900,22 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn BeforeEach(func(ctx context.Context) { liveCRD := compatibilityCRD.DeepCopy() + + // Capture the existing scale subresource, remove it for the duration of this test, + // and then restore it to put the live CRD back to its original state. + var existingScaleSubresource *apiextensionsv1.CustomResourceSubresourceScale + Eventually(kWithCtx(ctx).Update(liveCRD, func() { + existingScaleSubresource = liveCRD.Spec.Versions[0].Subresources.Scale liveCRD.Spec.Versions[0].Subresources.Scale = nil })).Should(Succeed()) + DeferCleanup(func(ctx context.Context) { + Eventually(kWithCtx(ctx).Update(liveCRD, func() { + liveCRD.Spec.Versions[0].Subresources.Scale = existingScaleSubresource + })).WithContext(ctx).Should(Succeed()) + }, defaultNodeTimeout) + statusCRD := compatibilityCRD.DeepCopy() // Disable the scale subresource for these test cases statusCRD.Spec.Versions[0].Subresources.Scale = nil @@ -967,7 +925,7 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn Expect(warningClient.Create(ctx, warningStatusCompatibilityRequirement)).To(Succeed()) // Create ValidatingWebhookConfiguration for the warning requirement - warningStatusWebhookConfig := createValidatingWebhookConfig(warningStatusCompatibilityRequirement, baseCRD) + warningStatusWebhookConfig := createValidatingWebhookConfig(warningStatusCompatibilityRequirement, compatibilityCRD) warningStatusWebhookConfig.Name = fmt.Sprintf("test-warning-status-validation-%s", warningStatusCompatibilityRequirement.Name) Expect(warningClient.Create(ctx, warningStatusWebhookConfig)).To(Succeed()) @@ -978,9 +936,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should allow status updates with invalid enum values but generate warnings", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } // First create the object without status @@ -1017,9 +975,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should allow status updates with invalid nested structures but generate warnings", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } // First create the object without status @@ -1062,9 +1020,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should allow status updates with negative readyReplicas but generate warnings", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } // First create the object without status @@ -1108,12 +1066,24 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn BeforeEach(func(ctx context.Context) { liveCRD := compatibilityCRD.DeepCopy() + + // Capture the existing scale subresource, remove it for the duration of this test, + // and then restore it to put the live CRD back to its original state. + var existingScaleSubresource *apiextensionsv1.CustomResourceSubresourceScale + Eventually(kWithCtx(ctx).Update(liveCRD, func() { + existingScaleSubresource = liveCRD.Spec.Versions[0].Subresources.Scale // Disable the live CRD scale subresource else the objects will be rejected // and we won't be able to check the warnings. liveCRD.Spec.Versions[0].Subresources.Scale = nil })).WithContext(ctx).Should(Succeed()) + DeferCleanup(func(ctx context.Context) { + Eventually(kWithCtx(ctx).Update(liveCRD, func() { + liveCRD.Spec.Versions[0].Subresources.Scale = existingScaleSubresource + })).WithContext(ctx).Should(Succeed()) + }, defaultNodeTimeout) + scaleCRD := compatibilityCRD.DeepCopy() // Create a CompatibilityRequirement with Warn action using utility function @@ -1121,7 +1091,7 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn Expect(warningClient.Create(ctx, warningScaleCompatibilityRequirement)).To(Succeed()) // Create ValidatingWebhookConfiguration for the warning requirement - warningScaleWebhookConfig := createValidatingWebhookConfig(warningScaleCompatibilityRequirement, baseCRD) + warningScaleWebhookConfig := createValidatingWebhookConfig(warningScaleCompatibilityRequirement, compatibilityCRD) warningScaleWebhookConfig.Name = fmt.Sprintf("test-warning-scale-validation-%s", warningScaleCompatibilityRequirement.Name) Expect(warningClient.Create(ctx, warningScaleWebhookConfig)).To(Succeed()) @@ -1132,9 +1102,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should allow objects with replica count above maximum but generate warnings", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } objWithTooManyReplicas := test.NewTestObject(gvk). @@ -1165,9 +1135,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should allow objects with negative replica count but generate warnings", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } objWithNegativeReplicas := test.NewTestObject(gvk). @@ -1201,9 +1171,9 @@ var _ = Describe("End-to-End Admission Webhook Integration", Ordered, ContinueOn It("should allow status updates with negative readyReplicas but generate warnings", func(ctx context.Context) { gvk := schema.GroupVersionKind{ - Group: baseCRD.Spec.Group, - Version: baseCRD.Spec.Versions[0].Name, - Kind: baseCRD.Spec.Names.Kind, + Group: compatibilityCRD.Spec.Group, + Version: compatibilityCRD.Spec.Versions[0].Name, + Kind: compatibilityCRD.Spec.Names.Kind, } // First create the object with valid spec diff --git a/pkg/controllers/crdcompatibility/objectvalidation/suite_test.go b/pkg/controllers/crdcompatibility/objectvalidation/suite_test.go index 6fb8c27a6..e61092a70 100644 --- a/pkg/controllers/crdcompatibility/objectvalidation/suite_test.go +++ b/pkg/controllers/crdcompatibility/objectvalidation/suite_test.go @@ -24,6 +24,8 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/rest" ctrl "sigs.k8s.io/controller-runtime" @@ -38,9 +40,10 @@ import ( ) var ( - testEnv *envtest.Environment - cfg *rest.Config - cl client.Client + testEnv *envtest.Environment + cfg *rest.Config + cl client.Client + suiteCompatibilityCRD func() *apiextensionsv1.CustomResourceDefinition ) var defaultNodeTimeout = NodeTimeout(10 * time.Second) @@ -86,6 +89,8 @@ var _ = BeforeSuite(func(ctx context.Context) { InitValidator = func(ctx context.Context) (*validator, func()) { return initValidator(ctx, cfg, cl.Scheme(), testEnv) } + + suiteCompatibilityCRD = createSuiteCRDs(ctx) }, NodeTimeout(30*time.Second)) func initValidator(ctx context.Context, cfg *rest.Config, scheme *runtime.Scheme, testEnv *envtest.Environment) (*validator, func()) { @@ -163,3 +168,92 @@ func stopWebhookServer(ctx context.Context, mgrCancel context.CancelFunc, mgrDon func kWithCtx(ctx context.Context) komega.Komega { return komega.New(cl).WithContext(ctx) } + +func createSuiteCRDs(ctx context.Context) func() *apiextensionsv1.CustomResourceDefinition { + // Create base CRD with test fields and subresources and install it + specReplicasPath := ".spec.replicas" + statusReplicasPath := ".status.readyReplicas" + labelSelectorPath := ".status.selector" + + // Define status properties using schema builder pattern + statusProperties := map[string]apiextensionsv1.JSONSchemaProps{ + "phase": *test.NewStringSchema(). + WithStringEnum("Ready", "Pending", "Failed"). + Build(), + "readyReplicas": *test.NewIntegerSchema(). + WithMinimum(0). + Build(), + "selector": *test.NewStringSchema(). + Build(), + "conditions": *test.NewArraySchema(). + WithObjectItems( + test.NewObjectSchema(). + WithRequiredStringProperty("type"). + WithRequiredStringProperty("status"), + ). + Build(), + } + + // Define spec properties using schema builder pattern + specProperties := map[string]apiextensionsv1.JSONSchemaProps{ + "replicas": *test.NewIntegerSchema(). + WithMinimum(0). + WithMaximum(100). + Build(), + "selector": *test.NewObjectSchema(). + WithObjectProperty("matchLabels", + test.NewObjectSchema(). + WithAdditionalPropertiesSchema(test.NewStringSchema()), + ). + Build(), + } + + compatibilityCRD := test.NewCRDSchemaBuilder(). + WithStringProperty("testField"). + WithRequiredStringProperty("requiredField"). + WithIntegerProperty("optionalNumber"). + WithStatusSubresource(statusProperties). + WithScaleSubresource(&specReplicasPath, &statusReplicasPath, &labelSelectorPath). + WithObjectProperty("spec", specProperties). + WithObjectProperty("status", statusProperties). + Build() + + // Deepcopy here as when we use the baseCRD for create/read it wipes the type meta. + // Set spec and status to empty schemas with preserve unknown fields so that the only validation applied is the compatibility requirement. + baseCRD := compatibilityCRD.DeepCopy() + baseCRD.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["spec"] = *test.NewObjectSchema().WithXPreserveUnknownFields(true).Build() + baseCRD.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["status"] = *test.NewObjectSchema().WithXPreserveUnknownFields(true).Build() + + createCRD(ctx, baseCRD) + + return func() *apiextensionsv1.CustomResourceDefinition { + return compatibilityCRD.DeepCopy() + } +} + +func createCRD(ctx context.Context, crd *apiextensionsv1.CustomResourceDefinition) { + GinkgoHelper() + + By("Creating CRD "+crd.GetName(), func() { + // Install the CRD in the test environment + Expect(cl.Create(ctx, crd)).To(Succeed()) + }) + + DeferCleanup(func(ctx context.Context) { + Expect(test.CleanupAndWait(ctx, cl, crd)).To(Succeed()) + }) + + By("Waiting for CRD to have been established for at least 2 seconds", func() { + // Because the API server is programmed not to accept a response before then. + // See: https://github.com/kubernetes/kubernetes/blob/18dd17f7ce05bd79e21245278a4e88f901d2ebd6/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go#L381-L394 + Eventually(kWithCtx(ctx).Object(crd)).WithContext(ctx).Should(HaveField("Status.Conditions", + test.HaveCondition("Established"). + WithStatus(apiextensionsv1.ConditionTrue). + WithLastTransitionTime(WithTransform(timeSince, BeNumerically(">", 2*time.Second))), + )) + }) +} + +func timeSince(t metav1.Time) time.Duration { + return time.Since(t.Time) +} From 5b65be735d6c247fd83e8a89282eccccef7a4a12 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Fri, 13 Mar 2026 17:20:31 +0000 Subject: [PATCH 6/7] Ensure UID is set on MWC create --- .../crdcompatibility/objectpruning/handle_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/controllers/crdcompatibility/objectpruning/handle_test.go b/pkg/controllers/crdcompatibility/objectpruning/handle_test.go index e79332da4..144f741e4 100644 --- a/pkg/controllers/crdcompatibility/objectpruning/handle_test.go +++ b/pkg/controllers/crdcompatibility/objectpruning/handle_test.go @@ -347,7 +347,10 @@ var _ = Describe("Object Pruning Integration", func() { By("Creating MutatingWebhookConfiguration with non-existent CompatibilityRequirement") webhookConfig := createMutatingWebhookConfig(&apiextensionsv1alpha1.CompatibilityRequirement{ - ObjectMeta: metav1.ObjectMeta{Name: "non-existent-compat-req"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "non-existent-compat-req", + UID: "non-existent-compat-req-uid", + }, }, liveCRD) Expect(cl.Create(ctx, webhookConfig)).To(Succeed()) From 0a75757181e6dafa57a28d8d4f4dc3db19988e19 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Fri, 13 Mar 2026 17:21:16 +0000 Subject: [PATCH 7/7] Pre-create CRDs in object pruning suite --- .../objectpruning/handle_test.go | 32 ++--------- .../objectpruning/suite_test.go | 53 +++++++++++++++++-- 2 files changed, 53 insertions(+), 32 deletions(-) diff --git a/pkg/controllers/crdcompatibility/objectpruning/handle_test.go b/pkg/controllers/crdcompatibility/objectpruning/handle_test.go index 144f741e4..b38ec8378 100644 --- a/pkg/controllers/crdcompatibility/objectpruning/handle_test.go +++ b/pkg/controllers/crdcompatibility/objectpruning/handle_test.go @@ -65,19 +65,7 @@ var _ = Describe("Object Pruning Integration", func() { Context("admission pruning scenarios", func() { BeforeEach(func(ctx context.Context) { - By("Creating the live CRD with permissive schema") - - liveCRD = createPermissivePropertiesCRDSchema() - Expect(cl.Create(ctx, liveCRD)).To(Succeed()) - - DeferCleanup(func(ctx context.Context) { - Expect(test.CleanupAndWait(ctx, cl, liveCRD)).To(Succeed()) - }, defaultNodeTimeout) - - By("Waiting for CRD to be established") - Eventually(kWithCtx(ctx).Object(liveCRD)).WithContext(ctx).Should( - HaveField("Status.Conditions", test.HaveCondition("Established").WithStatus(apiextensionsv1.ConditionTrue)), - ) + liveCRD = permissiveSuiteCRD() }, defaultNodeTimeout) DescribeTable("object pruning scenarios through API server", @@ -326,21 +314,7 @@ var _ = Describe("Object Pruning Integration", func() { Context("error scenarios", func() { BeforeEach(func(ctx context.Context) { - By("Creating a live CRD with permissive schema") - - liveCRD = createEmptyPropertiesCRDSchema() - Expect(cl.Create(ctx, liveCRD)).To(Succeed()) - - By("Waiting for CRD to be established") - Eventually(kWithCtx(ctx).Object(liveCRD)).WithContext(ctx).Should( - HaveField("Status.Conditions", ContainElement(And( - HaveField("Type", BeEquivalentTo("Established")), - HaveField("Status", BeEquivalentTo(metav1.ConditionTrue)), - )))) - - DeferCleanup(func(ctx context.Context) { - Expect(test.CleanupAndWait(ctx, cl, liveCRD)).To(Succeed()) - }) + liveCRD = emptySuiteCRD() }, defaultNodeTimeout) It("should handle webhook when CompatibilityRequirement does not exist", func(ctx context.Context) { @@ -534,7 +508,7 @@ func createEmptyPropertiesCRDSchema() *apiextensionsv1.CustomResourceDefinition gvk := schema.GroupVersionKind{ Group: "test.example.com", Version: "v1", - Kind: "TestResource", + Kind: "TestEmptyResource", } crd := test.GenerateCRD(gvk) diff --git a/pkg/controllers/crdcompatibility/objectpruning/suite_test.go b/pkg/controllers/crdcompatibility/objectpruning/suite_test.go index a7b5f8516..4ad8e6644 100644 --- a/pkg/controllers/crdcompatibility/objectpruning/suite_test.go +++ b/pkg/controllers/crdcompatibility/objectpruning/suite_test.go @@ -24,6 +24,8 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/rest" ctrl "sigs.k8s.io/controller-runtime" @@ -38,9 +40,11 @@ import ( ) var ( - testEnv *envtest.Environment - cfg *rest.Config - cl client.Client + testEnv *envtest.Environment + cfg *rest.Config + cl client.Client + permissiveSuiteCRD func() *apiextensionsv1.CustomResourceDefinition + emptySuiteCRD func() *apiextensionsv1.CustomResourceDefinition ) var defaultNodeTimeout = NodeTimeout(10 * time.Second) @@ -77,6 +81,8 @@ var _ = BeforeSuite(func(ctx context.Context) { // Initialize validator and webhook server _, startWebhookServer := initValidator(ctx, cfg, cl.Scheme(), testEnv) startWebhookServer() + + permissiveSuiteCRD, emptySuiteCRD = createSuiteCRDs(ctx) }, NodeTimeout(30*time.Second)) func initValidator(ctx context.Context, cfg *rest.Config, scheme *runtime.Scheme, testEnv *envtest.Environment) (*validator, func()) { @@ -154,3 +160,44 @@ func stopWebhookServer(ctx context.Context, mgrCancel context.CancelFunc, mgrDon func kWithCtx(ctx context.Context) komega.Komega { return komega.New(cl).WithContext(ctx) } + +func createSuiteCRDs(ctx context.Context) (func() *apiextensionsv1.CustomResourceDefinition, func() *apiextensionsv1.CustomResourceDefinition) { + permissiveCRD := createPermissivePropertiesCRDSchema() + createCRD(ctx, permissiveCRD.DeepCopy()) + + emptyCRD := createEmptyPropertiesCRDSchema() + createCRD(ctx, emptyCRD.DeepCopy()) + + return func() *apiextensionsv1.CustomResourceDefinition { + return permissiveCRD.DeepCopy() + }, func() *apiextensionsv1.CustomResourceDefinition { + return emptyCRD.DeepCopy() + } +} + +func createCRD(ctx context.Context, crd *apiextensionsv1.CustomResourceDefinition) { + GinkgoHelper() + + By("Creating CRD "+crd.GetName(), func() { + // Install the CRD in the test environment + Expect(cl.Create(ctx, crd)).To(Succeed()) + }) + + DeferCleanup(func(ctx context.Context) { + Expect(test.CleanupAndWait(ctx, cl, crd)).To(Succeed()) + }) + + By("Waiting for CRD to have been established for at least 2 seconds", func() { + // Because the API server is programmed not to accept a response before then. + // See: https://github.com/kubernetes/kubernetes/blob/18dd17f7ce05bd79e21245278a4e88f901d2ebd6/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go#L381-L394 + Eventually(kWithCtx(ctx).Object(crd)).WithContext(ctx).Should(HaveField("Status.Conditions", + test.HaveCondition("Established"). + WithStatus(apiextensionsv1.ConditionTrue). + WithLastTransitionTime(WithTransform(timeSince, BeNumerically(">", 2*time.Second))), + )) + }) +} + +func timeSince(t metav1.Time) time.Duration { + return time.Since(t.Time) +}