http conn man: fix x-envoy-internal regression #2232
Conversation
This fixes a regression from #2217. In that PR, we refactored calculation of downstream address to use real addresses and not strings. As part of that change, I broke some behavior that people were depending on. Primarily that x-envoy-internal is based on: 1) XFF is present (either via use_remote_address or in request headers). 2) XFF contains a single address after any appending from use_remote_address. 3) The address is valid. In all other cases the request is not considered internal. The above algorithm is IMO not optimal in many ways, but it's how the code has worked for a long time and Lyft (and possibly) others are depending on it, so we can't change it. This change restores the old behavior. It reverts changes to integration tests for checking for bytes, as well as does a substantial cleanup to the connection manager utility tests since they were a mess. It also adds some new tests to specifically cover certain cases. In the future I think we will want to add additional inference modes for determining internal requests, but for now, we need to keep the old behavior. Signed-off-by: Matt Klein <[email protected]>
| @@ -1011,7 +1011,7 @@ void HttpIntegrationTest::testUpstreamProtocolError() { | |||
| FakeRawConnectionPtr fake_upstream_connection = fake_upstreams_[0]->waitForRawConnection(); | |||
| // TODO(mattklein123): Waiting for exact amount of data is a hack. This needs to | |||
| // be fixed. | |||
| fake_upstream_connection->waitForData(211); | |||
| fake_upstream_connection->waitForData(187); | |||
There was a problem hiding this comment.
revert from previous PR
| @@ -204,10 +204,10 @@ TEST_P(IntegrationTest, WebSocketConnectionDownstreamDisconnect) { | |||
| tcp_client = makeTcpConnection(lookupPort("http")); | |||
| // Send websocket upgrade request | |||
| // The request path gets rewritten from /websocket/test to /websocket. | |||
| // The size of headers received by the destination is 252 bytes. | |||
There was a problem hiding this comment.
All reverts from previous PR.
| if (final_remote_address == nullptr) { | ||
| final_remote_address = connection.remoteAddress(); | ||
| } | ||
| // If we find one, use it. |
There was a problem hiding this comment.
Not technically correct as we may chose to not use it below. Update please?
| // 1) After remote address/XFF appending, the XFF header must contain a *single* address. | ||
| // 2) The single address must be an internal address. | ||
| // 3) If configured to not use remote address, but no XFF header is available, even if the real | ||
| // remote is internal, the request is considered external. |
There was a problem hiding this comment.
Any docs which should be updated?
There was a problem hiding this comment.
I will check the docs. I think this is covered but if not I will clarify.
There was a problem hiding this comment.
I think these docs cover it: https://www.envoyproxy.io/docs/envoy/latest/configuration/http_conn_man/headers#x-forwarded-for
LMK if not.
| bool internal_; | ||
| }; | ||
|
|
||
| MutateRequestRet callMutateRequestHeaders(HeaderMap& headers, Protocol protocol) { |
There was a problem hiding this comment.
I think this results in harder to read tests than before - checking IP on return and checking the headers after return seems cleaner.
If you think it's worth the decrease in readability to force "internal" checking (reasonable, since it's scary fragile code) please add a comment.
There was a problem hiding this comment.
I did it to force it as you speculated, but if you think it makes it harder to read, I can revert to the old way. Do you feel strongly about it?
There was a problem hiding this comment.
Nope, hence a comment being fine. As is I could imagine cleaning it up "to make it easier to read" and losing the forcing function.
Follow up to #2232. Signed-off-by: Matt Klein <[email protected]>
Follow up to #2232. Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: GitHub Action <[email protected]> Co-authored-by: jpsim <[email protected]> Signed-off-by: JP Simard <[email protected]>
Signed-off-by: GitHub Action <[email protected]> Co-authored-by: jpsim <[email protected]> Signed-off-by: JP Simard <[email protected]>
This fixes a regression from #2217.
In that PR, we refactored calculation of downstream address to use real
addresses and not strings. As part of that change, I broke some behavior
that people were depending on. Primarily that x-envoy-internal is based on:
In all other cases the request is not considered internal. The above algorithm
is IMO not optimal in many ways, but it's how the code has worked for a long
time and Lyft (and possibly) others are depending on it, so we can't change it.
This change restores the old behavior. It reverts changes to integration tests
for checking for bytes, as well as does a substantial cleanup to the connection
manager utility tests since they were a mess. It also adds some new tests to
specifically cover certain cases.
In the future I think we will want to add additional inference modes for
determining internal requests, but for now, we need to keep the old behavior.
Risk Level: Medium (Hopefully not more broken than before, but still risky change)
Testing: Unit/integration (note reverted integration tests from previous commit which
actually caught this issue.
Documentation: N/A
Release notes: N/A