-
Notifications
You must be signed in to change notification settings - Fork 0
KAFKA-17897: Deprecate Admin.listConsumerGroups [2/N] #6
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.
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
2a821c2
5d03440
c3568c2
67b3d69
97a83fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3524,44 +3524,62 @@ void handleResponse(AbstractResponse abstractResponse) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (final Node node : allNodes) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| final long nowList = time.milliseconds(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| runnable.call(new Call("listGroups", deadline, new ConstantNodeIdProvider(node.id())) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // If only regular consumer group types are required, we can try an earlier request version if | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // UnsupportedVersionException is thrown | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| final boolean canTryEarlierRequestVersion = options.regularConsumerGroupTypes(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| boolean tryUsingEarlierRequestVersion = false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+3527
to
+3531
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feature Flag PatternBoolean flag controls version fallback behavior instead of using a strategy pattern. This creates implicit coupling between flag state and method behavior, making future version handling changes more complex. Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ListGroupsRequest.Builder createRequest(int timeoutMs) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+3544
to
+3555
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary Collection ConversionConverting Set to List unnecessarily creates intermediate collections. This conversion happens on every request in a hot path. For large numbers of group types/states, this creates avoidable object allocation and garbage collection pressure.
Suggested change
Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+3528
to
+3556
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Duplicate Protocol LogicDuplicate code for groupStates conversion appears in both branches. This violates DRY principle and increases maintenance burden when modifying the conversion logic. Standards
Comment on lines
+3535
to
+3556
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Repeated Request LogicDuplicated 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
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+3560
to
+3561
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Protocol Type Filtering Lacks Null SafetyThe 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
Comment on lines
+3560
to
+3561
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential NullPointerExceptionNo 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.
Suggested change
Standards
Comment on lines
+3560
to
+3561
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Protocol Type FilteringProtocol 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.
Suggested change
Standards
Comment on lines
+3560
to
+3561
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Protocol Type FilteringThe 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
Comment on lines
+3560
to
+3561
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential NPE RiskThe 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
Comment on lines
+3560
to
+3561
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Protocol Type FilterThe 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
Comment on lines
+3560
to
+3561
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Protocol Type FilteringThe 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
Comment on lines
+3560
to
+3561
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Duplicate Protocol FilteringProtocol 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
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| final String groupId = group.groupId(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+3560
to
+3562
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing Protocol Type ValidationProtocol type validation allows null values without explicit checks. An attacker could exploit null protocol types to bypass filtering. Implementing null-safety checks prevents unexpected behavior and potential security bypasses.
Suggested change
Standards
Comment on lines
+3561
to
+3562
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing Protocol Type ValidationThe 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
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
3559
to
+3569
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Client-Side Protocol Type Filtering BypassThe 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.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (group.groupState() == null || group.groupState().isEmpty()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| groupState = Optional.empty(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| groupState = Optional.of(GroupState.parse(group.groupState())); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| final GroupListing groupListing = new GroupListing( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| groupId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| protocolType, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| groupState | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| results.addListing(groupListing); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+3560
to
3582
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Protocol Type Filtering Logic InconsistencyProtocol type filtering happens client-side after receiving results, while type filtering happens server-side. This inconsistency causes unnecessary network traffic and processing when many groups exist, especially when using earlier request versions.
Suggested change
Standards
Comment on lines
+3560
to
3582
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Protocol Filter InconsistencyProtocol type filtering is applied client-side while group type filtering is applied server-side. This inconsistency could lead to incorrect filtering behavior when both filters are specified, as protocol filtering happens after receiving results. Standards
Comment on lines
+3560
to
3582
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Protocol Type FilteringProtocol type filtering happens client-side after fetching results from server. This can cause unnecessary network traffic and processing overhead when many groups exist but few match the filter. Standards
Comment on lines
+3560
to
3582
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Protocol Filter DuplicationClient-side filtering duplicates server-side filtering logic. This creates maintenance overhead as filtering logic must be synchronized between client and server implementations. Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| final GroupListing groupListing = new GroupListing( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| groupId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| protocolType, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| groupState | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| results.addListing(groupListing); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -3582,6 +3600,23 @@ void handleResponse(AbstractResponse abstractResponse) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| boolean handleUnsupportedVersionException(final UnsupportedVersionException exception) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 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; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+3605
to
+3613
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incomplete Error HandlingThe 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.
Suggested change
Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Have a try using the earlier request version | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tryUsingEarlierRequestVersion = true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+3616
to
+3617
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential SSRF RiskAutomatic fallback to earlier protocol versions could bypass security controls. Attackers could exploit older, less secure protocol versions with fewer filtering capabilities. Standards
Comment on lines
+3605
to
+3617
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incomplete Version FallbackThe 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
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+3604
to
+3618
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incomplete Error HandlingThe error handling logic doesn't reset filters when falling back to earlier request version. When using earlier version without type filtering capability, the client should adjust its expectations accordingly. Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| void handleFailure(Throwable throwable) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| synchronized (results) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -36,16 +36,38 @@ public class ListGroupsOptions extends AbstractOptions<ListGroupsOptions> { | |||||||||||||||||
| private Set<GroupType> types = Set.of(); | ||||||||||||||||||
| private Set<String> protocolTypes = Set.of(); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Types filter is supported by brokers with version 4.0.0 or later. Older brokers only support | ||||||||||||||||||
| // classic groups, so listing consumer groups on an older broker does not need to use a types filter. | ||||||||||||||||||
| private boolean regularConsumerGroupTypes = false; | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Only consumer groups will be returned by listGroups(). | ||||||||||||||||||
| * This operation sets filters on group type and protocol type which select consumer groups. | ||||||||||||||||||
| */ | ||||||||||||||||||
| public static ListGroupsOptions forConsumerGroups() { | ||||||||||||||||||
| return new ListGroupsOptions() | ||||||||||||||||||
| .withTypes(Set.of(GroupType.CLASSIC, GroupType.CONSUMER)) | ||||||||||||||||||
| .withTypes(Set.of(GroupType.CLASSIC, GroupType.CONSUMER), true) | ||||||||||||||||||
| .withProtocolTypes(Set.of("", ConsumerProtocol.PROTOCOL_TYPE)); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Only share groups will be returned by listGroups(). | ||||||||||||||||||
| * This operation sets a filter on group type which select share groups. | ||||||||||||||||||
| */ | ||||||||||||||||||
| public static ListGroupsOptions forShareGroups() { | ||||||||||||||||||
| return new ListGroupsOptions() | ||||||||||||||||||
| .withTypes(Set.of(GroupType.SHARE)); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Only streams groups will be returned by listGroups(). | ||||||||||||||||||
| * This operation sets a filter on group type which select streams groups. | ||||||||||||||||||
| */ | ||||||||||||||||||
| public static ListGroupsOptions forStreamsGroups() { | ||||||||||||||||||
| return new ListGroupsOptions() | ||||||||||||||||||
| .withTypes(Set.of(GroupType.STREAMS)); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * If groupStates is set, only groups in these states will be returned by listGroups(). | ||||||||||||||||||
| * Otherwise, all groups are returned. | ||||||||||||||||||
|
|
@@ -56,6 +78,10 @@ public ListGroupsOptions inGroupStates(Set<GroupState> groupStates) { | |||||||||||||||||
| return this; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * If protocol types is set, only groups of these protocol types will be returned by listGroups(). | ||||||||||||||||||
| * Otherwise, all groups are returned. | ||||||||||||||||||
| */ | ||||||||||||||||||
| public ListGroupsOptions withProtocolTypes(Set<String> protocolTypes) { | ||||||||||||||||||
| this.protocolTypes = (protocolTypes == null || protocolTypes.isEmpty()) ? Set.of() : Set.copyOf(protocolTypes); | ||||||||||||||||||
| return this; | ||||||||||||||||||
|
Comment on lines
85
to
87
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Insecure Default in Protocol Type FilteringThe 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.
Suggested change
|
||||||||||||||||||
|
|
@@ -66,7 +92,12 @@ public ListGroupsOptions withProtocolTypes(Set<String> protocolTypes) { | |||||||||||||||||
| * Otherwise, all groups are returned. | ||||||||||||||||||
| */ | ||||||||||||||||||
| public ListGroupsOptions withTypes(Set<GroupType> types) { | ||||||||||||||||||
| return this.withTypes(types, false); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| ListGroupsOptions withTypes(Set<GroupType> types, boolean regularConsumerGroupTypes) { | ||||||||||||||||||
| this.types = (types == null || types.isEmpty()) ? Set.of() : Set.copyOf(types); | ||||||||||||||||||
| this.regularConsumerGroupTypes = regularConsumerGroupTypes; | ||||||||||||||||||
| return this; | ||||||||||||||||||
|
Comment on lines
+98
to
101
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Boolean Flag ParameterBoolean 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
|
||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -90,4 +121,8 @@ public Set<String> protocolTypes() { | |||||||||||||||||
| public Set<GroupType> types() { | ||||||||||||||||||
| return types; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| boolean regularConsumerGroupTypes() { | ||||||||||||||||||
| return regularConsumerGroupTypes; | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.