Merge pull request #1470 from exdx/fix/bug-1825330

Bug 1825330: support creating v1beta CRDs to avoid data loss

Author: openshift-merge-robot

cmd/olm/cleanup_test.go

@@ -478,7 +478,7 @@ func TestCleanupOwnerReferences(t *testing.T) {
 			require.Equal(t, tt.expected.err, cleanupOwnerReferences(c, crc))
 
 			listOpts := metav1.ListOptions{}
-			csvs, err := crc.OperatorsV1alpha1().ClusterServiceVersions(metav1.NamespaceAll).List(listOpts)
+			csvs, err := crc.OperatorsV1alpha1().ClusterServiceVersions(metav1.NamespaceAll).List(context.TODO(), listOpts)
 			require.NoError(t, err)
 			require.ElementsMatch(t, tt.expected.csvs, csvs.Items)
 

pkg/controller/operators/catalog/operator.go

@@ -17,6 +17,7 @@ import (
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	"k8s.io/apiextensions-apiserver/pkg/apiserver/validation"
 	extinf "k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -1305,30 +1306,6 @@ func (o *Operator) ResolvePlan(plan *v1alpha1.InstallPlan) error {
 	return nil
 }
 
-func GetCRDV1VersionsMap(crd *apiextensionsv1.CustomResourceDefinition) map[string]struct{} {
-	versionsMap := map[string]struct{}{}
-
-	for _, version := range crd.Spec.Versions {
-		versionsMap[version.Name] = struct{}{}
-	}
-	return versionsMap
-}
-
-// Ensure all existing served versions are present in new CRD
-func EnsureCRDVersions(oldCRD *apiextensionsv1.CustomResourceDefinition, newCRD *apiextensionsv1.CustomResourceDefinition) error {
-	newCRDVersions := GetCRDV1VersionsMap(newCRD)
-
-	for _, oldVersion := range oldCRD.Spec.Versions {
-		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)
-			}
-		}
-	}
-	return nil
-}
-
 // Validate all existing served versions against new CRD's validation (if changed)
 func validateV1CRDCompatibility(dynamicClient dynamic.Interface, oldCRD *apiextensionsv1.CustomResourceDefinition, newCRD *apiextensionsv1.CustomResourceDefinition) error {
 	logrus.Debugf("Comparing %#v to %#v", oldCRD.Spec.Versions, newCRD.Spec.Versions)
@@ -1364,6 +1341,36 @@ func validateV1CRDCompatibility(dynamicClient dynamic.Interface, oldCRD *apiexte
 	return nil
 }
 
+// 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
+	}
+	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
+			}
+		}
+	}
+
+	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
+		}
+	}
+	logrus.Debugf("Successfully validated CRD %s\n", newCRD.Name)
+	return nil
+}
+
 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{})
@@ -1383,33 +1390,6 @@ func validateExistingCRs(dynamicClient dynamic.Interface, gvr schema.GroupVersio
 	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 removeDeprecatedV1StoredVersions(oldCRD *apiextensionsv1.CustomResourceDefinition, newCRD *apiextensionsv1.CustomResourceDefinition) []string {
-	// StoredVersions requires to have at least one version.
-	if len(oldCRD.Status.StoredVersions) <= 1 {
-		return nil
-	}
-
-	newStoredVersions := []string{}
-	newCRDVersions := GetCRDV1VersionsMap(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 {
@@ -1436,7 +1416,7 @@ func (o *Operator) ExecutePlan(plan *v1alpha1.InstallPlan) error {
 	}
 
 	ensurer := newStepEnsurer(kubeclient, crclient, dynamicClient)
-	b := newBuilder(kubeclient, dynamicClient, o.csvProvidedAPIsIndexer)
+	b := newBuilder(kubeclient, dynamicClient, o.csvProvidedAPIsIndexer, o.logger)
 
 	for i, step := range plan.Status.Plan {
 		doStep := true

pkg/controller/operators/catalog/operator_test.go

@@ -135,203 +135,6 @@ func TestTransitionInstallPlan(t *testing.T) {
 	}
 }
 
-func TestEnsureV1CRDVersions(t *testing.T) {
-	mainCRDPlural := "ins-main-abcde"
-
-	currentVersions := []apiextensionsv1.CustomResourceDefinitionVersion{
-		{
-			Name:    "v1alpha1",
-			Served:  true,
-			Storage: true,
-		},
-	}
-
-	addedVersions := []apiextensionsv1.CustomResourceDefinitionVersion{
-		{
-			Name:    "v1alpha1",
-			Served:  true,
-			Storage: false,
-		},
-		{
-			Name:    "v1alpha2",
-			Served:  true,
-			Storage: true,
-		},
-	}
-
-	missingVersions := []apiextensionsv1.CustomResourceDefinitionVersion{
-		{
-			Name:    "v1alpha2",
-			Served:  true,
-			Storage: true,
-		},
-	}
-
-	tests := []struct {
-		name            string
-		oldCRD          apiextensionsv1.CustomResourceDefinition
-		newCRD          apiextensionsv1.CustomResourceDefinition
-		expectedFailure bool
-	}{
-		{
-			name: "existing versions are present",
-			oldCRD: func() apiextensionsv1.CustomResourceDefinition {
-				oldCRD := v1crd(mainCRDPlural)
-				oldCRD.Spec.Versions = currentVersions
-				return oldCRD
-			}(),
-			newCRD: func() apiextensionsv1.CustomResourceDefinition {
-				newCRD := v1crd(mainCRDPlural)
-				newCRD.Spec.Versions = addedVersions
-				return newCRD
-			}(),
-			expectedFailure: false,
-		},
-		{
-			name: "missing versions in new CRD 1",
-			oldCRD: func() apiextensionsv1.CustomResourceDefinition {
-				oldCRD := v1crd(mainCRDPlural)
-				oldCRD.Spec.Versions = currentVersions
-				return oldCRD
-			}(),
-			newCRD: func() apiextensionsv1.CustomResourceDefinition {
-				newCRD := v1crd(mainCRDPlural)
-				newCRD.Spec.Versions = missingVersions
-				return newCRD
-			}(),
-			expectedFailure: true,
-		},
-		{
-			name: "missing version in new CRD 2",
-			oldCRD: func() apiextensionsv1.CustomResourceDefinition {
-				oldCRD := v1crd(mainCRDPlural)
-				oldCRD.Spec.Versions[0].Name = "v1alpha1"
-				return oldCRD
-			}(),
-			newCRD: func() apiextensionsv1.CustomResourceDefinition {
-				newCRD := v1crd(mainCRDPlural)
-				newCRD.Spec.Versions[0].Name = "v1alpha2"
-				return newCRD
-			}(),
-			expectedFailure: true,
-		},
-		{
-			name: "existing version is present in new CRD's versions",
-			oldCRD: func() apiextensionsv1.CustomResourceDefinition {
-				oldCRD := v1crd(mainCRDPlural)
-				oldCRD.Spec.Versions[0].Name = "v1alpha1"
-				return oldCRD
-			}(),
-			newCRD: func() apiextensionsv1.CustomResourceDefinition {
-				newCRD := v1crd(mainCRDPlural)
-				newCRD.Spec.Versions = addedVersions
-				return newCRD
-			}(),
-			expectedFailure: false,
-		},
-	}
-
-	for _, tt := range tests {
-		err := EnsureCRDVersions(&tt.oldCRD, &tt.newCRD)
-		if tt.expectedFailure {
-			require.Error(t, err)
-		}
-	}
-}
-
-func TestRemoveDeprecatedV1StoredVersions(t *testing.T) {
-	mainCRDPlural := "ins-main-test"
-
-	currentVersions := []apiextensionsv1.CustomResourceDefinitionVersion{
-		{
-			Name:    "v1alpha1",
-			Served:  true,
-			Storage: false,
-		},
-		{
-			Name:    "v1alpha2",
-			Served:  true,
-			Storage: true,
-		},
-	}
-
-	newVersions := []apiextensionsv1.CustomResourceDefinitionVersion{
-		{
-			Name:    "v1alpha2",
-			Served:  true,
-			Storage: false,
-		},
-		{
-			Name:    "apiextensionsv1beta1",
-			Served:  true,
-			Storage: true,
-		},
-	}
-
-	crdStatusStoredVersions := apiextensionsv1.CustomResourceDefinitionStatus{
-		StoredVersions: []string{},
-	}
-
-	tests := []struct {
-		name           string
-		oldCRD         apiextensionsv1.CustomResourceDefinition
-		newCRD         apiextensionsv1.CustomResourceDefinition
-		expectedResult []string
-	}{
-		{
-			name: "only one stored version exists",
-			oldCRD: func() apiextensionsv1.CustomResourceDefinition {
-				oldCRD := v1crd(mainCRDPlural)
-				oldCRD.Spec.Versions = currentVersions
-				oldCRD.Status = crdStatusStoredVersions
-				oldCRD.Status.StoredVersions = []string{"v1alpha1"}
-				return oldCRD
-			}(),
-			newCRD: func() apiextensionsv1.CustomResourceDefinition {
-				newCRD := v1crd(mainCRDPlural)
-				newCRD.Spec.Versions = newVersions
-				return newCRD
-			}(),
-			expectedResult: nil,
-		},
-		{
-			name: "multiple stored versions with one deprecated version",
-			oldCRD: func() apiextensionsv1.CustomResourceDefinition {
-				oldCRD := v1crd(mainCRDPlural)
-				oldCRD.Spec.Versions = currentVersions
-				oldCRD.Status.StoredVersions = []string{"v1alpha1", "v1alpha2"}
-				return oldCRD
-			}(),
-			newCRD: func() apiextensionsv1.CustomResourceDefinition {
-				newCRD := v1crd(mainCRDPlural)
-				newCRD.Spec.Versions = newVersions
-				return newCRD
-			}(),
-			expectedResult: []string{"v1alpha2"},
-		},
-		{
-			name: "multiple stored versions with all deprecated version",
-			oldCRD: func() apiextensionsv1.CustomResourceDefinition {
-				oldCRD := v1crd(mainCRDPlural)
-				oldCRD.Spec.Versions = currentVersions
-				oldCRD.Status.StoredVersions = []string{"v1alpha1", "v1alpha3"}
-				return oldCRD
-			}(),
-			newCRD: func() apiextensionsv1.CustomResourceDefinition {
-				newCRD := v1crd(mainCRDPlural)
-				newCRD.Spec.Versions = newVersions
-				return newCRD
-			}(),
-			expectedResult: nil,
-		},
-	}
-
-	for _, tt := range tests {
-		resultCRD := removeDeprecatedV1StoredVersions(&tt.oldCRD, &tt.newCRD)
-		require.Equal(t, tt.expectedResult, resultCRD)
-	}
-}
-
 func TestExecutePlan(t *testing.T) {
 	namespace := "ns"