Skip to content

Commit

Permalink
Add a -N flag to rolebinding commands
Browse files Browse the repository at this point in the history 
Add a -N/--rolebinding-name option to the rolebinding and
clusterrolebinding commands for specifying the name of the rolebinding to
modify.

Fixes #13035
  • Loading branch information
Matt Rogers committed Jun 23, 2017
1 parent 4423ff5 commit d6fe738
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 23 deletions.
23 changes: 20 additions & 3 deletions pkg/cmd/admin/policy/modify_roles.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ var (
type RoleModificationOptions struct {
RoleNamespace string
RoleName string
RoleBindingName string
RoleBindingAccessor RoleBindingAccessor

Targets []string
Expand Down Expand Up @@ -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")
cmd.Flags().StringVar(&options.RoleNamespace, "role-namespace", "", "namespace where the role is located: empty means a role defined in cluster policy")

return cmd
Expand Down Expand Up @@ -99,6 +101,7 @@ func NewCmdAddRoleToUser(name, fullName string, f *clientcmd.Factory, out io.Wri
},
}

cmd.Flags().StringVarP(&options.RoleBindingName, "rolebinding-name", "N", "", "name of the rolebinding to modify or create")
cmd.Flags().StringVar(&options.RoleNamespace, "role-namespace", "", "namespace where the role is located: empty means a role defined in cluster policy")
cmd.Flags().StringSliceVarP(&saNames, "serviceaccount", "z", saNames, "service account in the current namespace to use as a user")

Expand Down Expand Up @@ -126,6 +129,7 @@ func NewCmdRemoveRoleFromGroup(name, fullName string, f *clientcmd.Factory, out
},
}

cmd.Flags().StringVarP(&options.RoleBindingName, "rolebinding-name", "N", "", "name of the rolebinding to modify")
cmd.Flags().StringVar(&options.RoleNamespace, "role-namespace", "", "namespace where the role is located: empty means a role defined in cluster policy")

return cmd
Expand Down Expand Up @@ -153,6 +157,7 @@ func NewCmdRemoveRoleFromUser(name, fullName string, f *clientcmd.Factory, out i
},
}

cmd.Flags().StringVarP(&options.RoleBindingName, "rolebinding-name", "N", "", "name of the rolebinding to modify")
cmd.Flags().StringVar(&options.RoleNamespace, "role-namespace", "", "namespace where the role is located: empty means a role defined in cluster policy")
cmd.Flags().StringSliceVarP(&saNames, "serviceaccount", "z", saNames, "service account in the current namespace to use as a user")

Expand Down Expand Up @@ -180,6 +185,7 @@ func NewCmdAddClusterRoleToGroup(name, fullName string, f *clientcmd.Factory, ou
},
}

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

Expand All @@ -205,6 +211,7 @@ func NewCmdAddClusterRoleToUser(name, fullName string, f *clientcmd.Factory, out
},
}

cmd.Flags().StringVarP(&options.RoleBindingName, "rolebinding-name", "N", "", "name of the rolebinding to modify or create")
cmd.Flags().StringSliceVarP(&saNames, "serviceaccount", "z", saNames, "service account in the current namespace to use as a user")

return cmd
Expand All @@ -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")
return cmd
}

Expand All @@ -256,6 +264,7 @@ func NewCmdRemoveClusterRoleFromUser(name, fullName string, f *clientcmd.Factory
},
}

cmd.Flags().StringVarP(&options.RoleBindingName, "rolebinding-name", "N", "", "name of the rolebinding to modify")
cmd.Flags().StringSliceVarP(&saNames, "serviceaccount", "z", saNames, "service account in the current namespace to use as a user")

return cmd
Expand All @@ -267,6 +276,7 @@ func (o *RoleModificationOptions) CompleteUserWithSA(f *clientcmd.Factory, args
}

o.RoleName = args[0]

if len(args) > 1 {
o.Users = append(o.Users, args[1:]...)
}
Expand Down Expand Up @@ -332,7 +342,7 @@ func (o *RoleModificationOptions) Complete(f *clientcmd.Factory, args []string,
}

func (o *RoleModificationOptions) AddRole() error {
roleBindings, err := o.RoleBindingAccessor.GetExistingRoleBindingsForRole(o.RoleNamespace, o.RoleName)
roleBindings, err := o.RoleBindingAccessor.GetExistingRoleBindingsForRole(o.RoleNamespace, o.RoleName, o.RoleBindingName)
if err != nil {
return err
}
Expand Down Expand Up @@ -371,9 +381,16 @@ subjectCheck:
}

if isUpdate {
if o.RoleBindingName != "" {
roleBinding.Name = o.RoleBindingName
}
err = o.RoleBindingAccessor.UpdateRoleBinding(roleBinding)
} else {
roleBinding.Name = getUniqueName(o.RoleName, roleBindingNames)
if o.RoleBindingName != "" {
roleBinding.Name = o.RoleBindingName
} else {
roleBinding.Name = getUniqueName(o.RoleName, roleBindingNames)
}
err = o.RoleBindingAccessor.CreateRoleBinding(roleBinding)
// If the rolebinding was created in the meantime, rerun
if kapierrors.IsAlreadyExists(err) {
Expand All @@ -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)
if err != nil {
return err
}
Expand Down
39 changes: 19 additions & 20 deletions pkg/cmd/admin/policy/policy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
GetExistingRoleBindingNames() (*sets.String, error)
UpdateRoleBinding(binding *authorizationapi.RoleBinding) error
CreateRoleBinding(binding *authorizationapi.RoleBinding) error
Expand All @@ -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) {
existingBindings, err := a.Client.PolicyBindings(a.BindingNamespace).Get(authorizationapi.GetPolicyBindingName(roleNamespace), metav1.GetOptions{})
if err != nil && !kapierrors.IsNotFound(err) {
return nil, err
}

ret := make([]*authorizationapi.RoleBinding, 0)
// see if we can find an existing binding that points to the role in question.
for _, currBinding := range existingBindings.RoleBindings {
if currBinding.RoleRef.Name == role {
t := currBinding
ret = append(ret, t)
}
}

ret := findBindingsByRoleNames(existingBindings.RoleBindings, role, rb)
return ret, nil
}

Expand Down Expand Up @@ -186,21 +178,28 @@ func NewClusterRoleBindingAccessor(client client.Interface) ClusterRoleBindingAc
return ClusterRoleBindingAccessor{client}
}

func (a ClusterRoleBindingAccessor) GetExistingRoleBindingsForRole(roleNamespace, role string) ([]*authorizationapi.RoleBinding, error) {
// Filter out bindings by role or both role and rb if rb is a non-empty string
func findBindingsByRoleNames(bindings authorizationapi.RoleBindingsByName, role string, rb string) []*authorizationapi.RoleBinding {
ret := make([]*authorizationapi.RoleBinding, 0)

for _, currBinding := range bindings {
if currBinding.RoleRef.Name == role && (rb == "" || rb == currBinding.Name) {
t := currBinding
ret = append(ret, t)
}
}

return ret
}

func (a ClusterRoleBindingAccessor) GetExistingRoleBindingsForRole(roleNamespace, role, rb string) ([]*authorizationapi.RoleBinding, error) {
uncast, err := a.Client.ClusterPolicyBindings().Get(authorizationapi.GetPolicyBindingName(roleNamespace), metav1.GetOptions{})
if err != nil && !kapierrors.IsNotFound(err) {
return nil, err
}
existingBindings := authorizationapi.ToPolicyBinding(uncast)

ret := make([]*authorizationapi.RoleBinding, 0)
// see if we can find an existing binding that points to the role in question.
for _, currBinding := range existingBindings.RoleBindings {
if currBinding.RoleRef.Name == role {
t := currBinding
ret = append(ret, t)
}
}
ret := findBindingsByRoleNames(existingBindings.RoleBindings, role, rb)

return ret, nil
}
Expand Down
24 changes: 24 additions & 0 deletions test/cmd/admin.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
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'
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'
os::cmd::expect_success_and_text 'oc get rolebinding/rb2' 'rb2 /admin fake-sa2'
# Attempt to remove from the wrong rolebinding. The command succeeds but does not remove fake-sa1 from rb1
os::cmd::expect_success 'oadm policy remove-role-from-user admin -z fake-sa1 -N rb2'
os::cmd::expect_success_and_text 'oc get rolebinding/rb1' 'rb1 /admin fake-sa1'
os::cmd::expect_success 'oadm policy remove-role-from-user admin -z fake-sa1 -N rb1'
os::cmd::expect_success_and_text 'oc get rolebinding/rb1' 'rb1 /admin'
os::cmd::expect_success 'oadm policy add-cluster-role-to-user admin user1 -N crb1'
os::cmd::expect_success 'oadm policy add-cluster-role-to-user admin user2 -N crb2'
os::cmd::expect_success_and_text 'oc get clusterrolebinding/crb1' 'crb1 /admin user1'
os::cmd::expect_success_and_text 'oc get clusterrolebinding/crb2' 'crb2 /admin user2'
os::cmd::expect_success 'oadm policy add-cluster-role-to-group admin group1 -N crb2'
os::cmd::expect_success_and_text 'oc get clusterrolebinding/crb2' 'crb2 /admin user2 group1'
os::cmd::expect_success 'oadm policy remove-cluster-role-from-user admin user2 -N crb2'
os::cmd::expect_success_and_text 'oc get clusterrolebinding/crb2' 'crb2 /admin group1'
os::cmd::expect_success 'oadm policy remove-cluster-role-from-group admin group1 -N crb2'
os::cmd::expect_success_and_text 'oc get clusterrolebinding/crb2' 'crb2 /admin'
os::cmd::expect_success_and_text 'oc get clusterrolebinding/crb1' 'crb1 /admin user1'

os::cmd::expect_success 'oadm policy add-scc-to-user privileged fake-user'
os::cmd::expect_success_and_text 'oc get scc/privileged -o yaml' 'fake-user'
Expand Down

0 comments on commit d6fe738

Please sign in to comment.