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-8774: Regex can be found anywhere in config value #7197

Merged
merged 3 commits into from Aug 13, 2019

Conversation

@wicknicks
Copy link
Contributor

commented Aug 12, 2019

Signed-off-by: Arjun Satish arjun@confluent.io

The original method incorrectly used matcher.matches() instead of matcher.find(). The former method expects the entire string to match the regex, whereas the second one can find a pattern anywhere within the input string (which fits this use case more correctly).

Added unit tests.

Should be backported to 2.0.

Committer Checklist (excluded from commit message)

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

@wicknicks wicknicks force-pushed the wicknicks:KAFKA-8774 branch Aug 12, 2019

KAFKA-8774: Regex can be found anywhere in config value
Signed-off-by: Arjun Satish <arjun@confluent.io>

@wicknicks wicknicks force-pushed the wicknicks:KAFKA-8774 branch to 197021b Aug 12, 2019

@wicknicks

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@rhauch @avocader would appreciate a review. thanks!

@rhauch
Copy link
Contributor

left a comment

Great find, @wicknicks! I like the minimalist PR, but have a few suggestions for being a bit more thorough with the new unit tests.

KAFKA-8774: Add more cases for unit tests
Signed-off-by: Arjun Satish <arjun@confluent.io>

@wicknicks wicknicks force-pushed the wicknicks:KAFKA-8774 branch to 2d86b8e Aug 13, 2019

@rhauch
rhauch approved these changes Aug 13, 2019
Copy link
Contributor

left a comment

LGTM, pending a green build. Thanks, @wicknicks!

KAFKA-8774: Update system tests to cover case
Update system tests to cover case where config value contains
additional characters besides secret that requires regex pattern
to be found anywhere in the string (as opposed to complete match).

Signed-off-by: Arjun Satish <arjun@confluent.io>
@wicknicks

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

@rhauch
rhauch approved these changes Aug 13, 2019
Copy link
Contributor

left a comment

Thanks for updating the system test. LGTM!

@rhauch rhauch merged commit 7946372 into apache:trunk Aug 13, 2019

1 of 3 checks passed

JDK 11 and Scala 2.12 FAILURE 11799 tests run, 77 skipped, 1 failed.
Details
JDK 11 and Scala 2.13 FAILURE 10439 tests run, 65 skipped, 0 failed.
Details
JDK 8 and Scala 2.11 SUCCESS 11799 tests run, 77 skipped, 0 failed.
Details
rhauch added a commit that referenced this pull request Aug 13, 2019
KAFKA-8774: Regex can be found anywhere in config value (#7197)
Corrected the AbstractHerder to correctly identify task configs that contain variables for externalized secrets. The original method incorrectly used `matcher.matches()` instead of `matcher.find()`. The former method expects the entire string to match the regex, whereas the second one can find a pattern anywhere within the input string (which fits this use case more correctly).

Added unit tests to cover various cases of a config with externalized secrets, and updated system tests to cover case where config value contains additional characters besides secret that requires regex pattern to be found anywhere in the string (as opposed to complete match).

Author: Arjun Satish <arjun@confluent.io>
Reviewer: Randall Hauch <rhauch@gmail.com>
rhauch added a commit that referenced this pull request Aug 13, 2019
KAFKA-8774: Regex can be found anywhere in config value (#7197)
Corrected the AbstractHerder to correctly identify task configs that contain variables for externalized secrets. The original method incorrectly used `matcher.matches()` instead of `matcher.find()`. The former method expects the entire string to match the regex, whereas the second one can find a pattern anywhere within the input string (which fits this use case more correctly).

Added unit tests to cover various cases of a config with externalized secrets, and updated system tests to cover case where config value contains additional characters besides secret that requires regex pattern to be found anywhere in the string (as opposed to complete match).

Author: Arjun Satish <arjun@confluent.io>
Reviewer: Randall Hauch <rhauch@gmail.com>
rhauch added a commit that referenced this pull request Aug 13, 2019
KAFKA-8774: Regex can be found anywhere in config value (#7197)
Corrected the AbstractHerder to correctly identify task configs that contain variables for externalized secrets. The original method incorrectly used `matcher.matches()` instead of `matcher.find()`. The former method expects the entire string to match the regex, whereas the second one can find a pattern anywhere within the input string (which fits this use case more correctly).

Added unit tests to cover various cases of a config with externalized secrets, and updated system tests to cover case where config value contains additional characters besides secret that requires regex pattern to be found anywhere in the string (as opposed to complete match).

Author: Arjun Satish <arjun@confluent.io>
Reviewer: Randall Hauch <rhauch@gmail.com>
rhauch added a commit that referenced this pull request Aug 13, 2019
KAFKA-8774: Regex can be found anywhere in config value (#7197)
Corrected the AbstractHerder to correctly identify task configs that contain variables for externalized secrets. The original method incorrectly used `matcher.matches()` instead of `matcher.find()`. The former method expects the entire string to match the regex, whereas the second one can find a pattern anywhere within the input string (which fits this use case more correctly).

Added unit tests to cover various cases of a config with externalized secrets, and updated system tests to cover case where config value contains additional characters besides secret that requires regex pattern to be found anywhere in the string (as opposed to complete match).

Author: Arjun Satish <arjun@confluent.io>
Reviewer: Randall Hauch <rhauch@gmail.com>

@wicknicks wicknicks deleted the wicknicks:KAFKA-8774 branch Aug 13, 2019

xiowu0 added a commit to linkedin/kafka that referenced this pull request Aug 22, 2019
[LI-CHERRY-PICK] [b85707c] KAFKA-8774: Regex can be found anywhere in…
… config value (apache#7197)

TICKET = KAFKA-8774
LI_DESCRIPTION =
EXIT_CRITERIA = HASH [b85707c]
ORIGINAL_DESCRIPTION =

Corrected the AbstractHerder to correctly identify task configs that contain variables for externalized secrets. The original method incorrectly used `matcher.matches()` instead of `matcher.find()`. The former method expects the entire string to match the regex, whereas the second one can find a pattern anywhere within the input string (which fits this use case more correctly).

Added unit tests to cover various cases of a config with externalized secrets, and updated system tests to cover case where config value contains additional characters besides secret that requires regex pattern to be found anywhere in the string (as opposed to complete match).

Author: Arjun Satish <arjun@confluent.io>
Reviewer: Randall Hauch <rhauch@gmail.com>
(cherry picked from commit b85707c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.