Conversation
|
Amplify deployment status
|
7604f5a to
044ceb6
Compare
tool/tsh/common/access_request.go
Outdated
|
|
||
| var caps *types.AccessCapabilities | ||
| err = tc.WithRootClusterClient(cf.Context, func(clt authclient.ClientI) error { | ||
| caps, err = clt.GetAccessCapabilities( |
There was a problem hiding this comment.
What's the difference between GetAccessCapabilities and ListRequestableRoles?
The latter seems more appropriate (at least in name).
There was a problem hiding this comment.
Yeah, good point, ListRequestableRoles looks much better here
tool/tsh/common/access_request.go
Outdated
| ) | ||
|
|
||
| err = tc.WithRootClusterClient(cf.Context, func(clt authclient.ClientI) error { | ||
| for { |
There was a problem hiding this comment.
I would take a look at clientutils.Resources, which is a helper function that implements the pagination loop here.
There was a problem hiding this comment.
So, I used clientutils.Resources here instead of my original loop. I also thought about adding a new client method with a suitable signature, something like:
ListRequestableRolesV2(ctx context.Context, pageSize int, pageToken string) (*proto.ListRequestableRolesResponse, error)but I wasn’t sure. We have examples like:
// ListAccessLists returns a paginated list of access lists.
ListAccessLists(context.Context, int, string) ([]*accesslist.AccessList, string, error)
// ListAccessListsV2 returns a filtered and sorted paginated list of access lists.
ListAccessListsV2(context.Context, *accesslistv1.ListAccessListsV2Request) ([]*accesslist.AccessList, string, error)And the original ListAccessLists, which had a suitable signature, is now deprecated.
tool/tsh/common/access_request.go
Outdated
| resp, err := clt.ListRequestableRoles(ctx, req) | ||
| if err != nil { | ||
| return nil, "", trace.Wrap(err) | ||
| } | ||
|
|
||
| return resp.Roles, resp.NextPageToken, nil |
There was a problem hiding this comment.
If we used the generated Getters which are nil safe we could eliminate a few lines here.
| resp, err := clt.ListRequestableRoles(ctx, req) | |
| if err != nil { | |
| return nil, "", trace.Wrap(err) | |
| } | |
| return resp.Roles, resp.NextPageToken, nil | |
| resp, err := clt.ListRequestableRoles(ctx, req) | |
| return resp.GetRoles(), resp.GetNextPageToken(), trace.Wrap(err) |
There was a problem hiding this comment.
I really like that, I’ll update the PR
fe586d9 to
d9d7e1c
Compare
|
@tangyatsu See the table below for backport results.
|
What
Resolves: #7693
tsh now supports listing requestable roles via
tsh request search --roles, enforces mutual exclusivity between--rolesand--kindflags, and adds a hint totsh request new --roles, that suggests usingtsh request search --roleswhen the requested role is not allowed.changelog: Added
--rolesflag fortsh request search, allowing users to list all requestable roles. This flag is mutually exclusive with--kindManual Tests
A local test cluster was created with a requester role that is allowed to request several other roles.
A test user with the requester role was created.
The following tests were performed:
tsh request searchreturns an errortsh request search --rolessucceeds, lists requestable rolestsh request search --kindreturns an errortsh request search --kind=dbsucceedstsh request search --roles --kind=dbreturns errortsh request search --roles --kindreturns errortsh request new --rolesconfirmed the new hint suggestingtsh request search --roles