Support request specific TLS configuration

[madsodgaard]

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.

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[Lukasa]

@swift-server-bot add to allowlist

[weissi]

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.

[weissi]

@artemredkin should we add a test case that targets the pool directly here? Ie no actual connections?

[weissi]

@madsodgaard hmm, weird 5.0 compiler warning (that we turn into errors):

[449/452] Compiling Swift Module 'AsyncHTTPClient' (15 sources)
/code/Sources/AsyncHTTPClient/BestEffortHashableTLSConfiguration.swift:16:18: error: result of call to 'bestEffortEquals' is unused
        lhs.base.bestEffortEquals(rhs.base)
                 ^               ~~~~~~~~~~
Build step 'Execute shell' marked build as failure

Also, async-http-client uses automatic format checking with SwiftFormat. If you run SwiftFormat over the source, then the "soundness" part should pass.

[madsodgaard]

@weissi Whoops, forgot a return statement 😅

[weissi]

@madsodgaard also the API breakage checker detected an API breakage:

08:37:54 /* Removed Decls */
08:37:54 Constructor HTTPClient.Request.init(url:method:headers:body:) has been removed

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 tlsConfiguration: parameter with a default value, you'll need to add a new init with an extra (non-defaulted) parameter. The existing HTTPClient.Request.init(url:method:headers:body:) should then just call the new init with the extra parameter.

[Lukasa]

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?

[Lukasa]

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.

[madsodgaard]

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!

[Lukasa]

Cool, this LGTM.

[weissi]

Fantastic, thank you very much!