Refactor NIOHTTPServerConfiguration#67
Conversation
|
One configuration that this (and the current) configuration don't allow is to configure a single server with HTTP/1 over plaintext and then HTTP/2 over TLS for example. Not sure if we need to support it but might be worth thinking about if we need to allow a per-connection configuration somehow. |
|
@FranzBusch we considered this, but I'm not sure. Looking at Go's In Go, http.ListenAndServe(":8080", plaintextHandler)
http.ListenAndServeTLS(":443", "cert.pem", "key.pem", secureHandler)For Axum it's similar: you either start plaintext servers via Given this, I think it makes sense to have to create separate servers for plaintext vs TLS connections: let plaintextServer = NIOHTTPServer(configuration: .init(transportSecurity: .plaintext))
let tlsServer = NIOHTTPServer(configuration: .init(transportSecurity: .tls(...)))
try await withThrowingDiscardingTaskGroup { group in
group.addTask {
try await plaintextServer.serve { request, requestContext, requestBodyAndTrailers, responseSender in
// ...
}
}
group.addTask {
try await tlsServer.serve { request, requestContext, requestBodyAndTrailers, responseSender in
// ...
}
}
} |
| ) | ||
|
|
||
| if versions.isEmpty { | ||
| fatalError("Invalid configuration: at least one supported HTTP version must be specified.") |
There was a problem hiding this comment.
I think we should be more consistent about throwing vs fatal-erroring. We throw errors in other scenarios, so we should probably do the same here.
| fatalError( | ||
| "Only HTTP/1.1 can be served over plaintext. transportSecurity must be set to (m)TLS for serving HTTP/2." | ||
| ) | ||
| } | ||
|
|
||
| if supportedHTTPVersions.isEmpty { | ||
| fatalError("Invalid configuration: at least one supported HTTP version must be specified.") |
There was a problem hiding this comment.
I think we should throw errors here instead of fatal erroring. We should try to keep fatal errors for logic errors / theoretically unreachable states. However we could reach these states by user error (i.e. having invalid configs) so we should probably throw instead.
| let snapshot = config.snapshot() | ||
|
|
||
| let error = #expect(throws: Error.self) { | ||
| let configError = try #require(throws: Error.self) { |
There was a problem hiding this comment.
Have we got some more specific error type that gets thrown?
There was a problem hiding this comment.
Unfortunately not, as ConfigError is not public.
Motivation:
Currently,
NIOHTTPServerConfiguration'stransportSecurityproperty implicitly determines the HTTP versions served byNIOHTTPServer:transportSecurityis.plaintext, an HTTP/1.1 channel handler (over plaintext) is set up.transportSecurityis.tlsor.mTLS(or their certificate-reloading variants), an ALPN channel handler configured with both HTTP/1.1 and HTTP/2 handlers is set up.As a consequence of this tight coupling, it is currently not possible to configure the server to only serve HTTP/1.1 over TLS.
Modifications:
Added a new
supportedHTTPVersionsfield (Set<HTTPVersion>) toNIOHTTPServerConfiguration. This, along with thetransportSecurityvalue, allows for the following configurations to be represented:transportSecuritysupportedHTTPVersions.plaintext[.http1_1].tls/.mTLS[.http1_1].tls/.mTLS[.http2(...)].tls/.mTLS[.http1_1, .http2(...)]Simplified
TransportSecurityto three options (plaintext,tls,mTLS):tlsandmTLScases in aTLSCredentialstype which hasinMemory,reloading, andpemFilebackings.MTLSTrustConfigurationtype which hassystemDefaults,inMemory,pemFile, andcustomCertificateVerificationCallbackbackings.Updated
NIOHTTPServerbased on the changes to the configuration type.Updated the
swift-configurationintegration to reflect the new structure.Result:
The HTTP versions supported by
NIOHTTPServercan now be configured explicitly through thesupportedHTTPVersionsproperty onNIOHTTPServerConfiguration, rather than being implicitly determined by the transport security configuration.