Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions spanner/grpc_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,7 @@ import (
"google.golang.org/grpc/status"
)

const (
directPathIPV6Prefix = "[2001:4860:8040"
directPathIPV4Prefix = "34.126"
)


type contextKey string

Expand Down
21 changes: 10 additions & 11 deletions spanner/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ import (
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/grpc/credentials/alts"
"google.golang.org/grpc/peer"
"google.golang.org/grpc/status"
"google.golang.org/protobuf/proto"
Expand Down Expand Up @@ -6608,22 +6609,20 @@ func verifyDirectPathRemoteAddress(t *testing.T) {
return
}
if remoteIP, res := isDirectPathRemoteAddress(); !res {
if dpConfig.directPathIPv4Only {
t.Fatalf("Expect to access DirectPath via ipv4 only, but RPC was destined to %s", remoteIP)
} else {
t.Fatalf("Expect to access DirectPath via ipv4 or ipv6, but RPC was destined to %s", remoteIP)
}
t.Fatalf("Expect to access DirectPath via ALTS, but RPC was destined to %s", remoteIP)
}
}

func isDirectPathRemoteAddress() (_ string, _ bool) {
remoteIP := peerInfo.Addr.String()
// DirectPath ipv4-only can only use ipv4 traffic.
if dpConfig.directPathIPv4Only {
return remoteIP, strings.HasPrefix(remoteIP, directPathIPV4Prefix)
var remoteIP string
if peerInfo != nil && peerInfo.Addr != nil {
remoteIP = peerInfo.Addr.String()
}
isDP := false
if peerInfo != nil && peerInfo.AuthInfo != nil {
_, isDP = peerInfo.AuthInfo.(alts.AuthInfo)
}
// DirectPath ipv6 can use either ipv4 or ipv6 traffic.
return remoteIP, strings.HasPrefix(remoteIP, directPathIPV4Prefix) || strings.HasPrefix(remoteIP, directPathIPV6Prefix)
return remoteIP, isDP
}

// examineTraffic counts RPCs use DirectPath or CFE traffic.
Expand Down
6 changes: 3 additions & 3 deletions spanner/metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import (
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/experimental/stats"
"google.golang.org/grpc/credentials/alts"
"google.golang.org/grpc/peer"
"google.golang.org/grpc/stats/opentelemetry"
"google.golang.org/grpc/status"
Expand Down Expand Up @@ -533,9 +534,8 @@ func (o *opTracer) incrementAttemptCount() {
// setDirectPathUsed sets whether DirectPath was used for the attempt.
func (a *attemptTracer) setDirectPathUsed(ctx context.Context) {
peerInfo, ok := peer.FromContext(ctx)
if ok && peerInfo.Addr != nil {
remoteIP := peerInfo.Addr.String()
if strings.HasPrefix(remoteIP, directPathIPV4Prefix) || strings.HasPrefix(remoteIP, directPathIPV6Prefix) {
if ok && peerInfo.AuthInfo != nil {
if _, isALTS := peerInfo.AuthInfo.(alts.AuthInfo); isALTS {
a.directPathUsed = true
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The check peerInfo.AuthInfo != nil is redundant. The type assertion _, isALTS := peerInfo.AuthInfo.(alts.AuthInfo) will safely handle the case where peerInfo.AuthInfo is nil by setting isALTS to false. You can simplify the condition.

Suggested change
if ok && peerInfo.AuthInfo != nil {
if _, isALTS := peerInfo.AuthInfo.(alts.AuthInfo); isALTS {
a.directPathUsed = true
}
}
if ok {
if _, isALTS := peerInfo.AuthInfo.(alts.AuthInfo); isALTS {
a.directPathUsed = true
}
}

Expand Down
57 changes: 57 additions & 0 deletions spanner/metrics_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ import (
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/grpc/credentials/alts"
"google.golang.org/grpc/peer"
"google.golang.org/grpc/status"
)

Expand Down Expand Up @@ -314,3 +316,58 @@ func parseHex(hexStr string) (int64, error) {
_, err := fmt.Sscanf(hexStr, "%x", &value)
return value, err
}

type mockALTSAuthInfo struct {
alts.AuthInfo
}
func (m mockALTSAuthInfo) AuthType() string { return "alts" }

type mockOtherAuthInfo struct{}
func (m mockOtherAuthInfo) AuthType() string { return "other" }

func TestSetDirectPathUsed(t *testing.T) {
tests := []struct {
name string
peer *peer.Peer
want bool
}{
{
name: "ALTS AuthInfo",
peer: &peer.Peer{
AuthInfo: mockALTSAuthInfo{},
},
want: true,
},
{
name: "Other AuthInfo",
peer: &peer.Peer{
AuthInfo: mockOtherAuthInfo{},
},
want: false,
},
{
name: "No AuthInfo",
peer: &peer.Peer{},
want: false,
},
{
name: "No Peer",
peer: nil,
want: false,
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
tracer := &attemptTracer{}
ctx := context.Background()
if tc.peer != nil {
ctx = peer.NewContext(ctx, tc.peer)
}
tracer.setDirectPathUsed(ctx)
if tracer.directPathUsed != tc.want {
t.Errorf("setDirectPathUsed() = %v, want %v", tracer.directPathUsed, tc.want)
}
})
}
}
Loading