You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "pjfanning (via GitHub)" <gi...@apache.org> on 2023/12/25 18:39:53 UTC
[PR] revert grpc breaking netty tls change [incubator-pekko-grpc]
pjfanning opened a new pull request, #200:
URL: https://github.com/apache/incubator-pekko-grpc/pull/200
see #197 and #199
the akka-grpc 2.1.6 breakage was traced back to this OSS akka-grpc 2.1.6 change (https://github.com/akka/akka-grpc/pull/1649) and reverted here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] revert grpc breaking netty tls change [incubator-pekko-grpc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #200:
URL: https://github.com/apache/incubator-pekko-grpc/pull/200#discussion_r1436150003
##########
runtime/src/main/scala/org/apache/pekko/grpc/internal/NettyClientUtils.scala:
##########
@@ -183,22 +183,25 @@ object NettyClientUtils {
@InternalApi
private def createNettySslContext(javaSslContext: SSLContext): SslContext = {
import io.grpc.netty.shaded.io.netty.handler.ssl.{
- ApplicationProtocolConfig,
- ClientAuth,
- IdentityCipherSuiteFilter,
- JdkSslContext
+ JdkSslContext,
+ SslProvider
}
- // See
- // https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/JdkSslContext.java#L229-L309
- new JdkSslContext(
- javaSslContext,
- /* boolean isClient */ true,
- /* Iterable<String> ciphers */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
- IdentityCipherSuiteFilter.INSTANCE,
- /* ApplicationProtocolConfig apn */ ApplicationProtocolConfig.DISABLED, // use JDK default (null would also be acceptable, DISABLED config will select the NONE protocol and thus the JdkDefaultApplicationProtocolNegotiator)
- ClientAuth.NONE, // server-only option, which is ignored as isClient=true (as indicated in constructor Javadoc)
- /* String[] protocols */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
- /* boolean startTls */ false)
+ import java.lang.reflect.Field
+
+ // This is a hack for situations where the SSLContext is given.
+ // This approach forces using SslProvider.JDK, which is known not to work
+ // on JDK 1.8.0_252
+
+ // Create a Netty JdkSslContext object with all the correct ciphers, protocol settings, etc initialized.
+ val nettySslContext: JdkSslContext =
+ GrpcSslContexts.configure(GrpcSslContexts.forClient, SslProvider.JDK).build.asInstanceOf[JdkSslContext]
+
+ // Patch the SSLContext value inside the JdkSslContext object
+ val nettySslContextField: Field = classOf[JdkSslContext].getDeclaredField("sslContext")
+ nettySslContextField.setAccessible(true)
+ nettySslContextField.set(nettySslContext, javaSslContext)
+
+ nettySslContext
Review Comment:
The thing is that the git branch is protected - it will need to be unprotected to allow commits to be removed. Anyone who has a local git branch with the commit would also need to clean their set up afterwards. So forcing a removal of the commit is pretty drastic. It is not clear that it is absolutely needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] revert grpc breaking netty tls change [incubator-pekko-grpc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #200:
URL: https://github.com/apache/incubator-pekko-grpc/pull/200#discussion_r1436148208
##########
runtime/src/main/scala/org/apache/pekko/grpc/internal/NettyClientUtils.scala:
##########
@@ -183,22 +183,25 @@ object NettyClientUtils {
@InternalApi
private def createNettySslContext(javaSslContext: SSLContext): SslContext = {
import io.grpc.netty.shaded.io.netty.handler.ssl.{
- ApplicationProtocolConfig,
- ClientAuth,
- IdentityCipherSuiteFilter,
- JdkSslContext
+ JdkSslContext,
+ SslProvider
}
- // See
- // https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/JdkSslContext.java#L229-L309
- new JdkSslContext(
- javaSslContext,
- /* boolean isClient */ true,
- /* Iterable<String> ciphers */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
- IdentityCipherSuiteFilter.INSTANCE,
- /* ApplicationProtocolConfig apn */ ApplicationProtocolConfig.DISABLED, // use JDK default (null would also be acceptable, DISABLED config will select the NONE protocol and thus the JdkDefaultApplicationProtocolNegotiator)
- ClientAuth.NONE, // server-only option, which is ignored as isClient=true (as indicated in constructor Javadoc)
- /* String[] protocols */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
- /* boolean startTls */ false)
+ import java.lang.reflect.Field
+
+ // This is a hack for situations where the SSLContext is given.
+ // This approach forces using SslProvider.JDK, which is known not to work
+ // on JDK 1.8.0_252
+
+ // Create a Netty JdkSslContext object with all the correct ciphers, protocol settings, etc initialized.
+ val nettySslContext: JdkSslContext =
+ GrpcSslContexts.configure(GrpcSslContexts.forClient, SslProvider.JDK).build.asInstanceOf[JdkSslContext]
+
+ // Patch the SSLContext value inside the JdkSslContext object
+ val nettySslContextField: Field = classOf[JdkSslContext].getDeclaredField("sslContext")
+ nettySslContextField.setAccessible(true)
+ nettySslContextField.set(nettySslContext, javaSslContext)
+
+ nettySslContext
Review Comment:
it will work for now and we can build a new fix - code is a living thing, today's fixes can be improved later on
we have broken code today and #197 makes it hard for us to do a better fix than this in the short term
this change gets us back to the working code in akka-grpc 2.1.5
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] revert grpc breaking netty tls change [incubator-pekko-grpc]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #200:
URL: https://github.com/apache/incubator-pekko-grpc/pull/200#discussion_r1436239043
##########
runtime/src/main/scala/org/apache/pekko/grpc/internal/NettyClientUtils.scala:
##########
@@ -183,22 +183,25 @@ object NettyClientUtils {
@InternalApi
private def createNettySslContext(javaSslContext: SSLContext): SslContext = {
import io.grpc.netty.shaded.io.netty.handler.ssl.{
- ApplicationProtocolConfig,
- ClientAuth,
- IdentityCipherSuiteFilter,
- JdkSslContext
+ JdkSslContext,
+ SslProvider
}
- // See
- // https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/JdkSslContext.java#L229-L309
- new JdkSslContext(
- javaSslContext,
- /* boolean isClient */ true,
- /* Iterable<String> ciphers */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
- IdentityCipherSuiteFilter.INSTANCE,
- /* ApplicationProtocolConfig apn */ ApplicationProtocolConfig.DISABLED, // use JDK default (null would also be acceptable, DISABLED config will select the NONE protocol and thus the JdkDefaultApplicationProtocolNegotiator)
- ClientAuth.NONE, // server-only option, which is ignored as isClient=true (as indicated in constructor Javadoc)
- /* String[] protocols */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
- /* boolean startTls */ false)
+ import java.lang.reflect.Field
+
+ // This is a hack for situations where the SSLContext is given.
+ // This approach forces using SslProvider.JDK, which is known not to work
+ // on JDK 1.8.0_252
+
+ // Create a Netty JdkSslContext object with all the correct ciphers, protocol settings, etc initialized.
+ val nettySslContext: JdkSslContext =
+ GrpcSslContexts.configure(GrpcSslContexts.forClient, SslProvider.JDK).build.asInstanceOf[JdkSslContext]
+
+ // Patch the SSLContext value inside the JdkSslContext object
+ val nettySslContextField: Field = classOf[JdkSslContext].getDeclaredField("sslContext")
+ nettySslContextField.setAccessible(true)
+ nettySslContextField.set(nettySslContext, javaSslContext)
+
+ nettySslContext
Review Comment:
I am against unprotecting the git branch, it may have been acceptable right at the time of forking pekko but at this point there are way too many people that have this git branch checked out and over-writing the `main` branch basically forces everyone who has git cloned this repo to re-pull the changes, so even if we do remove the commit from `main` unless people wipe their repo they will still have the commit.
Thing is, I don't know what the ASF policy on this is, if I had a say in this I would add the commit (and the revert commit) to `.git-ignore-blame-revs` so that its more hidden but realistically I don't know what more can be done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] revert grpc breaking netty tls change [incubator-pekko-grpc]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #200:
URL: https://github.com/apache/incubator-pekko-grpc/pull/200#discussion_r1436150558
##########
runtime/src/main/scala/org/apache/pekko/grpc/internal/NettyClientUtils.scala:
##########
@@ -183,22 +183,25 @@ object NettyClientUtils {
@InternalApi
private def createNettySslContext(javaSslContext: SSLContext): SslContext = {
import io.grpc.netty.shaded.io.netty.handler.ssl.{
- ApplicationProtocolConfig,
- ClientAuth,
- IdentityCipherSuiteFilter,
- JdkSslContext
+ JdkSslContext,
+ SslProvider
}
- // See
- // https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/JdkSslContext.java#L229-L309
- new JdkSslContext(
- javaSslContext,
- /* boolean isClient */ true,
- /* Iterable<String> ciphers */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
- IdentityCipherSuiteFilter.INSTANCE,
- /* ApplicationProtocolConfig apn */ ApplicationProtocolConfig.DISABLED, // use JDK default (null would also be acceptable, DISABLED config will select the NONE protocol and thus the JdkDefaultApplicationProtocolNegotiator)
- ClientAuth.NONE, // server-only option, which is ignored as isClient=true (as indicated in constructor Javadoc)
- /* String[] protocols */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
- /* boolean startTls */ false)
+ import java.lang.reflect.Field
+
+ // This is a hack for situations where the SSLContext is given.
+ // This approach forces using SslProvider.JDK, which is known not to work
+ // on JDK 1.8.0_252
+
+ // Create a Netty JdkSslContext object with all the correct ciphers, protocol settings, etc initialized.
+ val nettySslContext: JdkSslContext =
+ GrpcSslContexts.configure(GrpcSslContexts.forClient, SslProvider.JDK).build.asInstanceOf[JdkSslContext]
+
+ // Patch the SSLContext value inside the JdkSslContext object
+ val nettySslContextField: Field = classOf[JdkSslContext].getDeclaredField("sslContext")
+ nettySslContextField.setAccessible(true)
+ nettySslContextField.set(nettySslContext, javaSslContext)
+
+ nettySslContext
Review Comment:
I know, but more safer way I think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] revert grpc breaking netty tls change [incubator-pekko-grpc]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #200:
URL: https://github.com/apache/incubator-pekko-grpc/pull/200#discussion_r1436149247
##########
runtime/src/main/scala/org/apache/pekko/grpc/internal/NettyClientUtils.scala:
##########
@@ -183,22 +183,25 @@ object NettyClientUtils {
@InternalApi
private def createNettySslContext(javaSslContext: SSLContext): SslContext = {
import io.grpc.netty.shaded.io.netty.handler.ssl.{
- ApplicationProtocolConfig,
- ClientAuth,
- IdentityCipherSuiteFilter,
- JdkSslContext
+ JdkSslContext,
+ SslProvider
}
- // See
- // https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/JdkSslContext.java#L229-L309
- new JdkSslContext(
- javaSslContext,
- /* boolean isClient */ true,
- /* Iterable<String> ciphers */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
- IdentityCipherSuiteFilter.INSTANCE,
- /* ApplicationProtocolConfig apn */ ApplicationProtocolConfig.DISABLED, // use JDK default (null would also be acceptable, DISABLED config will select the NONE protocol and thus the JdkDefaultApplicationProtocolNegotiator)
- ClientAuth.NONE, // server-only option, which is ignored as isClient=true (as indicated in constructor Javadoc)
- /* String[] protocols */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
- /* boolean startTls */ false)
+ import java.lang.reflect.Field
+
+ // This is a hack for situations where the SSLContext is given.
+ // This approach forces using SslProvider.JDK, which is known not to work
+ // on JDK 1.8.0_252
+
+ // Create a Netty JdkSslContext object with all the correct ciphers, protocol settings, etc initialized.
+ val nettySslContext: JdkSslContext =
+ GrpcSslContexts.configure(GrpcSslContexts.forClient, SslProvider.JDK).build.asInstanceOf[JdkSslContext]
+
+ // Patch the SSLContext value inside the JdkSslContext object
+ val nettySslContextField: Field = classOf[JdkSslContext].getDeclaredField("sslContext")
+ nettySslContextField.setAccessible(true)
+ nettySslContextField.set(nettySslContext, javaSslContext)
+
+ nettySslContext
Review Comment:
We should just drop it completly I think, otherwise the result commit log contains incompatible code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] revert grpc breaking netty tls change [incubator-pekko-grpc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #200:
URL: https://github.com/apache/incubator-pekko-grpc/pull/200#discussion_r1436148208
##########
runtime/src/main/scala/org/apache/pekko/grpc/internal/NettyClientUtils.scala:
##########
@@ -183,22 +183,25 @@ object NettyClientUtils {
@InternalApi
private def createNettySslContext(javaSslContext: SSLContext): SslContext = {
import io.grpc.netty.shaded.io.netty.handler.ssl.{
- ApplicationProtocolConfig,
- ClientAuth,
- IdentityCipherSuiteFilter,
- JdkSslContext
+ JdkSslContext,
+ SslProvider
}
- // See
- // https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/JdkSslContext.java#L229-L309
- new JdkSslContext(
- javaSslContext,
- /* boolean isClient */ true,
- /* Iterable<String> ciphers */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
- IdentityCipherSuiteFilter.INSTANCE,
- /* ApplicationProtocolConfig apn */ ApplicationProtocolConfig.DISABLED, // use JDK default (null would also be acceptable, DISABLED config will select the NONE protocol and thus the JdkDefaultApplicationProtocolNegotiator)
- ClientAuth.NONE, // server-only option, which is ignored as isClient=true (as indicated in constructor Javadoc)
- /* String[] protocols */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
- /* boolean startTls */ false)
+ import java.lang.reflect.Field
+
+ // This is a hack for situations where the SSLContext is given.
+ // This approach forces using SslProvider.JDK, which is known not to work
+ // on JDK 1.8.0_252
+
+ // Create a Netty JdkSslContext object with all the correct ciphers, protocol settings, etc initialized.
+ val nettySslContext: JdkSslContext =
+ GrpcSslContexts.configure(GrpcSslContexts.forClient, SslProvider.JDK).build.asInstanceOf[JdkSslContext]
+
+ // Patch the SSLContext value inside the JdkSslContext object
+ val nettySslContextField: Field = classOf[JdkSslContext].getDeclaredField("sslContext")
+ nettySslContextField.setAccessible(true)
+ nettySslContextField.set(nettySslContext, javaSslContext)
+
+ nettySslContext
Review Comment:
it will work for now and we can build a new fix - code is a living thing, today's fixes can be improved later on
we have broken code today and #197 makes it hard for us to do a better fix than this in the short term
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] revert grpc breaking netty tls change [incubator-pekko-grpc]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #200:
URL: https://github.com/apache/incubator-pekko-grpc/pull/200#discussion_r1436148439
##########
runtime/src/main/scala/org/apache/pekko/grpc/internal/NettyClientUtils.scala:
##########
@@ -183,22 +183,25 @@ object NettyClientUtils {
@InternalApi
private def createNettySslContext(javaSslContext: SSLContext): SslContext = {
import io.grpc.netty.shaded.io.netty.handler.ssl.{
- ApplicationProtocolConfig,
- ClientAuth,
- IdentityCipherSuiteFilter,
- JdkSslContext
+ JdkSslContext,
+ SslProvider
}
- // See
- // https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/JdkSslContext.java#L229-L309
- new JdkSslContext(
- javaSslContext,
- /* boolean isClient */ true,
- /* Iterable<String> ciphers */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
- IdentityCipherSuiteFilter.INSTANCE,
- /* ApplicationProtocolConfig apn */ ApplicationProtocolConfig.DISABLED, // use JDK default (null would also be acceptable, DISABLED config will select the NONE protocol and thus the JdkDefaultApplicationProtocolNegotiator)
- ClientAuth.NONE, // server-only option, which is ignored as isClient=true (as indicated in constructor Javadoc)
- /* String[] protocols */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
- /* boolean startTls */ false)
+ import java.lang.reflect.Field
+
+ // This is a hack for situations where the SSLContext is given.
+ // This approach forces using SslProvider.JDK, which is known not to work
+ // on JDK 1.8.0_252
+
+ // Create a Netty JdkSslContext object with all the correct ciphers, protocol settings, etc initialized.
+ val nettySslContext: JdkSslContext =
+ GrpcSslContexts.configure(GrpcSslContexts.forClient, SslProvider.JDK).build.asInstanceOf[JdkSslContext]
+
+ // Patch the SSLContext value inside the JdkSslContext object
+ val nettySslContextField: Field = classOf[JdkSslContext].getDeclaredField("sslContext")
+ nettySslContextField.setAccessible(true)
+ nettySslContextField.set(nettySslContext, javaSslContext)
+
+ nettySslContext
Review Comment:
Nice find, I have not check it:) maybe someone will do some tweets on X then.
And for the fix, I think that should not be hard. the tests suit maybe a little complex which need some keys setup.
Before we come up a fix, we should provide a test about it first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] revert grpc breaking netty tls change [incubator-pekko-grpc]
Posted by "mkurz (via GitHub)" <gi...@apache.org>.
mkurz commented on PR #200:
URL: https://github.com/apache/incubator-pekko-grpc/pull/200#issuecomment-1883209315
Thanks for fixing this over the holidays! I see you just released v1.0.2, so I released play-grpc 0.12.1 as well:
- https://github.com/playframework/play-grpc/releases/tag/0.12.1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] revert grpc breaking netty tls change [incubator-pekko-grpc]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #200:
URL: https://github.com/apache/incubator-pekko-grpc/pull/200#discussion_r1436148506
##########
runtime/src/main/scala/org/apache/pekko/grpc/internal/NettyClientUtils.scala:
##########
@@ -183,22 +183,25 @@ object NettyClientUtils {
@InternalApi
private def createNettySslContext(javaSslContext: SSLContext): SslContext = {
import io.grpc.netty.shaded.io.netty.handler.ssl.{
- ApplicationProtocolConfig,
- ClientAuth,
- IdentityCipherSuiteFilter,
- JdkSslContext
+ JdkSslContext,
+ SslProvider
}
- // See
- // https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/JdkSslContext.java#L229-L309
- new JdkSslContext(
- javaSslContext,
- /* boolean isClient */ true,
- /* Iterable<String> ciphers */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
- IdentityCipherSuiteFilter.INSTANCE,
- /* ApplicationProtocolConfig apn */ ApplicationProtocolConfig.DISABLED, // use JDK default (null would also be acceptable, DISABLED config will select the NONE protocol and thus the JdkDefaultApplicationProtocolNegotiator)
- ClientAuth.NONE, // server-only option, which is ignored as isClient=true (as indicated in constructor Javadoc)
- /* String[] protocols */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
- /* boolean startTls */ false)
+ import java.lang.reflect.Field
+
+ // This is a hack for situations where the SSLContext is given.
+ // This approach forces using SslProvider.JDK, which is known not to work
+ // on JDK 1.8.0_252
+
+ // Create a Netty JdkSslContext object with all the correct ciphers, protocol settings, etc initialized.
+ val nettySslContext: JdkSslContext =
+ GrpcSslContexts.configure(GrpcSslContexts.forClient, SslProvider.JDK).build.asInstanceOf[JdkSslContext]
+
+ // Patch the SSLContext value inside the JdkSslContext object
+ val nettySslContextField: Field = classOf[JdkSslContext].getDeclaredField("sslContext")
+ nettySslContextField.setAccessible(true)
+ nettySslContextField.set(nettySslContext, javaSslContext)
+
+ nettySslContext
Review Comment:
Another issue is, can't we just do a quick reset and git push -f to drop that commit?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] revert grpc breaking netty tls change [incubator-pekko-grpc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #200:
URL: https://github.com/apache/incubator-pekko-grpc/pull/200#issuecomment-1869085449
adding @mkurz too
Since we are now in license hell due to #197, I think the best starting place is to revert the original change that caused the issue (that was made before Pekko forked and we are safe to reference that change since it is an OSS friendly chane).
We can build on top of this but this might a starting point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] revert grpc breaking netty tls change [incubator-pekko-grpc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #200:
URL: https://github.com/apache/incubator-pekko-grpc/pull/200#issuecomment-1869473662
this seems to fix the play-grpc issue - https://github.com/playframework/play-grpc/issues/468#issuecomment-1869472024
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] revert grpc breaking netty tls change [incubator-pekko-grpc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #200:
URL: https://github.com/apache/incubator-pekko-grpc/pull/200#discussion_r1436148775
##########
runtime/src/main/scala/org/apache/pekko/grpc/internal/NettyClientUtils.scala:
##########
@@ -183,22 +183,25 @@ object NettyClientUtils {
@InternalApi
private def createNettySslContext(javaSslContext: SSLContext): SslContext = {
import io.grpc.netty.shaded.io.netty.handler.ssl.{
- ApplicationProtocolConfig,
- ClientAuth,
- IdentityCipherSuiteFilter,
- JdkSslContext
+ JdkSslContext,
+ SslProvider
}
- // See
- // https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/JdkSslContext.java#L229-L309
- new JdkSslContext(
- javaSslContext,
- /* boolean isClient */ true,
- /* Iterable<String> ciphers */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
- IdentityCipherSuiteFilter.INSTANCE,
- /* ApplicationProtocolConfig apn */ ApplicationProtocolConfig.DISABLED, // use JDK default (null would also be acceptable, DISABLED config will select the NONE protocol and thus the JdkDefaultApplicationProtocolNegotiator)
- ClientAuth.NONE, // server-only option, which is ignored as isClient=true (as indicated in constructor Javadoc)
- /* String[] protocols */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
- /* boolean startTls */ false)
+ import java.lang.reflect.Field
+
+ // This is a hack for situations where the SSLContext is given.
+ // This approach forces using SslProvider.JDK, which is known not to work
+ // on JDK 1.8.0_252
+
+ // Create a Netty JdkSslContext object with all the correct ciphers, protocol settings, etc initialized.
+ val nettySslContext: JdkSslContext =
+ GrpcSslContexts.configure(GrpcSslContexts.forClient, SslProvider.JDK).build.asInstanceOf[JdkSslContext]
+
+ // Patch the SSLContext value inside the JdkSslContext object
+ val nettySslContextField: Field = classOf[JdkSslContext].getDeclaredField("sslContext")
+ nettySslContextField.setAccessible(true)
+ nettySslContextField.set(nettySslContext, javaSslContext)
+
+ nettySslContext
Review Comment:
> Another issue is, can't we just do a quick reset and git push -f to drop that commit?
I'm sure if removing the #197 commit from git history is required but if it the consensus is to try to hide it altogether, we can go that way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] revert grpc breaking netty tls change [incubator-pekko-grpc]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #200:
URL: https://github.com/apache/incubator-pekko-grpc/pull/200#discussion_r1436146775
##########
runtime/src/main/scala/org/apache/pekko/grpc/internal/NettyClientUtils.scala:
##########
@@ -183,22 +183,25 @@ object NettyClientUtils {
@InternalApi
private def createNettySslContext(javaSslContext: SSLContext): SslContext = {
import io.grpc.netty.shaded.io.netty.handler.ssl.{
- ApplicationProtocolConfig,
- ClientAuth,
- IdentityCipherSuiteFilter,
- JdkSslContext
+ JdkSslContext,
+ SslProvider
}
- // See
- // https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/JdkSslContext.java#L229-L309
- new JdkSslContext(
- javaSslContext,
- /* boolean isClient */ true,
- /* Iterable<String> ciphers */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
- IdentityCipherSuiteFilter.INSTANCE,
- /* ApplicationProtocolConfig apn */ ApplicationProtocolConfig.DISABLED, // use JDK default (null would also be acceptable, DISABLED config will select the NONE protocol and thus the JdkDefaultApplicationProtocolNegotiator)
- ClientAuth.NONE, // server-only option, which is ignored as isClient=true (as indicated in constructor Javadoc)
- /* String[] protocols */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
- /* boolean startTls */ false)
+ import java.lang.reflect.Field
+
+ // This is a hack for situations where the SSLContext is given.
+ // This approach forces using SslProvider.JDK, which is known not to work
+ // on JDK 1.8.0_252
+
+ // Create a Netty JdkSslContext object with all the correct ciphers, protocol settings, etc initialized.
+ val nettySslContext: JdkSslContext =
+ GrpcSslContexts.configure(GrpcSslContexts.forClient, SslProvider.JDK).build.asInstanceOf[JdkSslContext]
+
+ // Patch the SSLContext value inside the JdkSslContext object
+ val nettySslContextField: Field = classOf[JdkSslContext].getDeclaredField("sslContext")
+ nettySslContextField.setAccessible(true)
+ nettySslContextField.set(nettySslContext, javaSslContext)
+
+ nettySslContext
Review Comment:
so this is leveraging the netty and based on a special impl too, future netty can do some kind of refactoring, and then this will fail again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] revert grpc breaking netty tls change [incubator-pekko-grpc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning merged PR #200:
URL: https://github.com/apache/incubator-pekko-grpc/pull/200
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org