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
merged 12 commits into from Sep 18, 2018

Conversation

Projects
None yet
4 participants
@mutdmour
Contributor

mutdmour commented Sep 9, 2018

Changing limit of split to 2 values, to only split to key and value. if split and value is empty, also throw error.

  • Added test to check feature with delimiter (equal sign) in different pos
  • deleted test that enforced no multiple equal signs in properties
  • added tests for password overrides with symbols

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@mutdmour mutdmour changed the title from KAFKA-7388 equal sign in property value to KAFKA-7388 [WIP] equal sign in property value Sep 9, 2018

mutdmour added some commits Sep 9, 2018

@mutdmour mutdmour changed the title from KAFKA-7388 [WIP] equal sign in property value to KAFKA-7388 equal sign in property value for password Sep 12, 2018

def testParseArgsWithMultipleDelimiters() {
val argArray = Array("first.property==first","second.property=second=", "third.property=thi=rd")
val props = CommandLineUtils.parseKeyValueArgs(argArray, false)
assertEquals("Value of first property should be '=first'",props.getProperty("first.property"),"=first")

This comment has been minimized.

@omkreddy

omkreddy Sep 12, 2018

Contributor

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

This comment has been minimized.

@mutdmour

mutdmour Sep 12, 2018

Contributor

added 👍

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

This comment has been minimized.

@omkreddy

omkreddy Sep 12, 2018

Contributor

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

This comment has been minimized.

@muralimani

muralimani Sep 12, 2018

Yes, the else block is not required

This comment has been minimized.

@mutdmour

mutdmour Sep 12, 2018

Contributor

removed and added more test cases for more edge cases

val props = new Properties
for (a <- splits) {
if (a.length == 1) {
if (a.length == 1 || (a.length == 2 && a(1).length() == 0)) {

This comment has been minimized.

@omkreddy

omkreddy Sep 12, 2018

Contributor

we can use a(1).isEmpty() check.

This comment has been minimized.

@mutdmour

mutdmour Sep 12, 2018

Contributor

changed it 👍

@muralimani

The final else block is not required

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

This comment has been minimized.

@omkreddy

omkreddy Sep 13, 2018

Contributor

nit: add a space between method arguments at below lines

This comment has been minimized.

@mutdmour

mutdmour Sep 13, 2018

Contributor

fixed for the whole file

mutdmour added some commits Sep 13, 2018

@omkreddy

This comment has been minimized.

Contributor

omkreddy commented Sep 14, 2018

retest this please

@omkreddy

This comment has been minimized.

Contributor

omkreddy commented Sep 14, 2018

@rajinisivaram Can you take a look at this? often passwords contain an '=' character.

@mutdmour

This comment has been minimized.

Contributor

mutdmour commented Sep 14, 2018

retest this please

@omkreddy what would you like me to retest? for manual testing, I have run the shell command line tools with password override. build seems to be failing for other reasons.

@omkreddy

This comment has been minimized.

Contributor

omkreddy commented Sep 14, 2018

@mutdmour Above comment is not for you :) . By commenting "retest this please", we can re-trigger the Jenkins builds/tests.

@mutdmour

This comment has been minimized.

Contributor

mutdmour commented Sep 14, 2018

it passed

@rajinisivaram

@mutdmour Thanks for the PR, looks good. Left a couple of minor comments, apart from that LGTM.

@@ -60,19 +60,15 @@ object CommandLineUtils extends Logging {
* Parse key-value pairs in the form key=value

This comment has been minimized.

@rajinisivaram

rajinisivaram Sep 14, 2018

Contributor

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

This comment has been minimized.

@mutdmour

mutdmour Sep 14, 2018

Contributor

done

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

This comment has been minimized.

@rajinisivaram

rajinisivaram Sep 14, 2018

Contributor

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

This comment has been minimized.

@mutdmour

mutdmour Sep 14, 2018

Contributor

done

CommandLineUtils.parseKeyValueArgs(argArray, false)
val acceptMissingValue = false;
CommandLineUtils.parseKeyValueArgs(argArray, acceptMissingValue)

This comment has been minimized.

@rajinisivaram

rajinisivaram Sep 17, 2018

Contributor

@mutdmour In scala, you can write this as:

CommandLineUtils.parseKeyValueArgs(argArray, acceptMissingValue = false)

and

CommandLineUtils.parseKeyValueArgs(argArray, acceptMissingValue = true)

That makes the code more readable. Same for the other uses of CommandLineUtils.parseKeyValueArgs in this file with the boolean.

This comment has been minimized.

@mutdmour

mutdmour Sep 17, 2018

Contributor

oh I see. that's clearer. will fix it up.

@rajinisivaram

@mutdmour Thanks for the updates, LGTM. Merging to trunk.

@rajinisivaram rajinisivaram merged commit 43e2125 into apache:trunk Sep 18, 2018

2 checks passed

JDK 10 and Scala 2.12 SUCCESS 9503 tests run, 5 skipped, 0 failed.
Details
JDK 8 and Scala 2.11 SUCCESS 9503 tests run, 5 skipped, 0 failed.
Details

nimosunbit added a commit to sunbit-dev/kafka that referenced this pull request Nov 6, 2018

KAFKA-7388 equal sign in property value for password (apache#5630)
Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>, Murali Mani <murali.mani@gmail.com>, Rajini Sivaram <rajinisivaram@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment