OCPBUGS-5744: dockercfg: use tokenrequest instead of SA secrets #223

stlaz · 2022-05-06T19:43:22Z

this PR makes use of the TokenRequest API to get a token for the dockercfg SA secret rather than creating a secret and polluting etcd for that very same purpose,

openshift-ci · 2022-05-06T19:45:08Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: stlaz
To complete the pull request process, please assign mfojtik after the PR has been reviewed.
You can assign the PR to them by writing /assign @mfojtik in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

stlaz · 2022-05-07T06:03:04Z

This also needs to implement token refresh.

Possible issue: we need to rotate the token inside the secret, is the change in the secret propagated in the pod?

s-urbaniak · 2022-05-09T10:33:53Z

Possible issue: we need to rotate the token inside the secret, is the change in the secret propagated in the pod?

generally, yes: https://kubernetes.io/docs/concepts/configuration/secret/#mounted-secrets-are-updated-automatically. But the pod is responsible for re-reading the value, either using a time based approach or by fsnotify.

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

stlaz · 2022-06-01T11:16:23Z

/retest
should be showing better test pass rate as creating tokens should now (master is kube 1.24) be allowed

stlaz · 2022-06-03T11:22:44Z

/retest

stlaz · 2022-06-16T12:53:40Z

/retest

deads2k · 2022-07-21T12:11:40Z

pkg/serviceaccounts/controllers/deleted_token_secrets.go

-}
-
-// NewDockercfgTokenDeletedController returns a new *DockercfgTokenDeletedController.
-func NewDockercfgTokenDeletedController(secrets informers.SecretInformer, cl kclientset.Interface, options DockercfgTokenDeletedControllerOptions) *DockercfgTokenDeletedController {


Without this controller, what deletes the token secrets when the dockercfg secret is deleted later on?

Is there an ownerref these days perhaps?

deads2k · 2022-07-21T12:12:20Z

pkg/serviceaccounts/controllers/docker_registry_service.go

@@ -138,7 +138,7 @@ type DockerRegistryServiceController struct {
 }

 // Runs controller loops and returns immediately
-func (e *DockerRegistryServiceController) Run(workers int, stopCh <-chan struct{}) {
+func (e *DockerRegistryServiceController) Run(ctx context.Context, workers int) {


pull stuff like this which are maintenance refactors without intent changes into a separate PR for easier review and merge

deads2k · 2022-07-21T12:12:59Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

-	ServiceAccountTokenSecretNameKey = "openshift.io/token-secret.name"
-	MaxRetriesBeforeResync           = 5
+	MaxRetriesBeforeResync = 5
+	ExpirationCheckPeriod  = 10 * time.Minute


if you make me look at a constant, I'll ask for godoc ;)

deads2k · 2022-07-21T12:13:24Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

 	// token data population
-	PendingTokenAnnotation = "openshift.io/create-dockercfg-secrets.pending-token"
+	PendingTokenAnnotation = "openshift.io/create-dockercfg-secrets.pending-secret"


a change here looks suspicious. I would expect a new constant instead.

deads2k · 2022-07-21T12:21:02Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

-		if !exists {
-			continue
+		serviceAccount.Annotations[PendingTokenAnnotation] = pendingTokenName
+		updatedServiceAccount, err := e.client.CoreV1().ServiceAccounts(serviceAccount.Namespace).Update(ctx, serviceAccount, metav1.UpdateOptions{})


I recommend using the apply client for this. You can do it as a separate step, but it will reduce conflicts.

I was considering patch when looking through the code, this helped with with other controllers in the past.

deads2k · 2022-07-21T12:22:10Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

+		klog.V(4).Infof("Creating dockercfg secret %q for service account %s/%s", dockercfgSecret.Name, serviceAccount.Namespace, serviceAccount.Name)
+
+		// Save the secret
+		_, err = e.client.CoreV1().Secrets(serviceAccount.Namespace).Create(ctx, dockercfgSecret, metav1.CreateOptions{})


why create before getting the content? With the tokenrequest API, the call to get the content is synchronous and it would simplify some of the waiting logic, right?

EDIT: OH! it's because you need the secret for the bound token to bind to, isn't it? Still that's fast, (api calls, not controller response time), so you could do a secret create, a tokenrequest, a secret apply (avoid the conflict), and an SA update.

deads2k · 2022-07-21T12:24:45Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

 }

-func (c *DockercfgController) waitForDockerURLs(ready chan<- struct{}, stopCh <-chan struct{}) {
+func (c *DockercfgController) waitForDockerURLs(ctx context.Context, ready chan<- struct{}) {


separate from this PR (before or after), I wonder if these are still logically configurable. They used to be cyclical with the rest of the controller-manager so I built this logic, but I wonder if on openshift these are now practically fixed in a way that the names can be predicted during installation and reacted to if they ever change to avoid the interlock and self-discovery logic.

Just imagining future you trying to maintain this area of code.

deads2k · 2022-07-21T12:25:55Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

+	pendingTokenName := serviceAccount.Annotations[PendingTokenAnnotation]
+
+	// If this service account has no record of a pending token name, record one
+	if len(pendingTokenName) == 0 {


If you can build a valid dockerconfig secret synchronously, you'd be able to first build the secret and second directly update the serviceaccount with the pull secret and never need the intermediate annotation, right?

deads2k · 2022-07-21T12:35:04Z

Just so I'm clear, this is about how we handle new dockercfg secrets, but how do we migrate existing ones and how do existing ones delete the old token secret and use a new tokenrequest based token instead?

soltysh · 2022-07-27T14:47:46Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

 				default:
 					utilruntime.HandleError(fmt.Errorf("object passed to %T that is not expected: %T", e, obj))
 					return false
 				}
 			},
 			Handler: cache.ResourceEventHandlerFuncs{
+				// We don't need to react to secret deletes, the deleted_dockercfg_secrets controller does that
+				// It also updates the SA so we will eventually get back to creating a new secret


Since you already use FilteringResoruceEventHandler it's sufficient for you to always e.enqueuSecret(secret)

soltysh · 2022-07-27T14:48:44Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

 }

-func (e *DockercfgController) enqueueServiceAccountForToken(tokenSecret *v1.Secret) {
+func (e *DockercfgController) enqueueServiceAccountForToken(dockerCfgSecret *v1.Secret) {


This looks like it's not being used anywhere.

soltysh · 2022-07-27T14:59:29Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

+		dockercfg[dockerURL] = credentialprovider.DockerConfigEntry{
+			Username: "serviceaccount",
+			Password: string(saToken),
+			Email:    "serviceaccount@example.org",


The email shouldn't be needed, looking at my own .docker/config.json so I'd suggest dropping it.

soltysh · 2022-07-27T17:02:21Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

-		if !exists {
-			continue
+		serviceAccount.Annotations[PendingTokenAnnotation] = pendingTokenName
+		updatedServiceAccount, err := e.client.CoreV1().ServiceAccounts(serviceAccount.Namespace).Update(ctx, serviceAccount, metav1.UpdateOptions{})


I was considering patch when looking through the code, this helped with with other controllers in the past.

openshift-merge-robot · 2022-10-20T18:21:08Z

@stlaz: PR needs rebase.

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 · 2022-10-20T18:25:23Z

@stlaz: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/openshift-e2e-aws-builds-techpreview	`bcb0404`	link	false	`/test openshift-e2e-aws-builds-techpreview`
ci/prow/e2e-gcp	`bcb0404`	link	true	`/test e2e-gcp`
ci/prow/e2e-aws-proxy	`bcb0404`	link	false	`/test e2e-aws-proxy`
ci/prow/e2e-aws	`bcb0404`	link	true	`/test e2e-aws`
ci/prow/e2e-gcp-builds	`bcb0404`	link	true	`/test e2e-gcp-builds`
ci/prow/e2e-gcp-ovn-builds	`bcb0404`	link	true	`/test e2e-gcp-ovn-builds`
ci/prow/e2e-aws-ovn-upgrade	`bcb0404`	link	true	`/test e2e-aws-ovn-upgrade`
ci/prow/e2e-gcp-ovn	`bcb0404`	link	true	`/test e2e-gcp-ovn`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

openshift-bot · 2023-04-19T01:00:33Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

stlaz · 2023-04-20T13:00:34Z

/remove-lifecycle stale
I might be able to get to this soon-ish

openshift-bot · 2023-07-20T01:01:08Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-bot · 2023-08-19T08:30:59Z

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

openshift-bot · 2023-09-19T00:00:15Z

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

openshift-ci · 2023-09-19T00:01:15Z

@openshift-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

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 · 2023-09-19T00:01:15Z

@stlaz: An error was encountered updating to the NEW state for bug OCPBUGS-5744 on the Jira server at https://issues.redhat.com/. No known errors were detected, please see the full error message for details.

Full error message.


No transition status with name `NEW` could be found. Please select from the following list: [Replan]

Please contact an administrator to resolve this issue, then request a bug refresh with /jira refresh.

In response to this:

this PR makes use of the TokenRequest API to get a token for the dockercfg SA secret rather than creating a secret and polluting etcd for that very same purpose,

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 bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 6, 2022

openshift-ci bot requested review from bparees and smarterclayton May 6, 2022 19:45

mfojtik reviewed May 10, 2022

View reviewed changes

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go Outdated Show resolved Hide resolved

mfojtik reviewed May 10, 2022

View reviewed changes

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go Outdated Show resolved Hide resolved

mfojtik reviewed May 10, 2022

View reviewed changes

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go Outdated Show resolved Hide resolved

stlaz force-pushed the token_request_dockercfg branch 3 times, most recently from 4889e36 to f0c1de4 Compare May 24, 2022 14:05

stlaz force-pushed the token_request_dockercfg branch from f0c1de4 to 2c53e05 Compare June 6, 2022 13:13

stlaz force-pushed the token_request_dockercfg branch from 00ecff4 to 7f552c7 Compare June 13, 2022 13:23

xingxingxia mentioned this pull request Jun 16, 2022

OCP 4.11 Release Notes Tracker openshift/openshift-docs#43249

Closed

stlaz force-pushed the token_request_dockercfg branch from 7f552c7 to 7f9aaf0 Compare June 16, 2022 12:50

update go to 1.18 and unify Dockerfiles

d18ffcb

stlaz force-pushed the token_request_dockercfg branch 2 times, most recently from 61bf205 to 4309267 Compare June 20, 2022 11:45

stlaz changed the title ~~wip: dockercfg: use tokenrequest instead of SA secrets~~ dockercfg: use tokenrequest instead of SA secrets Jun 20, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 20, 2022

stlaz added 3 commits June 29, 2022 11:17

dockercfg: use tokenrequest instead of SA secrets

10ef35b

dockercfg: wire contexts where possible

926b337

actually process all the items in the queue per wrapping Wait tick

bcb0404

stlaz force-pushed the token_request_dockercfg branch from a1f5947 to bcb0404 Compare June 29, 2022 09:18

deads2k reviewed Jul 21, 2022

View reviewed changes

soltysh reviewed Jul 27, 2022

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2022

mfojtik changed the title ~~dockercfg: use tokenrequest instead of SA secrets~~ Bug OCPBUGS-5744: dockercfg: use tokenrequest instead of SA secrets Jan 11, 2023

stlaz changed the title ~~Bug OCPBUGS-5744: dockercfg: use tokenrequest instead of SA secrets~~ OCPBUGS-5744: dockercfg: use tokenrequest instead of SA secrets Jan 18, 2023

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 19, 2023

openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 20, 2023

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 20, 2023

openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 19, 2023

openshift-ci bot closed this Sep 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-5744: dockercfg: use tokenrequest instead of SA secrets #223

OCPBUGS-5744: dockercfg: use tokenrequest instead of SA secrets #223

stlaz commented May 6, 2022

openshift-ci bot commented May 6, 2022

stlaz commented May 7, 2022 •

edited

s-urbaniak commented May 9, 2022

stlaz commented Jun 1, 2022

stlaz commented Jun 3, 2022

stlaz commented Jun 16, 2022

deads2k Jul 21, 2022 •

edited

deads2k Jul 21, 2022

deads2k Jul 21, 2022

deads2k Jul 21, 2022

deads2k Jul 21, 2022

soltysh Jul 27, 2022

deads2k Jul 21, 2022 •

edited

deads2k Jul 21, 2022

deads2k Jul 21, 2022

deads2k commented Jul 21, 2022

soltysh Jul 27, 2022

soltysh Jul 27, 2022

soltysh Jul 27, 2022

soltysh Jul 27, 2022

openshift-merge-robot commented Oct 20, 2022

openshift-ci bot commented Oct 20, 2022

openshift-bot commented Apr 19, 2023

stlaz commented Apr 20, 2023

openshift-bot commented Jul 20, 2023

openshift-bot commented Aug 19, 2023

openshift-bot commented Sep 19, 2023

openshift-ci bot commented Sep 19, 2023

openshift-ci-robot commented Sep 19, 2023

OCPBUGS-5744: dockercfg: use tokenrequest instead of SA secrets #223

OCPBUGS-5744: dockercfg: use tokenrequest instead of SA secrets #223

Conversation

stlaz commented May 6, 2022

openshift-ci bot commented May 6, 2022

stlaz commented May 7, 2022 • edited

s-urbaniak commented May 9, 2022

stlaz commented Jun 1, 2022

stlaz commented Jun 3, 2022

stlaz commented Jun 16, 2022

deads2k Jul 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k Jul 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jul 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-merge-robot commented Oct 20, 2022

openshift-ci bot commented Oct 20, 2022

openshift-bot commented Apr 19, 2023

stlaz commented Apr 20, 2023

openshift-bot commented Jul 20, 2023

openshift-bot commented Aug 19, 2023

openshift-bot commented Sep 19, 2023

openshift-ci bot commented Sep 19, 2023

openshift-ci-robot commented Sep 19, 2023

stlaz commented May 7, 2022 •

edited

deads2k Jul 21, 2022 •

edited

deads2k Jul 21, 2022 •

edited