Make dockercfg generation wait for the token secret using shared informer #11394

mfojtik · 2016-10-17T14:32:11Z

Fixes: #11333
Should fix: https://bugzilla.redhat.com/show_bug.cgi?id=1374091

mfojtik · 2016-10-17T14:34:21Z

[test]

mfojtik · 2016-10-17T14:34:38Z

@liggitt @deads2k PTAL

mfojtik · 2016-10-18T06:47:00Z

[test]

flake: #11381

mfojtik · 2016-10-18T06:47:07Z

@Kargakis ptal as well

0xmichalis · 2016-10-18T09:39:46Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

@@ -85,6 +90,8 @@ func NewDockercfgController(cl client.Interface, options DockercfgControllerOpti
 	e.serviceAccountCache = NewEtcdMutationCache(serviceAccountCache)
 	e.syncHandler = e.syncServiceAccount

+	e.addSecretInformer(options)


I don't think this deserves its own method. Inline the informer here.

Or if others like it, we should probably do the same for the sa informer.

i can move it up, was trying to keep the new function small

0xmichalis · 2016-10-18T09:41:27Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

+func (e *DockercfgController) addSecretInformer(options DockercfgControllerOptions) {
+	saTokenSelector := fields.OneTermEqualSelector(api.SecretTypeField, string(api.SecretTypeServiceAccountToken))
+
+	_, e.secretController = framework.NewInformer(


Should be Indexed

0xmichalis · 2016-10-18T09:43:21Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go


 	queue workqueue.RateLimitingInterface

 	// syncHandler does the work. It's factored out for unit testing
 	syncHandler func(serviceKey string) error
 }

+func (e *DockercfgController) secretTokenAvailable(_, newObj interface{}) {


0xmichalis · 2016-10-18T09:50:21Z

Needs a unit test

deads2k · 2016-10-18T12:59:58Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go


 	queue workqueue.RateLimitingInterface

 	// syncHandler does the work. It's factored out for unit testing
 	syncHandler func(serviceKey string) error
 }

+func (e *DockercfgController) secretTokenAvailable(_, newObj interface{}) {


This can happen on an Add instead of on a Update if your controller restarts after having been off. See kubernetes/kubernetes#32778

AddFunc added

deads2k · 2016-10-18T13:01:10Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

+	if _, isPending := secret.Annotations[PendingDockercfgNameKey]; !isPending {
+		return
+	}
+	if data, exists := secret.Data[api.ServiceAccountTokenKey]; exists && len(data) > 0 {


I don't think this logic should run synchronously.

can you elaborate?

ok, I think I see what you mean... when I create the dockerPullSecret and now observing the "addFunc", we should detect that and call addSecretToServiceAccount.

liggitt · 2016-10-18T14:54:38Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

@@ -312,50 +392,18 @@ func (e *DockercfgController) createTokenSecret(serviceAccount *api.ServiceAccou
 			Annotations: map[string]string{
 				api.ServiceAccountNameKey: serviceAccount.Name,
 				api.ServiceAccountUIDKey:  string(serviceAccount.UID),
+				PendingDockercfgNameKey:   "true",


why add a pending annotation we have to remove? why not just annotate this to indicate it was created by / owned by this controller, and we can leave the annotation in place? we can tell if a token is pending by the lack of a token in the data map

why add a pending annotation we have to remove? why not just annotate this to indicate it was created by / owned by this controller, and we can leave the annotation in place? we can tell if a token is pending by the lack of a token in the data map

Ah controllerRef, how difficult you are. It's owned by this controller, not the controller that fills in the secret? It's owned by both? It's an ownerRef pointing to an object that doesn't exist?

@liggitt agree, I don't think we need any annotation as we can just check if the state is pending by looking into secret data map.

@deads2k @liggitt I rewrote that add/update logic so the annotation is no longer needed and is is much cleaner now.

ok, I was wrong :-) we need that annotation to make sure the pull secret is not created everytime the "handleSecrets" function run, so we don't end up with:

@dev] .../openshift/origin # oc get secrets -n test NAME TYPE DATA AGE builder-dockercfg-0sk6b kubernetes.io/dockercfg 1 1m builder-dockercfg-8uao3 kubernetes.io/dockercfg 1 1m builder-dockercfg-c35kr kubernetes.io/dockercfg 1 1m builder-dockercfg-n1vzw kubernetes.io/dockercfg 1 1m builder-dockercfg-oiv6v kubernetes.io/dockercfg 1 1m builder-dockercfg-qpyi1 kubernetes.io/dockercfg 1 1m builder-dockercfg-tefb1 kubernetes.io/dockercfg 1 1m

actually I found that this is because I also generate a lot of tokens (which I think is because changes to syncServiceAccount

nvmd. pebkac... I had wrong field selector in the shared informer...

mfojtik · 2016-10-19T14:16:48Z

@deads2k @liggitt updated and ready for next review :-)

liggitt · 2016-10-19T15:19:22Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

 	mountableDockercfgSecrets, imageDockercfgPullSecrets := getGeneratedDockercfgSecretNames(serviceAccount)

 	// look for an ImagePullSecret in the form
-	if len(imageDockercfgPullSecrets) > 0 && len(mountableDockercfgSecrets) > 0 {
+	if len(imageDockercfgPullSecrets) > 0 || len(mountableDockercfgSecrets) > 0 {


revert this... needsDockercfgSecret means we have to do work on the service account, even if that work is just copying an existing dockercfg from one list to the other

@liggitt i think then I will need new method to ensure the dockercfg was created already

liggitt · 2016-10-19T16:00:13Z

this is turning into a rewrite of the controller, which is not what we want

track the tokens we've created that are waiting to be populated in a local variable
watch service account token secrets

when we decide we need a token for a service account:

if we are already waiting for a token and it is not yet populated, return early
if we are already waiting for a token and it is populated, continue to build and associate the dockercfg
if we are not already waiting for a token, create one and add it to the list we're waiting for

when we see an update to a token in the list we're waiting for, and it is populated, sync the service account again (will trigger case 2 above)

when we see a token get deleted, if it is in the list we're waiting for, remove it from the list and sync the service account again (will trigger case 3 above)

mfojtik · 2016-10-19T16:10:08Z

@liggitt i think this PR is doing what you're suggesting, I don't know if it is possible without rewriting the controller.

liggitt · 2016-10-19T16:54:54Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

-		return err
-	}
-
-	dockercfgSecret, err := e.createDockerPullSecret(serviceAccount)


I would expect the only change to syncServiceAccount to be to short-circuit if createDockerPullSecret says it didn't create the secret:

dockercfgSecret, created, err := e.createDockerPullSecret(serviceAccount) if err != nil { return err } if !created { return nil }

will the later population of the token trigger the service account resync?

exactly, that's why you need the list of token secrets you're waiting on. the secret watcher will resync the relevant service account if one of those secrets gets populated or deleted

liggitt · 2016-10-19T17:07:15Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

 }

 // createDockerPullSecret creates a dockercfg secret based on the token secret
-func (e *DockercfgController) createDockerPullSecret(serviceAccount *api.ServiceAccount) (*api.Secret, error) {
+func (e *DockercfgController) createDockerPullSecret(tokenSecret *api.Secret, serviceAccount *api.ServiceAccount) error {


and the only changes to createDockerPullSecret would be to short-circuit if createTokenSecret says it didn't create a populated secret:

tokenSecret, isPopulated, err := e.createTokenSecret(serviceAccount) if err != nil { return nil, false, err } if !isPopulated { return nil, false, nil }

and to only return true for created if it makes it to the end of the function

liggitt · 2016-10-19T17:08:09Z

all the logic changes should be able to be contained to the secret watcher (which should only be watching service account token secrets, and needs to handle secret deletion), and the createTokenSecret function

mfojtik · 2016-10-19T19:34:41Z

@liggitt the token secret deletion is handled already in separate controller, do we want to get rid of that controller?

liggitt · 2016-10-19T20:02:19Z

@liggitt the token secret deletion is handled already in separate controller, do we want to get rid of that controller?

no, this observer's job is not to go clean up the dockercfg, it is to react to population and deletion of pending-population tokens

liggitt · 2016-10-22T01:42:42Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

+	}
+	serviceAccountName, _ := secret.Annotations[api.ServiceAccountNameKey]
+	serviceAccountKey := makeServiceAccountKey(secret.Namespace, serviceAccountName)
+	obj, exists, err := e.serviceAccountCache.GetByKey(serviceAccountKey)


rather than looking up the service account in the cache, just add to the queue directly?

e.queue.Add(serviceAccountKey)

liggitt · 2016-10-22T01:43:17Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

+			return
+		}
+	}
+	if createdBy := secret.Annotations[api.CreatedByAnnotation]; createdBy != CreateDockercfgSecretsController {


if secret.Annotations[api.CreatedByAnnotation] != CreateDockercfgSecretsController

liggitt · 2016-10-22T01:46:48Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

+	serviceAccount := obj.(*api.ServiceAccount)
+	// The secret is now available in the cache so remove it from pendingTokens
+	e.pendingTokens.Delete(serviceAccount)
+	if len(secret.Data[api.ServiceAccountTokenKey]) > 0 {


should we only re-queue if we are observing the update where the old secret is missing the token and the new secret has it?

liggitt · 2016-10-22T01:47:48Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

+	if len(secret.Data[api.ServiceAccountTokenKey]) > 0 {
+		return
+	}
+	serviceAccountName, _ := secret.Annotations[api.ServiceAccountNameKey]


enqueue the key directly

liggitt · 2016-10-22T01:48:36Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

+	}
+	for _, obj := range secrets {
+		secret := obj.(*api.Secret)
+		if createdBy := secret.Annotations[api.CreatedByAnnotation]; createdBy == CreateDockercfgSecretsController {


if secret.Annotations[api.CreatedByAnnotation] == CreateDockercfgSecretsController

liggitt · 2016-10-22T01:53:43Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

-func (e *DockercfgController) createTokenSecret(serviceAccount *api.ServiceAccount) (*api.Secret, error) {
-	tokenSecret := &api.Secret{
+func (e *DockercfgController) createTokenSecret(serviceAccount *api.ServiceAccount) (*api.Secret, bool, error) {
+	tokenSecret, isPending, err := e.getTokenSecretFromCache(serviceAccount)


pretty sure looking up the token from cache here is a race:

service account exists, token exists, dockercfg exists, serviceaccount has reference, caches are populated

token gets deleted

deleted_token_secrets.go deletes the dockercfg in response

deleted_dockercfg_secrets.go removes the reference from the service account in response

service account update triggers this controller to resync the service account. this controller's token cache could still show the populated token that was deleted in step 2, and it would re-create a dockercfg secret now containing a defunct token and add it to the serviceaccount

@liggitt should we always do a live query then to get the live token?

live lookups don't do what you'd expect... I think we can do a minimal change in the state machine direction without a complete rewrite

liggitt · 2016-10-24T13:42:32Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

 		return err
 	}
+	if !created {
+		glog.V(2).Infof("The dockercfg secret was not created for service account %s/%s, will retry", serviceAccount.Namespace, serviceAccount.Name)


lower verbosity to 5... this is going to happen every time now

liggitt · 2016-10-24T13:43:16Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

 	if err != nil {
+		glog.V(2).Infof("Error during adding the dockercfg secret to %s/%s: %v", serviceAccount.Namespace, serviceAccount.Name, err)


no need to log here, error gets logged at top level on error returns

liggitt · 2016-10-24T13:46:39Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

-	_, err := e.client.Secrets(tokenSecret.Namespace).Create(tokenSecret)
-	if err != nil {
-		return nil, err
+	glog.V(2).Infof("Creating token secret %q for service account %s/%s", tokenSecret.Name, serviceAccount.Namespace, serviceAccount.Name)


liggitt · 2016-10-24T13:47:58Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

+		return nil, false, err
+	}
+	if !isPopulated {
+		glog.V(4).Infof("Token secret for service account %s/%s is not populated yet", serviceAccount.Namespace, serviceAccount.Name)


mfojtik · 2016-10-24T14:11:08Z

@liggitt log levels adjusted, manually tested, lets give it one more [test] run

liggitt · 2016-10-24T14:41:04Z

LGTM but I'd like a second-check from @deads2k

0xmichalis · 2016-10-24T15:43:07Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

+	// attached to all token secrets this controller create
+	CreateDockercfgSecretsController = "openshift.io/create-dockercfg-secrets"
+
+	// PendingTokenAnnotation


Missing comment

0xmichalis · 2016-10-24T15:44:04Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

 	e.serviceAccountCache = NewEtcdMutationCache(serviceAccountCache)
+
+	tokenSecretSelector := fields.OneTermEqualSelector(api.SecretTypeField, string(api.SecretTypeServiceAccountToken))
+	e.secretCache, e.secretController = framework.NewInformer(


Don't you need this to be indexed now?

not anymore

Secrets for a service account are always in the same namespace. So it seems that you can index by namespace.

No need, we're looking up by key only (implicit index)

deads2k · 2016-10-24T19:31:36Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

 	}
 	dockercfgSecret.Data[api.DockerConfigKey] = dockercfgContent

 	// Save the secret
 	createdSecret, err := e.client.Secrets(tokenSecret.Namespace).Create(dockercfgSecret)
 	if err != nil {
 		// Clean up the generated token secret if we're not going to use it
-		glog.V(2).Infof("deleting unused token secret %s/%s, error creating dockercfgSecret: %v", tokenSecret.Namespace, tokenSecret.Name, err)
+		glog.V(2).Infof("Deleting unused token secret %s/%s, error creating dockercfgSecret: %v", tokenSecret.Namespace, tokenSecret.Name, err)
 		if deleteErr := e.client.Secrets(tokenSecret.Namespace).Delete(tokenSecret.Name); (deleteErr != nil) && !kapierrors.IsNotFound(deleteErr) {


Under what conditions is this still necessary? Since we have the name of the token secret we created, we ought to be able to use it to back the docker pull secret instead of creating another one right?

yes, I will remove

deads2k · 2016-10-24T19:33:08Z

One questions/comment. lgtm otherwise.

mfojtik · 2016-10-25T08:00:00Z

@deads2k @liggitt removed the deletion of secret token [merge]

openshift-bot · 2016-10-25T08:05:29Z

Evaluated for origin test up to 6744424

openshift-bot · 2016-10-25T08:05:40Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10599/) (Image: devenv-rhel7_5239)

openshift-bot · 2016-10-25T08:05:41Z

Evaluated for origin merge up to 6744424

openshift-bot · 2016-10-25T09:24:08Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10599/) (Base Commit: 66847bb)

mfojtik mentioned this pull request Oct 17, 2016

WIP: Switch to use watch.Until to wait for dockercfg token #11356

Closed

mfojtik changed the title ~~WIP: serviceaccount: add secret informer to create_dockercfg_secret~~ serviceaccount: add secret informer to create_dockercfg_secret Oct 18, 2016

mfojtik changed the title ~~serviceaccount: add secret informer to create_dockercfg_secret~~ Make dockercfg generation wait for the token secret using shared informer Oct 18, 2016

0xmichalis reviewed Oct 18, 2016

View reviewed changes

deads2k reviewed Oct 18, 2016

View reviewed changes

liggitt reviewed Oct 18, 2016

View reviewed changes

mfojtik force-pushed the dockercfg-controller-2 branch 4 times, most recently from b9e07fa to 1bd2e78 Compare October 19, 2016 14:14

liggitt reviewed Oct 19, 2016

View reviewed changes

mfojtik force-pushed the dockercfg-controller-2 branch from 1bd2e78 to 00e07a6 Compare October 19, 2016 15:50

liggitt reviewed Oct 19, 2016

View reviewed changes

mfojtik force-pushed the dockercfg-controller-2 branch 2 times, most recently from ffa95f6 to 906c616 Compare October 20, 2016 12:26

liggitt reviewed Oct 22, 2016

View reviewed changes

mfojtik force-pushed the dockercfg-controller-2 branch 2 times, most recently from b237014 to 5a7903f Compare October 24, 2016 13:38

liggitt reviewed Oct 24, 2016

View reviewed changes

mfojtik force-pushed the dockercfg-controller-2 branch from 5a7903f to 87e625e Compare October 24, 2016 14:10

0xmichalis reviewed Oct 24, 2016

View reviewed changes

mfojtik force-pushed the dockercfg-controller-2 branch from 87e625e to 26e69cf Compare October 24, 2016 16:31

deads2k reviewed Oct 24, 2016

View reviewed changes

serviceaccount: add secret informer to create_dockercfg_secret

6744424

mfojtik force-pushed the dockercfg-controller-2 branch from 26e69cf to 6744424 Compare October 25, 2016 07:59

openshift-bot merged commit 1f62556 into openshift:master Oct 25, 2016

deads2k mentioned this pull request Oct 26, 2016

test-cmd flake: quota.sh:32: executing 'oc describe appliedclusterresourcequota/for-deads-by-annotation -n bar --as deads' #11560

Closed

mfojtik deleted the dockercfg-controller-2 branch September 5, 2018 21:07

		if err != nil {
		glog.V(2).Infof("Error during adding the dockercfg secret to %s/%s: %v", serviceAccount.Namespace, serviceAccount.Name, err)

Make dockercfg generation wait for the token secret using shared informer #11394

Make dockercfg generation wait for the token secret using shared informer #11394

Conversation

mfojtik commented Oct 17, 2016 • edited Loading

mfojtik commented Oct 17, 2016

mfojtik commented Oct 17, 2016

mfojtik commented Oct 18, 2016

mfojtik commented Oct 18, 2016

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

Choose a reason for hiding this comment

0xmichalis commented Oct 18, 2016

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 Oct 18, 2016 • edited Loading

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

mfojtik commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Oct 19, 2016 • edited Loading

mfojtik commented Oct 19, 2016

liggitt Oct 19, 2016 • edited Loading

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

liggitt commented Oct 19, 2016 • edited Loading

mfojtik commented Oct 19, 2016

liggitt commented Oct 19, 2016

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

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

liggitt Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented Oct 24, 2016

liggitt commented Oct 24, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Oct 24, 2016

mfojtik commented Oct 25, 2016

openshift-bot commented Oct 25, 2016

openshift-bot commented Oct 25, 2016 • edited Loading

openshift-bot commented Oct 25, 2016

openshift-bot commented Oct 25, 2016

mfojtik commented Oct 17, 2016 •

edited

Loading

deads2k Oct 18, 2016 •

edited

Loading

liggitt commented Oct 19, 2016 •

edited

Loading

liggitt Oct 19, 2016 •

edited

Loading

liggitt commented Oct 19, 2016 •

edited

Loading

liggitt Oct 24, 2016 •

edited

Loading

openshift-bot commented Oct 25, 2016 •

edited

Loading