Skip to content
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

Expose SslConfig through ConnectionInfo #2150

Merged
merged 1 commit into from
Mar 23, 2022

Conversation

idelpivnitskiy
Copy link
Member

Motivation:

SSLSession represents the result of SSL negotiation. However, it's
useful to know what SslConfig was used to establish the session for
observability or runtime checks.

Modifications:

  • Add ConnectionInfo#sslConfig() method;
  • Propagate SslConfig to all ConnectionInfo implementations;
  • Adjust tests;

Result:

Users have access to SslConfig from service or connection
factory/filter.

* @return The {@link SslConfig} if SSL/TLS is configured, or {@code null} otherwise.
*/
@Nullable
default SslConfig sslConfig() { // FIXME: 0.43 - consider removing default impl
Copy link
Member Author

Choose a reason for hiding this comment

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

Added more reviewers for visibility of this message:

WDYT about exposing sslConfig() through ExecutionContext instead of ConnectionInfo? That way it will be automatically visible through client.executionContext() and serverContext.executionContext() too.

Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed this offline, and agreed that its more relevant as part of the ConnectionInfo.

Copy link
Member Author

Choose a reason for hiding this comment

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

For visibility: client is an abstraction that does not know about the actual connections. There are implementations of the client (multi-address client) that can talk to HTTP and HTTPS targets. Therefore, having SslConfig at the client level does not make sense.
We can add serverContext.sslConfig() later, if we have a use-case for that. For now, will just keep it at ConnectionInfo.

Motivation:

`SSLSession` represents the result of SSL negotiation. However, it's
useful to know what `SslConfig` was used to establish the session for
observability or runtime checks.

Modifications:

- Add `ConnectionInfo#sslConfig()` method;
- Propagate `SslConfig` to all `ConnectionInfo` implementations;
- Adjust tests;

Result:

Users have access to `SslConfig` from service or connection
factory/filter.
Copy link
Contributor

@tkountis tkountis left a comment

Choose a reason for hiding this comment

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

lean and clean. :shipit:

@idelpivnitskiy idelpivnitskiy merged commit 0fae5cb into apple:main Mar 23, 2022
@idelpivnitskiy idelpivnitskiy deleted the ssl-config branch March 23, 2022 04:27
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.

2 participants