Documented a common integration tests problem#12933
Documented a common integration tests problem#12933alyssawilk merged 3 commits intoenvoyproxy:masterfrom
Conversation
…s not match the actual body length Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
cool, thanks for adding more debugging breadcrumbs!
test/integration/README.md
Outdated
| In addition to the existing test framework, which allows for carefully timed interaction and ordering of events between downstream, Envoy, and Upstream, there is now an “autonomous” framework which simplifies the common case where the timing is not essential (or bidirectional streaming is desired). When AutonomousUpstream is used, by setting `autonomous_upstream_ = true` before `initialize()`, upstream will by default create AutonomousHttpConnections for each incoming connection and AutonomousStreams for each incoming stream. By default, the streams will respond to each complete request with “200 OK” and 10 bytes of payload, but this behavior can be altered by setting various request headers, as documented in [`autonomous_upstream.h`](autonomous_upstream.h) | ||
|
|
||
| ## Common Problems | ||
| - If a response body length does not match the `content-length` header (or there is no header at all), any mock calls to wait for the response completion such as `sendRequestAndWaitForResponse` or `response_->waitForEndStream` could timeout. Also, any asserts that the response was completed such as `EXPECT_TRUE(response_->complete())` would fail. Make sure that the response body length matches the `content-length` header. |
There was a problem hiding this comment.
I think this could use a tweak - most requests are "sent" without a content length header, but helper functions add them, for example sendRequestAndWaitForResponse will send an end stream bit, indicating if the response is done (and sometimes causing content length to be added)
I think if you remove "(or there is no header at all)" it'd be more accurate.
There was a problem hiding this comment.
also timeout -> time out
There was a problem hiding this comment.
I faced this problem when I was sending a request using makeHeaderOnlyRequest and encoding the response using upstream_request_->encodeXXX(). Even when I set end_stream, for e.g. by using upstream_request_->encodeData(_, true), the call for response->waitForEndStream times out. This happened because I did not include a content-length to the encoded headers at all. That's why I added the "no header at all" part.
I guess whether the content-length header is added or not depends on the protocol, so it should not be something we depend on. I know this because when I was missing the content-length header, the integration test passed for some protocols and timed out for others.
I'd say its better for whoever is writing the test to add a correct content-length header, even if some functions/protocols would implicitly add it anyway. That's why I also chose the wording "it could time out" rather than "it will time out".
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
|
Can you share the code of your previously failing test with me?
I know mismatched content lengths will cause a problem, but if a response
is sent with no content length, Envoy will chunk encode with a terminal
chunk or use the end_stream bit for HTTP/2. I don't know what went wrong
with your test case but I literally just threw together an integration test
with no response content and it worked fine, so maybe if I understand the
error you encountered I can help you document exactly what the failure mode
is?
…On Wed, Sep 2, 2020 at 2:28 PM Yosry Ahmed ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In test/integration/README.md
<#12933 (comment)>:
> @@ -99,6 +99,9 @@ cluster:
In addition to the existing test framework, which allows for carefully timed interaction and ordering of events between downstream, Envoy, and Upstream, there is now an “autonomous” framework which simplifies the common case where the timing is not essential (or bidirectional streaming is desired). When AutonomousUpstream is used, by setting `autonomous_upstream_ = true` before `initialize()`, upstream will by default create AutonomousHttpConnections for each incoming connection and AutonomousStreams for each incoming stream. By default, the streams will respond to each complete request with “200 OK” and 10 bytes of payload, but this behavior can be altered by setting various request headers, as documented in [`autonomous_upstream.h`](autonomous_upstream.h)
+## Common Problems
+- If a response body length does not match the `content-length` header (or there is no header at all), any mock calls to wait for the response completion such as `sendRequestAndWaitForResponse` or `response_->waitForEndStream` could timeout. Also, any asserts that the response was completed such as `EXPECT_TRUE(response_->complete())` would fail. Make sure that the response body length matches the `content-length` header.
I faced this problem when I was sending a request using
makeHeaderOnlyRequest and encoding the response using
upstream_request_->encodeXXX(). Even when I set end_stream, for e.g. by
using upstream_request_->encodeData(_, true), the call for
response->waitForEndStream times out. This happened because I did not
include a content-length to the encoded headers at all. That's why I
added the "no header at all" part.
I guess whether the content-length header is added or not depends on the
protocol, so it should not be something we depend on. I know this because
when I was missing the content-length header, the integration test passed
for some protocols and timed out for others.
I'd say its better for whoever is writing the test to add a correct
content-length header, even if some functions/protocols would implicitly
add it anyway. That's why I also chose the wording "it could time out"
rather than "it will time out".
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#12933 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AELALPJFAZVZG2DQKKZPXNLSD2FEFANCNFSM4QSLLUNQ>
.
|
In #12832 that I just submitted recently, the test I added to protocol_integration_test here: In the If you comment line 32 ( |
|
Interestingly, some protocol combinations pass, some fail, and some timeout. Also in some combinations the But I noticed that all the cases where no |
|
That's an interesting failure mode for sure. Envoy totally handles bodies
without the content length header set, so if it's required for
injectEncodedDataToFilterChain I wonder if there's a bug somewhere for that
path? I don't think I can take a look this week but I'm going to suggest
that 1) it's worth understanding what's going on there for that PR and 2)
it's not a common failure mode (sending no content length in request or
response headers in end to end tests does work, and so I'd prefer we not
document it as something likely to break things as it may cause folks to do
unnecessary work to "fix" it
…On Thu, Sep 3, 2020 at 3:50 PM Yosry Ahmed ***@***.***> wrote:
Interestingly, some protocol combinations pass, some fail, and some
timeout. Also in some combinations the content-length header is
non-existent, while in others a value of 0 is added. Yet there's no direct
correlation between this and the time out. For example:
IPv4_HttpDownstream_HttpUpstream puts a content-length value of 0 but it
doesn't time out, it fails with a body mismatch.
IPv6_Http2Downstream_HttpUpstream also puts a content-length value of 0,
but it does time out.
But I noticed that all the cases where no content-length header is added
at all (null) pass. On the other hand, cases where the the content-length
is set to 0 (which I guess is a default behavior somewhere) either fail or
time out.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#12933 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AELALPL5DT2CZO5AQRMFQVDSD7XSBANCNFSM4QSLLUNQ>
.
|
|
I tried it before with
Unfortunately I am not able to investigate this. I can open an issue to report the bug, and then someone else might pick it up later. What do you think? |
|
Still a bit confused - if your "inject body" test only works with a content length up front, how are you going to combine chunks when you may not know the ultimate size? I think you have to solve that problem before you can land your caching feature. Either way I'm going to still say that given I've shown that the integration test framework (client and server) work fine without content length, saying you need to have content length seems misleading and I would not like to add that. I'm happy to call in another maintainer for a second opinion if you feel strongly it should be left in! |
|
Ah, per your earlier example
If you comment out the line it causes problems, but if you replace with My guess is the HCM was setting the content length to 0 in the utility functions, when there was no body So if you didn't clear the content length explicitly, you accidentally ended up with the wrong content length. So again I think the part of your change discussing incorrect content lengths is worth adding, but in this case setting a content length wasn't necessary either. |
The CacheFilter knows the length of the total body up front and sets the content length header accordingly in
I will modify the added docs to only mention incorrect content lengths. |
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
|
@alyssawilk looking again at It seems like if there's no Content-Length header, the HCM will set it to 0. Isn't this ultimately equivalent to having an incorrect Maybe I should modify the wording to something like "If the response has a body, and its |
|
Ugh I'm sorry, that was a bad link. Where we set it to 0 for header-only requests (which the test client was originally sending, before your filter altered things) |
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
LGTM, pending CI churn - thanks for improving our docs!
Makes sense, thanks!
I'm curious why the content-length is set to 0 in that path though. The filter returns |
I tried removing the Regardless, I think it should be documented somewhere that if a filter adds a body to headers-only response, either through |
2 participants
Commit Message:
Documented a common integration tests problem when the response content-length header does not match the actual body length.
Signed-off-by: Yosry Ahmed yosryahmed@google.com
Risk Level: N/A
Testing: N/A
Docs Changes: N/A
Release Notes: N/A