Merge pull request #2083 from ecordell/copied-csv-no-adopt

Do not adopt copied CSVs

Author: openshift-merge-robot

pkg/controller/operators/adoption_controller.go

@@ -187,6 +187,12 @@ func (r *AdoptionReconciler) ReconcileClusterServiceVersion(ctx context.Context,
 		return reconcile.Result{}, err
 	}
 
+	// disown copied csvs - a previous release inadvertently adopted them, this cleans any up if they are adopted
+	if in.IsCopied() {
+		err := r.disownFromAll(ctx, in)
+		return reconcile.Result{}, err
+	}
+
 	// Adopt all resources owned by the CSV if necessary
 	return reconcile.Result{}, r.adoptComponents(ctx, in)
 }
@@ -315,6 +321,31 @@ func (r *AdoptionReconciler) disown(ctx context.Context, operator *decorators.Op
 	return r.Patch(ctx, uCObj, client.MergeFrom(cObj))
 }
 
+func (r *AdoptionReconciler) disownFromAll(ctx context.Context, component runtime.Object) error {
+	cObj, ok := component.(client.Object)
+	if !ok {
+		return fmt.Errorf("Unable to typecast runtime.Object to client.Object")
+	}
+	var operators []decorators.Operator
+	for _, name := range decorators.OperatorNames(cObj.GetLabels()) {
+		o := &operatorsv1.Operator{}
+		o.SetName(name.Name)
+		operator, err := r.factory.NewOperator(o)
+		if err != nil {
+			return err
+		}
+		operators = append(operators, *operator)
+	}
+	errs := make([]error,0)
+	for _, operator := range operators {
+		if err := r.disown(ctx, &operator, component); err != nil {
+			errs = append(errs, err)
+		}
+	}
+
+	return utilerrors.NewAggregate(errs)
+}
+
 func (r *AdoptionReconciler) adoptees(ctx context.Context, operator decorators.Operator, csv *operatorsv1alpha1.ClusterServiceVersion) ([]runtime.Object, error) {
 	// Note: We need to figure out how to dynamically add new list types here (or some equivalent) in
 	// order to support operators composed of custom resources.

pkg/controller/operators/olm/operatorgroup.go

@@ -18,6 +18,7 @@ import (
 
 	"github.com/operator-framework/api/pkg/operators/v1alpha1"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/decorators"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
 	opregistry "github.com/operator-framework/operator-registry/pkg/registry"
@@ -708,15 +709,23 @@ func (a *Operator) copyToNamespace(csv *v1alpha1.ClusterServiceVersion, namespac
 	delete(newCSV.Annotations, v1.OperatorGroupTargetsAnnotationKey)
 
 	fetchedCSV, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(namespace).Get(newCSV.GetName())
-
 	logger = logger.WithField("csv", csv.GetName())
 	if fetchedCSV != nil {
 		logger.Debug("checking annotations")
 
-		if !reflect.DeepEqual(a.copyOperatorGroupAnnotations(&fetchedCSV.ObjectMeta), a.copyOperatorGroupAnnotations(&newCSV.ObjectMeta)) {
+		if !reflect.DeepEqual(a.copyOperatorGroupAnnotations(&fetchedCSV.ObjectMeta), a.copyOperatorGroupAnnotations(&newCSV.ObjectMeta)) ||
+			len(decorators.OperatorNames(fetchedCSV.GetLabels())) > 0 {
 			// TODO: only copy over the opgroup annotations, not _all_ annotations
 			fetchedCSV.Annotations = newCSV.Annotations
 			fetchedCSV.SetLabels(utillabels.AddLabel(fetchedCSV.GetLabels(), v1alpha1.CopiedLabelKey, csv.GetNamespace()))
+
+			// remove Operator object component labels before copying so that copied CSVs do not show up in the component list
+			for k := range fetchedCSV.GetLabels() {
+				if strings.HasPrefix(k, decorators.ComponentLabelKeyPrefix) {
+					delete(fetchedCSV.Labels, k)
+				}
+			}
+
 			// CRs don't support strategic merge patching, but in the future if they do this should be updated to patch
 			logger.Debug("updating target CSV")
 			if fetchedCSV, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(namespace).Update(context.TODO(), fetchedCSV, metav1.UpdateOptions{}); err != nil {

pkg/controller/operators/olm/operatorgroup_test.go

@@ -121,6 +121,65 @@ func TestCopyToNamespace(t *testing.T) {
 				},
 			},
 		},
+		{
+			Name:      "component labels are stripped before copy",
+			Namespace: "bar",
+			Original: &v1alpha1.ClusterServiceVersion{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "name",
+					Namespace: "foo",
+					Labels: map[string]string{
+						"operators.coreos.com/foo": "",
+						"operators.coreos.com/bar": "",
+						"untouched": "fine",
+					},
+				},
+			},
+			ExistingCopy: &v1alpha1.ClusterServiceVersion{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "name",
+					Namespace: "bar",
+					Labels: map[string]string{
+						"operators.coreos.com/foo": "",
+						"operators.coreos.com/bar": "",
+						"untouched": "fine",
+					},
+				},
+				Status: v1alpha1.ClusterServiceVersionStatus{
+					Message:        "The operator is running in foo but is managing this namespace",
+					Reason:         v1alpha1.CSVReasonCopied},
+			},
+			ExpectedResult: &v1alpha1.ClusterServiceVersion{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "name",
+					Namespace: "bar",
+					Labels: map[string]string{
+						"untouched": "fine",
+						"olm.copiedFrom": "foo",
+					},
+				},
+				Status: v1alpha1.ClusterServiceVersionStatus{
+					Message:        "The operator is running in foo but is managing this namespace",
+					Reason:         v1alpha1.CSVReasonCopied,
+				},
+			},
+			ExpectedActions: []ktesting.Action{
+				ktesting.NewUpdateAction(gvr, "bar", &v1alpha1.ClusterServiceVersion{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "name",
+						Namespace: "bar",
+						Labels: map[string]string{
+							"untouched": "fine",
+							"olm.copiedFrom": "foo",
+						},
+					},
+					Status: v1alpha1.ClusterServiceVersionStatus{
+						Message:        "The operator is running in foo but is managing this namespace",
+						Reason:         v1alpha1.CSVReasonCopied,
+					},
+				}),
+			},
+		},
 	} {
 		t.Run(tc.Name, func(t *testing.T) {
 			lister := &operatorlisterfakes.FakeOperatorLister{}

pkg/controller/operators/operatorcondition_controller_test.go

@@ -250,7 +250,7 @@ var _ = Describe("OperatorCondition", func() {
 					}
 					Expect(k8sClient.Create(ctx, operatorCondition)).To(Succeed())
 				})
-				It("does not injected the OperatorCondition name into the deployment's Environment Variables", func() {
+				It("does not inject the OperatorCondition name into the deployment's Environment Variables", func() {
 					deployment := &appsv1.Deployment{}
 					Consistently(func() error {
 						err := k8sClient.Get(ctx, types.NamespacedName{Name: operatorCondition.Spec.Deployments[0], Namespace: namespace.GetName()}, deployment)

test/e2e/operator_test.go

@@ -28,7 +28,7 @@ import (
 )
 
 // Describes test specs for the Operator resource.
-var _ = Describe("Operator", func() {
+var _ = Describe("Operator API", func() {
 	var (
 		clientCtx       context.Context
 		scheme          *runtime.Scheme
@@ -74,6 +74,8 @@ var _ = Describe("Operator", func() {
 		o := &operatorsv1.Operator{}
 		o.SetName(genName("o-"))
 
+		Consistently(o).ShouldNot(ContainCopiedCSVReferences())
+
 		Eventually(func() error {
 			return client.Create(clientCtx, o)
 		}).Should(Succeed())
@@ -327,6 +329,12 @@ var _ = Describe("Operator", func() {
 		})
 
 		It("should automatically adopt components", func() {
+			Consistently(func() (*operatorsv1.Operator, error) {
+				o := &operatorsv1.Operator{}
+				err := client.Get(clientCtx, operatorName, o)
+				return o, err
+			}).ShouldNot(ContainCopiedCSVReferences())
+
 			Eventually(func() (*operatorsv1.Operator, error) {
 				o := &operatorsv1.Operator{}
 				err := client.Get(clientCtx, operatorName, o)
@@ -346,8 +354,36 @@ var _ = Describe("Operator", func() {
 				getReference(scheme, testobj.WithName("monitoringdashboards.monitoring.kiali.io", &apiextensionsv1.CustomResourceDefinition{})),
 			}))
 		})
-	})
 
+		Context("when a namespace is added", func() {
+			var newNs  *corev1.Namespace
+
+			BeforeEach(func() {
+				// Subscribe to a package and await a successful install
+				newNs = &corev1.Namespace{}
+				newNs.SetName(genName("ns-"))
+				Eventually(func() error {
+					return client.Create(clientCtx, newNs)
+				}).Should(Succeed())
+			})
+			AfterEach(func() {
+				Eventually(func() error {
+					err := client.Delete(clientCtx, newNs)
+					if apierrors.IsNotFound(err) {
+						return nil
+					}
+					return err
+				}).Should(Succeed())
+			})
+			It("should not adopt copied csvs", func() {
+				Consistently(func() (*operatorsv1.Operator, error) {
+					o := &operatorsv1.Operator{}
+					err := client.Get(clientCtx, operatorName, o)
+					return o, err
+				}).ShouldNot(ContainCopiedCSVReferences())
+			})
+		})
+	})
 })
 
 func getReference(scheme *runtime.Scheme, obj runtime.Object) *corev1.ObjectReference {
@@ -380,6 +416,53 @@ func componentRefEventuallyExists(w watch.Interface, exists bool, ref *corev1.Ob
 	}))
 }
 
+func ContainCopiedCSVReferences() gomegatypes.GomegaMatcher {
+	return &copiedCSVRefMatcher{}
+}
+
+type copiedCSVRefMatcher struct {
+}
+
+func (matcher *copiedCSVRefMatcher) Match(actual interface{}) (success bool, err error) {
+	if actual == nil {
+		return false, nil
+	}
+	operator, ok := actual.(*operatorsv1.Operator)
+	if !ok {
+		return false, fmt.Errorf("copiedCSVRefMatcher matcher expects an *Operator")
+	}
+	if operator.Status.Components == nil {
+		return false, nil
+	}
+	for _, ref := range operator.Status.Components.Refs {
+		if ref.Kind != operatorsv1alpha1.ClusterServiceVersionKind {
+			continue
+		}
+		for _, c := range ref.Conditions {
+			if c.Reason == string(operatorsv1alpha1.CSVReasonCopied) {
+				return true, nil
+			}
+		}
+	}
+	return false, nil
+}
+
+func (matcher *copiedCSVRefMatcher) FailureMessage(actual interface{}) (message string) {
+	operator, ok := actual.(*operatorsv1.Operator)
+	if !ok {
+		return fmt.Sprintf("copiedCSVRefMatcher matcher expects an *Operator")
+	}
+	return fmt.Sprintf("Expected\n\t%#v\nto contain copied CSVs in components\n\t%#v\n", operator, operator.Status.Components)
+}
+
+func (matcher *copiedCSVRefMatcher) NegatedFailureMessage(actual interface{}) (message string) {
+	operator, ok := actual.(*operatorsv1.Operator)
+	if !ok {
+		return fmt.Sprintf("copiedCSVRefMatcher matcher expects an *Operator")
+	}
+	return fmt.Sprintf("Expected\n\t%#v\nto not contain copied CSVs in components\n\t%#v\n", operator, operator.Status.Components)
+}
+
 func operatorPredicate(fn func(*operatorsv1.Operator) bool) predicateFunc {
 	return func(event watch.Event) bool {
 		o, ok := event.Object.(*operatorsv1.Operator)