Fix WithIngressClass parameter resolution for Kubernetes publish#17423
Merged
Conversation
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 17423Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 17423" |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes Kubernetes publish output when ingress/gateway properties (e.g., ingressClassName, hostname, TLS secret) are provided via ParameterResource but are unresolved at publish time. Instead of emitting the literal "{0}" format string into generated YAML, the publish pipeline now renders Helm {{ .Values... }} template references and ensures corresponding placeholders exist in values.yaml, keeping published charts usable for non-Aspire Helm consumers.
Changes:
- Make ingress/gateway manifest expression resolution parameter-aware: unresolved
ParameterResources become Helm template references and are captured for deploy-time override generation. - Add a deploy-time-only resolver helper to preserve existing post-deploy pipeline behavior for steps that expect parameters to be resolved.
- Add regression tests covering both missing-parameter (Helm reference +
values.yamlplaceholder) and publish-default (inline value) behaviors.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.Kubernetes.Tests/KubernetesIngressTests.cs | Adds regression coverage for WithIngressClass(ParameterResource) with/without publish-time defaults, including values.yaml placeholder validation. |
| src/Aspire.Hosting.Kubernetes/KubernetesPublishingContext.cs | Ensures captured Helm values from ingress/gateway resources also get placeholders in values.yaml. |
| src/Aspire.Hosting.Kubernetes/KubernetesEnvironmentResource.cs | Introduces publish-time Helm-substituting resolver for ingress/gateway expressions and keeps deploy-time pipeline resolution behavior via a separate helper. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 0
Member
Author
|
/deployment-test |
Contributor
|
🚀 Deployment tests starting on PR #17423... This will deploy to real Azure infrastructure. Results will be posted here when complete. |
Member
Author
|
/cc @robrich |
JamesNK
approved these changes
May 25, 2026
Member
JamesNK
left a comment
JamesNK
left a comment
There was a problem hiding this comment.
LGTM — clean fix with good comments and test coverage. 1 low-severity observation posted (multi-parameter expression edge case).
Comment on lines
+585
to
+598
| { | ||
| if (valueProvider is ParameterResource parameter) | ||
| { | ||
| // Capture the parameter so HelmDeploymentEngine writes its resolved value to | ||
| // the deploy-time override file, and ensure values.yaml has a placeholder so | ||
| // `helm template` (and `aspire publish` consumers that don't deploy via Aspire) | ||
| // can render the chart without `<no value>` substitutions. | ||
| var section = parameter.Secret ? HelmExtensions.SecretsKey : HelmExtensions.ParametersKey; | ||
| var valueKey = parameter.Name.ToHelmValuesSectionName(); | ||
|
|
||
| if (!CapturedHelmValues.Any(c => | ||
| c.Section == section && | ||
| c.ResourceKey == owningResourceKey && | ||
| c.ValueKey == valueKey)) |
Member
There was a problem hiding this comment.
Low: When MissingParameterValueException is caught on the whole expression (because at least one parameter is unresolved), this handler generates Helm template references for every ParameterResource in the expression — including those that have default values and could have been inlined.
For example, if an expression combines paramWithDefault (has publishValueAsDefault: true) and paramWithoutDefault, both get Helm references even though the first could be resolved to its literal value here.
Impact: Functionally correct — HelmDeploymentEngine resolves all captured values at deploy time. But the published chart contains an unnecessary placeholder for parameters with known defaults, making the chart slightly less self-contained.
The fix would be to attempt GetValueAsync on each individual ParameterResource first and only fall back to a Helm reference for those that throw. However, I suspect multi-parameter expressions for ingress/gateway APIs are extremely rare in practice, so this may not be worth the added complexity.
…Path - Resolve each ParameterResource individually when the outer expression raises MissingParameterValueException, so parameters with publishValueAsDefault still get inlined in mixed-parameter expressions (per JamesNK review feedback). - Update new ingress tests to use WithPath after WithRoute was renamed on KubernetesIngressResource in #17424. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
Thanks @JamesNK — good catch. Addressed in edf3bbb: Also picked up the rename of |
Member
Author
|
/deployment-test |
Contributor
|
🚀 Deployment tests starting on PR #17423... This will deploy to real Azure infrastructure. Results will be posted here when complete. |
Contributor
|
❓ CLI E2E Tests unknown — 96 passed, 0 failed, 5 unknown (commit View all recordings
📹 Recordings uploaded automatically from CI run #26377291906 |
Contributor
|
❌ Deployment E2E Tests failed — 38 passed, 2 failed, 0 cancelled View test results and recordings
|
Contributor
|
✅ No documentation update needed. docs_optional → bug_fix_restores_documented_behavior No signals triggered (signal_count = 0). This PR fixes a bug in
The bug was that the implementation diverged from documented behavior; this fix closes the gap. No new user-facing surface, options, or APIs were introduced. |
This file contains hidden or 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
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
When an ingress or gateway resource referenced a
ParameterResourceviaWithIngressClass,WithHostname,WithTls, etc., the generated Kubernetes YAML rendered the literal format string"{0}"for unresolved parameters instead of a Helm template expression. This made the published Helm chart unusable unless the parameter happened to have a value at publish time. Reproduced from a bug shown on AspireFridays by @robrich:produced:
Why the bug existed
KubernetesEnvironmentResource.ResolveExpressionAsyncwas the single helper used to resolve everyReferenceExpressionconsumed by the Kubernetes publisher (ingress class, hostname, TLS, gateway listeners, etc.). It calledReferenceExpression.GetValueAsyncand, onMissingParameterValueException, returnedexpression.Formatverbatim as a "fallback":expression.Formatis the raw composite format string (e.g."{0}"for one parameter,"{0}-{1}"for two). It only resembles a useful value for expressions that contain no placeholders, which is essentially never the case for parameter-driven values. The docstring claimed the helper returned the parameter name, but the implementation never did — it just emitted the format string and let it land in the published YAML.This was harmless for the original callers (TLS bootstrap and gateway address discovery in pipeline steps) because those run at deploy time, where parameters already have values and the catch block is never hit. The bug only surfaced once the same helper was reused for publish-time manifest generation, where missing values are the normal case.
How the fix works
ResolveExpressionAsyncis now parameter-aware and runs at publish time. When the expression contains unresolvedParameterResources, the fix:IValueProviderin the expression individually instead of returning the raw format string.ParameterResource, substitutes a Helm template reference of the form{{ .Values.parameters.<owningResource>.<paramName> }}(orsecrets.for secret parameters), scoped to the owning ingress/gateway resource so multiple resources can reuse the same parameter name without colliding.CapturedHelmValuesso two downstream consumers get the right data:HelmDeploymentEnginewrites the resolved parameter value into the deploy-time override file Aspire passes tohelm install/helm upgrade.values.yamlgets a matching placeholder entry via the newEnsureCapturedHelmValuePlaceholderspass inKubernetesPublishingContext, sohelm templateagainst the published chart does not render<no value>for users who deploy outside of Aspire.string.Formatwith the substituted segments, preserving any literal text from the original expression (e.g. an expression like"prefix-{0}-suffix"becomes"prefix-{{ .Values.parameters.ingress.foo }}-suffix").Pipeline-time callers (TLS bootstrap, gateway address discovery) keep their original behavior through a renamed static helper
ResolveExpressionAtDeployTimeAsync. Those paths run after parameters are guaranteed to be resolved, so a missing-value fallback there is acceptable (and, again, never hit in practice).Testing notes
Added two regression tests in
KubernetesIngressTests.cscovering the parameter and missing-parameter cases. The missing-parameter test needsPipeline:ClearCache=trueplus a per-run unique parameter name because the deployment state file at~/.aspire/deployments/<sha>/<env>.jsonpersists across local test runs and would otherwise auto-resolve the parameter from a previous run, masking the failure.Fixes # (issue)
Checklist
<remarks />and<code />elements on your triple slash comments?