Bug 1732914: Operator upgrades fail when versions field is not set

1. Fix the ensureCRDVersions to account for `version` field 2. Add test cases for `version` scenarios Signed-off-by: Vu Dinh <vdinh@redhat.com>
operator-framework · Aug 16, 2019 · fcb5719 · fcb5719
1 parent 26e2cc3
commit fcb5719
Show file tree

Hide file tree

Showing 3 changed files with 92 additions and 15 deletions.
diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go
@@ -1064,23 +1064,35 @@ func (o *Operator) ResolvePlan(plan *v1alpha1.InstallPlan) error {
 	return nil
 }
 
-// Ensure all existing versions are present in new CRD
+// Ensure all existing served versions are present in new CRD
 func ensureCRDVersions(oldCRD *v1beta1ext.CustomResourceDefinition, newCRD *v1beta1ext.CustomResourceDefinition) error {
+	newCRDVersions := map[string]struct{}{}
+
+	for _, newVersion := range newCRD.Spec.Versions {
+		newCRDVersions[newVersion.Name] = struct{}{}
+	}
+	if newCRD.Spec.Version != "" {
+		newCRDVersions[newCRD.Spec.Version] = struct{}{}
+	}
+
 	for _, oldVersion := range oldCRD.Spec.Versions {
-		var versionPresent bool
-		for _, newVersion := range newCRD.Spec.Versions {
-			if oldVersion.Name == newVersion.Name {
-				versionPresent = true
+		if oldVersion.Served {
+			_, ok := newCRDVersions[oldVersion.Name]
+			if !ok {
+				return fmt.Errorf("New CRD (%s) must contain existing served versions (%s)", oldCRD.Name, oldVersion.Name)
 			}
 		}
-		if !versionPresent {
-			return fmt.Errorf("not allowing CRD (%v) update with unincluded version %v", newCRD.GetName(), oldVersion)
+	}
+	if oldCRD.Spec.Version != "" {
+		_, ok := newCRDVersions[oldCRD.Spec.Version]
+		if !ok {
+			return fmt.Errorf("New CRD (%s) must contain existing version (%s)", oldCRD.Name, oldCRD.Spec.Version)
 		}
 	}
-
 	return nil
 }
 
+// Validate all existing served versions against new CRD's validation (if changed)
 func (o *Operator) validateCustomResourceDefinition(oldCRD *v1beta1ext.CustomResourceDefinition, newCRD *v1beta1ext.CustomResourceDefinition) error {
 	o.logger.Debugf("Comparing %#v to %#v", oldCRD.Spec.Validation, newCRD.Spec.Validation)
 	// If validation schema is unchanged, return right away
@@ -1091,11 +1103,13 @@ func (o *Operator) validateCustomResourceDefinition(oldCRD *v1beta1ext.CustomRes
 	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: oldCRD.Spec.Names.Plural}
-		err := o.validateExistingCRs(gvr, convertedCRD)
-		if 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 := o.validateExistingCRs(gvr, convertedCRD)
+			if err != nil {
+				return err
+			}
 		}
 	}
 
@@ -1106,7 +1120,7 @@ func (o *Operator) validateCustomResourceDefinition(oldCRD *v1beta1ext.CustomRes
 			return err
 		}
 	}
-
+	o.logger.Debugf("Successfully validated CRD %s\n", newCRD.Name)
 	return nil
 }
 
@@ -1198,7 +1212,8 @@ func (o *Operator) ExecutePlan(plan *v1alpha1.InstallPlan) error {
 						} else if len(matchedCSV) > 1 {
 							o.logger.Debugf("Found multiple owners for CRD %v", crd)
 
-							if err := ensureCRDVersions(currentCRD, &crd); err != nil {
+							err := ensureCRDVersions(currentCRD, &crd)
+							if err != nil {
 								return errorwrap.Wrapf(err, "error missing existing CRD version(s) in new CRD: %s", step.Resource.Name)
 							}
 

diff --git a/pkg/controller/operators/catalog/operator_test.go b/pkg/controller/operators/catalog/operator_test.go
@@ -195,6 +195,35 @@ func TestEnsureCRDVersions(t *testing.T) {
 			}(),
 			expectedFailure: true,
 		},
+		{
+			name: "missing version in new CRD",
+			oldCRD: func() v1beta1.CustomResourceDefinition {
+				oldCRD := crd(mainCRDPlural)
+				oldCRD.Spec.Version = "v1alpha1"
+				return oldCRD
+			}(),
+			newCRD: func() v1beta1.CustomResourceDefinition {
+				newCRD := crd(mainCRDPlural)
+				newCRD.Spec.Version = "v1alpha2"
+				return newCRD
+			}(),
+			expectedFailure: true,
+		},
+		{
+			name: "existing version is present in new CRD's versions",
+			oldCRD: func() v1beta1.CustomResourceDefinition {
+				oldCRD := crd(mainCRDPlural)
+				oldCRD.Spec.Version = "v1alpha1"
+				return oldCRD
+			}(),
+			newCRD: func() v1beta1.CustomResourceDefinition {
+				newCRD := crd(mainCRDPlural)
+				newCRD.Spec.Version = ""
+				newCRD.Spec.Versions = addedVersions
+				return newCRD
+			}(),
+			expectedFailure: false,
+		},
 	}
 
 	for _, tt := range tests {

diff --git a/test/e2e/installplan_e2e_test.go b/test/e2e/installplan_e2e_test.go
@@ -823,6 +823,39 @@ func TestInstallPlanWithCRDSchemaChange(t *testing.T) {
 				return &newCRD
 			}(),
 		},
+		{
+			name:          "existing version is present in new CRD (deprecated field)",
+			expectedPhase: v1alpha1.InstallPlanPhaseComplete,
+			oldCRD: func() *apiextensions.CustomResourceDefinition {
+				oldCRD := newCRD(mainCRDPlural + "d")
+				oldCRD.Spec.Version = "v1alpha1"
+				return &oldCRD
+			}(),
+			newCRD: func() *apiextensions.CustomResourceDefinition {
+				newCRD := newCRD(mainCRDPlural + "d")
+				newCRD.Spec.Version = "v1alpha1"
+				newCRD.Spec.Validation = &apiextensions.CustomResourceValidation{
+					OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
+						Type: "object",
+						Properties: map[string]apiextensions.JSONSchemaProps{
+							"spec": {
+								Type:        "object",
+								Description: "Spec of a test object.",
+								Properties: map[string]apiextensions.JSONSchemaProps{
+									"scalar": {
+										Type:        "number",
+										Description: "Scalar value that should have a min and max.",
+										Minimum:     &min,
+										Maximum:     &max,
+									},
+								},
+							},
+						},
+					},
+				}
+				return &newCRD
+			}(),
+		},
 	}
 
 	for _, tt := range tests {