-
Notifications
You must be signed in to change notification settings - Fork 15.2k
KAFKA-20617: add validation for cluster-id in Formatter #22384
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -587,4 +587,28 @@ public void testFormatWithNoInitialControllers() throws Exception { | |
| assertNotNull(logDirProps1); | ||
| } | ||
| } | ||
|
|
||
| @ParameterizedTest | ||
| @ValueSource(strings = {"unrvTtQISjar0JUWGU/8Pg", "igNUVIdeSPO5JCZYFhOh7Q==", "AAAAAAAAAAAAAAAAAAAAAA", "AAAAAAAAAAAAAAAAAAAAAQ"}) | ||
| public void testFormatWithInvalidClusterId(String clusterId) throws Exception { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no corresponding positive test showing that a valid CID passes validation. This would guard against the validation being too aggressive. |
||
| try (TestEnv testEnv = new TestEnv(2)) { | ||
| FormatterContext formatter1 = testEnv.newFormatter(); | ||
| formatter1.formatter.setClusterId(clusterId); | ||
| String expectedPrefix = "The specified cluster id, " + clusterId; | ||
| assertEquals(expectedPrefix, | ||
| assertThrows(FormatterException.class, | ||
| formatter1.formatter::run). | ||
| getMessage().substring(0, expectedPrefix.length())); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't be easier to rewrite like: assertTrue(message.startsWith(expectedPrefix))
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm indifferent here - I followed the convention used in another test in the same class |
||
| } | ||
| } | ||
|
|
||
| @ParameterizedTest | ||
| @ValueSource(strings = {"iIygKoNNSpGzNeAEr_QW7w", "-w_KF-snTmuEnKPqZ0RDzA", "dZMgZA7nTLqZTdzb0zbvSQ", "hxqHyN2OSSmPajVhm_DR-Q"}) | ||
| public void testFormatWithValidClusterId(String clusterId) throws Exception { | ||
| try (TestEnv testEnv = new TestEnv(2)) { | ||
| FormatterContext formatter1 = testEnv.newFormatter(); | ||
| formatter1.formatter.setClusterId(clusterId); | ||
| formatter1.formatter.run(); | ||
| } | ||
| } | ||
| } | ||
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.
Let's also validate the starting
-sign.Uh oh!
There was an error while loading. Please reload this page.
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 think these are cosmetic improvemenst that've been added over time but have no bearing on correctness. Failing validation on them would hinder migration from older clusters where
-was allowed.The motivation for avoiding
-in the beginning was to avoid shell escaping issues when passing cluster id in CLI tools. https://issues.apache.org/jira/browse/KAFKA-13741Lately, https://issues.apache.org/jira/browse/KAFKA-20072 avoided
-altogether to allow easier copy-pasting.Uh oh!
There was an error while loading. Please reload this page.
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. Let's add a comment about this for future reference.