return error when long-form sa name is used #17061
Conversation
openshift-ci-robot
added
the
size/XS
|
/test extended_networking_minimal |
|
/retest |
| @@ -320,6 +321,13 @@ func (o *RoleModificationOptions) CompleteUserWithSA(f *clientcmd.Factory, cmd * | |||
| return errors.New("you must specify at least one user or service account") | |||
| } | |||
|
|
|||
| // return an error if a fully-qualified service-account name is used | |||
There was a problem hiding this comment.
You should instead check that all names are valid SA names.
There was a problem hiding this comment.
Thanks, kept this check and added an additional check for the overall validity of each given SA name
juanvallejo
force-pushed
the
jvallejo/prevent-use-of-sa-long-form-names-with-z
branch
from
f94a3a5
to
764476d
Compare
openshift-ci-robot
added
size/S
juanvallejo
force-pushed
the
jvallejo/prevent-use-of-sa-long-form-names-with-z
branch
from
764476d
to
34f7b71
Compare
pkg/oc/admin/policy/modify_roles.go
Outdated
| // return an error if a fully-qualified service-account name is used | ||
| for _, sa := range saNames { | ||
| if strings.HasPrefix(sa, "system:serviceaccount") { | ||
| return errors.New("--serviceaccount (-z) should only be used with short-form serviceaccount names (e.g. \"default\")") |
There was a problem hiding this comment.
Could use back ticks to avoid having to escape.
pkg/oc/admin/policy/modify_roles.go
Outdated
| return errors.New("--serviceaccount (-z) should only be used with short-form serviceaccount names (e.g. \"default\")") | ||
| } | ||
|
|
||
| if len(validation.ValidateServiceAccountName(sa, false)) > 0 { |
There was a problem hiding this comment.
Why suppress the actual errors?
There was a problem hiding this comment.
Saw this done in at least one other place, so copied it; will store and append the errors to the final message
There was a problem hiding this comment.
Updated message to show indented causes in new lines:
$ oc policy add-role-to-user admin -z :default
error: ":default" is not a valid serviceaccount name:
a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')
See 'oc policy add-role-to-user -h' for help and examples.
34f7b71
to
24088e7
Compare
|
@enj thanks for the feedback, review comments addressed |
pkg/oc/admin/policy/modify_roles.go
Outdated
| if errCauses := validation.ValidateServiceAccountName(sa, false); len(errCauses) > 0 { | ||
| message := fmt.Sprintf("%q is not a valid serviceaccount name:\n", sa) | ||
| for _, cause := range errCauses { | ||
| message += " " + cause |
There was a problem hiding this comment.
I could, although I've seen it done both ways throughout the code - would one format over the other make much difference here?
bug: 1425398 Returns an error when the long-form name of a ServiceAccount is used with the --serviceaccount (-z) flag in `oc policy ...' commands, or if the name given is invalid.
juanvallejo
force-pushed
the
jvallejo/prevent-use-of-sa-long-form-names-with-z
branch
from
24088e7
to
d70ebd6
Compare
|
/test extended_conformance_install_update |
|
/lgtm |
|
/approve |
openshift-ci-robot
added
the
kind/bug
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enj, fabianofranz, juanvallejo 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 |
openshift-merge-robot
added
the
approved
|
Automatic merge from submit-queue. |
juanvallejo
deleted the
jvallejo/prevent-use-of-sa-long-form-names-with-z
branch
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1425398
Returns an error when the long-form name of a ServiceAccount is used
with the --serviceaccount (-z) flag in `oc policy ...' commands.
/assign enj
cc @openshift/cli-review