Skip to content

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Jun 28, 2024

Instead, we use azure.identity azure package and AzureCliCredential

@ghost ghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 28, 2024
@JulieLeeMSFT JulieLeeMSFT added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 1, 2024
@JulieLeeMSFT JulieLeeMSFT added this to the 9.0.0 milestone Jul 1, 2024
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

@JulieLeeMSFT
Copy link
Member

CC @agocke @jkoritzinsky.

@JulieLeeMSFT JulieLeeMSFT requested a review from hoyosjs July 1, 2024 20:11
@JulieLeeMSFT
Copy link
Member

@hoyosjs, we need your code review that removes the Azure Key from SPMI collection.

@jkoritzinsky
Copy link
Member

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)?

Copy link
Member

@hoyosjs hoyosjs left a 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"
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced AzureCliCredential with DefaultAzureCredential

@hoyosjs
Copy link
Member

hoyosjs commented Jul 2, 2024

Just checked and the perms on the MI sound reasonable on the account and the service connection also looks good.

@EgorBo
Copy link
Member Author

EgorBo commented Jul 2, 2024

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)?

I've kicked off two runs:

  • spmi-collect (will take some time, but some 'collect' job already finished & uploaded successfully)
  • jitrollingbuild (finished successfully)

Since both of them use the key.

@EgorBo EgorBo marked this pull request as ready for review July 2, 2024 13:25
@EgorBo
Copy link
Member Author

EgorBo commented Jul 2, 2024

I've kicked off two runs

Seem to be working correctly. Do we need to save our current key somewhere or it's fine to just delete it?

@kunalspathak
Copy link
Contributor

I've kicked off two runs

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.

Copy link
Member

@jkoritzinsky jkoritzinsky left a 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.

@EgorBo EgorBo merged commit d198728 into dotnet:main Jul 4, 2024
@EgorBo EgorBo deleted the spmi-1es branch July 4, 2024 13:54
@github-actions github-actions bot locked and limited conversation to collaborators Aug 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants