Add extra check for v1alpha CRD

The operator should print a human friendly message when v1alpha1 CRDs are detected. Such cluster can't support OCP v1beta1 version of snapshots and users should know why.
openshift · Jun 9, 2020 · 973f1d6 · 973f1d6
1 parent 5a54714
commit 973f1d6
Show file tree

Hide file tree

Showing 3 changed files with 179 additions and 3 deletions.
diff --git a/pkg/operator/operator.go b/pkg/operator/operator.go
@@ -1,6 +1,7 @@
 package operator
 
 import (
+	"errors"
 	"fmt"
 	"time"
 
@@ -177,11 +178,16 @@ func (c *csiSnapshotOperator) sync() error {
 
 func (c *csiSnapshotOperator) updateSyncError(status *operatorv1.OperatorStatus, err error) {
 	if err != nil {
+		degradedReason := "OperatorSync"
+		var errAlpha *AlphaCRDError
+		if errors.As(err, &errAlpha) {
+			degradedReason = "AlphaCRDsExist"
+		}
 		v1helpers.SetOperatorCondition(&status.Conditions,
 			operatorv1.OperatorCondition{
 				Type:    operatorv1.OperatorStatusTypeDegraded,
 				Status:  operatorv1.ConditionTrue,
-				Reason:  "OperatorSync",
+				Reason:  degradedReason,
 				Message: err.Error(),
 			})
 	} else {
@@ -195,7 +201,8 @@ func (c *csiSnapshotOperator) updateSyncError(status *operatorv1.OperatorStatus,
 
 func (c *csiSnapshotOperator) handleSync(instance *operatorv1.CSISnapshotController) error {
 	if err := c.syncCustomResourceDefinitions(); err != nil {
-		return fmt.Errorf("failed to sync CRDs: %s", err)
+		// Pass through AlphaCRDError via %w
+		return fmt.Errorf("failed to sync CRDs: %w", err)
 	}
 
 	deployment, err := c.syncDeployment(instance)

diff --git a/pkg/operator/operator_test.go b/pkg/operator/operator_test.go
@@ -87,7 +87,7 @@ func newOperator(test operatorTest) *testContext {
 	extAPIInformerFactory := apiextinformers.NewSharedInformerFactory(extAPIClient, 0)
 	// Fill the informer
 	for i := range test.initialObjects.crds {
-		extAPIInformerFactory.Apiextensions().V1().CustomResourceDefinitions().Informer().GetIndexer().Add(test.initialObjects.crds[i])
+		extAPIInformerFactory.Apiextensions().V1beta1().CustomResourceDefinitions().Informer().GetIndexer().Add(test.initialObjects.crds[i])
 	}
 	if test.reactors.crds != nil {
 		test.reactors.crds(extAPIClient, extAPIInformerFactory)
@@ -317,6 +317,86 @@ func withEstablishedConditions(instance *apiextv1beta1.CustomResourceDefinition)
 	return instance
 }
 
+func getAlphaCRD(crdName string) *apiextv1beta1.CustomResourceDefinition {
+	var crdFile string
+	switch crdName {
+	case "VolumeSnapshot":
+		crdFile = `
+apiVersion: apiextensions.k8s.io/v1beta1
+kind: CustomResourceDefinition
+metadata:
+    name: volumesnapshots.snapshot.storage.k8s.io
+spec:
+    conversion:
+      strategy: None
+    group: snapshot.storage.k8s.io
+    names:
+      kind: VolumeSnapshot
+      listKind: VolumeSnapshotList
+      plural: volumesnapshots
+      singular: volumesnapshot
+    preserveUnknownFields: true
+    scope: Namespaced
+    versions:
+    - name: v1alpha1
+      served: true
+      storage: true
+    subresources:
+      status: {}
+`
+
+	case "VolumeSnapshotContent":
+		crdFile = `
+apiVersion: apiextensions.k8s.io/v1beta1
+kind: CustomResourceDefinition
+metadata:
+    name: volumesnapshotcontents.snapshot.storage.k8s.io
+spec:
+    conversion:
+      strategy: None
+    group: snapshot.storage.k8s.io
+    names:
+      kind: VolumeSnapshotContent
+      listKind: VolumeSnapshotContentList
+      plural: volumesnapshotcontents
+      singular: volumesnapshotcontent
+    preserveUnknownFields: true
+    scope: Cluster
+    versions:
+    - name: v1alpha1
+      served: true
+      storage: true
+`
+	case "VolumeSnapshotClass":
+		crdFile = `
+apiVersion: apiextensions.k8s.io/v1beta1
+kind: CustomResourceDefinition
+metadata:
+    name: volumesnapshotclasses.snapshot.storage.k8s.io
+spec:
+    conversion:
+      strategy: None
+    group: snapshot.storage.k8s.io
+    names:
+      kind: VolumeSnapshotClass
+      listKind: VolumeSnapshotClassList
+      plural: volumesnapshotclasses
+      singular: volumesnapshotclass
+    preserveUnknownFields: true
+    scope: Cluster
+    versions:
+    - name: v1alpha1
+      served: true
+      storage: true
+    subresources:
+      status:
+`
+	default:
+		panic(crdName)
+	}
+	return resourceread.ReadCustomResourceDefinitionV1Beta1OrDie([]byte(crdFile))
+}
+
 // Optional reactor that sets Established condition to True. It's needed by the operator that polls for CRDs until they get the condition
 func addCRDEstablishedRector(client *fakeextapi.Clientset, informer apiextinformers.SharedInformerFactory) {
 	client.PrependReactor("*", "customresourcedefinitions", func(action core.Action) (handled bool, ret runtime.Object, err error) {
@@ -536,6 +616,58 @@ func TestSync(t *testing.T) {
 			},
 			expectErr: true,
 		},
+		{
+			// error: v1alpha1 VolumeSnapshot already exists
+			name:  "v1alpha1 VolumeSnapshot",
+			image: defaultImage,
+			initialObjects: testObjects{
+				csiSnapshotController: csiSnapshotController(),
+				crds:                  []*apiextv1beta1.CustomResourceDefinition{getAlphaCRD("VolumeSnapshot")},
+			},
+			expectedObjects: testObjects{
+				crds:                  []*apiextv1beta1.CustomResourceDefinition{getAlphaCRD("VolumeSnapshot")},
+				csiSnapshotController: csiSnapshotController(withTrueConditions(opv1.OperatorStatusTypeDegraded)),
+			},
+			expectErr: true,
+			reactors: testReactors{
+				crds: addCRDEstablishedRector,
+			},
+		},
+		{
+			// error: v1alpha1 VolumeSnapshotContent already exists
+			name:  "v1alpha1 VolumeSnapshotContent",
+			image: defaultImage,
+			initialObjects: testObjects{
+				csiSnapshotController: csiSnapshotController(),
+				crds:                  []*apiextv1beta1.CustomResourceDefinition{getAlphaCRD("VolumeSnapshotContent")},
+			},
+			expectedObjects: testObjects{
+				crds:                  []*apiextv1beta1.CustomResourceDefinition{getAlphaCRD("VolumeSnapshotContent")},
+				csiSnapshotController: csiSnapshotController(withTrueConditions(opv1.OperatorStatusTypeDegraded)),
+			},
+			expectErr: true,
+			reactors: testReactors{
+				crds: addCRDEstablishedRector,
+			},
+		},
+		{
+			// error: v1alpha1 VolumeSnapshotClass already exists
+			name:  "v1alpha1 VolumeSnapshotClass",
+			image: defaultImage,
+			initialObjects: testObjects{
+				csiSnapshotController: csiSnapshotController(),
+				crds:                  []*apiextv1beta1.CustomResourceDefinition{getAlphaCRD("VolumeSnapshotClass")},
+			},
+			expectedObjects: testObjects{
+				crds:                  []*apiextv1beta1.CustomResourceDefinition{getAlphaCRD("VolumeSnapshotClass")},
+				csiSnapshotController: csiSnapshotController(withTrueConditions(opv1.OperatorStatusTypeDegraded)),
+			},
+			expectErr: true,
+			reactors: testReactors{
+				crds: addCRDEstablishedRector,
+			},
+		},
+
 		// TODO: more error cases? Deployment creation fails and things like that?
 	}
 

diff --git a/pkg/operator/sync.go b/pkg/operator/sync.go
@@ -7,6 +7,7 @@ import (
 
 	appsv1 "k8s.io/api/apps/v1"
 	apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/util/wait"
 
 	operatorv1 "github.com/openshift/api/operator/v1"
@@ -29,7 +30,18 @@ var (
 	customResourceReadyTimeout  = 10 * time.Minute
 )
 
+type AlphaCRDError struct {
+	alphaCRDs []string
+}
+
+func (a *AlphaCRDError) Error() string {
+	return fmt.Sprintf("cluster-csi-snapshot-controller-operator does not support v1alpha1 version of snapshot CRDs %s installed by user or 3rd party controller", strings.Join(a.alphaCRDs, ", "))
+}
+
 func (c *csiSnapshotOperator) syncCustomResourceDefinitions() error {
+	if err := c.checkAlphaCRDs(); err != nil {
+		return err
+	}
 	for _, file := range crds {
 		crd := resourceread.ReadCustomResourceDefinitionV1Beta1OrDie(generated.MustAsset(file))
 		_, updated, err := resourceapply.ApplyCustomResourceDefinition(c.crdClient.ApiextensionsV1beta1(), c.eventRecorder, crd)
@@ -70,6 +82,31 @@ func (c *csiSnapshotOperator) waitForCustomResourceDefinition(resource *apiextv1
 	return nil
 }
 
+// checkCRDAlpha checks if v1alpha1 version of the CRD exists and returns human-friendly error if so.
+// This happens during update from 4.3 cluster that may use v1alpha1 CRD version installed by cluster admin.
+func (c *csiSnapshotOperator) checkAlphaCRDs() error {
+	var alphas []string
+	for _, file := range crds {
+		crd := resourceread.ReadCustomResourceDefinitionV1Beta1OrDie(generated.MustAsset(file))
+		oldCRD, err := c.crdLister.Get(crd.Name)
+		if err != nil {
+			if errors.IsNotFound(err) {
+				continue
+			}
+			return fmt.Errorf("error getting CustomResourceDefinition %s: %v", crd.Name, err)
+		}
+		for _, version := range oldCRD.Spec.Versions {
+			if version.Name == "v1alpha1" {
+				alphas = append(alphas, oldCRD.Name)
+			}
+		}
+	}
+	if len(alphas) != 0 {
+		return &AlphaCRDError{alphas}
+	}
+	return nil
+}
+
 func (c *csiSnapshotOperator) syncDeployment(instance *operatorv1.CSISnapshotController) (*appsv1.Deployment, error) {
 	deploy := c.getExpectedDeployment(instance)