Add e2e test case for existing CR validation

Signed-off-by: Vu Dinh <[email protected]>

Author: dinhxuanvu

Documentation/design/dependency-resolution.md

@@ -2,15 +2,23 @@
 
 OLM manages the dependency resolution and upgrade lifecycle of running operators. In many ways, thes problems OLM faces are similar to other OS package managers like `apt`/`dkpg` and `yum`/`rpm`.
 
-However, there is one constraint that similar systems don't generally have that OLM does: because operators are always running, OLM attempts to ensure that at no point in time are you left with a set of operators that do not work with each other. 
+However, there is one constraint that similar systems don't generally have that OLM does: because operators are always running, OLM attempts to ensure that at no point in time are you left with a set of operators that do not work with each other.
 
 This means that OLM needs to never:
- 
+
  - install a set of operators that require APIs that can't be provided
  - update an operator in a way that breaks another that depends upon it
- 
+
 The following examples motivate why OLM's dependency resolution and upgrade strategy works as it does, followed by a description of the current algorithm.
 
+## CustomResourceDefinition (CRD) Upgrade
+
+OLM will upgrade CRD right away if it is owned by singular CSV. If CRD is owned by multiple CSVs, then CRD is upgraded when it is
+satisfied all of the following backward compatible conditions:
+
+- All existing versions in current CRD are present in new CRD
+- All existing instances (Custom Resource) of current CRD are valid when validated against new CRD's validation schema
+
 # Example: Deprecate dependant API
 
 A and B are APIs (e.g. CRDs)
@@ -41,7 +49,7 @@ If we attempt to update A without simultaneously updating B, or vice-versa, we w
 This is another case we prevent with OLM's upgrade strategy.
 
 
-# Dependency resolution 
+# Dependency resolution
 
 A Provider is an operator which "Owns" a CRD or APIService.
 
@@ -55,12 +63,12 @@ Consider the set of operators defined by running operators in a namespace:
         provisionally add the operator to the generation
      else
         check for a replacement in the source/package/channel
-  
+
   // Generation resolution
   For each required API with no provider in gen:
     search through prioritized sources to pick a provider
     provisionally add any new operators found to the generation, this could also add to the required APIs without providers
-    
+
   // Downgrade
   if there are still required APIs that can't be satisfied by sources:
     downgrade the operator(s) that require the APIs that can't be satisfied

pkg/controller/operators/catalog/operator.go

@@ -20,7 +20,6 @@ import (
 	extinf "k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	conversion "k8s.io/apimachinery/pkg/conversion"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	utilclock "k8s.io/apimachinery/pkg/util/clock"
@@ -1045,26 +1044,12 @@ func (o *Operator) ResolvePlan(plan *v1alpha1.InstallPlan) error {
 	return nil
 }
 
-// TODO: in the future this will be extended to verify more than just whether or not the schema changed
-func checkCRDSchemas(oldCRD *v1beta1ext.CustomResourceDefinition, newCRD *v1beta1ext.CustomResourceDefinition) bool {
-	if reflect.DeepEqual(oldCRD.Spec.Validation, newCRD.Spec.Validation) {
-		return true
-	}
-
-	return false
-}
-
-func safeToUpdate(oldCRD *v1beta1ext.CustomResourceDefinition, newCRD *v1beta1ext.CustomResourceDefinition) error {
-	shouldCheckSchema := len(oldCRD.Spec.Versions) == len(newCRD.Spec.Versions) // no version bump
-
-	// 1) if there's no version change, verify that schemas match 2) ensure all old versions are present
+// Ensure all existing versions are present in new CRD
+func ensureCRDVersions(oldCRD *v1beta1ext.CustomResourceDefinition, newCRD *v1beta1ext.CustomResourceDefinition) error {
 	for _, oldVersion := range oldCRD.Spec.Versions {
 		var versionPresent bool
 		for _, newVersion := range newCRD.Spec.Versions {
 			if oldVersion.Name == newVersion.Name {
-				if shouldCheckSchema && !checkCRDSchemas(oldCRD, newCRD) {
-					return fmt.Errorf("not allowing CRD (%v) update with multiple owners with schema change on version %v", newCRD.GetName(), oldVersion)
-				}
 				versionPresent = true
 			}
 		}
@@ -1073,32 +1058,29 @@ func safeToUpdate(oldCRD *v1beta1ext.CustomResourceDefinition, newCRD *v1beta1ex
 		}
 	}
 
-	// ensure a CRD with multiple versions isn't checked
-	if newCRD.Spec.Version != "" {
-		if !checkCRDSchemas(oldCRD, newCRD) {
-			return fmt.Errorf("not allowing CRD (%v) update with multiple owners with schema change on (single) version %v", newCRD.GetName(), newCRD.Spec.Version)
-		}
-	}
 	return nil
 }
 
 func (o *Operator) validateCustomResourceDefinition(oldCRD *v1beta1ext.CustomResourceDefinition, newCRD *v1beta1ext.CustomResourceDefinition) error {
 	o.logger.Debugf("Comparing %#v to %#v", oldCRD.Spec.Validation, newCRD.Spec.Validation)
-	var convertedCRD *apiextensions.CustomResourceDefinition
-	var scope conversion.Scope
-	if err := v1beta1ext.Convert_v1beta1_CustomResourceDefinition_To_apiextensions_CustomResourceDefinition(newCRD, convertedCRD, scope); err != nil {
+	// If validation schema is unchanged, return right away
+	if reflect.DeepEqual(oldCRD.Spec.Validation, newCRD.Spec.Validation) {
+		return nil
+	}
+	convertedCRD := &apiextensions.CustomResourceDefinition{}
+	if err := v1beta1ext.Convert_v1beta1_CustomResourceDefinition_To_apiextensions_CustomResourceDefinition(newCRD, convertedCRD, nil); err != nil {
 		return err
 	}
 	for _, oldVersion := range oldCRD.Spec.Versions {
-		gvr := schema.GroupVersionResource{Group: oldCRD.Spec.Group, Version: oldVersion.Name, Resource: newCRD.Spec.Names.Plural}
+		gvr := schema.GroupVersionResource{Group: oldCRD.Spec.Group, Version: oldVersion.Name, Resource: oldCRD.Spec.Names.Plural}
 		err := o.validateExistingCRs(gvr, convertedCRD)
 		if err != nil {
 			return err
 		}
 	}
 
 	if oldCRD.Spec.Version != "" {
-		gvr := schema.GroupVersionResource{Group: oldCRD.Spec.Group, Version: oldCRD.Spec.Version, Resource: newCRD.Spec.Names.Plural}
+		gvr := schema.GroupVersionResource{Group: oldCRD.Spec.Group, Version: oldCRD.Spec.Version, Resource: oldCRD.Spec.Names.Plural}
 		err := o.validateExistingCRs(gvr, convertedCRD)
 		if err != nil {
 			return err
@@ -1118,7 +1100,7 @@ func (o *Operator) validateExistingCRs(gvr schema.GroupVersionResource, newCRD *
 		if err != nil {
 			return fmt.Errorf("error creating validator for schema %#v: %s", newCRD.Spec.Validation, err)
 		}
-		err = validation.ValidateCustomResource(cr, validator)
+		err = validation.ValidateCustomResource(cr.UnstructuredContent(), validator)
 		if err != nil {
 			return fmt.Errorf("error validating custom resource against new schema %#v: %s", newCRD.Spec.Validation, err)
 		}
@@ -1188,21 +1170,22 @@ func (o *Operator) ExecutePlan(plan *v1alpha1.InstallPlan) error {
 						crd.SetResourceVersion(currentCRD.GetResourceVersion())
 						if len(matchedCSV) == 1 {
 							o.logger.Debugf("Found one owner for CRD %v", crd)
-							if err = o.validateCustomResourceDefinition(currentCRD, &crd); err != nil {
-								return errorwrap.Wrapf(err, "error validating existing CRs agains new CRD's schema: %s", step.Resource.Name)
-							}
+
 							_, err = o.opClient.ApiextensionsV1beta1Interface().ApiextensionsV1beta1().CustomResourceDefinitions().Update(&crd)
 							if err != nil {
 								return errorwrap.Wrapf(err, "error updating CRD: %s", step.Resource.Name)
 							}
 						} else if len(matchedCSV) > 1 {
 							o.logger.Debugf("Found multiple owners for CRD %v", crd)
-							if err := safeToUpdate(currentCRD, &crd); err != nil {
-								return err
+
+							if err := ensureCRDVersions(currentCRD, &crd); err != nil {
+								return errorwrap.Wrapf(err, "error missing existing CRD version(s) in new CRD: %s", step.Resource.Name)
 							}
+
 							if err = o.validateCustomResourceDefinition(currentCRD, &crd); err != nil {
 								return errorwrap.Wrapf(err, "error validating existing CRs agains new CRD's schema: %s", step.Resource.Name)
 							}
+
 							_, err = o.opClient.ApiextensionsV1beta1Interface().ApiextensionsV1beta1().CustomResourceDefinitions().Update(&crd)
 							if err != nil {
 								return errorwrap.Wrapf(err, "error update CRD: %s", step.Resource.Name)

pkg/controller/operators/catalog/operator_test.go

@@ -125,34 +125,18 @@ func TestTransitionInstallPlan(t *testing.T) {
 	}
 }
 
-func TestSafeToUpdate(t *testing.T) {
+func TestEnsureCRDVersions(t *testing.T) {
 	mainCRDPlural := "ins-main-abcde"
-	differentSchema := &v1beta1.CustomResourceValidation{
-		OpenAPIV3Schema: &v1beta1.JSONSchemaProps{
-			Properties: map[string]v1beta1.JSONSchemaProps{
-				"spec": {
-					Type:        "object",
-					Description: "Spec of a test object.",
-					Properties: map[string]v1beta1.JSONSchemaProps{
-						"scalar": {
-							Type:        "number",
-							Description: "Scalar value that should have a min and max.",
-							Minimum: func() *float64 {
-								var min float64 = 2
-								return &min
-							}(),
-							Maximum: func() *float64 {
-								var max float64 = 256
-								return &max
-							}(),
-						},
-					},
-				},
-			},
+
+	currentVersions := []v1beta1.CustomResourceDefinitionVersion{
+		{
+			Name:    "v1alpha1",
+			Served:  true,
+			Storage: true,
 		},
 	}
 
-	differentVersions := []v1beta1.CustomResourceDefinitionVersion{
+	addedVersions := []v1beta1.CustomResourceDefinitionVersion{
 		{
 			Name:    "v1alpha1",
 			Served:  true,
@@ -165,53 +149,56 @@ func TestSafeToUpdate(t *testing.T) {
 		},
 	}
 
+	missingVersions := []v1beta1.CustomResourceDefinitionVersion{
+		{
+			Name:    "v1alpha2",
+			Served:  true,
+			Storage: true,
+		},
+	}
+
 	tests := []struct {
 		name            string
 		oldCRD          v1beta1.CustomResourceDefinition
 		newCRD          v1beta1.CustomResourceDefinition
 		expectedFailure bool
 	}{
 		{
-			// in reality this would never happen within ExecutePlan, but fully exercises function
-			name:   "same version, same schema",
-			oldCRD: crd(mainCRDPlural),
-			newCRD: crd(mainCRDPlural),
-		},
-		{
-			name:   "same version, different schema",
-			oldCRD: crd(mainCRDPlural),
-			newCRD: func() v1beta1.CustomResourceDefinition {
-				newCRD := crd(mainCRDPlural)
-				newCRD.Spec.Validation = differentSchema
-				return newCRD
+			name: "existing versions are present",
+			oldCRD: func() v1beta1.CustomResourceDefinition {
+				oldCRD := crd(mainCRDPlural)
+				oldCRD.Spec.Version = ""
+				oldCRD.Spec.Versions = currentVersions
+				return oldCRD
 			}(),
-			expectedFailure: true,
-		},
-		{
-			name:   "different version, different schema",
-			oldCRD: crd(mainCRDPlural),
 			newCRD: func() v1beta1.CustomResourceDefinition {
 				newCRD := crd(mainCRDPlural)
 				newCRD.Spec.Version = ""
-				newCRD.Spec.Versions = differentVersions
-				newCRD.Spec.Validation = differentSchema
+				newCRD.Spec.Versions = addedVersions
 				return newCRD
 			}(),
+			expectedFailure: false,
 		},
 		{
-			name:   "different version, same schema",
-			oldCRD: crd(mainCRDPlural),
+			name: "missing versions in new CRD",
+			oldCRD: func() v1beta1.CustomResourceDefinition {
+				oldCRD := crd(mainCRDPlural)
+				oldCRD.Spec.Version = ""
+				oldCRD.Spec.Versions = currentVersions
+				return oldCRD
+			}(),
 			newCRD: func() v1beta1.CustomResourceDefinition {
 				newCRD := crd(mainCRDPlural)
 				newCRD.Spec.Version = ""
-				newCRD.Spec.Versions = differentVersions
+				newCRD.Spec.Versions = missingVersions
 				return newCRD
 			}(),
+			expectedFailure: true,
 		},
 	}
 
 	for _, tt := range tests {
-		err := safeToUpdate(&tt.oldCRD, &tt.newCRD)
+		err := ensureCRDVersions(&tt.oldCRD, &tt.newCRD)
 		if tt.expectedFailure {
 			require.Error(t, err)
 		}