support SSLContext setting in Akka-Http backend

[dwhjames]

mirroring the behavior of the Netty backend

[raboof]

This means if sslContext is set the trustManager setting will be ignored, right?

I think that probably makes sense, but perhaps we should add a validation and throw an error when a user sets both?

[dwhjames]

This means if sslContext is set the trustManager setting will be ignored, right?

I think that probably makes sense, but perhaps we should add a validation and throw an error when a user sets both?

Sorry @raboof, I could do with paying closer attention to my notifications 😄 .

I was attempting to mirror the existing behavior

akka-grpc/runtime/src/main/scala/akka/grpc/internal/NettyClientUtils.scala

Lines 59 to 76 in 5c0f851

	builder = settings.sslContext match {
	case Some(sslContext) =>
	builder.sslContext(createNettySslContext(sslContext))
	case None =>
	(settings.trustManager, settings.sslProvider) match {
	case (None, None) =>
	builder
	case (tm, provider) =>
	val context = provider match {
	case None =>
	GrpcSslContexts.configure(SslContextBuilder.forClient())
	case Some(sslProvider) =>
	GrpcSslContexts.configure(SslContextBuilder.forClient(), sslProvider)
	}
	builder.sslContext((tm match {
	case None => context
	case Some(trustManager) => context.trustManager(trustManager)
	}).build())

---

But I take your point that it would be a good safety feature in the config builder to throw an error on ‘ambiguous’ configuration, rather than taking some silent ordering of precedence.
However, my main concern there would be a conflict between loaded config (including defaults) from the resource classpath and then overriding in code, i.e., it might be legitimate to want to use the builder to override rather than just accumulate. Which would mean that you’d need two methods in the builder: one that errors, and one that supplants all prior TLS related config.

Could we leave that for a follow on PR?

[raboof]

This means if sslContext is set the trustManager setting will be ignored, right?
I think that probably makes sense, but perhaps we should add a validation and throw an error when a user sets both?

I was attempting to mirror the existing behavior

akka-grpc/runtime/src/main/scala/akka/grpc/internal/NettyClientUtils.scala

Lines 59 to 76 in 5c0f851

	builder = settings.sslContext match {
	case Some(sslContext) =>
	builder.sslContext(createNettySslContext(sslContext))
	case None =>
	(settings.trustManager, settings.sslProvider) match {
	case (None, None) =>
	builder
	case (tm, provider) =>
	val context = provider match {
	case None =>
	GrpcSslContexts.configure(SslContextBuilder.forClient())
	case Some(sslProvider) =>
	GrpcSslContexts.configure(SslContextBuilder.forClient(), sslProvider)
	}
	builder.sslContext((tm match {
	case None => context
	case Some(trustManager) => context.trustManager(trustManager)
	}).build())

But I take your point that it would be a good safety feature in the config builder to throw an error on ‘ambiguous’ configuration, rather than taking some silent ordering of precedence. (...)

Could we leave that for a follow on PR?

Yes, I guess that makes sense.

my main concern there would be a conflict between loaded config (including defaults) from the resource classpath and then overriding in code, i.e., it might be legitimate to want to use the builder to override rather than just accumulate. Which would mean that you’d need two methods in the builder: one that errors, and one that supplants all prior TLS related config.

I'm not sure I understand what you mean here. I agree it's legitimate to want to use the builder to override rather than just accumulate. When an existing GrpcClientSettings object already has a trustManager set, it might not be unreasonable to require the user to explicitly remove that trustManager before setting the sslContext?

[raboof]

Could we leave that for a follow on PR?

filed as issue #1674