fix: resolve socket settings across listeners sharing an address and port - #9673
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
7e5fec2 to
a615cf9
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e5fec26fe
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if settings.maxAcceptPerSocketEvent == nil { | ||
| settings.maxAcceptPerSocketEvent = connection.MaxAcceptPerSocketEvent |
There was a problem hiding this comment.
With Gateway API input, ClientConnection.MaxAcceptPerSocketEvent is defaulted to 1 whenever a policy contains any spec.connection (api/v1alpha1/connection_types.go:46), and buildConnection copies any non-nil value into the IR. In a shared-port socket where an earlier listener sets only connection.bufferLimit and a later listener explicitly sets maxAcceptPerSocketEvent: 64, this merge records the defaulted 1 at the earlier listener and the later explicit value can never win, so the real Gateway API path still emits the default max accept that this PR is trying to stop from shadowing configured settings.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, and it is real — I checked the generated CRD and maxAcceptPerSocketEvent does carry default: 1 (charts/gateway-crds-helm/templates/generated/gateway.envoyproxy.io_clienttrafficpolicies.yaml:208). So any ClientTrafficPolicy with a connection block reaches the IR with MaxAcceptPerSocketEvent = 1 even when the user never wrote it, and buildConnection copies it through.
Two things are tangled here, though.
The case this PR is for — a listener with no ClientTrafficPolicy at all — is unaffected: buildConnection(nil) returns nil, so nothing is contributed and the configured listener wins. That is the case @zhaohuabing asked to fix unconditionally.
Your case is a listener whose policy sets only bufferLimit. From the IR that is indistinguishable from someone writing maxAcceptPerSocketEvent: 1 on purpose, because the defaulting happens in the API server before the controller ever sees the object. Under the rule we agreed on (when two listeners both set a field, the first wins and the status says so) it is technically correct — but it is a bad shape, since the user never typed that 1.
The clean fix is to drop +kubebuilder:default=1 from ClientConnection.MaxAcceptPerSocketEvent. buildMaxAcceptPerSocketEvent already returns 1 for nil and is the only consumer of the field, so the emitted xDS would be byte-identical while the IR regains the unset-vs-set distinction. That is a change under /api though, which the contributing guide wants agreed before implementation.
@zhaohuabing — happy to do it either way: fold it into this PR, or send it as its own small API PR and rebase this one on top. Which do you prefer?
| @@ -0,0 +1 @@ | |||
| Fixed Gateway listeners that share an address and port silently losing their client connection settings. Those listeners collapse into a single xDS listener, and the TCP keepalive, connection buffer limit and max accept per socket event were taken from whichever listener happened to be translated first, so a listener without a ClientTrafficPolicy would replace the values configured on another listener on the same socket with the hardcoded defaults. These settings are now resolved across every listener on the socket, and a listener that leaves one unset no longer overrides a listener that sets it. | |||
There was a problem hiding this comment.
This fragment is under bug_fixes/, but the change intentionally modifies generated xDS listener content for existing configurations, such as perConnectionBufferLimitBytes, maxConnectionsToAcceptPerSocketEvent, and socketOptions on shared listeners. EnvoyPatchPolicies and extension servers can target those fields, so this needs a breaking-change fragment as well; otherwise users relying on patches or hooks will not see the compatibility warning in the generated release notes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I looked at how breaking_changes/ has been used and I don't think this one qualifies, but I did make the note explicit — please tell me if you disagree.
The v1.8.3 breaking entry is the Lua one, where the filter names and config layout moved, so patches keyed on envoy.filters.http.lua/<index> stopped matching. Here nothing structural moves: the same three fields stay on the same listener, only their values change on sockets that were previously being handed a default. A JSON patch or extension hook pointing at per_connection_buffer_limit_bytes matches exactly as before.
It is also the change @zhaohuabing explicitly asked to make unconditionally, in contrast with rejecting the config, which he did call out as the breaking option.
That said, the value change is observable, so I've spelled it out in the bug-fix fragment rather than leaving it implied — it now names the three fields and says the names and layout are unchanged. Happy to add a breaking_changes/ fragment too if a maintainer reads it the other way.
a615cf9 to
b49afd4
Compare
|
I took a quick peek and one issue (besides that obvious 100% AI copy-paste) I saw, is that this PR is specific to socket settings but it should be about any setting that is listener specific. Maybe @zhaohuabing has a different opinion / view on this matter but we need to make sure that all settings from https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/listener/v3/listener.proto#config-listener-v3-listener that are accounted for. EG probably does not expose all of them but we need to find a way to make sure that the behaviour is the same for all of them. |
|
Fair point on the scope. Here is what I found when I went through the Listener proto against what the translator actually writes. EG only sets five things at the listener level today:
The first three are what this PR covers. The rest of the proto is not plumbed at all right now: That leaves the two listener filters, and they both break in a different way than the value fields: proxy_protocol. tls_inspector fingerprints. Same early-return shape, and I do not think this one has been reported anywhere. On So there is a real gap, but it is two listener filters rather than a long tail of settings, and they need different handling and different status wording than the value fields. @zhaohuabing happy to go either way: widen this PR to cover the filters as well, or land the three value fields here and do the filters as their own PR. Which do you prefer? |
|
Thanks for taking a look, and sorry — that is fair feedback. AI does speed my work up a lot, but you are right that it shows in the writing. I will be more careful with it from here and keep these in my own words. |
Maybe my comment about AI sounded harsher than intended. What I meant is that since the PR description, PR comments and proposed changes in the PR sounded all like AI, it contains a lot of information that should be validated and can be tiresome to read (wall of text). |
|
No worries, that is a fair read. I will keep them short. |
b49afd4 to
7935a7c
Compare
|
@guydc thanks, pushed the doc work you asked for. The four affected fields — I also folded The filter chain case @HusseinKabbout raised is a layer down and is still open; I left it on #9652 rather than growing this PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9673 +/- ##
==========================================
+ Coverage 76.22% 76.27% +0.04%
==========================================
Files 261 261
Lines 44146 44228 +82
==========================================
+ Hits 33650 33733 +83
+ Misses 8266 8265 -1
Partials 2230 2230 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/retest |
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Signed-off-by hint: the field names and layout do not change, only the values on the sockets that were previously getting a default. Signed-off-by: kadircanyildirm-crypto <kadir.can.yildirm@gmail.com>
Resolve the connection inspection timeout across listeners on the same socket as well, and call out in the API docs and the ClientTrafficPolicy task page that these four fields belong to the shared listener socket. Signed-off-by: Kadir Can Yildirim <252162627+kadircanyildirm-crypto@users.noreply.github.com>
Signed-off-by: Kadir Can Yildirim <252162627+kadircanyildirm-crypto@users.noreply.github.com>
main no longer emits initialFetchTimeout: 0s in the RDS config source, so the expected listeners output for the shared socket settings test case had to be regenerated. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B4riP5modRF9PLRwTWKeUN Signed-off-by: Kadir Can Yildirim <252162627+kadircanyildirm-crypto@users.noreply.github.com>
7935a7c to
4cc118f
Compare
3 participants
What this PR does / why we need it:
Listeners that share an address and port collapse into a single xDS listener, and
buildXdsTCPListeneronly runs for the first of them. It is the only place that writessocket_options,per_connection_buffer_limit_bytesandmax_connections_to_accept_per_socket_event, and nothing patches them afterwards, so the shared socket simply keeps whatever the first listener happened to carry.What turns that from an ordering quirk into a bug is the fallback:
buildPerConnectionBufferLimitBytesandbuildMaxAcceptPerSocketEventreturn 32768 and 1 when a listener has noConnectionat all. So a listener with no ClientTrafficPolicy attached doesn't just win a race — it actively replaces a value another listener on the same socket had configured with a default nobody asked for.This is the first of the two changes @zhaohuabing asked for in #9652 (comment) — fix the default overwrite unconditionally, no status involved. The status message for the case where two listeners both set a value explicitly is a separate follow-up.
How it works
buildSocketSettingswalks the HTTP and TCP listeners once, in translation order, and resolves the three settings per address and port before any listener is built. Each field is resolved on its own, so a listener that only setsbufferLimitdoesn't stop another one from contributingmaxAcceptPerSocketEvent. When more than one listener sets the same field the first one still wins, which keeps today's behaviour for configurations that were already explicit — the only sockets whose output changes are the ones that were silently getting a default.HTTP and TCP listeners are walked together because they really can land on the same socket:
getProtocolForListenermaps both HTTPS and TLS passthrough tohttps/tlson purpose so they can share a port, but HTTPS ends up inxdsIR.HTTPand passthrough inxdsIR.TCP.connection.limitis deliberately left out. It is a network filter and stays per filter chain, so it is genuinely unaffected by the collapse.Testing
New golden case
multiple-listeners-same-port-socket-settings: three listeners on port 10080, where the first sets nothing, the second sets the keepalive and the buffer limit, and the TCP passthrough listener sets the max accept. The socket now gets1048576/64/ the keepalive socket options instead of32768/1/ none.All 273 existing
xds-irgolden files are unchanged, which is the point — this only moves the cases that were previously losing their configuration.Which issue(s) this PR fixes:
Part of #9652
PR Checklist
git commit -s). See DCO: Sign your work./api.golangci-lint,go vetandgofmtare clean, and./internal/xds/...,./internal/gatewayapi/...and./internal/cmd/egctl/...pass.release-notes/current/bug_fixes/9652-shared-socket-listener-settings.md.