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.

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-7388 equal sign in property value for password #5630

Merged
10 changes: 3 additions & 7 deletions core/src/main/scala/kafka/utils/CommandLineUtils.scala
Expand Up @@ -60,19 +60,15 @@ object CommandLineUtils extends Logging {
* Parse key-value pairs in the form key=value
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can you add a comment to say that value may contain equals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

*/
def parseKeyValueArgs(args: Iterable[String], acceptMissingValue: Boolean = true): Properties = {
val splits = args.map(_ split "=").filterNot(_.length == 0)
val splits = args.map(_.split("=", 2)).filterNot(_.length == 0)

val props = new Properties
for (a <- splits) {
if (a.length == 1) {
if (a.length == 1 || (a.length == 2 && a(1).isEmpty())) {
if (acceptMissingValue) props.put(a(0), "")
else throw new IllegalArgumentException(s"Missing value for key ${a(0)}")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

below else {} is not required as we always have length 2

Choose a reason for hiding this comment

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

Yes, the else block is not required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed and added more test cases for more edge cases

else if (a.length == 2) props.put(a(0), a(1))
else {
System.err.println("Invalid command line properties: " + args.mkString(" "))
Exit.exit(1)
}
else props.put(a(0), a(1))
}
props
}
Expand Down
23 changes: 17 additions & 6 deletions core/src/test/scala/unit/kafka/KafkaConfigTest.scala
Expand Up @@ -58,12 +58,6 @@ class KafkaTest {
assertEquals(util.Arrays.asList("compact","delete"), config4.logCleanupPolicy)
}

@Test(expected = classOf[FatalExitError])
def testGetKafkaConfigFromArgsWrongSetValue(): Unit = {
val propertiesFile = prepareDefaultConfig()
KafkaConfig.fromProps(Kafka.getPropsFromArgs(Array(propertiesFile, "--override", "a=b=c")))
}

@Test(expected = classOf[FatalExitError])
def testGetKafkaConfigFromArgsNonArgsAtTheEnd(): Unit = {
val propertiesFile = prepareDefaultConfig()
Expand Down Expand Up @@ -97,6 +91,23 @@ class KafkaTest {
assertEquals("truststore_password", config.getPassword(KafkaConfig.SslTruststorePasswordProp).value)
}

@Test
def testKafkaSslPasswordsWithSymbols(): Unit = {
val password = "=!#-+!?*/\"\'^%$=\\.,@:;="
val propertiesFile = prepareDefaultConfig()
val config = KafkaConfig.fromProps(Kafka.getPropsFromArgs(Array(propertiesFile,
"--override", "ssl.keystore.password=" + password,
"--override", "ssl.key.password=" + password,
"--override", "ssl.truststore.password=" + password)))
assertEquals(Password.HIDDEN, config.getPassword(KafkaConfig.SslKeyPasswordProp).toString)
assertEquals(Password.HIDDEN, config.getPassword(KafkaConfig.SslKeystorePasswordProp).toString)
assertEquals(Password.HIDDEN, config.getPassword(KafkaConfig.SslTruststorePasswordProp).toString)

assertEquals(password, config.getPassword(KafkaConfig.SslKeystorePasswordProp).value)
assertEquals(password, config.getPassword(KafkaConfig.SslKeyPasswordProp).value)
assertEquals(password, config.getPassword(KafkaConfig.SslTruststorePasswordProp).value)
}

def prepareDefaultConfig(): String = {
prepareConfig(Array("broker.id=1", "zookeeper.connect=somewhere"))
}
Expand Down
19 changes: 18 additions & 1 deletion core/src/test/scala/unit/kafka/utils/CommandLineUtilsTest.scala
Expand Up @@ -29,12 +29,19 @@ class CommandLineUtilsTest {
CommandLineUtils.parseKeyValueArgs(argArray, false)
}

@Test(expected = classOf[java.lang.IllegalArgumentException])
def testParseEmptyArgWithNoDelimiter() {
val argArray = Array("my.empty.property")
CommandLineUtils.parseKeyValueArgs(argArray, false)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Perhaps name the boolean arg here, acceptMissingValue = false? Could do the same in the existing method above as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

@Test
def testParseEmptyArgAsValid() {
val argArray = Array("my.empty.property=")
val argArray = Array("my.empty.property=", "my.empty.property1")
val props = CommandLineUtils.parseKeyValueArgs(argArray)

assertEquals("Value of a key with missing value should be an empty string",props.getProperty("my.empty.property"),"")
assertEquals("Value of a key with missing value with no delimiter should be an empty string",props.getProperty("my.empty.property1"),"")
}

@Test
Expand All @@ -52,4 +59,14 @@ class CommandLineUtilsTest {
assertEquals("Value of second property should be 'second'",props.getProperty("second.property"),"second")
}

@Test
def testParseArgsWithMultipleDelimiters() {
val argArray = Array("first.property==first","second.property=second=", "third.property=thi=rd")
val props = CommandLineUtils.parseKeyValueArgs(argArray, false)

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add a space between method arguments at below lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed for the whole file

assertEquals("Value of first property should be '=first'",props.getProperty("first.property"),"=first")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add a space between method arguments here and other places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added 👍

assertEquals("Value of second property should be 'second='",props.getProperty("second.property"),"second=")
assertEquals("Value of second property should be 'thi=rd'",props.getProperty("third.property"),"thi=rd")
}

}