Add AddHelmChart for installing external Helm charts#16589
Add AddHelmChart for installing external Helm charts#16589mitchdenny wants to merge 7 commits intomainfrom
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.
| 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)); | |
| } | |
| } |
| 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.
| /// <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.
| 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.
| 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); |
|
/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 2 jobs were identified as retry-safe transient failures in the CI run attempt.
Matched test failure patterns (1 test)
|
|
/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 2 jobs were identified as retry-safe transient failures in the CI run attempt.
Matched test failure patterns (1 test)
|
|
/deployment-test |
|
🚀 Deployment tests starting on PR #16589... This will deploy to real Azure infrastructure. Results will be posted here when complete. |
|
/deployment-test |
|
🚀 Deployment tests starting on PR #16589... This will deploy to real Azure infrastructure. Results will be posted here when complete. |
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>
|
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.
|
|
🎬 CLI E2E Test Recordings — 77 recordings uploaded (commit View all recordings
📹 Recordings uploaded automatically from CI run #25214852417 |
|
/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 failed — 29 passed, 5 failed, 0 cancelled View test results and recordings
|
Description
Adds
AddHelmChartinfrastructure for installing external Helm charts into a Kubernetes environment as pipeline steps.This enables installing pre-existing Helm charts (e.g., cert-manager, NGINX ingress controller, monitoring tools) alongside the Aspire-generated application Helm chart. Charts are installed via
helm upgrade --installafter the main application deploy.New types and APIs
KubernetesHelmChartResource— models an external chart (OCI/repo reference, version, namespace, release name, values)AddHelmChart(name, chartReference, chartVersion)— creates the resource and registers ahelm-install-{name}pipeline stepWithHelmValue(key, value)— sets--setvalues for the chartWithNamespace(namespace)/WithReleaseName(releaseName)— configure chart installationAddHelmChartonAzureKubernetesEnvironmentResourcethat delegates to the inner K8S environmentExample usage
Also included
Moves
KubernetesGatewayExtensionsandKubernetesIngressExtensionsto theAspire.Hostingnamespace (matching convention used by other hosting extension methods). This is also shipped as a standalone fix in #16588.Testing
Depends on #16588 (namespace fix, can merge independently)
Checklist
<remarks />and<code />elements on your triple slash comments?