Bug 1828550: add check for storage version changes when installing CRDs

pkg/controller/operators/catalog/step.go

@@ -139,7 +139,16 @@ func (b *builder) NewCRDV1Step(client apiextensionsv1client.ApiextensionsV1Inter
 					}
 				}
 
-				// TODO ensure stored version compatibility
+				// check to see if stored versions changed and whether the upgrade could cause potential data loss
+				safe, err := crdlib.SafeStorageVersionUpgrade(currentCRD, crd)
+				if !safe {
+					b.logger.Errorf("risk of data loss updating %s: %s", step.Resource.Name, err)
+					return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "risk of data loss updating %s", step.Resource.Name)
+				}
+				if err != nil {
+					return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "checking CRD for potential data loss updating %s", step.Resource.Name)
+				}
+
 				// Update CRD to new version
 				_, err = client.CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{})
 				if err != nil {
@@ -219,7 +228,16 @@ func (b *builder) NewCRDV1Beta1Step(client apiextensionsv1beta1client.Apiextensi
 						return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "error validating existing CRs agains new CRD's schema: %s", step.Resource.Name)
 					}
 				}
-				// TODO ensure stored version compatibility
+
+				// check to see if stored versions changed and whether the upgrade could cause potential data loss
+				safe, err := crdlib.SafeStorageVersionUpgrade(currentCRD, crd)
+				if !safe {
+					b.logger.Errorf("risk of data loss updating %s: %s", step.Resource.Name, err)
+					return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "risk of data loss updating %s", step.Resource.Name)
+				}
+				if err != nil {
+					return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "checking CRD for potential data loss updating %s", step.Resource.Name)
+				}
 
 				// Update CRD to new version
 				_, err = client.CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{})

pkg/lib/crd/storage.go

@@ -0,0 +1,68 @@
+package crd
+
+import (
+	"fmt"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
+	"k8s.io/apimachinery/pkg/runtime"
+)
+
+// SafeStorageVersionUpgrade determines whether the new CRD spec includes all the storage versions of the existing on-cluster CRD.
+// For each stored version in the status of the CRD on the cluster (there will be at least one) - each version must exist in the spec of the new CRD that is being installed.
+// See https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#upgrade-existing-objects-to-a-new-stored-version.
+func SafeStorageVersionUpgrade(existingCRD runtime.Object, newCRD runtime.Object) (bool, error) {
+	newSpecVersions, existingStatusVersions := getStoredVersions(existingCRD, newCRD)
+	if newSpecVersions == nil {
+		return true, fmt.Errorf("could not find any versions in the new CRD")
+	}
+	if existingStatusVersions == nil {
+		// every on-cluster CRD should have at least one stored version in its status
+		// in the case where there are no existing stored versions, checking against new versions is not relevant
+		return true, nil
+	}
+
+	for name := range existingStatusVersions {
+		if _, ok := newSpecVersions[name]; !ok {
+			// a storage version in the status of the old CRD is not present in the spec of the new CRD
+			// potential data loss of CRs without a storage migration - throw error and block the CRD upgrade
+			return false, fmt.Errorf("new CRD removes version %s that is listed as a stored version on the existing CRD", name)
+		}
+	}
+
+	return true, nil
+}
+
+// getStoredVersions returns the storage versions listed in the status of the old on-cluster CRD
+// and all the versions listed in the spec of the new CRD.
+func getStoredVersions(oldCRD runtime.Object, newCRD runtime.Object) (newSpecVersions map[string]struct{}, existingStatusVersions map[string]struct{}) {
+	existingStatusVersions = make(map[string]struct{})
+	newSpecVersions = make(map[string]struct{})
+
+	// find old storage versions by inspect the status field of the existing on-cluster CRD
+	switch crd := oldCRD.(type) {
+	case *apiextensionsv1.CustomResourceDefinition:
+		for _, version := range crd.Status.StoredVersions {
+			existingStatusVersions[version] = struct{}{}
+		}
+	case *apiextensionsv1beta1.CustomResourceDefinition:
+		for _, version := range crd.Status.StoredVersions {
+			existingStatusVersions[version] = struct{}{}
+		}
+	}
+
+	switch crd := newCRD.(type) {
+	case *apiextensionsv1.CustomResourceDefinition:
+		for _, version := range crd.Spec.Versions {
+			newSpecVersions[version.Name] = struct{}{}
+		}
+	case *apiextensionsv1beta1.CustomResourceDefinition:
+		if crd.Spec.Version != "" {
+			newSpecVersions[crd.Spec.Version] = struct{}{}
+		}
+		for _, version := range crd.Spec.Versions {
+			newSpecVersions[version.Name] = struct{}{}
+		}
+	}
+
+	return newSpecVersions, existingStatusVersions
+}

pkg/lib/crd/storage_test.go

@@ -0,0 +1,155 @@
+package crd
+
+import (
+	"github.com/stretchr/testify/require"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"testing"
+)
+
+const crdName = "test"
+
+func TestSafeStorageVersionUpgradeFailure(t *testing.T) {
+	existingCRD := &apiextensionsv1.CustomResourceDefinition{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: crdName,
+		},
+		Spec: apiextensionsv1.CustomResourceDefinitionSpec{
+			Group: "cluster.com",
+			Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
+				{
+					Name:    "v1alpha2",
+					Served:  true,
+					Storage: true,
+					Schema: &apiextensionsv1.CustomResourceValidation{
+						OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
+							Type:        "object",
+							Description: "my crd schema",
+						},
+					},
+				},
+			},
+		},
+		Status: apiextensionsv1.CustomResourceDefinitionStatus{
+			StoredVersions: []string{"v1alpha2"},
+		},
+	}
+
+	newCRD := &apiextensionsv1.CustomResourceDefinition{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: crdName,
+		},
+		Spec: apiextensionsv1.CustomResourceDefinitionSpec{
+			Group: "cluster.com",
+			Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
+				{
+					Name:    "v1alpha3",
+					Served:  true,
+					Storage: true,
+					Schema: &apiextensionsv1.CustomResourceValidation{
+						OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
+							Type:        "object",
+							Description: "my crd schema",
+						},
+					},
+				},
+			},
+		},
+	}
+
+	safe, err := SafeStorageVersionUpgrade(existingCRD, newCRD)
+	// expect safe to be false, since crd upgrade is not safe
+	require.False(t, safe)
+	// expect error, since crd upgrade is not safe
+	require.Error(t, err, "expected error for unsafe CRD upgrade")
+	// error should be related to the storage upgrade removing a version
+	require.Contains(t, err.Error(), "new CRD removes version", "expected storage version error")
+}
+
+func TestSafeStorageVersionUpgradeSuccess(t *testing.T) {
+	existingCRD := &apiextensionsv1.CustomResourceDefinition{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: crdName,
+		},
+		Spec: apiextensionsv1.CustomResourceDefinitionSpec{
+			Group: "cluster.com",
+			Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
+				{
+					Name:    "v1alpha2",
+					Served:  true,
+					Storage: false,
+					Schema: &apiextensionsv1.CustomResourceValidation{
+						OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
+							Type:        "object",
+							Description: "my crd schema",
+						},
+					},
+				},
+				{
+					Name:    "v1alpha3",
+					Served:  true,
+					Storage: true,
+					Schema: &apiextensionsv1.CustomResourceValidation{
+						OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
+							Type:        "object",
+							Description: "my crd schema",
+						},
+					},
+				},
+			},
+		},
+		Status: apiextensionsv1.CustomResourceDefinitionStatus{
+			StoredVersions: []string{"v1alpha2", "v1alpha3"},
+		},
+	}
+
+	newCRD := &apiextensionsv1.CustomResourceDefinition{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: crdName,
+		},
+		Spec: apiextensionsv1.CustomResourceDefinitionSpec{
+			Group: "cluster.com",
+			Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
+				{
+					Name:    "v1alpha2",
+					Served:  true,
+					Storage: false,
+					Schema: &apiextensionsv1.CustomResourceValidation{
+						OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
+							Type:        "object",
+							Description: "my crd schema",
+						},
+					},
+				},
+				{
+					Name:    "v1alpha3",
+					Served:  true,
+					Storage: false,
+					Schema: &apiextensionsv1.CustomResourceValidation{
+						OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
+							Type:        "object",
+							Description: "my crd schema",
+						},
+					},
+				},
+				{
+					Name:    "v1alpha4",
+					Served:  true,
+					Storage: true,
+					Schema: &apiextensionsv1.CustomResourceValidation{
+						OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
+							Type:        "object",
+							Description: "my crd schema",
+						},
+					},
+				},
+			},
+		},
+	}
+
+	safe, err := SafeStorageVersionUpgrade(existingCRD, newCRD)
+	// expect safe to be true, since crd upgrade is safe
+	require.True(t, safe)
+	// expect no error, since crd upgrade is safe
+	require.NoError(t, err, "did not expect error for safe CRD upgrade")
+}