Update sso credential provider to support token provider #4875
Conversation
sync feat-sso-session into current branch
aws/credentials/ssocreds/provider.go
Outdated
|
|
||
| // Used by the SSOCredentialProvider if a token configuration | ||
| // profile is used in the shared config | ||
| SSOTokenProvider *SSOTokenProvider |
There was a problem hiding this comment.
@aajtodd @lucix-aws i am curious about your opinion here.
in my V2 implementation i did this same thing.
at the time, not knowing Go that well (or the Go SDK), this seemed fine to me. but i think i prob should have used the TokenProvider interface rather than the SSOTokenProvider struct implementation. this is because our use of it doesnt seem to depend on anything specific to the SSOTokenProvider: its just calling RetrieveBearerToken. also, another benefit as @wty-Bryant pointed out in a conversation I had with him, if this were an interface we can more easily unit test it.
There was a problem hiding this comment.
At first glance I think that's ok if you want to go down that route.
There was a problem hiding this comment.
Ok I will try to implement the interface route
aws/credentials/ssocreds/provider.go
Outdated
|
|
||
| // Used by the SSOCredentialProvider if a token configuration | ||
| // profile is used in the shared config | ||
| SSOTokenProvider *SSOTokenProvider |
There was a problem hiding this comment.
At first glance I think that's ok if you want to go down that route.
sync feat-sso-session into current branch.
isaiahvita
dismissed
their stale review
accidentally approved, while still reviewing it. need to finish my review.
aws/credentials/ssocreds/provider.go
Outdated
| @@ -64,9 +65,12 @@ type Provider struct { | |||
| // parameter will be ignored. | |||
| CachedTokenFilepath string | |||
|
|
|||
| // Used by the SSOCredentialProvider to judge if TokenProvider is configured | |||
| HasTokenProvider bool | |||
There was a problem hiding this comment.
Why do we need this? bearer.TokenProvider is an interface, you can check if it's nil as you were before.
aws/session/credentials.go
Outdated
| @@ -187,7 +187,8 @@ func resolveSSOCredentials(cfg *aws.Config, sharedCfg sharedConfig, handlers req | |||
| oidcClient := ssooidc.New(mySession, cfgCopy) | |||
| tokenProvider := ssocreds.NewSSOTokenProvider(oidcClient, cachedPath) | |||
| optFns = append(optFns, func(p *ssocreds.Provider) { | |||
| p.SSOTokenProvider = tokenProvider | |||
| p.HasTokenProvider = true | |||
| p.TokenProvider = *tokenProvider | |||
There was a problem hiding this comment.
Why are we dereferencing here? This doesn't look right. The methods should probably be pointer receivers (looks like they are value receivers currently).
| }, nil | ||
| }, | ||
| }, | ||
| HasTokenProvider: true, |
There was a problem hiding this comment.
wasnt this field removed in the source code? why is it still in the test data?
|
|
||
| // Used by the SSOCredentialProvider if a token configuration | ||
| // profile is used in the shared config | ||
| TokenProvider bearer.TokenProvider |
There was a problem hiding this comment.
nit: id like to discuss this as a team. my understanding is that this can be optional. and my understanding of Go is that the best way to signal optionality is via pointer types? but im wondering if thats only the case for primitive types and not object types?
There was a problem hiding this comment.
The bear.TokenProvider is an interface so it can be nil which I think can mark it is optional? Interface doesn't support pointer receiver afaik
Update sso credential provider logic to support both sso token provider and legacy sso config.
The manual test of sso-session section support in shared config file follows steps below:
This PR will resolve #4649 with prvious PRs on the same branch.