Skip to content

Commit 22c130d

Browse files
author
bowenislandsong
committed
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.
1 parent 812d184 commit 22c130d

File tree

3 files changed

+144
-7
lines changed

3 files changed

+144
-7
lines changed

‎pkg/controller/operators/catalog/operator_test.go

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -382,6 +382,96 @@ func TestExecutePlan(t *testing.T) {
382382
want: []runtime.Object{service("service", namespace), csv("csv", namespace, nil, nil)},
383383
err: nil,
384384
},
385+
{
386+
testName: "CreateServiceAccount",
387+
in: withSteps(installPlan("p", namespace, v1alpha1.InstallPlanPhaseInstalling, "csv"),
388+
[]*v1alpha1.Step{
389+
{
390+
Resource: v1alpha1.StepResource{
391+
CatalogSource: "catalog",
392+
CatalogSourceNamespace: namespace,
393+
Group: "",
394+
Version: "v1",
395+
Kind: "ServiceAccount",
396+
Name: "sa",
397+
Manifest: toManifest(serviceAccount("sa", namespace, "",
398+
objectReference("init secret"))),
399+
},
400+
Status: v1alpha1.StepStatusUnknown,
401+
},
402+
},
403+
),
404+
want: []runtime.Object{serviceAccount("sa", namespace, "", objectReference("init secret"))},
405+
err: nil,
406+
},
407+
{
408+
testName: "UpdateServiceAccountWithSameFields",
409+
in: withSteps(installPlan("p", namespace, v1alpha1.InstallPlanPhaseInstalling, "csv"),
410+
[]*v1alpha1.Step{
411+
{
412+
Resource: v1alpha1.StepResource{
413+
CatalogSource: "catalog",
414+
CatalogSourceNamespace: namespace,
415+
Group: "",
416+
Version: "v1",
417+
Kind: "ServiceAccount",
418+
Name: "sa",
419+
Manifest: toManifest(serviceAccount("sa", namespace, "name",
420+
objectReference("init secret"))),
421+
},
422+
Status: v1alpha1.StepStatusUnknown,
423+
},
424+
{
425+
Resource: v1alpha1.StepResource{
426+
CatalogSource: "catalog",
427+
CatalogSourceNamespace: namespace,
428+
Group: "",
429+
Version: "v1",
430+
Kind: "ServiceAccount",
431+
Name: "sa",
432+
Manifest: toManifest(serviceAccount("sa", namespace, "name", nil)),
433+
},
434+
Status: v1alpha1.StepStatusUnknown,
435+
},
436+
},
437+
),
438+
want: []runtime.Object{serviceAccount("sa", namespace, "name", objectReference("init secret"))},
439+
err: nil,
440+
},
441+
{
442+
testName: "UpdateServiceAccountWithDiffFields",
443+
in: withSteps(installPlan("p", namespace, v1alpha1.InstallPlanPhaseInstalling, "csv"),
444+
[]*v1alpha1.Step{
445+
{
446+
Resource: v1alpha1.StepResource{
447+
CatalogSource: "catalog",
448+
CatalogSourceNamespace: namespace,
449+
Group: "",
450+
Version: "v1",
451+
Kind: "ServiceAccount",
452+
Name: "sa",
453+
Manifest: toManifest(serviceAccount("sa", namespace, "old_name",
454+
objectReference("init secret"))),
455+
},
456+
Status: v1alpha1.StepStatusUnknown,
457+
},
458+
{
459+
Resource: v1alpha1.StepResource{
460+
CatalogSource: "catalog",
461+
CatalogSourceNamespace: namespace,
462+
Group: "",
463+
Version: "v1",
464+
Kind: "ServiceAccount",
465+
Name: "sa",
466+
Manifest: toManifest(serviceAccount("sa", namespace, "new_name", nil)),
467+
},
468+
Status: v1alpha1.StepStatusUnknown,
469+
},
470+
},
471+
),
472+
want: []runtime.Object{serviceAccount("sa", namespace, "new_name", objectReference("init secret"))},
473+
err: nil,
474+
},
385475
}
386476

387477
for _, tt := range tests {
@@ -411,6 +501,8 @@ func TestExecutePlan(t *testing.T) {
411501
fetched, err = op.opClient.GetRoleBinding(namespace, o.GetName())
412502
case *corev1.ServiceAccount:
413503
fetched, err = op.opClient.GetServiceAccount(namespace, o.GetName())
504+
case *corev1.Secret:
505+
fetched, err = op.opClient.GetSecret(namespace, o.GetName())
414506
case *corev1.Service:
415507
fetched, err = op.opClient.GetService(namespace, o.GetName())
416508
case *v1alpha1.ClusterServiceVersion:
@@ -1010,6 +1102,25 @@ func service(name, namespace string) *corev1.Service {
10101102
}
10111103
}
10121104

1105+
func serviceAccount(name, namespace, generateName string, secretRef *corev1.ObjectReference) *corev1.ServiceAccount {
1106+
if secretRef == nil {
1107+
return &corev1.ServiceAccount{
1108+
ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace, GenerateName: generateName},
1109+
}
1110+
}
1111+
return &corev1.ServiceAccount{
1112+
ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace, GenerateName: generateName},
1113+
Secrets: []corev1.ObjectReference{*secretRef},
1114+
}
1115+
}
1116+
1117+
func objectReference(name string) *corev1.ObjectReference {
1118+
if name == "" {
1119+
return &corev1.ObjectReference{}
1120+
}
1121+
return &corev1.ObjectReference{Name: name}
1122+
}
1123+
10131124
func toManifest(obj runtime.Object) string {
10141125
raw, _ := json.Marshal(obj)
10151126
return string(raw)

‎pkg/controller/operators/catalog/step_ensurer.go

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,17 @@ package catalog
33
import (
44
"fmt"
55

6-
"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
7-
"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned"
8-
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
9-
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
106
errorwrap "github.com/pkg/errors"
117
corev1 "k8s.io/api/core/v1"
128
rbacv1 "k8s.io/api/rbac/v1"
9+
apiequality "k8s.io/apimachinery/pkg/api/equality"
1310
k8serrors "k8s.io/apimachinery/pkg/api/errors"
1411
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
12+
13+
"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
14+
"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned"
15+
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
16+
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
1517
)
1618

1719
func newStepEnsurer(kubeClient operatorclient.ClientInterface, crClient versioned.Interface) *StepEnsurer {
@@ -116,11 +118,24 @@ func (o *StepEnsurer) EnsureServiceAccount(namespace string, sa *corev1.ServiceA
116118
return
117119
}
118120

119-
sa.SetNamespace(namespace)
120-
if _, updateErr := o.kubeClient.UpdateServiceAccount(sa); updateErr != nil {
121-
err = errorwrap.Wrapf(updateErr, "error updating service account: %s", sa.GetName())
121+
// Carrying secrets through the service account update.
122+
preSa, getErr := o.kubeClient.KubernetesInterface().CoreV1().ServiceAccounts(namespace).Get(sa.Name,
123+
metav1.GetOptions{})
124+
if getErr != nil {
125+
err = errorwrap.Wrapf(getErr, "error getting older version of service account: %s", sa.GetName())
122126
return
123127
}
128+
sa.Secrets = preSa.Secrets
129+
130+
sa.SetNamespace(namespace)
131+
132+
// Use DeepDerivative to check if new SA is the same as the old SA. If no field is changed, we skip the update call.
133+
if !apiequality.Semantic.DeepDerivative(sa, preSa) {
134+
if _, updateErr := o.kubeClient.UpdateServiceAccount(sa); updateErr != nil {
135+
err = errorwrap.Wrapf(updateErr, "error updating service account: %s", sa.GetName())
136+
return
137+
}
138+
}
124139

125140
status = v1alpha1.StepStatusPresent
126141
return

‎test/e2e/installplan_e2e_test.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"time"
1010

1111
"github.com/blang/semver"
12+
"github.com/stretchr/testify/assert"
1213
"github.com/stretchr/testify/require"
1314
appsv1 "k8s.io/api/apps/v1"
1415
authorizationv1 "k8s.io/api/authorization/v1"
@@ -1610,6 +1611,9 @@ func TestUpdateCatalogForSubscription(t *testing.T) {
16101611
},
16111612
}
16121613

1614+
oldSecrets, err := c.KubernetesInterface().CoreV1().Secrets(testNamespace).List(metav1.ListOptions{})
1615+
require.NoError(t, err, "error listing secrets")
1616+
16131617
// Create the catalog sources
16141618
updatedNamedStrategy := newNginxInstallStrategy(genName("dep-"), updatedPermissions, updatedClusterPermissions)
16151619
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) {
16401644
_, err = awaitCSV(t, crc, testNamespace, updatedCSV.GetName(), csvSucceededChecker)
16411645
require.NoError(t, err)
16421646

1647+
newSecrets, err := c.KubernetesInterface().CoreV1().Secrets(testNamespace).List(metav1.ListOptions{})
1648+
require.NoError(t, err, "error listing secrets")
1649+
// Assert that the number of secrets is not increased from updating service account as part of the install plan,
1650+
assert.EqualValues(t, len(oldSecrets.Items), len(newSecrets.Items))
1651+
// And that the secret list is indeed updated.
1652+
assert.Equal(t, oldSecrets.Items, newSecrets.Items)
1653+
16431654
// Wait for ServiceAccount to not have access anymore
16441655
err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
16451656
res, err := c.KubernetesInterface().AuthorizationV1().SubjectAccessReviews().Create(&authorizationv1.SubjectAccessReview{

0 commit comments

Comments
 (0)