Bug 1765294: Use OwnerRefs to clean up SA pull secrets by adambkaplan · Pull Request #61 · openshift/openshift-controller-manager

adambkaplan · 2020-01-29T15:04:55Z

Update the service account pull secret controller to add owner
references to generated pull secrets. This ensures pull secrets are
deleted when the associated token is deleted.

openshift-ci-robot · 2020-01-29T15:05:02Z

@adambkaplan: This pull request references Bugzilla bug 1765294, 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.

Details

In response to this:

WIP - Bug 1765294: Use OwnerRefs to clean up SA pull 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.

adambkaplan · 2020-01-29T22:06:46Z

/test e2e-aws-builds

adambkaplan · 2020-01-30T14:07:03Z

/test e2e-aws-builds

adambkaplan · 2020-01-30T14:07:44Z

/assign @bparees

/cc @smarterclayton @dmage

bparees · 2020-01-30T19:58:00Z

+			Name:       tokenSecret.Name,
+			UID:        tokenSecret.UID,
+		},
+	})


@deads2k are you aware of any reason we wouldn't want to have ownerrefs between the dockercfg secret and the token secret? I'm sure we didn't do it originally because ownerrefs didn't exist, but now that they do can we just lifecycle the dockercfg secret w/ the tokensecret? (modulo migration concerns)

bparees · 2020-01-30T20:34:46Z

lgtm assuming @deads2k doesn't have a reason why it's not ok to use GC here.

adambkaplan · 2020-01-31T21:46:52Z

/test e2e-aws-builds

adambkaplan · 2020-02-03T20:20:24Z

/retest

adambkaplan · 2020-02-04T16:21:54Z

/retest

adambkaplan · 2020-02-05T22:52:12Z

/retest

I suspect the errors in e2e-aws-builds are real and need to be investigated further.

adambkaplan · 2020-02-06T01:59:36Z

/test e2e-aws-builds

adambkaplan · 2020-02-11T14:03:07Z

@bparees @deads2k this is ready for final review

bparees · 2020-02-11T17:46:28Z

my review stands:
lgtm assuming @deads2k doesn't have a reason why it's not ok to use GC here.

adambkaplan · 2020-02-13T14:30:51Z

/retest

ping @deads2k per #61 (comment)

mfojtik · 2020-02-13T14:35:24Z

+
+// hasOwnerReference checks if the given tokenSecret is the owner of the provided pullSecret
+func hasOwnerReference(tokenSecret, pullSecret *v1.Secret) bool {
+	for _, owner := range pullSecret.OwnerReferences {


i think there is upstream helper that does this in generic way

@mfojtik I found the functions in https://github.com/kubernetes/apimachinery/blob/master/pkg/apis/meta/v1/controller_ref.go. Works -ish - I need to set blockOwnerDeletion to false, otherwise I need additional RBAC permissions.

adambkaplan · 2020-02-20T14:09:16Z

/retest

Update the service account pull secret controller to add owner references to generated pull secrets. This ensures pull secrets are deleted when the associated token is deleted.

adambkaplan

@bparees @mfojtik updated to use the upstream ControllerRef methods.

I spoke with @deads2k last week on potential risks - no immediate red flags, though he did caution "if this breaks, you own it."

adambkaplan · 2020-02-25T19:55:24Z

/retest

AWS rate limit issues

bparees · 2020-02-25T21:38:09Z

/lgtm

openshift-ci-robot · 2020-02-25T21:38:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, bparees

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [adambkaplan,bparees]

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

openshift-ci-robot · 2020-02-25T21:50:58Z

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

Details

In response to this:

Bug 1765294: Use OwnerRefs to clean up SA pull 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.

adambkaplan · 2020-02-25T23:04:32Z

/cherrypick release-4.4

/cherrypick release-4.3

openshift-cherrypick-robot · 2020-02-25T23:04:42Z

@adambkaplan: new pull request created: #67

Details

In response to this:

/cherrypick release-4.4

/cherrypick 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.

adambkaplan · 2020-02-25T23:07:08Z

/cherrypick release-4.3

openshift-cherrypick-robot · 2020-02-25T23:07:14Z

@adambkaplan: new pull request created: #68

Details

In response to this:

/cherrypick 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-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Jan 29, 2020

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 29, 2020

openshift-ci-robot requested review from mfojtik and soltysh January 29, 2020 15:07

adambkaplan force-pushed the pull-secret-use-ownerrefs branch from 42b2649 to 3d24283 Compare January 30, 2020 14:05

adambkaplan changed the title ~~WIP - Bug 1765294: Use OwnerRefs to clean up SA pull secrets~~ Bug 1765294: Use OwnerRefs to clean up SA pull secrets Jan 30, 2020

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 Jan 30, 2020

openshift-ci-robot assigned bparees Jan 30, 2020

openshift-ci-robot requested review from dmage and smarterclayton January 30, 2020 14:07

adambkaplan force-pushed the pull-secret-use-ownerrefs branch from 3d24283 to 7455a42 Compare January 30, 2020 14:12

bparees reviewed Jan 30, 2020

View reviewed changes

Comment thread pkg/serviceaccounts/controllers/pull_secrets.go Outdated

bparees reviewed Jan 30, 2020

View reviewed changes

Comment thread pkg/serviceaccounts/controllers/create_dockercfg_secrets.go Outdated

adambkaplan force-pushed the pull-secret-use-ownerrefs branch from 7455a42 to 13b5462 Compare January 30, 2020 20:44

adambkaplan commented Jan 31, 2020

View reviewed changes

Comment thread pkg/serviceaccounts/controllers/pull_secrets.go Outdated

adambkaplan mentioned this pull request Feb 4, 2020

[release-4.3] Bug 1765294: Increase timeout for Dockercfg tests openshift/origin#24329

Closed

adambkaplan force-pushed the pull-secret-use-ownerrefs branch from 5395f2b to 938624a Compare February 11, 2020 14:02

mfojtik reviewed Feb 13, 2020

View reviewed changes

adambkaplan changed the title ~~Bug 1765294: Use OwnerRefs to clean up SA pull secrets~~ WIP - Bug 1765294: Use OwnerRefs to clean up SA pull secrets Feb 20, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 20, 2020

Bug 1765294: Use OwnerRefs to clean up SA pull secrets

2d3371c

Update the service account pull secret controller to add owner references to generated pull secrets. This ensures pull secrets are deleted when the associated token is deleted.

adambkaplan force-pushed the pull-secret-use-ownerrefs branch from 1c3dff5 to 2d3371c Compare February 24, 2020 13:54

adambkaplan changed the title ~~WIP - Bug 1765294: Use OwnerRefs to clean up SA pull secrets~~ Bug 1765294: Use OwnerRefs to clean up SA pull secrets Feb 24, 2020

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 Feb 24, 2020

adambkaplan commented Feb 24, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 25, 2020

openshift-merge-robot merged commit 5306fa2 into openshift:master Feb 25, 2020

openshift-cherrypick-robot mentioned this pull request Feb 25, 2020

[release-4.4] Bug 1806792: Use OwnerRefs to clean up SA pull secrets #67

Merged

openshift-cherrypick-robot mentioned this pull request Feb 25, 2020

[release-4.3] Bug 1779282: Use OwnerRefs to clean up SA pull secrets #68

Closed

smarterclayton mentioned this pull request Mar 26, 2020

Revert "Bug 1765294: Use OwnerRefs to clean up SA pull secrets" #80

Closed

Conversation

adambkaplan commented Jan 29, 2020

Uh oh!

openshift-ci-robot commented Jan 29, 2020

Uh oh!

adambkaplan commented Jan 29, 2020

Uh oh!

adambkaplan commented Jan 30, 2020

Uh oh!

adambkaplan commented Jan 30, 2020

Uh oh!

bparees Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bparees commented Jan 30, 2020

Uh oh!

Uh oh!

adambkaplan commented Jan 31, 2020

Uh oh!

adambkaplan commented Feb 3, 2020

Uh oh!

adambkaplan commented Feb 4, 2020

Uh oh!

adambkaplan commented Feb 5, 2020

Uh oh!

adambkaplan commented Feb 6, 2020

Uh oh!

adambkaplan commented Feb 11, 2020

Uh oh!

bparees commented Feb 11, 2020

Uh oh!

adambkaplan commented Feb 13, 2020

Uh oh!

mfojtik Feb 13, 2020

Choose a reason for hiding this comment

Uh oh!

adambkaplan Feb 20, 2020

Choose a reason for hiding this comment

Uh oh!

adambkaplan commented Feb 20, 2020

Uh oh!

adambkaplan left a comment

Choose a reason for hiding this comment

Uh oh!

adambkaplan commented Feb 25, 2020

Uh oh!

bparees commented Feb 25, 2020

Uh oh!

openshift-ci-robot commented Feb 25, 2020

Uh oh!

openshift-ci-robot commented Feb 25, 2020

Uh oh!

adambkaplan commented Feb 25, 2020

Uh oh!

openshift-cherrypick-robot commented Feb 25, 2020

Uh oh!

adambkaplan commented Feb 25, 2020

Uh oh!

openshift-cherrypick-robot commented Feb 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants