Repository: operator-framework/operator-lifecycle-manager
Pull request #1160 (2019-12-02 16:58:03)

[release-4.3] Bug 1776521: Replacing operator creates duplicate secrets
This is an automated cherry-pick of #1123

/assign Bowenislandsong


Patch:
diff --git a/pkg/controller/operators/catalog/step_ensurer.go b/pkg/controller/operators/catalog/step_ensurer.go
index eeccb9a42..32fae5182 100644
--- a/pkg/controller/operators/catalog/step_ensurer.go
+++ b/pkg/controller/operators/catalog/step_ensurer.go
@@ -3,15 +3,17 @@ package catalog
 import (
 	"fmt"
 
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
 	errorwrap "github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
+	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
 )
 
 func newStepEnsurer(kubeClient operatorclient.ClientInterface, crClient versioned.Interface) *StepEnsurer {
@@ -116,11 +118,24 @@ func (o *StepEnsurer) EnsureServiceAccount(namespace string, sa *corev1.ServiceA
 		return
 	}
 
-	sa.SetNamespace(namespace)
-	if _, updateErr := o.kubeClient.UpdateServiceAccount(sa); updateErr != nil {
-		err = errorwrap.Wrapf(updateErr, "error updating service account: %s", sa.GetName())
+	// Carrying secrets through the service account update.
+	preSa, getErr := o.kubeClient.KubernetesInterface().CoreV1().ServiceAccounts(namespace).Get(sa.Name,
+		metav1.GetOptions{})
+	if getErr != nil {
+		err = errorwrap.Wrapf(getErr, "error getting older version of service account: %s", sa.GetName())
 		return
 	}
+	sa.Secrets = preSa.Secrets
+
+	sa.SetNamespace(namespace)
+
+	// Use DeepDerivative to check if new SA is the same as the old SA. If no field is changed, we skip the update call.
+	if !apiequality.Semantic.DeepDerivative(sa, preSa) {
+		if _, updateErr := o.kubeClient.UpdateServiceAccount(sa); updateErr != nil {
+			err = errorwrap.Wrapf(updateErr, "error updating service account: %s", sa.GetName())
+			return
+		}
+	}
 
 	status = v1alpha1.StepStatusPresent
 	return


Test patch:
diff --git a/pkg/controller/operators/catalog/operator_test.go b/pkg/controller/operators/catalog/operator_test.go
index 9283a4b46..ca4c324aa 100644
--- a/pkg/controller/operators/catalog/operator_test.go
+++ b/pkg/controller/operators/catalog/operator_test.go
@@ -382,6 +382,96 @@ func TestExecutePlan(t *testing.T) {
 			want: []runtime.Object{service("service", namespace), csv("csv", namespace, nil, nil)},
 			err:  nil,
 		},
+		{
+			testName: "CreateServiceAccount",
+			in: withSteps(installPlan("p", namespace, v1alpha1.InstallPlanPhaseInstalling, "csv"),
+				[]*v1alpha1.Step{
+					{
+						Resource: v1alpha1.StepResource{
+							CatalogSource:          "catalog",
+							CatalogSourceNamespace: namespace,
+							Group:                  "",
+							Version:                "v1",
+							Kind:                   "ServiceAccount",
+							Name:                   "sa",
+							Manifest: toManifest(serviceAccount("sa", namespace, "",
+								objectReference("init secret"))),
+						},
+						Status: v1alpha1.StepStatusUnknown,
+					},
+				},
+			),
+			want: []runtime.Object{serviceAccount("sa", namespace, "", objectReference("init secret"))},
+			err:  nil,
+		},
+		{
+			testName: "UpdateServiceAccountWithSameFields",
+			in: withSteps(installPlan("p", namespace, v1alpha1.InstallPlanPhaseInstalling, "csv"),
+				[]*v1alpha1.Step{
+					{
+						Resource: v1alpha1.StepResource{
+							CatalogSource:          "catalog",
+							CatalogSourceNamespace: namespace,
+							Group:                  "",
+							Version:                "v1",
+							Kind:                   "ServiceAccount",
+							Name:                   "sa",
+							Manifest: toManifest(serviceAccount("sa", namespace, "name",
+								objectReference("init secret"))),
+						},
+						Status: v1alpha1.StepStatusUnknown,
+					},
+					{
+						Resource: v1alpha1.StepResource{
+							CatalogSource:          "catalog",
+							CatalogSourceNamespace: namespace,
+							Group:                  "",
+							Version:                "v1",
+							Kind:                   "ServiceAccount",
+							Name:                   "sa",
+							Manifest:               toManifest(serviceAccount("sa", namespace, "name", nil)),
+						},
+						Status: v1alpha1.StepStatusUnknown,
+					},
+				},
+			),
+			want: []runtime.Object{serviceAccount("sa", namespace, "name", objectReference("init secret"))},
+			err:  nil,
+		},
+		{
+			testName: "UpdateServiceAccountWithDiffFields",
+			in: withSteps(installPlan("p", namespace, v1alpha1.InstallPlanPhaseInstalling, "csv"),
+				[]*v1alpha1.Step{
+					{
+						Resource: v1alpha1.StepResource{
+							CatalogSource:          "catalog",
+							CatalogSourceNamespace: namespace,
+							Group:                  "",
+							Version:                "v1",
+							Kind:                   "ServiceAccount",
+							Name:                   "sa",
+							Manifest: toManifest(serviceAccount("sa", namespace, "old_name",
+								objectReference("init secret"))),
+						},
+						Status: v1alpha1.StepStatusUnknown,
+					},
+					{
+						Resource: v1alpha1.StepResource{
+							CatalogSource:          "catalog",
+							CatalogSourceNamespace: namespace,
+							Group:                  "",
+							Version:                "v1",
+							Kind:                   "ServiceAccount",
+							Name:                   "sa",
+							Manifest:               toManifest(serviceAccount("sa", namespace, "new_name", nil)),
+						},
+						Status: v1alpha1.StepStatusUnknown,
+					},
+				},
+			),
+			want: []runtime.Object{serviceAccount("sa", namespace, "new_name", objectReference("init secret"))},
+			err:  nil,
+		},
 	}
 
 	for _, tt := range tests {
@@ -411,6 +501,8 @@ func TestExecutePlan(t *testing.T) {
 					fetched, err = op.opClient.GetRoleBinding(namespace, o.GetName())
 				case *corev1.ServiceAccount:
 					fetched, err = op.opClient.GetServiceAccount(namespace, o.GetName())
+				case *corev1.Secret:
+					fetched, err = op.opClient.GetSecret(namespace, o.GetName())
 				case *corev1.Service:
 					fetched, err = op.opClient.GetService(namespace, o.GetName())
 				case *v1alpha1.ClusterServiceVersion:
@@ -1010,6 +1102,25 @@ func service(name, namespace string) *corev1.Service {
 	}
 }
 
+func serviceAccount(name, namespace, generateName string, secretRef *corev1.ObjectReference) *corev1.ServiceAccount {
+	if secretRef == nil {
+		return &corev1.ServiceAccount{
+			ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace, GenerateName: generateName},
+		}
+	}
+	return &corev1.ServiceAccount{
+		ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace, GenerateName: generateName},
+		Secrets:    []corev1.ObjectReference{*secretRef},
+	}
+}
+
+func objectReference(name string) *corev1.ObjectReference {
+	if name == "" {
+		return &corev1.ObjectReference{}
+	}
+	return &corev1.ObjectReference{Name: name}
+}
+
 func toManifest(obj runtime.Object) string {
 	raw, _ := json.Marshal(obj)
 	return string(raw)
diff --git a/test/e2e/installplan_e2e_test.go b/test/e2e/installplan_e2e_test.go
index fa6940b79..f907b8492 100644
--- a/test/e2e/installplan_e2e_test.go
+++ b/test/e2e/installplan_e2e_test.go
@@ -9,6 +9,7 @@ import (
 	"time"
 
 	"github.com/blang/semver"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	appsv1 "k8s.io/api/apps/v1"
 	authorizationv1 "k8s.io/api/authorization/v1"
@@ -1610,6 +1611,9 @@ func TestUpdateCatalogForSubscription(t *testing.T) {
 			},
 		}
 
+		oldSecrets, err := c.KubernetesInterface().CoreV1().Secrets(testNamespace).List(metav1.ListOptions{})
+		require.NoError(t, err, "error listing secrets")
+
 		// Create the catalog sources
 		updatedNamedStrategy := newNginxInstallStrategy(genName("dep-"), updatedPermissions, updatedClusterPermissions)
 		updatedCSV := newCSV(mainPackageStable+"-next", testNamespace, mainCSV.GetName(), semver.MustParse("0.2.0"), []apiextensions.CustomResourceDefinition{mainCRD}, nil, updatedNamedStrategy)
@@ -1640,6 +1644,13 @@ func TestUpdateCatalogForSubscription(t *testing.T) {
 		_, err = awaitCSV(t, crc, testNamespace, updatedCSV.GetName(), csvSucceededChecker)
 		require.NoError(t, err)
 
+		newSecrets, err := c.KubernetesInterface().CoreV1().Secrets(testNamespace).List(metav1.ListOptions{})
+		require.NoError(t, err, "error listing secrets")
+		// Assert that the number of secrets is not increased from updating service account as part of the install plan,
+		assert.EqualValues(t, len(oldSecrets.Items), len(newSecrets.Items))
+		// And that the secret list is indeed updated.
+		assert.Equal(t, oldSecrets.Items, newSecrets.Items)
+
 		// Wait for ServiceAccount to not have access anymore
 		err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
 			res, err := c.KubernetesInterface().AuthorizationV1().SubjectAccessReviews().Create(&authorizationv1.SubjectAccessReview{