Pin AKS credential pipeline to the deployment subscription - #19219
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19219Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19219" |
github-actions
Bot
added
the
area-integrations
There was a problem hiding this comment.
Pull request overview
Pins AKS credential acquisition and resource-group discovery to Aspire’s deployment subscription.
Changes:
- Reads Azure context from deployment state.
- Adds
--subscriptionto Azure CLI calls and reconnect output. - Adds focused unit tests with an injected CLI runner.
Show a summary per file
| File | Description |
|---|---|
AzureKubernetesEnvironmentResource.AksPipeline.cs |
Scopes AKS CLI operations to the deployment subscription. |
AzureKubernetesInfrastructureTests.cs |
Tests deployment-state lookup and CLI arguments. |
Aspire.Hosting.Azure.Kubernetes.Tests.csproj |
Includes the shared in-memory state manager. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
This comment has been minimized.
This comment has been minimized.
PR Testing ReportPR Information
Artifact Version Verification
Changes AnalyzedFiles Changed
Change Categories
Test Scenarios ExecutedScenario 1: Dogfood artifact version verificationObjective: Confirm the tested bits are the PR's bits. Installed via the PR dogfood script into an isolated Source checkout independently confirmed at the same SHA with a clean tree. Scenario 2: The fix is present in the shipped NuGet packageObjective: Verify the fix actually compiled into the artifact users would consume, not just the source tree. Extracted
The shipped composite format string is: Scenario 3: Full test suite at PR headObjective: Confirm the suite passes on exactly the PR commit. Cross-platform coverage came from this PR's own CI run (31457579207), where Scenario 4: Real
|
| Scenario | Status | Notes |
|---|---|---|
| 1. Artifact version verification | ✅ Passed | 13.6.0-pr.19219.g7ac00b8d ↔ 7ac00b8d1f |
| 2. Fix present in shipped nupkg | ✅ Passed | Format string + new error message in shipped DLL |
| 3. Full suite at PR head | ✅ Passed | 73/73 local; CI green on ubuntu + windows |
4. Real az contract validation |
✅ Passed | Retargeting proven live; 0 parse errors |
| 5. Missing subscription (unhappy) | ✅ Passed | Fails fast, actionable message |
6. az failure (unhappy) |
✅ Passed | Surfaced as InvalidOperationException |
| 7. Anti-tautology regression proof | ✅ Passed | Correct test fails on subtle regression |
Overall Result
✅ PR VERIFIED
Known limitations
- No live
aspire deployto a real AKS cluster was performed. Provisioning an AKS cluster is slow
and costly, andGetAksCredentialsAsyncis only reachable afterProvisionInfrastructureStepName
completes with real Bicep outputs. The ~15 lines insideGetAksCredentialsAsyncthat wire the
resolved subscription intoGetResourceGroupAsync/FetchKubeConfigAsyncare therefore covered by
unit tests plus code inspection, not by a live deploy. - Scenario 4a demonstrates the retargeting mechanism using
az resource listgenerically, because no
AKS cluster was available in any accessible subscription. The command and the--subscriptionglobal
argument are the same ones the fix emits, so the result transfers, but it is not an AKS-specific run. - Existing AKS deployment E2E tests authenticate with an ambient subscription that already matches the
deployment subscription, so they structurally cannot observe this bug. Closing that gap needs a second
subscription in CI and is worth tracking as follow-up.
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Addendum: Real AKS Cluster ValidationThe earlier test report noted one residual gap: no real AKS cluster was provisioned, so The discriminating setupThe reason the existing 12 AKS deployment E2E tests structurally cannot catch this bug is that they authenticate with an ambient
Both in tenant Test 1 — A/B against a pre-existing real clusterAgainst a real running cluster (
The variant-B kubeconfig was verified genuinely usable — Test 2 — Full
|
Addendum — deployment E2E run against PR bitsRan Result: ✅ 1/1 passed, 11m 58s, exit code 0. Versions under test
Resolved from the PR hive ( Evidence for the changed code pathThe user-facing hint now carries the subscription: Helm deploying successfully is the real signal — it proves the kubeconfig fetched by the pinned Scope caveatThis is a regression test, not a discriminating one. The harness itself runs Unrelated harness bug foundThe first attempt reported a green pass while actually exercising |
Addresses review feedback on #19219: the existing tests all invoked GetResourceGroupAsync / FetchKubeConfigAsync directly, so nothing executed GetAksCredentialsAsync itself. Reverting the call site to the pre-fix unscoped `az` invocation left every test green -- the exact mutation that shipped as #19216 was undetectable one level up from where we asserted. Adds two internal test seams at the action boundary (az CLI path resolution and command execution) and a step-level test that resolves the registered aks-get-credentials-{name} step from the pipeline and runs its Action. It asserts the full command line of both `az` calls carry --subscription, plus the "Connect to cluster" summary hint, which was previously untested. Verified by mutation: restoring the unscoped call site now fails the new test while the six helper-level tests still pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b15f47d-c522-4d08-a976-b3ee69c4ebb4
There was a problem hiding this comment.
Review details
Suppressed comments (1)
src/Aspire.Hosting.Azure.Kubernetes/AzureKubernetesEnvironmentResource.AksPipeline.cs:295
- The reconnect command leaves all values unquoted, but valid Azure resource-group names may contain parentheses (for example,
team(prod)). In common shells those parentheses are syntax, so the command advertised as copy-pasteable fails even though the actualazinvocation above correctly quotes its values. Quote the resource group, cluster name, and subscription ID here, and update the summary assertion accordingly.
new MarkdownString(
$"`az aks get-credentials --resource-group {resourceGroup} --name {clusterName} --subscription {subscriptionId}`"));
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This comment has been minimized.
This comment has been minimized.
James Newton-King (JamesNK)
left a comment
James Newton-King (JamesNK)
left a comment
There was a problem hiding this comment.
Found 1 correctness issue: existing AKS resources with a resource-specific Azure scope can acquire credentials from the global deployment scope.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review details
Suppressed comments (2)
tests/Aspire.Hosting.Azure.Kubernetes.Tests/AzureKubernetesInfrastructureTests.cs:238
- This test exercises
GetAzureDeploymentContextAsync, but that helper has no production caller; the registered credential step goes throughResolveDeploymentScopeAsync, which duplicates the missing-subscription check. A regression in the actual pipeline branch would therefore leave this test green. ExerciseResolveDeploymentScopeAsyncor the registered step with missing state, and remove or reuse the dead helper.
var exception = await Assert.ThrowsAsync<InvalidOperationException>(
() => AzureKubernetesEnvironmentResource.GetAzureDeploymentContextAsync(
services,
TestContext.Current.CancellationToken));
src/Aspire.Hosting.Azure.Kubernetes/AzureKubernetesEnvironmentResource.AksPipeline.cs:302
- The displayed reconnect command is still not always copy-pasteable:
ValidateAzureResourceNamepermits parentheses, which are valid in resource-group names but are shell syntax when unquoted (for example,team(prod)fails in bash). Quote the resource group, cluster name, and subscription exactly as the real Azure CLI invocation does, and update the summary assertion.
new MarkdownString(
$"`az aks get-credentials --resource-group {resourceGroup} --name {clusterName} --subscription {subscriptionId}`"));
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
James Newton-King (JamesNK)
left a comment
James Newton-King (JamesNK)
left a comment
There was a problem hiding this comment.
Two credential-scope correctness issues remain before merge:
- The existing thread on
AzureBicepResource.Scopeprecedence is confirmed by a registered-pipeline-action probe: provisioning targets the configured scope, but credential acquisition still uses the annotation/global state. - An explicit deferred scope value that resolves null silently falls back to global deployment state here, while provisioning rejects it.
Focused proof at 89d876c8e3: restore succeeded; 10 targeted AKS tests passed; mutation probes failed as expected for both global-scope regression and ignored resource.Scope. A live cross-subscription AKS deploy was not run because it requires Azure credentials and billable infrastructure.
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review details
Suppressed comments (3)
src/Aspire.Hosting.Azure.Kubernetes/AzureKubernetesEnvironmentResource.AksPipeline.cs:302
- The reconnect command still is not copy-pasteable for every accepted resource-group name.
ValidateAzureResourceNameexplicitly permits parentheses, but an unquoted value such asteam(prod)is parsed as shell syntax in Bash/Zsh rather than as the--resource-groupargument. Quote the resource group, cluster name, and subscription in the summary command, and update the corresponding assertions.
new MarkdownString(
$"`az aks get-credentials --resource-group {resourceGroup} --name {clusterName} --subscription {subscriptionId}`"));
tests/Aspire.Hosting.Azure.Kubernetes.Tests/AzureKubernetesInfrastructureTests.cs:238
- This test exercises
GetAzureDeploymentContextAsync, which is now referenced only by tests; the registered credential step callsResolveDeploymentScopeAsyncinstead. Consequently, a regression in the live missing-subscription branch or its actionable message would leave this test green. InvokeResolveDeploymentScopeAsyncwith both scoped values null so the assertion covers the production path.
var exception = await Assert.ThrowsAsync<InvalidOperationException>(
() => AzureKubernetesEnvironmentResource.GetAzureDeploymentContextAsync(
services,
TestContext.Current.CancellationToken));
src/Aspire.Hosting.Azure/AzureBicepResourceScope.cs:102
- Changing
HasResourceGroupfrom internal to public adds a new public API onAzureBicepResourceScope, but the PR checklist says no public API was added. Either keep this member internal through an assembly-level implementation approach, or update the checklist and send the new API through the required API review.
public bool HasResourceGroup => _resourceGroup is not null;
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
/deployment-test |
|
🚀 Deployment tests starting on PR #19219... This will deploy to real Azure infrastructure. Results will be posted here when complete. |
Tests selector (audit mode)The full test matrix and all jobs still run in audit mode. The tests and jobs below are what selective CI would run under enforcement. 11 / 100 test projects · 3 jobs, from 4 changed files. Selected test projects (11 / 100)
Selected jobs (3)
How these were chosen — grouped by what changed
🔧 🔧 🧪 🧪 Job reasons
Selection computed for commit |
There was a problem hiding this comment.
Review details
Suppressed comments (1)
src/Aspire.Hosting.Azure.Kubernetes/AzureKubernetesEnvironmentResource.AksPipeline.cs:306
- The reconnect command is not copy-pasteable in Windows Command Prompt:
cmd.exedoes not treat single quotes as delimiters, so the apostrophes become part of the resource-group and cluster-name arguments andazfails to find them. Use double quotes for all three values (as the real command builder does); they work in Command Prompt, PowerShell, bash, and zsh, and update the two summary assertions accordingly.
new MarkdownString(
$"`az aks get-credentials --resource-group '{resourceGroup}' --name '{clusterName}' --subscription {subscriptionId}`"));
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
/backport to release/13.5 |
|
Started backporting to |
|
Pull request created: #1476
|
|
📝 Documentation has been drafted in microsoft/aspire.dev#1476 targeting Updated Note This draft PR needs human review before merging. |
|
The CI build failed due to test failure(s) that appear unrelated to the PR changes. These may be flaky tests. Suspected flaky test(s):
Suggested actions:
You can re-run the failed jobs from the workflow run page. |
…tion (#19271) * Pin AKS credential pipeline to the deployment subscription The AKS post-provisioning step invoked the Azure CLI without --subscription, so `az aks get-credentials` and the `az resource list` resource-group fallback both ran against whatever subscription the ambient `az` CLI defaulted to rather than the one Aspire selected for the deployment. The resource group lookup also read IConfiguration["Azure:ResourceGroup"], which is only populated at host startup. On a first deploy that value is empty, so the fallback query fired and silently resolved the cluster from the wrong subscription. Read the Azure subscription and resource group from IDeploymentStateManager instead. Provisioning writes them during the pipeline run, and SaveSectionAsync updates the in-memory state that AcquireSectionAsync reads back, so the values are current even on a first deploy. Both Azure CLI call sites now pass --subscription explicitly. The az command runner is injected into GetResourceGroupAsync and FetchKubeConfigAsync so tests can assert the exact command lines without spawning the CLI. This matters: asserting only on the argument-builder helpers would not have caught the original bug, since nothing would prove the pipeline actually calls them. Fixes #19216 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b15f47d-c522-4d08-a976-b3ee69c4ebb4 * Cover the get-credentials call site, not just its helpers Addresses review feedback on #19219: the existing tests all invoked GetResourceGroupAsync / FetchKubeConfigAsync directly, so nothing executed GetAksCredentialsAsync itself. Reverting the call site to the pre-fix unscoped `az` invocation left every test green -- the exact mutation that shipped as #19216 was undetectable one level up from where we asserted. Adds two internal test seams at the action boundary (az CLI path resolution and command execution) and a step-level test that resolves the registered aks-get-credentials-{name} step from the pipeline and runs its Action. It asserts the full command line of both `az` calls carry --subscription, plus the "Connect to cluster" summary hint, which was previously untested. Verified by mutation: restoring the unscoped call site now fails the new test while the six helper-level tests still pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b15f47d-c522-4d08-a976-b3ee69c4ebb4 * Honor per-resource scope when fetching AKS credentials A cluster adopted with AsExistingInResourceGroup(name, rg, subscription) can live outside the subscription and resource group Aspire deploys the rest of the app into. The provisioner already targets that per-resource scope, but the credential pipeline only ever read the global Azure deployment state, so the Azure CLI calls could authenticate against the wrong subscription -- or find a same-named cluster in the wrong place. Resolve the resource's ExistingAzureResourceAnnotation first and fall back to deployment state only for values it does not pin. When the resource pins a subscription that differs from the deployment subscription, the saved resource group is deliberately not inherited: it names a group inside the other subscription, so the step rediscovers it instead. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b15f47d-c522-4d08-a976-b3ee69c4ebb4 * Honor explicitly assigned scope and reject unresolvable scope values Two follow-ups on the cross-scope credential fix. ConfigureInfrastructure can assign AzureBicepResource.Scope directly, and the provisioner gives that precedence over ExistingAzureResourceAnnotation. The credential step only looked at the annotation, so a cluster placed in another subscription via Scope still had its credentials fetched from the app's own subscription. Mirror the provisioner's precedence: Scope first, annotation second. AzureBicepResourceScope.HasResourceGroup becomes public so callers can test the scope before reading ResourceGroup, which throws for subscription- and tenant-scoped resources. Resolving a scope value from an IValueProvider that yields null now throws, matching BicepProvisioner.ResolveScopeValueAsync. Previously it returned null and the caller silently substituted the app's deployment scope, so an unavailable scope parameter could fetch a same-named cluster from the wrong place instead of failing the way provisioning does. Empty is rejected too since it would otherwise be dropped by the downstream IsNullOrEmpty checks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b15f47d-c522-4d08-a976-b3ee69c4ebb4 * Reject empty literal scope values, not just empty provider results The provider path already threw on a null or empty result, but a literal empty string fell through to `string s => s`. Nothing upstream rejects it: AsExistingInResourceGroup and the AzureBicepResourceScope constructors only guard against null. Downstream `string.IsNullOrEmpty` checks then treated the value as unpinned and silently fell back to the global deployment scope, which is the same silent-wrong-scope failure the provider check was added to prevent. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b15f47d-c522-4d08-a976-b3ee69c4ebb4 * Address review feedback: dead code, shell-safe hint, stale docs, test hygiene Removes GetAzureDeploymentContextAsync, which had no production caller. Its two tests read as coverage for the credential step's subscription handling while exercising only the dead helper, so they now target ResolveDeploymentScopeAsync where the check actually runs. Quotes the resource group and cluster name in the "Connect to cluster" hint. ValidateAzureResourceName permits parentheses, so a group named team(prod) produced a hint that bash and zsh reject, despite the hint being advertised as copy-pasteable. The real invocation already quoted these. Refreshes GetResourceGroupAsync docs, which still described pre-PR behavior, to capture the cross-subscription rationale for why discovery now fires. Tests: disposes the ServiceProvider returned by CreateServicesWithAzureState at all 8 call sites, and replaces ?.TrySetResult() with an explicit Assert.NotNull plus unconditional call so a broken precondition fails immediately instead of degrading into a CI timeout. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b15f47d-c522-4d08-a976-b3ee69c4ebb4 * Fail on ambiguous AKS cluster names instead of picking the first The resource-group fallback query used `--query [0].resourceGroup`, taking whichever match Azure happened to return first. AKS cluster names are only unique within a resource group, not within a subscription, so a subscription can legitimately hold several clusters with the same name. Silently choosing one means fetching credentials for, and deploying into, an unrelated cluster. Query all matches and require exactly one, directing the user to AsExistingInResourceGroup when the name is ambiguous. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b15f47d-c522-4d08-a976-b3ee69c4ebb4 --------- Co-authored-by: Mitch Denny <midenn@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b15f47d-c522-4d08-a976-b3ee69c4ebb4
Description
When deploying an AKS environment, the post-provisioning step that fetches cluster credentials invoked the Azure CLI without
--subscription. Bothaz aks get-credentialsand theaz resource listresource-group fallback therefore ran against whatever subscription the ambientazCLI happened to default to, rather than the subscription Aspire selected for the deployment.This is easy to hit in practice: anyone whose
az account showdefault differs from the subscription they deploy to (multi-subscription tenants, CI agents, or after anaz account setelsewhere) would either get a confusing "cluster not found" failure or, worse, silently pull a kubeconfig for a same-named cluster in a different subscription and deploy into it.The resource-group lookup compounded the problem. It read
IConfiguration["Azure:ResourceGroup"], which is only populated at host startup. Provisioning writes the resource group into deployment state during the pipeline run, so on a first deploy that config value is empty — the fallbackaz resource listquery always fired, and it was the unscoped query.What changed for users
aspire deployagainst an AKS environment now always targets the subscription Aspire selected, regardless of the ambientazCLI default. No more wrong-subscription credential fetches or spurious "cluster not found" errors on first deploy.The deploy summary's reconnect command is now copy-pasteable on a machine with a different CLI default:
If the Azure subscription genuinely cannot be resolved from deployment state, the step now fails fast with an actionable message instead of silently falling back to the ambient CLI context:
In practice this should not surface during a normal deploy: the
aks-get-credentials-{name}step declaresDependsOnSteps = [AzureEnvironmentResource.ProvisionInfrastructureStepName], so provisioning (which persistsSubscriptionId) always completes first.Implementation
GetAzureDeploymentContextAsyncreadsSubscriptionId/ResourceGroupfrom theAzuredeployment-state section viaIDeploymentStateManagerrather thanIConfiguration. This is the key correctness point:SaveSectionAsyncupdates the same in-memory_statethatAcquireSectionAsyncreads back, so the values are current even on a first deploy when nothing has been written to disk-backed configuration yet. Both Azure CLI call sites then pass--subscriptionexplicitly, and the subscription ID goes through the existingValidateAzureResourceNamedefense-in-depth check before being embedded in a command line.The
azcommand runner is injected intoGetResourceGroupAsyncand the newFetchKubeConfigAsyncso tests can assert the exact command lines without spawning the CLI. This seam is load-bearing rather than cosmetic — see the testing note below.Testing
Unit tests in
tests/Aspire.Hosting.Azure.Kubernetes.Testscover:azat all.azfailure surfacing as anInvalidOperationException.An earlier revision of these tests asserted only the output of the argument-builder helpers. That was verified to be inadequate: reverting the
--subscriptionfix at the call site left the entire suite green, because nothing proved the pipeline actually called those helpers. The tests now drive the real call paths through an injected runner and assert the captured command lines, so a regression that drops--subscriptionfails the suite.Aspire.Hosting.Azure.Kubernetesbuilds with 0 warnings; 73/73 tests pass.Security considerations
This change affects how cluster credentials are acquired, so calling it out for reviewer awareness:
--subscriptionremoves an ambient-authority failure mode where a kubeconfig could be fetched from an unintended subscription that happens to contain a same-named cluster.azargument string. It is validated with the pre-existingValidateAzureResourceNameregex (^[a-zA-Z0-9\-_\.\(\)]+$) before use, matching how the cluster name and resource group were already handled.--file -and written by Aspire to a temp file with owner-only (0600) permissions on Unix; that behavior is untouched.Fixes #19216
Checklist
Is this feature complete?
Are you including unit tests for the changes and scenario tests if relevant?
Did you add public API?
<remarks />and<code />elements on your triple slash comments?AzureBicepResourceScope.HasResourceGroupchanges frominternaltopublic.AzureBicepResourceScopeis already a public sealed type andAzureBicepResource.Scopeis already public, butResourceGroupis a public property that throws for subscription- and tenant-scoped resources, with no public way to test the scope first. This closes that guard gap. Flagging for API review.Does the change make any security assumptions or guarantees?