-
Notifications
You must be signed in to change notification settings - Fork 435
Add TLS support for NIOPosix H2 client #2036
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great so far. I left a few comments but the bigger piece of feedback is that we should now be adding some end-to-end tests for this.
Sources/GRPCHTTP2TransportNIOPosix/HTTP2ClientTransport+Posix.swift
Outdated
Show resolved
Hide resolved
Sources/GRPCHTTP2TransportNIOPosix/HTTP2ClientTransport+Posix.swift
Outdated
Show resolved
Hide resolved
f389a06
to
9590227
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small thing to shuffle around but looks good otherwise, thanks Gus!
serverHostname = nil | ||
case .tls(let tlsConfig): | ||
do { | ||
nioSSLContext = try NIOSSLContext(configuration: TLSConfiguration(tlsConfig)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are expensive to create (IIRC it's of the order 20k allocations!) and at the moment we do it per connection. Can we do it in init
instead and then reuse it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While making this change I realised that we're exposing the tls
case for TransportSecurity
even when NIOSSL
cannot be imported. I think this is unsafe, because it means users may be able to add a TLSConfig
to their server/client, which then simply will be ignored if the platform the code's being built on doesn't support NIOSSL. If this happens on both the server and the client, it could mean the connection is plaintext and there would be no warnings that the config wasn't used.
I've wrapped the tls
case of TransportSecurity
in #if canImport(NIOSSL)
to avoid this. Let me know if you agree with it or if I'm missing something/think it should be resolved in some other way.
Sources/GRPCHTTP2TransportNIOPosix/HTTP2ServerTransport+Posix.swift
Outdated
Show resolved
Hide resolved
Sources/GRPCHTTP2TransportNIOPosix/HTTP2ServerTransport+Posix.swift
Outdated
Show resolved
Hide resolved
4f19c21
to
8853b7e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks Gus!
Motivation
We currently have a
NIOPosix
client transport implementation in gRPC v2, but it doesn't support TLS.Modifications
This PR adds support for TLS in the NIOPosix-backed HTTP/2 implementation of the client transport for gRPC v2.
Result
We now support TLS when using the NIOPosix client transport in gRPC V2.