Correct CRD validation to validate CRs against each version of the updated CRD

Signed-off-by: Daniel Franz <[email protected]>

Author: dtfranz

pkg/controller/operators/catalog/operator.go

@@ -32,6 +32,7 @@ import (
 	"k8s.io/apimachinery/pkg/selection"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/apimachinery/pkg/util/yaml"
 	batchv1applyconfigurations "k8s.io/client-go/applyconfigurations/batch/v1"
@@ -2077,81 +2078,106 @@ func transitionInstallPlanState(log logrus.FieldLogger, transitioner installPlan
 func validateV1CRDCompatibility(dynamicClient dynamic.Interface, oldCRD *apiextensionsv1.CustomResourceDefinition, newCRD *apiextensionsv1.CustomResourceDefinition) error {
 	logrus.Debugf("Comparing %#v to %#v", oldCRD.Spec.Versions, newCRD.Spec.Versions)
 
-	// If validation schema is unchanged, return right away
-	newestSchema := newCRD.Spec.Versions[len(newCRD.Spec.Versions)-1].Schema
-	for i, oldVersion := range oldCRD.Spec.Versions {
-		if !reflect.DeepEqual(oldVersion.Schema, newestSchema) {
-			break
-		}
-		if i == len(oldCRD.Spec.Versions)-1 {
-			// we are on the last iteration
-			// schema has not changed between versions at this point.
-			return nil
+	oldVersionSet := sets.New[string]()
+	for _, oldVersion := range oldCRD.Spec.Versions {
+		if !oldVersionSet.Has(oldVersion.Name) && oldVersion.Served {
+			oldVersionSet.Insert(oldVersion.Name)
 		}
 	}
 
-	convertedCRD := &apiextensions.CustomResourceDefinition{}
-	if err := apiextensionsv1.Convert_v1_CustomResourceDefinition_To_apiextensions_CustomResourceDefinition(newCRD, convertedCRD, nil); err != nil {
-		return err
-	}
-	for _, version := range oldCRD.Spec.Versions {
-		if version.Served {
-			gvr := schema.GroupVersionResource{Group: oldCRD.Spec.Group, Version: version.Name, Resource: oldCRD.Spec.Names.Plural}
-			err := validateExistingCRs(dynamicClient, gvr, convertedCRD)
-			if err != nil {
+	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
+			convertedValidation := &apiextensions.CustomResourceValidation{}
+			if err := apiextensionsv1.Convert_v1_CustomResourceValidation_To_apiextensions_CustomResourceValidation(newVersion.Schema, convertedValidation, nil); err != nil {
 				return err
 			}
+			validationsMap[newVersion.Name] = convertedValidation
 		}
 	}
-
-	logrus.Debugf("Successfully validated CRD %s\n", 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)
 func validateV1Beta1CRDCompatibility(dynamicClient dynamic.Interface, oldCRD *apiextensionsv1beta1.CustomResourceDefinition, newCRD *apiextensionsv1beta1.CustomResourceDefinition) error {
 	logrus.Debugf("Comparing %#v to %#v", oldCRD.Spec.Validation, newCRD.Spec.Validation)
-
-	// TODO return early of all versions are equal
-	convertedCRD := &apiextensions.CustomResourceDefinition{}
-	if err := apiextensionsv1beta1.Convert_v1beta1_CustomResourceDefinition_To_apiextensions_CustomResourceDefinition(newCRD, convertedCRD, nil); err != nil {
-		return err
+	oldVersionSet := sets.New[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 _, version := range oldCRD.Spec.Versions {
-		if version.Served {
-			gvr := schema.GroupVersionResource{Group: oldCRD.Spec.Group, Version: version.Name, Resource: oldCRD.Spec.Names.Plural}
-			err := validateExistingCRs(dynamicClient, gvr, convertedCRD)
-			if err != nil {
-				return err
-			}
+	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)
 		}
 	}
 
-	if oldCRD.Spec.Version != "" {
-		gvr := schema.GroupVersionResource{Group: oldCRD.Spec.Group, Version: oldCRD.Spec.Version, Resource: oldCRD.Spec.Names.Plural}
-		err := validateExistingCRs(dynamicClient, gvr, convertedCRD)
-		if err != nil {
-			return err
+	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) {
+			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
+				}
+				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
+			}
+			validationsMap[newVersion.Name] = convertedValidation
 		}
 	}
-	logrus.Debugf("Successfully validated CRD %s\n", newCRD.Name)
-	return nil
+	return validateExistingCRs(dynamicClient, gr, validationsMap)
 }
 
-func validateExistingCRs(dynamicClient dynamic.Interface, gvr schema.GroupVersionResource, newCRD *apiextensions.CustomResourceDefinition) error {
-	// make dynamic client
-	crList, err := dynamicClient.Resource(gvr).List(context.TODO(), metav1.ListOptions{})
-	if err != nil {
-		return fmt.Errorf("error listing resources in GroupVersionResource %#v: %s", gvr, err)
-	}
-	for _, cr := range crList.Items {
-		validator, _, err := validation.NewSchemaValidator(newCRD.Spec.Validation)
+// validateExistingCRs lists all CRs for each version entry in validationsMap, then validates each using the paired validation.
+func validateExistingCRs(dynamicClient dynamic.Interface, gr schema.GroupResource, validationsMap map[string]*apiextensions.CustomResourceValidation) error {
+	for version, schemaValidation := range validationsMap {
+		// create validator from given crdValidation
+		validator, _, err := validation.NewSchemaValidator(schemaValidation)
 		if err != nil {
-			return fmt.Errorf("error creating validator for schema %#v: %s", newCRD.Spec.Validation, err)
+			return fmt.Errorf("error creating validator for schema version %s: %s", version, err)
 		}
-		err = validation.ValidateCustomResource(field.NewPath(""), cr.UnstructuredContent(), validator).ToAggregate()
+
+		gvr := schema.GroupVersionResource{Group: gr.Group, Version: version, Resource: gr.Resource}
+		crList, err := dynamicClient.Resource(gvr).List(context.TODO(), metav1.ListOptions{})
 		if err != nil {
-			return fmt.Errorf("error validating custom resource against new schema for %s %s/%s: %v", newCRD.Spec.Names.Kind, cr.GetNamespace(), cr.GetName(), err)
+			return fmt.Errorf("error listing resources in GroupVersionResource %#v: %s", gvr, err)
+		}
+
+		// validate each CR against this version schema
+		for _, cr := range crList.Items {
+			err = validation.ValidateCustomResource(field.NewPath(""), cr.UnstructuredContent(), validator).ToAggregate()
+			if err != nil {
+				var namespacedName string
+				if cr.GetNamespace() == "" {
+					namespacedName = cr.GetName()
+				} else {
+					namespacedName = fmt.Sprintf("%s/%s", cr.GetNamespace(), cr.GetName())
+				}
+				return fmt.Errorf("error validating %s %q: updated validation is too restrictive: %v", cr.GroupVersionKind(), namespacedName, err)
+			}
 		}
 	}
 	return nil

pkg/controller/operators/catalog/operator_test.go

@@ -14,7 +14,6 @@ import (
 	"time"
 
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install"
-	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
 
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
@@ -1459,7 +1458,7 @@ func TestCompetingCRDOwnersExist(t *testing.T) {
 	}
 }
 
-func TestValidateExistingCRs(t *testing.T) {
+func TestValidateV1Beta1CRDCompatibility(t *testing.T) {
 	unstructuredForFile := func(file string) *unstructured.Unstructured {
 		data, err := os.ReadFile(file)
 		require.NoError(t, err)
@@ -1469,22 +1468,21 @@ func TestValidateExistingCRs(t *testing.T) {
 		return k8sFile
 	}
 
-	unversionedCRDForV1beta1File := func(file string) *apiextensions.CustomResourceDefinition {
+	unversionedCRDForV1beta1File := func(file string) *apiextensionsv1beta1.CustomResourceDefinition {
 		data, err := os.ReadFile(file)
 		require.NoError(t, err)
 		dec := utilyaml.NewYAMLOrJSONDecoder(strings.NewReader(string(data)), 30)
 		k8sFile := &apiextensionsv1beta1.CustomResourceDefinition{}
 		require.NoError(t, dec.Decode(k8sFile))
-		convertedCRD := &apiextensions.CustomResourceDefinition{}
-		require.NoError(t, apiextensionsv1beta1.Convert_v1beta1_CustomResourceDefinition_To_apiextensions_CustomResourceDefinition(k8sFile, convertedCRD, nil))
-		return convertedCRD
+		return k8sFile
 	}
 
 	tests := []struct {
 		name            string
 		existingObjects []runtime.Object
 		gvr             schema.GroupVersionResource
-		newCRD          *apiextensions.CustomResourceDefinition
+		oldCRD          *apiextensionsv1beta1.CustomResourceDefinition
+		newCRD          *apiextensionsv1beta1.CustomResourceDefinition
 		want            error
 	}{
 		{
@@ -1497,6 +1495,7 @@ func TestValidateExistingCRs(t *testing.T) {
 				Version:  "v1",
 				Resource: "machinepools",
 			},
+			oldCRD: unversionedCRDForV1beta1File("testdata/hivebug/crd.yaml"),
 			newCRD: unversionedCRDForV1beta1File("testdata/hivebug/crd.yaml"),
 		},
 		{
@@ -1509,16 +1508,144 @@ func TestValidateExistingCRs(t *testing.T) {
 				Version:  "v1",
 				Resource: "machinepools",
 			},
+			oldCRD: unversionedCRDForV1beta1File("testdata/hivebug/crd.yaml"),
 			newCRD: unversionedCRDForV1beta1File("testdata/hivebug/crd.yaml"),
-			want:   fmt.Errorf("error validating custom resource against new schema for MachinePool /test: [[].spec.clusterDeploymentRef: Invalid value: \"null\": spec.clusterDeploymentRef in body must be of type object: \"null\", [].spec.name: Required value, [].spec.platform: Required value]"),
+			want:   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]"),
+		},
+		{
+			name: "backwards incompatible change",
+			existingObjects: []runtime.Object{
+				unstructuredForFile("testdata/apiextensionsv1beta1/cr.yaml"),
+			},
+			gvr: schema.GroupVersionResource{
+				Group:    "cluster.com",
+				Version:  "v1alpha1",
+				Resource: "testcrd",
+			},
+			oldCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.old.yaml"),
+			newCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.yaml"),
+			want:   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"),
+		},
+		{
+			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",
+			},
+			oldCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.old.yaml"),
+			newCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.unserved.yaml"),
+		},
+		{
+			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"),
+			newCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.yaml"),
+		},
+		{
+			name: "crd with no versions list",
+			existingObjects: []runtime.Object{
+				unstructuredForFile("testdata/apiextensionsv1beta1/cr.yaml"),
+				unstructuredForFile("testdata/apiextensionsv1beta1/cr.v2.yaml"),
+			},
+			oldCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.no-versions-list.old.yaml"),
+			newCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.no-versions-list.yaml"),
+			want:   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"),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			client := fakedynamic.NewSimpleDynamicClient(runtime.NewScheme(), tt.existingObjects...)
+			require.Equal(t, tt.want, validateV1Beta1CRDCompatibility(client, tt.oldCRD, tt.newCRD))
+		})
+	}
+}
+
+func TestValidateV1CRDCompatibility(t *testing.T) {
+	unstructuredForFile := func(file string) *unstructured.Unstructured {
+		data, err := os.ReadFile(file)
+		require.NoError(t, err)
+		dec := utilyaml.NewYAMLOrJSONDecoder(strings.NewReader(string(data)), 30)
+		k8sFile := &unstructured.Unstructured{}
+		require.NoError(t, dec.Decode(k8sFile))
+		return k8sFile
+	}
+
+	unversionedCRDForV1File := func(file string) *apiextensionsv1.CustomResourceDefinition {
+		data, err := os.ReadFile(file)
+		require.NoError(t, err)
+		dec := utilyaml.NewYAMLOrJSONDecoder(strings.NewReader(string(data)), 30)
+		k8sFile := &apiextensionsv1.CustomResourceDefinition{}
+		require.NoError(t, dec.Decode(k8sFile))
+		return k8sFile
+	}
+
+	tests := []struct {
+		name        string
+		existingCRs []runtime.Object
+		gvr         schema.GroupVersionResource
+		oldCRD      *apiextensionsv1.CustomResourceDefinition
+		newCRD      *apiextensionsv1.CustomResourceDefinition
+		want        error
+	}{
+		{
+			name: "valid",
+			existingCRs: []runtime.Object{
+				unstructuredForFile("testdata/apiextensionsv1/crontabs.cr.valid.v1.yaml"),
+				unstructuredForFile("testdata/apiextensionsv1/crontabs.cr.valid.v2.yaml"),
+			},
+			oldCRD: unversionedCRDForV1File("testdata/apiextensionsv1/crontabs.crd.old.yaml"),
+			newCRD: unversionedCRDForV1File("testdata/apiextensionsv1/crontabs.crd.yaml"),
+		},
+		{
+			name: "validation failure",
+			existingCRs: []runtime.Object{
+				unstructuredForFile("testdata/apiextensionsv1/crontabs.cr.valid.v1.yaml"),
+				unstructuredForFile("testdata/apiextensionsv1/crontabs.cr.fail.v2.yaml"),
+			},
+			oldCRD: unversionedCRDForV1File("testdata/apiextensionsv1/crontabs.crd.old.yaml"),
+			newCRD: unversionedCRDForV1File("testdata/apiextensionsv1/crontabs.crd.yaml"),
+			want:   fmt.Errorf("error validating stable.example.com/v2, Kind=CronTab \"my-crontab\": updated validation is too restrictive: [].spec.replicas: Invalid value: 10: spec.replicas in body should be less than or equal to 9"),
+		},
+		{
+			name: "cr not invalidated by unserved version",
+			existingCRs: []runtime.Object{
+				unstructuredForFile("testdata/apiextensionsv1/crontabs.cr.valid.v1.yaml"),
+				unstructuredForFile("testdata/apiextensionsv1/crontabs.cr.valid.v2.yaml"),
+			},
+			oldCRD: unversionedCRDForV1File("testdata/apiextensionsv1/crontabs.crd.old.yaml"),
+			newCRD: unversionedCRDForV1File("testdata/apiextensionsv1/crontabs.crd.unserved.yaml"),
+		},
+		{
+			name: "cr not validated against currently unserved version",
+			existingCRs: []runtime.Object{
+				unstructuredForFile("testdata/apiextensionsv1/crontabs.cr.valid.v1.yaml"),
+				unstructuredForFile("testdata/apiextensionsv1/crontabs.cr.valid.v2.yaml"),
+			},
+			oldCRD: unversionedCRDForV1File("testdata/apiextensionsv1/crontabs.crd.old.unserved.yaml"),
+			newCRD: unversionedCRDForV1File("testdata/apiextensionsv1/crontabs.crd.yaml"),
+		},
+		{
+			name: "validation failure with single CRD version",
+			existingCRs: []runtime.Object{
+				unstructuredForFile("testdata/apiextensionsv1/single-version-cr.yaml"),
+			},
+			oldCRD: unversionedCRDForV1File("testdata/apiextensionsv1/single-version-crd.old.yaml"),
+			newCRD: unversionedCRDForV1File("testdata/apiextensionsv1/single-version-crd.yaml"),
+			want:   fmt.Errorf("error validating cluster.com/v1alpha1, Kind=testcrd \"my-cr-1\": updated validation is too restrictive: [].spec.scalar: Invalid value: 100: spec.scalar in body should be less than or equal to 50"),
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			client := fakedynamic.NewSimpleDynamicClientWithCustomListKinds(runtime.NewScheme(), map[schema.GroupVersionResource]string{
-				tt.gvr: "UnstructuredList",
-			}, tt.existingObjects...)
-			require.Equal(t, tt.want, validateExistingCRs(client, tt.gvr, tt.newCRD))
+			client := fakedynamic.NewSimpleDynamicClient(runtime.NewScheme(), tt.existingCRs...)
+			require.Equal(t, tt.want, validateV1CRDCompatibility(client, tt.oldCRD, tt.newCRD))
 		})
 	}
 }

pkg/controller/operators/catalog/testdata/apiextensionsv1/crontabs.cr.fail.v2.yaml

@@ -0,0 +1,8 @@
+apiVersion: stable.example.com/v2
+kind: CronTab
+metadata:
+  name: my-crontab
+spec:
+  cronSpec: "* * * * *"
+  image: ""
+  replicas: 10

pkg/controller/operators/catalog/testdata/apiextensionsv1/crontabs.cr.valid.v1.yaml

@@ -0,0 +1,8 @@
+apiVersion: stable.example.com/v1
+kind: CronTab
+metadata:
+  name: my-crontab-v1
+spec:
+  cronSpec: "* * * * *"
+  image: ""
+  replicas: 9

pkg/controller/operators/catalog/testdata/apiextensionsv1/crontabs.cr.valid.v2.yaml

@@ -0,0 +1,8 @@
+apiVersion: stable.example.com/v2
+kind: CronTab
+metadata:
+  name: my-crontab-v2
+spec:
+  cronSpec: "* * * * *"
+  image: ""
+  replicas: 9