Skip to content

KAFKA-17833: Convert DescribeAuthorizedOperationsTest to use kraft#18252

Merged
mimaison merged 4 commits intoapache:trunkfrom
FrankYang0529:KAFKA-17833
Feb 7, 2025
Merged

KAFKA-17833: Convert DescribeAuthorizedOperationsTest to use kraft#18252
mimaison merged 4 commits intoapache:trunkfrom
FrankYang0529:KAFKA-17833

Conversation

@FrankYang0529
Copy link
Copy Markdown
Member

@FrankYang0529 FrankYang0529 commented Dec 18, 2024 •
edited
Loading

We disabled DescribeAuthorizedOperationsTest when implementing KAFKA-17614. Re-enable with a new test framework.

Committer Checklist (excluded from commit message)

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

@github-actions github-actions Bot added the core Kafka Broker label Dec 18, 2024
@FrankYang0529 FrankYang0529 changed the title Convert DescribeAuthorizedOperationsTest to use kraft KAFKA-17833: Convert DescribeAuthorizedOperationsTest to use kraft Dec 18, 2024
@FrankYang0529 FrankYang0529 force-pushed the KAFKA-17833 branch 6 times, most recently from 7259a8d to 426b733 Compare December 22, 2024 12:03
@FrankYang0529 FrankYang0529 force-pushed the KAFKA-17833 branch 3 times, most recently from 81d206d to 59066ef Compare January 8, 2025 15:53
@dajac
Copy link
Copy Markdown
Member

dajac commented Jan 21, 2025

@FrankYang0529 Could you please rebase the PR? I can help with reviews for this one.

@FrankYang0529
Copy link
Copy Markdown
Member Author

FrankYang0529 commented Jan 21, 2025

@dajac Thanks. Rebased trunk.

@FrankYang0529 FrankYang0529 requested a review from dajac January 23, 2025 05:44
@FrankYang0529 FrankYang0529 force-pushed the KAFKA-17833 branch 7 times, most recently from 50da078 to 5cc93e3 Compare January 30, 2025 00:50
@FrankYang0529
KAFKA-17833: Convert DescribeAuthorizedOperationsTest to use kraft
81504b5 
Signed-off-by: PoAn Yang <payang@apache.org>
mimaison
mimaison reviewed Feb 4, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@mimaison mimaison left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I left a few comments.

Comment thread core/src/test/java/kafka/admin/DescribeAuthorizedOperationsTest.java Outdated
return createAdminConfig(username, password, Map.of());
}

private Map<String, Object> createAdminConfig(String username, String password, Map<String, Object> additionalConfigs) {
Copy link
Copy Markdown
Member

@mimaison mimaison Feb 4, 2025

Choose a reason for hiding this comment

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

Not sure createAdminConfig() is the best name since this is also used to create consumer configs. What about createClientConfig() or createConfig()?

Copy link
Copy Markdown
Member Author

@FrankYang0529 FrankYang0529 Feb 6, 2025

Choose a reason for hiding this comment

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

We use alterConsumerGroupOffsets to create group, so we don't need this function. Thanks!

Comment thread core/src/test/java/kafka/admin/DescribeAuthorizedOperationsTest.java
private static final AccessControlEntry ALTER_ENTRY = createAccessControlEntry(JaasUtils.KAFKA_PLAIN_USER1, ALTER);
private static final AccessControlEntry DESCRIBE_ENTRY = createAccessControlEntry(JaasUtils.KAFKA_PLAIN_USER1, DESCRIBE);

static List<ClusterConfig> generator() {
Copy link
Copy Markdown
Member

@mimaison mimaison Feb 4, 2025

Choose a reason for hiding this comment

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

Is this now the preferred approach over using @ClusterTest?

Copy link
Copy Markdown
Member Author

@FrankYang0529 FrankYang0529 Feb 6, 2025

Choose a reason for hiding this comment

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

All cases in this class use same configuration. However, ClusterTestDefaults doesn't have brokerSecurityProtocol and controllerSecurityProtocol fields, so I use ClusterTemplate to avoid defining similar configuration in each case. How about adding these fields to ClusterTestDefaults in another Jira? WDYT? Thanks.

Comment thread core/src/test/java/kafka/admin/DescribeAuthorizedOperationsTest.java Outdated
Consumer<Byte, Byte> consumer3 = clusterInstance.consumer(createAdminConfig(JaasUtils.KAFKA_PLAIN_ADMIN, JaasUtils.KAFKA_PLAIN_ADMIN_PASSWORD, Map.of(ConsumerConfig.GROUP_ID_CONFIG, GROUP3)))
) {
// create consumers to avoid group not found error
consumer1.subscribe(List.of("topic1"));
Copy link
Copy Markdown
Member

@mimaison mimaison Feb 4, 2025

Choose a reason for hiding this comment

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

Could we use Admin.alterConsumerGroupOffsets() to create the consumer groups?

Copy link
Copy Markdown
Member Author

@FrankYang0529 FrankYang0529 Feb 6, 2025

Choose a reason for hiding this comment

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

Updated it. Thanks.

Comment thread core/src/test/java/kafka/admin/DescribeAuthorizedOperationsTest.java Outdated
public void testConsumerGroupAuthorizedOperations(ClusterInstance clusterInstance) {
setupSecurity(clusterInstance);
try (Admin admin = clusterInstance.admin(createAdminConfig(JaasUtils.KAFKA_PLAIN_ADMIN, JaasUtils.KAFKA_PLAIN_ADMIN_PASSWORD))) {
assertDoesNotThrow(() -> admin.createTopics(List.of(new NewTopic("topic1", 1, (short) 1))));
Copy link
Copy Markdown
Member

@mimaison mimaison Feb 4, 2025

Choose a reason for hiding this comment

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

Do we need these assertDoesNotThrow() calls everywhere? If an exception was thrown wouldn't it automatically fail the test anyway?

Copy link
Copy Markdown
Member Author

@FrankYang0529 FrankYang0529 Feb 6, 2025

Choose a reason for hiding this comment

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

Removed useless assertDoesNotThrow. Thanks.

Copy link
Copy Markdown
Member

@mimaison mimaison Feb 6, 2025

Choose a reason for hiding this comment

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

There are still a bunch of assertDoesNotThrow() calls. Are they needed?

Copy link
Copy Markdown
Member Author

@FrankYang0529 FrankYang0529 Feb 7, 2025

Choose a reason for hiding this comment

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

Removed it. Thanks.

@FrankYang0529
Merge branch 'trunk' into KAFKA-17833
1fe84a6
@FrankYang0529
address comment
5d63be1 
Signed-off-by: PoAn Yang <payang@apache.org>
@FrankYang0529
address comment
3c75740 
Signed-off-by: PoAn Yang <payang@apache.org>
mimaison
mimaison approved these changes Feb 7, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@mimaison mimaison left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR

@mimaison mimaison merged commit b22c7d5 into apache:trunk Feb 7, 2025
mimaison pushed a commit that referenced this pull request Feb 7, 2025
@FrankYang0529 @mimaison
KAFKA-17833: Convert DescribeAuthorizedOperationsTest to use KRaft (#…
b3837f8 
…18252)


Reviewers: Mickael Maison <mickael.maison@gmail.com>
@mimaison
Copy link
Copy Markdown
Member

mimaison commented Feb 7, 2025

Applied to 4.0 too: b3837f8

@FrankYang0529
Copy link
Copy Markdown
Member Author

FrankYang0529 commented Feb 7, 2025

Thanks for the review. 😄

@FrankYang0529 FrankYang0529 deleted the KAFKA-17833 branch February 7, 2025 14:54
pdruley pushed a commit to pdruley/kafka that referenced this pull request Feb 12, 2025
@FrankYang0529
KAFKA-17833: Convert DescribeAuthorizedOperationsTest to use KRaft (a…
4b10114 
…pache#18252)


Reviewers: Mickael Maison <mickael.maison@gmail.com>
manoj-mathivanan pushed a commit to manoj-mathivanan/kafka that referenced this pull request Feb 19, 2025
@FrankYang0529 @manoj-mathivanan
KAFKA-17833: Convert DescribeAuthorizedOperationsTest to use KRaft (a…
b3c6b8f 
…pache#18252)


Reviewers: Mickael Maison <mickael.maison@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mimaison mimaison mimaison approved these changes

@chia7712 chia7712 Awaiting requested review from chia7712

@dajac dajac Awaiting requested review from dajac

Assignees

No one assigned

Labels

core Kafka Broker

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@FrankYang0529 @dajac @mimaison