Designate CDI as CDIConfig authority (#1516)

* Formally designate CDI as owner of CDIConfig by adding annotation cdi.kubevirt.io/configAuthority

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

* More robust upgrade handling.  No error if beta api not installed yet.

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

pkg/controller/config-controller.go

@@ -31,6 +31,11 @@ import (
 	"kubevirt.io/containerized-data-importer/pkg/util"
 )
 
+// AnnConfigAuthority is the annotation specifying a resource as the CDIConfig authority
+const (
+	AnnConfigAuthority = "cdi.kubevirt.io/configAuthority"
+)
+
 // CDIConfigReconciler members
 type CDIConfigReconciler struct {
 	client client.Client
@@ -105,6 +110,10 @@ func (r *CDIConfigReconciler) setOperatorParams(config *cdiv1.CDIConfig) error {
 		return nil
 	}
 
+	if _, ok := cdiCR.Annotations[AnnConfigAuthority]; !ok {
+		return nil
+	}
+
 	if cdiCR.Spec.Config == nil {
 		config.Spec = cdiv1.CDIConfigSpec{}
 	} else {

pkg/controller/config-controller_test.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 
 	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
 
 	routev1 "github.com/openshift/api/route/v1"
@@ -64,7 +65,7 @@ var _ = Describe("CDIConfig Controller reconcile loop", func() {
 		Expect(err).ToNot(HaveOccurred())
 	})
 
-	It("Should set proxyURL to override if no ingress or route exists", func() {
+	DescribeTable("Should set proxyURL to override if no ingress or route exists", func(authority bool) {
 		reconciler, cdiConfig := createConfigReconciler(createConfigMap(operator.ConfigMapName, testNamespace))
 		_, err := reconciler.Reconcile(reconcile.Request{})
 		err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: reconciler.configName}, cdiConfig)
@@ -75,16 +76,26 @@ var _ = Describe("CDIConfig Controller reconcile loop", func() {
 		cdi.Spec.Config = &cdiv1.CDIConfigSpec{
 			UploadProxyURLOverride: &override,
 		}
+		if !authority {
+			delete(cdi.Annotations, "cdi.kubevirt.io/configAuthority")
+		}
 		err = reconciler.client.Update(context.TODO(), cdi)
 		Expect(err).ToNot(HaveOccurred())
 		_, err = reconciler.Reconcile(reconcile.Request{})
 		Expect(err).ToNot(HaveOccurred())
 		err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: reconciler.configName}, cdiConfig)
 		Expect(err).ToNot(HaveOccurred())
-		Expect(override).To(Equal(*cdiConfig.Status.UploadProxyURL))
+		if authority {
-	})
+			Expect(override).To(Equal(*cdiConfig.Status.UploadProxyURL))
+		} else {
+			Expect(cdiConfig.Status.UploadProxyURL).To(BeNil())
+		}
+	},
+		Entry("as authority", true),
+		Entry("not authority", false),
+	)
 
-	It("Should set proxyURL to override if ingress or route exists", func() {
+	DescribeTable("Should set proxyURL to override if ingress or route exists", func(authority bool) {
 		reconciler, cdiConfig := createConfigReconciler(createConfigMap(operator.ConfigMapName, testNamespace),
 			createIngressList(
 				*createIngress("test-ingress", "test-ns", testServiceName, testURL),
@@ -99,14 +110,25 @@ var _ = Describe("CDIConfig Controller reconcile loop", func() {
 		cdi.Spec.Config = &cdiv1.CDIConfigSpec{
 			UploadProxyURLOverride: &override,
 		}
+		if !authority {
+			delete(cdi.Annotations, "cdi.kubevirt.io/configAuthority")
+		}
 		err = reconciler.client.Update(context.TODO(), cdi)
 		Expect(err).ToNot(HaveOccurred())
 		_, err = reconciler.Reconcile(reconcile.Request{})
 		Expect(err).ToNot(HaveOccurred())
 		err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: reconciler.configName}, cdiConfig)
 		Expect(err).ToNot(HaveOccurred())
-		Expect(override).To(Equal(*cdiConfig.Status.UploadProxyURL))
+		if authority {
-	})
+			Expect(override).To(Equal(*cdiConfig.Status.UploadProxyURL))
+		} else {
+			Expect(cdiConfig.Status.UploadProxyURL).ToNot(BeNil())
+			Expect(override).ToNot(Equal(*cdiConfig.Status.UploadProxyURL))
+		}
+	},
+		Entry("as authority", true),
+		Entry("not authority", false),
+	)
 })
 
 var _ = Describe("Controller ingress reconcile loop", func() {
@@ -636,6 +658,9 @@ func createConfigReconciler(objects ...runtime.Object) (*CDIConfigReconciler, *c
 	cdi := &cdiv1.CDI{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "cdi",
+			Annotations: map[string]string{
+				"cdi.kubevirt.io/configAuthority": "",
+			},
 		},
 	}
 
@@ -646,12 +671,13 @@ func createConfigReconciler(objects ...runtime.Object) (*CDIConfigReconciler, *c
 
 	// Create a ReconcileMemcached object with the scheme and fake client.
 	r := &CDIConfigReconciler{
-		client:         cl,
+		client:                 cl,
-		uncachedClient: cl,
+		uncachedClient:         cl,
-		scheme:         s,
+		scheme:                 s,
-		log:            configLog,
+		log:                    configLog,
-		configName:     "cdiconfig",
+		configName:             "cdiconfig",
-		cdiNamespace:   testNamespace,
+		cdiNamespace:           testNamespace,
+		uploadProxyServiceName: testServiceName,
 	}
 	return r, cdiConfig
 }

pkg/operator/controller/BUILD.bazel

@@ -18,6 +18,7 @@ go_library(
     deps = [
         "//pkg/apis/core/v1beta1:go_default_library",
         "//pkg/common:go_default_library",
+        "//pkg/controller:go_default_library",
         "//pkg/operator:go_default_library",
         "//pkg/operator/resources/cert:go_default_library",
         "//pkg/operator/resources/cluster:go_default_library",

pkg/operator/controller/callbacks.go

@@ -21,21 +21,22 @@ import (
 	"fmt"
 	"reflect"
 
-	"kubevirt.io/controller-lifecycle-operator-sdk/pkg/sdk/callbacks"
-
-	extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
-
 	"github.com/go-logr/logr"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	sdk "kubevirt.io/controller-lifecycle-operator-sdk/pkg/sdk"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
+	cdiv1 "kubevirt.io/containerized-data-importer/pkg/apis/core/v1beta1"
 	"kubevirt.io/containerized-data-importer/pkg/common"
+	cdicontroller "kubevirt.io/containerized-data-importer/pkg/controller"
+	"kubevirt.io/controller-lifecycle-operator-sdk/pkg/sdk/callbacks"
 )
 
 func addReconcileCallbacks(r *ReconcileCDI) {
@@ -46,7 +47,7 @@ func addReconcileCallbacks(r *ReconcileCDI) {
 	r.reconciler.AddCallback(&appsv1.Deployment{}, reconcileCreateRoute)
 	r.reconciler.AddCallback(&appsv1.Deployment{}, reconcileDeleteSecrets)
 	r.reconciler.AddCallback(&extv1.CustomResourceDefinition{}, reconcileInitializeCRD)
-
+	r.reconciler.AddCallback(&extv1.CustomResourceDefinition{}, reconcileSetConfigAuthority)
 }
 
 func isControllerDeployment(d *appsv1.Deployment) bool {
@@ -173,3 +174,51 @@ func deleteWorkerResources(l logr.Logger, c client.Client) error {
 
 	return nil
 }
+
+func reconcileSetConfigAuthority(args *callbacks.ReconcileCallbackArgs) error {
+	if args.State != callbacks.ReconcileStatePostRead {
+		return nil
+	}
+
+	crd := args.CurrentObject.(*extv1.CustomResourceDefinition)
+	if crd.Name != "cdiconfigs.cdi.kubevirt.io" {
+		return nil
+	}
+
+	cdi, ok := args.Resource.(*cdiv1.CDI)
+	if !ok {
+		return nil
+	}
+
+	if _, ok = cdi.Annotations[cdicontroller.AnnConfigAuthority]; ok {
+		return nil
+	}
+
+	if cdi.Spec.Config == nil {
+		cl := &cdiv1.CDIConfigList{}
+		err := args.Client.List(context.TODO(), cl)
+		if err != nil {
+			if meta.IsNoMatchError(err) {
+				return nil
+			}
+
+			return err
+		}
+
+		if len(cl.Items) != 1 {
+			return nil
+		}
+
+		cs := cl.Items[0].Spec.DeepCopy()
+		if !reflect.DeepEqual(cs, &cdiv1.CDIConfigSpec{}) {
+			cdi.Spec.Config = cs
+		}
+	}
+
+	if cdi.Annotations == nil {
+		cdi.Annotations = map[string]string{}
+	}
+	cdi.Annotations[cdicontroller.AnnConfigAuthority] = ""
+
+	return args.Client.Update(context.TODO(), cdi)
+}

pkg/operator/controller/controller_test.go

@@ -316,6 +316,53 @@ var _ = Describe("Controller", func() {
 				validateEvents(args.reconciler, createReadyEventValidationMap())
 			})
 
+			It("should set config authority", func() {
+				args := createArgs()
+				doReconcile(args)
+
+				cfg := &cdiv1.CDIConfig{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "config",
+					},
+				}
+
+				err := args.client.Create(context.TODO(), cfg)
+				Expect(err).ToNot(HaveOccurred())
+
+				Expect(setDeploymentsReady(args)).To(BeTrue())
+
+				cdi, err := getCDI(args.client, args.cdi)
+				Expect(err).ToNot(HaveOccurred())
+				_, ok := cdi.Annotations["cdi.kubevirt.io/configAuthority"]
+				Expect(ok).To(BeTrue())
+				Expect(cdi.Spec.Config).To(BeNil())
+			})
+
+			It("should set config authority (existing values)", func() {
+				args := createArgs()
+				doReconcile(args)
+
+				cfg := &cdiv1.CDIConfig{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "config",
+					},
+					Spec: cdiv1.CDIConfigSpec{
+						FeatureGates: []string{"foobar"},
+					},
+				}
+
+				err := args.client.Create(context.TODO(), cfg)
+				Expect(err).ToNot(HaveOccurred())
+
+				Expect(setDeploymentsReady(args)).To(BeTrue())
+
+				cdi, err := getCDI(args.client, args.cdi)
+				Expect(err).ToNot(HaveOccurred())
+				_, ok := cdi.Annotations["cdi.kubevirt.io/configAuthority"]
+				Expect(ok).To(BeTrue())
+				Expect(cdi.Spec.Config).To(Equal(&cfg.Spec))
+			})
+
 			It("can become become ready, un-ready, and ready again", func() {
 				var deployment *appsv1.Deployment
 

tests/operator_test.go

@@ -99,6 +99,17 @@ var _ = Describe("Operator tests", func() {
 		Expect(conditionMap[conditions.ConditionProgressing]).To(Equal(corev1.ConditionFalse))
 		Expect(conditionMap[conditions.ConditionDegraded]).To(Equal(corev1.ConditionFalse))
 	})
+
+	It("should make CDI config authority", func() {
+		Eventually(func() bool {
+			cdiObjects, err := f.CdiClient.CdiV1beta1().CDIs().List(context.TODO(), metav1.ListOptions{})
+			Expect(err).ToNot(HaveOccurred())
+			Expect(len(cdiObjects.Items)).To(Equal(1))
+			cdiObject := cdiObjects.Items[0]
+			_, ok := cdiObject.Annotations["cdi.kubevirt.io/configAuthority"]
+			return ok
+		}, 1*time.Minute, 2*time.Second).Should(BeTrue())
+	})
 })
 
 var _ = Describe("Tests needing the restore of nodes", func() {