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

[Feature Request]: Use reqwest instead of isahc #3091

Closed
ozgrakkurt opened this issue Mar 22, 2023 · 12 comments
Closed

[Feature Request]: Use reqwest instead of isahc #3091

ozgrakkurt opened this issue Mar 22, 2023 · 12 comments
Labels
bug Something isn't working no-issue-activity no-stale Opt-out of closing issue due to no activity

Comments

@ozgrakkurt
Copy link
Contributor

ozgrakkurt commented Mar 22, 2023

Hey!

Currently windows build is a little cumbersome because the project depends on OpenSSL which seems to be because it uses isahc.
It could depend on surf with the rustls option to remove OpenSSL dependency. This would improve build experience in case the user doesn't have OpenSSL on their system already.

I can do this if it makes sense

@ozgrakkurt ozgrakkurt added the bug Something isn't working label Mar 22, 2023
@ozgrakkurt ozgrakkurt changed the title [Feature Request]: Use [Feature Request]: Use surf instead of isahc Mar 22, 2023
@ozgrakkurt
Copy link
Contributor Author

I see Windows isn't supported but I'll leave this issue in case it is useful for something else

@digikata
Copy link
Contributor

Improving the build experience on windows is always nice and in general openssl as a dep can be annoying to get correctly configured in various build environments. So we'd definitely take a look at any contributions in that area.

I think you're talking about isahc in fluvio-cli-common? It would be nice to switch it over to surf with the hyper_client feature on, which IIRC uses rustls by default. From fluvio-hub-util, we're getting a warning with surf in default config with nom, so there could be a similar change there too.

@ozgrakkurt
Copy link
Contributor Author

ozgrakkurt commented Mar 23, 2023

@digikata would using `surf` with `async-h1-rustls` be good enough? `hyper` is more `tokio` oriented afaik. [async-h1](https://crates.io/crates/async-h1) After checking `async-h1`, it seems abandoned but `hyper` directly depends on `tokio`

@ozgrakkurt
Copy link
Contributor Author

ozgrakkurt commented Mar 28, 2023

hyper-client feature still pulls in openssl. But hyper, apparently, only depends on tokio for traits

@digikata
Copy link
Contributor

As an experiment, if you create a standalone crate with a surf dep configured to hyper-client, no openssl is pulled in.
So openssl may get pulled in from some other part of the dep tree? It may be something we need to chip away at a little at a time.

@ozgrakkurt
Copy link
Contributor Author

It pulls it in for me, it looks like hyper_client -> http_client/hyper_client -> hyper-tls -> native-tls -> openssl

@digikata
Copy link
Contributor

digikata commented Apr 7, 2023

Thanks for checking that, I re-checked on macos it is not used in that config, but w/ linux it is. Macos must use a different native tls lib. It's possible to use reqwest w/ rustls (which I can confirm doesn't pull in openssh), but it can get tricky in mixing it into dependencies in various crates and workspace in the total fluvio ecosystem.

You are welcome to give it a try and we can try to review and work through the issues, but it is looking like this change will take a little more time than a simple dep reconfig.

@ozgrakkurt
Copy link
Contributor Author

I checked reqwest, it seems like it depends on tokio so we would have to enable tokio03 feature on async-std and the more I look into it, it seems like the main issue is async-std isn't being developed much. I would propose to use hyper which might be too low level or switching to tokio which might be too much work so not sure. Also fluvio-hub-util already depends on surf/async-h1 so might be ok to just use it maybe?

@digikata
Copy link
Contributor

fluvio-hub-util dep w/ surf/async-h1 is what is giving a nom future deprecated feature warning. The async-h1 dep unfortunately also seems not very maintained. So we have two problems to potentially solve:

  • move off of openssl usage
  • replace current http client dependencies with something more maintained (the nom warning)

In some other portions of the internal codebase we've been mixing in some uses of rewqest/hyper-rustls, but its on a few selected cli only binaries. It might be work try and see a few more places for reqwest to see if it can be used, or at least identify the pain points.

@ozgrakkurt ozgrakkurt changed the title [Feature Request]: Use surf instead of isahc [Feature Request]: Use reqwest instead of isahc Apr 17, 2023
@github-actions
Copy link

Stale issue message

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 24, 2023
@ozgrakkurt ozgrakkurt reopened this Jun 24, 2023
@github-actions
Copy link

Stale issue message

@digikata digikata added the no-stale Opt-out of closing issue due to no activity label Aug 25, 2023
@digikata
Copy link
Contributor

digikata commented Oct 11, 2023

doing other work on this instead see #3585

@digikata digikata closed this as not planned Won't fix, can't repro, duplicate, stale Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working no-issue-activity no-stale Opt-out of closing issue due to no activity
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants