Obtain auth tokens via the new azidentity/MSAL method #1926
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. |
This looks great! Pardon my ignorance but what auth options are currently possible with the old ADAL? I think if we get parity there and can get test coverage for the scenarios, we should feel fairly confident about pushing this through. |
provider/pkg/provider/provider.go
Outdated
| @@ -233,7 +250,7 @@ func (k *azureNativeProvider) Invoke(ctx context.Context, req *rpc.InvokeRequest | |||
| if endpointArg := args["endpoint"]; endpointArg.HasValue() && endpointArg.IsString() { | |||
| endpoint = endpointArg.StringValue() | |||
| } | |||
| token, err := k.getOAuthToken(ctx, auth, endpoint) | |||
| token, err := k.getOAuthTokenNew(ctx, auth, endpoint) | |||
There was a problem hiding this comment.
If we have some trepidation on support, we can add the new API behind an experimental provider option - we do this often in other providers, e.g. kubernetes: https://github.com/pulumi/pulumi-kubernetes/blob/master/provider/pkg/provider/provider.go#L436
There was a problem hiding this comment.
That's the kind of pointer I was hoping for :) Is there any precedent for making instead the previous version opt-in? In this case, customers are already asking for the new version so we don't want to delay too much.
Note that - unless I misunderstand something? - there is no API change here, the change is internal.
There was a problem hiding this comment.
That's the kind of pointer I was hoping for :) Is there any precedent for making instead the previous version opt-in? In this case, customers are already asking for the new version so we don't want to delay too much.
Yes we have done something similar elsewhere before, i.e. make reverting to old default opt-in.
Note that - unless I misunderstand something? - here is no API change here, the change is internal.
Yup we are on the same page here. I honestly think we can probably be somewhat aggressive here and just go with it. If users are having issues, they can pin to an older version of the provider as a fallback.
There was a problem hiding this comment.
If the new way of working is backward compatible then let's just push forward
If it's not, we may need to allow people to opt-in
Does the PR have any schema changes?Looking good! No breaking changes found. |
ADAL with the hashicorp package tries these auth options:
I have replicated this order in my PR, except (2) is not supported yet by azidentity. And I've tested only (5) so far. |
There was a problem hiding this comment.
This looks great - really nice that we can re-use all the same configuration.
Btw, another alternative to using q is to attach to the provider and step through - I've just written the instructions for debugging providers in our team docs.
I have now tested service principal with cert and service principal with client secret. MSI (managed identity) is only available when running in certain Azure environments, but I have verified that setting ARM_USE_MSI attempts to use it as expected. |
thomas11
force-pushed
the
tkappler/msal-azidentity-auth
branch
from
71cfff0
to
f4f35a5
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
thomas11
force-pushed
the
tkappler/msal-azidentity-auth
branch
from
f4f35a5
to
e33a3eb
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
thomas11
force-pushed
the
tkappler/msal-azidentity-auth
branch
from
2c66daf
to
324583a
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
thomas11
force-pushed
the
tkappler/msal-azidentity-auth
branch
from
324583a
to
22d2d3d
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
thomas11
force-pushed
the
tkappler/msal-azidentity-auth
branch
from
22d2d3d
to
316d69c
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
thomas11
added
the
resolution/duplicate
|
Implemented by #2320 |
This PR replaces the default implementation of
getOAuthTokenwith one that uses the new azidentity package, part of azure-sdk-for-go, to obtain an Azure auth token. This is also known as MSAL, whereas the previous, deprecated method is ADAL.Multitenant OAuth is not implemented yet in azidentity, that's the only feature gap I'm aware of. For that, we'll continue to fall back on ADAL.