Skip to content

http: Improving HTTP/1.1 proxy support#23077

Merged
alyssawilk merged 2 commits intoenvoyproxy:mainfrom
alyssawilk:proxy
Sep 19, 2022
Merged

http: Improving HTTP/1.1 proxy support#23077
alyssawilk merged 2 commits intoenvoyproxy:mainfrom
alyssawilk:proxy

Conversation

@alyssawilk
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk commented Sep 13, 2022

When the proxy filter was implemented it did direct string checks to ensure a 200 response. This addresses a TODO to accept any well formed 200 response.

Risk Level: low
Testing: new unit tests, integration tests
Docs Changes: n/a
Release Notes: n/a
Part of envoyproxy/envoy-mobile#1622

@repokitteh-read-only
Copy link
Copy Markdown

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #23077 was opened by alyssawilk.

see: more, trace.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk alyssawilk marked this pull request as ready for review September 14, 2022 18:09
@alyssawilk
Copy link
Copy Markdown
Contributor Author

cc @bencebeky I'm just straight up using balsa here if you're good with it. It turns out http_parser stops one character early and I'd rather just use balsa. This filter is being rolled out for Envoy Mobile and all this does is validate a 200 OK response for CONNECT requests

@bencebeky
Copy link
Copy Markdown
Contributor

cc @bencebeky I'm just straight up using balsa here if you're good with it. It turns out http_parser stops one character early and I'd rather just use balsa. This filter is being rolled out for Envoy Mobile and all this does is validate a 200 OK response for CONNECT requests

Sounds good to me.

@alyssawilk
Copy link
Copy Markdown
Contributor Author

@snowp ping!

Copy link
Copy Markdown
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

LGTM, nice!

if (need_to_strip_connect_response_) {
// Limit the CONNECT response headers to an arbitrary 200 bytes.
constexpr uint32_t MAX_RESPONSE_HEADER_SIZE = 200;
// Limit the CONNECT response headers to an arbitrary 2000 bytes.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know this is arbitrary but why the increase?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I decided the prior arbitrary limit was too small :-)
Kuat has been talking about adding extra headers to the CONNECT headers, and Rafal was adding test headers in his integration test, so I want to handle more than just a raw response.

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.

3 participants