-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
io,grpc,netty: Add support for IBMJSSE2 provider #5374
Conversation
Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement. After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.
Regards, |
4a8af86
to
b1819c5
Compare
Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement. After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.
Regards, |
Filled in the CLA form. |
This commit improves gRPC to run gRPC server with TLS enabled on AIX machines with IBMJSSE2 as their JDK SSL provider.
b1819c5
to
c0235e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM.
I am curious if this can get a test. We don't have an IBM JDK testbed set up so it might be unknowingly broken in the future.
The link in the PR comment doesn't work - the text is correct but the actual link is |
@@ -305,3 +324,4 @@ static void ensureAlpnAndH2Enabled( | |||
alpnNegotiator.protocols()); | |||
} | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the newline needed? I prefer to see only changes that are needed in the PR.
} else if (IBM_PROVIDER_NAME.equals(provider.getName())) { | ||
if (JettyTlsUtil.isJettyAlpnConfigured() | ||
|| JettyTlsUtil.isJettyNpnConfigured() | ||
|| JettyTlsUtil.isJava9AlpnAvailable()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lines 285-287 are identical to lines 279-281. The checks seem to be independent of provider
(the for loop iterator) and loop invariant. Can we create a helper method and also initialize the value outside the loop?
I agree with @carl-mastrangelo : it will be very desirable to have a couple of tests. One for SunJSSE (with the Sun JDK) and another one for IBMJSSE2 (with the IBM JDK?). If there are problems adding tests I would like to know |
@sanjaypujare I would be happy to add the tests. May I know what exactly should we be covering in the tests? Also, as @carl-mastrangelo earlier said that you do not have IBM test bed set up, how am I supposed to verify if the tests pass? |
@ankitshubham97 Were you able to run our tests on this JVM? (I assume you have access to it). You should be able to run As for what we want to see, the coverage numbers from codecov would be good. Lastly, since we don't have access to this JVM ourselves, it would have to be clear up front that we may break support (accidentally, or from technical compromise) for it. We do want gRPC to run everywhere, but short of regular testing it would be best effort. Is that okay with you? |
@ankitshubham97 I was thinking you could add tests that are enabled only for the IBM JVM and you are able to verify those tests. So you can verify that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as tests, TlsTest could be extended to try out this IBM provider. It'd probably be easiest to have a big if
in the case JDK
section and have two code paths (within the case) depending on whether using SunJSSE or IBMJSSE2. (I say that mainly because I don't believe Jetty ALPN works with IBMJSSE2.)
@@ -225,6 +226,18 @@ public static SslContextBuilder configure(SslContextBuilder builder, Provider jd | |||
throw new IllegalArgumentException( | |||
SUN_PROVIDER_NAME + " selected, but Jetty NPN/ALPN unavailable"); | |||
} | |||
} else if (IBM_PROVIDER_NAME.equals(jdkProvider.getName())) { | |||
// Jetty ALPN/NPN only supports one of NPN or ALPN | |||
if (JettyTlsUtil.isJettyAlpnConfigured()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it actually support Jetty ALPN? I don't see anything about Jetty ALPN working for IBM and based on the IBM documentation, it seems like Jetty ALPN only works if using Oracle's JDK on IBM hardware. It looks like it has its own API for Java 8 (which is very similar to Jetty NPN/ALPN).
I would really like to know which of these is being selected when it works for you. I hope it is the isJava9AlpnAvailable()
case.
My eventual goal in this part of the code was to use the Java 9+ API, when available, and check whether this specific provider supports ALPN by creating an sslEngine and calling getApplicationProtocol()
(or similar); if it throws UnsupportedOperationException, then it doesn't support ALPN. That way we don't have to hard-code provider names (for the Java 9 ALPN API).
SunJSSE is hard-coded today because it must be hard-coded for Jetty ALPN/NPN. Java 9+ support was a community contribution and just followed most of the current flow. Unfortunately, just being on Java 9+ doesn't mean the provider supports ALPN, thus the need for eventually adding a getApplicationProtocol()
check. But this had been "good enough" up until now since we know Java 9+'s SunJSSE implementation supports ALPN.
It also looks like we can add |
What's the status of this PR? Any interest? |
I just stumbled upon this and have a need for it as I'm using the IBM JDK, it seems like the solution might simply be to use |
Closing in favor of #7422 |
This commit addresses the issue: https://github.com/grpc/grpc-java/issues/5369
This commit improves gRPC to run gRPC server with TLS enabled on
AIX machines with IBMJSSE2 as their JDK SSL provider.