Make some policy commands behave "better"

Instead of deprecating add/remove-role commands, change them to
behave better.
On add: do not add to a random rolebinding, always create a new
rolebinding if none was specified explicitly.
On Remove: if a rolebinding name is specified remove only from it.

Signed-off-by: Simo Sorce <[email protected]>

Author: simo5

pkg/oc/admin/policy/modify_roles.go

@@ -80,10 +80,9 @@ func NewCmdAddRoleToGroup(name, fullName string, f *clientcmd.Factory, out io.Wr
 				printSuccessForCommand(options.RoleName, true, "group", options.Targets, true, options.DryRun, out)
 			}
 		},
-		Deprecated: fmt.Sprintf("Use oc edit rolebinding"),
 	}
 
-	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.RoleBindingName, "rolebinding-name", "", "Name of the rolebinding to modify or create. If left empty creates a new rolebinding with a default name")
 	cmd.Flags().StringVar(&options.RoleNamespace, "role-namespace", "", "namespace where the role is located: empty means a role defined in cluster policy")
 
 	kcmdutil.AddDryRunFlag(cmd)
@@ -117,7 +116,7 @@ func NewCmdAddRoleToUser(name, fullName string, f *clientcmd.Factory, out io.Wri
 		Deprecated: fmt.Sprintf("Use oc edit rolebinding"),
 	}
 
-	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.RoleBindingName, "rolebinding-name", "", "Name of the rolebinding to modify or create. If left empty creates a new rolebinding with a default name")
 	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")
 
@@ -147,9 +146,9 @@ func NewCmdRemoveRoleFromGroup(name, fullName string, f *clientcmd.Factory, out
 				printSuccessForCommand(options.RoleName, false, "group", options.Targets, true, options.DryRun, out)
 			}
 		},
-		Deprecated: fmt.Sprintf("Use oc edit rolebinding"),
 	}
 
+	cmd.Flags().StringVar(&options.RoleBindingName, "rolebinding-name", "", "Name of the rolebinding to modify. If left empty it will operate on all rolebindings")
 	cmd.Flags().StringVar(&options.RoleNamespace, "role-namespace", "", "namespace where the role is located: empty means a role defined in cluster policy")
 
 	kcmdutil.AddDryRunFlag(cmd)
@@ -179,9 +178,9 @@ func NewCmdRemoveRoleFromUser(name, fullName string, f *clientcmd.Factory, out i
 				printSuccessForCommand(options.RoleName, false, "user", options.Targets, true, options.DryRun, out)
 			}
 		},
-		Deprecated: fmt.Sprintf("Use oc edit rolebinding"),
 	}
 
+	cmd.Flags().StringVar(&options.RoleBindingName, "rolebinding-name", "", "Name of the rolebinding to modify. If left empty it will operate on all rolebindings")
 	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")
 
@@ -211,10 +210,9 @@ func NewCmdAddClusterRoleToGroup(name, fullName string, f *clientcmd.Factory, ou
 				printSuccessForCommand(options.RoleName, true, "group", options.Targets, false, options.DryRun, out)
 			}
 		},
-		Deprecated: fmt.Sprintf("Use oc edit clusterrolebinding"),
 	}
 
-	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.RoleBindingName, "rolebinding-name", "", "Name of the rolebinding to modify or create. If left empty creates a new rolebinding with a default name")
 	kcmdutil.AddDryRunFlag(cmd)
 	kcmdutil.AddPrinterFlags(cmd)
 	return cmd
@@ -242,10 +240,9 @@ func NewCmdAddClusterRoleToUser(name, fullName string, f *clientcmd.Factory, out
 				printSuccessForCommand(options.RoleName, true, "user", options.Targets, false, options.DryRun, out)
 			}
 		},
-		Deprecated: fmt.Sprintf("Use oc edit clusterrolebinding"),
 	}
 
-	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.RoleBindingName, "rolebinding-name", "", "Name of the rolebinding to modify or create. If left empty creates a new rolebinding with a default name")
 	cmd.Flags().StringSliceVarP(&saNames, "serviceaccount", "z", saNames, "service account in the current namespace to use as a user")
 
 	kcmdutil.AddDryRunFlag(cmd)
@@ -274,9 +271,10 @@ func NewCmdRemoveClusterRoleFromGroup(name, fullName string, f *clientcmd.Factor
 				printSuccessForCommand(options.RoleName, false, "group", options.Targets, false, options.DryRun, out)
 			}
 		},
-		Deprecated: fmt.Sprintf("Use oc edit clusterrolebinding"),
 	}
 
+	cmd.Flags().StringVar(&options.RoleBindingName, "rolebinding-name", "", "Name of the rolebinding to modify. If left empty it will operate on all rolebindings")
+
 	kcmdutil.AddDryRunFlag(cmd)
 	kcmdutil.AddPrinterFlags(cmd)
 	return cmd
@@ -304,9 +302,9 @@ func NewCmdRemoveClusterRoleFromUser(name, fullName string, f *clientcmd.Factory
 				printSuccessForCommand(options.RoleName, false, "user", options.Targets, false, options.DryRun, out)
 			}
 		},
-		Deprecated: fmt.Sprintf("Use oc edit clusterrolebinding"),
 	}
 
+	cmd.Flags().StringVar(&options.RoleBindingName, "rolebinding-name", "", "Name of the rolebinding to modify. If left empty it will operate on all rolebindings")
 	cmd.Flags().StringSliceVarP(&saNames, "serviceaccount", "z", saNames, "service account in the current namespace to use as a user")
 
 	kcmdutil.AddDryRunFlag(cmd)
@@ -436,18 +434,7 @@ func (o *RoleModificationOptions) getUserSpecifiedBinding() (*authorizationapi.R
 }
 
 func (o *RoleModificationOptions) getUnspecifiedBinding() (*authorizationapi.RoleBinding, bool /* isUpdate */, error) {
-	// Look for existing bindings by role.
-	roleBindings, err := o.RoleBindingAccessor.GetExistingRoleBindingsForRole(o.RoleNamespace, o.RoleName)
-	if err != nil {
-		return nil, false, 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
-		return roleBindings[0], true, nil
-	}
-
-	// Create a new rolebinding with the default naming.
+	// Always create a new role binding with the default naming
 	roleBinding := &authorizationapi.RoleBinding{}
 	roleBindingNames, err := o.RoleBindingAccessor.GetExistingRoleBindingNames()
 	if err != nil {
@@ -520,9 +507,20 @@ subjectCheck:
 }
 
 func (o *RoleModificationOptions) RemoveRole() error {
-	roleBindings, err := o.RoleBindingAccessor.GetExistingRoleBindingsForRole(o.RoleNamespace, o.RoleName)
-	if err != nil {
-		return err
+	var roleBindings []*authorizationapi.RoleBinding
+	var err error
+	if len(o.RoleBindingName) > 0 {
+		existingRoleBinding, err := o.RoleBindingAccessor.GetRoleBinding(o.RoleBindingName)
+		if err != nil {
+			return err
+		}
+		roleBindings = make([]*authorizationapi.RoleBinding, 1)
+		roleBindings[0] = existingRoleBinding
+	} else {
+		roleBindings, err = o.RoleBindingAccessor.GetExistingRoleBindingsForRole(o.RoleNamespace, o.RoleName)
+		if err != nil {
+			return err
+		}
 	}
 	if len(roleBindings) == 0 {
 		return fmt.Errorf("unable to locate RoleBinding for %v/%v", o.RoleNamespace, o.RoleName)
@@ -554,7 +552,7 @@ func (o *RoleModificationOptions) RemoveRole() error {
 	for _, roleBinding := range roleBindings {
 		roleBinding.Subjects = removeSubjects(roleBinding.Subjects, subjectsToRemove)
 
-		err = o.RoleBindingAccessor.UpdateRoleBinding(roleBinding)
+		err := o.RoleBindingAccessor.UpdateRoleBinding(roleBinding)
 		if err != nil {
 			return err
 		}