Repository navigation
Conversation
6ef901d to
c546005
Compare
abd287d to
db527d6
Compare
db527d6 to
f321ac6
Compare
f321ac6 to
2a91264
Compare
9c2f22c to
bf43ed8
Compare
bf43ed8 to
10a428b
Compare
ecv
left a comment
There was a problem hiding this comment.
Approving. Context lookups are namespace-scoped and UID-pinned, and withdrawal and shard selection filter on project UID, so I found no cross-tenant path. The reconciler is wired and given RBAC in #238. Follow-ups are in #261: destination claims that are never released and share no key space with generated addresses, and duplicate consumerID ownership decided by name order rather than age.
The merge-base changed after approval.
ecv
left a comment
There was a problem hiding this comment.
Re-approving at ae2a3d6. The new commits reject non-routable destinations, refuse addresses in the regional prefix or with a regional claim, and give consumerID ownership to the oldest context. Claim release and the DNSResolverBinding watch are still open, and #261 tracks them along with a zoned-address gap in ClaimDestination.
…l-dns-context-access
The parent branch removed `DNSZoneAssociationSpec.Managed`. The context reconciler already marks the managed association with its controller owner reference and the managed label, so it no longer sets the flag. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…l-dns-context-access
…l-dns-context-access Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…l-dns-context-access Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kevwilliams
left a comment
There was a problem hiding this comment.
Verified destination claims refuse the regional resolver pool and other contexts' addresses, and duplicate consumer IDs keep the older context.
ecv
left a comment
There was a problem hiding this comment.
Approve at 9a52cea. Since my last approval at ae2a3d6, the only change in this PR's own files is 2513558, which stops setting the removed DNSZoneAssociation managed flag. Everything else came in from the base branch through merges. I found no path that lets one project resolve another project's private zones. go build ./..., go vet ./internal/internaldns/... and the tests for the three touched packages pass locally, and CI is green. The envtest case skips locally, so I relied on CI for it.
Earlier findings at head
| Finding | Status | Evidence |
|---|---|---|
Duplicate consumerID goes to the older context, not the earlier name |
Fixed | context_reconciler.go:114-128 sorts by creationTimestamp then UID; TestNewContextCannotDisplaceExistingConsumerOwner covers it |
| Destination claims accept loopback, link-local and multicast | Fixed | allocator.go:51 requires IsGlobalUnicast |
| Destination can equal another context's regional address | Fixed | allocator.go:55-69 refuses the regional prefix and any existing dns-address-claim-*; tests at allocator_test.go:80,102 |
| Zoned or IPv4-mapped destination | Closed by the API | destinationAddress is isIP(self) and immutable (internaldns_project_types.go:95,103), and isIP rejects zones |
| Destination claims never released | Open, tracked in #261 | allocator.go:45-48 keeps claims on purpose. Because the destination is immutable, each new access binding leaks one claim, rather than each edit |
DNSResolverBinding not watched |
Open, tracked in #261 | reconciler.go:118-124; serving acknowledgements are seen only on the LeaseDuration/2 requeue |
Blocking
None.
Non-blocking
confirmAccessWithdrawaltreats reasonInvalidManagedDomainas terminal (context_reconciler.go:427), but nothing sets that reason. An invalidManagedDomainSuffixis reported asInvalidIdentitywith the message "consumerID is required" (context_reconciler.go:131-152). Withdrawal behaves the same either way, but the condition misleads whoever reads it. SetInvalidManagedDomainin that branch.- A transient error from
ClaimDestinationis folded intovalid = false(context_reconciler.go:279). The access binding then flips toAccepted=False/AccessRejecteduntil the next requeue. Serving is safe, becauseconfirmAccessWithdrawalkeeps the binding when the spec still matches. Return API errors and keepInvalidOrStaleAccessfor genuine refusals. ResolveNamingaccepts anyReadycontext (naming.go:40-44). A context withmanagedNamespace.enabled: falseisReadyatcontext_reconciler.go:139, so the canonical name gets an emptyDNSZoneRef. If the context was once enabled, it gets the stale ref, becauseStatus.ManagedNamespaceis never cleared. The function also skips theObservedGenerationcheck thatresolverContextReadymakes. No caller exists in this PR, so fix this before one lands.ClaimDestination's second parameter is namedaccessUID, but the call atcontext_reconciler.go:279passes the context UID. The behavior is fine, since the claim belongs to the context. Rename the parameter so the owner key reads correctly.Reconcilefalls back toreq.NamespacewhenProjectNamespaceis empty (reconciler.go:99-102). It then writes bindings for that namespace under the configuredProjectUID. That is safe only with one project per control plane. RequireProjectNamespace, or say in the code that this is the deployment assumption.
…l-dns-context-access
…laims A resolver context rejected for an invalid managed domain suffix now reports `InvalidManagedDomain` with a message saying the suffix is not a valid DNS name, instead of `InvalidIdentity` and "consumerID is required". The access binding withdrawal path already treated that reason as terminal. The allocator now wraps every merit-based refusal of a service destination in `model.ErrDestinationRefused`. The access reconciler rejects the binding only for that error and returns any other error, so a transient API failure is retried instead of flipping an accepted binding to `AccessRejected` until the next requeue. `ResolveNaming` now requires the context's Ready condition to match its current generation and its managed namespace to carry a zone UID, so a context without a managed namespace, or with a stale one, yields no canonical name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ecv
left a comment
There was a problem hiding this comment.
Re-approving at a489e2d. An invalid suffix now reports InvalidManagedDomain. Only model.ErrDestinationRefused rejects a binding, so a transient API error retries instead of flipping an accepted binding to AccessRejected. ResolveNaming now requires a current Ready context with a managed zone UID.
The merge-base changed after approval.
ecv
left a comment
There was a problem hiding this comment.
Re-approving. The head is unchanged; only the base moved after the parent merged.
Summary
Create managed private zones under their DNS context. Product services discover the assigned zone and domain through context status. DNS reserves query destinations and manages downstream versions; network services supplies private connectivity. Applied access persists until confirmed withdrawal.
An established context keeps ownership when another context requests the same consumer identity. Integration-selected destinations must be unicast addresses outside the regional resolver pool, preventing overlap with another context's allocated address.
Test plan
Stacked on #232. Architecture: #229.
Related: datum-cloud/enhancements#921