Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Pull request overview

Copilot reviewed 37 out of 38 changed files in this pull request and generated 13 comments.

Pull request overview

Copilot reviewed 37 out of 38 changed files in this pull request and generated 15 comments.

            resourceBuilder.WithContainerFiles("/k8s-manifests", async (ctx, ct) =>
            {
                if (Directory.Exists(absolutePath))

            .WithContainerFiles("/helm-values", async (ctx, ct) =>
                release.ValuesFiles
                    .Select((hostPath, i) => (ContainerFileSystemItem)new ContainerFile

            var selector = string.Join(",",
                (svc.Spec.Selector ?? new Dictionary<string, string>()).Select(kv => $"{kv.Key}={kv.Value}"));

            var pods = await k8sClient.CoreV1
                .ListNamespacedPodAsync(@namespace, labelSelector: selector, cancellationToken: ct)

Pull request overview

Copilot reviewed 37 out of 38 changed files in this pull request and generated 7 comments.

Pull request overview

Copilot reviewed 37 out of 38 changed files in this pull request and generated 5 comments.

Thorough code review

13 findings total: 1 blocking, 7 major, 5 minor/medium. Overall verdict: request changes. The design is sound — the execution needs a careful pass on lifecycle, idempotency, and atomicity.

🚨 Blocking

1. Unresolved Git merge conflict markers in .gitignore (lines 26–38)

Literal <<<<<<< main, =======, >>>>>>> main markers were committed. git grep "<<<<<<<" confirms .gitignore:26. The conflict was resolved locally but the markers were never deleted. This alone should keep the PR out of main.

🔴 Major

2. K3sReadinessHealthCheck._cachedClient is dead state — Kubernetes client leaks every tick

K3sBuilderExtensions.cs:211-215 registers the check with sp => new K3sReadinessHealthCheck(resource, resource.ApiEndpoint). DefaultHealthCheckService.RunCheckAsync invokes the factory on every check, so _cachedClient always starts null and the cache is dead code. Every tick allocates a fresh Kubernetes/HttpClient (line 116) which is never disposed (the class is not IDisposable). The "stale credentials" recovery in the catch block (lines 70–84) can only ever dispose the just-created instance, never a stale one across calls — it's a no-op pattern.

Fix: register as a singleton (var hc = new K3sReadinessHealthCheck(...); sp => hc) and make the class IDisposable, or drop the cache and using the client locally within each CheckHealthAsync call.

3. Non-atomic kubeconfig overwrite on every health check

K3sReadinessHealthCheck.cs:99-113. Because of #2, EnsureClientAsync runs every tick and File.WriteAllTextAsync truncates both local/kubeconfig.yaml and container/kubeconfig.yaml in place. Readers hitting the race window can observe a partial/empty file — host projects with KUBECONFIG=…/local/kubeconfig.yaml, the helm/kubectl scripts polling for the file, and any guest BuildConfigFromConfigFile(...) at startup that does not retry.

Fix: write to a temp file in the same directory then File.Move(tempPath, finalPath, overwrite: true); only rewrite when the source raw YAML's hash has changed since last write.

4. AllocatePort() TOCTOU race + interface mismatch

K3sBuilderExtensions.ServiceEndpoint.cs:200-207. Two distinct defects:

1. Window race: the probe listener is stopped before K3sInProcessPortForwarder rebinds on the same port. Any other process can claim the port in the gap, causing the subsequent new TcpListener(IPAddress.Any, localPort).Start() to throw SocketException(AddressAlreadyInUse). The exception is caught by the outer catch (Exception ex) (lines 79–85), logged, and the loop backs off and retries with the same already-stolen port — it never picks a fresh one. The endpoint appears in the dashboard as perpetually unhealthy with no obvious cause.
2. Interface mismatch: the probe binds on IPAddress.Loopback, but the forwarder binds on IPAddress.Any (line 43 of K3sInProcessPortForwarder.cs). A port free on 127.0.0.1 can still conflict on 0.0.0.0 (another service bound on a non-loopback interface) — same failure mode.

Fix: probe on IPAddress.Any (match the actual bind). Better: pass the live listener instance into K3sInProcessPortForwarder and keep it bound through the handoff so there is no release-rebind window. On retry inside the forwarder loop, allocate a fresh port rather than reusing the same one.

5. WithDataVolume is not idempotent — duplicate volume mounts

K3sBuilderExtensions.cs:327-335. Each call appends a fresh volume annotation targeting /var/lib/rancher/k3s. Two calls (a common error when composing builder extensions) produce two ContainerMountAnnotations at the same target → Docker rejects with Duplicate mount point: /var/lib/rancher/k3s. The unit test WithDataVolumeAddsSingleVolumeMount only exercises a single call; there is no test asserting idempotency.

Fix: remove the previous data-volume annotation before adding, or use ResourceAnnotationMutationBehavior.Replace semantics consistent with WithLifetime (line 410-412).

6. WithReference(cluster) adds a duplicate bind-mount on repeat invocation

K3sBuilderExtensions.cs:368-388. Same defect as #5: the container branch unconditionally appends a new ContainerMountAnnotation for /var/k3s. Two calls — easy when composing helper extension methods — create two bind-mounts to the same target, which Docker rejects at start with the duplicate-mount error buried in DCP logs.

Fix: check destination.Resource.Annotations.OfType<ContainerMountAnnotation>().Any(m => m.Target == "/var/k3s") and skip (or replace).

7. Helm --set key injection — only the value is escaped against Helm's parser

K3sBuilderExtensions.Helm.cs:214-228:

foreach (var (key, value) in release.HelmValues)
    sb.Append($" --set {ShellEscape($"{key}={HelmEscape(value)}")}");

The shell layer is correct — POSIX '\'' single-quoting is robust. But HelmEscape is only applied to the value, never the key. Helm's own --set parser splits on ,, treats {/} as list literals, and uses \ as the escape character. A key containing a comma — e.g. WithHelmValue("a.b,c.d=evil", "true") — produces a single shell-quoted argument that Helm then parses as two assignments: a.b=true and c.d=evil. For local dev this is a foot-gun rather than a vulnerability (the AppHost is the trust boundary), but a real bug. None of the tests exercise a key with commas/braces/backslashes.

Fix: apply HelmEscape to both key and value, or validate keys against a conservative pattern (e.g. ^[A-Za-z_][A-Za-z0-9_.\[\]]*$) and throw ArgumentException otherwise.

8. Port-forwarder has no shutdown/dispose path — fire-and-forget leak risk

K3sBuilderExtensions.ServiceEndpoint.cs:179 + the whole of K3sInProcessPortForwarder.cs. _ = Task.Run(() => forwarder.RunAsync(logger, ct), ct); — fire-and-forget with no handle retained. K3sInProcessPortForwarder exposes neither IDisposable/IAsyncDisposable nor a Stop method. The only way to terminate the listener on 0.0.0.0:{port} is the ct passed in from the ResourceReadyEvent subscription.

Two concerns:

1. Cancellation token lifetime: ct is the event-handler's token. It is application-scoped in most Aspire versions, but the contract is not "AppHost stopping" — if the platform ever changes the semantics, the 0.0.0.0 socket leaks for the rest of the AppHost session with no IAsyncDisposable to fall back to.
2. Per-connection task orphan: inside RunAsync, each accepted connection spawns _ = Task.Run(() => ForwardConnectionAsync(tcp, logger, ct), CancellationToken.None) — uses CancellationToken.None, not ct. Unhandled exceptions in those tasks are caught and LogDebug'd only, so a thrown WebSocket exception during shutdown is invisible to operators.

Also note: ForwardConnectionAsync creates a fresh Kubernetes client (and thus a fresh TLS handshake) per accepted TCP connection (line 168-169) — wasteful but each is using-disposed.

Fix: make K3sInProcessPortForwarder implement IAsyncDisposable, store the listener and a linked CancellationTokenSource as fields, retain the forwarder reference on the endpoint resource (or in a registry keyed by resource name), and dispose on AppHost stop via builder.Eventing.Subscribe<BeforeStopEvent> or similar.

🟡 Medium

9. Kustomize directory and helm/kubectl kubeconfig bind-mounts are read-write

K3sBuilderExtensions.Manifest.cs:81, 88 and K3sBuilderExtensions.Helm.cs:99. None of these bind-mounts pass isReadOnly: true:

• Kustomize overlay dir → /k8s-manifests (Manifest.cs:88) — the user's source tree under their checkout.
• container/ (kubeconfig) → /root/.kube (Manifest.cs:81 and Helm.cs:99) — host-side kubeconfig directory.

The kubectl/helm scripts don't write to these paths today, so this is latent. But any plugin (krew, helm-secrets, etc.) the user adds via a custom image override (opts.HelmImage, opts.KubectlImage) gains write access to the host filesystem at those paths. WithReference(cluster) for user containers correctly uses isReadOnly: true (line 385) — the helm/kubectl/manifest paths should match.

Fix: add an overload that passes isReadOnly: true, or build ContainerMountAnnotation directly as WithReference does.

10. K3sServiceEndpointResource.IsReady can be stuck false forever with no recovery surface

K3sBuilderExtensions.ServiceEndpoint.cs:134-188 + K3sServiceEndpointResource.cs:56. RunEndpointAsync is fire-and-forget. The outer try/catch covers the synchronous setup, but the spawned forwarder.RunAsync(...) Task is not awaited — any unhandled exception inside RunAsync that escapes its inner catch (Exception ex) (e.g. an exception thrown by notifications.PublishUpdateAsync during the onReadyChanged callback) terminates the task silently. IsReady stays false, the dependent K3sServiceEndpointHealthCheck.CheckHealthAsync keeps returning Unhealthy, and any consumer using WaitFor(serviceEndpoint) blocks forever with no diagnostic.

The _ = notifications.PublishUpdateAsync(...) call on line 172 is also fire-and-forget; if it throws, the exception escapes the synchronous part of the callback (since Action<bool> cannot await), reaches TaskScheduler.UnobservedTaskException, and disappears.

Fix: make onReadyChanged an async-aware delegate, await PublishUpdateAsync inside it, and surface failures via the resource notification with State = FailedToStart. Add a watchdog: if the forwarder Task faults, publish FailedToStart on the endpoint resource so the dashboard surfaces the problem.

🟢 Minor

11. PR description references an integration test project that doesn't exist

The PR overview claims tests/CommunityToolkit.Aspire.Hosting.K3s.IntegrationTests/ — ubuntu-latest only exists. Only the unit-test project tests/CommunityToolkit.Aspire.Hosting.K3s.Tests/ (87 facts, no [RequiresDocker]) is present. Given the scope of dangerous code (port forwarder lifecycle, kubeconfig race, privileged containers, shell-script injection), the absence of any integration test that exercises an actual k3s container is a real gap — but the discrepancy with the description is what concerns me here.

Fix: add the integration test project (it would catch #2, #3, #5, #6, #8 trivially) or correct the PR description.

12. WithHttpsDeveloperCertificate() on the k3s container is misuse

K3sBuilderExtensions.cs:141. WithHttpsDeveloperCertificate() configures the ASP.NET Core dev cert for JavaScript/Python apps. k3s manages its own server certificate (the very cert whose SANs are extended via --tls-san on lines 90-93). Injecting the ASP.NET dev cert into the k3s container is at best a no-op, at worst pollutes the container with unused files. The build still succeeds.

Fix: remove the .WithHttpsDeveloperCertificate() call; the WithHttpsEndpoint declaration alone is sufficient.

13. AddServiceEndpoint missing whitespace validation on serviceName/@namespace

K3sBuilderExtensions.ServiceEndpoint.cs:32-47. The port range validation is correctly added (lines 44-46). But serviceName and @namespace only get ArgumentNullException.ThrowIfNull — empty or whitespace strings pass validation and produce an unhelpful runtime failure inside the Kubernetes API call. The convention elsewhere in this same file (e.g. WithHelmValuesFile line 133, WithPodSubnet line 281, WithServiceSubnet line 293) is ArgumentException.ThrowIfNullOrWhiteSpace.

Fix: use ArgumentException.ThrowIfNullOrWhiteSpace(serviceName) and (@namespace).

✅ Categories with no findings

• Aspire resource semantics for parent-child — correctly modeled via IResourceWithParent<K3sClusterResource>.
• Helm shell injection via ShellEscape — the POSIX '\'' technique is correctly implemented and properly applied to release name, chart ref, namespace, version, values-file paths, and --set values. The keys are not Helm-escaped (#7) but the shell layer itself is safe.
• Repo conventions on namespaces — extension method classes correctly use namespace Aspire.Hosting; file-scoped; resources use Aspire.Hosting.ApplicationModel;; internal helpers use CommunityToolkit.Aspire.Hosting;. is null/is not null is used consistently. [ResourceName] is applied to name parameters in public extension methods. The CI workflow (tests.yaml) was updated.
• Container runtime annotation usage — ContainerRuntimeArgsCallbackAnnotation(Action<IList<object>>) matches the documented Aspire API. Multiple invocations append redundant --add-host=... flags but Docker handles duplicates idempotently.
• api/CommunityToolkit.Aspire.Hosting.K3s.cs — this is a new file, not an edit to an existing generated file, so the convention is not violated.

Summary

Top 3 most important findings:

1. Blocking: .gitignore contains literal Git merge conflict markers.
2. Major: K3sReadinessHealthCheck._cachedClient is dead state — the registration-factory pattern creates a fresh instance per check, so the cache never carries over. Every health-check tick allocates a new Kubernetes/HttpClient that is never disposed, and rewrites both kubeconfig variants non-atomically (#2 + #3 combined).
3. Major: AllocatePort() + K3sInProcessPortForwarder lifecycle. The port-allocation TOCTOU + interface-mismatch race (#4), the fire-and-forget forwarder with no dispose path (#8), and the IsReady-stuck-false failure mode (#10) together mean the service-endpoint feature has no reliable failure path: a transient startup race can leave the resource permanently unhealthy with no operator-visible diagnostic.

Design assessment:

The overall design — modelling the cluster as a privileged container with Helm/manifest/service-endpoint children — is sound for the stated local-dev inner-loop use case. The decisions to (a) run helm/kubectl as containers rather than requiring host binaries, (b) rewrite kubeconfig into local/ and container/ variants and bind-mount the appropriate one per consumer type, (c) make child containers poll for the kubeconfig file rather than WaitFor-ing the parent (avoiding the cluster-vs-node Ready deadlock), and (d) use an in-process KubernetesClient WebSocket port-forward bound on 0.0.0.0 for both host and container reach — are all well-considered and individually correct. The k3d-inspired init entrypoint and the deliberate avoidance of --cluster-init (with the explanatory comment about etcd peer IP changes) show evidence of real operational testing.

The execution falls short on resource lifecycle management (no IDisposable surfaces, fire-and-forget tasks with no observability, broken cache, leaked HttpClients), idempotency of builder operations (WithDataVolume, WithReference are not safe to call twice), and atomicity of shared state (kubeconfig file rewrites). These are all fixable without touching the design — the core architecture is good; the implementation needs a careful pass on "what happens on the second call / on shutdown / on transient failure".

A few other things to consider.

This LGTM, I'm not going to merge it though as this really belongs to @aaronpowell — I'll leave that to him. Thank you for the PR though, @edmondshtogu.