Skip to content

KAFKA-19894: Reintroduce SaslPlainSslEndToEndAuthorizationTest #20915

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 19, 2025
Merged

KAFKA-19894: Reintroduce SaslPlainSslEndToEndAuthorizationTest #20915

merged 1 commit into from
Nov 19, 2025

Conversation

@gaurav-narula
Copy link
Contributor

@gaurav-narula gaurav-narula commented Nov 18, 2025 •
edited by github-actions bot
Loading

PR #17424 removed SaslPlainSslEndToEndAuthorizationTest along with
AclAuthorizer.

While there was a test within SaslPlainSslEndToEndAuthorizationTest
which tested ZK ACLs, it also tested all the tests in its inheritance
hierarchy.

We should therefore re-introduce it as the suite lacks a test for
SASL/PLAIN mechanism.

Reviewers: Chia-Ping Tsai [email protected], PoAn Yang
[email protected]

@gaurav-narula
KAFKA-19894: Reintroduce SaslPlainSslEndToEndAuthorizationTest
da2d3ce 
PR 17424 removed `SaslPlainSslEndToEndAuthorizationTest` along with `AclAuthorizer`.

While there was a test within `SaslPlainSslEndToEndAuthorizationTest` which tested ZK ACLs, it *also* tested all the tests in its inheritance hierarchy.

We should therefore re-introduce it as the suite lacks a test for `SASL/PLAIN` mechanism.
@github-actions github-actions bot added triage PRs from the community core Kafka Broker tests Test fixes (including flaky tests) labels Nov 18, 2025
@gaurav-narula
Copy link
Contributor Author

gaurav-narula commented Nov 18, 2025

CC: @mimaison @FrankYang0529

chia7712
chia7712 approved these changes Nov 18, 2025
View reviewed changes
Copy link
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

LGTM

There was a discussion about deleting this class 😄
https://github.com/apache/kafka/pull/17424/files#r1799467146

@gaurav-narula
Copy link
Contributor Author

gaurav-narula commented Nov 18, 2025

Thanks for the review @chia7712! Can you confirm if this is eligible to be backported to 4.0 and 4.1 as well?

@chia7712
Copy link
Member

chia7712 commented Nov 18, 2025

Can you confirm if this is eligible to be backported to 4.0 and 4.1 as well?

we can safely backport this patch since it does not change any production code. Also, it improves the test coverage 😄

FrankYang0529
FrankYang0529 approved these changes Nov 19, 2025
View reviewed changes
Copy link
Member

@FrankYang0529 FrankYang0529 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 adding the test case back.

@github-actions github-actions bot removed the triage PRs from the community label Nov 19, 2025
@chia7712
Copy link
Member

chia7712 commented Nov 19, 2025

/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=1 origin +77cca8ddac74b40d48faa35dd4ec1ae36fc61162:refs/remotes/pull/20915/merge
remote: Internal Server Error
Error: fatal: unable to access 'https://github.com/apache/kafka/': The requested URL returned error: 500

It looks like we were affected by the Cloudflare issue

@chia7712
Copy link
Member

chia7712 commented Nov 19, 2025

After looping the tests 20 times, all passed. proceeding with the merge

@chia7712 chia7712 merged commit 26e45f6 into apache:trunk Nov 19, 2025
20 of 21 checks passed
chia7712 pushed a commit to chia7712/kafka that referenced this pull request Nov 19, 2025
@gaurav-narula @chia7712
KAFKA-19894 Reintroduce SaslPlainSslEndToEndAuthorizationTest (apache…
a7c63f5 
…#20915)

PR apache#17424 removed `SaslPlainSslEndToEndAuthorizationTest` along with
`AclAuthorizer`.

While there was a test within `SaslPlainSslEndToEndAuthorizationTest`
which tested ZK ACLs, it *also* tested all the tests in its inheritance
hierarchy.

We should therefore re-introduce it as the suite lacks a test for
`SASL/PLAIN` mechanism.

Reviewers: Chia-Ping Tsai <[email protected]>, PoAn Yang
 <[email protected]>
chia7712 pushed a commit that referenced this pull request Nov 19, 2025
@gaurav-narula @chia7712
KAFKA-19894 Reintroduce SaslPlainSslEndToEndAuthorizationTest (#20915)
f49a2fd 
PR #17424 removed `SaslPlainSslEndToEndAuthorizationTest` along with
`AclAuthorizer`.

While there was a test within `SaslPlainSslEndToEndAuthorizationTest`
which tested ZK ACLs, it *also* tested all the tests in its inheritance
hierarchy.

We should therefore re-introduce it as the suite lacks a test for
`SASL/PLAIN` mechanism.

Reviewers: Chia-Ping Tsai <[email protected]>, PoAn Yang
 <[email protected]>
chia7712 pushed a commit that referenced this pull request Nov 19, 2025
@gaurav-narula @chia7712
KAFKA-19894 Reintroduce SaslPlainSslEndToEndAuthorizationTest (#20915)
5428c70 
PR #17424 removed `SaslPlainSslEndToEndAuthorizationTest` along with
`AclAuthorizer`.

While there was a test within `SaslPlainSslEndToEndAuthorizationTest`
which tested ZK ACLs, it *also* tested all the tests in its inheritance
hierarchy.

We should therefore re-introduce it as the suite lacks a test for
`SASL/PLAIN` mechanism.

Reviewers: Chia-Ping Tsai <[email protected]>, PoAn Yang
 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@FrankYang0529 FrankYang0529 FrankYang0529 approved these changes

@chia7712 chia7712 chia7712 approved these changes

Assignees

No one assigned

Labels

ci-approved core Kafka Broker tests Test fixes (including flaky tests)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gaurav-narula @chia7712 @FrankYang0529