-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Remove uses of CLRJIT_AZ_KEY/clrjit_key1 from SPMI #104164
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
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
CC @agocke @jkoritzinsky. |
@hoyosjs, we need your code review that removes the Azure Key from SPMI collection. |
Can we see a test run of the superpmi-collect pipeline with these changes (and a different JIT-EE guid to avoid overwriting existing collections)? |
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.
From an Azure.Identity perspective, this looks good. As long as you've tested the identity has all necessary scopes and it's a federated connection, looks good for Wave 1 reqs.
scriptType: 'pscore' | ||
scriptLocation: 'inlineScript' | ||
inlineScript: | | ||
$Env:AZCOPY_AUTO_LOGIN_TYPE="AZCLI" |
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.
Do you shell onto azcopy?
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.
@hoyosjs not sure I understand the question. we don't use azcopy
here explicitly, but we use AzureCliCredential
provider inside the python script we invoke here.
Perhaps, we can also remove it and use DefaultAzureCredential
I didn't test it - is it an issue?
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.
It's not an issue - but DefaultAzureCredential
should be enough. The only caveat BTW is the possibility that DefaultAzureCredential might pick up other ambient identities (MI's and Workload identities) before the Az CLI creds. if you ever hit this, just swap DefaultAzureCredential
for AzureCliCredential
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.
Replaced AzureCliCredential
with DefaultAzureCredential
Just checked and the perms on the MI sound reasonable on the account and the service connection also looks good. |
I've kicked off two runs:
Since both of them use the key. |
Seem to be working correctly. Do we need to save our current key somewhere or it's fine to just delete it? |
Just make sure we have backup. lets talk offline. |
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.
Code and pipeline runs look good to me.
Instead, we use
azure.identity
azure package andAzureCliCredential