OCPCLOUD-3384, OCPCLOUD-3365: Implement admission action Warn and ensure validations not active when not configured

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",
@@ -95,9 +83,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 +114,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())
+
+					Eventually(kWithCtx(ctx).Get(retrievedObj)).WithContext(ctx).Should(Succeed())
 
-				retrievedObj := &unstructured.Unstructured{}
-				retrievedObj.SetGroupVersionKind(inputObject.GroupVersionKind())
-				retrievedObj.SetName(inputObject.GetName())
-				retrievedObj.SetNamespace(inputObject.GetNamespace())
+					Expect(retrievedObj.Object).To(test.IgnoreFields([]string{"apiVersion", "kind", "metadata"}, Equal(scenario.ExpectedObject)), "Expected object to be pruned correctly")
+				})
+
+				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())
+				}
 
-				Eventually(kWithCtx(ctx).Get(retrievedObj)).WithContext(ctx).Should(Succeed())
+				By("Verifying the object was not pruned", func() {
+					retrievedObj := &unstructured.Unstructured{}
+					retrievedObj.SetGroupVersionKind(inputObject.GroupVersionKind())
+					retrievedObj.SetName(inputObject.GetName())
+					retrievedObj.SetNamespace(inputObject.GetNamespace())
 
-				Expect(retrievedObj.Object).To(test.IgnoreFields([]string{"apiVersion", "kind", "metadata"}, Equal(scenario.ExpectedObject)))
+					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 +298,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",
@@ -277,28 +314,17 @@ 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) {
 			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())
 
@@ -482,7 +508,7 @@ func createEmptyPropertiesCRDSchema() *apiextensionsv1.CustomResourceDefinition
 	gvk := schema.GroupVersionKind{
 		Group:   "test.example.com",
 		Version: "v1",
-		Kind:    "TestResource",
+		Kind:    "TestEmptyResource",
 	}
 
 	crd := test.GenerateCRD(gvk)
@@ -526,6 +552,9 @@ func createCompatibilityRequirement(crd *apiextensionsv1.CustomResourceDefinitio
 					DefaultSelection: apiextensionsv1alpha1.APIVersionSetTypeAllServed,
 				},
 			},
+			ObjectSchemaValidation: apiextensionsv1alpha1.ObjectSchemaValidation{
+				Action: apiextensionsv1alpha1.CRDAdmitActionDeny,
+			},
 		},
 	}
 }

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)
+}

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,15 +52,15 @@ 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())
 			})
 
 			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,24 +86,14 @@ 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)
 				brokenCompatibilityRequirement.Spec.CompatibilitySchema.CustomResourceDefinition.Data = "invalid: yaml: content: ["
 
 				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")))
 			})

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)
@@ -235,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{
 			{
@@ -288,3 +299,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
+}