Enable KRaft for BaseAdminIntegrationTest and SaslSslAdminIntegrationTest

[tinaselenge]

• Refactor SSL/SASL admin integration tests to not use a custom authorizer.

This makes the test simpler to enable KRaft. Also unclear why a custom authoriser had to be used from the commit made this change and tests seem to work without it. The initial ACLs required by the tests are created with a super admin client (replacing AclAuthorizationAdmin). IDEMPOTENT_WRITE was added to the initial ACLs created in the SASL setup, because SSL and BaseAdmin tests that being extended or extending this class do not override the configuredClusterPermissions method resulting in mismatching the expected permissions set on the cluster initially. Overriding this method does not seem necessary otherwise, hence it was removed from the SASL test.

• 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]

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

[tinaselenge]

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.

[github-actions[bot]]

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.

[tinaselenge]

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.

[CalvinConfluent]

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]

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]

I created https://issues.apache.org/jira/browse/KAFKA-16865

[tinaselenge]

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.

[chia7712]

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 :)

[mimaison]

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

[mimaison]

None of the test failures are related, merging to trunk