From f7dcbc9ba2e73966afd20c27cbc4b94dfe1ce43c Mon Sep 17 00:00:00 2001 From: Arjan Bal Date: Tue, 15 Oct 2024 22:13:05 +0530 Subject: [PATCH 1/6] Interleave addresses for happy eyeballs --- .../pickfirst/pickfirstleaf/pickfirstleaf.go | 81 ++++++++++++- .../pickfirstleaf/pickfirstleaf_test.go | 109 ++++++++++++++++++ internal/testutils/balancer.go | 2 + 3 files changed, 189 insertions(+), 3 deletions(-) diff --git a/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go b/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go index 985b6edc7f4c..4c3bde2de4af 100644 --- a/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go +++ b/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go @@ -29,6 +29,7 @@ import ( "encoding/json" "errors" "fmt" + "net" "sync" "google.golang.org/grpc/balancer" @@ -61,6 +62,14 @@ var ( // TODO: change to pick-first when this becomes the default pick_first policy. const logPrefix = "[pick-first-leaf-lb %p] " +type ipAddrType int + +const ( + ipTypeUnknown ipAddrType = iota + ipv4 + ipv6 +) + type pickfirstBuilder struct{} func (pickfirstBuilder) Build(cc balancer.ClientConn, _ balancer.BuildOptions) balancer.Balancer { @@ -206,9 +215,6 @@ func (b *pickfirstBalancer) UpdateClientConnState(state balancer.ClientConnState // "Flatten the list by concatenating the ordered list of addresses for // each of the endpoints, in order." - A61 for _, endpoint := range endpoints { - // "In the flattened list, interleave addresses from the two address - // families, as per RFC-8305 section 4." - A61 - // TODO: support the above language. newAddrs = append(newAddrs, endpoint.Addresses...) } } else { @@ -232,6 +238,10 @@ func (b *pickfirstBalancer) UpdateClientConnState(state balancer.ClientConnState // SubConn multiple times in the same pass. We don't want this. newAddrs = deDupAddresses(newAddrs) + // Interleave addresses of both families (IPv4 and IPv6) as per RFC-8305 + // section 4. + newAddrs = interleaveAddresses(newAddrs) + // Since we have a new set of addresses, we are again at first pass. b.firstPass = true @@ -314,6 +324,71 @@ func deDupAddresses(addrs []resolver.Address) []resolver.Address { return retAddrs } +func interleaveAddresses(addrs []resolver.Address) []resolver.Address { + // Group the addresses by their type and determine the order in which to + // interleave the address families. The order of interleaving the families + // is the order in which the first address of a particular type appears in + // the address list. + familyAddrsMap := map[ipAddrType][]resolver.Address{} + interleavingOrder := []ipAddrType{} + for _, addr := range addrs { + typ := addressType(addr.Addr) + if _, found := familyAddrsMap[typ]; !found { + interleavingOrder = append(interleavingOrder, typ) + } + familyAddrsMap[typ] = append(familyAddrsMap[typ], addr) + } + + interleavedAddrs := make([]resolver.Address, 0, len(addrs)) + curTypeIndex := 0 + for i := 0; i < len(addrs); i++ { + // Some IP types may have fewer addresses than others, so we look for + // the next type that has a remaining member to add to the interleaved + // list. + for { + curType := interleavingOrder[curTypeIndex] + remainingMembers := familyAddrsMap[curType] + if len(remainingMembers) > 0 { + break + } + curTypeIndex = (curTypeIndex + 1) % len(interleavingOrder) + } + curType := interleavingOrder[curTypeIndex] + remainingMembers := familyAddrsMap[curType] + interleavedAddrs = append(interleavedAddrs, remainingMembers[0]) + familyAddrsMap[curType] = remainingMembers[1:] + curTypeIndex = (curTypeIndex + 1) % len(interleavingOrder) + } + + return interleavedAddrs +} + +func addressType(address string) ipAddrType { + // Try parsing addresses without a port specified. + ip := net.ParseIP(address) + if ip == nil { + // Try to parse the IP after removing the port. + host, _, err := net.SplitHostPort(address) + if err != nil { + return ipTypeUnknown + } + ip = net.ParseIP(host) + } + + // If using the passthrough resolver, the hostnames would be unresolved + // and therefore not valid IP addresses. + if ip == nil { + return ipTypeUnknown + } + + if ip.To4() != nil { + return ipv4 + } else if ip.To16() != nil { + return ipv6 + } + return ipTypeUnknown +} + // reconcileSubConnsLocked updates the active subchannels based on a new address // list from the resolver. It does this by: // - closing subchannels: any existing subchannels associated with addresses diff --git a/balancer/pickfirst/pickfirstleaf/pickfirstleaf_test.go b/balancer/pickfirst/pickfirstleaf/pickfirstleaf_test.go index 84b3cb65bed4..df41f7e0acd3 100644 --- a/balancer/pickfirst/pickfirstleaf/pickfirstleaf_test.go +++ b/balancer/pickfirst/pickfirstleaf/pickfirstleaf_test.go @@ -24,6 +24,7 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" "google.golang.org/grpc/attributes" "google.golang.org/grpc/balancer" "google.golang.org/grpc/connectivity" @@ -257,3 +258,111 @@ func (s) TestPickFirstLeaf_TFPickerUpdate(t *testing.T) { t.Fatalf("cc.WaitForPickerWithErr(%v) returned error: %v", newTfErr, err) } } + +func (s) TestPickFirstLeaf_InterleavingIPV4Preffered(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + cc := testutils.NewBalancerClientConn(t) + bal := pickfirstBuilder{}.Build(cc, balancer.BuildOptions{}) + defer bal.Close() + ccState := balancer.ClientConnState{ + ResolverState: resolver.State{ + Endpoints: []resolver.Endpoint{ + {Addresses: []resolver.Address{{Addr: "1.1.1.1"}}}, // no port + {Addresses: []resolver.Address{{Addr: "2.2.2.2:2"}}}, + {Addresses: []resolver.Address{{Addr: "3.3.3.3:3"}}}, + {Addresses: []resolver.Address{{Addr: "4.4.4.4:4"}}}, + {Addresses: []resolver.Address{{Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}}}, // ipv6 with port + {Addresses: []resolver.Address{{Addr: "0002:0002:0002:0002:0002:0002:0002:0002"}}}, + {Addresses: []resolver.Address{{Addr: "0003:0003:0003:0003:0003:0003:0003:0003"}}}, + {Addresses: []resolver.Address{{Addr: "grpc.io:80"}}}, // not an IP. + }, + }, + } + if err := bal.UpdateClientConnState(ccState); err != nil { + t.Fatalf("UpdateClientConnState(%v) returned error: %v", ccState, err) + } + + wantAddrs := []resolver.Address{ + {Addr: "1.1.1.1"}, + {Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}, + {Addr: "grpc.io:80"}, + {Addr: "2.2.2.2:2"}, + {Addr: "0002:0002:0002:0002:0002:0002:0002:0002"}, + {Addr: "3.3.3.3:3"}, + {Addr: "0003:0003:0003:0003:0003:0003:0003:0003"}, + {Addr: "4.4.4.4:4"}, + } + + gotAddrs, err := subConnAddresses(ctx, cc, 8) + if err != nil { + t.Fatalf("%v", err) + } + if diff := cmp.Diff(wantAddrs, gotAddrs); diff != "" { + t.Errorf("subconn creation order mismatch (-want +got):\n%s", diff) + } +} + +func (s) TestPickFirstLeaf_InterleavingIPv6Preffered(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + cc := testutils.NewBalancerClientConn(t) + bal := pickfirstBuilder{}.Build(cc, balancer.BuildOptions{}) + defer bal.Close() + ccState := balancer.ClientConnState{ + ResolverState: resolver.State{ + Endpoints: []resolver.Endpoint{ + {Addresses: []resolver.Address{{Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}}}, // ipv6 with port + {Addresses: []resolver.Address{{Addr: "1.1.1.1"}}}, // no port + {Addresses: []resolver.Address{{Addr: "2.2.2.2:2"}}}, + {Addresses: []resolver.Address{{Addr: "3.3.3.3:3"}}}, + {Addresses: []resolver.Address{{Addr: "4.4.4.4:4"}}}, + {Addresses: []resolver.Address{{Addr: "0002:0002:0002:0002:0002:0002:0002:0002"}}}, + {Addresses: []resolver.Address{{Addr: "0003:0003:0003:0003:0003:0003:0003:0003"}}}, + {Addresses: []resolver.Address{{Addr: "grpc.io:80"}}}, // not an IP. + }, + }, + } + if err := bal.UpdateClientConnState(ccState); err != nil { + t.Fatalf("UpdateClientConnState(%v) returned error: %v", ccState, err) + } + + wantAddrs := []resolver.Address{ + {Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}, + {Addr: "1.1.1.1"}, + {Addr: "grpc.io:80"}, + {Addr: "0002:0002:0002:0002:0002:0002:0002:0002"}, + {Addr: "2.2.2.2:2"}, + {Addr: "0003:0003:0003:0003:0003:0003:0003:0003"}, + {Addr: "3.3.3.3:3"}, + {Addr: "4.4.4.4:4"}, + } + + gotAddrs, err := subConnAddresses(ctx, cc, 8) + if err != nil { + t.Fatalf("%v", err) + } + if diff := cmp.Diff(wantAddrs, gotAddrs); diff != "" { + t.Errorf("subconn creation order mismatch (-want +got):\n%s", diff) + } +} +func subConnAddresses(ctx context.Context, cc *testutils.BalancerClientConn, subConnCount int) ([]resolver.Address, error) { + addresses := []resolver.Address{} + for i := 0; i < subConnCount; i++ { + select { + case <-ctx.Done(): + return nil, fmt.Errorf("Context timed out waiting for SubConn") + case sc := <-cc.NewSubConnCh: + if len(sc.Addresses) != 1 { + return nil, fmt.Errorf("len(SubConn.Addresses) = %d, want 1", len(sc.Addresses)) + } + addresses = append(addresses, sc.Addresses[0]) + sc.UpdateState(balancer.SubConnState{ConnectivityState: connectivity.Connecting}) + sc.UpdateState(balancer.SubConnState{ + ConnectivityState: connectivity.TransientFailure, + ConnectionError: fmt.Errorf("test error"), + }) + } + } + return addresses, nil +} diff --git a/internal/testutils/balancer.go b/internal/testutils/balancer.go index 80021903df3c..2df2376c4f57 100644 --- a/internal/testutils/balancer.go +++ b/internal/testutils/balancer.go @@ -37,6 +37,7 @@ type TestSubConn struct { ConnectCh chan struct{} stateListener func(balancer.SubConnState) connectCalled *grpcsync.Event + Addresses []resolver.Address } // NewTestSubConn returns a newly initialized SubConn. Typically, subconns @@ -131,6 +132,7 @@ func (tcc *BalancerClientConn) NewSubConn(a []resolver.Address, o balancer.NewSu ConnectCh: make(chan struct{}, 1), stateListener: o.StateListener, connectCalled: grpcsync.NewEvent(), + Addresses: a, } tcc.subConnIdx++ tcc.logger.Logf("testClientConn: NewSubConn(%v, %+v) => %s", a, o, sc) From 6d8e920c5e826f0e73863523aa7323da2e975b47 Mon Sep 17 00:00:00 2001 From: Arjan Bal Date: Thu, 17 Oct 2024 09:45:25 +0530 Subject: [PATCH 2/6] Move test to ext_test.go --- .../pickfirstleaf/pickfirstleaf_ext_test.go | 108 +++++++++++++++++ .../pickfirstleaf/pickfirstleaf_test.go | 109 ------------------ 2 files changed, 108 insertions(+), 109 deletions(-) diff --git a/balancer/pickfirst/pickfirstleaf/pickfirstleaf_ext_test.go b/balancer/pickfirst/pickfirstleaf/pickfirstleaf_ext_test.go index 2ab40ef1615a..9f90f6bde8b5 100644 --- a/balancer/pickfirst/pickfirstleaf/pickfirstleaf_ext_test.go +++ b/balancer/pickfirst/pickfirstleaf/pickfirstleaf_ext_test.go @@ -851,6 +851,114 @@ func (s) TestPickFirstLeaf_EmptyAddressList(t *testing.T) { } } +func (s) TestPickFirstLeaf_InterleavingIPV4Preffered(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + cc := testutils.NewBalancerClientConn(t) + bal := balancer.Get(pickfirstleaf.Name).Build(cc, balancer.BuildOptions{}) + defer bal.Close() + ccState := balancer.ClientConnState{ + ResolverState: resolver.State{ + Endpoints: []resolver.Endpoint{ + {Addresses: []resolver.Address{{Addr: "1.1.1.1"}}}, // no port + {Addresses: []resolver.Address{{Addr: "2.2.2.2:2"}}}, + {Addresses: []resolver.Address{{Addr: "3.3.3.3:3"}}}, + {Addresses: []resolver.Address{{Addr: "4.4.4.4:4"}}}, + {Addresses: []resolver.Address{{Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}}}, // ipv6 with port + {Addresses: []resolver.Address{{Addr: "0002:0002:0002:0002:0002:0002:0002:0002"}}}, + {Addresses: []resolver.Address{{Addr: "0003:0003:0003:0003:0003:0003:0003:0003"}}}, + {Addresses: []resolver.Address{{Addr: "grpc.io:80"}}}, // not an IP. + }, + }, + } + if err := bal.UpdateClientConnState(ccState); err != nil { + t.Fatalf("UpdateClientConnState(%v) returned error: %v", ccState, err) + } + + wantAddrs := []resolver.Address{ + {Addr: "1.1.1.1"}, + {Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}, + {Addr: "grpc.io:80"}, + {Addr: "2.2.2.2:2"}, + {Addr: "0002:0002:0002:0002:0002:0002:0002:0002"}, + {Addr: "3.3.3.3:3"}, + {Addr: "0003:0003:0003:0003:0003:0003:0003:0003"}, + {Addr: "4.4.4.4:4"}, + } + + gotAddrs, err := subConnAddresses(ctx, cc, 8) + if err != nil { + t.Fatalf("%v", err) + } + if diff := cmp.Diff(wantAddrs, gotAddrs); diff != "" { + t.Errorf("subconn creation order mismatch (-want +got):\n%s", diff) + } +} + +func (s) TestPickFirstLeaf_InterleavingIPv6Preffered(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + cc := testutils.NewBalancerClientConn(t) + bal := balancer.Get(pickfirstleaf.Name).Build(cc, balancer.BuildOptions{}) + defer bal.Close() + ccState := balancer.ClientConnState{ + ResolverState: resolver.State{ + Endpoints: []resolver.Endpoint{ + {Addresses: []resolver.Address{{Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}}}, // ipv6 with port + {Addresses: []resolver.Address{{Addr: "1.1.1.1"}}}, // no port + {Addresses: []resolver.Address{{Addr: "2.2.2.2:2"}}}, + {Addresses: []resolver.Address{{Addr: "3.3.3.3:3"}}}, + {Addresses: []resolver.Address{{Addr: "4.4.4.4:4"}}}, + {Addresses: []resolver.Address{{Addr: "0002:0002:0002:0002:0002:0002:0002:0002"}}}, + {Addresses: []resolver.Address{{Addr: "0003:0003:0003:0003:0003:0003:0003:0003"}}}, + {Addresses: []resolver.Address{{Addr: "grpc.io:80"}}}, // not an IP. + }, + }, + } + if err := bal.UpdateClientConnState(ccState); err != nil { + t.Fatalf("UpdateClientConnState(%v) returned error: %v", ccState, err) + } + + wantAddrs := []resolver.Address{ + {Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}, + {Addr: "1.1.1.1"}, + {Addr: "grpc.io:80"}, + {Addr: "0002:0002:0002:0002:0002:0002:0002:0002"}, + {Addr: "2.2.2.2:2"}, + {Addr: "0003:0003:0003:0003:0003:0003:0003:0003"}, + {Addr: "3.3.3.3:3"}, + {Addr: "4.4.4.4:4"}, + } + + gotAddrs, err := subConnAddresses(ctx, cc, 8) + if err != nil { + t.Fatalf("%v", err) + } + if diff := cmp.Diff(wantAddrs, gotAddrs); diff != "" { + t.Errorf("subconn creation order mismatch (-want +got):\n%s", diff) + } +} +func subConnAddresses(ctx context.Context, cc *testutils.BalancerClientConn, subConnCount int) ([]resolver.Address, error) { + addresses := []resolver.Address{} + for i := 0; i < subConnCount; i++ { + select { + case <-ctx.Done(): + return nil, fmt.Errorf("Context timed out waiting for SubConn") + case sc := <-cc.NewSubConnCh: + if len(sc.Addresses) != 1 { + return nil, fmt.Errorf("len(SubConn.Addresses) = %d, want 1", len(sc.Addresses)) + } + addresses = append(addresses, sc.Addresses[0]) + sc.UpdateState(balancer.SubConnState{ConnectivityState: connectivity.Connecting}) + sc.UpdateState(balancer.SubConnState{ + ConnectivityState: connectivity.TransientFailure, + ConnectionError: fmt.Errorf("test error"), + }) + } + } + return addresses, nil +} + // stateStoringBalancer stores the state of the subconns being created. type stateStoringBalancer struct { balancer.Balancer diff --git a/balancer/pickfirst/pickfirstleaf/pickfirstleaf_test.go b/balancer/pickfirst/pickfirstleaf/pickfirstleaf_test.go index df41f7e0acd3..84b3cb65bed4 100644 --- a/balancer/pickfirst/pickfirstleaf/pickfirstleaf_test.go +++ b/balancer/pickfirst/pickfirstleaf/pickfirstleaf_test.go @@ -24,7 +24,6 @@ import ( "testing" "time" - "github.com/google/go-cmp/cmp" "google.golang.org/grpc/attributes" "google.golang.org/grpc/balancer" "google.golang.org/grpc/connectivity" @@ -258,111 +257,3 @@ func (s) TestPickFirstLeaf_TFPickerUpdate(t *testing.T) { t.Fatalf("cc.WaitForPickerWithErr(%v) returned error: %v", newTfErr, err) } } - -func (s) TestPickFirstLeaf_InterleavingIPV4Preffered(t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) - defer cancel() - cc := testutils.NewBalancerClientConn(t) - bal := pickfirstBuilder{}.Build(cc, balancer.BuildOptions{}) - defer bal.Close() - ccState := balancer.ClientConnState{ - ResolverState: resolver.State{ - Endpoints: []resolver.Endpoint{ - {Addresses: []resolver.Address{{Addr: "1.1.1.1"}}}, // no port - {Addresses: []resolver.Address{{Addr: "2.2.2.2:2"}}}, - {Addresses: []resolver.Address{{Addr: "3.3.3.3:3"}}}, - {Addresses: []resolver.Address{{Addr: "4.4.4.4:4"}}}, - {Addresses: []resolver.Address{{Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}}}, // ipv6 with port - {Addresses: []resolver.Address{{Addr: "0002:0002:0002:0002:0002:0002:0002:0002"}}}, - {Addresses: []resolver.Address{{Addr: "0003:0003:0003:0003:0003:0003:0003:0003"}}}, - {Addresses: []resolver.Address{{Addr: "grpc.io:80"}}}, // not an IP. - }, - }, - } - if err := bal.UpdateClientConnState(ccState); err != nil { - t.Fatalf("UpdateClientConnState(%v) returned error: %v", ccState, err) - } - - wantAddrs := []resolver.Address{ - {Addr: "1.1.1.1"}, - {Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}, - {Addr: "grpc.io:80"}, - {Addr: "2.2.2.2:2"}, - {Addr: "0002:0002:0002:0002:0002:0002:0002:0002"}, - {Addr: "3.3.3.3:3"}, - {Addr: "0003:0003:0003:0003:0003:0003:0003:0003"}, - {Addr: "4.4.4.4:4"}, - } - - gotAddrs, err := subConnAddresses(ctx, cc, 8) - if err != nil { - t.Fatalf("%v", err) - } - if diff := cmp.Diff(wantAddrs, gotAddrs); diff != "" { - t.Errorf("subconn creation order mismatch (-want +got):\n%s", diff) - } -} - -func (s) TestPickFirstLeaf_InterleavingIPv6Preffered(t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) - defer cancel() - cc := testutils.NewBalancerClientConn(t) - bal := pickfirstBuilder{}.Build(cc, balancer.BuildOptions{}) - defer bal.Close() - ccState := balancer.ClientConnState{ - ResolverState: resolver.State{ - Endpoints: []resolver.Endpoint{ - {Addresses: []resolver.Address{{Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}}}, // ipv6 with port - {Addresses: []resolver.Address{{Addr: "1.1.1.1"}}}, // no port - {Addresses: []resolver.Address{{Addr: "2.2.2.2:2"}}}, - {Addresses: []resolver.Address{{Addr: "3.3.3.3:3"}}}, - {Addresses: []resolver.Address{{Addr: "4.4.4.4:4"}}}, - {Addresses: []resolver.Address{{Addr: "0002:0002:0002:0002:0002:0002:0002:0002"}}}, - {Addresses: []resolver.Address{{Addr: "0003:0003:0003:0003:0003:0003:0003:0003"}}}, - {Addresses: []resolver.Address{{Addr: "grpc.io:80"}}}, // not an IP. - }, - }, - } - if err := bal.UpdateClientConnState(ccState); err != nil { - t.Fatalf("UpdateClientConnState(%v) returned error: %v", ccState, err) - } - - wantAddrs := []resolver.Address{ - {Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}, - {Addr: "1.1.1.1"}, - {Addr: "grpc.io:80"}, - {Addr: "0002:0002:0002:0002:0002:0002:0002:0002"}, - {Addr: "2.2.2.2:2"}, - {Addr: "0003:0003:0003:0003:0003:0003:0003:0003"}, - {Addr: "3.3.3.3:3"}, - {Addr: "4.4.4.4:4"}, - } - - gotAddrs, err := subConnAddresses(ctx, cc, 8) - if err != nil { - t.Fatalf("%v", err) - } - if diff := cmp.Diff(wantAddrs, gotAddrs); diff != "" { - t.Errorf("subconn creation order mismatch (-want +got):\n%s", diff) - } -} -func subConnAddresses(ctx context.Context, cc *testutils.BalancerClientConn, subConnCount int) ([]resolver.Address, error) { - addresses := []resolver.Address{} - for i := 0; i < subConnCount; i++ { - select { - case <-ctx.Done(): - return nil, fmt.Errorf("Context timed out waiting for SubConn") - case sc := <-cc.NewSubConnCh: - if len(sc.Addresses) != 1 { - return nil, fmt.Errorf("len(SubConn.Addresses) = %d, want 1", len(sc.Addresses)) - } - addresses = append(addresses, sc.Addresses[0]) - sc.UpdateState(balancer.SubConnState{ConnectivityState: connectivity.Connecting}) - sc.UpdateState(balancer.SubConnState{ - ConnectivityState: connectivity.TransientFailure, - ConnectionError: fmt.Errorf("test error"), - }) - } - } - return addresses, nil -} From 7b4222240c3249fd0a758a0766fef8555f6caf7f Mon Sep 17 00:00:00 2001 From: Arjan Bal Date: Sun, 20 Oct 2024 02:55:37 +0530 Subject: [PATCH 3/6] Simplify for loop --- .../pickfirst/pickfirstleaf/pickfirstleaf.go | 21 +++++++------------ 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go b/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go index 4c3bde2de4af..962be33469fa 100644 --- a/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go +++ b/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go @@ -340,24 +340,17 @@ func interleaveAddresses(addrs []resolver.Address) []resolver.Address { } interleavedAddrs := make([]resolver.Address, 0, len(addrs)) - curTypeIndex := 0 - for i := 0; i < len(addrs); i++ { + + for curTypeIndex := 0; len(interleavedAddrs) < len(addrs); curTypeIndex = (curTypeIndex + 1) % len(interleavingOrder) { // Some IP types may have fewer addresses than others, so we look for // the next type that has a remaining member to add to the interleaved // list. - for { - curType := interleavingOrder[curTypeIndex] - remainingMembers := familyAddrsMap[curType] - if len(remainingMembers) > 0 { - break - } - curTypeIndex = (curTypeIndex + 1) % len(interleavingOrder) + typ := interleavingOrder[curTypeIndex] + remainingMembers := familyAddrsMap[typ] + if len(remainingMembers) > 0 { + interleavedAddrs = append(interleavedAddrs, remainingMembers[0]) + familyAddrsMap[typ] = remainingMembers[1:] } - curType := interleavingOrder[curTypeIndex] - remainingMembers := familyAddrsMap[curType] - interleavedAddrs = append(interleavedAddrs, remainingMembers[0]) - familyAddrsMap[curType] = remainingMembers[1:] - curTypeIndex = (curTypeIndex + 1) % len(interleavingOrder) } return interleavedAddrs From a00f0cd1541bcd74de344e69ba93ae2012f4bd9a Mon Sep 17 00:00:00 2001 From: Arjan Bal Date: Wed, 23 Oct 2024 17:58:52 +0530 Subject: [PATCH 4/6] Address review comments --- .../pickfirst/pickfirstleaf/pickfirstleaf.go | 73 +++++++------ .../pickfirstleaf/pickfirstleaf_ext_test.go | 103 +++++++++++++----- 2 files changed, 116 insertions(+), 60 deletions(-) diff --git a/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go b/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go index 962be33469fa..ba4e835fe262 100644 --- a/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go +++ b/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go @@ -62,12 +62,14 @@ var ( // TODO: change to pick-first when this becomes the default pick_first policy. const logPrefix = "[pick-first-leaf-lb %p] " -type ipAddrType int +type ipAddrFamily int const ( - ipTypeUnknown ipAddrType = iota - ipv4 - ipv6 + // ipAddrFamilyUnknown represents strings that can't be parsed as an IP + // address. + ipAddrFamilyUnknown ipAddrFamily = iota + ipAddrFamilyV4 + ipAddrFamilyV6 ) type pickfirstBuilder struct{} @@ -238,8 +240,6 @@ func (b *pickfirstBalancer) UpdateClientConnState(state balancer.ClientConnState // SubConn multiple times in the same pass. We don't want this. newAddrs = deDupAddresses(newAddrs) - // Interleave addresses of both families (IPv4 and IPv6) as per RFC-8305 - // section 4. newAddrs = interleaveAddresses(newAddrs) // Since we have a new set of addresses, we are again at first pass. @@ -324,62 +324,69 @@ func deDupAddresses(addrs []resolver.Address) []resolver.Address { return retAddrs } +// interleaveAddresses interleaves addresses of both families (IPv4 and IPv6) +// as per RFC-8305 section 4. +// Whichever address family is first in the list is followed by an address of +// the other address family; that is, if the first address in the list is IPv6, +// then the first IPv4 address should be moved up in the list to be second in +// the list. It doesn't support configuring "First Address Family Count", i.e. +// there will always be a single member of the first address family at the +// beginning of the interleaved list. +// Addresses that are neither IPv4 nor IPv6 are treated as part of a third +// "unknown" family for interleaving. +// See: https://datatracker.ietf.org/doc/html/rfc8305#autoid-6 func interleaveAddresses(addrs []resolver.Address) []resolver.Address { - // Group the addresses by their type and determine the order in which to - // interleave the address families. The order of interleaving the families - // is the order in which the first address of a particular type appears in - // the address list. - familyAddrsMap := map[ipAddrType][]resolver.Address{} - interleavingOrder := []ipAddrType{} + familyAddrsMap := map[ipAddrFamily][]resolver.Address{} + interleavingOrder := []ipAddrFamily{} for _, addr := range addrs { - typ := addressType(addr.Addr) - if _, found := familyAddrsMap[typ]; !found { - interleavingOrder = append(interleavingOrder, typ) + family := addressFamily(addr.Addr) + if _, found := familyAddrsMap[family]; !found { + interleavingOrder = append(interleavingOrder, family) } - familyAddrsMap[typ] = append(familyAddrsMap[typ], addr) + familyAddrsMap[family] = append(familyAddrsMap[family], addr) } interleavedAddrs := make([]resolver.Address, 0, len(addrs)) - for curTypeIndex := 0; len(interleavedAddrs) < len(addrs); curTypeIndex = (curTypeIndex + 1) % len(interleavingOrder) { + for curFamilyIdx := 0; len(interleavedAddrs) < len(addrs); curFamilyIdx = (curFamilyIdx + 1) % len(interleavingOrder) { // Some IP types may have fewer addresses than others, so we look for // the next type that has a remaining member to add to the interleaved // list. - typ := interleavingOrder[curTypeIndex] - remainingMembers := familyAddrsMap[typ] + family := interleavingOrder[curFamilyIdx] + remainingMembers := familyAddrsMap[family] if len(remainingMembers) > 0 { interleavedAddrs = append(interleavedAddrs, remainingMembers[0]) - familyAddrsMap[typ] = remainingMembers[1:] + familyAddrsMap[family] = remainingMembers[1:] } } return interleavedAddrs } -func addressType(address string) ipAddrType { +func addressFamily(address string) ipAddrFamily { // Try parsing addresses without a port specified. ip := net.ParseIP(address) if ip == nil { // Try to parse the IP after removing the port. host, _, err := net.SplitHostPort(address) if err != nil { - return ipTypeUnknown + return ipAddrFamilyUnknown } ip = net.ParseIP(host) } - // If using the passthrough resolver, the hostnames would be unresolved - // and therefore not valid IP addresses. - if ip == nil { - return ipTypeUnknown - } - - if ip.To4() != nil { - return ipv4 - } else if ip.To16() != nil { - return ipv6 + // If using a resolver like passthrough, the hostnames may not be IP + // addresses but in some format that the dialer can parse. + switch { + case ip == nil: + return ipAddrFamilyUnknown + case ip.To4() != nil: + return ipAddrFamilyV4 + case ip.To16() != nil: + return ipAddrFamilyV6 + default: + return ipAddrFamilyUnknown } - return ipTypeUnknown } // reconcileSubConnsLocked updates the active subchannels based on a new address diff --git a/balancer/pickfirst/pickfirstleaf/pickfirstleaf_ext_test.go b/balancer/pickfirst/pickfirstleaf/pickfirstleaf_ext_test.go index 9f90f6bde8b5..6d46231ae8aa 100644 --- a/balancer/pickfirst/pickfirstleaf/pickfirstleaf_ext_test.go +++ b/balancer/pickfirst/pickfirstleaf/pickfirstleaf_ext_test.go @@ -114,7 +114,7 @@ func setupPickFirstLeaf(t *testing.T, backendCount int, opts ...grpc.DialOption) // of the servers is running. // 2. RPCs are sent to verify they reach the running server. // -// The state transitions of the ClientConn and all the subconns created are +// The state transitions of the ClientConn and all the SubConns created are // verified. func (s) TestPickFirstLeaf_SimpleResolverUpdate_FirstServerReady(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) @@ -144,7 +144,7 @@ func (s) TestPickFirstLeaf_SimpleResolverUpdate_FirstServerReady(t *testing.T) { {Addrs: []resolver.Address{addrs[0]}, State: connectivity.Ready}, } if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" { - t.Errorf("subconn states mismatch (-want +got):\n%s", diff) + t.Errorf("SubConn states mismatch (-want +got):\n%s", diff) } wantConnStateTransitions := []connectivity.State{ @@ -186,7 +186,7 @@ func (s) TestPickFirstLeaf_SimpleResolverUpdate_FirstServerUnReady(t *testing.T) {Addrs: []resolver.Address{addrs[1]}, State: connectivity.Ready}, } if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" { - t.Errorf("subconn states mismatch (-want +got):\n%s", diff) + t.Errorf("SubConn states mismatch (-want +got):\n%s", diff) } wantConnStateTransitions := []connectivity.State{ @@ -231,7 +231,7 @@ func (s) TestPickFirstLeaf_SimpleResolverUpdate_DuplicateAddrs(t *testing.T) { {Addrs: []resolver.Address{addrs[1]}, State: connectivity.Ready}, } if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" { - t.Errorf("subconn states mismatch (-want +got):\n%s", diff) + t.Errorf("SubConn states mismatch (-want +got):\n%s", diff) } wantConnStateTransitions := []connectivity.State{ @@ -252,7 +252,7 @@ func (s) TestPickFirstLeaf_SimpleResolverUpdate_DuplicateAddrs(t *testing.T) { // running. This may not be the same server as before. // 4. RPCs are sent to verify they reach the running server. // -// The state transitions of the ClientConn and all the subconns created are +// The state transitions of the ClientConn and all the SubConns created are // verified. func (s) TestPickFirstLeaf_ResolverUpdates_DisjointLists(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) @@ -285,7 +285,7 @@ func (s) TestPickFirstLeaf_ResolverUpdates_DisjointLists(t *testing.T) { } if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" { - t.Errorf("subconn states mismatch (-want +got):\n%s", diff) + t.Errorf("SubConn states mismatch (-want +got):\n%s", diff) } bm.backends[2].S.Stop() @@ -303,7 +303,7 @@ func (s) TestPickFirstLeaf_ResolverUpdates_DisjointLists(t *testing.T) { } if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" { - t.Errorf("subconn states mismatch (-want +got):\n%s", diff) + t.Errorf("SubConn states mismatch (-want +got):\n%s", diff) } wantConnStateTransitions := []connectivity.State{ @@ -348,7 +348,7 @@ func (s) TestPickFirstLeaf_ResolverUpdates_ActiveBackendInUpdatedList(t *testing } if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" { - t.Errorf("subconn states mismatch (-want +got):\n%s", diff) + t.Errorf("SubConn states mismatch (-want +got):\n%s", diff) } bm.backends[2].S.Stop() @@ -369,7 +369,7 @@ func (s) TestPickFirstLeaf_ResolverUpdates_ActiveBackendInUpdatedList(t *testing } if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" { - t.Errorf("subconn states mismatch (-want +got):\n%s", diff) + t.Errorf("SubConn states mismatch (-want +got):\n%s", diff) } wantConnStateTransitions := []connectivity.State{ @@ -412,7 +412,7 @@ func (s) TestPickFirstLeaf_ResolverUpdates_InActiveBackendInUpdatedList(t *testi } if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" { - t.Errorf("subconn states mismatch (-want +got):\n%s", diff) + t.Errorf("SubConn states mismatch (-want +got):\n%s", diff) } bm.backends[2].S.Stop() @@ -432,7 +432,7 @@ func (s) TestPickFirstLeaf_ResolverUpdates_InActiveBackendInUpdatedList(t *testi } if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" { - t.Errorf("subconn states mismatch (-want +got):\n%s", diff) + t.Errorf("SubConn states mismatch (-want +got):\n%s", diff) } wantConnStateTransitions := []connectivity.State{ @@ -477,7 +477,7 @@ func (s) TestPickFirstLeaf_ResolverUpdates_IdenticalLists(t *testing.T) { } if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" { - t.Errorf("subconn states mismatch (-want +got):\n%s", diff) + t.Errorf("SubConn states mismatch (-want +got):\n%s", diff) } r.UpdateState(resolver.State{Addresses: []resolver.Address{addrs[0], addrs[1]}}) @@ -496,7 +496,7 @@ func (s) TestPickFirstLeaf_ResolverUpdates_IdenticalLists(t *testing.T) { } if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" { - t.Errorf("subconn states mismatch (-want +got):\n%s", diff) + t.Errorf("SubConn states mismatch (-want +got):\n%s", diff) } wantConnStateTransitions := []connectivity.State{ @@ -551,7 +551,7 @@ func (s) TestPickFirstLeaf_StopConnectedServer_FirstServerRestart(t *testing.T) } if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" { - t.Errorf("subconn states mismatch (-want +got):\n%s", diff) + t.Errorf("SubConn states mismatch (-want +got):\n%s", diff) } // Shut down the connected server. @@ -569,7 +569,7 @@ func (s) TestPickFirstLeaf_StopConnectedServer_FirstServerRestart(t *testing.T) } if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" { - t.Errorf("subconn states mismatch (-want +got):\n%s", diff) + t.Errorf("SubConn states mismatch (-want +got):\n%s", diff) } wantConnStateTransitions := []connectivity.State{ @@ -617,7 +617,7 @@ func (s) TestPickFirstLeaf_StopConnectedServer_SecondServerRestart(t *testing.T) } if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" { - t.Errorf("subconn states mismatch (-want +got):\n%s", diff) + t.Errorf("SubConn states mismatch (-want +got):\n%s", diff) } // Shut down the connected server. @@ -641,7 +641,7 @@ func (s) TestPickFirstLeaf_StopConnectedServer_SecondServerRestart(t *testing.T) } if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" { - t.Errorf("subconn states mismatch (-want +got):\n%s", diff) + t.Errorf("SubConn states mismatch (-want +got):\n%s", diff) } wantConnStateTransitions := []connectivity.State{ @@ -689,7 +689,7 @@ func (s) TestPickFirstLeaf_StopConnectedServer_SecondServerToFirst(t *testing.T) } if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" { - t.Errorf("subconn states mismatch (-want +got):\n%s", diff) + t.Errorf("SubConn states mismatch (-want +got):\n%s", diff) } // Shut down the connected server. @@ -713,7 +713,7 @@ func (s) TestPickFirstLeaf_StopConnectedServer_SecondServerToFirst(t *testing.T) } if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" { - t.Errorf("subconn states mismatch (-want +got):\n%s", diff) + t.Errorf("SubConn states mismatch (-want +got):\n%s", diff) } wantConnStateTransitions := []connectivity.State{ @@ -760,7 +760,7 @@ func (s) TestPickFirstLeaf_StopConnectedServer_FirstServerToSecond(t *testing.T) } if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" { - t.Errorf("subconn states mismatch (-want +got):\n%s", diff) + t.Errorf("SubConn states mismatch (-want +got):\n%s", diff) } // Shut down the connected server. @@ -783,7 +783,7 @@ func (s) TestPickFirstLeaf_StopConnectedServer_FirstServerToSecond(t *testing.T) } if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" { - t.Errorf("subconn states mismatch (-want +got):\n%s", diff) + t.Errorf("SubConn states mismatch (-want +got):\n%s", diff) } wantConnStateTransitions := []connectivity.State{ @@ -891,7 +891,7 @@ func (s) TestPickFirstLeaf_InterleavingIPV4Preffered(t *testing.T) { t.Fatalf("%v", err) } if diff := cmp.Diff(wantAddrs, gotAddrs); diff != "" { - t.Errorf("subconn creation order mismatch (-want +got):\n%s", diff) + t.Errorf("SubConn creation order mismatch (-want +got):\n%s", diff) } } @@ -935,31 +935,80 @@ func (s) TestPickFirstLeaf_InterleavingIPv6Preffered(t *testing.T) { t.Fatalf("%v", err) } if diff := cmp.Diff(wantAddrs, gotAddrs); diff != "" { - t.Errorf("subconn creation order mismatch (-want +got):\n%s", diff) + t.Errorf("SubConn creation order mismatch (-want +got):\n%s", diff) } } + +func (s) TestPickFirstLeaf_InterleavingUnknownPreffered(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + cc := testutils.NewBalancerClientConn(t) + bal := balancer.Get(pickfirstleaf.Name).Build(cc, balancer.BuildOptions{}) + defer bal.Close() + ccState := balancer.ClientConnState{ + ResolverState: resolver.State{ + Endpoints: []resolver.Endpoint{ + {Addresses: []resolver.Address{{Addr: "grpc.io:80"}}}, // not an IP. + {Addresses: []resolver.Address{{Addr: "1.1.1.1"}}}, // no port + {Addresses: []resolver.Address{{Addr: "2.2.2.2:2"}}}, + {Addresses: []resolver.Address{{Addr: "3.3.3.3:3"}}}, + {Addresses: []resolver.Address{{Addr: "4.4.4.4:4"}}}, + {Addresses: []resolver.Address{{Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}}}, // ipv6 with port + {Addresses: []resolver.Address{{Addr: "0002:0002:0002:0002:0002:0002:0002:0002"}}}, + {Addresses: []resolver.Address{{Addr: "0003:0003:0003:0003:0003:0003:0003:0003"}}}, + {Addresses: []resolver.Address{{Addr: "example.com:80"}}}, // not an IP. + }, + }, + } + if err := bal.UpdateClientConnState(ccState); err != nil { + t.Fatalf("UpdateClientConnState(%v) returned error: %v", ccState, err) + } + + wantAddrs := []resolver.Address{ + {Addr: "grpc.io:80"}, + {Addr: "1.1.1.1"}, + {Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}, + {Addr: "example.com:80"}, + {Addr: "2.2.2.2:2"}, + {Addr: "0002:0002:0002:0002:0002:0002:0002:0002"}, + {Addr: "3.3.3.3:3"}, + {Addr: "0003:0003:0003:0003:0003:0003:0003:0003"}, + {Addr: "4.4.4.4:4"}, + } + + gotAddrs, err := subConnAddresses(ctx, cc, 9) + if err != nil { + t.Fatalf("%v", err) + } + if diff := cmp.Diff(wantAddrs, gotAddrs); diff != "" { + t.Errorf("SubConn creation order mismatch (-want +got):\n%s", diff) + } +} + +// subConnAddresses makes the pickfirst balancer create the requested number of +// SubConns by triggering transient failures. The function returns the +// addresses of the created SubConns. func subConnAddresses(ctx context.Context, cc *testutils.BalancerClientConn, subConnCount int) ([]resolver.Address, error) { addresses := []resolver.Address{} for i := 0; i < subConnCount; i++ { select { case <-ctx.Done(): - return nil, fmt.Errorf("Context timed out waiting for SubConn") + return nil, fmt.Errorf("test timed out after creating %d subchannels, want %d", i, subConnCount) case sc := <-cc.NewSubConnCh: if len(sc.Addresses) != 1 { - return nil, fmt.Errorf("len(SubConn.Addresses) = %d, want 1", len(sc.Addresses)) + return nil, fmt.Errorf("new subchannel created with %d addresses, want 1", len(sc.Addresses)) } addresses = append(addresses, sc.Addresses[0]) sc.UpdateState(balancer.SubConnState{ConnectivityState: connectivity.Connecting}) sc.UpdateState(balancer.SubConnState{ ConnectivityState: connectivity.TransientFailure, - ConnectionError: fmt.Errorf("test error"), }) } } return addresses, nil } -// stateStoringBalancer stores the state of the subconns being created. +// stateStoringBalancer stores the state of the SubConns being created. type stateStoringBalancer struct { balancer.Balancer mu sync.Mutex From 12edd88fb8246368dd0ee7664fa472d821e5cbf5 Mon Sep 17 00:00:00 2001 From: Arjan Bal Date: Mon, 4 Nov 2024 14:22:32 +0530 Subject: [PATCH 5/6] Assume port always exists and handle IPv6 mapped IPv4 addresses --- .../pickfirst/pickfirstleaf/pickfirstleaf.go | 30 +++++++------- .../pickfirstleaf/pickfirstleaf_ext_test.go | 39 +++++++++---------- 2 files changed, 34 insertions(+), 35 deletions(-) diff --git a/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go b/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go index ba4e835fe262..e86d512c99b4 100644 --- a/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go +++ b/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go @@ -30,6 +30,7 @@ import ( "errors" "fmt" "net" + "strings" "sync" "google.golang.org/grpc/balancer" @@ -363,24 +364,23 @@ func interleaveAddresses(addrs []resolver.Address) []resolver.Address { return interleavedAddrs } +// addressFamily returns the ipAddrFamily after parsing the address string. +// If the address isn't of the format "ip-address:port", it returns +// ipAddrFamilyUnknown. The address may be valid even if it's not an IP when +// using a resolver like passthrough where the address may be a hostname in +// some format that the dialer can resolve. func addressFamily(address string) ipAddrFamily { - // Try parsing addresses without a port specified. - ip := net.ParseIP(address) - if ip == nil { - // Try to parse the IP after removing the port. - host, _, err := net.SplitHostPort(address) - if err != nil { - return ipAddrFamilyUnknown - } - ip = net.ParseIP(host) + // Parse the IP after removing the port. + host, _, err := net.SplitHostPort(address) + if err != nil { + return ipAddrFamilyUnknown } - - // If using a resolver like passthrough, the hostnames may not be IP - // addresses but in some format that the dialer can parse. + ip := net.ParseIP(host) switch { - case ip == nil: - return ipAddrFamilyUnknown - case ip.To4() != nil: + // Check for existence of IPv4-mapped IPv6 addresses, which are + // considered as IPv4 by net.IP. + // e.g: "::FFFF:127.0.0.1" + case ip.To4() != nil && !strings.Contains(host, ":"): return ipAddrFamilyV4 case ip.To16() != nil: return ipAddrFamilyV6 diff --git a/balancer/pickfirst/pickfirstleaf/pickfirstleaf_ext_test.go b/balancer/pickfirst/pickfirstleaf/pickfirstleaf_ext_test.go index 6d46231ae8aa..4647bc382ccc 100644 --- a/balancer/pickfirst/pickfirstleaf/pickfirstleaf_ext_test.go +++ b/balancer/pickfirst/pickfirstleaf/pickfirstleaf_ext_test.go @@ -13,7 +13,6 @@ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. - * */ package pickfirstleaf_test @@ -860,13 +859,13 @@ func (s) TestPickFirstLeaf_InterleavingIPV4Preffered(t *testing.T) { ccState := balancer.ClientConnState{ ResolverState: resolver.State{ Endpoints: []resolver.Endpoint{ - {Addresses: []resolver.Address{{Addr: "1.1.1.1"}}}, // no port + {Addresses: []resolver.Address{{Addr: "1.1.1.1:1111"}}}, {Addresses: []resolver.Address{{Addr: "2.2.2.2:2"}}}, {Addresses: []resolver.Address{{Addr: "3.3.3.3:3"}}}, {Addresses: []resolver.Address{{Addr: "4.4.4.4:4"}}}, {Addresses: []resolver.Address{{Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}}}, // ipv6 with port - {Addresses: []resolver.Address{{Addr: "0002:0002:0002:0002:0002:0002:0002:0002"}}}, - {Addresses: []resolver.Address{{Addr: "0003:0003:0003:0003:0003:0003:0003:0003"}}}, + {Addresses: []resolver.Address{{Addr: "[::FFFF:192.168.0.1]:2222"}}}, + {Addresses: []resolver.Address{{Addr: "[0003:0003:0003:0003:0003:0003:0003:0003]:3333"}}}, {Addresses: []resolver.Address{{Addr: "grpc.io:80"}}}, // not an IP. }, }, @@ -876,13 +875,13 @@ func (s) TestPickFirstLeaf_InterleavingIPV4Preffered(t *testing.T) { } wantAddrs := []resolver.Address{ - {Addr: "1.1.1.1"}, + {Addr: "1.1.1.1:1111"}, {Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}, {Addr: "grpc.io:80"}, {Addr: "2.2.2.2:2"}, - {Addr: "0002:0002:0002:0002:0002:0002:0002:0002"}, + {Addr: "[::FFFF:192.168.0.1]:2222"}, {Addr: "3.3.3.3:3"}, - {Addr: "0003:0003:0003:0003:0003:0003:0003:0003"}, + {Addr: "[0003:0003:0003:0003:0003:0003:0003:0003]:3333"}, {Addr: "4.4.4.4:4"}, } @@ -905,12 +904,12 @@ func (s) TestPickFirstLeaf_InterleavingIPv6Preffered(t *testing.T) { ResolverState: resolver.State{ Endpoints: []resolver.Endpoint{ {Addresses: []resolver.Address{{Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}}}, // ipv6 with port - {Addresses: []resolver.Address{{Addr: "1.1.1.1"}}}, // no port + {Addresses: []resolver.Address{{Addr: "1.1.1.1:1111"}}}, {Addresses: []resolver.Address{{Addr: "2.2.2.2:2"}}}, {Addresses: []resolver.Address{{Addr: "3.3.3.3:3"}}}, {Addresses: []resolver.Address{{Addr: "4.4.4.4:4"}}}, - {Addresses: []resolver.Address{{Addr: "0002:0002:0002:0002:0002:0002:0002:0002"}}}, - {Addresses: []resolver.Address{{Addr: "0003:0003:0003:0003:0003:0003:0003:0003"}}}, + {Addresses: []resolver.Address{{Addr: "[::FFFF:192.168.0.1]:2222"}}}, + {Addresses: []resolver.Address{{Addr: "[0003:0003:0003:0003:0003:0003:0003:0003]:3333"}}}, {Addresses: []resolver.Address{{Addr: "grpc.io:80"}}}, // not an IP. }, }, @@ -921,11 +920,11 @@ func (s) TestPickFirstLeaf_InterleavingIPv6Preffered(t *testing.T) { wantAddrs := []resolver.Address{ {Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}, - {Addr: "1.1.1.1"}, + {Addr: "1.1.1.1:1111"}, {Addr: "grpc.io:80"}, - {Addr: "0002:0002:0002:0002:0002:0002:0002:0002"}, + {Addr: "[::FFFF:192.168.0.1]:2222"}, {Addr: "2.2.2.2:2"}, - {Addr: "0003:0003:0003:0003:0003:0003:0003:0003"}, + {Addr: "[0003:0003:0003:0003:0003:0003:0003:0003]:3333"}, {Addr: "3.3.3.3:3"}, {Addr: "4.4.4.4:4"}, } @@ -949,13 +948,13 @@ func (s) TestPickFirstLeaf_InterleavingUnknownPreffered(t *testing.T) { ResolverState: resolver.State{ Endpoints: []resolver.Endpoint{ {Addresses: []resolver.Address{{Addr: "grpc.io:80"}}}, // not an IP. - {Addresses: []resolver.Address{{Addr: "1.1.1.1"}}}, // no port + {Addresses: []resolver.Address{{Addr: "1.1.1.1:1111"}}}, {Addresses: []resolver.Address{{Addr: "2.2.2.2:2"}}}, {Addresses: []resolver.Address{{Addr: "3.3.3.3:3"}}}, {Addresses: []resolver.Address{{Addr: "4.4.4.4:4"}}}, - {Addresses: []resolver.Address{{Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}}}, // ipv6 with port - {Addresses: []resolver.Address{{Addr: "0002:0002:0002:0002:0002:0002:0002:0002"}}}, - {Addresses: []resolver.Address{{Addr: "0003:0003:0003:0003:0003:0003:0003:0003"}}}, + {Addresses: []resolver.Address{{Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}}}, + {Addresses: []resolver.Address{{Addr: "[::FFFF:192.168.0.1]:2222"}}}, + {Addresses: []resolver.Address{{Addr: "[0003:0003:0003:0003:0003:0003:0003:0003]:3333"}}}, {Addresses: []resolver.Address{{Addr: "example.com:80"}}}, // not an IP. }, }, @@ -966,13 +965,13 @@ func (s) TestPickFirstLeaf_InterleavingUnknownPreffered(t *testing.T) { wantAddrs := []resolver.Address{ {Addr: "grpc.io:80"}, - {Addr: "1.1.1.1"}, + {Addr: "1.1.1.1:1111"}, {Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}, {Addr: "example.com:80"}, {Addr: "2.2.2.2:2"}, - {Addr: "0002:0002:0002:0002:0002:0002:0002:0002"}, + {Addr: "[::FFFF:192.168.0.1]:2222"}, {Addr: "3.3.3.3:3"}, - {Addr: "0003:0003:0003:0003:0003:0003:0003:0003"}, + {Addr: "[0003:0003:0003:0003:0003:0003:0003:0003]:3333"}, {Addr: "4.4.4.4:4"}, } From 4d5c3a9effda0dea20d988295c72d9d85d348002 Mon Sep 17 00:00:00 2001 From: Arjan Bal Date: Wed, 6 Nov 2024 11:16:32 +0530 Subject: [PATCH 6/6] Let IPv4-mapped v6 addresses be considered IPv4 --- .../pickfirst/pickfirstleaf/pickfirstleaf.go | 6 +---- .../pickfirstleaf/pickfirstleaf_ext_test.go | 26 ++++++++++--------- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go b/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go index e86d512c99b4..4b54866058d5 100644 --- a/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go +++ b/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go @@ -30,7 +30,6 @@ import ( "errors" "fmt" "net" - "strings" "sync" "google.golang.org/grpc/balancer" @@ -377,10 +376,7 @@ func addressFamily(address string) ipAddrFamily { } ip := net.ParseIP(host) switch { - // Check for existence of IPv4-mapped IPv6 addresses, which are - // considered as IPv4 by net.IP. - // e.g: "::FFFF:127.0.0.1" - case ip.To4() != nil && !strings.Contains(host, ":"): + case ip.To4() != nil: return ipAddrFamilyV4 case ip.To16() != nil: return ipAddrFamilyV6 diff --git a/balancer/pickfirst/pickfirstleaf/pickfirstleaf_ext_test.go b/balancer/pickfirst/pickfirstleaf/pickfirstleaf_ext_test.go index 4647bc382ccc..46e47be43ffa 100644 --- a/balancer/pickfirst/pickfirstleaf/pickfirstleaf_ext_test.go +++ b/balancer/pickfirst/pickfirstleaf/pickfirstleaf_ext_test.go @@ -862,9 +862,11 @@ func (s) TestPickFirstLeaf_InterleavingIPV4Preffered(t *testing.T) { {Addresses: []resolver.Address{{Addr: "1.1.1.1:1111"}}}, {Addresses: []resolver.Address{{Addr: "2.2.2.2:2"}}}, {Addresses: []resolver.Address{{Addr: "3.3.3.3:3"}}}, - {Addresses: []resolver.Address{{Addr: "4.4.4.4:4"}}}, - {Addresses: []resolver.Address{{Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}}}, // ipv6 with port + // IPv4-mapped IPv6 address, considered as an IPv4 for + // interleaving. {Addresses: []resolver.Address{{Addr: "[::FFFF:192.168.0.1]:2222"}}}, + {Addresses: []resolver.Address{{Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}}}, + {Addresses: []resolver.Address{{Addr: "[0002:0002:0002:0002:0002:0002:0002:0002]:8080"}}}, {Addresses: []resolver.Address{{Addr: "[0003:0003:0003:0003:0003:0003:0003:0003]:3333"}}}, {Addresses: []resolver.Address{{Addr: "grpc.io:80"}}}, // not an IP. }, @@ -879,10 +881,10 @@ func (s) TestPickFirstLeaf_InterleavingIPV4Preffered(t *testing.T) { {Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}, {Addr: "grpc.io:80"}, {Addr: "2.2.2.2:2"}, - {Addr: "[::FFFF:192.168.0.1]:2222"}, + {Addr: "[0002:0002:0002:0002:0002:0002:0002:0002]:8080"}, {Addr: "3.3.3.3:3"}, {Addr: "[0003:0003:0003:0003:0003:0003:0003:0003]:3333"}, - {Addr: "4.4.4.4:4"}, + {Addr: "[::FFFF:192.168.0.1]:2222"}, } gotAddrs, err := subConnAddresses(ctx, cc, 8) @@ -903,12 +905,12 @@ func (s) TestPickFirstLeaf_InterleavingIPv6Preffered(t *testing.T) { ccState := balancer.ClientConnState{ ResolverState: resolver.State{ Endpoints: []resolver.Endpoint{ - {Addresses: []resolver.Address{{Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}}}, // ipv6 with port + {Addresses: []resolver.Address{{Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}}}, {Addresses: []resolver.Address{{Addr: "1.1.1.1:1111"}}}, {Addresses: []resolver.Address{{Addr: "2.2.2.2:2"}}}, {Addresses: []resolver.Address{{Addr: "3.3.3.3:3"}}}, - {Addresses: []resolver.Address{{Addr: "4.4.4.4:4"}}}, {Addresses: []resolver.Address{{Addr: "[::FFFF:192.168.0.1]:2222"}}}, + {Addresses: []resolver.Address{{Addr: "[0002:0002:0002:0002:0002:0002:0002:0002]:2222"}}}, {Addresses: []resolver.Address{{Addr: "[0003:0003:0003:0003:0003:0003:0003:0003]:3333"}}}, {Addresses: []resolver.Address{{Addr: "grpc.io:80"}}}, // not an IP. }, @@ -922,11 +924,11 @@ func (s) TestPickFirstLeaf_InterleavingIPv6Preffered(t *testing.T) { {Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}, {Addr: "1.1.1.1:1111"}, {Addr: "grpc.io:80"}, - {Addr: "[::FFFF:192.168.0.1]:2222"}, + {Addr: "[0002:0002:0002:0002:0002:0002:0002:0002]:2222"}, {Addr: "2.2.2.2:2"}, {Addr: "[0003:0003:0003:0003:0003:0003:0003:0003]:3333"}, {Addr: "3.3.3.3:3"}, - {Addr: "4.4.4.4:4"}, + {Addr: "[::FFFF:192.168.0.1]:2222"}, } gotAddrs, err := subConnAddresses(ctx, cc, 8) @@ -951,9 +953,9 @@ func (s) TestPickFirstLeaf_InterleavingUnknownPreffered(t *testing.T) { {Addresses: []resolver.Address{{Addr: "1.1.1.1:1111"}}}, {Addresses: []resolver.Address{{Addr: "2.2.2.2:2"}}}, {Addresses: []resolver.Address{{Addr: "3.3.3.3:3"}}}, - {Addresses: []resolver.Address{{Addr: "4.4.4.4:4"}}}, - {Addresses: []resolver.Address{{Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}}}, {Addresses: []resolver.Address{{Addr: "[::FFFF:192.168.0.1]:2222"}}}, + {Addresses: []resolver.Address{{Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}}}, + {Addresses: []resolver.Address{{Addr: "[0002:0002:0002:0002:0002:0002:0002:0002]:8080"}}}, {Addresses: []resolver.Address{{Addr: "[0003:0003:0003:0003:0003:0003:0003:0003]:3333"}}}, {Addresses: []resolver.Address{{Addr: "example.com:80"}}}, // not an IP. }, @@ -969,10 +971,10 @@ func (s) TestPickFirstLeaf_InterleavingUnknownPreffered(t *testing.T) { {Addr: "[0001:0001:0001:0001:0001:0001:0001:0001]:8080"}, {Addr: "example.com:80"}, {Addr: "2.2.2.2:2"}, - {Addr: "[::FFFF:192.168.0.1]:2222"}, + {Addr: "[0002:0002:0002:0002:0002:0002:0002:0002]:8080"}, {Addr: "3.3.3.3:3"}, {Addr: "[0003:0003:0003:0003:0003:0003:0003:0003]:3333"}, - {Addr: "4.4.4.4:4"}, + {Addr: "[::FFFF:192.168.0.1]:2222"}, } gotAddrs, err := subConnAddresses(ctx, cc, 9)