Add the --rolebinding-name option to rolebinding commands #14868

mrogers950 · 2017-06-23T18:27:32Z

Add a --rolebinding-name option to the rolebinding and clusterrolebinding commands for specifying the name of the rolebinding to modify.

Fixes #13035

stevekuznetsov · 2017-06-23T18:49:09Z

test/cmd/admin.sh

@@ -155,6 +155,30 @@ os::cmd::expect_success 'oadm policy add-cluster-role-to-group cluster-admin sys
 os::cmd::expect_success 'oadm policy remove-cluster-role-from-group cluster-admin system:unauthenticated'
 os::cmd::expect_success 'oadm policy add-cluster-role-to-user cluster-admin system:no-user'
 os::cmd::expect_success 'oadm policy remove-cluster-role-from-user cluster-admin system:no-user'
+# test the rolebinding and clusterrolebinding name flag


Please break this out into it's own file under test/cmd, We don't need this one to grow any more.

stevekuznetsov · 2017-06-23T18:51:47Z

test/cmd/admin.sh

+os::cmd::expect_success 'oadm policy add-role-to-user admin -z fake-sa1 -N rb1'
+os::cmd::expect_success 'oadm policy add-role-to-user admin -z fake-sa2 -N rb2'
+os::cmd::expect_success 'oadm policy add-role-to-user admin -z fake-sa3 -N rb2'
+os::cmd::expect_success_and_text 'oc get rolebinding/rb1' 'rb1       /admin                        fake-sa1'


When doing action-state checks with os::cmd always prefer to use jsonpath to explicitly query the object -- so here, I would expect:

os::cmd::expect_success_and_text "oc get rolebinding rb1 -o jsonpath='{ .roleref.name }'" "admin"

and the same for the subject

Is it possible for the jsonpath output to display those three fields from one command? For example, making the output display as 'rb1 /admin fake-sa1'.

It is, when the paths are comma-delimited. See the doc

Thanks, though I will turn these into unit tests as you suggested.

stevekuznetsov · 2017-06-23T18:52:31Z

test/cmd/admin.sh

+os::cmd::expect_success 'oadm policy add-role-to-user admin -z fake-sa3 -N rb2'
+os::cmd::expect_success_and_text 'oc get rolebinding/rb1' 'rb1       /admin                        fake-sa1'
+os::cmd::expect_success_and_text 'oc get rolebinding/rb2' 'rb2       /admin                        fake-sa2, fake-sa3'
+os::cmd::expect_success 'oadm policy remove-role-from-user admin -z fake-sa3 -N rb2'


That being said, these look more like unit-level tests rather than cmd integration.

Specifically -- a lot of the create and check flows seem superfluous -- we should be able to check that the struct of Options would lead to the correct objects being passed to the client in a unit test. Writing the test like this outside only also checks that the client correctly updates the server, which we don't need to be doing here.

enj · 2017-06-23T19:43:16Z

@openshift/cli-review

juanvallejo · 2017-06-23T20:47:13Z

pkg/cmd/admin/policy/modify_roles.go

@@ -371,9 +381,16 @@ subjectCheck:
 	}

 	if isUpdate {
+		if o.RoleBindingName != "" {


nit: len(o.RoleBindingName) > 0 when checking for a non-empty empty string

juanvallejo · 2017-06-23T20:54:17Z

pkg/cmd/admin/policy/policy.go

@@ -115,7 +115,7 @@ func getUniqueName(basename string, existingNames *sets.String) string {

 // RoleBindingAccessor is used by role modification commands to access and modify roles
 type RoleBindingAccessor interface {
-	GetExistingRoleBindingsForRole(roleNamespace, role string) ([]*authorizationapi.RoleBinding, error)
+	GetExistingRoleBindingsForRole(roleNamespace, role, rb string) ([]*authorizationapi.RoleBinding, error)


I think this might benefit from introducing a new method instead

The new method would replace both uses of GetExistingRoleBindingsForRole(), so maybe it should be renamed. Eg., GetRoleBindingByRoleNames().

juanvallejo · 2017-06-23T20:56:26Z

pkg/cmd/admin/policy/policy.go

+	ret := make([]*authorizationapi.RoleBinding, 0)
+
+	for _, currBinding := range bindings {
+		if currBinding.RoleRef.Name == role && (rb == "" || rb == currBinding.Name) {


nit: len(rb) == 0

juanvallejo · 2017-06-23T21:02:00Z

pkg/cmd/admin/policy/modify_roles.go

@@ -71,6 +72,7 @@ func NewCmdAddRoleToGroup(name, fullName string, f *clientcmd.Factory, out io.Wr
 		},
 	}

+	cmd.Flags().StringVarP(&options.RoleBindingName, "rolebinding-name", "N", "", "name of the rolebinding to modify or create")


name of the rolebinding to modify or create. If left empty, overrides the first rolebinding found for the given role

s/overrides/appends to/

@openshift/cli-review is it normal to have capital parameters like -N?

We have a few cases of this, such as in new-app, rsh, and new-build

avoid claiming new single-letter flags, per https://github.com/kubernetes/community/blob/master/contributors/devel/kubectl-conventions.md#flag-conventions

enj · 2017-06-28T20:25:46Z

pkg/cmd/admin/policy/modify_roles.go

@@ -388,7 +405,7 @@ subjectCheck:
 }

 func (o *RoleModificationOptions) RemoveRole() error {
-	roleBindings, err := o.RoleBindingAccessor.GetExistingRoleBindingsForRole(o.RoleNamespace, o.RoleName)
+	roleBindings, err := o.RoleBindingAccessor.GetExistingRoleBindingsForRole(o.RoleNamespace, o.RoleName, o.RoleBindingName)


Specifying RoleBindingName here seems strange to me. If you are removing a role, you probably want to remove it from all bindings. Otherwise you risk the user continuing to have the permission you just tried to remove.

Miciah · 2017-06-29T13:12:41Z

pkg/cmd/admin/policy/modify_roles.go

@@ -231,6 +238,7 @@ func NewCmdRemoveClusterRoleFromGroup(name, fullName string, f *clientcmd.Factor
 		},
 	}

+	cmd.Flags().StringVarP(&options.RoleBindingName, "rolebinding-name", "N", "", "name of the rolebinding to modify or create")


remove-cluster-role-from-group doesn't create.

mrogers950 · 2017-07-03T16:16:11Z

Replaced cmd tests with unit tests
Use default role name behavior for deletes
Use only --rolebinding-name not -N
Revised option description, other nits.

stevekuznetsov · 2017-07-03T16:56:58Z

pkg/cmd/admin/policy/policy.go

@@ -115,7 +115,7 @@ func getUniqueName(basename string, existingNames *sets.String) string {

 // RoleBindingAccessor is used by role modification commands to access and modify roles
 type RoleBindingAccessor interface {
-	GetExistingRoleBindingsForRole(roleNamespace, role string) ([]*authorizationapi.RoleBinding, error)
+	GetExistingRoleBindingsForRole(roleNamespace, role, rb string) ([]*authorizationapi.RoleBinding, error)


Not sure this parameter makes sense for this method -- also, s/rb/roleBindingName/

stevekuznetsov · 2017-07-03T16:57:39Z

pkg/cmd/admin/policy/policy.go

@@ -131,21 +131,13 @@ func NewLocalRoleBindingAccessor(bindingNamespace string, client client.Interfac
 	return LocalRoleBindingAccessor{bindingNamespace, client}
 }

-func (a LocalRoleBindingAccessor) GetExistingRoleBindingsForRole(roleNamespace, role string) ([]*authorizationapi.RoleBinding, error) {
+func (a LocalRoleBindingAccessor) GetExistingRoleBindingsForRole(roleNamespace, role, rb string) ([]*authorizationapi.RoleBinding, error) {


Callers upstream should just filter by names themselves

mrogers950 · 2017-07-11T14:32:37Z

Changed to filtering by name in AddRole(), leaving GetExistingRoleBindingsForRole() as-is.

stevekuznetsov

seems fine to me but @enj / @deads2k should look

liggitt · 2017-07-11T16:50:06Z

pkg/cmd/admin/policy/modify_roles.go

@@ -371,9 +390,16 @@ subjectCheck:
 	}

 	if isUpdate {
+		if len(o.RoleBindingName) > 0 {
+			roleBinding.Name = o.RoleBindingName


setting the name here is wrong... it's either a no-op or it will fail trying to mutate a name

liggitt · 2017-07-11T16:51:35Z

pkg/cmd/admin/policy/modify_roles.go

@@ -336,6 +342,19 @@ func (o *RoleModificationOptions) AddRole() error {
 	if err != nil {


you only need to look up existing rolebindings if they didn't provide a rolebinding name

liggitt · 2017-07-11T16:53:02Z

pkg/cmd/admin/policy/modify_roles.go

@@ -336,6 +342,19 @@ func (o *RoleModificationOptions) AddRole() error {
 	if err != nil {
 		return err
 	}
+
+	if len(o.RoleBindingName) > 0 {
+		filteredRoleBindings := []*authorizationapi.RoleBinding{}


filtering the list to a single item is weird... it also means if the named rolebinding already exists but points to a different roleref, it'll get missed here... that should be an error

liggitt · 2017-07-11T16:53:14Z

pkg/cmd/admin/policy/modify_roles.go

+			roleBindings = filteredRoleBindings
+		}
+	}
+
 	roleBindingNames, err := o.RoleBindingAccessor.GetExistingRoleBindingNames()


you only need to do this if they didn't provide a name

liggitt · 2017-07-11T16:56:57Z

pkg/cmd/admin/policy/modify_roles.go

 		err = o.RoleBindingAccessor.UpdateRoleBinding(roleBinding)
 	} else {
-		roleBinding.Name = getUniqueName(o.RoleName, roleBindingNames)
+		if len(o.RoleBindingName) > 0 {


I'd like to see all this logic hoisted up to the top

if name provided { look up named binding if exists and points to other roleref, error if exists, set isUpdate=true if doesn't exist, populate object with specified name, set isUpdate=false } else { get bindings to role if exists, pick first to modify, set isUpdate=true if doesn't exist, get existing names and generate unique name, set isUpdate=false }

everything from that point on can deal with the rolebinding the same way in either case

mrogers950 · 2017-07-11T20:02:29Z

@liggitt thanks for the review, I've adjusted the AddRole() logic as you suggested, and added the accessor method GetRoleBinding() in order to look for a rolebinding directly by name.

liggitt · 2017-07-12T20:29:22Z

pkg/cmd/admin/policy/modify_roles.go

+	isUpdate := false
+	if len(o.RoleBindingName) > 0 {
+		// Look for an existing rolebinding by name.
+		roleBinding, err = o.RoleBindingAccessor.GetRoleBinding(o.RoleNamespace, o.RoleBindingName)


if you get an error other than NotFound, return early

liggitt · 2017-07-12T20:30:19Z

pkg/cmd/admin/policy/modify_roles.go

+		// Look for an existing rolebinding by name.
+		roleBinding, err = o.RoleBindingAccessor.GetRoleBinding(o.RoleNamespace, o.RoleBindingName)
+		if roleBinding != nil {
+			if roleBinding.RoleRef.Name != o.RoleName {


or if the roleRef namespace is different

liggitt · 2017-07-12T20:32:50Z

pkg/cmd/admin/policy/policy.go

@@ -165,6 +166,21 @@ func (a LocalRoleBindingAccessor) GetExistingRoleBindingNames() (*sets.String, e
 	return ret, nil
 }

+func (a LocalRoleBindingAccessor) GetRoleBinding(roleNamespace, roleBindingName string) (*authorizationapi.RoleBinding, error) {


namespace, name string

liggitt · 2017-07-12T20:33:27Z

pkg/cmd/admin/policy/policy.go

@@ -165,6 +166,21 @@ func (a LocalRoleBindingAccessor) GetExistingRoleBindingNames() (*sets.String, e
 	return ret, nil
 }

+func (a LocalRoleBindingAccessor) GetRoleBinding(roleNamespace, roleBindingName string) (*authorizationapi.RoleBinding, error) {
+	roleBindings, err := a.Client.PolicyBindings(a.BindingNamespace).Get(authorizationapi.GetPolicyBindingName(roleNamespace), metav1.GetOptions{})


why is this not just return a.Client.RoleBindings(namespace).Get(name, metav1.GetOptions{})?

liggitt · 2017-07-12T20:35:07Z

pkg/cmd/admin/policy/policy.go

@@ -222,6 +238,22 @@ func (a ClusterRoleBindingAccessor) GetExistingRoleBindingNames() (*sets.String,
 	return ret, nil
 }

+func (a ClusterRoleBindingAccessor) GetRoleBinding(roleNamespace, roleBindingName string) (*authorizationapi.RoleBinding, error) {


you only need a name

I guess you have to conform to the interface, but namespace should always be empty

liggitt · 2017-07-12T20:41:33Z

pkg/cmd/admin/policy/policy.go

@@ -222,6 +238,22 @@ func (a ClusterRoleBindingAccessor) GetExistingRoleBindingNames() (*sets.String,
 	return ret, nil
 }

+func (a ClusterRoleBindingAccessor) GetRoleBinding(roleNamespace, roleBindingName string) (*authorizationapi.RoleBinding, error) {
+	uncast, err := a.Client.ClusterPolicyBindings().Get(authorizationapi.GetPolicyBindingName(roleNamespace), metav1.GetOptions{})


return a.Client.ClusterRoleBindings().Get(name, metav1.GetOptions{})?

I guess you have to convert the result in non-error cases

enj · 2017-08-08T17:12:44Z

/test end_to_end

mrogers950 · 2017-08-09T18:14:06Z

After Mo's in-person feedback, I've refactored the revised AddRole logic and simplified the tests. Also rebased.

mrogers950 · 2017-08-10T13:52:15Z

/test extended_conformance_install_update

mrogers950 · 2017-08-10T16:56:41Z

Flake #11114

mrogers950 · 2017-08-10T21:49:31Z

/retest

Add the --rolebinding-name option to the rolebinding and clusterrolebinding add commands for specifying the name of the rolebinding to modify. Signed-off-by: Matt Rogers <[email protected]>

enj · 2017-08-24T20:35:28Z

/lgtm

enj · 2017-08-24T20:37:57Z

@fabianofranz can you approve this?

mrogers950 · 2017-08-25T15:54:46Z

flake #13966

mrogers950 · 2017-08-25T15:54:54Z

/retest

juanvallejo · 2017-08-25T18:10:04Z

/test extended_conformance_install_update

/lgtm

openshift-merge-robot · 2017-08-25T18:10:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: enj, juanvallejo, mrogers950
We suggest the following additional approver: liggitt

Assign the PR to them by writing /assign @liggitt 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 OWNERS Files:

~~contrib/completions/OWNERS~~ [juanvallejo]
~~pkg/cmd/OWNERS~~ [enj]
pkg/oc/OWNERS
~~test/cmd/OWNERS~~ [enj]

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

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

Approving based on feedback from @juanvallejo

mrogers950 · 2017-08-29T20:27:58Z

/retest

openshift-bot · 2017-08-30T10:06:55Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-09-01T08:39:52Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-merge-robot · 2017-09-01T12:54:31Z

Automatic merge from submit-queue

mrogers950 requested review from enj and deads2k June 23, 2017 18:27

stevekuznetsov suggested changes Jun 23, 2017

View reviewed changes

juanvallejo reviewed Jun 23, 2017

View reviewed changes

enj suggested changes Jun 28, 2017

View reviewed changes

Miciah reviewed Jun 29, 2017

View reviewed changes

mrogers950 force-pushed the rb-add-name branch from d6fe738 to 98ec082 Compare July 3, 2017 16:08

stevekuznetsov reviewed Jul 3, 2017

View reviewed changes

mrogers950 force-pushed the rb-add-name branch from 98ec082 to 667621f Compare July 11, 2017 14:17

stevekuznetsov approved these changes Jul 11, 2017

View reviewed changes

liggitt reviewed Jul 11, 2017

View reviewed changes

mrogers950 force-pushed the rb-add-name branch from 667621f to b8cff80 Compare July 11, 2017 19:56

liggitt reviewed Jul 12, 2017

View reviewed changes

mrogers950 force-pushed the rb-add-name branch from b8cff80 to c059d78 Compare July 18, 2017 19:22

mrogers950 force-pushed the rb-add-name branch from bfc3339 to d04ede7 Compare August 9, 2017 18:09

mrogers950 force-pushed the rb-add-name branch 2 times, most recently from 5a26326 to 79714d0 Compare August 22, 2017 19:12

Matt Rogers added 2 commits August 24, 2017 16:03

Add --rolebinding-name to policy commands

2b72402

Add the --rolebinding-name option to the rolebinding and clusterrolebinding add commands for specifying the name of the rolebinding to modify. Signed-off-by: Matt Rogers <[email protected]>

Update completions

a3ab73a

mrogers950 force-pushed the rb-add-name branch from 79714d0 to a3ab73a Compare August 24, 2017 20:04

enj approved these changes Aug 24, 2017

View reviewed changes

openshift-ci-robot assigned enj Aug 24, 2017

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

juanvallejo approved these changes Aug 25, 2017

View reviewed changes

openshift-ci-robot assigned juanvallejo Aug 25, 2017

mrogers950 changed the title ~~Add a -N flag to rolebinding commands~~ Add the --rolebinding-name option to rolebinding commands Aug 29, 2017

enj added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 29, 2017

openshift-merge-robot merged commit 1d698c8 into openshift:master Sep 1, 2017

		@@ -336,6 +342,19 @@ func (o *RoleModificationOptions) AddRole() error {
		if err != nil {

Add the --rolebinding-name option to rolebinding commands #14868

Add the --rolebinding-name option to rolebinding commands #14868

Conversation

mrogers950 commented Jun 23, 2017 • edited by enj 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

Choose a reason for hiding this comment

enj commented Jun 23, 2017

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

mrogers950 commented Jul 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrogers950 commented Jul 11, 2017

stevekuznetsov 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

mrogers950 commented Jul 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Jul 12, 2017 • 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

enj commented Aug 8, 2017

mrogers950 commented Aug 9, 2017

mrogers950 commented Aug 10, 2017

mrogers950 commented Aug 10, 2017

mrogers950 commented Aug 10, 2017

enj commented Aug 24, 2017

enj commented Aug 24, 2017

mrogers950 commented Aug 25, 2017

mrogers950 commented Aug 25, 2017

juanvallejo commented Aug 25, 2017

openshift-merge-robot commented Aug 25, 2017

enj commented Aug 29, 2017

mrogers950 commented Aug 29, 2017

openshift-bot commented Aug 30, 2017

openshift-bot commented Sep 1, 2017

openshift-merge-robot commented Sep 1, 2017

mrogers950 commented Jun 23, 2017 •

edited by enj

Loading

liggitt Jul 12, 2017 •

edited

Loading