Skip to content

Commit 2e0b396

Browse files
petedannemannpetedannemann
authored
fix: describe groups should not fail on an issue with a single group (#1431)
1 parent 3ce2979 commit 2e0b396

2 files changed

Lines changed: 248 additions & 2 deletions

File tree

‎describegroups.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -135,11 +135,15 @@ func (c *Client) DescribeGroups(
135135
for _, member := range apiGroup.Members {
136136
decodedMetadata, err := decodeMemberMetadata(member.MemberMetadata)
137137
if err != nil {
138-
return nil, err
138+
group.Error = fmt.Errorf("failed to decode member metadata for group %s: %w", apiGroup.GroupID, err)
139+
group.Members = nil // clear any previously decoded members
140+
break
139141
}
140142
decodedAssignments, err := decodeMemberAssignments(member.MemberAssignment)
141143
if err != nil {
142-
return nil, err
144+
group.Error = fmt.Errorf("failed to decode member assignments for group %s: %w", apiGroup.GroupID, err)
145+
group.Members = nil // clear any previously decoded members
146+
break
143147
}
144148

145149
group.Members = append(group.Members, DescribeGroupsResponseMember{

‎describegroups_test.go‎

Lines changed: 242 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,14 @@ package kafka
33
import (
44
"context"
55
"fmt"
6+
"net"
67
"os"
78
"reflect"
89
"sort"
910
"testing"
1011
"time"
12+
13+
"github.com/segmentio/kafka-go/protocol/describegroups"
1114
)
1215

1316
func TestClientDescribeGroups(t *testing.T) {
@@ -128,3 +131,242 @@ func TestClientDescribeGroups(t *testing.T) {
128131
)
129132
}
130133
}
134+
135+
// mockRoundTripper is a mock transport that returns a fixed response.
136+
type mockRoundTripper struct {
137+
response Response
138+
err error
139+
}
140+
141+
func (m *mockRoundTripper) RoundTrip(ctx context.Context, addr net.Addr, req Request) (Response, error) {
142+
if m.err != nil {
143+
return nil, m.err
144+
}
145+
return m.response, nil
146+
}
147+
148+
func TestDescribeGroupsErrorHandling(t *testing.T) {
149+
// This test verifies that when one group has invalid metadata/assignments,
150+
// it doesn't prevent other groups from being returned successfully.
151+
152+
// Create a mock response with multiple groups where one has invalid data.
153+
mockResp := &describegroups.Response{
154+
Groups: []describegroups.ResponseGroup{
155+
{
156+
ErrorCode: 0,
157+
GroupID: "valid-group-1",
158+
GroupState: "Stable",
159+
ProtocolType: "consumer",
160+
Members: []describegroups.ResponseGroupMember{
161+
{
162+
MemberID: "member-1",
163+
ClientID: "client-1",
164+
ClientHost: "/127.0.0.1",
165+
// Valid metadata: version (int16) = 0, topics array length (int32) = 0, userdata length (int32) = 0
166+
MemberMetadata: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
167+
// Valid assignments: version (int16) = 0, topics array length (int32) = 0, userdata length (int32) = 0
168+
MemberAssignment: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
169+
},
170+
},
171+
},
172+
{
173+
ErrorCode: 0,
174+
GroupID: "invalid-group",
175+
GroupState: "Stable",
176+
ProtocolType: "consumer",
177+
Members: []describegroups.ResponseGroupMember{
178+
{
179+
MemberID: "member-2",
180+
ClientID: "client-2",
181+
ClientHost: "/127.0.0.1",
182+
// Invalid metadata - truncated data
183+
MemberMetadata: []byte{0x00},
184+
MemberAssignment: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
185+
},
186+
},
187+
},
188+
{
189+
ErrorCode: 0,
190+
GroupID: "valid-group-2",
191+
GroupState: "Stable",
192+
ProtocolType: "consumer",
193+
Members: []describegroups.ResponseGroupMember{
194+
{
195+
MemberID: "member-3",
196+
ClientID: "client-3",
197+
ClientHost: "/127.0.0.1",
198+
// Valid metadata
199+
MemberMetadata: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
200+
// Valid assignments
201+
MemberAssignment: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
202+
},
203+
},
204+
},
205+
},
206+
}
207+
208+
client := &Client{
209+
Transport: &mockRoundTripper{response: mockResp},
210+
}
211+
212+
ctx := context.Background()
213+
resp, err := client.DescribeGroups(ctx, &DescribeGroupsRequest{
214+
Addr: TCP("localhost:9092"),
215+
GroupIDs: []string{"valid-group-1", "invalid-group", "valid-group-2"},
216+
})
217+
218+
if err != nil {
219+
t.Fatalf("Unexpected error from DescribeGroups: %v", err)
220+
}
221+
222+
if len(resp.Groups) != 3 {
223+
t.Fatalf("Expected 3 groups, got %d", len(resp.Groups))
224+
}
225+
226+
// Verify valid-group-1 has no error and has members
227+
if resp.Groups[0].GroupID != "valid-group-1" {
228+
t.Errorf("Expected first group to be valid-group-1, got %s", resp.Groups[0].GroupID)
229+
}
230+
if resp.Groups[0].Error != nil {
231+
t.Errorf("Expected valid-group-1 to have no error, got %v", resp.Groups[0].Error)
232+
}
233+
if len(resp.Groups[0].Members) != 1 {
234+
t.Errorf("Expected valid-group-1 to have 1 member, got %d", len(resp.Groups[0].Members))
235+
}
236+
237+
// Verify invalid-group has an error and no members
238+
if resp.Groups[1].GroupID != "invalid-group" {
239+
t.Errorf("Expected second group to be invalid-group, got %s", resp.Groups[1].GroupID)
240+
}
241+
if resp.Groups[1].Error == nil {
242+
t.Error("Expected invalid-group to have an error, got nil")
243+
}
244+
if len(resp.Groups[1].Members) != 0 {
245+
t.Errorf("Expected invalid-group to have 0 members due to error, got %d", len(resp.Groups[1].Members))
246+
}
247+
248+
// Verify valid-group-2 has no error and has members
249+
if resp.Groups[2].GroupID != "valid-group-2" {
250+
t.Errorf("Expected third group to be valid-group-2, got %s", resp.Groups[2].GroupID)
251+
}
252+
if resp.Groups[2].Error != nil {
253+
t.Errorf("Expected valid-group-2 to have no error, got %v", resp.Groups[2].Error)
254+
}
255+
if len(resp.Groups[2].Members) != 1 {
256+
t.Errorf("Expected valid-group-2 to have 1 member, got %d", len(resp.Groups[2].Members))
257+
}
258+
}
259+
260+
func TestDescribeGroupsInvalidAssignments(t *testing.T) {
261+
// Test the case where assignments are invalid but metadata is valid
262+
mockResp := &describegroups.Response{
263+
Groups: []describegroups.ResponseGroup{
264+
{
265+
ErrorCode: 0,
266+
GroupID: "group-with-bad-assignments",
267+
GroupState: "Stable",
268+
ProtocolType: "consumer",
269+
Members: []describegroups.ResponseGroupMember{
270+
{
271+
MemberID: "member-1",
272+
ClientID: "client-1",
273+
ClientHost: "/127.0.0.1",
274+
// Valid metadata
275+
MemberMetadata: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
276+
// Invalid assignments - truncated
277+
MemberAssignment: []byte{0x00},
278+
},
279+
},
280+
},
281+
},
282+
}
283+
284+
client := &Client{
285+
Transport: &mockRoundTripper{response: mockResp},
286+
}
287+
288+
ctx := context.Background()
289+
resp, err := client.DescribeGroups(ctx, &DescribeGroupsRequest{
290+
Addr: TCP("localhost:9092"),
291+
GroupIDs: []string{"group-with-bad-assignments"},
292+
})
293+
294+
if err != nil {
295+
t.Fatalf("Unexpected error from DescribeGroups: %v", err)
296+
}
297+
298+
// Verify we got the group back with an error
299+
if len(resp.Groups) != 1 {
300+
t.Fatalf("Expected 1 group, got %d", len(resp.Groups))
301+
}
302+
303+
if resp.Groups[0].Error == nil {
304+
t.Error("Expected group to have an error for invalid assignments, got nil")
305+
}
306+
307+
if len(resp.Groups[0].Members) != 0 {
308+
t.Errorf("Expected group to have 0 members due to error, got %d", len(resp.Groups[0].Members))
309+
}
310+
}
311+
312+
func TestDescribeGroupsPartialMembersCleared(t *testing.T) {
313+
mockResp := &describegroups.Response{
314+
Groups: []describegroups.ResponseGroup{
315+
{
316+
ErrorCode: 0,
317+
GroupID: "multi-member-group",
318+
GroupState: "Stable",
319+
ProtocolType: "consumer",
320+
Members: []describegroups.ResponseGroupMember{
321+
{
322+
MemberID: "member-1", // valid
323+
ClientID: "client-1",
324+
ClientHost: "/127.0.0.1",
325+
MemberMetadata: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
326+
MemberAssignment: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
327+
},
328+
{
329+
MemberID: "member-2", // invalid
330+
ClientID: "client-2",
331+
ClientHost: "/127.0.0.1",
332+
MemberMetadata: []byte{0x00},
333+
MemberAssignment: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
334+
},
335+
{
336+
MemberID: "member-3", // valid, but never processed
337+
ClientID: "client-3",
338+
ClientHost: "/127.0.0.1",
339+
MemberMetadata: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
340+
MemberAssignment: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
341+
},
342+
},
343+
},
344+
},
345+
}
346+
347+
client := &Client{
348+
Transport: &mockRoundTripper{response: mockResp},
349+
}
350+
351+
ctx := context.Background()
352+
resp, err := client.DescribeGroups(ctx, &DescribeGroupsRequest{
353+
Addr: TCP("localhost:9092"),
354+
GroupIDs: []string{"multi-member-group"},
355+
})
356+
357+
if err != nil {
358+
t.Fatalf("Unexpected error from DescribeGroups: %v", err)
359+
}
360+
361+
if len(resp.Groups) != 1 {
362+
t.Fatalf("Expected 1 group, got %d", len(resp.Groups))
363+
}
364+
365+
group := resp.Groups[0]
366+
if group.Error == nil {
367+
t.Fatal("Expected group to have an error, got nil")
368+
}
369+
if len(group.Members) != 0 {
370+
t.Errorf("Expected 0 members when error occurs, got %d", len(group.Members))
371+
}
372+
}

0 commit comments

Comments
 (0)