Rework DockercfgTokenDeletedController by enj · Pull Request #13579 · openshift/origin

enj · 2017-03-30T01:24:07Z

This works for me locally but needs an integration test.

[test]

Signed-off-by: Monis Khan mkhan@redhat.com

Trello xref: https://trello.com/c/ZtOfFpFz

Signed-off-by: Monis Khan <mkhan@redhat.com>

deads2k · 2017-03-30T11:52:32Z

pkg/cmd/server/admin/create_bootstrappolicy_file.go

 		policyTemplate.Objects = append(policyTemplate.Objects, versionedObject)
 	}

+	controllerRoles := bootstrappolicy.GetBootstrapControllerRoles()


Is this method used anywhere? I seem recall thinking that it should die.

@liggitt I like the upstream, reconcile on start combined with system:masters authorized separately which obviated the need for an external policy.json. You see any reason to keep creating the "BootstrapPolicyFile"?

deads2k · 2017-03-30T11:54:48Z

pkg/cmd/server/bootstrappolicy/controller_policy.go

+
+var (
+	// controllerRoles is a slice of roles used for controllers
+	controllerRoles = []authorizationapi.ClusterRole{}


You'll want a clusterrole local role distinction. Found that out upstream.

deads2k · 2017-03-30T11:56:01Z

pkg/cmd/server/bootstrappolicy/controller_policy.go

+	controllerRoleBindings = []authorizationapi.ClusterRoleBinding{}
+)
+
+func addControllerRole(role authorizationapi.ClusterRole) {


No new openshift roles in our code here. Let's use the rbac roles and our converter. It will make future unification easier.

deads2k · 2017-03-30T12:05:50Z

pkg/cmd/server/origin/run_components.go

+			CoreClient:   c.PrivilegedLoopbackKubernetesClientset.Core(),
+			Namespace:    bootstrappolicy.DefaultOpenShiftInfraNamespace,
+		},
+		Stop: make(chan struct{}),


This doesn't look. You didn't anonymously include the kube one, did you?

deads2k · 2017-03-30T12:07:29Z

pkg/cmd/server/origin/run_components.go

 	serviceaccountcontrollers.NewDockercfgDeletedController(c.KubeClientset(), serviceaccountcontrollers.DockercfgDeletedControllerOptions{}).Run()
-	serviceaccountcontrollers.NewDockercfgTokenDeletedController(c.KubeClientset(), serviceaccountcontrollers.DockercfgTokenDeletedControllerOptions{}).Run()
+
+	ctx := origincontroller.ControllerContext{


This block has the right idea. Please pull it out into a method which constructs the context and initializers and starts them all. Having only one in the map (as you have now) is perfect.

deads2k · 2017-03-30T12:10:29Z

pkg/controller/controllermanager.go

+	DockercfgTokenDeletedControllerName = "dockercfg-token-deleted-controller"
+)
+
+type ControllerContext struct {


Add some comments explaining why we have a different context. I think I see why you've done it now. Different informer factory, no options, and no use for avaialbleResources?

deads2k · 2017-03-30T12:14:50Z

Relatively minor comments. This is a good start. I suspect that we'll end up swizzling packages around to adjust for this.

Signed-off-by: Monis Khan <mkhan@redhat.com>

enj · 2017-03-31T21:48:28Z

[test]

openshift-bot · 2017-03-31T21:53:20Z

Evaluated for origin test up to 2d48afa

openshift-bot · 2017-04-01T00:04:32Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/524/) (Base Commit: 645cac4)

enj · 2017-04-01T01:18:43Z

@deads2k the integration test I wrote seems to flake every so often locally. My best guess is kubernetes/kubernetes#41661 which we should get in the rebase?

liggitt · 2017-04-01T01:26:50Z

kubernetes/kubernetes#41661 doesn't fix flakes, it fixes DOA clusters started after changing token signing keys while leaving invalid tokens in etcd

enj · 2017-04-03T17:47:43Z

Here is what I have gathered from the logs:

ClientBuilder creates DockercfgTokenDeletedController's SA
DockercfgController makes dockercfg token for DockercfgTokenDeletedController's SA
TokensController makes JWT token for DockercfgTokenDeletedController's SA
DockercfgController and TokensController try to update DockercfgTokenDeletedController's SA to reference their respective secrets at the same time
DockercfgController wins and updates DockercfgTokenDeletedController's SA
ClientBuilder sees JWT token for DockercfgTokenDeletedController's SA and uses it to make a client for DockercfgTokenDeletedController
DockercfgTokenDeletedController starts with JWT token for DockercfgTokenDeletedController's SA
TokensController deletes JWT token for DockercfgTokenDeletedController's SA because it got a conflict error during the update
DockercfgTokenDeletedController no longer functions

@deads2k @liggitt some questions:

What is the expected behavior for a controller that has its token become invalid?
Why does TokensController delete tokens when the error is a conflict?
Should ClientBuilder wait for both the SA and the Secret to have references to each other before using the token?

soltysh · 2017-04-04T14:17:16Z

Why does TokensController delete tokens when the error is a conflict?

It's cleaning the secret, otherwise you might endup with some old crap that's not being linked anywhere

Should ClientBuilder wait for both the SA and the Secret to have references to each other before using the token?

My guess is - yes. To mitigate the problem that you're watching just secrets you need to double check if that secret is actually used/linked in the SA and act upon it only then.

openshift-bot · 2017-06-10T01:37:30Z

Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again

enj · 2017-06-15T00:04:55Z

Superseded by #14293

Rework DockercfgTokenDeletedController

680f3d7

Signed-off-by: Monis Khan <mkhan@redhat.com>

enj assigned deads2k Mar 30, 2017

enj requested review from deads2k and liggitt March 30, 2017 01:24

deads2k reviewed Mar 30, 2017

View reviewed changes

enj added 2 commits March 31, 2017 15:53

Address most comments

d196a15

Signed-off-by: Monis Khan <mkhan@redhat.com>

add test

2d48afa

Signed-off-by: Monis Khan <mkhan@redhat.com>

enj mentioned this pull request Apr 4, 2017

Race condition with ControllerClientBuilder + TokensController + custom token controller kubernetes/kubernetes#44054

Closed

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 1, 2017

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 20, 2017

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 28, 2017

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2017

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2017

enj closed this Jun 15, 2017

Conversation

enj commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deads2k Mar 30, 2017

Choose a reason for hiding this comment

Uh oh!

deads2k Mar 30, 2017

Choose a reason for hiding this comment

Uh oh!

deads2k Mar 30, 2017

Choose a reason for hiding this comment

Uh oh!

deads2k Mar 30, 2017

Choose a reason for hiding this comment

Uh oh!

deads2k Mar 30, 2017

Choose a reason for hiding this comment

Uh oh!

deads2k Mar 30, 2017

Choose a reason for hiding this comment

Uh oh!

deads2k Mar 30, 2017

Choose a reason for hiding this comment

Uh oh!

deads2k commented Mar 30, 2017

Uh oh!

enj commented Mar 31, 2017

Uh oh!

openshift-bot commented Mar 31, 2017

Uh oh!

openshift-bot commented Apr 1, 2017

Uh oh!

enj commented Apr 1, 2017

Uh oh!

liggitt commented Apr 1, 2017

Uh oh!

enj commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soltysh commented Apr 4, 2017

Uh oh!

openshift-bot commented Jun 10, 2017

Uh oh!

enj commented Jun 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

enj commented Mar 30, 2017 •

edited

Loading

enj commented Apr 3, 2017 •

edited

Loading