Skip to content

Commit 0720d4b

Browse files
author
OpenShift Bot
authored
Merge pull request #15196 from enj/enj/i/remove_policy_cli
Merged by openshift-bot
2 parents bca9c79 + 775b51d commit 0720d4b

File tree

4 files changed

+64
-84
lines changed

4 files changed

+64
-84
lines changed

‎pkg/authorization/apis/authorization/helpers.go

+1-21
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package authorization
22

33
import (
44
"fmt"
5-
"sort"
65
"strings"
76
"unicode"
87

@@ -105,25 +104,6 @@ func (r PolicyRule) CompactString() string {
105104
return fmt.Sprintf(formatString, formatArgs...)
106105
}
107106

108-
func getRoleBindingValues(roleBindingMap map[string]*RoleBinding) []*RoleBinding {
109-
ret := []*RoleBinding{}
110-
for _, currBinding := range roleBindingMap {
111-
ret = append(ret, currBinding)
112-
}
113-
114-
return ret
115-
}
116-
func SortRoleBindings(roleBindingMap map[string]*RoleBinding, reverse bool) []*RoleBinding {
117-
roleBindings := getRoleBindingValues(roleBindingMap)
118-
if reverse {
119-
sort.Sort(sort.Reverse(RoleBindingSorter(roleBindings)))
120-
} else {
121-
sort.Sort(RoleBindingSorter(roleBindings))
122-
}
123-
124-
return roleBindings
125-
}
126-
127107
type PolicyBindingSorter []PolicyBinding
128108

129109
func (s PolicyBindingSorter) Len() int {
@@ -136,7 +116,7 @@ func (s PolicyBindingSorter) Swap(i, j int) {
136116
s[i], s[j] = s[j], s[i]
137117
}
138118

139-
type RoleBindingSorter []*RoleBinding
119+
type RoleBindingSorter []RoleBinding
140120

141121
func (s RoleBindingSorter) Len() int {
142122
return len(s)

‎pkg/cmd/admin/policy/policy.go

+24-23
Original file line numberDiff line numberDiff line change
@@ -132,34 +132,33 @@ func NewLocalRoleBindingAccessor(bindingNamespace string, client client.Interfac
132132
}
133133

134134
func (a LocalRoleBindingAccessor) GetExistingRoleBindingsForRole(roleNamespace, role string) ([]*authorizationapi.RoleBinding, error) {
135-
existingBindings, err := a.Client.PolicyBindings(a.BindingNamespace).Get(authorizationapi.GetPolicyBindingName(roleNamespace), metav1.GetOptions{})
135+
existingBindings, err := a.Client.RoleBindings(a.BindingNamespace).List(metav1.ListOptions{})
136136
if err != nil && !kapierrors.IsNotFound(err) {
137137
return nil, err
138138
}
139139

140140
ret := make([]*authorizationapi.RoleBinding, 0)
141141
// see if we can find an existing binding that points to the role in question.
142-
for _, currBinding := range existingBindings.RoleBindings {
143-
if currBinding.RoleRef.Name == role {
144-
t := currBinding
145-
ret = append(ret, t)
142+
for i := range existingBindings.Items {
143+
// shallow copy outside of the loop so that we can take its address
144+
currBinding := existingBindings.Items[i]
145+
if currBinding.RoleRef.Name == role && currBinding.RoleRef.Namespace == roleNamespace {
146+
ret = append(ret, &currBinding)
146147
}
147148
}
148149

149150
return ret, nil
150151
}
151152

152153
func (a LocalRoleBindingAccessor) GetExistingRoleBindingNames() (*sets.String, error) {
153-
policyBindings, err := a.Client.PolicyBindings(a.BindingNamespace).List(metav1.ListOptions{})
154+
roleBindings, err := a.Client.RoleBindings(a.BindingNamespace).List(metav1.ListOptions{})
154155
if err != nil {
155156
return nil, err
156157
}
157158

158159
ret := &sets.String{}
159-
for _, existingBindings := range policyBindings.Items {
160-
for _, currBinding := range existingBindings.RoleBindings {
161-
ret.Insert(currBinding.Name)
162-
}
160+
for _, currBinding := range roleBindings.Items {
161+
ret.Insert(currBinding.Name)
163162
}
164163

165164
return ret, nil
@@ -187,36 +186,38 @@ func NewClusterRoleBindingAccessor(client client.Interface) ClusterRoleBindingAc
187186
}
188187

189188
func (a ClusterRoleBindingAccessor) GetExistingRoleBindingsForRole(roleNamespace, role string) ([]*authorizationapi.RoleBinding, error) {
190-
uncast, err := a.Client.ClusterPolicyBindings().Get(authorizationapi.GetPolicyBindingName(roleNamespace), metav1.GetOptions{})
191-
if err != nil && !kapierrors.IsNotFound(err) {
189+
// cluster role bindings can only reference cluster roles
190+
if roleNamespace != "" {
191+
return nil, nil
192+
}
193+
194+
existingBindings, err := a.Client.ClusterRoleBindings().List(metav1.ListOptions{})
195+
if err != nil {
192196
return nil, err
193197
}
194-
existingBindings := authorizationapi.ToPolicyBinding(uncast)
195198

196199
ret := make([]*authorizationapi.RoleBinding, 0)
197200
// see if we can find an existing binding that points to the role in question.
198-
for _, currBinding := range existingBindings.RoleBindings {
199-
if currBinding.RoleRef.Name == role {
200-
t := currBinding
201-
ret = append(ret, t)
201+
for i := range existingBindings.Items {
202+
// shallow copy outside of the loop so that we can take its address
203+
currBinding := existingBindings.Items[i]
204+
if currBinding.RoleRef.Name == role && currBinding.RoleRef.Namespace == "" {
205+
ret = append(ret, authorizationapi.ToRoleBinding(&currBinding))
202206
}
203207
}
204208

205209
return ret, nil
206210
}
207211

208212
func (a ClusterRoleBindingAccessor) GetExistingRoleBindingNames() (*sets.String, error) {
209-
uncast, err := a.Client.ClusterPolicyBindings().List(metav1.ListOptions{})
213+
existingBindings, err := a.Client.ClusterRoleBindings().List(metav1.ListOptions{})
210214
if err != nil {
211215
return nil, err
212216
}
213-
policyBindings := authorizationapi.ToPolicyBindingList(uncast)
214217

215218
ret := &sets.String{}
216-
for _, existingBindings := range policyBindings.Items {
217-
for _, currBinding := range existingBindings.RoleBindings {
218-
ret.Insert(currBinding.Name)
219-
}
219+
for _, currBinding := range existingBindings.Items {
220+
ret.Insert(currBinding.Name)
220221
}
221222

222223
return ret, nil

‎pkg/cmd/admin/policy/remove_from_project.go

+37-38
Original file line numberDiff line numberDiff line change
@@ -96,11 +96,12 @@ func (o *RemoveFromProjectOptions) Complete(f *clientcmd.Factory, args []string,
9696
}
9797

9898
func (o *RemoveFromProjectOptions) Run() error {
99-
bindingList, err := o.Client.PolicyBindings(o.BindingNamespace).List(metav1.ListOptions{})
99+
roleBindings, err := o.Client.RoleBindings(o.BindingNamespace).List(metav1.ListOptions{})
100100
if err != nil {
101101
return err
102102
}
103-
sort.Sort(authorizationapi.PolicyBindingSorter(bindingList.Items))
103+
// maintain David's hack from #1973 (see #1975, #1976 and https://bugzilla.redhat.com/show_bug.cgi?id=1215969)
104+
sort.Sort(sort.Reverse(authorizationapi.RoleBindingSorter(roleBindings.Items)))
104105

105106
usersRemoved := sets.String{}
106107
groupsRemoved := sets.String{}
@@ -109,47 +110,45 @@ func (o *RemoveFromProjectOptions) Run() error {
109110

110111
subjectsToRemove := authorizationapi.BuildSubjects(o.Users, o.Groups, uservalidation.ValidateUserName, uservalidation.ValidateGroupName)
111112

112-
for _, currPolicyBinding := range bindingList.Items {
113-
for _, currBinding := range authorizationapi.SortRoleBindings(currPolicyBinding.RoleBindings, true) {
114-
originalSubjects := make([]kapi.ObjectReference, len(currBinding.Subjects))
115-
copy(originalSubjects, currBinding.Subjects)
116-
oldUsers, oldGroups, oldSAs, oldOthers := authorizationapi.SubjectsStrings(currBinding.Namespace, originalSubjects)
117-
oldUsersSet, oldGroupsSet, oldSAsSet, oldOtherSet := sets.NewString(oldUsers...), sets.NewString(oldGroups...), sets.NewString(oldSAs...), sets.NewString(oldOthers...)
113+
for _, currBinding := range roleBindings.Items {
114+
originalSubjects := make([]kapi.ObjectReference, len(currBinding.Subjects))
115+
copy(originalSubjects, currBinding.Subjects)
116+
oldUsers, oldGroups, oldSAs, oldOthers := authorizationapi.SubjectsStrings(currBinding.Namespace, originalSubjects)
117+
oldUsersSet, oldGroupsSet, oldSAsSet, oldOtherSet := sets.NewString(oldUsers...), sets.NewString(oldGroups...), sets.NewString(oldSAs...), sets.NewString(oldOthers...)
118118

119-
currBinding.Subjects = removeSubjects(currBinding.Subjects, subjectsToRemove)
120-
newUsers, newGroups, newSAs, newOthers := authorizationapi.SubjectsStrings(currBinding.Namespace, currBinding.Subjects)
121-
newUsersSet, newGroupsSet, newSAsSet, newOtherSet := sets.NewString(newUsers...), sets.NewString(newGroups...), sets.NewString(newSAs...), sets.NewString(newOthers...)
119+
currBinding.Subjects = removeSubjects(currBinding.Subjects, subjectsToRemove)
120+
newUsers, newGroups, newSAs, newOthers := authorizationapi.SubjectsStrings(currBinding.Namespace, currBinding.Subjects)
121+
newUsersSet, newGroupsSet, newSAsSet, newOtherSet := sets.NewString(newUsers...), sets.NewString(newGroups...), sets.NewString(newSAs...), sets.NewString(newOthers...)
122122

123-
if len(currBinding.Subjects) == len(originalSubjects) {
124-
continue
125-
}
123+
if len(currBinding.Subjects) == len(originalSubjects) {
124+
continue
125+
}
126126

127-
_, err = o.Client.RoleBindings(o.BindingNamespace).Update(currBinding)
128-
if err != nil {
129-
return err
130-
}
127+
_, err = o.Client.RoleBindings(o.BindingNamespace).Update(&currBinding)
128+
if err != nil {
129+
return err
130+
}
131131

132-
roleDisplayName := fmt.Sprintf("%s/%s", currBinding.RoleRef.Namespace, currBinding.RoleRef.Name)
133-
if len(currBinding.RoleRef.Namespace) == 0 {
134-
roleDisplayName = currBinding.RoleRef.Name
135-
}
132+
roleDisplayName := fmt.Sprintf("%s/%s", currBinding.RoleRef.Namespace, currBinding.RoleRef.Name)
133+
if len(currBinding.RoleRef.Namespace) == 0 {
134+
roleDisplayName = currBinding.RoleRef.Name
135+
}
136136

137-
if diff := oldUsersSet.Difference(newUsersSet); len(diff) != 0 {
138-
fmt.Fprintf(o.Out, "Removing %s from users %v in project %s.\n", roleDisplayName, diff.List(), o.BindingNamespace)
139-
usersRemoved.Insert(diff.List()...)
140-
}
141-
if diff := oldGroupsSet.Difference(newGroupsSet); len(diff) != 0 {
142-
fmt.Fprintf(o.Out, "Removing %s from groups %v in project %s.\n", roleDisplayName, diff.List(), o.BindingNamespace)
143-
groupsRemoved.Insert(diff.List()...)
144-
}
145-
if diff := oldSAsSet.Difference(newSAsSet); len(diff) != 0 {
146-
fmt.Fprintf(o.Out, "Removing %s from serviceaccounts %v in project %s.\n", roleDisplayName, diff.List(), o.BindingNamespace)
147-
sasRemoved.Insert(diff.List()...)
148-
}
149-
if diff := oldOtherSet.Difference(newOtherSet); len(diff) != 0 {
150-
fmt.Fprintf(o.Out, "Removing %s from subjects %v in project %s.\n", roleDisplayName, diff.List(), o.BindingNamespace)
151-
othersRemoved.Insert(diff.List()...)
152-
}
137+
if diff := oldUsersSet.Difference(newUsersSet); len(diff) != 0 {
138+
fmt.Fprintf(o.Out, "Removing %s from users %v in project %s.\n", roleDisplayName, diff.List(), o.BindingNamespace)
139+
usersRemoved.Insert(diff.List()...)
140+
}
141+
if diff := oldGroupsSet.Difference(newGroupsSet); len(diff) != 0 {
142+
fmt.Fprintf(o.Out, "Removing %s from groups %v in project %s.\n", roleDisplayName, diff.List(), o.BindingNamespace)
143+
groupsRemoved.Insert(diff.List()...)
144+
}
145+
if diff := oldSAsSet.Difference(newSAsSet); len(diff) != 0 {
146+
fmt.Fprintf(o.Out, "Removing %s from serviceaccounts %v in project %s.\n", roleDisplayName, diff.List(), o.BindingNamespace)
147+
sasRemoved.Insert(diff.List()...)
148+
}
149+
if diff := oldOtherSet.Difference(newOtherSet); len(diff) != 0 {
150+
fmt.Fprintf(o.Out, "Removing %s from subjects %v in project %s.\n", roleDisplayName, diff.List(), o.BindingNamespace)
151+
othersRemoved.Insert(diff.List()...)
153152
}
154153
}
155154

‎test/cmd/policy-storage-admin.sh

+2-2
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,8 @@ os::cmd::expect_success_and_text 'oc policy can-i create pv' 'yes'
3131
os::cmd::expect_success_and_text 'oc policy can-i create storageclass' 'yes'
3232

3333
# Test failure to change policy on users for storage-admin
34-
os::cmd::expect_failure_and_text 'oc policy add-role-to-user admin storage-adm' 'cannot get policybindings'
35-
os::cmd::expect_failure_and_text 'oc policy remove-user screeley' 'cannot list policybindings'
34+
os::cmd::expect_failure_and_text 'oc policy add-role-to-user admin storage-adm' 'cannot list rolebindings'
35+
os::cmd::expect_failure_and_text 'oc policy remove-user screeley' 'cannot list rolebindings'
3636
os::cmd::expect_success 'oc logout'
3737

3838
# Test that scoped storage-admin now an admin in project foo

0 commit comments

Comments
 (0)