[management, proxy] Support multiple L4 port mappings per reverse-proxy domain - #6842
Draft
heywander wants to merge 7 commits into
Draft
[management, proxy] Support multiple L4 port mappings per reverse-proxy domain#6842heywander wants to merge 7 commits into
heywander wants to merge 7 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughReverse-proxy services gain canonical domain handling, shared HTTP/L4 ownership rules, persistent multi-port mappings, capability-aware delivery, migration support, and runtime routing updates. Related management lookups, proxy lifecycle cleanup, policy generation, and tests are updated accordingly. ChangesReverse-proxy service contracts
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Management
participant Store
participant Proxy
Client->>Management: submit service with port_mappings
Management->>Store: validate and persist mappings
Store-->>Management: service mapping
Management->>Proxy: send capability-filtered mapping
Proxy->>Proxy: expand listener and target ranges
Proxy-->>Management: install or remove runtime routes
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
|
This was referenced Jul 21, 2026
heywander
marked this pull request as ready for review
July 21, 2026 05:50
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
proxy/internal/tcp/router.go (1)
218-231: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winConditionally cancel the service context to prevent dropping connections on stale removals.
If
RemoveRouteis called redundantly or processes a stale removal for a route that no longer exists on thishost,slices.DeleteFuncsafely ignores the missing route, butr.cancelServiceLocked(svcID)is executed unconditionally.This will abruptly cancel the shared context for
svcID, unintentionally dropping active connections for the service if it had been re-established or if it operates as a fallback route. Compare the length of the slice before and after deletion to ensure the service context is only canceled when a route is actually removed.🔒️ Proposed fix to conditionally cancel
func (r *Router) RemoveRoute(host SNIHost, svcID types.ServiceID) { host = SNIHost(netutil.NormalizeHost(string(host))) r.mu.Lock() defer r.mu.Unlock() + initialLen := len(r.routes[host]) r.routes[host] = slices.DeleteFunc(r.routes[host], func(route Route) bool { return route.ServiceID == svcID }) if len(r.routes[host]) == 0 { delete(r.routes, host) } - r.cancelServiceLocked(svcID) + if initialLen != len(r.routes[host]) { + r.cancelServiceLocked(svcID) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@proxy/internal/tcp/router.go` around lines 218 - 231, Update Router.RemoveRoute to record the host route slice length before slices.DeleteFunc and cancel svcID via cancelServiceLocked only when deletion reduces that length; preserve the existing host cleanup behavior.
🧹 Nitpick comments (3)
management/server/store/sql_store_service_test.go (1)
207-268: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueTiming-based blocking assertion relies on a fixed 100ms window.
The
case <-time.After(100 * time.Millisecond):branch (line 254) asserts non-blocking behavior indirectly by absence of completion within a fixed window. This is a common lock-test pattern but can be flaky under heavy CI load if the first transaction's lock acquisition is slow to actually contend.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@management/server/store/sql_store_service_test.go` around lines 207 - 268, Replace the fixed 100ms time.After assertion in TestSqlStore_ServiceDomainLockSerializesAbsentHostname with a deterministic synchronization mechanism that confirms the second transaction cannot complete before releaseFirst is closed. Preserve the existing checks that the first transaction holds the lock, the second completes only after release, and exactly one canonical domain-lock row is created.proxy/server.go (1)
1752-1791: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce
mappingsOwnSameRuntimecognitive complexity to satisfy the failing SonarCloud gate.SonarCloud reports this method at cognitive complexity 26 (limit 25), which is failing the check. Extracting the HTTP-vs-L4 coexistence branch (the
!aL4 || !bL4block) and the L4-vs-L4 listener match into small helpers would drop it below the threshold without behavior change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@proxy/server.go` around lines 1752 - 1791, Reduce cognitive complexity in mappingsOwnSameRuntime by extracting the !aL4 || !bL4 coexistence logic and the L4-vs-L4 listener matching loop into focused helper methods. Keep mappingsOwnSameRuntime responsible for classification and delegating to these helpers, preserving all existing host, listener, fallback, network, and port matching behavior.Source: Linters/SAST tools
management/internals/modules/reverseproxy/service/manager/manager.go (1)
417-499: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce
checkPortConflictcognitive complexity (SonarCloud: 31 vs 25 allowed).Consider extracting the legacy single-port branch and the multi-port branch into two separate helper methods to bring this under the threshold.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@management/internals/modules/reverseproxy/service/manager/manager.go` around lines 417 - 499, Reduce the cognitive complexity of checkPortConflict by extracting its legacy single-port handling and multi-port mapping handling into separate Manager helper methods. Keep the existing validation, store queries, conflict rules, error messages, and self-service exclusions unchanged, with checkPortConflict retaining only shared setup and branch delegation.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@management/internals/modules/reverseproxy/service/manager/manager.go`:
- Around line 841-864: Update samePortBasedListeners to compare filtered
port-based mappings order-independently by sorting both left and right
collections using their protocol and listen-port boundaries before the existing
equality comparison. Preserve nil and non-port-based filtering and all compared
fields.
---
Outside diff comments:
In `@proxy/internal/tcp/router.go`:
- Around line 218-231: Update Router.RemoveRoute to record the host route slice
length before slices.DeleteFunc and cancel svcID via cancelServiceLocked only
when deletion reduces that length; preserve the existing host cleanup behavior.
---
Nitpick comments:
In `@management/internals/modules/reverseproxy/service/manager/manager.go`:
- Around line 417-499: Reduce the cognitive complexity of checkPortConflict by
extracting its legacy single-port handling and multi-port mapping handling into
separate Manager helper methods. Keep the existing validation, store queries,
conflict rules, error messages, and self-service exclusions unchanged, with
checkPortConflict retaining only shared setup and branch delegation.
In `@management/server/store/sql_store_service_test.go`:
- Around line 207-268: Replace the fixed 100ms time.After assertion in
TestSqlStore_ServiceDomainLockSerializesAbsentHostname with a deterministic
synchronization mechanism that confirms the second transaction cannot complete
before releaseFirst is closed. Preserve the existing checks that the first
transaction holds the lock, the second completes only after release, and exactly
one canonical domain-lock row is created.
In `@proxy/server.go`:
- Around line 1752-1791: Reduce cognitive complexity in mappingsOwnSameRuntime
by extracting the !aL4 || !bL4 coexistence logic and the L4-vs-L4 listener
matching loop into focused helper methods. Keep mappingsOwnSameRuntime
responsible for classification and delegating to these helpers, preserving all
existing host, listener, fallback, network, and port matching behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a9a1714e-6cab-4d9a-8e31-24da0a0c62e2
⛔ Files ignored due to path filters (1)
shared/management/proto/proxy_service.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (49)
management/internals/modules/agentnetwork/synthesizer.gomanagement/internals/modules/agentnetwork/synthesizer_test.gomanagement/internals/modules/reverseproxy/proxy/proxy.gomanagement/internals/modules/reverseproxy/service/interface.gomanagement/internals/modules/reverseproxy/service/interface_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/modules/reverseproxy/service/multiport_test.gomanagement/internals/modules/reverseproxy/service/service.gomanagement/internals/modules/reverseproxy/service/service_test.gomanagement/internals/shared/grpc/expose_service.gomanagement/internals/shared/grpc/proxy.gomanagement/internals/shared/grpc/proxy_group_access_test.gomanagement/internals/shared/grpc/proxy_test.gomanagement/internals/shared/grpc/validate_session_test.gomanagement/server/http/handlers/proxy/auth_callback_integration_test.gomanagement/server/migration/reverse_proxy_port_mappings.gomanagement/server/migration/reverse_proxy_port_mappings_test.gomanagement/server/migration/reverse_proxy_shared_domains.gomanagement/server/migration/reverse_proxy_shared_domains_test.gomanagement/server/store/sql_store.gomanagement/server/store/sql_store_service_test.gomanagement/server/store/store.gomanagement/server/store/store_mock.gomanagement/server/types/account.gomanagement/server/types/account_proxy_multiport_test.goproxy/internal/acme/manager.goproxy/internal/acme/manager_test.goproxy/internal/auth/middleware.goproxy/internal/auth/middleware_test.goproxy/internal/auth/oidc.goproxy/internal/auth/oidc_test.goproxy/internal/conntrack/hijacked.goproxy/internal/conntrack/hijacked_test.goproxy/internal/netutil/host.goproxy/internal/netutil/host_test.goproxy/internal/proxy/reverseproxy_test.goproxy/internal/proxy/servicemapping.goproxy/internal/roundtrip/netbird.goproxy/internal/tcp/router.goproxy/internal/tcp/router_test.goproxy/management_integration_test.goproxy/process_mappings_bench_test.goproxy/server.goproxy/server_test.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.goshared/management/proto/proxy_service.proto
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@management/internals/modules/reverseproxy/service/service_api.go`:
- Around line 74-93: Update servicePortMappingsToAPI so response conversion does
not mutate the received Service. Replace the call to
PopulatePortMappingsFromLegacy on service with equivalent population into a
local mappings source or perform it on an owned copy, while preserving legacy
mapping conversion and nil-entry handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 48e69c2f-9e6a-457b-a2ce-5376bbeff8b5
📒 Files selected for processing (6)
management/internals/modules/agentnetwork/synthesizer.gomanagement/internals/modules/reverseproxy/service/manager/manager.gomanagement/internals/modules/reverseproxy/service/service.gomanagement/internals/modules/reverseproxy/service/service_api.gomanagement/server/store/sql_store.goproxy/server.go
🚧 Files skipped from review as they are similar to previous changes (3)
- management/internals/modules/agentnetwork/synthesizer.go
- management/internals/modules/reverseproxy/service/manager/manager.go
- management/server/store/sql_store.go
heywander
force-pushed
the
codex/multiport-pr
branch
from
July 21, 2026 17:16
20077a5 to
54ca8e1
Compare
|
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
This draft implements the multi-port L4 reverse-proxy design requested in #5821 and is being opened for maintainer confirmation of the API/model and hostname-conflict behavior.
Validation
Issue ticket number and link
Closes #5821
Stack
Checklist
The design discussion is continuing in #5821 and this draft. This checkbox will be updated after maintainer confirmation.
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
netbirdio/docs#867
Summary by CodeRabbit
New Features
port_mappings, plus capability reporting (supports_port_mappings) and HTTP-domain aware service lookup.Bug Fixes
Migration
port_mappings, and update ownership/overlap safety checks.