Draft mirror: Fix Radius service discovery for deployment testing#18809
Draft mirror: Fix Radius service discovery for deployment testing#18809mitchdenny wants to merge 6 commits into
Conversation
…ius E2E Add RadiusAzureResourcesDeploymentTests documenting the current gap where publishing a Radius container that WithReferences a cloud-managed Azure resource hangs resolving Azure Bicep outputs. Add Redis output-cache and direct apiservice diagnostic endpoints to the Starter Radius E2E and assert the cache path; the direct apiservice/weather assertions remain gated pending confirmation of the Radius container Service DNS. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b43262f1-59bc-4b4b-a2b1-33b782363a43
Aspire's Radius publisher delegates Kubernetes Service creation to an
external recipe (radius-project/resource-types-contrib
kubernetes-containers.bicep), but emitted service-discovery values did not
match what that recipe creates, so cross-container calls (e.g. a Blazor
/weather page calling apiservice) could not resolve in-cluster.
Align the emitted services__* values with the recipe contract:
- Host: the recipe names the ClusterIP Service {name}-{name}, not the bare
resource name. GetHostAddressExpression now emits
{name}-{name}.{ns}.svc.cluster.local via a shared RadiusServiceDiscovery
helper (single source of truth for the Service name).
- Port: the recipe Service listens on the container port (port ==
targetPort == containerPort), not the proxy/host port. Override
GetEndpointPropertyExpression so Url/Port/HostAndPort/TargetPort all use
the resolved container port from the same helper the Bicep container-port
emission uses, so the URL and the generated Service can never disagree.
- Service existence: resolve ports through the framework's
ResolveEndpoints() (the same primitive the Kubernetes publisher uses) so
a containerized project's default HTTP endpoint gets the standard
container port (8080) and the recipe creates a Service; the synthetic
default HTTPS endpoint is skipped (no in-cluster TLS). Reading
DefaultHttpsEndpoint requires internals access, granted via
InternalsVisibleTo mirroring Aspire.Hosting.Kubernetes.
- Dedup: multiple endpoints can resolve to the same (containerPort,
protocol); ResolvePorts now deduplicates them (first wins), matching the
Kubernetes publisher's ToService dedup, so the recipe never emits
duplicate Service ports.
Unit tests cover the resolver, per-property emission, the dedup, and the
container-name rename guard. The RadiusStarterDeploymentTests /weather and
direct apiservice assertions are un-skipped and now assert the
apiservice-apiservice Service and its container port. README updated.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b43262f1-59bc-4b4b-a2b1-33b782363a43
…iew feedback Addresses review findings on PR #18797: - Fail fast when the doubled '{name}-{name}' recipe Service name would exceed the 63-character Kubernetes DNS-label limit. - Reject non-literal container-name renames and container-port changes/removals in ConfigureRadiusInfrastructure callbacks, since service discovery is emitted before callbacks run and would otherwise silently point at a stale name/port. - Strengthen the E2E Step 29 assertion to verify the full recipe contract (type == ClusterIP, port == targetPort == 8080). - Correct the stale Step 26 comment that claimed the recipe creates no Service. Adds regression tests for the new publish-time validations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b43262f1-59bc-4b4b-a2b1-33b782363a43
Respond to Copilot review feedback on #18797: - Key the pre-callback container-port snapshot by the stable app-model map key (resource name) and capture protocol as well as port, so a callback that swaps in a new construct or changes only the protocol is still validated. - Reject non-literal container names/ports/protocols using a non-null Bicep Expression as the signal (an expression-backed BicepValue<int> reports a default LiteralValue of 0, not null). - Detect removal/replacement of a whole container, not just port changes. - Run the Kubernetes 63-char Service-name length check on the FINAL container set so a callback that adds the first port to a portless container is caught. - Reword the container-name guard to attribute the restriction to Aspire service discovery, not the Radius v2 schema (which permits differing keys). - Fix the ResolveServicePort XML-doc bullets to document that the first portless HTTP/HTTPS project endpoint defaults to 8080 while the synthetic default HTTPS endpoint returns null. - Pin the recipe doc-link to an immutable commit SHA so the documented Service-name/port contract can be verified even if the recipe's main moves. - Rewrite the Radius/Azure cloud-managed-resource gap test to assert the intended fail-fast behavior and mark it [ActiveIssue(#18802)]; pin its container image to aci-helloworld@sha256. Add regression tests for protocol change, non-literal port, whole-container removal, and post-callback port addition on a too-long name. Radius unit tests: 204/204. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b43262f1-59bc-4b4b-a2b1-33b782363a43
Respond to the follow-up Copilot review on #18797: - Skip the container-preservation check for empty (portless) port snapshots so a ConfigureRadiusInfrastructure callback can remove a portless container. A portless container has no ClusterIP Service and no `services__*` value can address it, so removing it is harmless and must not be rejected. Add a regression test asserting the removal succeeds. - Correct three doc/comment locations that still attributed the container name==map-key equality to the Radius v2 schema/recipe. Radius permits distinct top-level and container-map names; equality is an Aspire service-discovery limitation. Reword RadiusServiceDiscovery remarks, RadiusContainerConstruct ContainerMapKey doc, and the ConfigureCallback_RenamingContainerName_Throws test comment accordingly. - Update the deployment E2E README section for the Radius/Azure gap test to describe the new [ActiveIssue(#18802)]-skipped, fail-fast-asserting test and instruct maintainers to remove [ActiveIssue] when the gap is closed. Radius unit tests: 205/205. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b43262f1-59bc-4b4b-a2b1-33b782363a43
- Gate the container name/map-key equality guard on a non-empty pre-callback port snapshot. Portless baseline containers and callback-added containers have no Service and no services__* contract, so Radius permits their top-level name to differ from the map key; renaming them must be allowed (matching the already-permitted removal of a portless container). - Re-run the (containerPort, protocol) dedup on the FINAL post-callback port set so a callback that adds a second endpoint colliding with a preserved port can't make the recipe emit duplicate Kubernetes Service ports. - Compute the Service-name length check from the actual literal top-level name + map key via a new RadiusServiceDiscovery.GetServiceName(topLevelName, mapKey) overload, instead of assuming name == map key. - Make RadiusEnvironmentResource.GetEndpointPropertyExpression an explicit IComputeEnvironmentResource implementation so it stays off the public class surface (matching the Kubernetes/Docker publishers). - Tests: give the two rename-guard tests an endpoint so the guard still applies; add ConfigureCallback_RenamingPortlessContainerName_DoesNotThrow and ConfigureCallback_AddingDuplicatePortToContainer_Throws. Radius suite 207/207. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b43262f1-59bc-4b4b-a2b1-33b782363a43
|
/deployment-test |
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 18809Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 18809" |
github-actions
Bot
added
the
needs-area-label
|
🚀 Deployment tests starting on PR #18809... 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. 49 / 100 test projects · 4 jobs, from 11 changed files. Selected test projects (49 / 100)
Selected jobs (4)
How these were chosen — grouped by what changed
🔧 show 46
📦 affected project 🔧 🔧 🔧 🔧 🧪 🧪 🧪 🧪 🧪 🧪 Job reasons
Selection computed for commit |
There was a problem hiding this comment.
Pull request overview
Fixes Radius service discovery to match recipe-generated Kubernetes Services and ports.
Changes:
- Generates
{name}-{name}service FQDNs using container ports. - Deduplicates ports and validates callback mutations.
- Expands unit and live AKS deployment coverage.
Show a summary per file
| File | Description |
|---|---|
tests/Aspire.Hosting.Radius.Tests/RadiusEnvironmentResourceTests.cs |
Tests Radius endpoint expressions. |
tests/Aspire.Hosting.Radius.Tests/Publishing/RadiusServiceDiscoveryTests.cs |
Tests service names and port resolution. |
tests/Aspire.Hosting.Radius.Tests/Publishing/ContainerEnvironmentEmissionTests.cs |
Tests service-discovery and port emission. |
tests/Aspire.Hosting.Radius.Tests/Publishing/ConfigureRadiusInfrastructureTests.cs |
Tests post-callback invariants. |
tests/Aspire.Deployment.EndToEnd.Tests/README.md |
Documents Radius deployment coverage. |
tests/Aspire.Deployment.EndToEnd.Tests/RadiusStarterDeploymentTests.cs |
Verifies live cross-container connectivity. |
tests/Aspire.Deployment.EndToEnd.Tests/RadiusAzureResourcesDeploymentTests.cs |
Documents the Azure-reference publish gap. |
src/Aspire.Hosting/Aspire.Hosting.csproj |
Grants required internals access. |
src/Aspire.Hosting.Radius/RadiusEnvironmentResource.cs |
Emits recipe-compatible endpoint values. |
src/Aspire.Hosting.Radius/Publishing/RadiusServiceDiscovery.cs |
Centralizes service naming and port resolution. |
src/Aspire.Hosting.Radius/Publishing/RadiusInfrastructureBuilder.cs |
Emits and validates recipe-compatible ports. |
src/Aspire.Hosting.Radius/Publishing/Constructs/RadiusContainerConstruct.cs |
Exposes the immutable container map key internally. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 3
- Review effort level: Medium
| "if [ \"$code\" = \"0\" ]; then cat ../publish.log; echo \"Radius publish unexpectedly succeeded for a cloud-managed Azure reference; it should fail fast until Azure resource translation is supported.\"; exit 1; fi; " + | ||
| "if [ -f ../out/app.bicep ]; then cat ../publish.log; echo \"Radius publish produced app.bicep for a cloud-managed Azure reference; it should fail before emitting output.\"; exit 1; fi; " + |
| $"label). Shorten the resource name to at most {(MaxKubernetesServiceNameLength - 1) / 2} characters " + | ||
| $"so the doubled '{{name}}-{{name}}' Service name stays within the limit."); |
| /// The immutable <c>properties.containers</c> map key captured at construction (the Aspire | ||
| /// resource name). The Radius v2 schema and recipe do <em>not</em> require this to equal the | ||
| /// top-level <c>name:</c> (<see cref="ContainerName"/>) — Radius permits distinct names. Aspire | ||
| /// requires them to match because it derives service discovery from the resource name, and the | ||
| /// publisher validates that invariant after running <c>ConfigureRadiusInfrastructure</c> | ||
| /// callbacks so service discovery cannot silently disagree with the generated Service name. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Description
Draft mirror of #18797 used to run the Deployment E2E Tests workflow from a branch in
microsoft/aspire.This branch points at the exact source PR commit (
9fa6df87189624e61aa552e73bb853dce24bf584) and contains no additional changes. The functional change fixes Radius service discovery so generatedservices__*addresses match the ClusterIP Service created by the Radius Kubernetes recipe.User-facing usage
Radius-deployed containers now discover a referenced project at the recipe-created service name and container port, for example:
This mirror exists only to trigger the live Radius/AKS deployment scenario through
/deployment-test. Review and merge the original PR rather than this draft.Fixes # (issue)
Checklist
<remarks />and<code />elements on your triple slash comments?