[v10] Connect: Fetch correct role set when listing db users (#13790) #13877
Conversation
* Move fetchRoleSet from tsh/db to lib/services as FetchAllClusterRoles. * Connect: Fetch correct role set when listing db users * Add tests for FetchAllClusterRoles
github-actions
bot
added
the
tsh
ravicious
requested review from
atburke and
codingllama
and removed request for
zmb3 and
GavinFrazar
ravicious
changed the title
| @@ -32,7 +32,7 @@ type Config struct { | |||
| // InsecureSkipVerify is an option to skip TLS cert check | |||
| InsecureSkipVerify bool | |||
| // Log is a component logger | |||
| Log logrus.FieldLogger | |||
| Log *logrus.Entry | |||
There was a problem hiding this comment.
What's wrong with using FieldLogger here?
There was a problem hiding this comment.
Ah, that was because initially I was passing log to the new function (see #13790 (comment)). Then I removed it in favor of using just the default logger but left this change here. Without it the type system didn't let me pass Log as logrus.Entry and from what I remember this is the type that we typically use when passing a logger around.
| }) | ||
| if err != nil { | ||
| return nil, trace.Wrap(err) | ||
| } | ||
|
|
||
| defer authClient.Close() |
There was a problem hiding this comment.
Do we need a nil check here, or is authClient guaranteed to be initialized if we got this far?
There was a problem hiding this comment.
I think it is guaranteed to be initialized? If the function passed to addMetadataToRetryableError returns an error, the execution stops at line 173. The only way for that function to return nil is after the execution has succeeded and authClient is equal to the return value of proxyClient.ConnectToCluster.
| @@ -80,7 +78,7 @@ func onListDatabases(cf *CLIConf) error { | |||
| return trace.Wrap(err) | |||
| } | |||
|
|
|||
| roleSet, err := fetchRoleSet(cf.Context, cluster, profile) | |||
| roleSet, err := services.FetchAllClusterRoles(cf.Context, cluster, profile.Roles, profile.Traits) | |||
There was a problem hiding this comment.
@rosstimothy do you foresee any performance issues with this approach?
| @@ -735,6 +735,38 @@ func FetchRoles(roleNames []string, access RoleGetter, traits map[string][]strin | |||
| return NewRoleSet(roles...), nil | |||
| } | |||
|
|
|||
| // CurrentUserRoleGetter limits the interface of auth.ClientI to methods needed by FetchClusterRoles. | |||
There was a problem hiding this comment.
| // CurrentUserRoleGetter limits the interface of auth.ClientI to methods needed by FetchClusterRoles. | |
| // CurrentUserRoleGetter limits the interface of auth.ClientI to methods needed by FetchAllClusterRoles. |
nit
There was a problem hiding this comment.
Thanks, I'll push a PR for that tomorrow.
Backport #13790. Needs #13625 to be merged first.