credentials: Expose provider names

[radeksimko]

This should help in situations such as debugging the ChainProvider or even giving the user better idea of what's happening under the hood - i.e. where are the used credentials coming from.

This PR may introduce a breaking change for users having custom providers as these will be missing the new method per modified interface (Name()).

[jasdel]

@radeksimko thanks for taking the time to put this PR together. I like the idea of adding a Name to the credentials.Value type so that it's easier to see where your credentials came from at runtime.

I think we make this change without creating a breaking change to the Providers interface. If instead of adding a Name func to each Provider. The providers could be responsible for setting the fields themselves. Since the providers are solely responsible for filling the credentials.Value's value it would make sense for them to also populate this field.

Its possible that a provider implementor forgets to provide this field, but it would also not require a breaking change, nor add additional logic to the credentials.ChainProvider and credentials.Credentials types.

[radeksimko]

Since the providers are solely responsible for filling the credentials.Value's value it would make sense for them to also populate this field.

I was actually playing with this version before I submitted the PR. The reason I have chosen the implementation with Name() func is because this would work at any time in any context, no matter how would you use the provider.

If we were to rely on a struct string field, we'd also have to rely on the user to either always use New*() constructors (no way to enforce this in go) or make sure that all provider methods are consistently setting the provider name, or just accept the fact that the name will only be available once you call Retrieve().

The ultimate question is then, do we want to make this work?

p := &awsCredentials.StaticProvider{Value: awsCredentials.Value{
    AccessKeyID:     key,
    SecretAccessKey: secret,
    SessionToken:    token,
}}
p.Name // this would be always empty, because no method was called to set it

Its possible that a provider implementor forgets to provide this field, but it would also not require a breaking change, nor add additional logic to the credentials.ChainProvider and credentials.Credentials types.

Agreed that no changes would have to be made to credentials.Credentials.

I think it's necessary to add some logic to the credentials.ChainProvider as it's responsible for choosing the real provider, otherwise we'd be getting ChainProvider as the name of the provider, which isn't very useful (depends on context, but most of the time you'd probably want to see the chosen provider name from the chain).

I'm fine with implementing the changes you suggested, after all I've been in that stage already a few days ago 😃 , can you just confirm you're ok with all the pros (no breaking change) and cons (providerName likely to be empty in many cases)?

[jasdel]

Thanks for the feedback @radeksimko. I agree avoiding the breaking change would be great. We'd like to go down this path of ensuring no breaking changes.

If we were to rely on a struct string field, we'd also have to rely on the user to either always use New*() constructors (no way to enforce this in go) or make sure that all provider methods are consistently setting the provider name, or just accept the fact that the name will only be available once you call Retrieve().

I was thinking that the individual Providers would have a const provider name which is used by default when the Provider returns a credentials Value via Retrieve. Optionally we could add a way of setting a custom provider name which will override the default in Retrieve if set.

I think it's necessary to add some logic to the credentials.ChainProvider as it's responsible for choosing the real provider

If the chain provider just passes the credential object return by the nested provider then the ProviderName field would already be set and reflect the value that was set by the nested provider before it returned the name. Chain provider wouldn't need to mutate the received credentials.Value it would just pass it along. If instead we went with the Name() method we would need to implement it via an optional NamedProvider like interface that Providers could optionally support. In that case we could expect Providers to not also be named.

The ultimate question is then, do we want to make this work? awsCredentials.StaticProvider{Value: awsCredentials.Value{...

In this case I would expect the StaticProvider's Retrieve() function to be responsible for setting the ProviderName. Its up for discussion if the StaticProvider should ignore, passion or augment the passed in credentials.Value.ProviderName field.

[radeksimko]

Constant is a great idea (I'm actually now embarrassed I didn't come up with this 😃 ) and solves the problems I raised! I will make those changes asap.

[jasdel]

Awesome!, and thanks for your time and help getting this feature implemented.

[radeksimko]

@jasdel Modifications done.

I thought I could make this even cleaner by moving each provider into a special package from credentials, but that would introduce breaking change, so I decided not to do that.

OT: Thank you (and the rest of the team behind AWS SDK) for changing my perception of Amazon & OSS. Nothing personal against your colleagues & maintainers of Boto, but PRs w/out any response for 1year+ (I am able to find even 4 years old PRs with 0 responses) don't usually leave any positive feelings. 😉

[jasdel]

@radeksimko thanks for making the updates, and glad we can help :) We'll review your changes and get back to you with feedback soon. Thanks again for taking the time to put this PR together.

[jasdel]

Looks good @radeksimko thanks again for taking the time, and working with us to add this feature to the credentials.