From 83118abcd7e03d173a57c35da39de760b93ed05c Mon Sep 17 00:00:00 2001 From: Yolan Romailler Date: Wed, 17 Apr 2024 17:20:07 +0200 Subject: [PATCH 1/9] Satisfy the Stringer interface for Peer and Metadata Effectively allowing to print context containing Peers properly Also enabling logging of Metadata in an easier way RELEASE NOTES: - peer/peer: implement the Stringer interface for pretty-printing Peers - metadata/metadata: implement the Stringer interface for pretty printing metadata Signed-off-by: Yolan Romailler --- metadata/metadata.go | 16 +++++ metadata/metadata_test.go | 21 ++++++ peer/peer.go | 21 ++++++ peer/peer_test.go | 145 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 203 insertions(+) create mode 100644 peer/peer_test.go diff --git a/metadata/metadata.go b/metadata/metadata.go index 1e9485fd6e2..38674cd381e 100644 --- a/metadata/metadata.go +++ b/metadata/metadata.go @@ -90,6 +90,22 @@ func Pairs(kv ...string) MD { return md } +// String implements the Stringer interface for pretty-printing a MD. Ordering of the values is non-deterministic as it ranges over a map. +func (md MD) String() string { + var sb strings.Builder + fmt.Fprintf(&sb, "MD{") + needSep := false + for k, v := range md { + if needSep { + fmt.Fprintf(&sb, ", ") + } + fmt.Fprintf(&sb, "%s=[%s]", k, strings.Join(v, ", ")) + needSep = true + } + fmt.Fprintf(&sb, "}") + return sb.String() +} + // Len returns the number of items in md. func (md MD) Len() int { return len(md) diff --git a/metadata/metadata_test.go b/metadata/metadata_test.go index fbee086fb91..5b562438263 100644 --- a/metadata/metadata_test.go +++ b/metadata/metadata_test.go @@ -22,6 +22,7 @@ import ( "context" "reflect" "strconv" + "strings" "testing" "time" @@ -338,6 +339,26 @@ func (s) TestAppendToOutgoingContext_FromKVSlice(t *testing.T) { } } +func TestStringerMD(t *testing.T) { + for _, test := range []struct { + md MD + want []string + }{ + {MD{}, []string{"MD{}"}}, + {MD{"k1": []string{}}, []string{"MD{k1=[]}"}}, + {MD{"k1": []string{"v1", "v2"}}, []string{"MD{k1=[v1, v2]}"}}, + {MD{"k1": []string{"v1"}}, []string{"MD{k1=[v1]}"}}, + {MD{"k1": []string{"v1", "v2"}, "k2": []string{}, "k3": []string{"1", "2", "3"}}, []string{"MD{", "k1=[v1, v2]", "k2=[]", "k3=[1, 2, 3]", "}"}}, + } { + strMd := test.md.String() + for _, want := range test.want { + if !strings.Contains(strMd, want) { + t.Fatalf("Metadata string '%s' is missing '%s'", strMd, want) + } + } + } +} + // Old/slow approach to adding metadata to context func Benchmark_AddingMetadata_ContextManipulationApproach(b *testing.B) { // TODO: Add in N=1-100 tests once Go1.6 support is removed. diff --git a/peer/peer.go b/peer/peer.go index a821ff9b2b7..d4155943e9f 100644 --- a/peer/peer.go +++ b/peer/peer.go @@ -22,6 +22,7 @@ package peer import ( "context" + "fmt" "net" "google.golang.org/grpc/credentials" @@ -39,6 +40,26 @@ type Peer struct { AuthInfo credentials.AuthInfo } +// String ensures the Peer types implements the Stringer interface in order to +// allow to print a context with a peerKey value effectively. +func (p *Peer) String() string { + if p == nil { + return "Peer" + } + ret := "Peer{" + if p.Addr != nil { + ret += fmt.Sprintf("Addr: '%s'", p.Addr.String()) + } + if p.AuthInfo != nil { + if len(ret) > 5 { + ret += ", " + } + ret += fmt.Sprintf("AuthInfo: '%s'", p.AuthInfo.AuthType()) + } + ret += "}" + return ret +} + type peerKey struct{} // NewContext creates a new context with peer information attached. diff --git a/peer/peer_test.go b/peer/peer_test.go new file mode 100644 index 00000000000..36145c1a374 --- /dev/null +++ b/peer/peer_test.go @@ -0,0 +1,145 @@ +/* + * + * Copyright 2024 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * 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 peer + +import ( + "context" + "fmt" + "testing" + + "google.golang.org/grpc/credentials" +) + +// A struct that implements AuthInfo interface and implements CommonAuthInfo() method. +type testAuthInfo struct { + credentials.CommonAuthInfo +} + +func (ta testAuthInfo) AuthType() string { + return "testAuthInfo" +} + +func TestPeerSecurityLevel(t *testing.T) { + testCases := []struct { + authLevel credentials.SecurityLevel + testLevel credentials.SecurityLevel + want bool + }{ + { + authLevel: credentials.PrivacyAndIntegrity, + testLevel: credentials.PrivacyAndIntegrity, + want: true, + }, + { + authLevel: credentials.IntegrityOnly, + testLevel: credentials.PrivacyAndIntegrity, + want: false, + }, + { + authLevel: credentials.IntegrityOnly, + testLevel: credentials.NoSecurity, + want: true, + }, + { + authLevel: credentials.InvalidSecurityLevel, + testLevel: credentials.IntegrityOnly, + want: true, + }, + { + authLevel: credentials.InvalidSecurityLevel, + testLevel: credentials.PrivacyAndIntegrity, + want: true, + }, + } + for _, tc := range testCases { + ctx := NewContext(context.Background(), &Peer{AuthInfo: testAuthInfo{credentials.CommonAuthInfo{SecurityLevel: tc.authLevel}}}) + p, ok := FromContext(ctx) + if !ok { + t.Fatalf("Unable to get peer from context") + } + err := credentials.CheckSecurityLevel(p.AuthInfo, tc.testLevel) + if tc.want && (err != nil) { + t.Fatalf("CheckSeurityLevel(%s, %s) returned failure but want success", tc.authLevel.String(), tc.testLevel.String()) + } else if !tc.want && (err == nil) { + t.Fatalf("CheckSeurityLevel(%s, %s) returned success but want failure", tc.authLevel.String(), tc.testLevel.String()) + } + } +} + +type addr struct { + ipAddress string +} + +func (addr) Network() string { return "" } +func (a *addr) String() string { return a.ipAddress } + +func TestPeerStringer(t *testing.T) { + testCases := []struct { + addr *addr + authLevel credentials.SecurityLevel + want string + }{ + { + addr: &addr{"example.com:1234"}, + authLevel: credentials.PrivacyAndIntegrity, + want: "Peer{Addr: 'example.com:1234', AuthInfo: 'testAuthInfo'}", + }, + { + addr: &addr{"1.2.3.4:1234"}, + authLevel: -1, + want: "Peer{Addr: '1.2.3.4:1234'}", + }, + { + authLevel: credentials.InvalidSecurityLevel, + want: "Peer{AuthInfo: 'testAuthInfo'}", + }, + { + authLevel: -1, + want: "Peer{}", + }, + } + for _, tc := range testCases { + ctx := NewContext(context.Background(), &Peer{Addr: tc.addr, AuthInfo: testAuthInfo{credentials.CommonAuthInfo{SecurityLevel: tc.authLevel}}}) + p, ok := FromContext(ctx) + if tc.authLevel == -1 { + p.AuthInfo = nil + } + if tc.addr == nil { + p.Addr = nil + } + if !ok { + t.Fatalf("Unable to get peer from context") + } + if p.String() != tc.want { + t.Fatalf("Error using peer String(): expected %q, got %q", tc.want, p.String()) + } + } + t.Run("test String on nil Peer", func(st *testing.T) { + var test *Peer + if test.String() != "Peer" { + st.Fatalf("Error using String on nil Peer. Expected 'Peer', got: '%s'", test.String()) + } + }) + t.Run("test Stringer on context", func(st *testing.T) { + ctx := NewContext(context.Background(), &Peer{Addr: &addr{"1.2.3.4:1234"}, AuthInfo: testAuthInfo{credentials.CommonAuthInfo{SecurityLevel: credentials.PrivacyAndIntegrity}}}) + if fmt.Sprintf("%v", ctx) != "context.Background.WithValue(type peer.peerKey, val Peer{Addr: '1.2.3.4:1234', AuthInfo: 'testAuthInfo'})" { + st.Fatalf("Error printing context with embedded Peer. Got: %v", ctx) + } + }) +} From 20ee25cc21afa50888d5f2973e43ece680b21361 Mon Sep 17 00:00:00 2001 From: Yolan Romailler Date: Mon, 22 Apr 2024 17:43:26 +0200 Subject: [PATCH 2/9] Apply code review comment Signed-off-by: Yolan Romailler --- metadata/metadata.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/metadata/metadata.go b/metadata/metadata.go index 38674cd381e..0e5b3184114 100644 --- a/metadata/metadata.go +++ b/metadata/metadata.go @@ -94,13 +94,11 @@ func Pairs(kv ...string) MD { func (md MD) String() string { var sb strings.Builder fmt.Fprintf(&sb, "MD{") - needSep := false for k, v := range md { - if needSep { + if sb.Len() > 3 { fmt.Fprintf(&sb, ", ") } fmt.Fprintf(&sb, "%s=[%s]", k, strings.Join(v, ", ")) - needSep = true } fmt.Fprintf(&sb, "}") return sb.String() From 172603ccef3b8658e7f630e16085e31a6688269a Mon Sep 17 00:00:00 2001 From: Yolan Romailler Date: Mon, 22 Apr 2024 21:12:03 +0200 Subject: [PATCH 3/9] Improve Stringer interface on acBalancerWrapper to allow human identification of which SubConn is used Protect access to curAddr with ac.mutex Signed-off-by: Yolan Romailler --- balancer_wrapper.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/balancer_wrapper.go b/balancer_wrapper.go index af39b8a4c73..395d18fc8be 100644 --- a/balancer_wrapper.go +++ b/balancer_wrapper.go @@ -261,7 +261,9 @@ func (acbw *acBalancerWrapper) updateState(s connectivity.State, err error) { } func (acbw *acBalancerWrapper) String() string { - return fmt.Sprintf("SubConn(id:%d)", acbw.ac.channelz.ID) + acbw.ac.mu.Lock() + defer acbw.ac.mu.Unlock() + return fmt.Sprintf("SubConn(id:%d;addr:%s)", acbw.ac.channelz.ID, acbw.ac.curAddr.Addr) } func (acbw *acBalancerWrapper) UpdateAddresses(addrs []resolver.Address) { From edfd323c7ef7245ab268923b5ab020ae168032b8 Mon Sep 17 00:00:00 2001 From: Yolan Romailler Date: Mon, 22 Apr 2024 21:37:18 +0200 Subject: [PATCH 4/9] Use a strings.Builder in Peer.String with explicit nil fields Signed-off-by: Yolan Romailler --- peer/peer.go | 20 ++++++++++++-------- peer/peer_test.go | 6 +++--- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/peer/peer.go b/peer/peer.go index d4155943e9f..e41d90d8451 100644 --- a/peer/peer.go +++ b/peer/peer.go @@ -24,6 +24,7 @@ import ( "context" "fmt" "net" + "strings" "google.golang.org/grpc/credentials" ) @@ -46,18 +47,21 @@ func (p *Peer) String() string { if p == nil { return "Peer" } - ret := "Peer{" + sb := &strings.Builder{} + sb.WriteString("Peer{") if p.Addr != nil { - ret += fmt.Sprintf("Addr: '%s'", p.Addr.String()) + fmt.Fprintf(sb, "Addr: '%s', ", p.Addr.String()) + } else { + fmt.Fprintf(sb, "Addr: , ") } if p.AuthInfo != nil { - if len(ret) > 5 { - ret += ", " - } - ret += fmt.Sprintf("AuthInfo: '%s'", p.AuthInfo.AuthType()) + fmt.Fprintf(sb, "AuthInfo: '%s'", p.AuthInfo.AuthType()) + } else { + fmt.Fprintf(sb, "AuthInfo: ") } - ret += "}" - return ret + sb.WriteString("}") + + return sb.String() } type peerKey struct{} diff --git a/peer/peer_test.go b/peer/peer_test.go index 36145c1a374..97f34a63d3d 100644 --- a/peer/peer_test.go +++ b/peer/peer_test.go @@ -103,15 +103,15 @@ func TestPeerStringer(t *testing.T) { { addr: &addr{"1.2.3.4:1234"}, authLevel: -1, - want: "Peer{Addr: '1.2.3.4:1234'}", + want: "Peer{Addr: '1.2.3.4:1234', AuthInfo: }", }, { authLevel: credentials.InvalidSecurityLevel, - want: "Peer{AuthInfo: 'testAuthInfo'}", + want: "Peer{Addr: , AuthInfo: 'testAuthInfo'}", }, { authLevel: -1, - want: "Peer{}", + want: "Peer{Addr: , AuthInfo: }", }, } for _, tc := range testCases { From 985347ed7e8192c5b650dcaef371ff83032a5b4f Mon Sep 17 00:00:00 2001 From: Yolan Romailler Date: Wed, 24 Apr 2024 11:54:35 +0200 Subject: [PATCH 5/9] Revert "Improve Stringer interface on acBalancerWrapper to allow human identification of which SubConn is used" This reverts commit 001f394f81ecf20a1383290b6415425402b839c0. Signed-off-by: Yolan Romailler --- balancer_wrapper.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/balancer_wrapper.go b/balancer_wrapper.go index 395d18fc8be..af39b8a4c73 100644 --- a/balancer_wrapper.go +++ b/balancer_wrapper.go @@ -261,9 +261,7 @@ func (acbw *acBalancerWrapper) updateState(s connectivity.State, err error) { } func (acbw *acBalancerWrapper) String() string { - acbw.ac.mu.Lock() - defer acbw.ac.mu.Unlock() - return fmt.Sprintf("SubConn(id:%d;addr:%s)", acbw.ac.channelz.ID, acbw.ac.curAddr.Addr) + return fmt.Sprintf("SubConn(id:%d)", acbw.ac.channelz.ID) } func (acbw *acBalancerWrapper) UpdateAddresses(addrs []resolver.Address) { From 067b07e277418fa2130d5c76d040e306ea9e6ef0 Mon Sep 17 00:00:00 2001 From: Yolan Romailler Date: Wed, 1 May 2024 18:50:25 +0200 Subject: [PATCH 6/9] Addressing code review comments Signed-off-by: Yolan Romailler --- metadata/metadata.go | 3 +- peer/peer.go | 5 +++ peer/peer_test.go | 101 +++++++++++++++++++++---------------------- 3 files changed, 57 insertions(+), 52 deletions(-) diff --git a/metadata/metadata.go b/metadata/metadata.go index 0e5b3184114..6c01a9b359c 100644 --- a/metadata/metadata.go +++ b/metadata/metadata.go @@ -90,7 +90,8 @@ func Pairs(kv ...string) MD { return md } -// String implements the Stringer interface for pretty-printing a MD. Ordering of the values is non-deterministic as it ranges over a map. +// String implements the Stringer interface for pretty-printing a MD. +// Ordering of the values is non-deterministic as it ranges over a map. func (md MD) String() string { var sb strings.Builder fmt.Fprintf(&sb, "MD{") diff --git a/peer/peer.go b/peer/peer.go index e41d90d8451..499a49c8c1c 100644 --- a/peer/peer.go +++ b/peer/peer.go @@ -54,6 +54,11 @@ func (p *Peer) String() string { } else { fmt.Fprintf(sb, "Addr: , ") } + if p.LocalAddr != nil { + fmt.Fprintf(sb, "LocalAddr: '%s', ", p.LocalAddr.String()) + } else { + fmt.Fprintf(sb, "LocalAddr: , ") + } if p.AuthInfo != nil { fmt.Fprintf(sb, "AuthInfo: '%s'", p.AuthInfo.AuthType()) } else { diff --git a/peer/peer_test.go b/peer/peer_test.go index 97f34a63d3d..d7ac9e3313b 100644 --- a/peer/peer_test.go +++ b/peer/peer_test.go @@ -21,6 +21,7 @@ package peer import ( "context" "fmt" + "strings" "testing" "google.golang.org/grpc/credentials" @@ -32,7 +33,7 @@ type testAuthInfo struct { } func (ta testAuthInfo) AuthType() string { - return "testAuthInfo" + return fmt.Sprintf("testAuthInfo-%d", ta.SecurityLevel) } func TestPeerSecurityLevel(t *testing.T) { @@ -68,17 +69,19 @@ func TestPeerSecurityLevel(t *testing.T) { }, } for _, tc := range testCases { - ctx := NewContext(context.Background(), &Peer{AuthInfo: testAuthInfo{credentials.CommonAuthInfo{SecurityLevel: tc.authLevel}}}) - p, ok := FromContext(ctx) - if !ok { - t.Fatalf("Unable to get peer from context") - } - err := credentials.CheckSecurityLevel(p.AuthInfo, tc.testLevel) - if tc.want && (err != nil) { - t.Fatalf("CheckSeurityLevel(%s, %s) returned failure but want success", tc.authLevel.String(), tc.testLevel.String()) - } else if !tc.want && (err == nil) { - t.Fatalf("CheckSeurityLevel(%s, %s) returned success but want failure", tc.authLevel.String(), tc.testLevel.String()) - } + t.Run(tc.authLevel.String()+"-"+tc.testLevel.String(), func(t *testing.T) { + ctx := NewContext(context.Background(), &Peer{AuthInfo: testAuthInfo{credentials.CommonAuthInfo{SecurityLevel: tc.authLevel}}}) + p, ok := FromContext(ctx) + if !ok { + t.Fatalf("Unable to get peer from context") + } + err := credentials.CheckSecurityLevel(p.AuthInfo, tc.testLevel) + if tc.want && (err != nil) { + t.Fatalf("CheckSeurityLevel(%s, %s) returned failure but want success", tc.authLevel.String(), tc.testLevel.String()) + } else if !tc.want && (err == nil) { + t.Fatalf("CheckSeurityLevel(%s, %s) returned success but want failure", tc.authLevel.String(), tc.testLevel.String()) + } + }) } } @@ -91,55 +94,51 @@ func (a *addr) String() string { return a.ipAddress } func TestPeerStringer(t *testing.T) { testCases := []struct { - addr *addr - authLevel credentials.SecurityLevel - want string + peer *Peer + want string }{ { - addr: &addr{"example.com:1234"}, - authLevel: credentials.PrivacyAndIntegrity, - want: "Peer{Addr: 'example.com:1234', AuthInfo: 'testAuthInfo'}", + peer: &Peer{Addr: &addr{"example.com:1234"}, AuthInfo: testAuthInfo{credentials.CommonAuthInfo{SecurityLevel: credentials.PrivacyAndIntegrity}}}, + want: "Peer{Addr: 'example.com:1234', LocalAddr: , AuthInfo: 'testAuthInfo-3'}", }, { - addr: &addr{"1.2.3.4:1234"}, - authLevel: -1, - want: "Peer{Addr: '1.2.3.4:1234', AuthInfo: }", + peer: &Peer{Addr: &addr{"example.com:1234"}, LocalAddr: &addr{"example.com:1234"}, AuthInfo: testAuthInfo{credentials.CommonAuthInfo{SecurityLevel: credentials.PrivacyAndIntegrity}}}, + want: "Peer{Addr: 'example.com:1234', LocalAddr: 'example.com:1234', AuthInfo: 'testAuthInfo-3'}", }, { - authLevel: credentials.InvalidSecurityLevel, - want: "Peer{Addr: , AuthInfo: 'testAuthInfo'}", + peer: &Peer{Addr: &addr{"1.2.3.4:1234"}, AuthInfo: testAuthInfo{credentials.CommonAuthInfo{}}}, + want: "Peer{Addr: '1.2.3.4:1234', LocalAddr: , AuthInfo: 'testAuthInfo-0'}", + }, + { + peer: &Peer{AuthInfo: testAuthInfo{}}, + want: "Peer{Addr: , LocalAddr: , AuthInfo: 'testAuthInfo-0'}", }, { - authLevel: -1, - want: "Peer{Addr: , AuthInfo: }", + peer: &Peer{}, + want: "Peer{Addr: , LocalAddr: , AuthInfo: }", + }, + { + peer: nil, + want: "Peer", }, } for _, tc := range testCases { - ctx := NewContext(context.Background(), &Peer{Addr: tc.addr, AuthInfo: testAuthInfo{credentials.CommonAuthInfo{SecurityLevel: tc.authLevel}}}) - p, ok := FromContext(ctx) - if tc.authLevel == -1 { - p.AuthInfo = nil - } - if tc.addr == nil { - p.Addr = nil - } - if !ok { - t.Fatalf("Unable to get peer from context") - } - if p.String() != tc.want { - t.Fatalf("Error using peer String(): expected %q, got %q", tc.want, p.String()) - } + t.Run(strings.ReplaceAll(tc.want, " ", ""), func(t *testing.T) { + ctx := NewContext(context.Background(), tc.peer) + p, ok := FromContext(ctx) + if !ok { + t.Fatalf("Unable to get peer from context") + } + if p.String() != tc.want { + t.Fatalf("Error using peer String(): expected %q, got %q", tc.want, p.String()) + } + }) + } +} + +func TestPeerStringerOnContext(t *testing.T) { + ctx := NewContext(context.Background(), &Peer{Addr: &addr{"1.2.3.4:1234"}, AuthInfo: testAuthInfo{credentials.CommonAuthInfo{SecurityLevel: credentials.PrivacyAndIntegrity}}}) + if fmt.Sprintf("%v", ctx) != "context.Background.WithValue(type peer.peerKey, val Peer{Addr: '1.2.3.4:1234', LocalAddr: , AuthInfo: 'testAuthInfo-3'})" { + t.Fatalf("Error printing context with embedded Peer. Got: %v", ctx) } - t.Run("test String on nil Peer", func(st *testing.T) { - var test *Peer - if test.String() != "Peer" { - st.Fatalf("Error using String on nil Peer. Expected 'Peer', got: '%s'", test.String()) - } - }) - t.Run("test Stringer on context", func(st *testing.T) { - ctx := NewContext(context.Background(), &Peer{Addr: &addr{"1.2.3.4:1234"}, AuthInfo: testAuthInfo{credentials.CommonAuthInfo{SecurityLevel: credentials.PrivacyAndIntegrity}}}) - if fmt.Sprintf("%v", ctx) != "context.Background.WithValue(type peer.peerKey, val Peer{Addr: '1.2.3.4:1234', AuthInfo: 'testAuthInfo'})" { - st.Fatalf("Error printing context with embedded Peer. Got: %v", ctx) - } - }) } From e482bccf1b37d1e9827c36f07b74f199a8a3fa99 Mon Sep 17 00:00:00 2001 From: AnomalRoil Date: Thu, 2 May 2024 12:36:10 +0200 Subject: [PATCH 7/9] Apply suggestions from code review Co-authored-by: Arvind Bright --- metadata/metadata_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/metadata/metadata_test.go b/metadata/metadata_test.go index 5b562438263..6753764b9ba 100644 --- a/metadata/metadata_test.go +++ b/metadata/metadata_test.go @@ -350,10 +350,10 @@ func TestStringerMD(t *testing.T) { {MD{"k1": []string{"v1"}}, []string{"MD{k1=[v1]}"}}, {MD{"k1": []string{"v1", "v2"}, "k2": []string{}, "k3": []string{"1", "2", "3"}}, []string{"MD{", "k1=[v1, v2]", "k2=[]", "k3=[1, 2, 3]", "}"}}, } { - strMd := test.md.String() + got := test.md.String() for _, want := range test.want { - if !strings.Contains(strMd, want) { - t.Fatalf("Metadata string '%s' is missing '%s'", strMd, want) + if !strings.Contains(got, want) { + t.Fatalf("Metadata string %q is missing %q", got, want) } } } From c908bad166fa49caac6c79186c12d394d6f39f52 Mon Sep 17 00:00:00 2001 From: Yolan Romailler Date: Thu, 2 May 2024 12:38:19 +0200 Subject: [PATCH 8/9] Not doing Peer's security level test Signed-off-by: Yolan Romailler --- peer/peer_test.go | 52 ++--------------------------------------------- 1 file changed, 2 insertions(+), 50 deletions(-) diff --git a/peer/peer_test.go b/peer/peer_test.go index d7ac9e3313b..de29a71e742 100644 --- a/peer/peer_test.go +++ b/peer/peer_test.go @@ -36,60 +36,12 @@ func (ta testAuthInfo) AuthType() string { return fmt.Sprintf("testAuthInfo-%d", ta.SecurityLevel) } -func TestPeerSecurityLevel(t *testing.T) { - testCases := []struct { - authLevel credentials.SecurityLevel - testLevel credentials.SecurityLevel - want bool - }{ - { - authLevel: credentials.PrivacyAndIntegrity, - testLevel: credentials.PrivacyAndIntegrity, - want: true, - }, - { - authLevel: credentials.IntegrityOnly, - testLevel: credentials.PrivacyAndIntegrity, - want: false, - }, - { - authLevel: credentials.IntegrityOnly, - testLevel: credentials.NoSecurity, - want: true, - }, - { - authLevel: credentials.InvalidSecurityLevel, - testLevel: credentials.IntegrityOnly, - want: true, - }, - { - authLevel: credentials.InvalidSecurityLevel, - testLevel: credentials.PrivacyAndIntegrity, - want: true, - }, - } - for _, tc := range testCases { - t.Run(tc.authLevel.String()+"-"+tc.testLevel.String(), func(t *testing.T) { - ctx := NewContext(context.Background(), &Peer{AuthInfo: testAuthInfo{credentials.CommonAuthInfo{SecurityLevel: tc.authLevel}}}) - p, ok := FromContext(ctx) - if !ok { - t.Fatalf("Unable to get peer from context") - } - err := credentials.CheckSecurityLevel(p.AuthInfo, tc.testLevel) - if tc.want && (err != nil) { - t.Fatalf("CheckSeurityLevel(%s, %s) returned failure but want success", tc.authLevel.String(), tc.testLevel.String()) - } else if !tc.want && (err == nil) { - t.Fatalf("CheckSeurityLevel(%s, %s) returned success but want failure", tc.authLevel.String(), tc.testLevel.String()) - } - }) - } -} - type addr struct { ipAddress string } -func (addr) Network() string { return "" } +func (addr) Network() string { return "" } + func (a *addr) String() string { return a.ipAddress } func TestPeerStringer(t *testing.T) { From d36bfd5942975ebf2c2322d17640e5b4f82c8f1d Mon Sep 17 00:00:00 2001 From: Yolan Romailler Date: Thu, 2 May 2024 12:46:19 +0200 Subject: [PATCH 9/9] Applying the rest of the code review comments Signed-off-by: Yolan Romailler --- peer/peer_test.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/peer/peer_test.go b/peer/peer_test.go index de29a71e742..45240475eec 100644 --- a/peer/peer_test.go +++ b/peer/peer_test.go @@ -21,7 +21,6 @@ package peer import ( "context" "fmt" - "strings" "testing" "google.golang.org/grpc/credentials" @@ -46,36 +45,43 @@ func (a *addr) String() string { return a.ipAddress } func TestPeerStringer(t *testing.T) { testCases := []struct { + name string peer *Peer want string }{ { + name: "+Addr-LocalAddr+ValidAuth", peer: &Peer{Addr: &addr{"example.com:1234"}, AuthInfo: testAuthInfo{credentials.CommonAuthInfo{SecurityLevel: credentials.PrivacyAndIntegrity}}}, want: "Peer{Addr: 'example.com:1234', LocalAddr: , AuthInfo: 'testAuthInfo-3'}", }, { + name: "+Addr+LocalAddr+ValidAuth", peer: &Peer{Addr: &addr{"example.com:1234"}, LocalAddr: &addr{"example.com:1234"}, AuthInfo: testAuthInfo{credentials.CommonAuthInfo{SecurityLevel: credentials.PrivacyAndIntegrity}}}, want: "Peer{Addr: 'example.com:1234', LocalAddr: 'example.com:1234', AuthInfo: 'testAuthInfo-3'}", }, { + name: "+Addr-LocalAddr+emptyAuth", peer: &Peer{Addr: &addr{"1.2.3.4:1234"}, AuthInfo: testAuthInfo{credentials.CommonAuthInfo{}}}, want: "Peer{Addr: '1.2.3.4:1234', LocalAddr: , AuthInfo: 'testAuthInfo-0'}", }, { + name: "-Addr-LocalAddr+emptyAuth", peer: &Peer{AuthInfo: testAuthInfo{}}, want: "Peer{Addr: , LocalAddr: , AuthInfo: 'testAuthInfo-0'}", }, { + name: "zeroedPeer", peer: &Peer{}, want: "Peer{Addr: , LocalAddr: , AuthInfo: }", }, { + name: "nilPeer", peer: nil, want: "Peer", }, } for _, tc := range testCases { - t.Run(strings.ReplaceAll(tc.want, " ", ""), func(t *testing.T) { + t.Run(tc.name, func(t *testing.T) { ctx := NewContext(context.Background(), tc.peer) p, ok := FromContext(ctx) if !ok { @@ -90,7 +96,8 @@ func TestPeerStringer(t *testing.T) { func TestPeerStringerOnContext(t *testing.T) { ctx := NewContext(context.Background(), &Peer{Addr: &addr{"1.2.3.4:1234"}, AuthInfo: testAuthInfo{credentials.CommonAuthInfo{SecurityLevel: credentials.PrivacyAndIntegrity}}}) - if fmt.Sprintf("%v", ctx) != "context.Background.WithValue(type peer.peerKey, val Peer{Addr: '1.2.3.4:1234', LocalAddr: , AuthInfo: 'testAuthInfo-3'})" { - t.Fatalf("Error printing context with embedded Peer. Got: %v", ctx) + want := "context.Background.WithValue(type peer.peerKey, val Peer{Addr: '1.2.3.4:1234', LocalAddr: , AuthInfo: 'testAuthInfo-3'})" + if got := fmt.Sprintf("%v", ctx); got != want { + t.Fatalf("Unexpected stringer output, got: %q; want: %q", got, want) } }