KAFKA-3279: Remove checks for JAAS system property #967
Conversation
|
You mean, one could set it via |
|
cc @fpj |
|
@ijuma Yes, exactly. |
|
Makes sense. |
|
+1 |
| Configuration loginConf = Configuration.getConfiguration(); | ||
| isSecurityEnabled = loginConf.getAppConfigurationEntry(zkLoginContextName) != null; | ||
| } catch (Exception e) { | ||
| throw new KafkaException(e); |
There was a problem hiding this comment.
I think we should provide some context on the exception here.
|
LGTM but it would be good to get a second review from @fpj as he worked on this code. |
java.security.auth.login.config
rajinisivaram
force-pushed
the
KAFKA-3279
branch
from
3492eea
to
e73390d
Compare
|
@fpj Do you have time to review this? Thank you. |
| @@ -88,25 +87,17 @@ public static boolean isZkSecurityEnabled() { | |||
| boolean zkSaslEnabled = Boolean.parseBoolean(System.getProperty(ZK_SASL_CLIENT, "true")); | |||
| String zkLoginContextName = System.getProperty(ZK_LOGIN_CONTEXT_NAME_KEY, "Client"); | |||
|
|
|||
| String loginConfigFile = System.getProperty(JAVA_LOGIN_CONFIG_PARAM); | |||
| if (loginConfigFile != null && loginConfigFile.length() > 0) { | |||
There was a problem hiding this comment.
@rajinisivaram It sounds ok to remove this verification, if the configuration isn't right, then isSecurityEnabled below will be false, is that right?
There was a problem hiding this comment.
@fpj Yes, that is correct. If the configuration is not set using the property or using some other mechanism, then isSecurityEnabled below will be false.
|
+1, LGTM |
|
@harshach, that's 3 reviews including your own. Would you like to merge this? |
|
LGTM, and validated w/ some of the system tests as well. Thanks @rajinisivaram! |
JAAS configuration may be set using other methods and hence the check for System property doesn't always match where the actual configuration used by Kafka is loaded from.