Support request specific TLS configuration#358
Uh oh!
There was an error while loading. Please reload this page.
Conversation
swift-server-bot commented Apr 30, 2021
Can one of the admins verify this patch? |
5 similar comments
swift-server-bot commented Apr 30, 2021
Can one of the admins verify this patch? |
swift-server-bot commented Apr 30, 2021
Can one of the admins verify this patch? |
swift-server-bot commented Apr 30, 2021
Can one of the admins verify this patch? |
swift-server-bot commented Apr 30, 2021
Can one of the admins verify this patch? |
swift-server-bot commented Apr 30, 2021
Can one of the admins verify this patch? |
Lukasa commented Apr 30, 2021
@swift-server-bot add to allowlist |
weissi left a comment
weissi left a comment There was a problem hiding this comment.
Fab, thank you! This looks like a great start!
I left a few comments, mostly requesting changes that we don't accidentally merge the branch dependency.
Uh oh!
There was an error while loading. Please reload this page.
| self.port = request.port | ||
| self.host = request.host | ||
| self.unixPath = request.socketPath | ||
| iflet tls = request.tlsConfiguration { |
There was a problem hiding this comment.
@artemredkin should we add a test case that targets the pool directly here? Ie no actual connections?
Uh oh!
There was an error while loading. Please reload this page.
weissi commented May 6, 2021
@madsodgaard hmm, weird 5.0 compiler warning (that we turn into errors): Also, async-http-client uses automatic format checking with SwiftFormat. If you run SwiftFormat over the source, then the "soundness" part should pass. |
madsodgaard commented May 6, 2021
@weissi Whoops, forgot a return statement 😅 |
weissi commented May 6, 2021
@madsodgaard also the API breakage checker detected an API breakage: Note that in Swift you cannot just add a new parameter (even if it has a default value) without breaking API. Instead of adding the |
Sources/AsyncHTTPClient/BestEffortHashableTLSConfiguration.swift Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
Sources/AsyncHTTPClient/Utils.swift Outdated
| letrequiresTLS= key.scheme.requiresTLS | ||
| // Override optional connection pool configuration. | ||
| varkeyConfiguration= configuration |
There was a problem hiding this comment.
I'm finding the naming here somewhat confusing: keyConfiguration is not derived from the key but from configuration, and then we override it with the TLS configuration from key.
I think it'd be nice to wrap this logic up into something written as a function that clarifies what it does (merges config from two sources, preferring config in the key to the general configuration.
I'm also a bit uncertain as to why this is necessary. Why isn't configuration already carrying this TLS config?
There was a problem hiding this comment.
Also, should this merge perhaps be done at a higher level, say when we create the HTTP1ConnectionProvider? I am a bit nervous about having two separate configs from the perspective of the connection provider: it should always be creating the exact same connection each time.
There was a problem hiding this comment.
configuration is specific for the entire HTTPClient, so atm it passes down the configuration of client through all these methods. So, we need at some point to override the tlsConfiguration of it.
I moved the actual configuration "generation" to the place where we initialize the connection provider as you suggested, and added config(overriding:) to Key to retrieve key-specific configuration. Let me know, if this is better!
weissi left a comment
weissi left a comment There was a problem hiding this comment.
Fantastic, thank you very much!
4 participants
Adds support for request-specific TLS configuration:
Request(url: "https://webserver.com", tlsConfiguration: .forClient())apple/swift-nio-ssl#280 must be released, before this can be merged.