Add AddHelmChart for installing external Helm charts - #16589
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 16589Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 16589" |
There was a problem hiding this comment.
Pull request overview
Adds infrastructure to model and deploy external Helm charts as post-deploy pipeline steps for Kubernetes (and AKS via delegation), plus aligns Kubernetes Ingress/Gateway extension-method namespaces with other hosting extensions.
Changes:
- Introduces
KubernetesHelmChartResourceandKubernetesHelmChartExtensions.AddHelmChart(...)withWithHelmValue/WithNamespace/WithReleaseNameconfiguration. - Registers a
helm-install-{name}deploy pipeline step that runs afterhelm-deploy-{environment}. - Moves
KubernetesIngressExtensions/KubernetesGatewayExtensionsinto theAspire.Hostingnamespace and adds an AKSAddHelmChartdelegating overload.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.Kubernetes.Tests/KubernetesHelmChartTests.cs | Adds unit coverage for the new Helm chart resource/builder behavior and pipeline annotation presence. |
| src/Aspire.Hosting.Kubernetes/KubernetesIngressExtensions.cs | Moves ingress extensions into Aspire.Hosting namespace (adds using Aspire.Hosting.Kubernetes). |
| src/Aspire.Hosting.Kubernetes/KubernetesGatewayExtensions.cs | Moves gateway extensions into Aspire.Hosting namespace (adds using Aspire.Hosting.Kubernetes). |
| src/Aspire.Hosting.Kubernetes/KubernetesHelmChartResource.cs | Adds the public resource type modeling an external Helm chart and its configuration. |
| src/Aspire.Hosting.Kubernetes/KubernetesHelmChartExtensions.cs | Adds the public fluent APIs and the deploy-time helm install pipeline step implementation. |
| src/Aspire.Hosting.Azure.Kubernetes/AzureKubernetesIngressExtensions.cs | Adds AKS AddHelmChart overload delegating to the inner Kubernetes environment. |
| public static IResourceBuilder<KubernetesHelmChartResource> WithNamespace( | ||
| this IResourceBuilder<KubernetesHelmChartResource> builder, | ||
| string @namespace) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(builder); | ||
| ArgumentException.ThrowIfNullOrEmpty(@namespace); | ||
|
|
||
| builder.Resource.Namespace = @namespace; |
There was a problem hiding this comment.
WithNamespace only checks for null/empty, but Kubernetes namespaces must be valid DNS labels (max length 63, lowercase alnum + '-' and must start/end alnum). Without validating here (and/or at deploy time), invalid namespaces can make the generated helm command fail or allow unexpected argument splitting. Consider validating with the same rules used by HelmChartOptions/HelmDeploymentEngine and throwing an ArgumentException with a helpful message.
There was a problem hiding this comment.
Addressed in 14cb05c — WithNamespace now calls HelmChartOptions.ValidateNamespace (the same RFC 1123 DNS-label rules, max 63 chars) so invalid namespaces throw ArgumentException immediately.
| ArgumentException.ThrowIfNullOrEmpty(releaseName); | ||
|
|
||
| builder.Resource.ReleaseName = releaseName; | ||
| return builder; | ||
| } | ||
|
|
There was a problem hiding this comment.
WithReleaseName only checks for null/empty, but Helm release names are constrained (DNS label, max length 53). Please validate releaseName (and ideally enforce lowercase) similarly to HelmChartOptions/HelmDeploymentEngine so consumers get an early, actionable error instead of a helm failure later.
| ArgumentException.ThrowIfNullOrEmpty(releaseName); | |
| builder.Resource.ReleaseName = releaseName; | |
| return builder; | |
| } | |
| ValidateHelmReleaseName(releaseName); | |
| builder.Resource.ReleaseName = releaseName; | |
| return builder; | |
| } | |
| private static void ValidateHelmReleaseName(string releaseName) | |
| { | |
| ArgumentException.ThrowIfNullOrEmpty(releaseName); | |
| if (releaseName.Length > 53) | |
| { | |
| throw new ArgumentException("Helm release name must be 53 characters or fewer.", nameof(releaseName)); | |
| } | |
| if (!char.IsAsciiLetterOrDigit(releaseName[0]) || !char.IsAsciiLetterOrDigit(releaseName[^1])) | |
| { | |
| throw new ArgumentException("Helm release name must start and end with a lowercase letter or digit.", nameof(releaseName)); | |
| } | |
| foreach (var c in releaseName) | |
| { | |
| if (char.IsAsciiLower(c) || char.IsDigit(c) || c == '-') | |
| { | |
| continue; | |
| } | |
| if (char.IsLetter(c) && char.IsUpper(c)) | |
| { | |
| throw new ArgumentException("Helm release name must be lowercase and may contain only lowercase letters, digits, and hyphens.", nameof(releaseName)); | |
| } | |
| throw new ArgumentException("Helm release name may contain only lowercase letters, digits, and hyphens.", nameof(releaseName)); | |
| } | |
| } |
There was a problem hiding this comment.
Addressed in 14cb05c — WithReleaseName now calls HelmChartOptions.ValidateReleaseName (DNS label, max 53 chars, lowercase-alnum start/end). I made the existing validators in HelmChartOptions internal and reused them rather than duplicating the rules.
| public static IResourceBuilder<KubernetesHelmChartResource> AddHelmChart( | ||
| this IResourceBuilder<KubernetesEnvironmentResource> builder, | ||
| [ResourceName] string name, | ||
| string chartReference, | ||
| string chartVersion) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(builder); | ||
| ArgumentException.ThrowIfNullOrEmpty(name); | ||
| ArgumentException.ThrowIfNullOrEmpty(chartReference); | ||
| ArgumentException.ThrowIfNullOrEmpty(chartVersion); | ||
|
|
||
| var environment = builder.Resource; | ||
| var resource = new KubernetesHelmChartResource(name, environment) | ||
| { | ||
| ChartReference = chartReference, | ||
| ChartVersion = chartVersion | ||
| }; |
There was a problem hiding this comment.
AddHelmChart currently accepts any non-empty chartVersion, but elsewhere (HelmChartOptions.WithChartVersion) the repo validates Helm chart versions as strict semantic versions. Consider validating chartVersion similarly here so invalid versions fail fast with a clear ArgumentException rather than surfacing as a helm CLI error during deployment.
There was a problem hiding this comment.
Addressed in 14cb05c — AddHelmChart now validates chartVersion via SemanticVersion.TryParse (the same check used by HelmChartOptions.WithChartVersion), so non-semver versions fail fast with ArgumentException instead of surfacing as a helm CLI error during deployment.
| /// <summary> | ||
| /// Gets or sets the Helm chart reference. This can be an OCI registry URL | ||
| /// (e.g., <c>oci://quay.io/jetstack/charts/cert-manager</c>) or a chart name | ||
| /// from an added repository. | ||
| /// </summary> | ||
| public string? ChartReference { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the chart version to install. | ||
| /// </summary> | ||
| public string? ChartVersion { get; set; } | ||
|
|
There was a problem hiding this comment.
ChartReference and ChartVersion are nullable/settable even though AddHelmChart requires them to be provided. This makes it easy to put the resource into an invalid state (e.g., ChartVersion set to null) and then get a late failure or different behavior at deploy time. Consider making these non-nullable and required (constructor/init-only), or enforce non-null/valid values consistently before running helm.
There was a problem hiding this comment.
Addressed in cd58a41 — KubernetesHelmChartResource now takes chartReference and chartVersion as constructor arguments and exposes them as non-nullable, read-only properties. They can no longer be put into an invalid state after construction.
| var releaseName = chart.ReleaseName ?? chart.Name; | ||
| var @namespace = chart.Namespace ?? chart.Name; | ||
| var chartRef = chart.ChartReference ?? throw new InvalidOperationException($"Helm chart '{chart.Name}' has no chart reference configured."); | ||
| var chartVersion = chart.ChartVersion; | ||
|
|
||
| logger.LogInformation( | ||
| "Installing Helm chart '{ChartName}' ({ChartRef}:{ChartVersion}) into namespace '{Namespace}'.", | ||
| chart.Name, chartRef, chartVersion, @namespace); | ||
|
|
||
| var arguments = new StringBuilder(); | ||
| arguments.Append(CultureInfo.InvariantCulture, $"upgrade --install {releaseName} \"{chartRef}\""); | ||
| arguments.Append(CultureInfo.InvariantCulture, $" --namespace {@namespace}"); | ||
| arguments.Append(" --create-namespace"); | ||
| arguments.Append(" --wait"); | ||
|
|
||
| if (!string.IsNullOrEmpty(chartVersion)) | ||
| { | ||
| arguments.Append(CultureInfo.InvariantCulture, $" --version {chartVersion}"); | ||
| } |
There was a problem hiding this comment.
InstallHelmChartAsync treats ChartVersion as optional (it may be null/empty and then no --version is passed), but AddHelmChart requires a version. This can lead to silently installing 'latest' if ChartVersion is cleared via direct mutation. Either make chartVersion truly optional in the public API/docs, or throw if ChartVersion is missing at deploy time to preserve the API contract.
There was a problem hiding this comment.
Addressed in cd58a41 — since ChartVersion is now non-nullable and read-only, it can't be cleared after construction; InstallHelmChartAsync always passes --version.
| string value) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(builder); | ||
| ArgumentException.ThrowIfNullOrEmpty(key); |
There was a problem hiding this comment.
WithHelmValue validates the key but not the value. At runtime a null value can be stored in the dictionary and will produce a "key=" assignment in the generated helm arguments, which is hard to diagnose and may not match caller intent. Consider throwing for null values (and possibly validating/escaping quotes/newlines) to keep the generated command line well-formed.
| ArgumentException.ThrowIfNullOrEmpty(key); | |
| ArgumentException.ThrowIfNullOrEmpty(key); | |
| ArgumentNullException.ThrowIfNull(value); |
There was a problem hiding this comment.
Addressed in 14cb05c — WithHelmValue now rejects null values and validates both key and value. Keys must match ^[A-Za-z0-9_.\-\[\]]+$ (alphanumerics, ., -, _, plus [] for indexed paths like args[0]). Values reject ", \, and control characters so the generated --set key="value" argument is always well-formed.
|
/deployment-test |
|
🚀 Deployment tests starting on PR #16589... This will deploy to real Azure infrastructure. Results will be posted here when complete. |
|
Re-running the failed jobs in the CI workflow for this pull request because 1 job was identified as retry-safe transient failures in the CI run attempt.
|
|
/deployment-test |
|
🚀 Deployment tests starting on PR #16589... This will deploy to real Azure infrastructure. Results will be posted here when complete. |
James Newton-King (JamesNK)
left a comment
James Newton-King (JamesNK)
left a comment
There was a problem hiding this comment.
Code Review Summary
Found 1 correctness issue not covered by existing review comments:
- Unescaped chart reference in helm arguments (line 173): The
chartRefparameter is interpolated directly into a raw helm command string. Special characters could cause argument injection or parsing errors. Recommend early validation to reject or escape problematic characters.
| chart.Name, chartRef, chartVersion, @namespace); | ||
|
|
||
| var arguments = new StringBuilder(); | ||
| arguments.Append(CultureInfo.InvariantCulture, $"upgrade --install {releaseName} \"{chartRef}\""); |
There was a problem hiding this comment.
The chartRef value is interpolated directly into a raw helm argument string without escaping. If a user supplies a chart reference containing special characters (e.g., quotes, spaces, or shell metacharacters), they could unintentionally break out of the intended token and inject additional helm flags, changing deployment behavior or causing failures. Since the runner executes a single raw arguments string (not structured argv), validation is needed.
Recommendation: Validate chartRef format early (whitelist allowed characters like alphanumerics, hyphens, slashes, colons, dots for OCI/repo syntax), or reject characters like quotes and newlines with a clear error message so users get fast feedback at definition time rather than a helm CLI error at deploy time.
There was a problem hiding this comment.
Addressed in 14cb05c — chartReference is now validated up-front against an allowlist regex (^[A-Za-z0-9_./:@+~\-]+$). It permits OCI URLs (oci://...), HTTP(S) URLs, local paths, packaged .tgz filenames, and repo/chart references, but rejects whitespace, quotes, and shell metacharacters that could break helm's argument tokenization. Coverage in KubernetesHelmChartTests.cs exercises the rejected forms.
Adds KubernetesHelmChartResource and extension methods for installing external Helm charts into a Kubernetes environment as pipeline steps. - KubernetesHelmChartResource: models an external chart (OCI/repo ref, version, namespace, release name, values) - AddHelmChart(): creates the resource and registers a helm-install pipeline step that runs after the main app Helm deploy - WithHelmValue(): sets --set values for the chart - WithNamespace()/WithReleaseName(): configure chart installation The pipeline step uses IHelmRunner (existing abstraction) to execute helm upgrade --install with the configured values. This is the foundation for AddCertManager and other Helm-based integrations in subsequent PRs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move KubernetesGatewayExtensions, KubernetesIngressExtensions, and KubernetesHelmChartExtensions to the Aspire.Hosting namespace to match the convention used by other hosting extension methods. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The chart reference (OCI URL) and --set values need quoting to prevent shell interpretation issues with special characters like # in values. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds AddHelmChart extension on AzureKubernetesEnvironmentResource that delegates to the inner KubernetesEnvironmentResource, matching the existing pattern for AddIngress and AddGateway. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tests the full aspire deploy flow with an external Helm chart (podinfo) on a local KinD cluster with a local Docker registry. Verifies: - aspire deploy installs both the app and the external chart - podinfo is deployed with 2 replicas as configured via WithHelmValue - Helm release exists in the podinfo namespace Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- WithHelmValue: validate value is not null (prevents --set key= args) - InstallHelmChartAsync: throw if ChartVersion is null at deploy time (enforces the contract established by AddHelmChart requiring version) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tests the full flow on a real AKS cluster: - Creates AKS cluster + ACR via az CLI - Scaffolds project with AddHelmChart for podinfo (2 replicas) - Deploys with aspire deploy - Verifies podinfo has 2 ready replicas - Verifies podinfo serves HTTP 200 via port-forward - Verifies Helm release exists Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ChartReference and ChartVersion previously were nullable settable properties, which meant the resource's identity could drift from what the install pipeline step described. Switch them to constructor-set read-only properties, and add a DestroyOnUninstall opt-in flag (set via the new WithDestroy() extension) so callers can request destroy-time uninstall without it becoming the default for shared charts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Validate every interpolated token before it's spliced into the helm
argument string. chartReference must match an OCI/HTTP/path
whitelist; chartVersion goes through HelmChartOptions.ValidateChartVersion;
WithNamespace and WithReleaseName reuse the existing DNS-label
validators (which are now internal); WithHelmValue rejects keys
containing whitespace/quotes and values containing quotes,
backslashes, or control characters, and the install code wraps every
--set value in double quotes so the OS process call sees one token
per --set.
* Switch the install step to chartBuilder.WithAnnotation(...)
(matches the pattern in JavaScriptHostingExtensions and
ContainerResourceBuilderExtensions).
* Add WithDestroy() opt-in. When set, an extra helm-uninstall-{name}
pipeline step is registered to run during 'aspire destroy' and the
install step persists release name + namespace into the deployment
state manager so the uninstall step can find the release later.
* Add <remarks> docs to the AKS AddHelmChart overload pointing at the
shared WithDestroy/WithHelmValue/WithNamespace/WithReleaseName
extensions.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Extend KubernetesHelmChartTests with: null-arg coverage, malicious chart-reference rejection, valid/invalid chart version cases, DNS label rejection for namespace/release name (including length), WithHelmValue key/value rejection, WithDestroy default+opt-in behavior, and end-to-end pipeline-step factory checks for both the default (install only) and WithDestroy (install + uninstall) shapes. * Add AzureKubernetesHelmChartTests covering the AKS-side overload: parent type, basic properties, WithHelmValue, WithNamespace, WithReleaseName, WithDestroy, null/empty arg rejection, version validation, and chart-reference validation. * Add AksWithHelmChartDeploymentTests, an AKS E2E test that mirrors AksWithAzureResourcesDeploymentTests but uses AddAzureKubernetesEnvironment + aks.AddHelmChart(podinfo) + WithDestroy(). Verifies podinfo pods come up with the configured replica count, that podinfo serves traffic, and that aspire destroy uninstalls the chart (helm list -n podinfo is empty afterward). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Per repo terminology preference; no functional change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/deployment-test |
|
🚀 Deployment tests starting on PR #16589... This will deploy to real Azure infrastructure. Results will be posted here when complete. |
AddAzureKubernetesEnvironment implicitly adds a default workload pool with Standard_D2s_v5 (DSv5 family). The previous test only overrode the system pool and routinely hit DSv5 vCPU quota in westus3. Mirror AksWithAzureResourcesDeploymentTests by also pinning the workload pool to Standard_D2as_v5 (DASv5 family). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/deployment-test |
|
🚀 Deployment tests starting on PR #16589... This will deploy to real Azure infrastructure. Results will be posted here when complete. |
|
✅ Deployment E2E Tests passed — 35 passed, 0 failed, 0 cancelled View test results and recordings
|
James Newton-King (JamesNK)
left a comment
James Newton-King (JamesNK)
left a comment
There was a problem hiding this comment.
Reviewed the new AddHelmChart infrastructure. Found 2 issues:\n\n- 1 correctness issue: Default namespace/release name fallback bypasses DNS-label validation that the explicit WithNamespace/WithReleaseName paths enforce.\n- 1 resource leak: Process not disposed in test cleanup method.
| } | ||
|
|
||
| await deploymentStateManager.DeleteSectionAsync(stateSection, context.CancellationToken).ConfigureAwait(false); | ||
|
|
There was a problem hiding this comment.
Bug: Default namespace/release name fallback is not DNS-label-validated
ResolveReleaseAndNamespace falls back to chart.Name when no explicit namespace or release name is set. This value is then interpolated unquoted into the helm command on lines 222 and 302 without any DNS label validation.
By contrast, the existing HelmDeploymentEngine.ResolveReleaseNameAsync (line 63) explicitly calls ValidateHelmReleaseName on the resolved value, and ResolveNamespaceAsync calls ValidateKubernetesNamespace.
If a resource name contains uppercase characters or exceeds 53/63 chars, the helm command will fail at deploy time with an opaque helm error instead of a clear ArgumentException at configuration time.
Suggestion: Validate the resolved release name and namespace here (or at the call sites) using the same HelmChartOptions.ValidateReleaseName/ValidateNamespace helpers already used by WithReleaseName/WithNamespace:
private static (string ReleaseName, string Namespace) ResolveReleaseAndNamespace(KubernetesHelmChartResource chart)
{
var releaseName = chart.ReleaseName ?? chart.Name;
var @namespace = chart.Namespace ?? chart.Name;
HelmChartOptions.ValidateReleaseName(releaseName, nameof(releaseName));
HelmChartOptions.ValidateNamespace(@namespace, nameof(@namespace));
return (releaseName, @namespace);
}Alternatively, validate at AddHelmChart time when the name is first provided, since that's the value that will be used as the default.
There was a problem hiding this comment.
Done in 04fc1cc — added validation inside ResolveReleaseAndNamespace and hoisted the call into the PipelineStepAnnotation factory so it runs at step-creation time. Now the user gets a clear InvalidOperationException (with a WithReleaseName/WithNamespace hint) before helm ever runs. Added PipelineStepFactory_RejectsResourceNameThatIsNotValidDnsLabel covering uppercase and over-length resource names, plus PipelineStepFactory_AcceptsInvalidResourceNameWhenOverridesProvided to confirm explicit overrides skip the fallback.
| finally | ||
| { | ||
| output.WriteLine($"Cleaning up resource group: {resourceGroupName}"); | ||
| await CleanupResourceGroupAsync(resourceGroupName); |
There was a problem hiding this comment.
Resource leak: Process not disposed
CleanupResourceGroupAsync creates a System.Diagnostics.Process and awaits WaitForExitAsync() but never disposes the process. Compare with the equivalent method in AksWithHelmChartDeploymentTests.TriggerCleanupResourceGroup which correctly uses using var process.
| await CleanupResourceGroupAsync(resourceGroupName); | |
| using var process = new System.Diagnostics.Process |
James Newton-King (JamesNK)
left a comment
James Newton-King (JamesNK)
left a comment
There was a problem hiding this comment.
Test coverage suggestions: 2 missing tests for chart reference allowlist positive coverage and uninstall step description fallback behavior.
|
|
||
| // Allowlist for Helm chart references. Covers OCI URLs (oci://host/path), HTTP/HTTPS URLs, | ||
| // local paths, plain chart names ("repo/chart"), and packaged chart filenames. Rejects anything | ||
| // that could break helm argument tokenization (whitespace, quotes, control chars). | ||
| [GeneratedRegex(@"^[A-Za-z0-9_./:@+~\-]+$")] | ||
| private static partial Regex ChartReferencePattern(); |
There was a problem hiding this comment.
Missing positive test coverage for chart reference allowlist
The existing tests (AddHelmChart_RejectsMaliciousChartReference) only verify rejection of invalid chart references. There are no positive tests confirming the regex accepts the real-world formats documented in the comment above (OCI URLs, HTTP/HTTPS URLs, local paths, repo/chart names, .tgz filenames, + and ~ characters).
Suggested test:
[Theory]
[InlineData("oci://quay.io/jetstack/charts/cert-manager")] // OCI URL
[InlineData("oci://ghcr.io/stefanprodan/charts/podinfo")] // OCI URL with ghcr.io
[InlineData("https://charts.example.com/repo/chart")] // HTTPS URL
[InlineData("http://charts.example.com/repo/chart")] // HTTP URL
[InlineData("myrepo/mychart")] // repo/chart
[InlineData("mychart-1.0.0.tgz")] // packaged chart filename
[InlineData("./local-chart")] // local path
[InlineData("chart+extra~tag")] // plus and tilde chars
[InlineData("oci://registry:5000/charts/app")] // registry with port
[InlineData("oci://user@registry.io/charts/app")] // registry with @
public void AddHelmChart_AcceptsValidChartReferences(string chartReference)
{
var builder = TestDistributedApplicationBuilder.Create(DistributedApplicationOperation.Publish);
var k8s = builder.AddKubernetesEnvironment("env");
var chart = k8s.AddHelmChart("test", chartReference, "1.0.0");
Assert.Equal(chartReference, chart.Resource.ChartReference);
}This protects against accidental narrowing of the regex in future refactors.
There was a problem hiding this comment.
Done in 04fc1cc — added AddHelmChart_AcceptsValidChartReferences [Theory] covering all 10 cases you suggested (OCI URLs, HTTPS/HTTP URLs, repo/chart, packaged .tgz, relative path, +~ chars, registry with port, and registry with @ user).
| var uninstallStep = new PipelineStep | ||
| { | ||
| Name = $"helm-uninstall-{name}", | ||
| Description = $"Uninstalls Helm chart '{name}' from namespace '{resource.Namespace ?? name}'", |
There was a problem hiding this comment.
Missing test: uninstall step description uses resource name as namespace fallback
When Namespace is null, this description falls back to name. This fallback behavior is untested — the existing PipelineStepFactory_WithDestroy_ProducesInstallAndUninstallSteps test only checks step names and dependency wiring, not the description text.
Suggested test:
[Fact]
public async Task PipelineStepFactory_UninstallStepDescription_ShowsNamespaceDefault()
{
var builder = TestDistributedApplicationBuilder.Create(DistributedApplicationOperation.Publish);
var k8s = builder.AddKubernetesEnvironment("env");
var chart = k8s.AddHelmChart("my-release", "oci://example.com/chart", "1.0.0")
.WithDestroy();
// Namespace is not set, so the description should use the resource name as the namespace.
Assert.Null(chart.Resource.Namespace);
var steps = await CreateStepsAsync(builder, chart.Resource);
var uninstallStep = Assert.Single(steps, s => s.Name == "helm-uninstall-my-release");
Assert.Contains("my-release", uninstallStep.Description); // fallback namespace = name
}There was a problem hiding this comment.
Done in 04fc1cc — added PipelineStepFactory_UninstallStepDescription_FallsBackToResourceNameForNamespace (asserts the description contains the resource name when Namespace is null) and PipelineStepFactory_UninstallStepDescription_UsesExplicitNamespace (asserts it uses the explicit value when WithNamespace is called).
…d coverage - ResolveReleaseAndNamespace now validates the resource-name fallback for both release name and namespace, and is invoked at step-creation time so a clear error fires before helm runs. - Dispose Process in KubernetesHelmChartDeploymentTests.CleanupResourceGroupAsync. - Added [Theory] of valid chart references that the allowlist must accept. - Added [Fact]s for uninstall step description fallback (and explicit namespace) plus tests proving the new validation rejects uppercase / over-length names that would otherwise fail in helm. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🎬 CLI E2E Test Recordings — 78 recordings uploaded (commit View all recordings
📹 Recordings uploaded automatically from CI run #25647795567 |
|
Pull request created: #905
|
|
📝 Documentation has been drafted in microsoft/aspire.dev#905 targeting Drafted documentation for the new Files created/modified:
Note This draft PR needs human review before merging. |
Description
Adds
AddHelmChartinfrastructure for installing external Helm charts into a Kubernetes environment asaspire deploypipeline steps. This is in addition to (and runs after) the application's own Helm chart that Aspire generates and deploys.This enables installing pre-existing Helm charts (e.g., cert-manager, NGINX ingress controller, podinfo, monitoring tools) alongside the Aspire-generated application chart. Charts are installed with
helm upgrade --installafter the main application deploy step, and — when opted in — uninstalled withhelm uninstallduringaspire destroy.New types and APIs
KubernetesHelmChartResource— models an external chart.ChartReferenceandChartVersionare non-nullable, get-only constructor arguments (the resource cannot be put into an invalid state).Namespace,ReleaseName, andValuesremain mutable via the builder.AddHelmChart(name, chartReference, chartVersion)— extension onIResourceBuilder<KubernetesEnvironmentResource>. Validates inputs eagerly, creates the resource, and registers ahelm-install-{name}pipeline step that runs after the environment''s ownhelm-deploy-{environment}step.WithHelmValue(key, value)— sets--set key=value. Both the key (allowlist regex^[A-Za-z0-9_.\-\[\]]+$, supports indexed paths likeargs[0]) and value (rejects",\, and control characters) are validated to keep the generatedhelmargument string well-formed.WithNamespace(namespace)— validated against the same RFC 1123 DNS-label rules used byHelmChartOptions(max 63 chars).WithReleaseName(releaseName)— validated against Helm''s release-name rules (DNS label, max 53 chars).WithDestroy()— opt-in: registers a second pipeline step that runshelm uninstallduringaspire destroy. Off by default so unrelated workloads installed in the cluster (e.g., cert-manager) aren''t accidentally torn down.AddHelmChartonAzureKubernetesEnvironmentResourcethat delegates to the inner Kubernetes environment.Input validation / hardening
chartReferenceis validated against an allowlist regex (^[A-Za-z0-9_./:@+~\-]+$). Permits OCI URLs, HTTP(S) URLs, local paths, packaged.tgzfilenames, andrepo/chartreferences; rejects whitespace, quotes, and shell metacharacters that could break helm''s argument tokenization.chartVersionis parsed withSemanticVersion.TryParse(matchesHelmChartOptions.WithChartVersion).WithHelmValuerejectsnullvalues and validates both key and value as above.Example usage
Also included
Moves
KubernetesGatewayExtensionsandKubernetesIngressExtensionsto theAspire.Hostingnamespace (matching the convention used by other hosting extension methods). Also shipped as #16588.Testing
KubernetesHelmChartTests.cscovering: argument validation (null/empty, malicious chart references, invalid namespaces/release names, invalid value keys),WithDestroydefault + opt-in + chaining, and end-to-end pipeline-step factory shape (1 step withoutWithDestroy, 2 steps with).AzureKubernetesHelmChartTests.cscovering the AKS overload (parent type, basic props, allWith*builders, null-arg rejection, version validation, chart-ref validation).KubernetesHelmChartDeploymentTests) — installs podinfo into a KinD cluster.AksWithHelmChartDeploymentTests) — provisions a real AKS cluster viaAddAzureKubernetesEnvironment, installs podinfo, verifiesreplicaCount=2was applied, port-forwards and curls the chart''s Service for HTTP 200, then runsaspire destroyand assertshelm list -n podinfois empty (provesWithDestroy()actually uninstalled the chart).✅ All 194 K8S tests + 53 Azure K8S tests pass locally. Latest deployment run: 35/35 passed including
AksWithHelmChartDeploymentTests.Checklist
<remarks />and<code />elements on your triple slash comments?