http inspector:inline the recv in the onAccept #8111

lambdai · 2019-08-30T22:41:39Z

Signed-off-by: Yuchen Dai [email protected]

Similar to #7951

Description:

There is one behavior change.
This PR watch Closed type event. ~~On closed event http_inspector close the connection and http_inspector doesn't pass to follow up listener filters~~
Closed indicates a FIN is received on the OS supporting this event.
Upon Closed event http inspector would parse the last round. If the parser cannot determine the protocol is http this listener would give up since there is no more data from client.
Previous behavior: watch only READ event and hoping peek would return errno.

With this PR:

A poll cycle is saved.
A client stream "ping"+FIN flag could pass the listener filter. Not sure this is very useful in production but it helps me debug with ping-pong client server. And this behavior is less surprising.

I am not sure what event libevent would pass on when socket buffer is not empty and if peek would return -1 under this condition.

Risk Level: MID
Testing: UT
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: Yuchen Dai <[email protected]>

lambdai · 2019-08-30T22:54:13Z

CC initial author @yxue

lambdai · 2019-08-31T00:19:36Z

To reviewer: the wrong spelling is intended

htuch · 2019-09-03T02:22:59Z

@yxue would be great if you could take a first pass on this review, thanks.

yxue · 2019-09-03T02:47:13Z

@yxue would be great if you could take a first pass on this review, thanks.

Sure! I was reviewing and should finish the first pass earlier tomorrow.

source/extensions/filters/listener/http_inspector/http_inspector.h

yxue · 2019-09-03T16:18:28Z

source/extensions/filters/listener/http_inspector/http_inspector.cc

+        socket.ioHandle().fd(),
+        [this](uint32_t events) {
+          if (events & Event::FileReadyType::Closed) {
+            config_->stats().read_error_.inc();


Do we need to increase the read_error_ here?

That's a great point! I found out Closed actually means the client send FIN. It is probably not an error.
According to the above observation the latest commit allow http inspector to finalize the parse. Please review!

source/extensions/filters/listener/http_inspector/http_inspector.cc

Signed-off-by: Yuchen Dai <[email protected]>

lambdai · 2019-09-03T17:50:40Z

Update the description

lizan · 2019-09-06T04:36:21Z

@lambdai fix format?
/wait

lambdai · 2019-09-06T18:14:35Z

@lizan double checked the format is the spelling in string literal and it is intended. Do you known any annotation so that we can skip the check in certain line?

lizan · 2019-09-06T18:20:31Z

@jmarantz any idea above?

lambdai · 2019-09-10T16:38:17Z

Gentle ping @jmarantz

stale · 2019-09-17T17:03:23Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

jmarantz

The blocker on this is this error from the pedantic checker:

check_spelling_pedantic...
./test/extensions/filters/listener/http_inspector/http_inspector_test.cc:457:  const absl::string_view data = "GET /index HTT\r";
Checked 2284 file(s) and 59998 comment(s), found 1 error(s).
ERROR: spell check failed. Run 'tools/check_spelling_pedantic.py fix and/or add new words to tools/spelling_dictionary.txt'

for the record I did not create the pedantic spell checker (even though I am pedantic in other things) and find it annoying though I also see the value.

Suggestions for solutions:

Find a way in the pedantic checker to allow for a // NOPEDANTIC escape on the line in question, or similar. This is IMO the best option as it saves a ton of annoyance for others later.
This test on line 457 looking for "GET /index HTT\r looks kind of broken. Is it meant to be that way? Fix it by changing HTT to HTTP/1.1 or similar?
If you really need to send that string you could maybe octal-escape the "HTT" part?

I realize also you didn't even change that part of the code but are being dinged because you are editing a file that probably predated the pedantic spell checker. Not sure what to say..

typos and fix spelling Signed-off-by: Yuchen Dai <[email protected]>

lambdai · 2019-09-17T20:04:28Z

/retest ci/circleci: asan

repokitteh-read-only · 2019-09-17T20:04:32Z

🔨 rebuilding ci/circleci: asan (failed build)

🐱

Caused by: a #8111 (comment) was created by @lambdai.

see: more, trace.

jmarantz

this last change to side-step the pedantic check LGTM.

Please try to avoid force-pushing in the future though as it removes comment history from the PR review process. Just do normal pushes.

lizan · 2019-09-17T21:31:25Z

test/extensions/filters/listener/http_inspector/http_inspector_test.cc

-  const absl::string_view data = "GET /index HTT\r";
+  const std::string valid_header = "GET /index HTTP/1.1\r";
+  //  offset:                       0         10
+  const std::string truncate_header = valid_header.substr(0, 14).append("\r");


lambdai · 2019-09-17T22:02:08Z

this last change to side-step the pedantic check LGTM.

Please try to avoid force-pushing in the future though as it removes comment history from the PR review process. Just do normal pushes.

I won't if the commits attached with comment.

@jmarantz What if I modify the last commits that no one comment?

jmarantz · 2019-09-17T22:05:31Z

idk; why would you need to? why not just do normal commits always?

I guess there can be a race where you think no one has commented, but someone is commenting while you are doing the force-push.

Description: There is one behavior change. This PR watch Closed type event. `Closed` indicates a FIN is received on the OS supporting this event. Upon `Closed` event http inspector would parse the last round. If the parser cannot determine the protocol is http this listener would give up since there is no more data from client. Previous behavior: watch only READ event and hoping peek would return errno. With this PR: 1. A poll cycle is saved. 2. A client stream "ping"+FIN flag could pass the listener filter. Not sure this is very useful in production but it helps me debug with ping-pong client server. And this behavior is less surprising. Risk Level: MID Testing: UT Docs Changes: Release Notes: Signed-off-by: Yuchen Dai <[email protected]>

filter: http inspector read at onAccept

926bea8

Signed-off-by: Yuchen Dai <[email protected]>

lambdai requested a review from lizan as a code owner August 30, 2019 22:41

htuch assigned lizan Sep 3, 2019

yxue reviewed Sep 3, 2019

View reviewed changes

source/extensions/filters/listener/http_inspector/http_inspector.h Outdated Show resolved Hide resolved

yxue reviewed Sep 3, 2019

View reviewed changes

incomplete parse is not fatal

c659a4d

Signed-off-by: Yuchen Dai <[email protected]>

repokitteh-read-only bot added the waiting label Sep 6, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Sep 17, 2019

jmarantz reviewed Sep 17, 2019

View reviewed changes

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Sep 17, 2019

repokitteh-read-only bot removed the waiting label Sep 17, 2019

address comment

922214d

typos and fix spelling Signed-off-by: Yuchen Dai <[email protected]>

lambdai force-pushed the httpins branch from 5286c62 to 922214d Compare September 17, 2019 18:52

jmarantz reviewed Sep 17, 2019

View reviewed changes

lizan reviewed Sep 17, 2019

View reviewed changes

lizan approved these changes Sep 17, 2019

View reviewed changes

lizan merged commit 99004b8 into envoyproxy:master Sep 17, 2019

lambdai deleted the httpins branch September 17, 2019 22:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http inspector:inline the recv in the onAccept #8111

http inspector:inline the recv in the onAccept #8111

lambdai commented Aug 30, 2019 •

edited

Loading

lambdai commented Aug 30, 2019

lambdai commented Aug 31, 2019

htuch commented Sep 3, 2019

yxue commented Sep 3, 2019

yxue Sep 3, 2019

lambdai Sep 3, 2019

lambdai commented Sep 3, 2019

lizan commented Sep 6, 2019

lambdai commented Sep 6, 2019

lizan commented Sep 6, 2019

lambdai commented Sep 10, 2019

stale bot commented Sep 17, 2019

jmarantz left a comment

lambdai commented Sep 17, 2019

repokitteh-read-only bot commented Sep 17, 2019

jmarantz left a comment

lizan Sep 17, 2019

lambdai commented Sep 17, 2019

jmarantz commented Sep 17, 2019

http inspector:inline the recv in the onAccept #8111

http inspector:inline the recv in the onAccept #8111

Conversation

lambdai commented Aug 30, 2019 • edited Loading

lambdai commented Aug 30, 2019

lambdai commented Aug 31, 2019

htuch commented Sep 3, 2019

yxue commented Sep 3, 2019

yxue Sep 3, 2019

Choose a reason for hiding this comment

lambdai Sep 3, 2019

Choose a reason for hiding this comment

lambdai commented Sep 3, 2019

lizan commented Sep 6, 2019

lambdai commented Sep 6, 2019

lizan commented Sep 6, 2019

lambdai commented Sep 10, 2019

stale bot commented Sep 17, 2019

jmarantz left a comment

Choose a reason for hiding this comment

lambdai commented Sep 17, 2019

repokitteh-read-only bot commented Sep 17, 2019

jmarantz left a comment

Choose a reason for hiding this comment

lizan Sep 17, 2019

Choose a reason for hiding this comment

lambdai commented Sep 17, 2019

jmarantz commented Sep 17, 2019

lambdai commented Aug 30, 2019 •

edited

Loading