You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "tinaselenge (via GitHub)" <gi...@apache.org> on 2024/05/30 12:52:54 UTC
[PR] Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest [kafka]
tinaselenge opened a new pull request, #16136:
URL: https://github.com/apache/kafka/pull/16136
*More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.*
*Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.*
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on code in PR #16136:
URL: https://github.com/apache/kafka/pull/16136#discussion_r1620732915
##########
clients/src/main/resources/common/message/DescribeTopicPartitionsRequest.json:
##########
@@ -30,6 +30,8 @@
},
{ "name": "ResponsePartitionLimit", "type": "int32", "versions": "0+", "default": "2000",
"about": "The maximum number of partitions included in the response." },
+ { "name": "IncludeTopicAuthorizedOperations", "type": "bool", "versions": "1+",
Review Comment:
I think you're along the right lines, but I think there's a problem with this approach. KIP-966 introduced the DescribeTopicPartitions RPC and it does indeed not include `IncludeTopicAuthorizedOperations`. But that KIP introduced v0 of the RPC, and it would take another KIP to introduce v1. It's quite hard to figure out which KIP introduced which RPC versions, so I might have missed it.
I suggest a code-only change in the admin client which only fills in the authorized operations given to the client if the client did request them. Of course, the RPC response contains them regardless of whether they were requested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison merged PR #16136:
URL: https://github.com/apache/kafka/pull/16136
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #16136:
URL: https://github.com/apache/kafka/pull/16136#discussion_r1634645333
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -1464,13 +1464,54 @@ public void testDescribeTopicsWithDescribeTopicPartitionsApiBasic() {
assertEquals(0, topicDescription.partitions().get(0).partition());
assertEquals(1, topicDescription.partitions().get(1).partition());
topicDescription = topicDescriptions.get(topicName1);
+ assertNull(topicDescription.authorizedOperations());
assertEquals(1, topicDescription.partitions().size());
} catch (Exception e) {
fail("describe using DescribeTopics API should not fail", e);
}
}
}
+ @Test
+ public void testDescribeTopicPartitionsApiWithAuthorizedOps() {
+ try (AdminClientUnitTestEnv env = mockClientEnv()) {
+ env.kafkaClient().setNodeApiVersions(NodeApiVersions.create());
+ String topicName0 = "test-0";
+ Uuid topicId = Uuid.randomUuid();
+
+ int authorisedOperations = 1 << AclOperation.DESCRIBE.code() | 1 << AclOperation.ALTER.code();
+ env.kafkaClient().prepareResponse(
+ prepareDescribeClusterResponse(0,
+ env.cluster().nodes(),
+ env.cluster().clusterResource().clusterId(),
+ 2,
+ authorisedOperations)
+ );
+
+ DescribeTopicPartitionsResponseData responseData = new DescribeTopicPartitionsResponseData();
+ responseData.topics().add(new DescribeTopicPartitionsResponseTopic()
+ .setErrorCode((short) 0)
+ .setTopicId(topicId)
+ .setName(topicName0)
+ .setIsInternal(false)
+ .setTopicAuthorizedOperations(authorisedOperations));
+ env.kafkaClient().prepareResponse(new DescribeTopicPartitionsResponse(responseData));
+
+ try {
+ DescribeTopicsResult result = env.adminClient().describeTopics(
+ Arrays.asList(topicName0), new DescribeTopicsOptions().includeAuthorizedOperations(true)
Review Comment:
`asList()` is statically imported, so you can drop the `Arrays.` prefix. Same below
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -1464,13 +1464,54 @@ public void testDescribeTopicsWithDescribeTopicPartitionsApiBasic() {
assertEquals(0, topicDescription.partitions().get(0).partition());
assertEquals(1, topicDescription.partitions().get(1).partition());
topicDescription = topicDescriptions.get(topicName1);
+ assertNull(topicDescription.authorizedOperations());
assertEquals(1, topicDescription.partitions().size());
} catch (Exception e) {
fail("describe using DescribeTopics API should not fail", e);
}
}
}
+ @Test
+ public void testDescribeTopicPartitionsApiWithAuthorizedOps() {
+ try (AdminClientUnitTestEnv env = mockClientEnv()) {
+ env.kafkaClient().setNodeApiVersions(NodeApiVersions.create());
+ String topicName0 = "test-0";
+ Uuid topicId = Uuid.randomUuid();
+
+ int authorisedOperations = 1 << AclOperation.DESCRIBE.code() | 1 << AclOperation.ALTER.code();
+ env.kafkaClient().prepareResponse(
+ prepareDescribeClusterResponse(0,
+ env.cluster().nodes(),
+ env.cluster().clusterResource().clusterId(),
+ 2,
+ authorisedOperations)
+ );
+
+ DescribeTopicPartitionsResponseData responseData = new DescribeTopicPartitionsResponseData();
+ responseData.topics().add(new DescribeTopicPartitionsResponseTopic()
+ .setErrorCode((short) 0)
+ .setTopicId(topicId)
+ .setName(topicName0)
+ .setIsInternal(false)
+ .setTopicAuthorizedOperations(authorisedOperations));
+ env.kafkaClient().prepareResponse(new DescribeTopicPartitionsResponse(responseData));
+
+ try {
Review Comment:
We don't need this try/catch, any exceptions thrown should fail the test. I understand you copied the format from the existing tests above. If you prefer not changing this in this PR, we can address it in a follow up PR.
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -1464,13 +1464,54 @@ public void testDescribeTopicsWithDescribeTopicPartitionsApiBasic() {
assertEquals(0, topicDescription.partitions().get(0).partition());
assertEquals(1, topicDescription.partitions().get(1).partition());
topicDescription = topicDescriptions.get(topicName1);
+ assertNull(topicDescription.authorizedOperations());
assertEquals(1, topicDescription.partitions().size());
} catch (Exception e) {
fail("describe using DescribeTopics API should not fail", e);
}
}
}
+ @Test
+ public void testDescribeTopicPartitionsApiWithAuthorizedOps() {
+ try (AdminClientUnitTestEnv env = mockClientEnv()) {
+ env.kafkaClient().setNodeApiVersions(NodeApiVersions.create());
+ String topicName0 = "test-0";
+ Uuid topicId = Uuid.randomUuid();
+
+ int authorisedOperations = 1 << AclOperation.DESCRIBE.code() | 1 << AclOperation.ALTER.code();
Review Comment:
Here you can use `Utils.to32BitField(Utils.mkSet(AclOperation.DESCRIBE.code(), AclOperation.ALTER.code()));` instead of doing the byte operations yourself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest [kafka]
Posted by "tinaselenge (via GitHub)" <gi...@apache.org>.
tinaselenge commented on code in PR #16136:
URL: https://github.com/apache/kafka/pull/16136#discussion_r1636225411
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -1464,13 +1464,50 @@ public void testDescribeTopicsWithDescribeTopicPartitionsApiBasic() {
assertEquals(0, topicDescription.partitions().get(0).partition());
assertEquals(1, topicDescription.partitions().get(1).partition());
topicDescription = topicDescriptions.get(topicName1);
+ assertNull(topicDescription.authorizedOperations());
assertEquals(1, topicDescription.partitions().size());
} catch (Exception e) {
fail("describe using DescribeTopics API should not fail", e);
}
}
}
+ @Test
+ public void testDescribeTopicPartitionsApiWithAuthorizedOps() throws ExecutionException, InterruptedException {
+ try (AdminClientUnitTestEnv env = mockClientEnv()) {
+ env.kafkaClient().setNodeApiVersions(NodeApiVersions.create());
+ String topicName0 = "test-0";
+ Uuid topicId = Uuid.randomUuid();
+
+ int authorisedOperations = Utils.to32BitField(Utils.mkSet(AclOperation.DESCRIBE.code(), AclOperation.ALTER.code()));
+ env.kafkaClient().prepareResponse(
+ prepareDescribeClusterResponse(0,
+ env.cluster().nodes(),
+ env.cluster().clusterResource().clusterId(),
+ 2,
+ authorisedOperations)
+ );
+
+ DescribeTopicPartitionsResponseData responseData = new DescribeTopicPartitionsResponseData();
+ responseData.topics().add(new DescribeTopicPartitionsResponseTopic()
+ .setErrorCode((short) 0)
+ .setTopicId(topicId)
+ .setName(topicName0)
+ .setIsInternal(false)
+ .setTopicAuthorizedOperations(authorisedOperations));
+ env.kafkaClient().prepareResponse(new DescribeTopicPartitionsResponse(responseData));
+
+ DescribeTopicsResult result = env.adminClient().describeTopics(
+ singletonList(topicName0), new DescribeTopicsOptions().includeAuthorizedOperations(true)
+ );
+
+ Map<String, TopicDescription> topicDescriptions = result.allTopicNames().get();
+ TopicDescription topicDescription = topicDescriptions.get(topicName0);
+ assertEquals(new HashSet<>(asList(AclOperation.DESCRIBE, AclOperation.ALTER)),
Review Comment:
@chia7712 that's a good point! Thanks. I added another test as suggested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #16136:
URL: https://github.com/apache/kafka/pull/16136#discussion_r1635939546
##########
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##########
@@ -2404,14 +2404,16 @@ void handleFailure(Throwable throwable) {
private TopicDescription getTopicDescriptionFromDescribeTopicsResponseTopic(
DescribeTopicPartitionsResponseTopic topic,
- Map<Integer, Node> nodes
+ Map<Integer, Node> nodes,
+ boolean includeAuthorizedOperations
) {
List<DescribeTopicPartitionsResponsePartition> partitionInfos = topic.partitions();
List<TopicPartitionInfo> partitions = new ArrayList<>(partitionInfos.size());
for (DescribeTopicPartitionsResponsePartition partitionInfo : partitionInfos) {
partitions.add(DescribeTopicPartitionsResponse.partitionToTopicPartitionInfo(partitionInfo, nodes));
}
- return new TopicDescription(topic.name(), topic.isInternal(), partitions, validAclOperations(topic.topicAuthorizedOperations()), topic.topicId());
+ Set<AclOperation> authorisedOperations = includeAuthorizedOperations ? validAclOperations(topic.topicAuthorizedOperations()) : null;
Review Comment:
This was briefly discussed in https://github.com/apache/kafka/pull/15175#issuecomment-2137870907 where we identified this issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #16136:
URL: https://github.com/apache/kafka/pull/16136#discussion_r1634876940
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -1464,13 +1464,54 @@ public void testDescribeTopicsWithDescribeTopicPartitionsApiBasic() {
assertEquals(0, topicDescription.partitions().get(0).partition());
assertEquals(1, topicDescription.partitions().get(1).partition());
topicDescription = topicDescriptions.get(topicName1);
+ assertNull(topicDescription.authorizedOperations());
assertEquals(1, topicDescription.partitions().size());
} catch (Exception e) {
fail("describe using DescribeTopics API should not fail", e);
}
}
}
+ @Test
+ public void testDescribeTopicPartitionsApiWithAuthorizedOps() {
+ try (AdminClientUnitTestEnv env = mockClientEnv()) {
+ env.kafkaClient().setNodeApiVersions(NodeApiVersions.create());
+ String topicName0 = "test-0";
+ Uuid topicId = Uuid.randomUuid();
+
+ int authorisedOperations = 1 << AclOperation.DESCRIBE.code() | 1 << AclOperation.ALTER.code();
+ env.kafkaClient().prepareResponse(
+ prepareDescribeClusterResponse(0,
+ env.cluster().nodes(),
+ env.cluster().clusterResource().clusterId(),
+ 2,
+ authorisedOperations)
+ );
+
+ DescribeTopicPartitionsResponseData responseData = new DescribeTopicPartitionsResponseData();
+ responseData.topics().add(new DescribeTopicPartitionsResponseTopic()
+ .setErrorCode((short) 0)
+ .setTopicId(topicId)
+ .setName(topicName0)
+ .setIsInternal(false)
+ .setTopicAuthorizedOperations(authorisedOperations));
+ env.kafkaClient().prepareResponse(new DescribeTopicPartitionsResponse(responseData));
+
+ try {
Review Comment:
We can do that in another PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #16136:
URL: https://github.com/apache/kafka/pull/16136#issuecomment-2163319999
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on PR #16136:
URL: https://github.com/apache/kafka/pull/16136#issuecomment-2158818059
@tinaselenge Are you going to be able to add a unit test? It would be good to get this PR into 3.8.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest [kafka]
Posted by "tinaselenge (via GitHub)" <gi...@apache.org>.
tinaselenge commented on code in PR #16136:
URL: https://github.com/apache/kafka/pull/16136#discussion_r1634840314
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -1464,13 +1464,54 @@ public void testDescribeTopicsWithDescribeTopicPartitionsApiBasic() {
assertEquals(0, topicDescription.partitions().get(0).partition());
assertEquals(1, topicDescription.partitions().get(1).partition());
topicDescription = topicDescriptions.get(topicName1);
+ assertNull(topicDescription.authorizedOperations());
assertEquals(1, topicDescription.partitions().size());
} catch (Exception e) {
fail("describe using DescribeTopics API should not fail", e);
}
}
}
+ @Test
+ public void testDescribeTopicPartitionsApiWithAuthorizedOps() {
+ try (AdminClientUnitTestEnv env = mockClientEnv()) {
+ env.kafkaClient().setNodeApiVersions(NodeApiVersions.create());
+ String topicName0 = "test-0";
+ Uuid topicId = Uuid.randomUuid();
+
+ int authorisedOperations = 1 << AclOperation.DESCRIBE.code() | 1 << AclOperation.ALTER.code();
+ env.kafkaClient().prepareResponse(
+ prepareDescribeClusterResponse(0,
+ env.cluster().nodes(),
+ env.cluster().clusterResource().clusterId(),
+ 2,
+ authorisedOperations)
+ );
+
+ DescribeTopicPartitionsResponseData responseData = new DescribeTopicPartitionsResponseData();
+ responseData.topics().add(new DescribeTopicPartitionsResponseTopic()
+ .setErrorCode((short) 0)
+ .setTopicId(topicId)
+ .setName(topicName0)
+ .setIsInternal(false)
+ .setTopicAuthorizedOperations(authorisedOperations));
+ env.kafkaClient().prepareResponse(new DescribeTopicPartitionsResponse(responseData));
+
+ try {
Review Comment:
I made the suggested change for the new test. Should it be removed from the existing tests? I can do that in a follow up PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest [kafka]
Posted by "tinaselenge (via GitHub)" <gi...@apache.org>.
tinaselenge commented on code in PR #16136:
URL: https://github.com/apache/kafka/pull/16136#discussion_r1620819925
##########
clients/src/main/resources/common/message/DescribeTopicPartitionsRequest.json:
##########
@@ -30,6 +30,8 @@
},
{ "name": "ResponsePartitionLimit", "type": "int32", "versions": "0+", "default": "2000",
"about": "The maximum number of partitions included in the response." },
+ { "name": "IncludeTopicAuthorizedOperations", "type": "bool", "versions": "1+",
Review Comment:
Thanks Andrew for pointing it out. You are right, this can be solved by code-only change in the admin client change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest [kafka]
Posted by "tinaselenge (via GitHub)" <gi...@apache.org>.
tinaselenge commented on PR #16136:
URL: https://github.com/apache/kafka/pull/16136#issuecomment-2160342680
Thanks everyone for reviewing the PR. I have now added an unit test for the change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #16136:
URL: https://github.com/apache/kafka/pull/16136#discussion_r1635282686
##########
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##########
@@ -2404,14 +2404,16 @@ void handleFailure(Throwable throwable) {
private TopicDescription getTopicDescriptionFromDescribeTopicsResponseTopic(
DescribeTopicPartitionsResponseTopic topic,
- Map<Integer, Node> nodes
+ Map<Integer, Node> nodes,
+ boolean includeAuthorizedOperations
) {
List<DescribeTopicPartitionsResponsePartition> partitionInfos = topic.partitions();
List<TopicPartitionInfo> partitions = new ArrayList<>(partitionInfos.size());
for (DescribeTopicPartitionsResponsePartition partitionInfo : partitionInfos) {
partitions.add(DescribeTopicPartitionsResponse.partitionToTopicPartitionInfo(partitionInfo, nodes));
}
- return new TopicDescription(topic.name(), topic.isInternal(), partitions, validAclOperations(topic.topicAuthorizedOperations()), topic.topicId());
+ Set<AclOperation> authorisedOperations = includeAuthorizedOperations ? validAclOperations(topic.topicAuthorizedOperations()) : null;
Review Comment:
Pardon me, why we don't make `DescribeTopicPartitionsRequest` support `IncludeAuthorizedOperations` (similar to `MetadataRequest`, `DescribeGroupsRequest,` and `ConsumerGroupDescribeRequest`)?
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -1464,13 +1464,50 @@ public void testDescribeTopicsWithDescribeTopicPartitionsApiBasic() {
assertEquals(0, topicDescription.partitions().get(0).partition());
assertEquals(1, topicDescription.partitions().get(1).partition());
topicDescription = topicDescriptions.get(topicName1);
+ assertNull(topicDescription.authorizedOperations());
assertEquals(1, topicDescription.partitions().size());
} catch (Exception e) {
fail("describe using DescribeTopics API should not fail", e);
}
}
}
+ @Test
+ public void testDescribeTopicPartitionsApiWithAuthorizedOps() throws ExecutionException, InterruptedException {
+ try (AdminClientUnitTestEnv env = mockClientEnv()) {
+ env.kafkaClient().setNodeApiVersions(NodeApiVersions.create());
+ String topicName0 = "test-0";
+ Uuid topicId = Uuid.randomUuid();
+
+ int authorisedOperations = Utils.to32BitField(Utils.mkSet(AclOperation.DESCRIBE.code(), AclOperation.ALTER.code()));
+ env.kafkaClient().prepareResponse(
+ prepareDescribeClusterResponse(0,
+ env.cluster().nodes(),
+ env.cluster().clusterResource().clusterId(),
+ 2,
+ authorisedOperations)
+ );
+
+ DescribeTopicPartitionsResponseData responseData = new DescribeTopicPartitionsResponseData();
+ responseData.topics().add(new DescribeTopicPartitionsResponseTopic()
+ .setErrorCode((short) 0)
+ .setTopicId(topicId)
+ .setName(topicName0)
+ .setIsInternal(false)
+ .setTopicAuthorizedOperations(authorisedOperations));
+ env.kafkaClient().prepareResponse(new DescribeTopicPartitionsResponse(responseData));
+
+ DescribeTopicsResult result = env.adminClient().describeTopics(
+ singletonList(topicName0), new DescribeTopicsOptions().includeAuthorizedOperations(true)
+ );
+
+ Map<String, TopicDescription> topicDescriptions = result.allTopicNames().get();
+ TopicDescription topicDescription = topicDescriptions.get(topicName0);
+ assertEquals(new HashSet<>(asList(AclOperation.DESCRIBE, AclOperation.ALTER)),
Review Comment:
This test case is great. It tests the case that server returns `topicAuthorizedOperations` and `includeAuthorizedOperations=true`.
However, it seems to me the issue is that we don't filter `topicAuthorizedOperations` out when `includeAuthorizedOperations=false`. Other test cases having `assertNull(topicDescription.authorizedOperations())` don't prepare `topicAuthorizedOperations` to response so the scenario is different to the issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest [kafka]
Posted by "tinaselenge (via GitHub)" <gi...@apache.org>.
tinaselenge commented on PR #16136:
URL: https://github.com/apache/kafka/pull/16136#issuecomment-2160125219
Sorry, yes, I will add it in today.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest [kafka]
Posted by "tinaselenge (via GitHub)" <gi...@apache.org>.
tinaselenge commented on PR #16136:
URL: https://github.com/apache/kafka/pull/16136#issuecomment-2163350503
Thank @mimaison!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #16136:
URL: https://github.com/apache/kafka/pull/16136#discussion_r1634876424
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -1464,13 +1464,54 @@ public void testDescribeTopicsWithDescribeTopicPartitionsApiBasic() {
assertEquals(0, topicDescription.partitions().get(0).partition());
assertEquals(1, topicDescription.partitions().get(1).partition());
topicDescription = topicDescriptions.get(topicName1);
+ assertNull(topicDescription.authorizedOperations());
assertEquals(1, topicDescription.partitions().size());
} catch (Exception e) {
fail("describe using DescribeTopics API should not fail", e);
}
}
}
+ @Test
+ public void testDescribeTopicPartitionsApiWithAuthorizedOps() {
+ try (AdminClientUnitTestEnv env = mockClientEnv()) {
+ env.kafkaClient().setNodeApiVersions(NodeApiVersions.create());
+ String topicName0 = "test-0";
+ Uuid topicId = Uuid.randomUuid();
+
+ int authorisedOperations = 1 << AclOperation.DESCRIBE.code() | 1 << AclOperation.ALTER.code();
+ env.kafkaClient().prepareResponse(
+ prepareDescribeClusterResponse(0,
+ env.cluster().nodes(),
+ env.cluster().clusterResource().clusterId(),
+ 2,
+ authorisedOperations)
+ );
+
+ DescribeTopicPartitionsResponseData responseData = new DescribeTopicPartitionsResponseData();
+ responseData.topics().add(new DescribeTopicPartitionsResponseTopic()
+ .setErrorCode((short) 0)
+ .setTopicId(topicId)
+ .setName(topicName0)
+ .setIsInternal(false)
+ .setTopicAuthorizedOperations(authorisedOperations));
+ env.kafkaClient().prepareResponse(new DescribeTopicPartitionsResponse(responseData));
+
+ try {
+ DescribeTopicsResult result = env.adminClient().describeTopics(
+ Arrays.asList(topicName0), new DescribeTopicsOptions().includeAuthorizedOperations(true)
Review Comment:
There's another `Arrays.` a few lines below
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #16136:
URL: https://github.com/apache/kafka/pull/16136#issuecomment-2162963626
I'll merge to trunk and 3.8 once the CI completes (assuming no related failures)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #16136:
URL: https://github.com/apache/kafka/pull/16136#discussion_r1636406293
##########
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##########
@@ -2404,14 +2404,16 @@ void handleFailure(Throwable throwable) {
private TopicDescription getTopicDescriptionFromDescribeTopicsResponseTopic(
DescribeTopicPartitionsResponseTopic topic,
- Map<Integer, Node> nodes
+ Map<Integer, Node> nodes,
+ boolean includeAuthorizedOperations
) {
List<DescribeTopicPartitionsResponsePartition> partitionInfos = topic.partitions();
List<TopicPartitionInfo> partitions = new ArrayList<>(partitionInfos.size());
for (DescribeTopicPartitionsResponsePartition partitionInfo : partitionInfos) {
partitions.add(DescribeTopicPartitionsResponse.partitionToTopicPartitionInfo(partitionInfo, nodes));
}
- return new TopicDescription(topic.name(), topic.isInternal(), partitions, validAclOperations(topic.topicAuthorizedOperations()), topic.topicId());
+ Set<AclOperation> authorisedOperations = includeAuthorizedOperations ? validAclOperations(topic.topicAuthorizedOperations()) : null;
Review Comment:
@mimaison thanks for the info. I will leave some questions on #15175 :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest [kafka]
Posted by "CalvinConfluent (via GitHub)" <gi...@apache.org>.
CalvinConfluent commented on PR #16136:
URL: https://github.com/apache/kafka/pull/16136#issuecomment-2155312008
Thanks for the change! we would better have a UT to test the describeTopics for admin client. There is no UT coverage for the `includeAuthorizedOptions`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org