Skip to content

KAFKA-15751, KAFKA-15752: Enable KRaft for BaseAdminIntegrationTest and SaslSslAdminIntegrationTest#15175

Merged
mimaison merged 2 commits intoapache:trunkfrom
tinaselenge:KAFKA-15751
Jun 17, 2024
Merged

KAFKA-15751, KAFKA-15752: Enable KRaft for BaseAdminIntegrationTest and SaslSslAdminIntegrationTest#15175
mimaison merged 2 commits intoapache:trunkfrom
tinaselenge:KAFKA-15751

Conversation

@tinaselenge
Copy link
Copy Markdown
Contributor

@tinaselenge tinaselenge commented Jan 11, 2024

  • KAFKA-15751: KRaft support in BaseAdminIntegrationTest
  • KAFKA-15752: KRaft support in SaslSslAdminIntegrationTest

Committer Checklist (excluded from commit message)

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

@mimaison
Copy link
Copy Markdown
Member

Thanks for the PR. The test failures look related as SaslSslAdminIntegrationTest and SslAdminIntegrationTest both extend BaseAdminIntegrationTest

@tinaselenge
Copy link
Copy Markdown
Contributor Author

tinaselenge commented Jan 16, 2024

Thanks for the PR. The test failures look related as SaslSslAdminIntegrationTest and SslAdminIntegrationTest both extend BaseAdminIntegrationTest

Thanks @mimaison for raising this. I am still looking into the failing tests.

@tinaselenge tinaselenge marked this pull request as draft January 17, 2024 14:10
@tinaselenge tinaselenge changed the title KAFKA-15751: KRaft support in BaseAdminIntegrationTest Enable KRaft for BaseAdminIntegrationTest and SaslSslAdminIntegrationTest Jan 29, 2024
@tinaselenge tinaselenge marked this pull request as ready for review February 9, 2024 15:55
@tinaselenge tinaselenge marked this pull request as draft February 15, 2024 16:15
@github-actions
Copy link
Copy Markdown

This PR is being marked as stale since it has not had any activity in 90 days. If you would like to keep this PR alive, please ask a committer for review. If the PR has merge conflicts, please update it with the latest from trunk (or appropriate release branch)

If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.

@github-actions github-actions bot added the stale Stale PRs label May 16, 2024
@github-actions github-actions bot removed the stale Stale PRs label May 29, 2024
@tinaselenge
Copy link
Copy Markdown
Contributor Author

Hi @CalvinConfluent, when enabling KRaft support for Admin integration tests, one of the tests failed (testAuthorizedOperations) due to how DescribeTopic API request is handled (introduced by KIP-966). The test is expecting a null authorizedOperations if includeAuthorizedOperations flag is not set when describing a topic partition. In zookeeper mode, the metadata response does not contain the authorizedOperations for the topic partitions, if this flag is not set. However, the response from the new DescribeTopic API, it seems to always include authorizedOperations, despite the flag. Is this an intentional change? It wasn't clear to me when reading the KIP. I think we should stay consistent with zookeeper behaviour. If this wasn't intentional, I can raise a Jira issue for it.

@CalvinLiu7947
Copy link
Copy Markdown
Contributor

Hi @tinaselenge The DescribeTopicPartitions API does not support ZK brokers[link]. The API should not be returned through ApiVersions API and UNSUPPORTED_VERSION error should be returned if a ZK broker receives DescribeTopicPartitions request.
If you discover the problem when using admin client. The expected behavior on the ZK brokers is that, the client will first try with DescribeTopicPartitions and receives UNSUPPORTED_VERSION. Then it will retry with Metadata Request.
Is the UNSUPPORTED_VERSION error in the response good enough to prevent checking authorizedOperations on ZK brokers?
On the other hand, the DescribeTopicPartitions API always includes authorizedOperations all the time on Kraft. Is there a reason why we want to avoid returning authorizedOperations?

@mimaison
Copy link
Copy Markdown
Member

Hi @CalvinConfluent,

Thanks for the clarification. The issue is that this causes a compatibility change. For example with the following code:

DescribeTopicsOptions options = new DescribeTopicsOptions().includeAuthorizedOperations(false);
TopicCollection topics = TopicCollection.ofTopicNames(Collections.singletonList(topic));
DescribeTopicsResult describeTopicsResult = admin.describeTopics(topics, options);
TopicDescription topicDescription = describeTopicsResult.topicNameValues().get(topic).get();
System.out.println(topicDescription.authorizedOperations());

In ZooKeeper mode, this prints null, while it prints [ALTER, READ, DELETE, ALTER_CONFIGS, CREATE, DESCRIBE_CONFIGS, WRITE, DESCRIBE] in KRaft mode.

In wonder if the getTopicDescriptionFromDescribeTopicsResponseTopic method should take into account the options provider by the caller to populate or not the authorizedOperations field of TopicDescription.

@mimaison
Copy link
Copy Markdown
Member

@tinaselenge
Copy link
Copy Markdown
Contributor Author

Thanks @mimaison raising the issue.

On the other hand, the DescribeTopicPartitions API always includes authorizedOperations all the time on Kraft. Is there a reason why we want to avoid returning authorizedOperations?

That is the a problem for this test as we now get 2 different behaviours when describing a topic partition with includeAuthorizedOperations set to false. In zk mode via metadata request, the response contains null authorised operations, but in Kraft mode via describeTopicPartition request, the response contains the authorised operations.

@tinaselenge tinaselenge changed the title Enable KRaft for BaseAdminIntegrationTest and SaslSslAdminIntegrationTest KAFKA-15751, KAFKA-15752: Enable KRaft for BaseAdminIntegrationTest and SaslSslAdminIntegrationTest May 30, 2024
@chia7712
Copy link
Copy Markdown
Member

On the other hand, the DescribeTopicPartitions API always includes authorizedOperations all the time on Kraft. Is there a reason why we want to avoid returning authorizedOperations?

Pardon me, why DescribeTopicPartitions request does not take a flag to indicate whether return authorizedOperations? Other req/res have flag to skip to return authorizedOperations, it seems DescribeTopicPartitions should support that for consistency?

the solution of #16136 is to filter authorizedOperations out on client-side, and that is ok to me. I'm just curios about the context :)

@tinaselenge tinaselenge marked this pull request as ready for review June 13, 2024 10:30
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/scala/integration/kafka/api/SaslSslAdminIntegrationTest.scala Outdated
Comment thread core/src/test/scala/integration/kafka/api/SaslSslAdminIntegrationTest.scala Outdated
@mimaison
Copy link
Copy Markdown
Member

The CI failed due to timeout. I kicked another build.

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

@mimaison
Copy link
Copy Markdown
Member

None of the test failures are related, merging to trunk

@mimaison mimaison merged commit 166d9e8 into apache:trunk Jun 17, 2024
@tinaselenge tinaselenge deleted the KAFKA-15751 branch June 18, 2024 09:56
TaiJuWu pushed a commit to TaiJuWu/kafka that referenced this pull request Jul 4, 2024
…nd SaslSslAdminIntegrationTest (apache#15175)


Reviewers: Mickael Maison <mickael.maison@gmail.com>
FrankYang0529 pushed a commit to FrankYang0529/kafka that referenced this pull request Sep 5, 2024
…nd SaslSslAdminIntegrationTest (apache#15175)

Reviewers: Mickael Maison <mickael.maison@gmail.com>
(cherry picked from commit 166d9e8)
FrankYang0529 pushed a commit to FrankYang0529/kafka that referenced this pull request Sep 5, 2024
…nd SaslSslAdminIntegrationTest (apache#15175)

Reviewers: Mickael Maison <mickael.maison@gmail.com>
(cherry picked from commit 166d9e8)
chia7712 pushed a commit that referenced this pull request Sep 8, 2024
…aft for BaseAdminIntegrationTest and SaslSslAdminIntegrationTest (#15175) (#17101)

(cherry picked from commit 166d9e8)

KAFKA-15751 and KAFKA-15752 enable the kraft mode in SaslSslAdminIntegrationTest, and that is useful in writing security-related IT. Without the backport, the fixes to security could be obstructed from backport due to IT (KAFKA-17315, for example).

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
chia7712 pushed a commit that referenced this pull request Sep 9, 2024
…aft for BaseAdminIntegrationTest and SaslSslAdminIntegrationTest (#15175) (#17102)

(cherry picked from commit 166d9e8)

KAFKA-15751 and KAFKA-15752 enable the kraft mode in SaslSslAdminIntegrationTest, and that is useful in writing security-related IT. Without the backport, the fixes to security could be obstructed from backport due to IT (KAFKA-17315, for example).

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants