Skip to content

TLS: expose connection reset errors in SslSocket#44297

Closed
TAOXUY wants to merge 3 commits intoenvoyproxy:mainfrom
TAOXUY:exposeReset
Closed

TLS: expose connection reset errors in SslSocket#44297
TAOXUY wants to merge 3 commits intoenvoyproxy:mainfrom
TAOXUY:exposeReset

Conversation

@TAOXUY
Copy link
Copy Markdown
Contributor

@TAOXUY TAOXUY commented Apr 7, 2026

Commit Message: currently only plain text connection will expose the reset type in DOWNSTREAM|UPSTREAM_DETECTED_CLOSE_TYPE but tls connection won't. This PR exposes reset errors to sslSocket caller(connectionImpl) so that RESET can be exposed to DETECED_CLOSE_TYPE in stream_info metadata.
Risk Level: lo
Testing: added
Docs Changes: no needed
Release Notes: updated
Platform Specific Features: no

TAOXUY added 3 commits April 6, 2026 23:33
…test coverage

Signed-off-by: Xuyang Tao <taoxuy@google.com>
Signed-off-by: Xuyang Tao <taoxuy@google.com>
Signed-off-by: Xuyang Tao <taoxuy@google.com>
@TAOXUY
Copy link
Copy Markdown
Contributor Author

TAOXUY commented Apr 7, 2026

/retest

@TAOXUY
Copy link
Copy Markdown
Contributor Author

TAOXUY commented Apr 7, 2026

@botengyao

@botengyao botengyao self-assigned this Apr 7, 2026
@botengyao
Copy link
Copy Markdown
Member

cc @nyckone who is working on another PR #44149

@nyckone
Copy link
Copy Markdown
Contributor

nyckone commented Apr 12, 2026

@TAOXUY your approach seems like it's solving an issue I had:
My PR uses ERR_peek_error() to read the error from BoringSSL's queue, this covers both doRead and doWrite and has a runtime guard. However, it only detects RST when SSL_read returns -1 (SSL_ERROR_SYSCALL).
When RST arrives as EOF (SSL_read returns 0), the error queue is empty and my approach misses it - which is why I couldn't get an integration test to pass.

Your drainErrorQueue() fallback solves exactly that gap - ECONNRESET surfaces later when the connection writes (e.g., close_notify), and your detected_io_error_ captures it.

Your PR missing the following behavior from what I can see:
if ECONNRESET happens during a doWrite - it falls through to default → drainErrorQueue() but the error code isn't returned in the IoResult (doWrite returns {PostIoAction::Close, total_bytes_written, false} and no error code in the IoResult)

I would love to combine forces and use your PR if that's ok with you for the scenario I am missing - WDYT?

@TAOXUY
Copy link
Copy Markdown
Contributor Author

TAOXUY commented Apr 12, 2026

@TAOXUY your approach seems like it's solving an issue I had: My PR uses ERR_peek_error() to read the error from BoringSSL's queue, this covers both doRead and doWrite and has a runtime guard. However, it only detects RST when SSL_read returns -1 (SSL_ERROR_SYSCALL). When RST arrives as EOF (SSL_read returns 0), the error queue is empty and my approach misses it - which is why I couldn't get an integration test to pass.

Your drainErrorQueue() fallback solves exactly that gap - ECONNRESET surfaces later when the connection writes (e.g., close_notify), and your detected_io_error_ captures it.

Your PR missing the following behavior from what I can see: if ECONNRESET happens during a doWrite - it falls through to default → drainErrorQueue() but the error code isn't returned in the IoResult (doWrite returns {PostIoAction::Close, total_bytes_written, false} and no error code in the IoResult)

I would love to combine forces and use your PR if that's ok with you for the scenario I am missing - WDYT?

Of course, please go ahead! Remember also copy the integration test on *_DETECTED_CLOSE_TYPE as well.

@botengyao
Copy link
Copy Markdown
Member

#44149

Closing this to work on @nyckone's PR. Thanks @TAOXUY!

@botengyao botengyao closed this Apr 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants