Merge pull request #983 from dinhxuanvu/removeDeprecated

Bug 1746270: Remove deprecated CRD's stored versions to allow CRD update

Author: openshift-merge-robot

pkg/controller/operators/catalog/operator.go

@@ -1127,17 +1127,23 @@ func (o *Operator) ResolvePlan(plan *v1alpha1.InstallPlan) error {
 	return nil
 }
 
-// Ensure all existing served versions are present in new CRD
-func ensureCRDVersions(oldCRD *v1beta1ext.CustomResourceDefinition, newCRD *v1beta1ext.CustomResourceDefinition) error {
-	newCRDVersions := map[string]struct{}{}
+func getCRDVersionsMap(crd *v1beta1ext.CustomResourceDefinition) map[string]struct{} {
+	versionsMap := map[string]struct{}{}
 
-	for _, newVersion := range newCRD.Spec.Versions {
-		newCRDVersions[newVersion.Name] = struct{}{}
+	for _, version := range crd.Spec.Versions {
+		versionsMap[version.Name] = struct{}{}
 	}
-	if newCRD.Spec.Version != "" {
-		newCRDVersions[newCRD.Spec.Version] = struct{}{}
+	if crd.Spec.Version != "" {
+		versionsMap[crd.Spec.Version] = struct{}{}
 	}
 
+	return versionsMap
+}
+
+// Ensure all existing served versions are present in new CRD
+func ensureCRDVersions(oldCRD *v1beta1ext.CustomResourceDefinition, newCRD *v1beta1ext.CustomResourceDefinition) error {
+	newCRDVersions := getCRDVersionsMap(newCRD)
+
 	for _, oldVersion := range oldCRD.Spec.Versions {
 		if oldVersion.Served {
 			_, ok := newCRDVersions[oldVersion.Name]
@@ -1202,10 +1208,36 @@ func (o *Operator) validateExistingCRs(gvr schema.GroupVersionResource, newCRD *
 			return fmt.Errorf("error validating custom resource against new schema %#v: %s", newCRD.Spec.Validation, err)
 		}
 	}
-
 	return nil
 }
 
+// Attempt to remove stored versions that have been deprecated before allowing
+// those versions to be removed from the new CRD.
+// The function may not always succeed as storedVersions requires at least one
+// version. If there is only stored version, it won't be removed until a new
+// stored version is added.
+func removeDeprecatedStoredVersions(oldCRD *v1beta1ext.CustomResourceDefinition, newCRD *v1beta1ext.CustomResourceDefinition) []string {
+	// StoredVersions requires to have at least one version.
+	if len(oldCRD.Status.StoredVersions) <= 1 {
+		return nil
+	}
+
+	newStoredVersions := []string{}
+	newCRDVersions := getCRDVersionsMap(newCRD)
+	for _, v := range oldCRD.Status.StoredVersions {
+		_, ok := newCRDVersions[v]
+		if ok {
+			newStoredVersions = append(newStoredVersions, v)
+		}
+	}
+
+	if len(newStoredVersions) < 1 {
+		return nil
+	} else {
+		return newStoredVersions
+	}
+}
+
 // ExecutePlan applies a planned InstallPlan to a namespace.
 func (o *Operator) ExecutePlan(plan *v1alpha1.InstallPlan) error {
 	if plan.Status.Phase != v1alpha1.InstallPlanPhaseInstalling {
@@ -1255,7 +1287,9 @@ func (o *Operator) ExecutePlan(plan *v1alpha1.InstallPlan) error {
 				if k8serrors.IsAlreadyExists(err) {
 					currentCRD, _ := o.lister.APIExtensionsV1beta1().CustomResourceDefinitionLister().Get(crd.GetName())
 					// Compare 2 CRDs to see if it needs to be updatetd
-					if !reflect.DeepEqual(crd, *currentCRD) {
+					if !(reflect.DeepEqual(crd.Spec.Version, currentCRD.Spec.Version) &&
+						reflect.DeepEqual(crd.Spec.Versions, currentCRD.Spec.Versions) &&
+						reflect.DeepEqual(crd.Spec.Validation, currentCRD.Spec.Validation)) {
 						// Verify CRD ownership, only attempt to update if
 						// CRD has only one owner
 						// Example: provided=database.coreos.com/v1alpha1/EtcdCluster
@@ -1266,11 +1300,6 @@ 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)
-
-							_, 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)
 
@@ -1282,11 +1311,21 @@ func (o *Operator) ExecutePlan(plan *v1alpha1.InstallPlan) error {
 							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)
+						}
+						// Remove deprecated version in CRD storedVersions
+						storeVersions := removeDeprecatedStoredVersions(currentCRD, &crd)
+						if storeVersions != nil {
+							currentCRD.Status.StoredVersions = storeVersions
+							resultCRD, err := o.opClient.ApiextensionsV1beta1Interface().ApiextensionsV1beta1().CustomResourceDefinitions().UpdateStatus(currentCRD)
 							if err != nil {
-								return errorwrap.Wrapf(err, "error update CRD: %s", step.Resource.Name)
+								return errorwrap.Wrapf(err, "error updating CRD's status: %s", step.Resource.Name)
 							}
+							crd.SetResourceVersion(resultCRD.GetResourceVersion())
+						}
+						// Update CRD to new version
+						_, err = o.opClient.ApiextensionsV1beta1Interface().ApiextensionsV1beta1().CustomResourceDefinitions().Update(&crd)
+						if err != nil {
+							return errorwrap.Wrapf(err, "error updating CRD: %s", step.Resource.Name)
 						}
 					}
 					// If it already existed, mark the step as Present.

pkg/controller/operators/catalog/operator_test.go

@@ -235,6 +235,105 @@ func TestEnsureCRDVersions(t *testing.T) {
 	}
 }
 
+func TestRemoveDeprecatedStoredVersions(t *testing.T) {
+	mainCRDPlural := "ins-main-test"
+
+	currentVersions := []v1beta1.CustomResourceDefinitionVersion{
+		{
+			Name:    "v1alpha1",
+			Served:  true,
+			Storage: false,
+		},
+		{
+			Name:    "v1alpha2",
+			Served:  true,
+			Storage: true,
+		},
+	}
+
+	newVersions := []v1beta1.CustomResourceDefinitionVersion{
+		{
+			Name:    "v1alpha2",
+			Served:  true,
+			Storage: false,
+		},
+		{
+			Name:    "v1beta1",
+			Served:  true,
+			Storage: true,
+		},
+	}
+
+	crdStatusStoredVersions := v1beta1.CustomResourceDefinitionStatus{
+		StoredVersions: []string{},
+	}
+
+	tests := []struct {
+		name           string
+		oldCRD         v1beta1.CustomResourceDefinition
+		newCRD         v1beta1.CustomResourceDefinition
+		expectedResult []string
+	}{
+		{
+			name: "only one stored version exists",
+			oldCRD: func() v1beta1.CustomResourceDefinition {
+				oldCRD := crd(mainCRDPlural)
+				oldCRD.Spec.Version = ""
+				oldCRD.Spec.Versions = currentVersions
+				oldCRD.Status = crdStatusStoredVersions
+				oldCRD.Status.StoredVersions = []string{"v1alpha1"}
+				return oldCRD
+			}(),
+			newCRD: func() v1beta1.CustomResourceDefinition {
+				newCRD := crd(mainCRDPlural)
+				newCRD.Spec.Version = ""
+				newCRD.Spec.Versions = newVersions
+				return newCRD
+			}(),
+			expectedResult: nil,
+		},
+		{
+			name: "multiple stored versions with one deprecated version",
+			oldCRD: func() v1beta1.CustomResourceDefinition {
+				oldCRD := crd(mainCRDPlural)
+				oldCRD.Spec.Version = ""
+				oldCRD.Spec.Versions = currentVersions
+				oldCRD.Status.StoredVersions = []string{"v1alpha1", "v1alpha2"}
+				return oldCRD
+			}(),
+			newCRD: func() v1beta1.CustomResourceDefinition {
+				newCRD := crd(mainCRDPlural)
+				newCRD.Spec.Version = ""
+				newCRD.Spec.Versions = newVersions
+				return newCRD
+			}(),
+			expectedResult: []string{"v1alpha2"},
+		},
+		{
+			name: "multiple stored versions with all deprecated version",
+			oldCRD: func() v1beta1.CustomResourceDefinition {
+				oldCRD := crd(mainCRDPlural)
+				oldCRD.Spec.Version = ""
+				oldCRD.Spec.Versions = currentVersions
+				oldCRD.Status.StoredVersions = []string{"v1alpha1", "v1alpha3"}
+				return oldCRD
+			}(),
+			newCRD: func() v1beta1.CustomResourceDefinition {
+				newCRD := crd(mainCRDPlural)
+				newCRD.Spec.Version = ""
+				newCRD.Spec.Versions = newVersions
+				return newCRD
+			}(),
+			expectedResult: nil,
+		},
+	}
+
+	for _, tt := range tests {
+		resultCRD := removeDeprecatedStoredVersions(&tt.oldCRD, &tt.newCRD)
+		require.Equal(t, tt.expectedResult, resultCRD)
+	}
+}
+
 func TestExecutePlan(t *testing.T) {
 	namespace := "ns"