Skip to content

Replace go-azure-helpers with azidentity#3630

Merged
thomas11 merged 29 commits into
masterfrom
tkappler/azidentity2
Oct 23, 2024
Merged

Replace go-azure-helpers with azidentity#3630
thomas11 merged 29 commits into
masterfrom
tkappler/azidentity2

Conversation

@thomas11

@thomas11 thomas11 commented Oct 7, 2024

Copy link
Copy Markdown
Contributor

Overview

This PR implements #3493 which is part of Epic https://github.com/pulumi/home/issues/3576, Replace deprecated REST and auth packages in Azure Native.

The goal is to replace outdated and deprecated libraries that receive no bug fixes, block us from fixing some issues, and might pose security risks.

Parts

The legacy authentication setup is in auth.go. This PR adds auth_azidentity.go in parallel, with the same purpose: read all auth-related configuration, decide on the correct authentication method, and initialize it.

The core library used is Azure's official azidentity. It has various FooCredential types that all return an azcore.TokenCredential, abstracting the authentication method being used.

Thanks to the existing AzureClient abstraction, which has an azcore implementation, we can simply pass the new TokenCredential from auth_azidentity.go there without further changes.

One place where we have to plug in the new auth backend manually is the getClientToken RPC method in provider.go.

Rollout

For the sake of caution, the new authentication backend is off by default, behind feature flag PULUMI_USE_LEGACY_AUTH (defaulting to true). It's not 100% clear yet how and when we'll decide to turn it on.

Testing

New unit tests should be self-explanatory.

Existing integration tests inherently cover authentication, as long as they're run with the feature flag. For that purpose, there's a new GH workflow azcore-scheduled.yml. It runs every night and can be run on demand via workflow dispatch as well. Note that this means that the tests that are part of this PR's checks do not run on the new backend, only azcore_scheduled does.

I expanded the existing go-azure-in-azure test to use the provider binary under test even on the remote VM, and to create two user-managed identities. That forces the test program to configure which one should be used, increasing test coverage of user-managed identities.

A missing chunk of test coverage I'm aware of is that there's no integration test using the az CLI for authentication. It's quite tricky as the only interesting authentication method here is interactive user authentication via the Azure portal. The CLI can also do service principal authentication but we already test that.

Missing parts

  1. The CLI integration test mentioned above.
  2. Our two custom Azure storage resources, StorageAccountStaticWebsite and Blob, are built using an obsolete Azure SDK that only works with the legacy authentication stack. I've started on updating and converting it but kept it out of this PR since it can be committed separately. Unless that's completed, the legacy auth stack cannot be removed, though.

Review guide

  1. Start at provider(_test).go. The changes are the introduction of a new feature flag, and the related logic to choose the correct authentication package.
  2. auth_azidentity(_test).go are the meat of the PR. That's the new azidentity-based authentication stack. func (k *azureNativeProvider) newTokenCredential() is the main entry point. (Would be nice if authentication was in its own package but with the removal of the legacy stack it'll shrink.)
  3. examples/azure-native-sdk-v2/go-azure-in-azure/main.go is the integration test that runs Pulumi on an Azure VM to test managed identity authentication. It was extended to properly test user-assigned identities.
  4. The changes in .github go together with the ones in examples/examples_nodejs_keyvault_test.go. They constitute a new integration test that uses az CLI authentication.

@github-actions

github-actions Bot commented Oct 7, 2024

Copy link
Copy Markdown
Contributor

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Comment thread examples/azure-native-sdk-v2/go-azure-in-azure/main.go
@codecov

codecov Bot commented Oct 7, 2024

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.53648% with 57 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.15%. Comparing base (83d511e) to head (eb24b2d).
⚠️ Report is 255 commits behind head on master.

Files with missing lines Patch % Lines
provider/pkg/provider/auth_azidentity.go 79.21% 27 Missing and 10 partials ⚠️
provider/pkg/provider/provider.go 54.54% 19 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3630      +/-   ##
==========================================
+ Coverage   60.06%   60.15%   +0.09%     
==========================================
  Files          67       68       +1     
  Lines       10667    10878     +211     
==========================================
+ Hits         6407     6544     +137     
- Misses       3759     3831      +72     
- Partials      501      503       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@thomas11 thomas11 force-pushed the tkappler/azidentity2 branch 2 times, most recently from d917e24 to 364eba3 Compare October 9, 2024 08:26
@thomas11 thomas11 force-pushed the tkappler/azidentity2 branch from 364eba3 to 5d56097 Compare October 9, 2024 10:05
@thomas11 thomas11 force-pushed the tkappler/azidentity2 branch from 5d56097 to eb1cd09 Compare October 9, 2024 10:53

@danielrbradley danielrbradley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

At an overview the approach looks good. With the feature switch and existing test coverage, it looks quite safe to go in. Could probably go through the net-new code with a finer toothcomb, but overall the logic looks to mirror the existing code.

It might be worth adding some extra provider e2e tests which the new auth being used.

Comment thread provider/pkg/provider/auth_azidentity.go Outdated
@thomas11

Copy link
Copy Markdown
Contributor Author

It might be worth adding some extra provider e2e tests which the new auth being used.

The azcore-scheduled workflow runs all e2e tests on the new auth. Do you have any scenarios in mind that are not covered?

@thomas11 thomas11 force-pushed the tkappler/azidentity2 branch from eb1cd09 to 834a8de Compare October 10, 2024 14:31
case "usgovernment":
return azcloud.AzureGovernment
}
return azcloud.AzurePublic

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should it fail if we get some random "brazil" in here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, now I can see it's the same as it used to be, it's okay then

azCoreTokenCredential := azCoreTokenCredential{p: k}
var credential azcore.TokenCredential
if enableAzcoreBackend() {
credential, err = k.newTokenCredential()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add a similar log message "Using azidentity authentication"?

@pulumi-bot

Copy link
Copy Markdown
Contributor

This PR has been shipped in release v2.69.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a test using az CLI authentication

4 participants