-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
KAFKA-3070: SASL unit tests dont work with IBM JDK #2878
Conversation
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
@ijuma @rajinisivaram can you have a look ? |
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.
@mimaison Thank you for the PR. I have left a few minor comments. Do all Kafka tests run successfully with this PR with the IBM JDK?
@@ -166,7 +187,7 @@ object JaasTestUtils { | |||
keyTab = keytabLocation.getOrElse(throw new IllegalArgumentException("Keytab location not specified for GSSAPI")).getAbsolutePath, | |||
principal = KafkaServerPrincipal, | |||
debug = true, | |||
serviceName = Some("kafka")).toJaasModule | |||
serviceName = serviceName).toJaasModule |
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 there a reason for this change? Even with non-IBM module, serviceName
is optional in the JAAS config, and may be specified as a Kafka property instead.
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.
@rajinisivaram Within the current tests, serviceName was always provided so using the option looked to me as speculative generality
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.
It's not speculative generality, the field is optional as @rajinisivaram said, so the type reflects 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.
I'll revert that bit. Even though it's not currently used, it serves a purpose. Thanks
var module = "" | ||
var entries = Map( | ||
"principal" -> principal | ||
) |
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.
You could perhaps avoid the vars by creating JaasModule
with the appropriate options within the if-else blocks. The code may be more readable with a set of key-value pairs in each section, keeping the current non-IBM code as-is and adding a similar block for the IBM module.
case Some(properties) => properties | ||
case None => new Properties | ||
} | ||
if (JaasTestUtils.isIBMJdk) | ||
result.put(KafkaConfig.SaslKerberosServiceNameProp, JaasTestUtils.serviceName) |
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.
An alternative would be to define a method in JaasTestUtils
to get additional properties. That would keep JRE checks and knowledge of service name restrictions within JaasTestUtils
.
@@ -122,6 +136,20 @@ object JaasTestUtils { | |||
val KafkaScramAdmin = "scram-admin" | |||
val KafkaScramAdminPassword = "scram-admin-secret" | |||
|
|||
val isIBMJdk = System.getProperty("java.vendor").contains("IBM") |
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.
This method should probably be in the Java
class. It would be good to have a test with some of the existing java.vendor
strings.
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.
Good point!
@@ -166,7 +187,7 @@ object JaasTestUtils { | |||
keyTab = keytabLocation.getOrElse(throw new IllegalArgumentException("Keytab location not specified for GSSAPI")).getAbsolutePath, | |||
principal = KafkaServerPrincipal, | |||
debug = true, | |||
serviceName = Some("kafka")).toJaasModule | |||
serviceName = serviceName).toJaasModule |
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.
It's not speculative generality, the field is optional as @rajinisivaram said, so the type reflects that.
case None => new Properties | ||
} | ||
if (isIBMJdk) | ||
result.put(KafkaConfig.SaslKerberosServiceNameProp, JaasTestUtils.serviceName) |
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.
Ideally we would take a JaasSection as a parameter and retrieve the service name from there, but not sure how easy that is. If complicated, then this approach may work, but we should at least check if the config is already present before setting it. And we should have a comment explaining why this is needed.
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.
The issue is that with IBM's JDK we can't have serviceName in a JaasSection. This field is not standard and J9 throws an exception if it's present. So we have to pass service name as a Kafka config and rely on KerberosLogin.getServiceName() to retrieve the value at runtime.
I agree this should be documented
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.
Yeah, I knew the reason, it was just about documenting it for others. :)
Btw, please don't squash commits. It makes it hard to know what has changed since the previous review. |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
I've pushed an update that should address the reviews |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
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.
@mimaison Thank you for the updates. Left a couple of minor comments. They don't necessarily have to go into this PR, but may be useful to do.
@@ -41,4 +41,8 @@ private Java() { | |||
public static final boolean IS_JAVA9_COMPATIBLE = JVM_MAJOR_VERSION > 1 || | |||
(JVM_MAJOR_VERSION == 1 && JVM_MINOR_VERSION >= 9); | |||
|
|||
public static boolean isIBMJdk() { | |||
return System.getProperty("java.vendor").contains("IBM"); | |||
} |
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.
We use the same IBM JDK check in SaslChannelBuilder
and MiniKdc
. Perhaps those could be changed to use Java.isIBMJdk
as well? Doesn't have to be in this PR.
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.
Good catch ! I've updated these 2. Thanks
System.setProperty("java.vendor", "Oracle Corporation"); | ||
assertFalse(Java.isIBMJdk()); | ||
System.setProperty("java.vendor", "IBM Corporation"); | ||
assertTrue(Java.isIBMJdk()); |
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.
It may also be useful to have a test that checks that IBM Kerberos classes are available if Java.isIBMJdk
is true and com.sun
Kerberos classes are available if false. In particular, you could check the classes com.ibm.security.krb5.internal.Config
and sun.security.krb5.Config
which are loaded in SaslChannelBuilder
.
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.
@rajinisivaram Just for curiosity I tried the IBM MacOS SDK. It has both classes. And neither work 👎 with our test harness! Anyway that JDK is not our priority
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.
I've added a test that tries to load the correct LoginModule. Not super keen about it as it's only relevant when run on different JVMs. Let me know if it's what you had in mind
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.
@mimaison It is true that the test would check only one class depending on the JRE. But it checks that the relationship between java.vendor
and Kerberos classes matches the expectation in the code (for that JRE). The other unit test is checking if String comparison works, which is fine as a unit test, but it doesn't really test the actual System property based on the JRE.
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
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.
@mimaison Can you rebase, please? Thanks.
System.setProperty("java.vendor", "Oracle Corporation"); | ||
assertFalse(Java.isIBMJdk()); | ||
System.setProperty("java.vendor", "IBM Corporation"); | ||
assertTrue(Java.isIBMJdk()); |
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.
@mimaison It is true that the test would check only one class depending on the JRE. But it checks that the relationship between java.vendor
and Kerberos classes matches the expectation in the code (for that JRE). The other unit test is checking if String comparison works, which is fine as a unit test, but it doesn't really test the actual System property based on the JRE.
Use IBM Kerberos module for SASL tests if running on IBM JDK Developed with @edoardocomar Based on apache#738 by @rajinisivaram
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
@rajinisivaram I rebased and it's now clean to merge |
@mimaison Thanks for the PR. Merging to trunk. |
Use IBM Kerberos module for SASL tests if running on IBM JDK
Developed with @edoardocomar
Based on #738 by @rajinisivaram