remove old controller roles #15364

deads2k · 2017-07-20T12:26:45Z

This pull shifts us over fully to the RBAC roles. It removes the old wiring, but does leave the ensure block (for now) to ensure give @mfojtik a chance to convince @liggitt he can pick it back instead of wiring a second path.

We no longer need to create the SAs ahead of time, since the client-builder does that for us.

[test]

deads2k · 2017-07-20T12:28:02Z

Also, I'm betting it was me, but having my controllers rely on a particular way the server started is really ugly.

0xmichalis · 2017-07-20T12:52:49Z

/lgtm

openshift-ci-robot · 2017-07-20T12:52:58Z

@Kargakis: changing LGTM is restricted to assignees, and only kubernetes org members may be assigned issues.

In response to this:

/lgtm

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.

0xmichalis · 2017-07-20T12:53:32Z

/assign

liggitt · 2017-07-20T12:56:07Z

only kubernetes org members may be assigned issues

heh

0xmichalis · 2017-07-20T12:57:35Z

The message is hardcoded, something else is going on here

0xmichalis · 2017-07-20T12:58:06Z

/assign

0xmichalis · 2017-07-20T12:58:30Z

/unassign

liggitt · 2017-07-20T13:04:09Z

pkg/cmd/server/origin/ensure.go

+	}
+	for _, roleBinding := range bootstrappolicy.ControllerRoleBindings() {
+		reconcileRoleBinding := &policy.ReconcileClusterRoleBindingsOptions{
+			RolesToReconcile:  []string{roleBinding.Name},


roleBinding.RoleRef.Name

liggitt · 2017-07-20T13:30:52Z

pkg/cmd/server/origin/ensure.go

-
-		role, _ := bootstrappolicy.InfraSAs.RoleFor(saName)
-
+	for _, role := range bootstrappolicy.ControllerRoles() {


doesn't this omit the kube bootstrappolicy.ControllerRoles()?

doesn't this omit the kube bootstrappolicy.ControllerRoles()?

We should need any of this moving forward and those are particularly easy to chase. We should stay strict in 3.7

liggitt · 2017-07-20T13:31:08Z

pkg/cmd/server/origin/ensure.go

-			RoleBindingAccessor: roleAccessor,
-			Subjects:            []kapi.ObjectReference{{Namespace: ns, Name: saName, Kind: "ServiceAccount"}},
+	}
+	for _, roleBinding := range bootstrappolicy.ControllerRoleBindings() {


same here for kube bootstrap controller role bindings

We should need any of this moving forward and those are particularly easy to chase. We should stay strict in 3.7

liggitt · 2017-07-20T14:10:36Z

--- FAIL: TestSystemOnlyRoles (0.00s)
	web_console_role_test.go:80: The list of expected end user roles has been changed.  Please discuss with the web console team to update role annotations.

liggitt · 2017-07-20T14:25:06Z

system:openshift:template-service-broker is the problem role

the test would be more helpful if it did this:

	if !show.Equal(rolesToShow) || !hide.Equal(rolesToHide) {
		t.Error("The list of expected end user roles has been changed.  Please discuss with the web console team to update role annotations.")
		t.Logf("These roles are visible but not in rolesToShow: %v", show.Difference(rolesToShow).List())
		t.Logf("These roles are hidden but not in rolesToHide: %v", hide.Difference(rolesToHide).List())
		t.Logf("These roles are in rolesToShow but are missing from the visible list: %v", rolesToShow.Difference(show).List())
		t.Logf("These roles are in rolesToHide but are missing from the hidden list: %v", rolesToHide.Difference(hide).List())
	}

liggitt · 2017-07-20T15:00:24Z

ok, fix the roleref.name and test, then LGTM

deads2k · 2017-07-20T15:02:47Z

[merge]

deads2k · 2017-07-20T15:03:11Z

pkg/cmd/server/origin/ensure.go

+	}
+	for _, roleBinding := range bootstrappolicy.ControllerRoleBindings() {
+		reconcileRoleBinding := &policy.ReconcileClusterRoleBindingsOptions{
+			RolesToReconcile:  []string{roleBinding.RoleRef.Name},


deads2k · 2017-07-20T15:03:27Z

pkg/cmd/server/bootstrappolicy/web_console_role_test.go

@@ -46,7 +46,6 @@ var rolesToHide = sets.NewString(
 	"system:node-proxier",
 	"system:node-reader",
 	"system:oauth-token-deleter",
-	"system:openshift:template-service-broker",


openshift-bot · 2017-07-20T15:08:20Z

Evaluated for origin test up to 67e8491

openshift-bot · 2017-07-20T15:08:30Z

continuous-integration/openshift-jenkins/merge Waiting: You are in the build queue at position: 15

openshift-bot · 2017-07-20T15:08:31Z

Evaluated for origin merge up to 67e8491

openshift-bot · 2017-07-20T17:17:14Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/3341/) (Base Commit: bfb45e9) (PR Branch Commit: 67e8491)

liggitt · 2017-07-20T17:26:21Z

green tests, prereq for #15353 which is blocking the queue, merging

enj · 2017-07-20T22:07:20Z

@openshift/security

enj · 2017-07-20T22:10:44Z

@benjaminapetersen in regards to Jordan's comment about test improvement.

enj · 2017-07-20T22:15:17Z

Oh NVM David took care of it 😁

smarterclayton · 2017-08-18T22:26:05Z

pkg/cmd/server/bootstrappolicy/infra_sa_policy.go

-	InfraPersistentVolumeRecyclerControllerServiceAccountName   = "pv-recycler-controller"
-	InfraResourceQuotaControllerServiceAccountName              = "resourcequota-controller"
-
-	InfraNodeBootstrapServiceAccountName = "node-bootstrapper"


Needs to be added back, this is the account that the node is using. Or needs an equivalent set of permissions so that we can merge the openshift-ansible node bootstrapping work. #15869

remove unnecessary duplicate clusterrole

181f498

deads2k assigned mfojtik Jul 20, 2017

deads2k mentioned this pull request Jul 20, 2017

Refactor controller initialization (final round?) #14633

Merged

openshift-ci-robot assigned 0xmichalis Jul 20, 2017

openshift-ci-robot unassigned 0xmichalis Jul 20, 2017

liggitt reviewed Jul 20, 2017

View reviewed changes

This was referenced Jul 20, 2017

Register SDN informers synchronously #15354

Merged

Register SDN informers synchronously #15353

Merged

remove old controller SA registration

67e8491

deads2k force-pushed the auth-22-fix-controller branch from 84c4123 to 67e8491 Compare July 20, 2017 15:02

deads2k commented Jul 20, 2017

View reviewed changes

deads2k added the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2017

liggitt merged commit 7a48deb into openshift:master Jul 20, 2017

deads2k mentioned this pull request Jul 20, 2017

Test flake: openshift router metrics The HAProxy router [It] should expose the profiling endpoints #15356

Closed

deads2k deleted the auth-22-fix-controller branch August 3, 2017 19:27

smarterclayton reviewed Aug 18, 2017

View reviewed changes


		role, _ := bootstrappolicy.InfraSAs.RoleFor(saName)

		for _, role := range bootstrappolicy.ControllerRoles() {

remove old controller roles #15364

remove old controller roles #15364

Uh oh!

Conversation

deads2k commented Jul 20, 2017

Uh oh!

deads2k commented Jul 20, 2017

Uh oh!

0xmichalis commented Jul 20, 2017

Uh oh!

openshift-ci-robot commented Jul 20, 2017

Uh oh!

0xmichalis commented Jul 20, 2017

Uh oh!

liggitt commented Jul 20, 2017

Uh oh!

0xmichalis commented Jul 20, 2017

Uh oh!

0xmichalis commented Jul 20, 2017

Uh oh!

0xmichalis commented Jul 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt commented Jul 20, 2017

Uh oh!

liggitt commented Jul 20, 2017

Uh oh!

liggitt commented Jul 20, 2017

Uh oh!

deads2k commented Jul 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-bot commented Jul 20, 2017

Uh oh!

openshift-bot commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-bot commented Jul 20, 2017

Uh oh!

openshift-bot commented Jul 20, 2017

Uh oh!

liggitt commented Jul 20, 2017

Uh oh!

enj commented Jul 20, 2017

Uh oh!

enj commented Jul 20, 2017

Uh oh!

enj commented Jul 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openshift-bot commented Jul 20, 2017 •

edited

Loading