KAFKA-17897: Deprecate Admin.listConsumerGroups [2/N] #6
Conversation
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
Show resolved
Hide resolved
|
/refacto-test |
1 similar comment
|
/refacto-test |
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
Show resolved
Hide resolved
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
| private void maybeAddGroup(ListGroupsResponseData.ListedGroup group) { | ||
| final String groupId = group.groupId(); | ||
| final Optional<GroupType> type; | ||
| if (group.groupType() == null || group.groupType().isEmpty()) { | ||
| type = Optional.empty(); | ||
| } else { | ||
| type = Optional.of(GroupType.parse(group.groupType())); | ||
| } | ||
| final String protocolType = group.protocolType(); | ||
| final Optional<GroupState> groupState; | ||
| if (group.groupState() == null || group.groupState().isEmpty()) { | ||
| groupState = Optional.empty(); | ||
| } else { | ||
| groupState = Optional.of(GroupState.parse(group.groupState())); | ||
| String protocolType = group.protocolType(); | ||
| if (options.protocolTypes().isEmpty() || options.protocolTypes().contains(protocolType)) { | ||
| final String groupId = group.groupId(); | ||
| final Optional<GroupType> type; | ||
| if (group.groupType() == null || group.groupType().isEmpty()) { | ||
| type = Optional.empty(); | ||
| } else { | ||
| type = Optional.of(GroupType.parse(group.groupType())); | ||
| } | ||
| final Optional<GroupState> groupState; |
There was a problem hiding this comment.
Client-Side Protocol Type Filtering Bypass
The protocol type filtering is performed client-side after receiving the response from the server. This means that even if a user is only authorized to see certain protocol types, they could modify the client code to bypass this filtering and see all group types returned by the server. The server should be responsible for enforcing access controls based on protocol types, not the client. This could lead to information disclosure where users can access group data they shouldn't have permission to view.
| private void maybeAddGroup(ListGroupsResponseData.ListedGroup group) { | |
| final String groupId = group.groupId(); | |
| final Optional<GroupType> type; | |
| if (group.groupType() == null || group.groupType().isEmpty()) { | |
| type = Optional.empty(); | |
| } else { | |
| type = Optional.of(GroupType.parse(group.groupType())); | |
| } | |
| final String protocolType = group.protocolType(); | |
| final Optional<GroupState> groupState; | |
| if (group.groupState() == null || group.groupState().isEmpty()) { | |
| groupState = Optional.empty(); | |
| } else { | |
| groupState = Optional.of(GroupState.parse(group.groupState())); | |
| String protocolType = group.protocolType(); | |
| if (options.protocolTypes().isEmpty() || options.protocolTypes().contains(protocolType)) { | |
| final String groupId = group.groupId(); | |
| final Optional<GroupType> type; | |
| if (group.groupType() == null || group.groupType().isEmpty()) { | |
| type = Optional.empty(); | |
| } else { | |
| type = Optional.of(GroupType.parse(group.groupType())); | |
| } | |
| final Optional<GroupState> groupState; | |
| // In createRequest method, add protocol types to the request | |
| List<String> protocolTypes = new ArrayList<>(options.protocolTypes()); | |
| return new ListGroupsRequest.Builder(new ListGroupsRequestData() | |
| .setTypesFilter(groupTypes) | |
| .setStatesFilter(groupStates) | |
| .setProtocolTypesFilter(protocolTypes)); |
| public ListGroupsOptions withProtocolTypes(Set<String> protocolTypes) { | ||
| this.protocolTypes = (protocolTypes == null || protocolTypes.isEmpty()) ? Set.of() : Set.copyOf(protocolTypes); | ||
| return this; |
There was a problem hiding this comment.
Insecure Default in Protocol Type Filtering
The withProtocolTypes method treats null or empty sets as 'no filtering' rather than 'no access'. This follows the principle of being permissive by default, which is not a secure design pattern. If a developer forgets to specify protocol types, the system will allow access to all protocol types rather than denying access by default, potentially leading to unintended information disclosure.
| public ListGroupsOptions withProtocolTypes(Set<String> protocolTypes) { | |
| this.protocolTypes = (protocolTypes == null || protocolTypes.isEmpty()) ? Set.of() : Set.copyOf(protocolTypes); | |
| return this; | |
| public ListGroupsOptions withProtocolTypes(Set<String> protocolTypes) { | |
| // Default to an empty set (no access) if null is provided | |
| this.protocolTypes = (protocolTypes == null) ? Set.of() : Set.copyOf(protocolTypes); | |
| return this; | |
| } |
Protocol Type Filtering Bypass in ListGroupsOptions👍 Well Done
📌 Files Processed
|
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: ListGroups Protocol Type Filtering👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| String protocolType = group.protocolType(); | ||
| if (options.protocolTypes().isEmpty() || options.protocolTypes().contains(protocolType)) { |
There was a problem hiding this comment.
Protocol Type Filtering Lacks Null Safety
The protocol type filtering doesn't handle null protocol types safely. If group.protocolType() returns null, the contains() check will throw NullPointerException. This would cause the listing operation to fail unexpectedly.
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- SRE-Error-Handling
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: ListGroups API Enhancement👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| String protocolType = group.protocolType(); | ||
| if (options.protocolTypes().isEmpty() || options.protocolTypes().contains(protocolType)) { |
There was a problem hiding this comment.
Potential NullPointerException
No null check on protocolType before using it in contains() method. If broker returns null protocolType, this will cause NullPointerException, crashing the client application. This breaks reliability when interacting with non-standard broker responses.
| String protocolType = group.protocolType(); | |
| if (options.protocolTypes().isEmpty() || options.protocolTypes().contains(protocolType)) { | |
| String protocolType = group.protocolType(); | |
| if (options.protocolTypes().isEmpty() || (protocolType != null && options.protocolTypes().contains(protocolType))) { |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Robustness
- DbC-Defensive-Programming
| String protocolType = group.protocolType(); | ||
| if (options.protocolTypes().isEmpty() || options.protocolTypes().contains(protocolType)) { |
There was a problem hiding this comment.
Protocol Type Filtering
Protocol type filtering happens after receiving response from broker, not in request. This creates unnecessary network traffic and processing overhead when filtering could be done server-side. May cause performance degradation under high load.
| String protocolType = group.protocolType(); | |
| if (options.protocolTypes().isEmpty() || options.protocolTypes().contains(protocolType)) { | |
| if (tryUsingEarlierRequestVersion) { | |
| List<String> groupStates = options.groupStates() | |
| .stream() | |
| .map(GroupState::toString) | |
| .collect(Collectors.toList()); | |
| return new ListGroupsRequest.Builder(new ListGroupsRequestData() | |
| .setStatesFilter(groupStates) | |
| ); | |
| } else { | |
| List<String> groupTypes = options.types() | |
| .stream() | |
| .map(GroupType::toString) | |
| .collect(Collectors.toList()); | |
| List<String> groupStates = options.groupStates() | |
| .stream() | |
| .map(GroupState::toString) | |
| .collect(Collectors.toList()); | |
| List<String> protocolTypes = options.protocolTypes() | |
| .stream() | |
| .collect(Collectors.toList()); | |
| return new ListGroupsRequest.Builder(new ListGroupsRequestData() | |
| .setTypesFilter(groupTypes) | |
| .setStatesFilter(groupStates) | |
| .setProtocolTypesFilter(protocolTypes) | |
| ); | |
| } |
Standards
- ISO-IEC-25010-Reliability-Resource-Utilization
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- SRE-Efficiency
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: ListGroups Implementation👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| String protocolType = group.protocolType(); | ||
| if (options.protocolTypes().isEmpty() || options.protocolTypes().contains(protocolType)) { |
There was a problem hiding this comment.
Protocol Type Filtering
The protocol type filtering doesn't handle null protocol types safely. If group.protocolType() returns null, the contains() check will throw NullPointerException, causing request failure. This breaks reliability when processing groups with null protocol types.
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- SRE-Error-Handling
| // If we cannot try the earlier request version, give up | ||
| if (!canTryEarlierRequestVersion) { | ||
| return false; | ||
| } | ||
|
|
||
| // If have already tried the earlier request version, give up | ||
| if (tryUsingEarlierRequestVersion) { | ||
| return false; | ||
| } | ||
|
|
||
| // Have a try using the earlier request version | ||
| tryUsingEarlierRequestVersion = true; | ||
| return true; |
There was a problem hiding this comment.
Incomplete Version Fallback
The version fallback mechanism only attempts one earlier version. If multiple version downgrades are needed, the code will fail after a single retry. This creates a reliability gap when interacting with brokers supporting much older versions.
Standards
- ISO-IEC-25010-Reliability-Compatibility
- ISO-IEC-25010-Reliability-Fault-Tolerance
- SRE-Graceful-Degradation
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Admin API Enhancements👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| String protocolType = group.protocolType(); | ||
| if (options.protocolTypes().isEmpty() || options.protocolTypes().contains(protocolType)) { |
There was a problem hiding this comment.
Potential NPE Risk
The protocolType variable may be null, causing NullPointerException when calling contains(). While the protocol type is unlikely to be null in practice, defensive null checking would prevent potential runtime failures.
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- DbC-Precondition-Validation
- SRE-Error-Prevention
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: ListGroups Implementation👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| if (options.protocolTypes().isEmpty() || options.protocolTypes().contains(protocolType)) { | ||
| final String groupId = group.groupId(); |
There was a problem hiding this comment.
Missing Protocol Type Validation
The code doesn't handle null protocol types before checking containment. If protocolType is null, the contains() check will throw NullPointerException, causing request processing to fail. This creates a reliability issue when processing responses with null protocol types.
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- DbC-Precondition-Validation
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: ListGroups API Enhancements👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| // If we cannot try the earlier request version, give up | ||
| if (!canTryEarlierRequestVersion) { | ||
| return false; | ||
| } | ||
|
|
||
| // If have already tried the earlier request version, give up | ||
| if (tryUsingEarlierRequestVersion) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Incomplete Error Handling
The handleUnsupportedVersionException method returns false when it cannot handle the exception, but doesn't provide any error information about why the version is unsupported. This can lead to silent failures and difficult debugging when version compatibility issues occur.
| // If we cannot try the earlier request version, give up | |
| if (!canTryEarlierRequestVersion) { | |
| return false; | |
| } | |
| // If have already tried the earlier request version, give up | |
| if (tryUsingEarlierRequestVersion) { | |
| return false; | |
| } | |
| // If we cannot try the earlier request version, give up | |
| if (!canTryEarlierRequestVersion) { | |
| log.debug("Cannot handle UnsupportedVersionException for listGroups request: only regular consumer group types can use earlier version"); | |
| return false; | |
| } | |
| // If have already tried the earlier request version, give up | |
| if (tryUsingEarlierRequestVersion) { | |
| log.debug("Already tried earlier request version for listGroups, cannot handle UnsupportedVersionException"); | |
| return false; | |
| } |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- DbC-Error-Reporting
- SRE-Observability
| String protocolType = group.protocolType(); | ||
| if (options.protocolTypes().isEmpty() || options.protocolTypes().contains(protocolType)) { |
There was a problem hiding this comment.
Protocol Type Filter
The protocol type filter check doesn't handle null protocol types correctly. If group.protocolType() returns null, the contains() check will throw NullPointerException when options.protocolTypes() contains non-null values.
Standards
- Algorithm-Correctness-Null-Safety
- Business-Rule-Input-Validation
- Logic-Verification-Defensive-Programming
| String protocolType = group.protocolType(); | ||
| if (options.protocolTypes().isEmpty() || options.protocolTypes().contains(protocolType)) { |
There was a problem hiding this comment.
Protocol Type Filtering
The protocol type filtering implementation checks if the protocol type is empty before applying the filter. This could allow groups with empty protocol types to bypass filtering restrictions, potentially exposing groups that should be restricted. An attacker could exploit this to gain visibility into groups they shouldn't have access to.
Standards
- CWE-284
- OWASP-A01
- NIST-SSDF-PW.1
| ListGroupsOptions withTypes(Set<GroupType> types, boolean regularConsumerGroupTypes) { | ||
| this.types = (types == null || types.isEmpty()) ? Set.of() : Set.copyOf(types); | ||
| this.regularConsumerGroupTypes = regularConsumerGroupTypes; | ||
| return this; |
There was a problem hiding this comment.
Boolean Flag Parameter
Boolean parameter creates unclear API and makes method calls harder to understand. The regularConsumerGroupTypes flag hides intention and creates maintenance burden. Consider using descriptive method names or builder pattern instead.
Standards
- Clean-Code-Functions
- Maintainability-Quality-API-Design
- Clean-Code-Boolean-Parameters
| String protocolType = group.protocolType(); | ||
| if (options.protocolTypes().isEmpty() || options.protocolTypes().contains(protocolType)) { |
There was a problem hiding this comment.
Duplicate Protocol Filtering
Protocol type filtering is duplicated between client and server. The server already filters by protocol type in ListGroupsRequest, making client-side filtering redundant. This creates maintenance burden when protocol handling changes.
Standards
- Clean-Code-DRY
- Maintainability-Quality-Duplication
- Design-Pattern-Consistency
| if (tryUsingEarlierRequestVersion) { | ||
| List<String> groupStates = options.groupStates() | ||
| .stream() | ||
| .map(GroupState::toString) | ||
| .collect(Collectors.toList()); | ||
| return new ListGroupsRequest.Builder(new ListGroupsRequestData() | ||
| .setStatesFilter(groupStates) | ||
| ); | ||
| } else { | ||
| List<String> groupTypes = options.types() | ||
| .stream() | ||
| .map(GroupType::toString) | ||
| .collect(Collectors.toList()); | ||
| List<String> groupStates = options.groupStates() | ||
| .stream() | ||
| .map(GroupState::toString) | ||
| .collect(Collectors.toList()); | ||
| return new ListGroupsRequest.Builder(new ListGroupsRequestData() | ||
| .setTypesFilter(groupTypes) | ||
| .setStatesFilter(groupStates) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Repeated Request Logic
Duplicated code for creating groupStates list in both branches increases maintenance burden. The stream transformation of groupStates is repeated, violating DRY principle. Extract common code to reduce duplication and improve maintainability.
Standards
- Clean-Code-DRY
- Refactoring-Extract-Method
- Maintainability-Quality-Duplication
3 participants
Admin.listConsumerGroups() was able to use the early versions of
ListGroups RPC with the version used dependent upon the filters the user
specified. Admin.listGroups(ListGroupsOptions.forConsumerGroups())
inadvertently required ListGroups v5 because it always set a types
filter. This patch handles the UnsupportedVersionException and winds
back the complexity of the request unless the user has specified filters
which demand a higher version.
It also adds ListGroupsOptions.forShareGroups() and forStreamsGroups().
The usability of Admin.listGroups() is much improved as a result.