Skip to content

Commit 4c7479c

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #1123 from Bowenislandsong/bugfix_dup_secrets
Bug 1769030: Replacing operator creates duplicate secrets
2 parents 812d184 + 22c130d commit 4c7479c

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)