tls: yield downstream read retries to the event loop#12140
Conversation
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
📝 WalkthroughWalkthroughAdds shared retryable network I/O codes, updates TLS event-driven reads, preserves partial forward handshake data, and prevents multiple network handlers from treating retryable reads as closed connections. An integration test covers partial TLS records alongside another active connection. ChangesNetwork retry handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TLSClient
participant ForwardConnection
participant flb_tls_net_read
participant io_tls_event_switch
participant ForwardBuffer
TLSClient->>ForwardConnection: send partial TLS record
ForwardConnection->>flb_tls_net_read: read encrypted data
flb_tls_net_read->>io_tls_event_switch: register READ/WRITE interest
flb_tls_net_read-->>ForwardConnection: retry result
ForwardConnection->>ForwardBuffer: retain partial handshake data
TLSClient->>ForwardConnection: send complete message on second connection
ForwardConnection-->>TLSClient: process and forward message
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 091ef93a0c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| else if (ret == FLB_TLS_WANT_WRITE) { | ||
| if (event_driven) { | ||
| if (io_tls_event_switch(session, MK_EVENT_READ | MK_EVENT_WRITE) == -1) { |
There was a problem hiding this comment.
Preserve custom dispatch after TLS read wants write
When a downstream TLS read returns FLB_TLS_WANT_WRITE, this new path calls io_tls_event_switch(), which re-registers the connection as FLB_ENGINE_EV_THREAD; these event-driven input/http-server connections do not set connection->coroutine, and the engine/downstream worker dispatches handlers only for FLB_ENGINE_EV_CUSTOM. In the SSL_read WANT_WRITE case (e.g. post-handshake TLS work), the next readiness notification is therefore ignored and the connection stalls instead of resuming the plugin handler.
Useful? React with 👍 / 👎.
| ret = flb_io_net_read(conn->connection, | ||
| conn->buf + conn->buf_len, avail); |
There was a problem hiding this comment.
Bound secure-forward reads by the allocated buffer
With buffer_chunk_size configured below 1023 bytes, conn->buf is allocated to that smaller size in fw_conn_add(), but the secure-forward PING reader now reads up to the hard-coded size (1023) into that buffer. A normal secure-forward handshake larger than the configured chunk can therefore write past conn->buf; either grow the connection buffer first or cap avail by conn->buf_size - conn->buf_len.
Useful? React with 👍 / 👎.
Summary
Fixes #11551. (workaround for v5.0.x)
A downstream TLS connection can return
FLB_TLS_WANT_READafter consuming an incomplete TLS record. Forward and the other event-driven server consumers currently enter the synchronousflb_tls_net_read()retry loop, which prevents the event loop from servicing another connection and can consume 100% CPU.This change:
Approach
Registered downstream connections are already owned by an event loop, even when the plugin does not use the coroutine-based
FLB_IO_ASYNCpath. For those connections,flb_tls_net_read()now returnsFLB_IO_WANT_READorFLB_IO_WANT_WRITEafter updating event interest instead of retrying synchronously.Unregistered and upstream synchronous connections retain their existing behavior. Successful I/O remains the only operation that refreshes
net.io_timeout; a retryable result does not count as activity. Therefore:net.io_timeoutcaps an idle wait through the existing downstream timeout sweep;net.io_timeout: 0continues to mean no idle timeout; andRelationship to #11547
This is an event-driven alternative to #11547.
#11547 avoids the hot loop by sleeping 5 ms between retries and adds a private 60-second timeout to synchronous TLS reads and writes. That reduces CPU consumption, but it still blocks the owning event loop and changes the meaning of
net.io_timeout: 0.This PR fixes the reported downstream read path by yielding ownership to the existing event loop and preserving the configured networking timeout semantics. It intentionally does not add a separate hard-coded timeout.
Validation
Configured and built with:
The new regression timed out on unpatched
masterbecause the second TLS handshake could not be serviced.Final focused integration run:
Result: 3 passed.
Strict Valgrind run:
Result: 3 passed, strict Valgrind clean.
Focused runtime coverage for
in_http,in_mqtt,in_syslog, andin_tcppassed. The localflb-rt-in_forwardbinary could not bind its hard-coded port 24224 because the host'sfluentd.servicealready owned that port; the Forward integration regression above uses an isolated dynamic port and passed.The full PR commit range also passes
.github/scripts/commit_prefix_check.py.Summary by CodeRabbit