Skip to content

feat(dns): create BYOC DNS zones only when needed, decided by networks - #380

Open
defang-sam[bot] wants to merge 1 commit into
mainfrom
feat/conditional-dns-zones
Open

feat(dns): create BYOC DNS zones only when needed, decided by networks#380
defang-sam[bot] wants to merge 1 commit into
mainfrom
feat/conditional-dns-zones

Conversation

@defang-sam

@defang-sam defang-sam Bot commented Aug 12, 2026

Copy link
Copy Markdown

What

Two related changes to the pulumi-defang providers (AWS + GCP + Azure).

1. Create BYOC DNS zones only when needed

  • Private zone (AWS Route53 <project>.internal / GCP google.internal.) — created only when a service is in a private network, has host-mode ports, or is a managed Postgres/Redis.
  • GCP public delegate zone + wildcard cert — created only when the project has a public ingress service. AWS already gated this (infra.go NeedIngress) and its public zone is caller-provided. Resolves both // TODO: make this optional, so we can save $$.

2. Decide public vs private by Compose networks, not port mode

Per the decided model (defang-mvp#1668 / defang#1105, defang-docs networking.mdx): the default network is public unless internal: true; any other network, or internal, is private. Port mode only selects the exposure typeingress = public load balancer, host = direct public IP.

This fixes a service that has an ingress port but sits in a private/internal network from leaking into the public zone, the public ALB/LB routing, and a public *.defang.app FQDN. Threaded networks through:

  • common.ServiceFQDN (the shared public/private FQDN chokepoint);
  • common.NeedIngress / common.NeedPrivateZone (the zone gates);
  • the AWS public listener-rule + endpoint gates (ecs.go) and the GCP external-LB filter (alb.go);
  • the GCP/Azure private-FQDN assignment.
  • Also wired real networks into Azure's ingress External flag (containerapp.go — was a nil TODO).

Behavior / compatibility

  • The common case is unchanged: a project with no explicit networks: is implicitly in the non-internal default network → public, exactly as before. Only projects that declare a private/internal network are affected (few, per maintainer guidance).
  • Transitional: host-mode ports remain a private trigger, so a default-network host service keeps its .internal name until public+host DNS is implemented (pulumi-defang#253). InPrivateNetwork is now the networks-based decision; port mode is retained only for the exposure type.

Follow-ups (out of scope, per the discussion)

  • Emit a warning when port mode and networks disagree (planned follow-up).
  • Full internal-LB routing for the private + ingress combo, and public+host public DNS (pulumi-defang#253 / defang#1282).

Tests

  • common: NeedPrivateZone, NeedIngress, and ServiceFQDN unit tests extended with network cases (private/internal network overrides port mode).
  • GCP integration: TestConstructProjectPrivateNetworkIngressStaysPrivate — an ingress service in a private network gets no public zone/cert but does get a private zone. Existing zone tests updated for the conditional behavior.

Verification (local, bootstrapped Go 1.26.5 + golangci-lint 2.11.4)

go build ./provider/... ✅ · go test ./provider/... ✅ · cd tests && go test -short ./... (AWS+GCP+Azure) ✅ · golangci-lint run --config=.golangci.yaml ./provider/...0 issues ✅. No provider input/output schema fields changed (only internal SharedInfra + function params), so no SDK/schema drift is expected; CI's schema/SDK job is the backstop.

🤖 Generated with Claude Code

@defang-sam
defang-sam Bot temporarily deployed to defang-staging August 12, 2026 18:02 Inactive
@defang-sam
defang-sam Bot force-pushed the feat/conditional-dns-zones branch from a209856 to c2e6f5c Compare August 12, 2026 18:21
@defang-sam

defang-sam Bot commented Aug 12, 2026

Copy link
Copy Markdown
Author

Force-pushed a lint fix (the test job runs golangci-lint, which I couldn't run in my first pass):

  • //nolint:funlen on BuildGlobalConfig — my restructure pushed it just over 100 lines; matches the existing CreateProjectInfra convention for sequential infra-setup functions.
  • Shortened two SharedInfra field comments to satisfy lll (≤120).
  • Drive-by: //nolint:funlen on provider/defanggcp/service.go createService — this was already failing lint on main (the last push, Merge feat(gcp): grant x-defang-policies roles to the service identity #350, has test = failure), unrelated to this change. Added the same convention so this PR's test job goes green rather than inheriting the pre-existing failure. Happy to drop it into a separate PR if preferred.

Verified locally this time with golangci-lint v2.11.4 (golangci-lint run --config=.golangci.yaml ./provider/...0 issues) plus build + unit + integration tests.

@defang-sam
defang-sam Bot temporarily deployed to defang-staging August 12, 2026 18:23 Inactive
Two related changes to the pulumi-defang providers.

1. Create DNS zones only when a project needs them, instead of always:
   - private zone: only when a service is in a private network, has host-mode
     ports, or uses managed Postgres/Redis;
   - GCP public delegate zone + wildcard cert: only when the project has a
     public ingress service. AWS already gated this and its public zone is
     config-provided. Resolves the two "TODO: make this optional, save $$".

2. Decide public vs private by Compose networks, not port mode. The default
   network is public unless internal:true; any other network, or internal, is
   private. Port mode only selects the exposure type (ingress = public load
   balancer, host = direct public IP). This stops a service that has an ingress
   port but sits in a private/internal network from leaking into the public
   zone, routing, and FQDN. Threaded networks through ServiceFQDN, the AWS/GCP
   public-routing gates, and the zone gates; also wired real networks into
   Azure's ingress External flag (was a nil TODO).

Host-mode ports stay a transitional private trigger, so default-network host
services keep their internal name until public+host DNS exists (pulumi-defang#253).
The common case (no explicit networks -> default -> public) is unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@defang-sam
defang-sam Bot force-pushed the feat/conditional-dns-zones branch from c2e6f5c to 0bf4d5e Compare August 12, 2026 21:10
@defang-sam defang-sam Bot changed the title feat(dns): create BYOC DNS zones only when needed feat(dns): create BYOC DNS zones only when needed, decided by networks Aug 12, 2026
@defang-sam

defang-sam Bot commented Aug 12, 2026

Copy link
Copy Markdown
Author

Amended per review feedback: public/private is now decided by Compose networks, not port mode (the decided model — defang-mvp#1668 / defang#1105). Port mode is kept only for the exposure type (ingress = public LB, host = direct public IP). Key effect: an ingress service in a private/internal network no longer leaks to the public zone/routing/FQDN. The no-explicit-networks common case is unchanged; a divergence warning and full private+ingress internal routing are follow-ups (#253/#1282). Also fixed the buildIngress(svc, nil) TODO in the Azure provider to pass real top-level networks.

@defang-sam
defang-sam Bot temporarily deployed to defang-staging August 12, 2026 21:18 Inactive
@defangdevs

Copy link
Copy Markdown
Contributor

Reviewed head 0bf4d5e. The gating logic is internally consistent (traced every zone-handle consumer — no nil output reaches a resource arg), but three items should block merge as-is:

1. GCP public delegate zone becomes destroyable — delegation-breaking. provider/defanggcp/gcp/gcp.go:191-199 gates createWildcardCert (and with it the public-dns managed zone) on common.NeedIngress. A stack that momentarily has no public ingress service (ports commented out, service moved to an internal network, worker-only phase) gets its managed zone deleted on the next up. Recreating it mints new name servers, and nothing re-delegates: the CLI's PrepareDomainDelegation/EnsureDNSZoneExists path is idempotent-create only, and nothing in this repo reads zone.NameServers. The zone carries no retainOnDelete/protect. This is structurally the same failure as the 2025-10-24 s.defang.io validation-CNAME deletion. The expensive resource (external LB) was already gated on main (alb.go:81-83), so this hunk saves ~$0.20/month per stack. Suggest: keep creating the zone whenever domain != "" and gate only the cert/DNS-authorization — or at minimum add pulumi.RetainOnDelete(true) to the zone (as cert.go:229 already does on the AWS BYOD cert).

2. AWS private-zone removal can hard-fail deploys. provider/defangaws/aws/networking.go:139-145 deletes the Route53 private zone for every stack with no host-mode port and no managed Postgres/Redis — the most common shape, not the edge case the description implies. The route53 sidecar writes A records into that zone out-of-band at runtime, so any stack with stale records hits HostedZoneNotEmpty and the deploy fails (ForceDestroy follows the recipe flag, default false — networking.go:182, recipe.go:23). Suggest ForceDestroy: pulumi.Bool(true) for this internal, Pulumi-owned zone.

3. InPublicNetwork false-negative now gates zone destruction. provider/common/algo.go:85-93: when top-level networks is non-empty but a service omits its networks: key, the service is treated as not-public — unlike AcceptPublicTraffic twelve lines above, which applies the "no networks section ⇒ default network" rule. compose-go normalization shields the CLI path, but direct Pulumi/YAML consumers of the provider hit it, and via item 1 it destroys the GCP public zone. One-line fix + a unit test for "top-level networks declared, service omits networks:".

Worth fixing in the same PR: the AWS side of "private-network ingress" is silently unreachable — the route53 sidecar is still gated on svc.HasHostPorts() (ecs.go:758), so DEFANG_FQDN=<label>.<project>.internal points at a name with no record (GCP got the internal LB + A record; AWS got nothing). And the endpoint switch at ecs.go:1006 disagrees with ServiceFQDN for the same shape.

Follow-ups (separate issues): ServiceNameReplacer in DefangLabs/defang decides private-vs-public substitution purely on port mode and must become networks-aware in lockstep, else env-var references to private-network ingress services rewrite to a public hostname that no longer resolves. And a domainname: on a private-network service still gets a public ALIAS + ACM cert pointed at an ALB with no listener rule for it (cert.go:177-190) — decide whether domainname is an explicit opt-in to public.

Also: the PR body needs a "what happens on the next up for an existing stack" section covering each of the three zones — that's the part a reviewer most needs here.

@lionello lionello left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also see comments from @defangdevs

Comment thread provider/common/algo.go
func NeedIngress(networks compose.Networks, services compose.Services) bool {
for _, svc := range services {
if svc.HasIngressPorts() && svc.Postgres == nil && svc.Redis == nil {
if svc.HasIngressPorts() && svc.Postgres == nil && svc.Redis == nil && InPublicNetwork(networks, svc) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should anticipate (future) private ingress. To that extend, after this edit we need to either rename this function now to NeedPublicIngress or move the InPublicNetwork to the caller.

BuildInfra *BuildInfra // non-nil when at least one service has a build config
ServiceConnection *servicenetworking.Connection // non-nil when any service uses managed Postgres or Redis
PrivateZone pulumi.StringOutput // managed zone name for the private google.internal. zone
PrivateZone pulumi.StringOutput // google.internal. zone; empty when not needed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be pulumi.StringPtrOutput then?

PublicIP *compute.GlobalAddress
WildcardCertId pulumi.StringInput // non-nil when a domain is configured
PublicZoneId pulumi.StringInput // managed zone name; non-nil when a domain is configured
WildcardCertId pulumi.StringInput // set when a domain is configured and the project has ingress

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto, should this be pulumi.StringPtrInput

WildcardCertId pulumi.StringInput // non-nil when a domain is configured
PublicZoneId pulumi.StringInput // managed zone name; non-nil when a domain is configured
WildcardCertId pulumi.StringInput // set when a domain is configured and the project has ingress
PublicZoneId pulumi.StringInput // public managed zone name; set alongside WildcardCertId

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto, should this be pulumi.StringPtrInput now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants