Bug 1769030: Replacing operator creates duplicate secrets #1123

Bowenislandsong · 2019-11-12T22:33:48Z

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.

Result:
Older secretes are again referred in the updated SA.

Bowenislandsong · 2019-11-12T22:34:45Z

e2e test inprog

jpeeler · 2019-11-18T15:52:21Z

pkg/controller/operators/catalog/step_ensurer.go

 	sa.SetNamespace(namespace)
 	if _, updateErr := o.kubeClient.UpdateServiceAccount(sa); updateErr != nil {
 		err = errorwrap.Wrapf(updateErr, "error updating service account: %s", sa.GetName())
 		return
 	}

+	for _, secret := range preSa.Secrets {
+		foregroundDelete := metav1.DeletePropagationForeground


I don't think doing a foreground type of deletion is necessary if there's nothing left referencing these secrets as dependencies, right?

You are right. This is unnecessary but I thought it would not hurt. Do you think we should just leave it blank then? Thanks

I believe you should have nothing passed into the DeleteOptions below.

ecordell · 2019-11-18T17:56:45Z

pkg/controller/operators/catalog/step_ensurer.go

@@ -116,12 +116,35 @@ func (o *StepEnsurer) EnsureServiceAccount(namespace string, sa *corev1.ServiceA
 		return
 	}

+	// Before UpdateServiceAccount and dereference secrets of the older SA,


Doesn't the original bug report have this issue with the same serviceaccount name?

From line 114, this section should only be hit if the serviceaccount doesn't exist at all, so I'm not convinced this will fix the bug.

I think the issue may be that Create on ServiceAccount always creates a secret even if the serviceaccount exists? Maybe we should start with a Get before the initial Create?

Do you agree, or am I missing how this fixes the issue?

You might have missed !k8serrors.IsAlreadyExists on line 114 which reports any SA creation problem other than SA exists. Therefore, the only scenario where my addition hits is if the SA does exists.

The logic of the original code got me at first as well.

This is what I get for reviewing too quickly 😄 Ignore my previous comment.

However - I really don't think we should be in the business of deleting the serviceaccount secrets.

I did some testing, and the issue is that when we Update, our SA definition doesn't have the secrets set, which removes the configured secret and causes the SA controller to generate a new one:

$ k -n default create serviceaccount test serviceaccount/test created $ k -n default get secrets NAME TYPE DATA AGE default-token-67bw8 kubernetes.io/service-account-token 3 3m23s test-token-9lx4p kubernetes.io/service-account-token 3 3s # an update that doesn't cause the bug $ k -n default label serviceaccounts test with=test serviceaccount/test labeled # no duplicate secrets $ k -n default get secrets NAME TYPE DATA AGE default-token-67bw8 kubernetes.io/service-account-token 3 6m40s test-token-9lx4p kubernetes.io/service-account-token 3 3m20s # an update that causes the bug - remove the secrets block $ k -n default edit serviceaccounts test serviceaccount/test edited # now there are duplicates $ k -n default get secrets NAME TYPE DATA AGE default-token-67bw8 kubernetes.io/service-account-token 3 8m3s test-token-9lx4p kubernetes.io/service-account-token 3 4m43s test-token-xwmpg kubernetes.io/service-account-token 3 4s

The solution here shouldn't be to delete the extra secrets, it should be to not create the extra secrets in the first place.

A quick option for now would be to:

Get the current ServiceAccount (if it exists)

Add the current secret list to the object from the step

Then call update with that object - it will have the secret list so a new one won't be generated

But it would be even better to use DeepDerivative or server-side apply.

That was my other thought on the fix too. I eventually chose this to make sure we always have the right token in the secrets. I worry that secrets may change in the install plan rendering old secrets to be invalid. However I might be worrying for nothing. I’ll make a commit to use that approach.

@ecordell I liked the server-side approach but it being in beta gives me goosebumps. I am not entirely sure about your suggestion about DeepDerivative. If you are talking about https://github.com/kubernetes/kubernetes/blob/6be19d85f206d64044ae940540a437c65675a526/third_party/forked/golang/reflect/deep_equal.go#L378 then I do not see how we can use it. If you are talking about the concept. Not every field can be copied (ie: timestamp, etc). If we copy every unset field in the new SA from the existing SA, we need to know exactly what is not copiable. I do not think my understanding of your DeepDerivative is correct. Therefore, please let me know what you are explicitly suggesting. Thank you!

Bowenislandsong · 2019-11-18T19:09:44Z

/hold
E2e test includes pulling quay image. This should be removed before merging.

Bowenislandsong · 2019-11-19T18:38:03Z

/bugzilla refresh

openshift-ci-robot · 2019-11-19T18:38:04Z

@Bowenislandsong: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2019-11-19T18:38:25Z

@Bowenislandsong: This pull request references Bugzilla bug 1769030, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1769030: Replacing operator creates duplicate secrets

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Bowenislandsong · 2019-11-19T21:22:13Z

/hold cancel

awgreene · 2019-11-26T18:09:44Z

@Bowenislandsong you may want to use deepderivative to compare the objects.

ecordell · 2019-11-27T22:19:27Z

/bugzilla refresh

openshift-ci-robot · 2019-11-27T22:19:30Z

@ecordell: This pull request references Bugzilla bug 1769030, which is valid.

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ecordell · 2019-11-27T22:20:22Z

/lgtm

Bowenislandsong · 2019-11-27T22:35:20Z

/cc @ecordell @awgreene added DeepDerivative check to save API calls if updated SA has the exact same fields.

ecordell · 2019-11-27T22:38:31Z

/lgtm

…or 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.

gallettilance

/lgtm

openshift-ci-robot · 2019-12-02T14:45:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, Bowenislandsong, ecordell, gallettilance

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ecordell]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Bowenislandsong · 2019-12-02T15:22:02Z

/test e2e-gcp-upgrade
/test e2e-gcp

openshift-ci-robot · 2019-12-02T16:57:47Z

@Bowenislandsong: All pull requests linked via external trackers have merged. Bugzilla bug 1769030 has been moved to the MODIFIED state.

In response to this:

Bug 1769030: Replacing operator creates duplicate secrets

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-cherrypick-robot · 2019-12-02T16:57:56Z

@Bowenislandsong: new pull request created: #1159

In response to this:

/cherry-pick release-4.2

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-cherrypick-robot · 2019-12-02T16:58:04Z

@Bowenislandsong: new pull request created: #1160

In response to this:

/cherry-pick release-4.3

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-cherrypick-robot · 2019-12-02T16:58:07Z

@Bowenislandsong: cannot checkout release-4.2 cancel: error checking out release-4.2 cancel: exit status 1. output: error: pathspec 'release-4.2 cancel' did not match any file(s) known to git.

In response to this:

/cherry-pick release-4.2 cancel

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-cherrypick-robot · 2019-12-02T16:58:12Z

@Bowenislandsong: #1123 failed to apply on top of branch "release-4.1":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	pkg/controller/operators/catalog/operator_test.go
A	pkg/controller/operators/catalog/step_ensurer.go
M	test/e2e/installplan_e2e_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/installplan_e2e_test.go
CONFLICT (content): Merge conflict in test/e2e/installplan_e2e_test.go
CONFLICT (modify/delete): pkg/controller/operators/catalog/step_ensurer.go deleted in HEAD and modified in Bug 1769030 Replacing (updating) operator creates duplicate secrets for the operator's ServiceAccount. Version Bug 1769030 Replacing (updating) operator creates duplicate secrets for the operator's ServiceAccount of pkg/controller/operators/catalog/step_ensurer.go left in tree.
Auto-merging pkg/controller/operators/catalog/operator_test.go
CONFLICT (content): Merge conflict in pkg/controller/operators/catalog/operator_test.go
Patch failed at 0001 Bug 1769030 Replacing (updating) operator creates duplicate secrets for the operator's ServiceAccount

In response to this:

/cherry-pick release-4.1

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 12, 2019

openshift-ci-robot requested review from ecordell and njhale November 12, 2019 22:34

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 13, 2019

Bowenislandsong force-pushed the bugfix_dup_secrets branch from decb744 to 4979bee Compare November 13, 2019 20:47

operator-framework deleted a comment Nov 14, 2019

operator-framework deleted a comment from h777xx Nov 14, 2019

operator-framework deleted a comment Nov 14, 2019

Bowenislandsong force-pushed the bugfix_dup_secrets branch 2 times, most recently from cadbe2d to c43b2ee Compare November 18, 2019 15:28

Bowenislandsong changed the title ~~[wip] Bug 1769030 Replacing (updating) operator creates duplicate secrets f…~~ Bug 1769030 Replacing operator creates duplicate secrets Nov 18, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 18, 2019

jpeeler reviewed Nov 18, 2019

View reviewed changes

ecordell reviewed Nov 18, 2019

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 18, 2019

Bowenislandsong changed the title ~~Bug 1769030 Replacing operator creates duplicate secrets~~ Bug 1769030: Replacing operator creates duplicate secrets Nov 19, 2019

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Nov 19, 2019

Bowenislandsong force-pushed the bugfix_dup_secrets branch 2 times, most recently from 98082e7 to 2fc7268 Compare November 19, 2019 21:21

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 19, 2019

openshift-ci-robot assigned ecordell Nov 27, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 27, 2019

Bowenislandsong force-pushed the bugfix_dup_secrets branch from f3d4781 to 22c130d Compare November 27, 2019 22:46

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 27, 2019

gallettilance approved these changes Dec 2, 2019

View reviewed changes

openshift-ci-robot assigned gallettilance Dec 2, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2019

openshift-merge-robot merged commit 4c7479c into operator-framework:master Dec 2, 2019

openshift-cherrypick-robot mentioned this pull request Dec 2, 2019

[release-4.2] Bug 1769561: Replacing operator creates duplicate secrets #1159

Merged

openshift-cherrypick-robot mentioned this pull request Dec 2, 2019

[release-4.3] Bug 1776521: Replacing operator creates duplicate secrets #1160

Merged

Bowenislandsong mentioned this pull request Jan 10, 2020

[release-4.2] Bug 1769561: Replacing operator creates duplicate secrets #1222

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1769030: Replacing operator creates duplicate secrets #1123

Bug 1769030: Replacing operator creates duplicate secrets #1123

Bowenislandsong commented Nov 12, 2019 •

edited

Loading

Bowenislandsong commented Nov 12, 2019

jpeeler Nov 18, 2019

Bowenislandsong Nov 18, 2019

jpeeler Nov 21, 2019

ecordell Nov 18, 2019

Bowenislandsong Nov 18, 2019 •

edited

Loading

ecordell Nov 22, 2019

Bowenislandsong Nov 22, 2019

Bowenislandsong Nov 25, 2019

Bowenislandsong commented Nov 18, 2019

Bowenislandsong commented Nov 19, 2019

openshift-ci-robot commented Nov 19, 2019

openshift-ci-robot commented Nov 19, 2019

Bowenislandsong commented Nov 19, 2019

awgreene commented Nov 26, 2019

ecordell commented Nov 27, 2019

openshift-ci-robot commented Nov 27, 2019

ecordell commented Nov 27, 2019

Bowenislandsong commented Nov 27, 2019 •

edited

Loading

ecordell commented Nov 27, 2019

gallettilance left a comment

openshift-ci-robot commented Dec 2, 2019

Bowenislandsong commented Dec 2, 2019

openshift-ci-robot commented Dec 2, 2019

openshift-cherrypick-robot commented Dec 2, 2019

openshift-cherrypick-robot commented Dec 2, 2019

openshift-cherrypick-robot commented Dec 2, 2019

openshift-cherrypick-robot commented Dec 2, 2019

Bug 1769030: Replacing operator creates duplicate secrets #1123

Bug 1769030: Replacing operator creates duplicate secrets #1123

Conversation

Bowenislandsong commented Nov 12, 2019 • edited Loading

Bowenislandsong commented Nov 12, 2019

jpeeler Nov 18, 2019

Choose a reason for hiding this comment

Bowenislandsong Nov 18, 2019

Choose a reason for hiding this comment

jpeeler Nov 21, 2019

Choose a reason for hiding this comment

ecordell Nov 18, 2019

Choose a reason for hiding this comment

Bowenislandsong Nov 18, 2019 • edited Loading

Choose a reason for hiding this comment

ecordell Nov 22, 2019

Choose a reason for hiding this comment

Bowenislandsong Nov 22, 2019

Choose a reason for hiding this comment

Bowenislandsong Nov 25, 2019

Choose a reason for hiding this comment

Bowenislandsong commented Nov 18, 2019

Bowenislandsong commented Nov 19, 2019

openshift-ci-robot commented Nov 19, 2019

openshift-ci-robot commented Nov 19, 2019

Bowenislandsong commented Nov 19, 2019

awgreene commented Nov 26, 2019

ecordell commented Nov 27, 2019

openshift-ci-robot commented Nov 27, 2019

ecordell commented Nov 27, 2019

Bowenislandsong commented Nov 27, 2019 • edited Loading

ecordell commented Nov 27, 2019

gallettilance left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Dec 2, 2019

Bowenislandsong commented Dec 2, 2019

openshift-ci-robot commented Dec 2, 2019

openshift-cherrypick-robot commented Dec 2, 2019

openshift-cherrypick-robot commented Dec 2, 2019

openshift-cherrypick-robot commented Dec 2, 2019

openshift-cherrypick-robot commented Dec 2, 2019

Bowenislandsong commented Nov 12, 2019 •

edited

Loading

Bowenislandsong Nov 18, 2019 •

edited

Loading

Bowenislandsong commented Nov 27, 2019 •

edited

Loading