Merge pull request #375 from benluddy/backport-46-1908431

Bug 1915905: Preserve custom catsrc w/ default catsrc name

Author: openshift-merge-robot

pkg/controller/catalogsource/catalogsource_controller.go

@@ -2,14 +2,13 @@ package catalogsource
 
 import (
 	"context"
-	"time"
 
 	olm "github.com/operator-framework/operator-marketplace/pkg/apis/olm/v1alpha1"
+	v1 "github.com/operator-framework/operator-marketplace/pkg/apis/operators/v1"
 	"github.com/operator-framework/operator-marketplace/pkg/controller/options"
 	"github.com/operator-framework/operator-marketplace/pkg/defaults"
 	"github.com/operator-framework/operator-marketplace/pkg/operatorhub"
 	log "github.com/sirupsen/logrus"
-	"k8s.io/apimachinery/pkg/api/errors"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/event"
@@ -89,39 +88,8 @@ type ReconcileCatalogSource struct {
 }
 
 func (r *ReconcileCatalogSource) Reconcile(request reconcile.Request) (reconcile.Result, error) {
-
 	_, defaultCatalogsources := defaults.GetGlobalDefinitions()
-	defaultCatsrcDef := defaultCatalogsources[request.Name]
-
-	if operatorhub.GetSingleton().Get()[defaultCatsrcDef.Name] {
-		log.Info("%s disabled. Not taking any action", defaultCatsrcDef.Name)
-		return reconcile.Result{}, nil
-	}
-	log.Infof("Reconciling default CatalogSource %s", request.Name)
-
-	// Fetch the CatalogSource instance
-	instance := &olm.CatalogSource{}
-	err := r.client.Get(context.TODO(), request.NamespacedName, instance)
-	if err != nil {
-		if errors.IsNotFound(err) {
-			createNewCatsrcInstance(r.client, defaultCatsrcDef)
-			return reconcile.Result{}, nil
-		}
-		// Error reading the object - requeue the request.
-		return reconcile.Result{Requeue: true, RequeueAfter: time.Second * 5}, err
-	}
-
-	if instance.DeletionTimestamp != nil {
-		return reconcile.Result{Requeue: true, RequeueAfter: time.Second * 5}, nil
-	}
-
-	if !defaults.AreCatsrcSpecsEqual(&defaultCatsrcDef.Spec, &instance.Spec) {
-		if err := r.client.Delete(context.TODO(), instance); err != nil {
-			log.Warnf("Could not set default CatalogSource %s's spec back to desired default state. Error in deleting updated CatalogSource: %s", defaultCatsrcDef.GetName(), err.Error())
-		}
-		return reconcile.Result{Requeue: true, RequeueAfter: time.Second * 5}, nil
-	}
-	return reconcile.Result{}, nil
+	return reconcile.Result{}, defaults.New(map[string]v1.OperatorSource{}, defaultCatalogsources, operatorhub.GetSingleton().Get()).Ensure(r.client, request.Name)
 }
 
 func createNewCatsrcInstance(client client.Client, catsrc olm.CatalogSource) error {

pkg/defaults/catsrcHelpers.go

@@ -64,7 +64,9 @@ func processCatsrc(client wrapper.Client, def olm.CatalogSource, disable bool) e
 	}
 
 	if disable {
-		err = ensureCatsrcAbsent(client, def, cluster)
+		if cluster.Annotations[defaultCatsrcAnnotationKey] == defaultCatsrcAnnotationValue {
+			err = ensureCatsrcAbsent(client, def, cluster)
+		}
 	} else {
 		err = ensureCatsrcPresent(client, def, cluster)
 	}
@@ -96,6 +98,12 @@ func ensureCatsrcAbsent(client wrapper.Client, def olm.CatalogSource, cluster *o
 
 // ensureCatsrcPresent ensure that that the default CatalogSource is present on the cluster
 func ensureCatsrcPresent(client wrapper.Client, def olm.CatalogSource, cluster *olm.CatalogSource) error {
+	def = *def.DeepCopy()
+	if def.Annotations == nil {
+		def.Annotations = make(map[string]string)
+	}
+	def.Annotations[defaultCatsrcAnnotationKey] = defaultCatsrcAnnotationValue
+
 	// Create if not present or is deleted
 	if cluster.Name == "" || (!cluster.ObjectMeta.DeletionTimestamp.IsZero() && len(cluster.Finalizers) == 0) {
 		err := client.Create(context.TODO(), &def)
@@ -106,13 +114,17 @@ func ensureCatsrcPresent(client wrapper.Client, def olm.CatalogSource, cluster *
 		return nil
 	}
 
-	if AreCatsrcSpecsEqual(&def.Spec, &cluster.Spec) {
-		logrus.Infof("[defaults] CatalogSource %s default and on cluster specs are same", def.Name)
+	if cluster.Annotations[defaultCatsrcAnnotationKey] == defaultCatsrcAnnotationValue && AreCatsrcSpecsEqual(&def.Spec, &cluster.Spec) {
+		logrus.Infof("[defaults] CatalogSource %s is annotated and its spec is the same as the default spec", def.Name)
 		return nil
 	}
 
 	// Update if the spec has changed
 	cluster.Spec = def.Spec
+	if cluster.Annotations == nil {
+		cluster.Annotations = make(map[string]string)
+	}
+	cluster.Annotations[defaultCatsrcAnnotationKey] = defaultCatsrcAnnotationValue
 	err := client.Update(context.TODO(), cluster)
 	if err != nil {
 		return err

pkg/defaults/defaults.go

@@ -6,7 +6,7 @@ import (
 	"os"
 
 	olm "github.com/operator-framework/operator-marketplace/pkg/apis/olm/v1alpha1"
-	"github.com/operator-framework/operator-marketplace/pkg/apis/operators/v1"
+	v1 "github.com/operator-framework/operator-marketplace/pkg/apis/operators/v1"
 	wrapper "github.com/operator-framework/operator-marketplace/pkg/client"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -38,6 +38,11 @@ var (
 	defaultConfig = make(map[string]bool)
 )
 
+const (
+	defaultCatsrcAnnotationKey   string = "operatorframework.io/managed-by"
+	defaultCatsrcAnnotationValue string = "marketplace-operator"
+)
+
 // Defaults is the interface that can be used to ensure default OperatorSources
 // are always present on the cluster.
 type Defaults interface {

test/helpers/helpers.go

@@ -391,31 +391,6 @@ func WaitForDeploymentScaled(client test.FrameworkClient, name, namespace string
 	return nil
 }
 
-// RestartMarketplace scales the marketplace deployment down to zero and then scales
-// it back up to it's original number of replicas, and waits for a successful deployment.
-func RestartMarketplace(client test.FrameworkClient, namespace string) error {
-	marketplace := &apps.Deployment{}
-	err := client.Get(context.TODO(), types.NamespacedName{Name: "marketplace-operator", Namespace: namespace}, marketplace)
-	if err != nil {
-		return err
-	}
-	initialReplicas := marketplace.Spec.Replicas
-
-	// Scale down deployment
-	err = ScaleMarketplace(client, namespace, int32(0))
-	if err != nil {
-		return err
-	}
-
-	// Now scale it back up
-	ScaleMarketplace(client, namespace, *initialReplicas)
-	if err != nil {
-		return err
-	}
-
-	return nil
-}
-
 // ScaleMarketplace scales the marketplace deployment to the specified replica scale size
 func ScaleMarketplace(client test.FrameworkClient, namespace string, scale int32) error {
 	marketplace := &apps.Deployment{}

test/testsuites/defaultcatsrctests.go

@@ -171,6 +171,9 @@ func testDefaultCatsrcWhileDisabled(t *testing.T) {
 	})
 	require.NoError(t, err, "The update on the custom CatalogSource was reverted back")
 
+	customCatsrc, err = checkForCatsrc("redhat-operators", namespace)
+	require.NoError(t, err, "Custom CatalogSource redhat-operators was removed from the cluster after marketplace was restarted")
+
 	err = toggle(t, 4, false, false) //Re-enable all default CatalogSources
 	require.NoError(t, err, "Could not enable default CatalogSources")
 

test/testsuites/operatorhubtests.go

@@ -243,10 +243,6 @@ func testClusterStatusDefaultsDisabled(t *testing.T) {
 	err = checkClusterOperatorHub(t, 4)
 	assert.NoError(t, err, "Incorrect cluster OperatorHub")
 
-	// Restart marketplace operator
-	err = helpers.RestartMarketplace(test.Global.Client, namespace)
-	require.NoError(t, err, "Could not restart marketplace operator")
-
 	// Check that the ClusterOperator resource has the correct status
 	clusterOperatorName := "marketplace"
 	expectedTypeStatus := map[apiconfigv1.ClusterStatusConditionType]apiconfigv1.ConditionStatus{
@@ -301,10 +297,6 @@ func testSomeClusterStatusDefaultsDisabled(t *testing.T) {
 	err = checkClusterOperatorHub(t, 2)
 	assert.NoError(t, err, "Incorrect cluster OperatorHub")
 
-	// Restart marketplace operator
-	err = helpers.RestartMarketplace(test.Global.Client, namespace)
-	require.NoError(t, err, "Could not restart marketplace operator")
-
 	// Check that the ClusterOperator resource has the correct status
 	clusterOperatorName := "marketplace"
 	expectedTypeStatus := map[apiconfigv1.ClusterStatusConditionType]apiconfigv1.ConditionStatus{