Repository: operator-framework/operator-lifecycle-manager
Pull request #2088 (2021-04-08 16:32:05)

[release-4.7] Bug 1949139: Preserve existing ServiceAccount owner references during installs.
This is an automated cherry-pick of #2076

/assign benluddy


Patch:
diff --git a/pkg/controller/operators/catalog/step_ensurer.go b/pkg/controller/operators/catalog/step_ensurer.go
index 0eb61e0c6..6ce7e505d 100644
--- a/pkg/controller/operators/catalog/step_ensurer.go
+++ b/pkg/controller/operators/catalog/step_ensurer.go
@@ -156,6 +156,7 @@ func (o *StepEnsurer) EnsureServiceAccount(namespace string, sa *corev1.ServiceA
 		return
 	}
 	sa.Secrets = preSa.Secrets
+	sa.OwnerReferences = mergedOwnerReferences(preSa.OwnerReferences, sa.OwnerReferences)
 
 	sa.SetNamespace(namespace)
 
@@ -349,3 +350,20 @@ func (o *StepEnsurer) EnsureConfigMap(namespace string, configmap *corev1.Config
 	status = v1alpha1.StepStatusPresent
 	return
 }
+
+func mergedOwnerReferences(in ...[]metav1.OwnerReference) []metav1.OwnerReference {
+	uniques := make(map[metav1.OwnerReference]struct{})
+	for _, refs := range in {
+		for _, ref := range refs {
+			uniques[ref] = struct{}{}
+		}
+	}
+	if len(uniques) == 0 {
+		return nil
+	}
+	merged := make([]metav1.OwnerReference, 0, len(uniques))
+	for ref := range uniques {
+		merged = append(merged, ref)
+	}
+	return merged
+}
diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go
index e9ca3061c..9f74485ad 100644
--- a/pkg/controller/operators/olm/operator.go
+++ b/pkg/controller/operators/olm/operator.go
@@ -1514,7 +1514,7 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 		// Check if we're not ready to install part of the replacement chain yet
 		if prev := a.isReplacing(out); prev != nil {
 			if prev.Status.Phase != v1alpha1.CSVPhaseReplacing {
-				logger.WithError(fmt.Errorf("CSV being replaced is in phase %s instead of %s", prev.Status.Phase, v1alpha1.CSVPhaseReplacing)).Debug("Unable to replace previous CSV")
+				logger.WithError(fmt.Errorf("CSV being replaced is in phase %s instead of %s", prev.Status.Phase, v1alpha1.CSVPhaseReplacing)).Warn("Unable to replace previous CSV")
 				return
 			}
 		}


Test patch:
diff --git a/pkg/controller/operators/catalog/step_ensurer_test.go b/pkg/controller/operators/catalog/step_ensurer_test.go
new file mode 100644
index 000000000..a62bab92c
--- /dev/null
+++ b/pkg/controller/operators/catalog/step_ensurer_test.go
@@ -0,0 +1,190 @@
+package catalog
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestMergedOwnerReferences(t *testing.T) {
+	var (
+		True  bool = true
+		False bool = false
+	)
+
+	for _, tc := range []struct {
+		Name string
+		In   [][]metav1.OwnerReference
+		Out  []metav1.OwnerReference
+	}{
+		{
+			Name: "empty",
+		},
+		{
+			Name: "different uid",
+			In: [][]metav1.OwnerReference{
+				{
+					{
+						APIVersion:         "a",
+						Kind:               "b",
+						Name:               "c",
+						Controller:         &True,
+						BlockOwnerDeletion: &True,
+						UID:                "x",
+					},
+					{
+						APIVersion:         "a",
+						Kind:               "b",
+						Name:               "c",
+						Controller:         &True,
+						BlockOwnerDeletion: &True,
+						UID:                "y",
+					},
+				},
+			},
+			Out: []metav1.OwnerReference{
+				{
+					APIVersion:         "a",
+					Kind:               "b",
+					Name:               "c",
+					Controller:         &True,
+					BlockOwnerDeletion: &True,
+					UID:                "x",
+				},
+				{
+					APIVersion:         "a",
+					Kind:               "b",
+					Name:               "c",
+					Controller:         &True,
+					BlockOwnerDeletion: &True,
+					UID:                "y",
+				},
+			},
+		},
+		{
+			Name: "different controller",
+			In: [][]metav1.OwnerReference{
+				{
+					{
+						APIVersion:         "a",
+						Kind:               "b",
+						Name:               "c",
+						Controller:         &True,
+						BlockOwnerDeletion: &True,
+						UID:                "x",
+					},
+					{
+						APIVersion:         "a",
+						Kind:               "b",
+						Name:               "c",
+						Controller:         &False,
+						BlockOwnerDeletion: &True,
+						UID:                "x",
+					},
+				},
+			},
+			Out: []metav1.OwnerReference{
+				{
+					APIVersion:         "a",
+					Kind:               "b",
+					Name:               "c",
+					Controller:         &True,
+					BlockOwnerDeletion: &True,
+					UID:                "x",
+				},
+				{
+					APIVersion:         "a",
+					Kind:               "b",
+					Name:               "c",
+					Controller:         &False,
+					BlockOwnerDeletion: &True,
+					UID:                "x",
+				},
+			},
+		},
+		{
+			Name: "add owner without uid",
+			In: [][]metav1.OwnerReference{
+				{
+					{
+						APIVersion:         "a",
+						Kind:               "b",
+						Name:               "c-1",
+						Controller:         &False,
+						BlockOwnerDeletion: &False,
+						UID:                "x",
+					},
+				},
+				{
+					{
+						APIVersion:         "a",
+						Kind:               "b",
+						Name:               "c-2",
+						Controller:         &False,
+						BlockOwnerDeletion: &False,
+						UID:                "",
+					},
+				},
+			},
+			Out: []metav1.OwnerReference{
+				{
+					APIVersion:         "a",
+					Kind:               "b",
+					Name:               "c-1",
+					Controller:         &False,
+					BlockOwnerDeletion: &False,
+					UID:                "x",
+				},
+				{
+					APIVersion:         "a",
+					Kind:               "b",
+					Name:               "c-2",
+					Controller:         &False,
+					BlockOwnerDeletion: &False,
+					UID:                "",
+				},
+			},
+		},
+		{
+			Name: "duplicates combined",
+			In: [][]metav1.OwnerReference{
+				{
+					{
+						APIVersion:         "a",
+						Kind:               "b",
+						Name:               "c",
+						Controller:         &False,
+						BlockOwnerDeletion: &False,
+						UID:                "x",
+					},
+				},
+				{
+					{
+						APIVersion:         "a",
+						Kind:               "b",
+						Name:               "c",
+						Controller:         &False,
+						BlockOwnerDeletion: &False,
+						UID:                "x",
+					},
+				},
+			},
+			Out: []metav1.OwnerReference{
+				{
+					APIVersion:         "a",
+					Kind:               "b",
+					Name:               "c",
+					Controller:         &False,
+					BlockOwnerDeletion: &False,
+					UID:                "x",
+				},
+			},
+		},
+	} {
+		t.Run(tc.Name, func(t *testing.T) {
+			assert.ElementsMatch(t, tc.Out, mergedOwnerReferences(tc.In...))
+		})
+	}
+}
diff --git a/test/e2e/installplan_e2e_test.go b/test/e2e/installplan_e2e_test.go
index c82b96328..e4102d04f 100644
--- a/test/e2e/installplan_e2e_test.go
+++ b/test/e2e/installplan_e2e_test.go
@@ -31,6 +31,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/apimachinery/pkg/watch"
 	"k8s.io/client-go/util/retry"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	runtimeclient "sigs.k8s.io/controller-runtime/pkg/client"
 
 	opver "github.com/operator-framework/api/pkg/lib/version"
@@ -50,6 +51,112 @@ var _ = Describe("Install Plan", func() {
 		TearDown(testNamespace)
 	})
 
+	When("an InstallPlan transfers ownership of a ServiceAccount to a new ClusterServiceVersion", func() {
+		var (
+			csv1, csv2 operatorsv1alpha1.ClusterServiceVersion
+			sa         corev1.ServiceAccount
+			plan       operatorsv1alpha1.InstallPlan
+		)
+
+		BeforeEach(func() {
+			csv1 = newCSV("test-csv-old", testNamespace, "", semver.Version{}, nil, nil, nil)
+			Expect(ctx.Ctx().Client().Create(context.TODO(), &csv1)).To(Succeed())
+			csv2 = newCSV("test-csv-new", testNamespace, "", semver.Version{}, nil, nil, nil)
+			Expect(ctx.Ctx().Client().Create(context.TODO(), &csv2)).To(Succeed())
+
+			sa = corev1.ServiceAccount{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: testNamespace,
+					Name:      "test-serviceaccount",
+					OwnerReferences: []metav1.OwnerReference{
+						{
+							APIVersion: operatorsv1alpha1.SchemeGroupVersion.String(),
+							Kind:       operatorsv1alpha1.ClusterServiceVersionKind,
+							Name:       csv1.GetName(),
+							UID:        csv1.GetUID(),
+						},
+					},
+				},
+			}
+			Expect(ctx.Ctx().Client().Create(context.TODO(), &sa)).To(Succeed())
+
+			scheme := runtime.NewScheme()
+			Expect(corev1.AddToScheme(scheme)).To(Succeed())
+			var manifest bytes.Buffer
+			Expect(k8sjson.NewSerializer(k8sjson.DefaultMetaFactory, scheme, scheme, false).Encode(&corev1.ServiceAccount{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: testNamespace,
+					Name:      "test-serviceaccount",
+					OwnerReferences: []metav1.OwnerReference{
+						{
+							APIVersion: operatorsv1alpha1.SchemeGroupVersion.String(),
+							Kind:       operatorsv1alpha1.ClusterServiceVersionKind,
+							Name:       csv2.GetName(),
+						},
+					},
+				},
+			}, &manifest)).To(Succeed())
+
+			plan = operatorsv1alpha1.InstallPlan{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: testNamespace,
+					Name:      "test-plan",
+				},
+				Spec: operatorsv1alpha1.InstallPlanSpec{
+					Approval:                   operatorsv1alpha1.ApprovalAutomatic,
+					Approved:                   true,
+					ClusterServiceVersionNames: []string{},
+				},
+				Status: operatorsv1alpha1.InstallPlanStatus{
+					Phase:          operatorsv1alpha1.InstallPlanPhaseInstalling,
+					CatalogSources: []string{},
+					Plan: []*operatorsv1alpha1.Step{
+						{
+							Status: operatorsv1alpha1.StepStatusUnknown,
+							Resource: operatorsv1alpha1.StepResource{
+								Name:     sa.GetName(),
+								Version:  "v1",
+								Kind:     "ServiceAccount",
+								Manifest: manifest.String(),
+							},
+						},
+					},
+				},
+			}
+			Expect(ctx.Ctx().Client().Create(context.Background(), &plan)).To(Succeed())
+			Expect(ctx.Ctx().Client().Status().Update(context.Background(), &plan)).To(Succeed())
+		})
+
+		AfterEach(func() {
+			Expect(ctx.Ctx().Client().Delete(context.TODO(), &sa)).To(Or(
+				Succeed(),
+				WithTransform(k8serrors.IsNotFound, BeTrue()),
+			))
+		})
+
+		It("preserves owner references to both the old and the new ClusterServiceVersion", func() {
+			Eventually(func() ([]metav1.OwnerReference, error) {
+				if err := ctx.Ctx().Client().Get(context.TODO(), client.ObjectKeyFromObject(&sa), &sa); err != nil {
+					return nil, err
+				}
+				return sa.GetOwnerReferences(), nil
+			}).Should(ContainElements([]metav1.OwnerReference{
+				{
+					APIVersion: operatorsv1alpha1.SchemeGroupVersion.String(),
+					Kind:       operatorsv1alpha1.ClusterServiceVersionKind,
+					Name:       csv1.GetName(),
+					UID:        csv1.GetUID(),
+				},
+				{
+					APIVersion: operatorsv1alpha1.SchemeGroupVersion.String(),
+					Kind:       operatorsv1alpha1.ClusterServiceVersionKind,
+					Name:       csv2.GetName(),
+					UID:        csv2.GetUID(),
+				},
+			}))
+		})
+	})
+
 	When("a ClusterIP service exists", func() {
 		var (
 			service *corev1.Service