TCP connection: detect and send RST

[botengyao]

In summary
Related issue #2703
This PR will add the support to detect RST from the peer and send RST by Envoy controlled by flag.

This PR added enableTcpRstDetectAndSend method to the connection object, and it is disabled by default. The filter or application can enable it by calling enableTcpRstDetectAndSend(true) to enable it. This is similar for the usage of enableHalfClose(). In the meantime, this feature will also be controlled by the runtime guard envoy_reloadable_features_detect_and_raise_rst_tcp_connection.

Detect RST:
The error code can be got from io_handle (through sys call) read or write and then is propagated back to the transport socket through IoResult. e.g., IoResult RawBufferSocket::doRead(..) will return {action, bytes_written, false, err};, and then the connection layer will read the err code and do some job based on it.

• Posix socket errors:
  • ECONNRESET 104 (hexadecimal 68)
• Windows
  • WSAECONNRESET 10054 (hexadecimal 2746)

In this PR I am mainly focusing on linux system.

We translated them to Envoy internal error: SOCKET_ERROR_CONNRESET

This PR only added the support to raw_buffer_socket, we need to make changes to other transport socket extensions.

Send RST:

struct linger linger;
  linger.l_onoff = 1;
  linger.l_linger = 0;
absl::string_view linger_bstr{reinterpret_cast<const char*>(&linger), sizeof(struct linger)};
ENVOY_MAKE_SOCKET_OPTION_NAME(SOL_SOCKET, SO_LINGER), linger_bstr

Discussion:

Event handling: to achieve that Envoy will close the upstream or downstream by sending RST when a RST is received from the peer, we need to find a way to extend the notification mechanism. Right now we are using Network::ConnectionEvent enum class, and the current PR will add 2 more events: LocalReset and RemoteReset.

Right now a lot of filters or extensions are only using LocalClose and RemoteClose to handle the close, e.g.,

void Filter::onEvent(Network::ConnectionEvent event) {
  if (event == Network::ConnectionEvent::LocalClose ||
      event == Network::ConnectionEvent::RemoteClose) {
    closed_ = true;
  }

If we reach a consensus that this is a good way to go, I will kick off the clean up plan for this to correctly handle the events for all the callback functions.

Another option is to use the combined event by leveraging bitfield like we can combine the LocalClose and LocalReset together. We actually converted from this bitfield way to the enum way 5 years ago: #1358.

Future Plan
Right now I only verified the sending and detection through an async tcp client with raw_buffer socket. In order to incrementally adopt this feature, further plan includes:

1. TcpProxy support for RST detection and actively send it to peer, integration tests and so on.
2. HttpConnectionManager changes to handle it for L7.
3. More transport socket adoption for the system error code. And I think the next big one is tls_transport_socket which is using boringSSL. The current state for that is we only handle all errors as SYSCALL_ERROR, and we need to distinguish the errno.

Let me know if this sounds reasonable, and thank you all for the review!

Commit Message:
Additional Description:
Risk Level: Medium
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #28817 was opened by botengyao.

see: more, trace.

[repokitteh-read-only]

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #28817 was opened by botengyao.

see: more, trace.

[botengyao]

cc @yanavlasov

[ggreenway]

This is cool! A few questions, concerns, or things to document:

• Does this work consistently across platforms?
• ECONNRESET (at least on linux) depends on the connected-state of the connection. Eg, there are different errors returned if the connection is only half-open I think. I think this is fine; we'll just want to document the behavior, or at least make a note in general about it without documenting all the specifics.
• Will you try to do things like proxy back-and-forth between TCP-RST for plaintext and SSL-Alert for TLS? And similar for other transport-sockets.

Another option is to use the combined event by leveraging bitfield like we can combine the LocalClose and LocalReset together.

No, I think it's better how you have it how.

[botengyao]

Hi @ggreenway, thanks for the comment!

Does this work consistently across platforms?

I think it supports both windows and POSIX systems with different error codes like

• Posix socket errors:
  • ECONNRESET 104 (hexadecimal 68)
• Windows
  • WSAECONNRESET 10054 (hexadecimal 2746)

ECONNRESET (at least on linux) depends on the connected-state of the connection. Eg, there are different errors returned if the connection is only half-open I think. I think this is fine; we'll just want to document the behavior, or at least make a note in general about it without documenting all the specifics.

This is a good point, and I am still looking into the UNIX networks for the RST behavior on half-closed connection to learn more. My take right now is it needs at least one additional write or read operations to know the errors from socket on half-closed socket. It could have other errors types, and do you have any reference for this?

Will you try to do things like proxy back-and-forth between TCP-RST for plaintext and SSL-Alert for TLS? And similar for other transport-sockets.

Yes, but I am thinking like an incremental adoption will be reasonable.

1. My plan is to make the basic sending and detection work for RST at first, and right now it only supports raw_buffer_socket.

2. Next item is to make TCPProxy proxy RST back-and-forth for plaintext. And maybe for HTTPConnectionManager after TCPProxy.

3. Then the next big thing is detection part of tls_socket which is using boring SSL. For Linux, we can get errno per thread whenever we get borring SSL SSL_ERROR_SYSCALL. This works for linux/pthread platform if we immediately read the errno from local thread. errno(3), errno is thread-local; setting it in one thread does not affect its value in any other thread.. It probably deserves another PR.

Regarding the event handling, we don't distinguish close type before, but right now we will have either RemoteClose or RemoteReset, we need to make sure filters and extensions correctly handle them so that no event is leaked.

[botengyao]

The CI eventually is green for this commit version and only CodeQL is pending. Adding more tests...

[botengyao]

/retest

[yanavlasov]

LGTM form me with a small nit.

Pinging @ggreenway for non Google review.

/wait-any

[ggreenway]

A couple nits, but LGTM overall

/wait

[botengyao]

/retest

[botengyao]

/retest

[howardjohn]

fyi @botengyao this broke us in Istio. I will try out #29616 to see if it fixes it

[howardjohn]

I believe this does, will know for sure in a few days when it rolls through our pipelines