[reverse-proxy] Add listener side proxy protocol support and enable it in traefik#5332
[reverse-proxy] Add listener side proxy protocol support and enable it in traefik#5332pascal-fischer merged 2 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional PROXY protocol support (flag/env + Server field), listener wrapper and trusted-proxies policy, health/debug startup refactors, unit tests for PROXY behavior, a go-proxyproto dependency, and centralizes Traefik IP handling in infra scripts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Traefik as "L4 Proxy (Traefik)"
participant Listener as "TCP Listener"
participant Wrapper as "PROXY Wrapper"
participant Policy as "TrustedProxies Policy"
participant TLS as "TLS Server"
Client->>Traefik: TCP connect
Traefik->>Listener: Forward connection with PROXY v2 header
Listener->>Wrapper: Accept connection
Wrapper->>Policy: Evaluate source addr vs TrustedProxies
alt Policy = REQUIRE
Policy-->>Wrapper: REQUIRE
Wrapper->>Wrapper: Parse PROXY v2 header, set RemoteAddr (client IP)
Wrapper->>TLS: Pass connection (client IP preserved)
else Policy = IGNORE
Policy-->>Wrapper: IGNORE
Wrapper->>TLS: Pass connection unchanged
else Policy = REJECT
Policy-->>Wrapper: REJECT
Wrapper->>Listener: Close / reject connection
end
TLS->>Client: Proceed with TLS handshake
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@go.mod`:
- Line 250: The go.mod lists github.com/pires/go-proxyproto as indirect while
proxy/server.go imports it directly; update go.mod to make
github.com/pires/go-proxyproto a direct dependency (or run module cleanup) so CI
recognizes it as required: add the dependency to the main require block at
v0.11.0 (remove it from the indirect section) or run go module tidy/resolve so
the direct import in proxy/server.go is reflected in go.mod.
In `@proxy/proxyprotocol_test.go`:
- Line 96: Update the assertion message string to fix the typo: change
"unparseable address should be rejected" to "unparsable address should be
rejected" in the test where assert.Equal(t, proxyproto.REJECT, policy, ...) is
called (the assertion referencing variable policy and using assert.Equal) so the
message uses the correct spelling "unparsable".
🧹 Nitpick comments (1)
proxy/server.go (1)
267-280: Consider warning whenProxyProtocolis enabled withoutTrustedProxies.When
TrustedProxiesis empty, no policy is set on the listener, sogo-proxyprotodefaults to accepting PROXY headers from any source. This allows untrusted clients to spoof their source IP. While this may be intentional for simple setups, a log warning would help operators notice the misconfiguration.Suggested addition
func (s *Server) wrapProxyProtocol(ln net.Listener) net.Listener { ppListener := &proxyproto.Listener{ Listener: ln, ReadHeaderTimeout: 5 * time.Second, } if len(s.TrustedProxies) > 0 { ppListener.Policy = s.proxyProtocolPolicy + } else { + s.Logger.Warn("PROXY protocol enabled without trusted proxies; any source may send PROXY headers") } s.Logger.Info("PROXY protocol enabled on listener") return ppListener }
lixmal
force-pushed
the
reverse-proxt-proxy-protocol
branch
2 times, most recently
from
2f72544 to
cf99f22
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@proxy/server.go`:
- Around line 267-280: The code currently enables proxyproto.Listener in
wrapProxyProtocol which defaults to trusting all senders if s.TrustedProxies is
empty; enforce a safety check in ListenAndServe before calling
wrapProxyProtocol: if Proxy protocol is enabled and len(s.TrustedProxies) == 0
then return an error (or log fatal) requiring TrustedProxies to be configured
(fail startup) so proxyproto.Listener is never created in the insecure default
mode; reference s.wrapProxyProtocol, s.TrustedProxies, ListenAndServe and
proxyproto.Listener when making the change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@infrastructure_files/getting-started.sh`:
- Around line 588-589: The label
traefik.tcp.services.proxy-tls.loadbalancer.proxyProtocol.version=2 is invalid
in Traefik v3.6; instead create a TCP ServersTransport dynamic resource (e.g.,
in dynamic.yaml) that sets proxyProtocol.version: 2 and then replace the invalid
label with the service reference
traefik.tcp.services.proxy-tls.loadbalancer.serverstransport=<transport-name>@<provider>
so the proxy protocol v2 is applied via the ServersTransport.
lixmal
force-pushed
the
reverse-proxt-proxy-protocol
branch
from
cf99f22 to
bbb89f5
Compare
lixmal
changed the title
# Conflicts: # infrastructure_files/getting-started.sh
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
infrastructure_files/getting-started.sh (2)
766-768:trustedHTTPProxiesis rendered for all reverse proxy types, but only applies to built-in Traefik.
render_combined_yamlis called for everyREVERSE_PROXY_TYPE, so non-Traefik setups (types 2–5) and external-Traefik setups (type 1, where Traefik has a different IP) will get atrustedHTTPProxiesentry pointing to172.30.0.10, which is meaningless or incorrect in those contexts. Consider conditionally emitting this block only whenREVERSE_PROXY_TYPE == "0".Proposed fix
+ local reverse_proxy_config="" + if [[ "$REVERSE_PROXY_TYPE" == "0" ]]; then + reverse_proxy_config=" + reverseProxy: + trustedHTTPProxies: + - \"$TRAEFIK_IP/32\"" + fi + cat <<EOF ... - reverseProxy: - trustedHTTPProxies: - - "$TRAEFIK_IP/32" - +${reverse_proxy_config} store:
646-656: Blank lines emitted in docker-compose when proxy is disabled — cosmetic only.When
ENABLE_PROXYis false,$traefik_file_provider(line 649) and$traefik_dynamic_volume(line 656) expand to empty strings, leaving blank lines in thecommand:andvolumes:blocks. This is valid YAML and won't cause issues — just noting for awareness.
3 participants
Describe your changes
Fixes #5324
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Tests
Chores