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

Allow HTTP1.1, H2, and H2C protocol for HttpClient #2659

Merged

Conversation

alextychan
Copy link
Contributor

@alextychan alextychan commented Jan 19, 2023

Currently a client that's configured for HTTP 1.1, H2, and H2C is unable to make requests to both HTTPS and HTTP urls due to checks in HttpClientConnect.java which can be safely ignored as long as a backup protocol is configured.

The following changes are in line with discussion in this PR #1403. Specifically the table as follows.

Configured Client Protocols Server URI scheme Used Protocol Current Status
{H2} http error Works
{H2} https H2 (pre-known) Works
{H2C} http H2C (pre-known) Works
{H2C} https error Works
{HTTP11} http HTTP11 Works
{HTTP11} https HTTP11 Works
{H2, H2C} http H2C (pre-known) Works
{H2, H2C} https H2 (pre-known) Works
{H2, HTTP11} http HTTP11 Works
{H2, HTTP11} https H2 (offered via ALPN, fallback to HTTP11) Works
{H2C, HTTP11} http H2C (offered via Upgrade header, fallback to HTTP11) Works
{H2C, HTTP11} https HTTP11 Works
{H2, H2C, HTTP11} http H2C (offered via Upgrade header, fallback to HTTP11). Fails with "Configured H2 protocol without TLS"
{H2, H2C, HTTP11} https H2 (offered via ALPN, fallback to HTTP11) Fails with "Configured H2 Clear-Text protocol with TLS"

This fix will allow clients configured with HTTP 1.1, H2, and H2C to work as expected by ignoring H2C in HTTPS, and by ignoring H2 in HTTP.

@pivotal-cla
Copy link

@alextychan Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@alextychan Thank you for signing the Contributor License Agreement!

@alextychan alextychan closed this Jan 19, 2023
@alextychan alextychan reopened this Jan 19, 2023
Copy link
Member

@violetagg violetagg left a comment

Choose a reason for hiding this comment

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

Can you provide also test that demonstrates how HTTP and HTTPS urls will behave when the client is configured with all 3 types of protocol?

@violetagg violetagg added this to the 1.0.28 milestone Jan 24, 2023
@violetagg violetagg added the type/bug A general bug label Jan 24, 2023
Enable H1, H2, and H2C protocols for HttpClient by
dropping H2C protocol when handling HTTPS urls, and by
dropping H2 protocol when handling HTTP urls
when another protocol is present

Signed-off-by: TaiYau Chan <alextychan@gmail.com>
@alextychan alextychan force-pushed the allow-h1-h2-h2c-protocols-for-httpclient branch from 232f38c to 6241b24 Compare January 26, 2023 11:58
@alextychan
Copy link
Contributor Author

I've only added tests for edge cases where the client is configured with all 3 types of protocol for the following cases.

  1. Calling a https url with ssl configured.
    • Previously: Fails with "Configured H2 Clear-Text protocol with TLS"
    • Now: Should succeed by ignoring H2C protocol
  2. Calling a http url without ssl configured.
    • Previously: Fails with "Configured H2 protocol without TLS"
    • Now: Should succeed by ignoring H2 protocol

Copy link
Member

@violetagg violetagg left a comment

Choose a reason for hiding this comment

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

@alextychan Thanks for the PR

@reactor/netty-team PTAL

@violetagg violetagg requested a review from a team January 26, 2023 14:14
@violetagg
Copy link
Member

The failing QUIC tests on CI are not related

Copy link
Member

@pderop pderop left a comment

Choose a reason for hiding this comment

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

looking good to me,

great patch; thanks @alextychan !

@violetagg violetagg merged commit 9a7b473 into reactor:main Jan 26, 2023
violetagg added a commit that referenced this pull request Jan 26, 2023
violetagg pushed a commit that referenced this pull request Jan 26, 2023
Enable H1, H2, and H2C protocols for HttpClient by
dropping H2C protocol when handling HTTPS urls, and by
dropping H2 protocol when handling HTTP urls
when another protocol is present

This is a backport for PR  #2659

Signed-off-by: TaiYau Chan <alextychan@gmail.com>
@violetagg violetagg changed the title Allow H1, H2, and H2C protocol for HttpClient Allow HTTP1.1, H2, and H2C protocol for HttpClient Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants