[management, proxy] Add require_subdomain capability for proxy clusters#5628
Merged
[management, proxy] Add require_subdomain capability for proxy clusters#5628
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a RequireSubdomain capability propagated from proxy config through gRPC and controller layers into domain API and service manager; service create/update now validate and reject bare cluster domains when a cluster requires a subdomain. Changes
Sequence DiagramsequenceDiagram
participant User as User/API
participant SMgr as Service Manager
participant Ctrl as Proxy Controller
participant GRPC as ProxyServiceServer (gRPC)
participant Conn as Connected Proxy(s)
participant DMgr as Domain Manager
User->>SMgr: Create/Update Service(domain, ...)
SMgr->>SMgr: derive ProxyCluster from domain
SMgr->>Ctrl: ClusterRequireSubdomain(cluster)
Ctrl->>GRPC: ClusterRequireSubdomain(cluster)
GRPC->>Conn: read conn.capabilities.RequireSubdomain
alt any conn true
GRPC-->>Ctrl: true
else any conn reported (but none true)
GRPC-->>Ctrl: false
else none reported
GRPC-->>Ctrl: nil
end
Ctrl-->>SMgr: *bool (true/false/nil)
alt capability == true AND domain == cluster
SMgr-->>User: Error "requires a subdomain label"
else
SMgr->>DMgr: GetDomains()
DMgr->>Ctrl: ClusterRequireSubdomain(cluster)
Ctrl-->>DMgr: *bool
DMgr-->>User: Domain list (RequireSubdomain set)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lixmal
changed the title
lixmal
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/internals/modules/reverseproxy/domain/manager/manager.go`:
- Around line 309-312: The function extractClusterFromCustomDomains currently
returns the first matching custom domain which can pick a less-specific suffix
match; instead, iterate all customDomains in extractClusterFromCustomDomains,
track the best (longest) matching cd.Domain where serviceDomain == cd.Domain or
strings.HasSuffix(serviceDomain, "."+cd.Domain), and after the loop return the
TargetCluster for the longest match (or false if none). Update the function to
prefer exact/more-specific domain matches by length and add a regression test
that covers overlapping domains (e.g., entries for "example.com" and
"app.example.com") to ensure app.example.com resolves to the more specific
target cluster.
In `@management/internals/modules/reverseproxy/service/manager/manager.go`:
- Around line 568-570: The current validateSubdomainRequirement check only runs
in handleDomainChange and is skipped on updates that don't change
service.Domain; move or add a call to
validateSubdomainRequirement(service.Domain, service.ProxyCluster) at the end of
persistServiceUpdate (after the domain-change branch completes) so the final
ProxyCluster is validated on every update; specifically, ensure
persistServiceUpdate invokes validateSubdomainRequirement with the
service.Domain and the resolved/new Cluster (the same values used when creating
newCluster in handleDomainChange) and returns the error to block saves when a
cluster now requires subdomains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f25788f2-7803-49bb-9b57-49a07e8e7c19
⛔ Files ignored due to path filters (1)
shared/management/proto/proxy_service.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (17)
management/internals/modules/reverseproxy/domain/domain.gomanagement/internals/modules/reverseproxy/domain/manager/api.gomanagement/internals/modules/reverseproxy/domain/manager/domain_test.gomanagement/internals/modules/reverseproxy/domain/manager/manager.gomanagement/internals/modules/reverseproxy/proxy/manager.gomanagement/internals/modules/reverseproxy/proxy/manager/controller.gomanagement/internals/modules/reverseproxy/proxy/manager_mock.gomanagement/internals/modules/reverseproxy/service/manager/l4_port_test.gomanagement/internals/modules/reverseproxy/service/manager/manager.gomanagement/internals/modules/reverseproxy/service/manager/manager_test.gomanagement/internals/shared/grpc/proxy.gomanagement/internals/shared/grpc/proxy_test.goproxy/cmd/proxy/cmd/root.goproxy/server.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.goshared/management/proto/proxy_service.proto
|
pascal-fischer
approved these changes
Mar 20, 2026
This was referenced Mar 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
require_subdomainfield toProxyCapabilitiesproto andReverseProxyDomainAPI responserequire_subdomain=true, service creation/update rejectsdomain == cluster_domainextractClusterFromCustomDomains--require-subdomain/NB_PROXY_REQUIRE_SUBDOMAINflag to proxy binary (default false)Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Configuration is via proxy flag/env var, no user-facing docs needed.
Summary by CodeRabbit
New Features
Bug Fixes & Validation
Other