From 22c130dda5c39375ac6bde150fead92138fe0b4f Mon Sep 17 00:00:00 2001 From: bowenislandsong Date: Tue, 12 Nov 2019 16:03:13 -0500 Subject: [PATCH] Bug 1769030 Replacing (updating) operator creates duplicate secrets for the operator's ServiceAccount Cause: OLM catalog ensurer EnsureServiceAccount makes sure the service account is updated when a new version of an operator is present. This happens during ExecutePlan applying InstallPlan to a namespace. If it is an update, fields of service account are updated but the references to older secrets are dropped. Consequence: This process of dereferencing secret fails to clean up the older secrets and result in the secrets pilling up as the operator upgrades. Eventually, there will be too many old secrets laying around and only getting cleaned up when the operator is uninstalled. Fix: We carry over older secrets through updating the service account. We also compare the update using DeepDerivative to see if the update changes any existing fields. If not, we skip the update API call since it will not change anything. Result: Older secretes are again referred in the updated SA and no new secrets are created. --- .../operators/catalog/operator_test.go | 111 ++++++++++++++++++ .../operators/catalog/step_ensurer.go | 29 +++-- test/e2e/installplan_e2e_test.go | 11 ++ 3 files changed, 144 insertions(+), 7 deletions(-) diff --git a/pkg/controller/operators/catalog/operator_test.go b/pkg/controller/operators/catalog/operator_test.go index 0a3f2b627b..20c61aec37 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/pkg/controller/operators/catalog/step_ensurer.go b/pkg/controller/operators/catalog/step_ensurer.go index eeccb9a42a..32fae51826 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 diff --git a/test/e2e/installplan_e2e_test.go b/test/e2e/installplan_e2e_test.go index fa6940b79e..f907b84927 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{