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

Support HTTP proxies #337

Closed
kylegentle opened this issue May 14, 2022 · 4 comments · Fixed by #338
Closed

Support HTTP proxies #337

kylegentle opened this issue May 14, 2022 · 4 comments · Fixed by #338

Comments

@kylegentle
Copy link
Collaborator

As written, Hubs require clients to use a hyper_rustls::HttpsConnector:

pub client: hyper::Client<hyper_rustls::HttpsConnector<hyper::client::connect::HttpConnector>, hyper::body::Body>,

Unfortunately, this connector doesn't support HTTP proxies (see related issue).

Can we make Hub::new() more generic, to enable customized connectors like those of hyper_proxy? This would also follow the convention of hyper's own builder.

@Byron
Copy link
Owner

Byron commented May 14, 2022

Thanks for bringing this up! I didn't look into the details yet, but if you think this can be solved by following hypers own conventions, the google crates should certainly follow suit.

PRs are welcome. In case you want to take it on, please only submit changes to src/ without regenerating the APIs. Thank you.

@kylegentle
Copy link
Collaborator Author

kylegentle commented May 16, 2022

Thanks for the quick response, @Byron!

I'm pretty new to Rust, and totally new to Mako, but I'll share what I've got so far. From there, if you can provide some guidance I'd be happy to write a fix, or if you decide it would be more efficient to do it yourself, of course that's great too.

The approach I have in mind is to update Hub's client type to:
hyper::Client<tower_service::Service<http::Uri>, hyper::body::Body>

This is more or less what hyper does with the connector parameter of their client builder (code pointer).

Link to the tower_service::Service trait for reference.

So my thinking is that we:

  1. Add tower_service as a dependency in Cargo.toml.
  2. Add use tower_service::Service and use http:Uri to src/rust/api/client.rs.
  3. Update the mako templates to appropriately generate these type annotations in the Hub code.
    a. Update src/mako/api/api.rs.mako, for the struct members and new function.
    b. Update the hub type parameters.

3b is the part I feel least equipped to tackle at this point, but I'd love a sanity check on the rest too. I'm sure I don't have a complete picture yet. 😄

Edit: tower::Service --> tower_service::Service, since that's how hyper_rustls does it.

@Byron
Copy link
Owner

Byron commented May 16, 2022

Thanks a lot for the summary!

It looks like tower_service adds the much needed abstraction layer to not bind so closely to the underlying transport, which sounds like the right approach.

Maybe there should also be a point 4) which updates the CLI code as it will instantiate an API hub as well.

As to how to proceed from here, maybe it helps to share my typical workflow. Usually I use the groups_migration codebase to try things. You could prototype the changes there and fiddle with the types until it works. The same could be done for the CLI of groups_migration for completeness. Once this works, these changes 'just' have to be transferred to the mako source, which usually is quite mechanical. make can be used to repeatedly make groupsmigration1-cargo ARGS=check for validation, along with make groupsmigration1-cli-cargo ARGS=check for the cli related code.

I hope that helps.

@kylegentle
Copy link
Collaborator Author

I discovered that this is also an issue in yup-oauth2; submitted a PR to fix it there as well.

kylegentle added a commit to kylegentle/google-apis-rs that referenced this issue May 22, 2022
Switch the constraints on Hub types to use public traits based on
tower::service, as recommended by Hyper. This enables support for
custom connectors beyond hyper_rustls::HttpsConnector

Closes Byron#337.
kylegentle added a commit to kylegentle/google-apis-rs that referenced this issue Jun 1, 2022
Switch the constraints on Hub types to use public traits based on
tower::service, as recommended by Hyper. This enables support for
custom connectors beyond hyper_rustls::HttpsConnector

Closes Byron#337.
kylegentle added a commit to kylegentle/google-apis-rs that referenced this issue Jun 1, 2022
Switch the constraints on Hub types to use public traits based on
tower::service, as recommended by Hyper. This enables support for
custom connectors beyond hyper_rustls::HttpsConnector

Closes Byron#337.
@Byron Byron closed this as completed in c5ff239 Jun 3, 2022
@Byron Byron closed this as completed in #338 Jun 3, 2022
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 a pull request may close this issue.

2 participants