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

WIP - Switch to Faraday #11087

Closed
wants to merge 1 commit into from
Closed

WIP - Switch to Faraday #11087

wants to merge 1 commit into from

Conversation

catlee
Copy link

@catlee catlee commented Jul 21, 2022

No description provided.

@google-cla
Copy link

google-cla bot commented Jul 21, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@bajajneha27
Copy link
Contributor

bajajneha27 commented Aug 1, 2022

Hi @catlee ,
Thank you for opening this PR. This is going to be a significantly big PR and we'll have to make sure that changes are backward compatible. So before you make further changes, I'd recommend you to keep these two things in mind:

  1. The interface shouldn't change. Currently the interface of BaseService#client is being changed; It now returns FaradayClient rather than HTTPClient, and
  2. No functionality is lost (we'd need to carefully audit all the capabilities of HTTPClient that are being used and depended on, and make sure they are replicated in the Faraday implementation). There are a lot of other libraries which are dependent on core library and hence we'll have to be careful.

@bajajneha27 bajajneha27 closed this Aug 1, 2022
@bajajneha27 bajajneha27 reopened this Aug 1, 2022
@catlee
Copy link
Author

catlee commented Aug 10, 2022

Hi @catlee , Thank you for opening this PR. This is going to be a significantly big PR and we'll have to make sure that changes are backward compatible. So before you make further changes, I'd recommend you to keep these two things in mind:

  1. The interface shouldn't change. Currently the interface of BaseService#client is being changed; It now returns FaradayClient rather than HTTPClient

This is a pretty significant restriction. Many of the methods in the code pass around a client object, which is assumed to be an HTTPClient instance. I'm not sure how we can switch to another implementation without changing this contract.

Do you have any suggestions for how to resolve this? I wonder if it's possible to create an adapter for FaradayClient that makes it behave like HTTPClient...

  1. No functionality is lost (we'd need to carefully audit all the capabilities of HTTPClient that are being used and depended on, and make sure they are replicated in the Faraday implementation). There are a lot of other libraries which are dependent on core library and hence we'll have to be careful.

The test suite seems very comprehensive. Are there areas of particular concern?

@bajajneha27
Copy link
Contributor

Hey @catlee
Sorry it's taking long for us to respond.
We generally don't allow any breaking change in our libraries; but I get your point. Let me discuss this internally and get back to you. Thank you for your patience!

@runephilosof-karnovgroup

@bajajneha27 Any news?

Since HTTPClient has been unmaintained for a long time, you should also consider whether this is a security issue #2348 (comment)

depending on abandonware that hasn't been touched since 2016 that handles establishing SSL connections is not secure at all

@Peredery
Copy link

bump :(

@hwo411
Copy link

hwo411 commented May 9, 2023

Unfortunately, we're having bigger and bigger issues with httpclient, so it'd be great if we can have this ready.

@bajajneha27 are there any updates on this topic? The issue is indeed severe (for us it's like if we touch the code that uses google cloud storage, we then likely need a workaround for a new problem).

UPD: sorry for double commenting, github was lagging yesterday

@bajajneha27
Copy link
Contributor

Hi everyone,
Appreciate your patience here. This is rather a big change. Unfortunately, we're not taking any significant feature requests for google-api-ruby-client and hence we won't be able to make this change. We'll circle back if anything pressing comes up.
I'll go ahead and close this PR. Thank you!

@casperisfine
Copy link
Contributor

@bajajneha27 I'm not sure if you are aware, but httpclient is straight out broken today because it has an old list of CA certificates from 2015 hardcoded in its source: nahi/httpclient#445

I don't understand how this can be considered, "not pressing".

@NivedhaSenthil
Copy link
Member

While we work on moving away from HttpClient, a new version of gem that patches the client to use systems default root CA path is now released.

@casperisfine
Copy link
Contributor

Now, httpclient is broken on Ruby 3.4 because it uses mutex_m that has been removed from the stdlib, so the gem would need to update its gemspec.

@dazuma
Copy link
Member

dazuma commented Jan 16, 2024

We'll make sure mutex_m is included in the google-apis-core gemspec if needed.

@mohamedhafez
Copy link

mohamedhafez commented Jul 8, 2024

Just registering my disappointment that you guys have open source community members willing to work on a security issue to make a Google product more secure, useable, and future-proof, but are refusing to let them. If breaking changes are an issue, just call it a new major version. A broken 8 year old abandoned SSL implementation is not going to hold forever, and is probably full of security holes as it is since literally nobody has touched that code in coming up on a decade... @dazuma @bajajneha27

@Firefishy
Copy link

While we work on moving away from HttpClient, a new version of gem that patches the client to use systems default root CA path is now released.

Here is the change if anyone is interested: #16446

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.

10 participants