Skip to content

Conversation

@glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Jul 12, 2019

Motivation:

Providing TLS configuration is difficult without prior knowledge since
one must provide the application protocols identifiers for ALPN. In
addition the default minimum TLS version provided by NIOs
TLSConfiguration is lower than that specified by the gRPC protococl.

Modifications:

Add TLS configuration for both server and client which defaults which
better align to gRPC.

Add shims marking the old APIs as deprecated.

Result:

TLS is easier to configure.

Motivation:

Providing TLS configuration is difficult without prior knowledge since
one must provide the application protocols identifiers for ALPN. In
addition the default minimum TLS version provided by NIOs
TLSConfiguration is lower than that specified by the gRPC protococl.

Modifications:

Add TLS configuration for both server and client which defaults which
better align to gRPC.

Add shims marking the old APIs as deprecated.

Result:

TLS is easier to configure.
// TODO: Remove these shims before v1.0.0 is tagged.

extension ClientConnection.Configuration {
@available(*, deprecated, message: "use 'tls' and 'ClientConnection.Configuration.TLS'")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use a renamed: attribute here to get Xcode to suggest a migration fixit

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a different type (albeit a very similar one) instead of a direct rename so this seemed more appropriate

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not delete it altogether right away, given that we are not at 1.0 yet?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, this makes it's easier to fix for anyone who is using the currently tagged version though.

// TODO: Remove these shims before v1.0.0 is tagged.

extension ClientConnection.Configuration {
@available(*, deprecated, message: "use 'tls' and 'ClientConnection.Configuration.TLS'")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not delete it altogether right away, given that we are not at 1.0 yet?

/// TLS configuration for a `ClientConnection`.
///
/// Note that this configuration is a subset of `NIOSSL.TLSConfiguration` where certain options
/// are removed from the users control to ensure the configuration complies with the gRPC
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super-nit: user's

trustRoots: trustRoots,
certificateChain: certificateChain,
privateKey: privateKey,
applicationProtocols: GRPCApplicationProtocolIdentifier.allCases.map { $0.rawValue }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, which identifiers does this include again?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off the top of my head: "h2" and "grpc-ext"

@MrMage MrMage merged commit 04227ec into grpc:nio Jul 15, 2019
@glbrntt glbrntt deleted the tls-configuration branch August 5, 2020 09:06
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.

3 participants