🐛 Fix CRD validation to only validate CRs against storage version schema

pkg/controller/operators/catalog/operator.go

@@ -2177,80 +2177,119 @@ func transitionInstallPlanState(log logrus.FieldLogger, transitioner installPlan
 	}
 }
 
-// Validate all existing served versions against new CRD's validation (if changed)
+// Validate existing CRs against the new CRD's validation schema for the storage version.
+// When a CRD has multiple versions with conversion.strategy=None, CRs are stored in the
+// storage version format. We should only validate CRs against the storage version's schema,
+// not against all served versions, because without a conversion webhook the CR data won't
+// be converted to match other versions' schemas.
 func validateV1CRDCompatibility(dynamicClient dynamic.Interface, oldCRD *apiextensionsv1.CustomResourceDefinition, newCRD *apiextensionsv1.CustomResourceDefinition) error {
 	logrus.Debugf("Comparing %#v to %#v", oldCRD.Spec.Versions, newCRD.Spec.Versions)
 
-	oldVersionSet := sets.New[string]()
-	for _, oldVersion := range oldCRD.Spec.Versions {
-		if !oldVersionSet.Has(oldVersion.Name) && oldVersion.Served {
-			oldVersionSet.Insert(oldVersion.Name)
+	// Find the storage version from the old CRD - this is the version CRs are actually stored in
+	var oldStorageVersion string
+	for _, v := range oldCRD.Spec.Versions {
+		if v.Storage {
+			oldStorageVersion = v.Name
+			break
 		}
 	}
 
+	if oldStorageVersion == "" {
+		return fmt.Errorf("no storage version found in old CRD %s", oldCRD.Name)
+	}
+
+	// Only validate existing CRs against the new CRD's schema for the old storage version.
+	// This is the correct behavior because:
+	// 1. All existing CRs are stored in the old storage version format
+	// 2. When conversion.strategy is "None", Kubernetes doesn't convert CR data between versions
+	// 3. Validating CRs against other versions' schemas would incorrectly fail when those
+	//    versions have different required fields
 	validationsMap := make(map[string]*apiextensions.CustomResourceValidation, 0)
 	for _, newVersion := range newCRD.Spec.Versions {
-		if oldVersionSet.Has(newVersion.Name) && newVersion.Served {
-			// If the new CRD's version is present in the cluster and still
-			// served then fill the map entry with the new validation
+		if newVersion.Name == oldStorageVersion && newVersion.Served {
 			convertedValidation := &apiextensions.CustomResourceValidation{}
 			if err := apiextensionsv1.Convert_v1_CustomResourceValidation_To_apiextensions_CustomResourceValidation(newVersion.Schema, convertedValidation, nil); err != nil {
 				return err
 			}
 			validationsMap[newVersion.Name] = convertedValidation
+			break
 		}
 	}
+
+	// If the old storage version is no longer served in the new CRD, skip validation.
+	// This scenario should be caught by SafeStorageVersionUpgrade check.
+	if len(validationsMap) == 0 {
+		logrus.Debugf("Old storage version %s not found or not served in new CRD %s, skipping CR validation", oldStorageVersion, newCRD.Name)
+		return nil
+	}
+
 	return validateExistingCRs(dynamicClient, schema.GroupResource{Group: newCRD.Spec.Group, Resource: newCRD.Spec.Names.Plural}, validationsMap)
 }
 
-// Validate all existing served versions against new CRD's validation (if changed)
+// Validate existing CRs against the new CRD's validation schema for the storage version.
+// See validateV1CRDCompatibility for detailed explanation.
 func validateV1Beta1CRDCompatibility(dynamicClient dynamic.Interface, oldCRD *apiextensionsv1beta1.CustomResourceDefinition, newCRD *apiextensionsv1beta1.CustomResourceDefinition) error {
 	logrus.Debugf("Comparing %#v to %#v", oldCRD.Spec.Validation, newCRD.Spec.Validation)
-	oldVersionSet := sets.New[string]()
+
+	// Find the storage version from the old CRD
+	var oldStorageVersion string
 	if len(oldCRD.Spec.Versions) == 0 {
-		// apiextensionsv1beta1 special case: if spec.Versions is empty, use the global version and validation
-		oldVersionSet.Insert(oldCRD.Spec.Version)
-	}
-	for _, oldVersion := range oldCRD.Spec.Versions {
-		// collect served versions from spec.Versions if the list is present
-		if !oldVersionSet.Has(oldVersion.Name) && oldVersion.Served {
-			oldVersionSet.Insert(oldVersion.Name)
+		// apiextensionsv1beta1 special case: if spec.Versions is empty, use the global version
+		oldStorageVersion = oldCRD.Spec.Version
+	} else {
+		for _, v := range oldCRD.Spec.Versions {
+			if v.Storage {
+				oldStorageVersion = v.Name
+				break
+			}
 		}
 	}
 
+	if oldStorageVersion == "" {
+		return fmt.Errorf("no storage version found in old CRD %s", oldCRD.Name)
+	}
+
 	validationsMap := make(map[string]*apiextensions.CustomResourceValidation, 0)
 	gr := schema.GroupResource{Group: newCRD.Spec.Group, Resource: newCRD.Spec.Names.Plural}
+
 	if len(newCRD.Spec.Versions) == 0 {
 		// apiextensionsv1beta1 special case: if spec.Versions of newCRD is empty, use the global version and validation
-		if oldVersionSet.Has(newCRD.Spec.Version) {
+		if newCRD.Spec.Version == oldStorageVersion {
 			convertedValidation := &apiextensions.CustomResourceValidation{}
 			if err := apiextensionsv1beta1.Convert_v1beta1_CustomResourceValidation_To_apiextensions_CustomResourceValidation(newCRD.Spec.Validation, convertedValidation, nil); err != nil {
 				return err
 			}
 			validationsMap[newCRD.Spec.Version] = convertedValidation
 		}
-	}
-	for _, newVersion := range newCRD.Spec.Versions {
-		if oldVersionSet.Has(newVersion.Name) && newVersion.Served {
-			// If the new CRD's version is present in the cluster and still
-			// served then fill the map entry with the new validation
-			if newCRD.Spec.Validation != nil {
-				// apiextensionsv1beta1 special case: spec.Validation and spec.Versions[].Schema are mutually exclusive;
-				// if spec.Versions is non-empty and spec.Validation is set then we can validate once against any
-				// single existing version.
-				convertedValidation := &apiextensions.CustomResourceValidation{}
-				if err := apiextensionsv1beta1.Convert_v1beta1_CustomResourceValidation_To_apiextensions_CustomResourceValidation(newCRD.Spec.Validation, convertedValidation, nil); err != nil {
-					return err
+	} else {
+		// Only validate against the old storage version's schema in the new CRD
+		for _, newVersion := range newCRD.Spec.Versions {
+			if newVersion.Name == oldStorageVersion && newVersion.Served {
+				if newCRD.Spec.Validation != nil {
+					// apiextensionsv1beta1 special case: spec.Validation and spec.Versions[].Schema are mutually exclusive
+					convertedValidation := &apiextensions.CustomResourceValidation{}
+					if err := apiextensionsv1beta1.Convert_v1beta1_CustomResourceValidation_To_apiextensions_CustomResourceValidation(newCRD.Spec.Validation, convertedValidation, nil); err != nil {
+						return err
+					}
+					validationsMap[newVersion.Name] = convertedValidation
+				} else {
+					convertedValidation := &apiextensions.CustomResourceValidation{}
+					if err := apiextensionsv1beta1.Convert_v1beta1_CustomResourceValidation_To_apiextensions_CustomResourceValidation(newVersion.Schema, convertedValidation, nil); err != nil {
+						return err
+					}
+					validationsMap[newVersion.Name] = convertedValidation
 				}
-				return validateExistingCRs(dynamicClient, gr, map[string]*apiextensions.CustomResourceValidation{newVersion.Name: convertedValidation})
-			}
-			convertedValidation := &apiextensions.CustomResourceValidation{}
-			if err := apiextensionsv1beta1.Convert_v1beta1_CustomResourceValidation_To_apiextensions_CustomResourceValidation(newVersion.Schema, convertedValidation, nil); err != nil {
-				return err
+				break
 			}
-			validationsMap[newVersion.Name] = convertedValidation
 		}
 	}
+
+	// If the old storage version is no longer served in the new CRD, skip validation
+	if len(validationsMap) == 0 {
+		logrus.Debugf("Old storage version %s not found or not served in new CRD %s, skipping CR validation", oldStorageVersion, newCRD.Name)
+		return nil
+	}
+
 	return validateExistingCRs(dynamicClient, gr, validationsMap)
 }
 

pkg/controller/operators/catalog/operator_test.go

@@ -1869,37 +1869,43 @@ func TestValidateV1Beta1CRDCompatibility(t *testing.T) {
 			want:   validationError{fmt.Errorf("error validating hive.openshift.io/v1, Kind=MachinePool \"test\": updated validation is too restrictive: [[].spec.clusterDeploymentRef: Invalid value: \"null\": spec.clusterDeploymentRef in body must be of type object: \"null\", [].spec.name: Required value, [].spec.platform: Required value]")},
 		},
 		{
+			// Use storage version (v2) for the CR since that's how CRs are actually stored in Kubernetes.
+			// When a CRD has storage version v2, all CRs are stored in v2 format regardless of which
+			// apiVersion they were created with.
 			name: "backwards incompatible change",
 			existingObjects: []runtime.Object{
-				unstructuredForFile("testdata/apiextensionsv1beta1/cr.yaml"),
+				unstructuredForFile("testdata/apiextensionsv1beta1/cr.v2.invalid.yaml"),
 			},
 			gvr: schema.GroupVersionResource{
 				Group:    "cluster.com",
-				Version:  "v1alpha1",
-				Resource: "testcrd",
+				Version:  "v2",
+				Resource: "testcrds",
 			},
 			oldCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.old.yaml"),
 			newCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.yaml"),
-			want:   validationError{fmt.Errorf("error validating cluster.com/v1alpha1, Kind=testcrd \"my-cr-1\": updated validation is too restrictive: [].spec.scalar: Invalid value: 2: spec.scalar in body should be greater than or equal to 3")},
+			want:   validationError{fmt.Errorf("error validating cluster.com/v2, Kind=testcrd \"my-cr-1\": updated validation is too restrictive: [].spec.scalar: Invalid value: 2: spec.scalar in body should be greater than or equal to 3")},
 		},
 		{
+			// Test that CRs are validated against the storage version schema even when
+			// other versions become unserved. Since storage version is v2, CRs are
+			// stored in v2 format and validated against v2 schema.
 			name: "unserved version",
 			existingObjects: []runtime.Object{
-				unstructuredForFile("testdata/apiextensionsv1beta1/cr.yaml"),
 				unstructuredForFile("testdata/apiextensionsv1beta1/cr.v2.yaml"),
 			},
 			gvr: schema.GroupVersionResource{
 				Group:    "cluster.com",
-				Version:  "v1alpha1",
-				Resource: "testcrd",
+				Version:  "v2",
+				Resource: "testcrds",
 			},
 			oldCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.old.yaml"),
 			newCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.unserved.yaml"),
 		},
 		{
+			// Test that CRs stored in storage version (v2) are validated correctly
+			// when the old CRD had v1alpha1 unserved.
 			name: "cr not validated against currently unserved version",
 			existingObjects: []runtime.Object{
-				unstructuredForFile("testdata/apiextensionsv1beta1/cr.yaml"),
 				unstructuredForFile("testdata/apiextensionsv1beta1/cr.v2.yaml"),
 			},
 			oldCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.unserved.yaml"),

pkg/controller/operators/catalog/testdata/apiextensionsv1beta1/cr.v2.invalid.yaml

@@ -0,0 +1,6 @@
+apiVersion: cluster.com/v2
+kind: testcrd
+metadata:
+  name: my-cr-1
+spec:
+  scalar: 2