Skip to content

Conversation

@benbp
Copy link
Member

@benbp benbp commented May 31, 2024

Some pre/post test resources scripts require az login to run setup commands (e.g. for identity). It seems useful to propagate the oidc token variable across the pipeline context regardless.

We could unset the variables after the pre/post script runs as well if we think we don't want it available everywhere.

@benbp benbp requested a review from a team as a code owner May 31, 2024 20:17
@benbp benbp self-assigned this May 31, 2024
@benbp benbp added the Central-EngSys This issue is owned by the Engineering System team. label May 31, 2024
@benbp benbp requested review from danieljurek and weshaggard May 31, 2024 20:18
@azure-sdk
Copy link
Collaborator

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

Copy link
Member

@danieljurek danieljurek left a comment

Choose a reason for hiding this comment

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

This seems reasonable given the dependency on both Az PowerShell modules and az CLI in some of the pre and post scripts.

It probably also makes sense to Delete the credentials after the test resource provisioning script has finished.

inlineScript: |
Write-Host "##vso[task.setvariable variable=ARM_CLIENT_ID;issecret=true]$($env:servicePrincipalId)"
Write-Host "##vso[task.setvariable variable=ARM_TENANT_ID;issecret=true]$($env:tenantId)"
Write-Host "##vso[task.setvariable variable=ARM_OIDC_TOKEN;issecret=true]$($env:idToken)"
Copy link
Member

Choose a reason for hiding this comment

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

Should this be called OIDC? In other places we are calling it Federated token for example.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was copying this off the task you added. Want me to edit both and normalize?

Copy link
Member

Choose a reason for hiding this comment

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

Which task are you referring to? I don't remember using OIDC naming anywhere.

Copy link
Member Author

@benbp benbp Jun 3, 2024

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

lol... Ok I don't remember why I called it that. What do you think is a good name for this value?

Copy link
Member

Choose a reason for hiding this comment

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

It wasn't an issue with my manual tests running the identity pipeline, and we have a fairly complex arm deployment.

Copy link
Member

Choose a reason for hiding this comment

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

Also, at least in Identity's case, we only need the token in the -pre script, so that is well before the long part of the arm deployment. So perhaps we should document a best practice for deployments that need this, they should az login in a pre script.

Copy link
Member Author

Choose a reason for hiding this comment

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

Even if they az login, the token used for login will still expire right? But maybe similar to what Wes said in the other comment thread we can possibly renew the access token in the deploy script if it becomes a problem for people.

Copy link
Member

Choose a reason for hiding this comment

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

No, the OIDC token is exchanged for a regular access token, and that token would typically be good for at least an hour.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, great then.

- template: /eng/common/TestResources/setup-environments.yml

- ${{ if eq('true', parameters.UseFederatedAuth) }}:
- task: AzureCLI@2
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 know if there is a way to get similar values in the PS context? Also should we make this an opt-in as I'd hate for everyone to have to spend time logging into Az CLI and Az Powershell and not even end up needing these values.

Copy link
Member Author

@benbp benbp Jun 3, 2024

Choose a reason for hiding this comment

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

From what I could tell there is not. It's really unfortunate. Although maybe it's just the addSpnToEnvironment field that's specific to az cli and it's always there for az powershell? I can test.

I agree re: opt-in. I can add the parameter plumbing.

Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we call Get-AzAccessToken to get another token to use with az cli?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't find any of the inner source for that call :(

Copy link
Member

Choose a reason for hiding this comment

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

You can definitely get the client id and tenant from https://github.com/microsoft/azure-pipelines-tasks/blob/master/Tasks/AzurePowerShellV5/InitializeAz.ps1#L121 the question is how do you get a token. I'm guessing we could call Get-AzAccessToken to get one as needed depending on the resource type.

@benbp
Copy link
Member Author

benbp commented Jun 6, 2024

Closing in favor of #8377 and PreSteps calling the AzureCli task contained in this PR via tests.yml in the language repos (right now this is constrained to identity).

@benbp benbp closed this Jun 6, 2024
@benbp benbp deleted the benbp/test-resources-oidc-vars branch June 6, 2024 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Central-EngSys This issue is owned by the Engineering System team.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants