diff --git a/pkg/cmd/admin/policy/modify_roles.go b/pkg/cmd/admin/policy/modify_roles.go index f0316797fab5..34dbb3bea68b 100644 --- a/pkg/cmd/admin/policy/modify_roles.go +++ b/pkg/cmd/admin/policy/modify_roles.go @@ -41,6 +41,7 @@ var ( type RoleModificationOptions struct { RoleNamespace string RoleName string + RoleBindingName string RoleBindingAccessor RoleBindingAccessor Targets []string @@ -71,6 +72,7 @@ func NewCmdAddRoleToGroup(name, fullName string, f *clientcmd.Factory, out io.Wr }, } + cmd.Flags().StringVar(&options.RoleBindingName, "rolebinding-name", "", "Name of the rolebinding to modify or create. If left empty, appends to the first rolebinding found for the given role") cmd.Flags().StringVar(&options.RoleNamespace, "role-namespace", "", "namespace where the role is located: empty means a role defined in cluster policy") return cmd @@ -99,6 +101,7 @@ func NewCmdAddRoleToUser(name, fullName string, f *clientcmd.Factory, out io.Wri }, } + cmd.Flags().StringVar(&options.RoleBindingName, "rolebinding-name", "", "Name of the rolebinding to modify or create. If left empty, appends to the first rolebinding found for the given role") 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") @@ -180,6 +183,7 @@ func NewCmdAddClusterRoleToGroup(name, fullName string, f *clientcmd.Factory, ou }, } + cmd.Flags().StringVar(&options.RoleBindingName, "rolebinding-name", "", "Name of the rolebinding to modify or create. If left empty, appends to the first rolebinding found for the given role") return cmd } @@ -205,6 +209,7 @@ func NewCmdAddClusterRoleToUser(name, fullName string, f *clientcmd.Factory, out }, } + cmd.Flags().StringVar(&options.RoleBindingName, "rolebinding-name", "", "Name of the rolebinding to modify or create. If left empty, appends to the first rolebinding found for the given role") cmd.Flags().StringSliceVarP(&saNames, "serviceaccount", "z", saNames, "service account in the current namespace to use as a user") return cmd @@ -267,6 +272,7 @@ func (o *RoleModificationOptions) CompleteUserWithSA(f *clientcmd.Factory, args } o.RoleName = args[0] + if len(args) > 1 { o.Users = append(o.Users, args[1:]...) } @@ -332,23 +338,43 @@ func (o *RoleModificationOptions) Complete(f *clientcmd.Factory, args []string, } func (o *RoleModificationOptions) AddRole() error { - roleBindings, err := o.RoleBindingAccessor.GetExistingRoleBindingsForRole(o.RoleNamespace, o.RoleName) - if err != nil { - return err - } - roleBindingNames, err := o.RoleBindingAccessor.GetExistingRoleBindingNames() - if err != nil { - return err - } - var roleBinding *authorizationapi.RoleBinding - isUpdate := true - if len(roleBindings) == 0 { - roleBinding = &authorizationapi.RoleBinding{} - isUpdate = false + var err error + isUpdate := false + if len(o.RoleBindingName) > 0 { + // Look for an existing rolebinding by name. + roleBinding, err = o.RoleBindingAccessor.GetRoleBinding(o.RoleBindingName) + if err != nil && !kapierrors.IsNotFound(err) { + return err + } + if !kapierrors.IsNotFound(err) && roleBinding != nil { + if roleBinding.RoleRef.Name != o.RoleName || roleBinding.RoleRef.Namespace != o.RoleNamespace { + return fmt.Errorf("rolebinding %s found for role %s, not %s", roleBinding.Name, roleBinding.RoleRef.Name, o.RoleName) + } + isUpdate = true + } else { + roleBinding = &authorizationapi.RoleBinding{} + roleBinding.Name = o.RoleBindingName + } } else { - // only need to add the user or group to a single roleBinding on the role. Just choose the first one - roleBinding = roleBindings[0] + // Look for existing bindings by role. + roleBindings, err := o.RoleBindingAccessor.GetExistingRoleBindingsForRole(o.RoleNamespace, o.RoleName) + if err != nil { + return err + } + + if len(roleBindings) > 0 { + // only need to add the user or group to a single roleBinding on the role. Just choose the first one + roleBinding = roleBindings[0] + isUpdate = true + } else { + roleBinding = &authorizationapi.RoleBinding{} + roleBindingNames, err := o.RoleBindingAccessor.GetExistingRoleBindingNames() + if err != nil { + return err + } + roleBinding.Name = getUniqueName(o.RoleName, roleBindingNames) + } } roleBinding.RoleRef.Namespace = o.RoleNamespace @@ -373,7 +399,6 @@ subjectCheck: if isUpdate { err = o.RoleBindingAccessor.UpdateRoleBinding(roleBinding) } 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) { diff --git a/pkg/cmd/admin/policy/modify_roles_test.go b/pkg/cmd/admin/policy/modify_roles_test.go new file mode 100644 index 000000000000..d64c6f0abfd0 --- /dev/null +++ b/pkg/cmd/admin/policy/modify_roles_test.go @@ -0,0 +1,319 @@ +package policy + +import ( + "reflect" + "testing" + + "github.com/openshift/origin/pkg/client/testclient" + "k8s.io/apimachinery/pkg/runtime" + + authorizationapi "github.com/openshift/origin/pkg/authorization/apis/authorization" + kapierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + clientgotesting "k8s.io/client-go/testing" + kapi "k8s.io/kubernetes/pkg/api" +) + +func TestModifyRoleBindingName(t *testing.T) { + var fake *testclient.Fake + fake = testclient.NewSimpleFake() + + tests := map[string]struct { + rolename string + rolenamespace string + rolebindingname string + users []string + accessor RoleBindingAccessor + startPolicyBinding *authorizationapi.PolicyBinding + startClusterPolicyBinding *authorizationapi.ClusterPolicyBinding + cluster bool + }{ + "create-default-binding": { + rolename: "edit", + rolenamespace: metav1.NamespaceDefault, + rolebindingname: "", + users: []string{ + "foo", + }, + accessor: NewLocalRoleBindingAccessor(metav1.NamespaceDefault, fake), + startPolicyBinding: &authorizationapi.PolicyBinding{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace", + Name: "policybinding", + }, + PolicyRef: kapi.ObjectReference{ + Namespace: "namespace", + }, + RoleBindings: map[string]*authorizationapi.RoleBinding{}, + }, + startClusterPolicyBinding: nil, + cluster: false, + }, + "create-named-binding": { + rolename: "edit", + rolenamespace: metav1.NamespaceDefault, + rolebindingname: "rb1", + users: []string{ + "foo", + }, + accessor: NewLocalRoleBindingAccessor(metav1.NamespaceDefault, fake), + startPolicyBinding: &authorizationapi.PolicyBinding{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace", + Name: "policybinding", + }, + PolicyRef: kapi.ObjectReference{ + Namespace: "namespace", + }, + RoleBindings: map[string]*authorizationapi.RoleBinding{}, + }, + startClusterPolicyBinding: nil, + cluster: false, + }, + "update-default-binding": { + rolename: "edit", + rolenamespace: metav1.NamespaceDefault, + rolebindingname: "", + users: []string{ + "foo", + "bar", + }, + accessor: NewLocalRoleBindingAccessor(metav1.NamespaceDefault, fake), + startPolicyBinding: &authorizationapi.PolicyBinding{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace", + Name: "policybinding", + }, + PolicyRef: kapi.ObjectReference{ + Namespace: "namespace", + }, + RoleBindings: map[string]*authorizationapi.RoleBinding{ + "edit": { + ObjectMeta: metav1.ObjectMeta{ + Name: "edit", + Namespace: metav1.NamespaceDefault, + }, + Subjects: []kapi.ObjectReference{{ + Name: "foo", + Kind: authorizationapi.UserKind, + }}, + RoleRef: kapi.ObjectReference{ + Name: "edit", + Namespace: metav1.NamespaceDefault, + }, + }, + "custom": { + ObjectMeta: metav1.ObjectMeta{ + Name: "custom", + Namespace: metav1.NamespaceDefault, + }, + Subjects: []kapi.ObjectReference{{ + Name: "baz", + Kind: authorizationapi.UserKind, + }}, + RoleRef: kapi.ObjectReference{ + Name: "edit", + Namespace: metav1.NamespaceDefault, + }, + }, + }, + }, + startClusterPolicyBinding: nil, + cluster: false, + }, + "update-named-binding": { + rolename: "edit", + rolenamespace: metav1.NamespaceDefault, + rolebindingname: "custom", + users: []string{ + "bar", + "baz", + }, + accessor: NewLocalRoleBindingAccessor(metav1.NamespaceDefault, fake), + startPolicyBinding: &authorizationapi.PolicyBinding{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace", + Name: "policybinding", + }, + PolicyRef: kapi.ObjectReference{ + Namespace: "namespace", + }, + RoleBindings: map[string]*authorizationapi.RoleBinding{ + "edit": { + ObjectMeta: metav1.ObjectMeta{ + Name: "edit", + Namespace: metav1.NamespaceDefault, + }, + Subjects: []kapi.ObjectReference{{ + Name: "foo", + Kind: authorizationapi.UserKind, + }}, + RoleRef: kapi.ObjectReference{ + Name: "edit", + Namespace: metav1.NamespaceDefault, + }, + }, + "custom": { + ObjectMeta: metav1.ObjectMeta{ + Name: "custom", + Namespace: metav1.NamespaceDefault, + }, + Subjects: []kapi.ObjectReference{{ + Name: "bar", + Kind: authorizationapi.UserKind, + }}, + RoleRef: kapi.ObjectReference{ + Name: "edit", + Namespace: metav1.NamespaceDefault, + }, + }, + }, + }, + startClusterPolicyBinding: nil, + cluster: false, + }, + "update-named-clusterbinding": { + rolename: "edit", + rolenamespace: "", + rolebindingname: "custom", + users: []string{ + "bar", + "baz", + }, + accessor: NewClusterRoleBindingAccessor(fake), + startPolicyBinding: nil, + startClusterPolicyBinding: &authorizationapi.ClusterPolicyBinding{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace", + Name: "clusterpolicybinding", + }, + PolicyRef: kapi.ObjectReference{ + Namespace: "namespace", + }, + RoleBindings: map[string]*authorizationapi.ClusterRoleBinding{ + "edit": { + ObjectMeta: metav1.ObjectMeta{ + Name: "edit", + }, + Subjects: []kapi.ObjectReference{{ + Name: "foo", + Kind: authorizationapi.UserKind, + }}, + RoleRef: kapi.ObjectReference{ + Name: "edit", + Namespace: metav1.NamespaceDefault, + }, + }, + "custom": { + ObjectMeta: metav1.ObjectMeta{ + Name: "custom", + }, + Subjects: []kapi.ObjectReference{{ + Name: "bar", + Kind: authorizationapi.UserKind, + }}, + RoleRef: kapi.ObjectReference{ + Name: "edit", + Namespace: metav1.NamespaceDefault, + }, + }, + }, + }, + cluster: true, + }, + } + + for tcName, tc := range tests { + pbList := &authorizationapi.PolicyBindingList{} + cpbList := &authorizationapi.ClusterPolicyBindingList{} + if tc.cluster { + fake.PrependReactor("get", "clusterpolicybindings", func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) { + return true, tc.startClusterPolicyBinding, nil + }) + + fake.PrependReactor("list", "clusterpolicybindings", func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) { + cpbList.Items = []authorizationapi.ClusterPolicyBinding{*tc.startClusterPolicyBinding} + return true, cpbList, nil + }) + } else { + fake.PrependReactor("get", "policybindings", func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) { + return true, tc.startPolicyBinding, nil + }) + + fake.PrependReactor("list", "policybindings", func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) { + pbList.Items = []authorizationapi.PolicyBinding{*tc.startPolicyBinding} + return true, pbList, nil + }) + } + + var actualRoleBinding *authorizationapi.RoleBinding + var clusterRoleBinding *authorizationapi.ClusterRoleBinding + if tc.cluster { + fake.PrependReactor("get", "clusterrolebindings", func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) { + if len(tc.rolebindingname) > 0 && tc.startClusterPolicyBinding.RoleBindings[tc.rolebindingname] != nil { + return true, tc.startClusterPolicyBinding.RoleBindings[tc.rolebindingname], nil + } + return true, nil, kapierrors.NewNotFound(authorizationapi.Resource("clusterrolebinding"), tc.rolebindingname) + }) + + fake.PrependReactor("update", "clusterrolebindings", func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) { + clusterRoleBinding = action.(clientgotesting.UpdateAction).GetObject().(*authorizationapi.ClusterRoleBinding) + return true, clusterRoleBinding, nil + }) + } + + fake.PrependReactor("get", "rolebindings", func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) { + if len(tc.rolebindingname) > 0 && tc.startPolicyBinding.RoleBindings[tc.rolebindingname] != nil { + return true, tc.startPolicyBinding.RoleBindings[tc.rolebindingname], nil + } + return true, nil, kapierrors.NewNotFound(authorizationapi.Resource("rolebinding"), tc.rolebindingname) + }) + + fake.PrependReactor("update", "rolebindings", func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) { + actualRoleBinding = action.(clientgotesting.UpdateAction).GetObject().(*authorizationapi.RoleBinding) + return true, actualRoleBinding, nil + }) + + fake.PrependReactor("create", "rolebindings", func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) { + actualRoleBinding = action.(clientgotesting.CreateAction).GetObject().(*authorizationapi.RoleBinding) + return true, actualRoleBinding, nil + }) + + o := &RoleModificationOptions{ + RoleName: tc.rolename, + RoleBindingName: tc.rolebindingname, + Users: tc.users, + RoleNamespace: tc.rolenamespace, + RoleBindingAccessor: tc.accessor, + } + + var err error + + err = o.AddRole() + if err != nil { + t.Errorf("%s: unexpected err %v", tcName, err) + } + + if len(tc.rolebindingname) < 1 { + // Check default case for rolebindingname which is the role name. + tc.rolebindingname = tc.rolename + } + + if tc.cluster { + actualRoleBinding = authorizationapi.ToRoleBinding(clusterRoleBinding) + } + + if tc.rolebindingname != actualRoleBinding.Name { + t.Errorf("%s: wrong rolebinding, expected: %v, actual: %v", tcName, tc.rolebindingname, actualRoleBinding.Name) + } + + if tc.rolename != actualRoleBinding.RoleRef.Name { + t.Errorf("%s: wrong role, expected: %v, actual: %v", tcName, tc.rolename, actualRoleBinding.RoleRef.Name) + } + + subs, _ := authorizationapi.StringSubjectsFor(actualRoleBinding.Namespace, actualRoleBinding.Subjects) + if !reflect.DeepEqual(tc.users, subs) { + t.Errorf("%s: err expected users: %v, actual: %v", tcName, tc.users, subs) + } + } +} diff --git a/pkg/cmd/admin/policy/policy.go b/pkg/cmd/admin/policy/policy.go index ee7e8e526ab3..543d30290926 100644 --- a/pkg/cmd/admin/policy/policy.go +++ b/pkg/cmd/admin/policy/policy.go @@ -117,6 +117,7 @@ func getUniqueName(basename string, existingNames *sets.String) string { type RoleBindingAccessor interface { GetExistingRoleBindingsForRole(roleNamespace, role string) ([]*authorizationapi.RoleBinding, error) GetExistingRoleBindingNames() (*sets.String, error) + GetRoleBinding(name string) (*authorizationapi.RoleBinding, error) UpdateRoleBinding(binding *authorizationapi.RoleBinding) error CreateRoleBinding(binding *authorizationapi.RoleBinding) error } @@ -165,6 +166,10 @@ func (a LocalRoleBindingAccessor) GetExistingRoleBindingNames() (*sets.String, e return ret, nil } +func (a LocalRoleBindingAccessor) GetRoleBinding(name string) (*authorizationapi.RoleBinding, error) { + return a.Client.RoleBindings(a.BindingNamespace).Get(name, metav1.GetOptions{}) +} + func (a LocalRoleBindingAccessor) UpdateRoleBinding(binding *authorizationapi.RoleBinding) error { _, err := a.Client.RoleBindings(a.BindingNamespace).Update(binding) return err @@ -222,6 +227,15 @@ func (a ClusterRoleBindingAccessor) GetExistingRoleBindingNames() (*sets.String, return ret, nil } +func (a ClusterRoleBindingAccessor) GetRoleBinding(name string) (*authorizationapi.RoleBinding, error) { + clusterRole, err := a.Client.ClusterRoleBindings().Get(name, metav1.GetOptions{}) + if err != nil { + return nil, err + } + roleBinding := authorizationapi.ToRoleBinding(clusterRole) + return roleBinding, nil +} + func (a ClusterRoleBindingAccessor) UpdateRoleBinding(binding *authorizationapi.RoleBinding) error { clusterBinding := authorizationapi.ToClusterRoleBinding(binding) _, err := a.Client.ClusterRoleBindings().Update(clusterBinding)