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

http inspector: rename h2 to h2c #8227

Merged
merged 2 commits into from
Sep 17, 2019
Merged

http inspector: rename h2 to h2c #8227

merged 2 commits into from
Sep 17, 2019

Conversation

yxue
Copy link
Contributor

@yxue yxue commented Sep 12, 2019

Signed-off-by: crazyxy yxyan@google.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: Rename h2 to h2c to respect the standard
Risk Level: Low
Testing: Unit test
Docs Changes: N/A
Release Notes: N/A
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: crazyxy <yxyan@google.com>
@yxue yxue requested a review from lizan as a code owner September 12, 2019 21:20
lizan
lizan previously approved these changes Sep 12, 2019
Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

Nice catch.

@lizan
Copy link
Member

lizan commented Sep 13, 2019

@yxue actually hold on this, can you double check this work with HCM when codec is auto? I guess this line will create http1 codec if ALPN is set to "h2c":
https://github.com/envoyproxy/envoy/blob/master/source/common/http/conn_manager_utility.cc#L41

@yxue
Copy link
Contributor Author

yxue commented Sep 13, 2019

@lizan it doesn't affect the selection of the codec no matter whichever I set here. It's seems to be a bug. When selecting the codec, it searches the connection's protocol first and for raw buffer transport socket, the protocol is empty. The codec selection will fallback to searching the preface inside the request body.

std::string ConnectionManagerUtility::determineNextProtocol(Network::Connection& connection,

@lizan
Copy link
Member

lizan commented Sep 16, 2019

Can you add a comment/TODO in HCM or HTTP inspector, to use detected protocol from http inspector and support h2c token?

Signed-off-by: crazyxy <yxyan@google.com>
@lizan lizan merged commit 8c28a4f into envoyproxy:master Sep 17, 2019
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Sep 24, 2019
Description: Rename h2 to h2c to respect the standard
Risk Level: Low
Testing: Unit test
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: crazyxy <yxyan@google.com>
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019
Description: Rename h2 to h2c to respect the standard
Risk Level: Low
Testing: Unit test
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: crazyxy <yxyan@google.com>
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019
Description: Rename h2 to h2c to respect the standard
Risk Level: Low
Testing: Unit test
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: crazyxy <yxyan@google.com>
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.

2 participants