topology_hiding: do not warn per request on a non-local send socket - #4120
topology_hiding: do not warn per request on a non-local send socket#4120Lt-Flash wants to merge 1 commit into
Conversation
|
A note on why the |
|
Thank you @Lt-Flash for the worker here. I still consider that the logs should stay as a warning, as it is an indicator if something in wrong with the traffic or the setup. |
|
Thanks for taking a look, @bogdan-iancu. On the socket tags question — good prompt, I went and checked both the code and our actual deployment. Our environment (3-node anycast SBC tier, sockets from The public/anycast leg never hits this bug in the first place - every node binds the identical literal IP, so the existing plain So I checked whether that tag is actually usable today, and it isn't - not because tags don't work, but because
Given we already tag our private sockets exactly the way this would need, wiring On the log level - I hear the concern, and agree it's a legitimate signal in cases where it indicates a real setup problem. The case this PR targets is specifically the routine one shown above: state legitimately created on one node, restored on another, as expected behavior in an anycast/LB tier - not a misconfiguration. In that topology it fires on every single sequential request of every such call, which is what made it unusable as a signal for us. Since the R-URI has already been correctly restored by this point and only the send socket falls back to the caller's choice, would you be open to something in between - e.g. rate-limiting the line, or only escalating to WARN after N occurrences for the same state - rather than either "always WARN" or "always DBG"? Open to whatever level best preserves the signal without the per-request flood. |
Both restore paths in the no-dialog decode logic (decode_info_buffer() and decode_info_buffer_legacy()) fell back to th_internal_trusted_tag when the encoded socket could not be resolved locally - but that tag governs one-way-hiding on trusted internal sockets, an unrelated concern. th_external_socket_tag is the tag actually documented (and already used correctly in topo_no_dlg_match_uri()) for resolving a socket that was encoded on a different node. The surrounding conditional was also inverted: the WARN fired whenever the first condition (no socket AND no tag) was false, which includes the case where the socket resolved directly with no problem at all, and stayed silent in the genuine "can't resolve, no tag configured" case. Restructured so the fallback is only attempted after a direct miss, and the log line only fires once both the direct lookup and the tag fallback have failed. That log line is also dropped from WARN to DBG: reaching this point is routine whenever topology hiding state reaches a node other than the one that created it (e.g. an anycast or load-balanced tier), not a sign of misconfiguration - it previously fired on every sequential request of every such call. The R-URI has already been restored at this point, so the request still routes; only the send socket falls back to the caller's own choice.
bcd40af to
db5b283
Compare
|
Update, since a lot happened to this file since I opened this PR - the underlying code moved into On the tags question specifically: someone already added a socket-tag fallback here independently ( The wrong-variable bug also broke the log condition itself: the I've rewritten both sites to: try the tag fallback only after a direct miss, and only log once neither resolves it - which is also where I kept the DBG (down from WARN) for the reasons in my last comment. Rebased onto current master and force-pushed since the file changed too much for a normal rebase to apply cleanly. Still very open to whatever log level you think is right for the "genuinely couldn't resolve it" case now that it's gated correctly - happy to bump it back to WARN, or the rate-limit idea, whatever you'd prefer. |
What
Two problems in
topo_no_dlg_seq_handling(), in the branch that restores the send socket recorded in the encoded topology-hiding state: a log line that fires on every sequential request in any multi-node deployment, and — in the same branch — the unusable socket being assigned anyway, clearing whatever the script had already chosen.1. A WARN per sequential request
When dialog-less topology hiding decodes the state of a sequential request, it looks up the socket the state was created on:
A node not owning that socket is routine, not exceptional: any anycast or load-balanced tier can land a sequential request on a different node than the one that handled the initial request, and a node whose listening set has simply changed hits it too. The result is one WARN per sequential request, per call — on a busy presence deployment that is the bulk of the log.
Dropped to
LM_DBG. The R-URI has already been restored by this point, so the request still routes; only the send socket is affected.2. The failed lookup was not actually ignored
The message says "ignoring", but the assignment sat outside the check:
So on the failure path it stored
NULL— wiping any socket the script had already selected (e.g. an earlierforce_send_socket()in the route) rather than leaving it alone. The fix only assigns when a local socket was actually found:Impact
Log volume on multi-node topology-hiding deployments, and — for scripts that set a send socket before
topology_hiding_match()— a silently discarded choice on any request whose state came from another node.