-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate some policy commands #18102
Conversation
fox is guarding the hen house on this one, but needs @openshift/cli-review |
pkg/oc/admin/policy/cani.go
Outdated
@@ -71,6 +71,7 @@ func NewCmdCanI(name, fullName string, f *clientcmd.Factory, out io.Writer) *cob | |||
os.Exit(2) | |||
} | |||
}, | |||
Deprecated: fmt.Sprintf("Use oc auth can-i"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simo5 Mind adding single quotes around the new oc
command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also use the moved
function where these commands are specifed, similarly to this. This way when you invoke oc policy
these deprecated commands will be appropriately marked as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simo5 Mind adding single quotes around the new oc command?
@juanvallejo why? I don't recall seeing oc in quotes anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing, use lowercase, since this gets appended to the following sentence Command xyz is deprecated, <here goes your string>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? I don't recall seeing oc in quotes anywhere
Saw this being the pattern in a few places:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into cobra (see here) and that's why it does make sense to be it lowercase.
One comment, but otherwise, merge at will after it's addressed |
/hold |
pkg/oc/admin/policy/modify_roles.go
Outdated
@@ -80,6 +80,7 @@ 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"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why all the unnecessary calls to fmt.Sprintf
?
pkg/oc/admin/policy/cani.go
Outdated
@@ -71,6 +71,7 @@ func NewCmdCanI(name, fullName string, f *clientcmd.Factory, out io.Writer) *cob | |||
os.Exit(2) | |||
} | |||
}, | |||
Deprecated: fmt.Sprintf("Use oc auth can-i"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also use the moved
function where these commands are specifed, similarly to this. This way when you invoke oc policy
these deprecated commands will be appropriately marked as well.
pkg/oc/admin/policy/cani.go
Outdated
@@ -71,6 +71,7 @@ func NewCmdCanI(name, fullName string, f *clientcmd.Factory, out io.Writer) *cob | |||
os.Exit(2) | |||
} | |||
}, | |||
Deprecated: fmt.Sprintf("Use oc auth can-i"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simo5 Mind adding single quotes around the new oc command?
@juanvallejo why? I don't recall seeing oc in quotes anywhere
/retest |
197dcbf
to
c989199
Compare
/hold cancel |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits and one question.
pkg/oc/admin/policy/cani.go
Outdated
@@ -71,6 +71,7 @@ func NewCmdCanI(name, fullName string, f *clientcmd.Factory, out io.Writer) *cob | |||
os.Exit(2) | |||
} | |||
}, | |||
Deprecated: fmt.Sprintf("use 'oc auth can-i'"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for fmt.Sprintf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, forgot, will rebase and fix this
@@ -104,6 +104,7 @@ func NewCmdReconcileClusterRoleBindings(name, fullName string, f *clientcmd.Fact | |||
kcmdutil.CheckErr(err) | |||
} | |||
}, | |||
Deprecated: fmt.Sprintf("use 'oc auth reconcile'"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
@@ -50,22 +50,21 @@ os::cmd::expect_failure_and_text 'oc policy add-role-to-user' 'you must specify | |||
os::cmd::expect_failure_and_text 'oc policy add-role-to-user -z NamespaceWithoutRole' 'you must specify a role' | |||
os::cmd::expect_failure_and_text 'oc policy add-role-to-user view' 'you must specify at least one user or service account' | |||
|
|||
os::cmd::expect_success_and_text 'oc policy add-role-to-group cluster-admin system:unauthenticated' 'role "cluster-admin" added: "system:unauthenticated"' | |||
os::cmd::expect_success_and_text 'oc policy add-role-to-user cluster-admin system:no-user' 'role "cluster-admin" added: "system:no-user"' | |||
os::cmd::expect_success_and_text 'oc policy add-role-to-group cluster-admin --rolebinding-name cluster-admin system:unauthenticated' 'role "cluster-admin" added: "system:unauthenticated"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any backwards compatible implications in changing this, or this is just cosmetics for stability of tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No real backwards incompatibility, just tests were making assumptions that are not true "in real life" about which rolebindings were going to be used by the test.
Because the commands was picking the first rolebinding it would find you can never be certain of what rolebinding was used. By changing the command to always pick a new rolebinding we may cause some incorrect expectations to "fail early", so that people will end up fixing latent bugs that were always there.
As you can see by tests being all green, luckily this faulty assumption seem to not have been relied upon anywhere in our code base except for the tests testing the feature itself.
Push people to use native RBAC commands where reasonable equivalents exist, or where the replacement command will lead to better practices. Signed-off-by: Simo Sorce <simo@redhat.com>
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 <simo@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: simo5, soltysh The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Automatic merge from submit-queue (batch tested with PRs 18415, 18419, 18268, 18102, 18440). |
/test all [submit-queue is verifying that this PR is safe to merge] |
Followup to openshift#18102 Signed-off-by: Simo Sorce <simo@redhat.com>
Followup to openshift#18102 Signed-off-by: Simo Sorce <simo@redhat.com>
Followup to openshift#18102 Signed-off-by: Simo Sorce <simo@redhat.com>
var roleBindings []*authorizationapi.RoleBinding | ||
var err error | ||
if len(o.RoleBindingName) > 0 { | ||
existingRoleBinding, err := o.RoleBindingAccessor.GetRoleBinding(o.RoleBindingName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing the role check like getUserSpecifiedBinding
:
// Check that we update the rolebinding for the intended role.
if roleBinding.RoleRef.Name != o.RoleName || roleBinding.RoleRef.Namespace != o.RoleNamespace {
return nil, false, fmt.Errorf("rolebinding %s found for role %s, not %s", roleBinding.Name, roleBinding.RoleRef.Name, o.RoleName)
}
This should error instead of doing the wrong thing:
$ oc new-project lol3
Now using project "lol3" on server "https://10.10.10.35:8443".
You can add applications to this project with the 'new-app' command. For example, try:
oc new-app centos/ruby-22-centos7~https://github.com/openshift/ruby-ex.git
to build a new example application in Ruby.
$ oc policy add-role-to-user edit foo
role "edit" added: "foo"
$ oc policy add-role-to-user admin foo
role "admin" added: "foo"
$ oc get rolebinding
NAME ROLE USERS GROUPS SERVICE ACCOUNTS SUBJECTS
admin /admin system:admin
admin-0 /admin foo
edit /edit foo
system:deployers /system:deployer deployer
system:image-builders /system:image-builder builder
system:image-pullers /system:image-puller system:serviceaccounts:lol3
$ oc policy remove-role-from-user admin foo --rolebinding-name=edit
role "admin" removed: "foo"
$ oc get rolebinding
NAME ROLE USERS GROUPS SERVICE ACCOUNTS SUBJECTS
admin /admin system:admin
admin-0 /admin foo
system:deployers /system:deployer deployer
system:image-builders /system:image-builder builder
system:image-pullers /system:image-puller system:serviceaccounts:lol3
Followup to openshift#18102 Signed-off-by: Simo Sorce <simo@redhat.com>
Followup to openshift#18102 Signed-off-by: Simo Sorce <simo@redhat.com>
Fixes: #18096