Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[http1] Refactor H/1 codec with a parser interface #15263

Merged
merged 22 commits into from
Mar 24, 2021

Conversation

asraa
Copy link
Contributor

@asraa asraa commented Mar 2, 2021

Co-authored-by: Derek Argueta darguetap@gmail.com
Signed-off-by: Asra Ali asraa@google.com

Commit Message: Refactors the HTTP/1 codec to use an interface Parser. http_parser implements the Parser as the legacy parser (hard-coded in) to prepare for a new parser implementation with llhttp.
Risk Level: Medium. big refactor.
Testing: Unit and integration tests pass.
See #9033 for history.
Part of: #5155

Performance: there is a small performance degradation with the refactor https://gist.github.com/asraa/8babd3516acd447aa0f2ff5aa61d2e19

BTW on naming the methods.

  • Names like onHeadersComplete(), onMessageComplete() are top-level virtual methods in the Parser. Server/Client specific sub-routings are called onHeadersCompleteBase()

asraa and others added 2 commits March 2, 2021 14:07
Co-authored-by: Derek Argueta <darguetap@gmail.com>
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
source/common/http/http1/legacy_parser_impl.cc Outdated Show resolved Hide resolved
source/common/http/http1/legacy_parser_impl.cc Outdated Show resolved Hide resolved
source/common/http/http1/codec_impl.h Outdated Show resolved Hide resolved
@derekargueta
Copy link
Member

Thanks for getting this pushed Asra! 🚀

asraa added 4 commits March 4, 2021 13:19
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
@yanavlasov yanavlasov self-assigned this Mar 8, 2021
asraa added 4 commits March 8, 2021 13:35
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
@yanavlasov
Copy link
Contributor

/wait

Signed-off-by: Asra Ali <asraa@google.com>
yanavlasov
yanavlasov previously approved these changes Mar 12, 2021
@asraa
Copy link
Contributor Author

asraa commented Mar 12, 2021

Yay! Thanks Yan!
@envoyproxy/senior-maintainers would love more eyes

@mattklein123 mattklein123 self-assigned this Mar 12, 2021
mattklein123
mattklein123 previously approved these changes Mar 14, 2021
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just skimmed through and looks great to me! Not sure if @alyssawilk wants to take a quick look or not.

Signed-off-by: Asra Ali <asraa@google.com>
@asraa asraa dismissed stale reviews from mattklein123 and yanavlasov via 4e71ed6 March 15, 2021 13:13
@phlax
Copy link
Member

phlax commented Mar 18, 2021

ah k - so not a bug - i was going to open another flake issue - phew!

@alyssawilk
Copy link
Contributor

ah, so verify examples failures should all merge main? Nice catch!
I'll tag the PRs I notice, other @envoyproxy/maintainers can do the same

Signed-off-by: Asra Ali <asraa@google.com>
@asraa asraa dismissed stale reviews from derekargueta and alyssawilk via 8edfd0c March 18, 2021 18:37
Signed-off-by: Asra Ali <asraa@google.com>
@asraa
Copy link
Contributor Author

asraa commented Mar 19, 2021

I think this is a legit failure in grpc-bridge, trying to debug with running locally, but I'll need to rebuild the docker images that verify_examples uses -- just turning up trace logging to identify the issue doesn't work (and grpc-bridge doesn't have integration tests, but maybe that's the fastest way to repro outside of verify_examples) @phlax

Signed-off-by: Asra Ali <asraa@google.com>
@asraa
Copy link
Contributor Author

asraa commented Mar 23, 2021

@phlax I was trying to build my own envoy binary locally and try to re-run the grpc-bridge verify examples. at this point i don't think it's a flake, but I am still pretty stuck debugging it.

@zuercher @snowp do you have any idea what might be causing grpc-bridge to fail from the grpc-bridge side?

@phlax
Copy link
Member

phlax commented Mar 23, 2021

@asraa is only failing locally or in ci also ?

@asraa
Copy link
Contributor Author

asraa commented Mar 23, 2021

I'm pretty sure it's local as well. I built my own envoy, spun up both proxies with the config, spun up the client and server, and ran the code, and couldn't get back the header per the example.

I couldn't run with the artifact created in CI or just be running ./verify_examples so I'm not 100% sure if something else went wrong.

@phlax
Copy link
Member

phlax commented Mar 23, 2021

the easier way to test an individual example/sandbox (eg grpc) is:

cd examples/grpc-bridge
./verify.sh

i would avoid the verify_examples.sh script

@phlax
Copy link
Member

phlax commented Mar 23, 2021

@asraa
Copy link
Contributor Author

asraa commented Mar 23, 2021

Yeah, I agree -- I'm trying right now to fix up the ci/Dockerfile-envoy to grab my locally build binary and change the dockerfiles in grpc bridge to use my local one, turn up trace logging and check

@phlax
Copy link
Member

phlax commented Mar 23, 2021

if you build the docker image with your binary and then set

export DOCKER_NO_PULL=1

as long as your docker image is correctly named it should use yours without attempting to pull the one from dockerhub

@asraa
Copy link
Contributor Author

asraa commented Mar 23, 2021

Sounds good, I think that's what I have, I've built envoy (and su-exec) binary, moved it to build_envoy/envoy-binary, run
docker build -f ci/Dockerfile-envoy -t envoy .
to get envoy:latest,

and changed grpc-bridge/Dockerfile-(server/client) to pull FROM envoy:latest

Seems like envoy is starting up, but I get failures when sending the request:

> [grpc-bridge] Set key value foo=bar
Traceback (most recent call last):
  File "/client/grpc-kv-client.py", line 85, in <module>
    run()
  File "/client/grpc-kv-client.py", line 79, in run
    response = client.set(key, value)
  File "/client/grpc-kv-client.py", line 40, in set
    return requests.post(HOST + "/kv.KV/Set", data=data, headers=HEADERS)
  File "/usr/local/lib/python2.7/site-packages/requests/api.py", line 119, in post
    return request('post', url, data=data, json=json, **kwargs)
  File "/usr/local/lib/python2.7/site-packages/requests/api.py", line 61, in request
    return session.request(method=method, url=url, **kwargs)
  File "/usr/local/lib/python2.7/site-packages/requests/sessions.py", line 542, in request
    resp = self.send(prep, **send_kwargs)
  File "/usr/local/lib/python2.7/site-packages/requests/sessions.py", line 655, in send
    r = adapter.send(request, **kwargs)
  File "/usr/local/lib/python2.7/site-packages/requests/adapters.py", line 498, in send
    raise ConnectionError(err, request=request)
requests.exceptions.ConnectionError: ('Connection aborted.', BadStatusLine("''",))
ERROR: 1

@asraa
Copy link
Contributor Author

asraa commented Mar 24, 2021

Got it!! I followed the directs in examples/grpc-bridge to generate the stubs and bring everything up. I see the failure in the parser on an ASSERT. Thanks!

(It's either a problem with the F_CHUNKED value or the presence of content length)

Edit: got it, I reverted the check for all one bits to compare with ULLONG_MAX as before, my logic was wrong to check that.

Signed-off-by: Asra Ali <asraa@google.com>
@mattklein123
Copy link
Member

Great news that you figured this out. @asraa would you mind adding a dedicated test for whatever broke that doesn't depend on verify examples?

/wait

Signed-off-by: Asra Ali <asraa@google.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@mattklein123 mattklein123 merged commit 38f6738 into envoyproxy:main Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants