-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Managed Identity: Support specifying the client ID when requesting a token from the metadata service #115
Conversation
…token from the metadata service
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor question, but otherwise LGTM 🚀
@@ -31,14 +31,18 @@ func (a *MsiAuthorizer) Token() (*oauth2.Token, error) { | |||
"api-version": []string{a.conf.MsiApiVersion}, | |||
"resource": []string{a.conf.Resource}, | |||
} | |||
|
|||
if a.conf.ClientID != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a chance conf
can ever be nil here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, will add checks 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one comment but this otherwise LGTM 👍
@@ -76,13 +80,22 @@ func (a *MsiAuthorizer) Token() (*oauth2.Token, error) { | |||
|
|||
// MsiConfig configures an MsiAuthorizer. | |||
type MsiConfig struct { | |||
// ClientID is optionally used to determine which application to assume when a resource has multiple managed identities | |||
ClientID string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is an optional value I'd suggest this should be a pointer rather than checking != ""?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good shout, this is a pattern used in all the authorizers - will look at improving this
Note: breaking change for the
auth.NewMsiAuthorizer()
andauth.NewMsiConfig()
functionsRelated: hashicorp/terraform-provider-azuread#604