Move project creation to use RBAC objects #15973

simo5 · 2017-08-24T23:01:04Z

Also move Service Account Project Role Bindings to RBAC objects

Fixes #15818
Fixes #15856

enj · 2017-08-25T00:59:33Z

You ran a generate script that added junk to your commit.

simo5 · 2017-08-25T12:20:43Z

/test cmd

simo5 · 2017-08-26T11:15:13Z

@enj PTAL

liggitt · 2017-08-27T03:16:48Z

pkg/cmd/server/origin/openshift_apiserver.go

+	for _, rbacBinding := range bootstrappolicy.GetBootstrapServiceAccountProjectRoleBindings(namespace.Name) {
+		binding, err := authzregutil.RoleBindingFromRBAC(&rbacBinding)
+		if err != nil {
+			glog.Errorf("Could not convert Role Bindning %v n the %q namespace: %v\n", rbacBinding.Name, namespace.Name, err)


Role Binding %v in

liggitt · 2017-08-27T03:17:47Z

pkg/project/registry/projectrequest/delegated/delegated.go

@@ -142,9 +143,16 @@ func (r *REST) Create(ctx apirequest.Context, obj runtime.Object, includeUniniti
 			continue
 		}

+		// Check for new RBAS RoleBinding first


liggitt · 2017-08-27T03:21:30Z

nits, lgtm

deads2k · 2017-08-28T13:02:52Z

/approve
/assign enj

simo5 · 2017-08-28T22:18:10Z

@liggitt fixed nits

enj

Nits.

enj · 2017-08-29T17:27:42Z

pkg/project/registry/projectrequest/delegated/delegated.go

@@ -187,8 +195,8 @@ func (r *REST) Create(ctx apirequest.Context, obj runtime.Object, includeUniniti
 	}

 	// wait for a rolebinding if we created one
-	if lastRoleBinding != nil {
-		r.waitForRoleBinding(createdProject.Name, lastRoleBinding.Name)
+	if lastRoleBindingName != "" {


len(lastRoleBindingName) != 0 is the convention

enj · 2017-08-29T17:28:22Z

pkg/project/registry/projectrequest/delegated/delegated.go

-	if lastRoleBinding != nil {
-		r.waitForRoleBinding(createdProject.Name, lastRoleBinding.Name)
+	if lastRoleBindingName != "" {
+		r.waitForRoleBinding(createdProject.Name, lastRoleBindingName)


Update waitForRoleBinding to use Get instead of List.

enj · 2017-08-29T17:29:57Z

pkg/project/registry/projectrequest/delegated/delegated.go

@@ -142,9 +143,16 @@ func (r *REST) Create(ctx apirequest.Context, obj runtime.Object, includeUniniti
 			continue
 		}

+		// Check for new RBAC RoleBinding first
+		if roleBinding, ok := list.Objects[i].(*rbac.RoleBinding); ok {


Change this block to use a type switch since that what we are doing here with mutually exclusive if statements.

enj · 2017-08-29T17:31:04Z

pkg/cmd/server/origin/openshift_apiserver.go

@@ -31,6 +31,7 @@ import (
 	"github.com/openshift/origin/pkg/api/v1"
 	"github.com/openshift/origin/pkg/authorization/authorizer"
 	authorizationinformer "github.com/openshift/origin/pkg/authorization/generated/informers/internalversion"
+	authzregutil "github.com/openshift/origin/pkg/authorization/registry/util"


authorizationregistryutil

enj · 2017-08-29T17:33:54Z

pkg/cmd/server/origin/openshift_apiserver.go

+	for _, rbacBinding := range bootstrappolicy.GetBootstrapServiceAccountProjectRoleBindings(namespace.Name) {
+		binding, err := authzregutil.RoleBindingFromRBAC(&rbacBinding)
+		if err != nil {
+			glog.Errorf("Could not convert Role Binding %v n the %q namespace: %v\n", rbacBinding.Name, namespace.Name, err)


glog.Errorf("Could not convert Role Binding %s in the %q namespace: %v", rbacBinding.Name, namespace.Name, err)

enj · 2017-08-29T17:38:10Z

pkg/oc/admin/project/new_project.go

+	for _, rbacBinding := range bootstrappolicy.GetBootstrapServiceAccountProjectRoleBindings(o.ProjectName) {
+		binding, err := authzregutil.RoleBindingFromRBAC(&rbacBinding)
+		if err != nil {
+			errs = append(errs, err)


fmt.Printf("Could not convert Role Binding %s in the %q namespace: %v\n", rbacBinding.Name, o.ProjectName, err)

enj · 2017-08-29T17:39:21Z

pkg/oc/admin/project/new_project.go

@@ -14,6 +14,7 @@ import (
 	kcmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"

 	oapi "github.com/openshift/origin/pkg/api"
+	authzregutil "github.com/openshift/origin/pkg/authorization/registry/util"


authorizationregistryutil

nits addressed

enj · 2017-08-29T20:04:29Z

pkg/project/registry/projectrequest/delegated/delegated.go

-
-		return false, nil
+		_, err := r.roleBindings.RoleBindings(namespace).Get(name)
+		return (err == nil), nil


Why the redundant parentheses?

@liggitt there wasn't some archaic reason this was using list before right?

maybe the lister we implemented for the virtual resources didn't implement Get?

enj · 2017-08-29T20:17:03Z

pkg/project/registry/projectrequest/delegated/delegated.go

@@ -133,18 +133,18 @@ func (r *REST) Create(ctx apirequest.Context, obj runtime.Object, includeUniniti

 	// one of the items in this list should be the project.  We are going to locate it, remove it from the list, create it separately
 	var projectFromTemplate *projectapi.Project
-	var lastRoleBinding *authorizationapi.RoleBinding
+	lastRoleBindingName := ""
 	objectsToCreate := &kapi.List{}
 	for i := range list.Objects {


for _, lobj := range list.Objects { switch t := lobj.(type) { case *projectapi.Project: projectFromTemplate = t // don't add this to the list to create. We'll create the project separately. continue case *rbac.RoleBinding: lastRoleBindingName = t.Name case *authorizationapi.RoleBinding: lastRoleBindingName = t.Name default: // no-op we only skip creation of project } objectsToCreate.Items = append(objectsToCreate.Items, lobj) }

@liggitt any reason we were doing list.Objects[i] before? Also, do we want to error if we see more than one project in the template?

@liggitt any reason we were doing list.Objects[i] before?

so we didn't have to reason about range variable reuse outside the loop

Also, do we want to error if we see more than one project in the template?

probably

so we didn't have to reason about range variable reuse outside the loop

At worst it would only shadow a var outside the loop (I picked a unique name to avoid that as well)?

at worst, you'd reuse a memory address and actually append the same item to the list of items to create N times

at worst, you'd reuse a memory address and actually append the same item to the list of items to create N times

Ah you are correct, I always forget the "don't use range vars + append together".

for i := range list.Objects { switch t := list.Objects[i].(type) { case *projectapi.Project: projectFromTemplate = t // don't add this to the list to create. We'll create the project separately. continue case *rbac.RoleBinding: lastRoleBindingName = t.Name case *authorizationapi.RoleBinding: lastRoleBindingName = t.Name default: // no-op we only skip creation of project } // use list.Objects[i] in append to avoid range memory address reuse objectsToCreate.Items = append(objectsToCreate.Items, list.Objects[i]) }

enj · 2017-08-29T20:20:45Z

pkg/cmd/server/origin/openshift_apiserver.go

+	for _, rbacBinding := range bootstrappolicy.GetBootstrapServiceAccountProjectRoleBindings(namespace.Name) {
+		binding, err := authorizationregistryutil.RoleBindingFromRBAC(&rbacBinding)
+		if err != nil {
+			glog.Errorf("Could not convert Role Binding %v n the %q namespace: %v\n", rbacBinding.Name, namespace.Name, err)


This is still wrong.

Should have addressed the additional nits

dobbymoodge · 2017-08-30T18:29:29Z

/retest

simo5 · 2017-08-31T13:41:17Z

Still seeing this flake even if it has been closed #14575

simo5 · 2017-08-31T13:43:39Z

/retest

enj · 2017-08-31T17:06:32Z

@simo5 clean up the type switch and have it fail on multiple projects.

Also move Service Account Project Role Bindings to RBAC objects Signed-off-by: Simo Sorce <[email protected]>

simo5 · 2017-09-06T03:16:35Z

@enj PTAL

enj · 2017-09-06T19:00:25Z

/lgtm

openshift-merge-robot · 2017-09-06T19:00:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, enj, simo5

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/cmd/OWNERS~~ [deads2k,enj]
~~pkg/oc/OWNERS~~ [deads2k]
~~pkg/project/OWNERS~~ [deads2k]
~~test/testdata/OWNERS~~ [deads2k,enj]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-merge-robot · 2017-09-06T22:41:26Z

Automatic merge from submit-queue (batch tested with PRs 15885, 15973, 16000)

openshift-merge-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 24, 2017

openshift-merge-robot assigned mfojtik and dcbw Aug 24, 2017

simo5 force-pushed the newprjrbac branch 2 times, most recently from 2980e12 to 6ebdbe4 Compare August 25, 2017 12:23

simo5 changed the title ~~Move project creation touse RBAC objects~~ Move project creation to use RBAC objects Aug 25, 2017

enj assigned liggitt and deads2k and unassigned mfojtik and dcbw Aug 25, 2017

simo5 force-pushed the newprjrbac branch 2 times, most recently from c435189 to 222ab5d Compare August 26, 2017 01:15

liggitt reviewed Aug 27, 2017

View reviewed changes

openshift-ci-robot assigned enj Aug 28, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 28, 2017

simo5 force-pushed the newprjrbac branch from 222ab5d to 5558ea5 Compare August 28, 2017 22:17

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2017

enj previously requested changes Aug 29, 2017

View reviewed changes

simo5 force-pushed the newprjrbac branch from 5558ea5 to 573b990 Compare August 29, 2017 19:33

simo5 force-pushed the newprjrbac branch from 573b990 to 1068811 Compare August 29, 2017 19:53

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2017

enj previously requested changes Aug 29, 2017

View reviewed changes

simo5 force-pushed the newprjrbac branch from 1068811 to 5ad2784 Compare August 30, 2017 13:26

simo5 force-pushed the newprjrbac branch from 5ad2784 to 3715818 Compare September 5, 2017 15:44

Move project creation to use RBAC objects

b6d8afb

Also move Service Account Project Role Bindings to RBAC objects Signed-off-by: Simo Sorce <[email protected]>

simo5 force-pushed the newprjrbac branch from 3715818 to b6d8afb Compare September 5, 2017 16:42

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2017

openshift-merge-robot merged commit 14ebc56 into openshift:master Sep 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move project creation to use RBAC objects #15973

Move project creation to use RBAC objects #15973

simo5 commented Aug 24, 2017

enj commented Aug 25, 2017

simo5 commented Aug 25, 2017

simo5 commented Aug 26, 2017

liggitt Aug 27, 2017

liggitt Aug 27, 2017

liggitt commented Aug 27, 2017

deads2k commented Aug 28, 2017

simo5 commented Aug 28, 2017

enj left a comment

enj Aug 29, 2017

enj Aug 29, 2017

enj Aug 29, 2017

enj Aug 29, 2017

enj Aug 29, 2017

enj Aug 29, 2017

enj Aug 29, 2017

enj Aug 29, 2017

liggitt Aug 29, 2017

enj Aug 29, 2017

liggitt Aug 29, 2017

enj Aug 29, 2017

liggitt Aug 29, 2017

enj Aug 29, 2017

enj Aug 29, 2017

dobbymoodge commented Aug 30, 2017

simo5 commented Aug 31, 2017

simo5 commented Aug 31, 2017

enj commented Aug 31, 2017

simo5 commented Sep 6, 2017

enj commented Sep 6, 2017

openshift-merge-robot commented Sep 6, 2017

openshift-merge-robot commented Sep 6, 2017

Move project creation to use RBAC objects #15973

Move project creation to use RBAC objects #15973

Conversation

simo5 commented Aug 24, 2017

enj commented Aug 25, 2017

simo5 commented Aug 25, 2017

simo5 commented Aug 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Aug 27, 2017

deads2k commented Aug 28, 2017

simo5 commented Aug 28, 2017

enj left a 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

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

dobbymoodge commented Aug 30, 2017

simo5 commented Aug 31, 2017

simo5 commented Aug 31, 2017

enj commented Aug 31, 2017

simo5 commented Sep 6, 2017

enj commented Sep 6, 2017

openshift-merge-robot commented Sep 6, 2017

openshift-merge-robot commented Sep 6, 2017