Replace Azure Service Principal auth with Azure Workload Identity auth in functional tests #7787
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: willdavsmith <[email protected]>
Signed-off-by: willdavsmith <[email protected]>
Signed-off-by: willdavsmith <[email protected]>
Signed-off-by: willdavsmith <[email protected]>
willdavsmith
had a problem deploying
to
functional-tests
GitHub Actions
Failure
Unit Tests3 299 tests ±0 3 293 ✅ ±0 3m 53s ⏱️ -2s Results for commit 5529ca1. ± Comparison against base commit d01ecf9. This pull request removes 6 and adds 6 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7787 +/- ##
==========================================
+ Coverage 61.05% 61.06% +0.01%
==========================================
Files 523 523
Lines 27457 27466 +9
==========================================
+ Hits 16763 16772 +9
- Misses 9209 9210 +1
+ Partials 1485 1484 -1 ☔ View full report in Codecov by Sentry. |
Signed-off-by: willdavsmith <[email protected]>
willdavsmith
had a problem deploying
to
functional-tests
GitHub Actions
Failure
Signed-off-by: willdavsmith <[email protected]>
willdavsmith
had a problem deploying
to
functional-tests
GitHub Actions
Failure
Signed-off-by: willdavsmith <[email protected]>
willdavsmith
had a problem deploying
to
functional-tests
GitHub Actions
Failure
Signed-off-by: willdavsmith <[email protected]>
willdavsmith
had a problem deploying
to
functional-tests
GitHub Actions
Failure
Signed-off-by: willdavsmith <[email protected]>
willdavsmith
had a problem deploying
to
functional-tests
GitHub Actions
Failure
Signed-off-by: willdavsmith <[email protected]>
willdavsmith
had a problem deploying
to
functional-tests
GitHub Actions
Failure
Signed-off-by: willdavsmith <[email protected]>
willdavsmith
had a problem deploying
to
functional-tests
GitHub Actions
Failure
Signed-off-by: willdavsmith <[email protected]>
willdavsmith
had a problem deploying
to
functional-tests
GitHub Actions
Failure
Signed-off-by: willdavsmith <[email protected]>
willdavsmith
had a problem deploying
to
functional-tests
GitHub Actions
Failure
Signed-off-by: willdavsmith <[email protected]>
willdavsmith
had a problem deploying
to
functional-tests
GitHub Actions
Failure
Signed-off-by: willdavsmith <[email protected]>
willdavsmith
had a problem deploying
to
functional-tests
GitHub Actions
Failure
Signed-off-by: willdavsmith <[email protected]>
willdavsmith
commented
Aug 8, 2024
| @@ -171,14 +171,21 @@ func refreshAzureWorkloadIdentityCredentials(ctx context.Context, c *UCPCredenti | |||
|
|
|||
| logger.Info("Retrieved Azure Credential - ClientID: " + azureWorkloadIdentityCredential.ClientID) | |||
|
|
|||
| var opt *azidentity.DefaultAzureCredentialOptions | |||
| var opt *azidentity.WorkloadIdentityCredentialOptions | |||
There was a problem hiding this comment.
I misunderstood what this code did on the first workload identity PR. this should have been WorkloadIdentityCredential instead.
willdavsmith
commented
Aug 8, 2024
| // this well known path. | ||
| // https://azure.github.io/azure-workload-identity/docs/installation/mutating-admission-webhook.html | ||
| // https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs#argument-reference | ||
| azureOIDCTokenFilePath = "/var/run/secrets/azure/tokens/azure-identity-token" |
There was a problem hiding this comment.
these are the changes to make Terraform + Azure WI work.
willdavsmith
commented
Aug 8, 2024
| configMap[azureUseCLIParam] = false | ||
| configMap[azureUseOIDCParam] = true | ||
| configMap[azureOIDCTokenFilePathParam] = azureOIDCTokenFilePath |
There was a problem hiding this comment.
now this should work for AKS and non-AKS clusters
willdavsmith
commented
Aug 8, 2024
| @@ -1,6 +1,6 @@ | |||
| module github.com/radius-project/radius/test/magpiego | |||
|
|
|||
| go 1.22 | |||
| go 1.22.0 | |||
There was a problem hiding this comment.
I changed this because I was getting an error trying to build this image on arm64
There was a problem hiding this comment.
Interesting. Can you share the error message? It was working fine for me.
There was a problem hiding this comment.
go: downloading go1.22 (darwin/arm64)
go: download go1.22 for darwin/arm64: toolchain not available
willdavsmith
commented
Aug 8, 2024
| @@ -2,7 +2,7 @@ terraform { | |||
| required_providers { | |||
| azurerm = { | |||
| source = "hashicorp/azurerm" | |||
| version = "~> 3.0.0" | |||
| version = "~> 3.114.0" | |||
There was a problem hiding this comment.
~> 3.0.0 will only match 3.0.*, but workload identity for azurerm needs greater than 3.7. 3.114 is the latest version as of today
willdavsmith
changed the title
willdavsmith
commented
Aug 8, 2024
| @@ -95,7 +95,7 @@ jobs: | |||
| build: | |||
| name: Build Radius for test | |||
| runs-on: ubuntu-latest | |||
| if: github.event_name == 'repository_dispatch' || (github.event_name == 'schedule' && github.repository == 'radius-project/radius') || github.event_name == 'workflow_run' | |||
| if: github.event_name == 'repository_dispatch' || (github.event_name == 'schedule' && github.repository == 'radius-project/radius') || github.event_name == 'workflow_run' || github.event_name == 'workflow_dispatch' | |||
There was a problem hiding this comment.
workflow_dispatch won't work yet unless you set up the federated credential to allow your branch. for testing this is what I did. in the future we should switch to github environments, but for now it doesn't hurt to have this.
Signed-off-by: willdavsmith <[email protected]>
willdavsmith
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
willdavsmith
commented
Aug 9, 2024
| @@ -49,6 +49,9 @@ type UCPCredentialOptions struct { | |||
|
|
|||
| // ClientOptions is the options for azure client. | |||
| ClientOptions *azcore.ClientOptions | |||
|
|
|||
| // TokenFilePath is the path to the azure token file (for use with Azure workload identity) | |||
| TokenFilePath string | |||
There was a problem hiding this comment.
added this to make this file unit testable. the azure sdk library will use the environment variable in the real world
willdavsmith
changed the title
willdavsmith
changed the title
lakshmimsft
reviewed
Aug 9, 2024
| // this well known path. | ||
| // https://azure.github.io/azure-workload-identity/docs/installation/mutating-admission-webhook.html | ||
| // https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs#argument-reference | ||
| azureOIDCTokenFilePath = "/var/run/secrets/azure/tokens/azure-identity-token" |
There was a problem hiding this comment.
Q: will we be able to retrieve the TokenFilePath in the fetchAzureCredentials() call (eg line 129 below) instead of the const value here, say, in a future PR?
There was a problem hiding this comment.
we totally could - if we have a user ask us to make this configurable then we can. for now it looks like most if not everyone uses the standard well known file path established by the azure ad workload identity project.
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Signed-off-by: willdavsmith <[email protected]>
willdavsmith
had a problem deploying
to
functional-tests
GitHub Actions
Failure
Signed-off-by: Will Smith <[email protected]>
willdavsmith
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
superbeeny
pushed a commit
to superbeeny/radius
that referenced
this pull request
Aug 14, 2024
…h in functional tests (radius-project#7787) # Description * Switch Azure SP auth to WI in functional tests * Fix issue with Azure WI + application WI in applications-rp * Fix issue with Azure WI in terraform * Adding `workflow-dispatch` triggers Example successful workflow: https://github.com/radius-project/radius/actions/runs/10311808492 ## Type of change <!-- Please select **one** of the following options that describes your change and delete the others. Clearly identifying the type of change you are making will help us review your PR faster, and is used in authoring release notes. If you are making a bug fix or functionality change to Radius and do not have an associated issue link please create one now. --> - This pull request fixes a bug in Radius and has an approved issue (issue link required). - This pull request adds or changes features of Radius and has an approved issue (issue link required). - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional). <!-- Please update the following to link the associated issue. This is required for some kinds of changes (see above). --> Fixes: radius-project#7715 --------- Signed-off-by: willdavsmith <[email protected]> Signed-off-by: Will Smith <[email protected]>
Reshrahim
pushed a commit
to Reshrahim/radius
that referenced
this pull request
Aug 27, 2024
…h in functional tests (radius-project#7787) # Description * Switch Azure SP auth to WI in functional tests * Fix issue with Azure WI + application WI in applications-rp * Fix issue with Azure WI in terraform * Adding `workflow-dispatch` triggers Example successful workflow: https://github.com/radius-project/radius/actions/runs/10311808492 ## Type of change <!-- Please select **one** of the following options that describes your change and delete the others. Clearly identifying the type of change you are making will help us review your PR faster, and is used in authoring release notes. If you are making a bug fix or functionality change to Radius and do not have an associated issue link please create one now. --> - This pull request fixes a bug in Radius and has an approved issue (issue link required). - This pull request adds or changes features of Radius and has an approved issue (issue link required). - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional). <!-- Please update the following to link the associated issue. This is required for some kinds of changes (see above). --> Fixes: radius-project#7715 --------- Signed-off-by: willdavsmith <[email protected]> Signed-off-by: Will Smith <[email protected]> Signed-off-by: Reshma Abdul Rahim <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
workflow-dispatchtriggersExample successful workflow: https://github.com/radius-project/radius/actions/runs/10311808492
Type of change
Fixes: #7715