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 using custom endpoints for Async::HTTP::Internet #51

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

binarycode
Copy link
Contributor

This PR introduces changes that allow passing custom Async::HTTP::Endpoint to Async::HTTP::Internet. This could be useful for example when we need to provide custom SSL options to requests.

@ioquatix
Copy link
Member

I agree with your assessment, but expanding the interface in this direction I need a little bit of time to think about it.

@binarycode
Copy link
Contributor Author

binarycode commented May 21, 2020

The thing is I want to add support of SSL options (https://github.com/lostisland/faraday/blob/master/lib/faraday/options/ssl_options.rb) to async-http-faraday adapter. I can see 2 approaches to that:

  1. Rewrite the adapter to use lower-level Async::HTTP::Client that allows customizing endpoints. The downside of this approach is that I'll have to basically duplicate the logic of Async::HTTP::Internet in the adapter to allow for persistent connections.
  2. Modify Async::HTTP::Internet to allow passing customized endpoints - that's why I created this PR.

What approach do you think is better? Should I try going with the first one to avoid changing Async::HTTP::Internet API?

@ioquatix
Copy link
Member

Can you make a subclass and add the appropriate hooks so you can configure the SSL to your requirements in the faraday adapter?

@binarycode
Copy link
Contributor Author

I guess that would be almost the same as the first approach, as most of the Async::HTTP::Internet logic is contained in the #call method that I'll need to rewrite to allow endpoint customization. Plus one of the downsides is that any upstream changes in Async::HTTP::Internet could possibly break this subclass and that will not be detected by CI, as the adapter is not pinned to a particular async-http version.
Anyway I'll try this approach and make a PR to the adapter gem today.

@ioquatix
Copy link
Member

ioquatix commented May 21, 2020

Can we expose the right internal hooks for Async::HTTP::Internet and add specs for them?

Do you imagine all clients would use the same SSL settings? I imagine something like:

internet = MyInternet.new

internet.client_options[hostname] = ssl_options

Or something like that (those names/methods had all of 30 seconds of brain power).

When you have specific SSL state, does it impact all connections or just some specific ones?

@binarycode
Copy link
Contributor Author

Well if we are talking about the faraday use-case, SSL can be configured only per Faraday::Connection, so all requests made through the adapter wrapped by that connection will use the same SSL settings.

One way this could be configured in Async::HTTP::Internet is to add a new initialize option:

def initialize(**options)
  @clients = {}
  @ssl_context = options.delete(:ssl_context)
  @options = options
end

or a separate method:

attr_writer :ssl_context

And then use that instance variable to build the endpoint:

def call(method, url, headers = [], body = nil)
  endpoint = Endpoint.parse(url, nil, ssl_context: @ssl_context)

Implementing the SSL options per-host will be trickier, as the actual per-host Async::HTTP::Client caching done by the Async::HTTP::Internet is an internal implementation detail.

@softbrada
Copy link

any news on this PR? i was trying to set SSL options today also and found this PR

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.

None yet

3 participants