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.

Sign up for GitHub

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

revert grpc breaking netty tls change #200

Merged
merged 2 commits into from
Dec 26, 2023
Merged

revert grpc breaking netty tls change #200

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
898c55c
revert grpc breaking netty tls change
pjfanning Dec 25, 2023
29be594
Update NettyClientUtils.scala
pjfanning Dec 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 17 additions & 17 deletions runtime/src/main/scala/org/apache/pekko/grpc/internal/NettyClientUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,23 +182,23 @@ object NettyClientUtils {
*/
@InternalApi
private def createNettySslContext(javaSslContext: SSLContext): SslContext = {
import io.grpc.netty.shaded.io.netty.handler.ssl.{
ApplicationProtocolConfig,
ClientAuth,
IdentityCipherSuiteFilter,
JdkSslContext
}
// 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 io.grpc.netty.shaded.io.netty.handler.ssl.{ JdkSslContext, SslProvider }
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
Comment on lines +199 to +201
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@pjfanning pjfanning Dec 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Another issue is, can't we just do a quick reset and git push -f to drop that commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

We should just drop it completly I think, otherwise the result commit log contains incompatible code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I know, but more safer way I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

/**
Expand Down
Loading