ConnectTransportException returns retryable BAD_GATEWAY

[DiannaHohensee]

I took a spin through the code with new ConnectTransportException, and saw this error case. That one doesn't seem like it should be a retryable error 🤔 Is this status code change safe, or maybe we should change that code to a different exception type?

Related ES-10214
Closes #118320

[elasticsearchmachine]

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

[elasticsearchmachine]

Hi @DiannaHohensee, I've created a changelog YAML for you.

[mhl-b]

I think that should be IllegalStateException, because we already have remote connection to another cluster.

Why it needs REST status attached? Do we send it back to the REST handler and client?

[DiannaHohensee]

Thanks for taking a look and confirming it seems strange, @mhl-b. I'll take a closer look at Proxy* -- I'm not familiar with it, atm.

I forgot to reference the ticket, but this relates to ES-10214 -- I just updated my description to include it. I don't myself know how the HTTP error code propagates back to the caller, but other folks believe that it does. I would think it could reasonably end up in a stack trace of 'caused by' errors, if not directly.

[DiannaHohensee]

Hmm. I think IllegalStateException would indicate ES has done something wrong. Whereas ES looks like it's throwing the ConnectTransportException when the information from the user doesn't match up with reality: like setting an address to a cluster and a cluster name, and then ES verifies that the connected to cluster's name matches, and it doesn't. I think we need a "user gave us a bad parameter" non-retryable exception?

There is some more code throwing the ConnectTransportException, and I think the idea is that we connected successfully to some server, but the type of server is not what ES expected to find.

[DiannaHohensee]

After reviewing the ConnectTransportException uses, I think we need a new exception type, something like InvalidRemoteNodeException, to replace the ConnectTransportException use cases where the node connection is successful but the node type is unexpected. It doesn't seem like those error cases should be retryable.

@DaveCTurner for thoughts.

[DaveCTurner]

My reading of the HTTP spec is that we are indeed acting as a gateway in this situation:

A "gateway" (a.k.a. "reverse proxy") is an intermediary that acts as an origin server for the outbound connection but translates received requests and forwards them inbound to another server or servers [...] A gateway communicates with inbound servers using any protocol that it desires [...]

Moreover, if the remote node turns out to belong to a cluster with an unexpected name then 502 Bad Gateway is a valid response code:

The 502 (Bad Gateway) status code indicates that the server, while acting as a gateway or proxy, received an invalid response from an inbound server it accessed while attempting to fulfill the request.

There's other ways that a bad in-cluster config might lead to a ConnectTransportException or a subclass, and indeed I can think of ways that this specific failure might go away on a retry. In any case, the effect on client retries is not really relevant here, at least not as much as following the HTTP spec. The whole notion of triggering client behaviour based solely on the HTTP response code is fundamentally doomed anyway, there's just too many different outcomes to map onto the very limited options available to us in the HTTP spec.

It's definitely not a client-side failure so we have to return some kind of 5xx error, and 502 seems more appropriate than 500. If clients choose to retry on all 502s then 🤷 that's not wrong I guess.

[mhl-b]

Would be nice to have an integ or yaml/rest test to show rest status propagation. I dont think status is very important, 500 or 502, since we model REST over HTTP and there is no strict requirement to confine to error codes, as David said there are not enough of them. So client still need to unwrap 5xx HTTP response to make decision, including retry.

[DaveCTurner]

LGTM except a couple of nits

[elasticsearchmachine]

Hi @DiannaHohensee, I've updated the changelog YAML for you.

[DiannaHohensee]

Would be nice to have an integ or yaml/rest test to show rest status propagation. I dont think status is very important, 500 or 502, since we model REST over HTTP and there is no strict requirement to confine to error codes, as David said there are not enough of them. So client still need to unwrap 5xx HTTP response to make decision, including retry.

In the related SDHE, Kibana was seeing message: "org.elasticsearch.transport.NodeDisconnectedException" and message: INTERNAL_SERVER_ERROR as the symptom. I suspect that's from the generic TransportService#onConnectionClosed method. I think it would be difficult to test this behavior end-to-end: I'm not aware of a related test suite where we set up ES servers and check HTTP codes. I think it would be more expedient to file a ticket to do that work, if we wanted it.

[DiannaHohensee]

The test failures match #118846, so they are not a blocker.

[pawankartik-elastic]

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Questions ?

Please refer to the Backport tool documentation