Skip to content

Conversation

@TingDaoK
Copy link
Contributor

@TingDaoK TingDaoK commented Nov 30, 2022

Issue #, if available:

Description of changes:

  • add support for using proxy to setup http/2 connection
  • right now only supports talking h1 with proxy and create the http/2 connection from the tunneling proxy.
  • Turns out google doesn't like :authority with host, I am totally okay with what ever the real world want. :)

TODO:

The AWS_TEST_HTTPS_H2_PROXY_URL now has a proxy host that can talk http/2 with client. Used apache server with mod_proxy_http2. You can simply use that host for the proxy option and update the tls of proxy options to have h2 in the alpn string to create the h2 connection with proxy. However, I don't know what's the expected behavior for client to talk to a h2 proxy host.

curl doesn't seem supporting it.

So, put it as TODO, but still have the host here incase we want to implement it in the future.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@TingDaoK TingDaoK marked this pull request as ready for review November 30, 2022 23:42
Copy link
Contributor

@graebm graebm left a comment

Choose a reason for hiding this comment

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

some trivial feedback, but looks good overall

bool prior_knowledge_http2;
struct aws_http1_connection_options original_http1_options;
struct aws_http2_connection_options original_http2_options; /* allocated with userdata */
struct aws_hash_table *alpn_string_map; /* allocated with userdata */
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it would be less confusing to just store the alpn_string_map by value? instead of the weird allocate_many thing?
struct aws_hash_table alpn_string_map;
instead of
struct aws_hash_table *alpn_string_map;

It's safe to call our aws_XYZ_clean_up() functions on something that you never called init() on, as long as it's zeroed out

size_t original_initial_window_size;
bool prior_knowledge_http2;
struct aws_http1_connection_options original_http1_options;
struct aws_http2_connection_options original_http2_options; /* allocated with userdata */
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why /* allocated with userdata */ comment is on original_http2_options which is a by-value thing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The options has setting array, which need to be allocated from heap. The comment was for that, will clarify it more.

@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@0c7b8b5). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head 4c7dc1b differs from pull request most recent head 135747f. Consider uploading reports for the commit 135747f to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #402   +/-   ##
=======================================
  Coverage        ?   41.80%           
=======================================
  Files           ?      859           
  Lines           ?   128280           
  Branches        ?    18041           
=======================================
  Hits            ?    53633           
  Misses          ?    70779           
  Partials        ?     3868           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

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