rls: Manually implement equality of key builder maps. (#3476)

cmp.Equal is not supposed to be used in production code because of its
propensity towards panicking.

This equality will eventually be checked from the LB policy when it gets
a service config update.

balancer/rls/internal/keys/builder.go

@@ -27,7 +27,6 @@ import (
 	"sort"
 	"strings"
 
-	"github.com/google/go-cmp/cmp"
 	rlspb "google.golang.org/grpc/balancer/rls/internal/proto/grpc_lookup_v1"
 	"google.golang.org/grpc/metadata"
 )
@@ -123,9 +122,25 @@ func (bm BuilderMap) RLSKey(md metadata.MD, path string) KeyMap {
 	return b.keys(md)
 }
 
-// BuilderMapEqual returns true if the provided BuilderMap objects are equal.
+// Equal reports whether bm and am represent equivalent BuilderMaps.
-func BuilderMapEqual(a, b BuilderMap) bool {
+func (bm BuilderMap) Equal(am BuilderMap) bool {
-	return cmp.Equal(a, b, cmp.AllowUnexported(builder{}, matcher{}))
+	if (bm == nil) != (am == nil) {
+		return false
+	}
+	if len(bm) != len(am) {
+		return false
+	}
+
+	for key, bBuilder := range bm {
+		aBuilder, ok := am[key]
+		if !ok {
+			return false
+		}
+		if !bBuilder.Equal(aBuilder) {
+			return false
+		}
+	}
+	return true
 }
 
 // builder provides the actual functionality of building RLS keys. These are
@@ -142,6 +157,28 @@ type builder struct {
 	matchers []matcher
 }
 
+// Equal reports whether b and a represent equivalent key builders.
+func (b builder) Equal(a builder) bool {
+	if (b.matchers == nil) != (a.matchers == nil) {
+		return false
+	}
+	if len(b.matchers) != len(a.matchers) {
+		return false
+	}
+	// Protobuf serialization maintains the order of repeated fields. Matchers
+	// are specified as a repeated field inside the KeyBuilder proto. If the
+	// order changes, it means that the order in the protobuf changed. We report
+	// this case as not being equal even though the builders could possible be
+	// functionally equal.
+	for i, bMatcher := range b.matchers {
+		aMatcher := a.matchers[i]
+		if !bMatcher.Equal(aMatcher) {
+			return false
+		}
+	}
+	return true
+}
+
 // matcher helps extract a key from request headers based on a given name.
 type matcher struct {
 	// The key used in the keyMap sent as part of the RLS request.
@@ -150,6 +187,25 @@ type matcher struct {
 	names []string
 }
 
+// Equal reports if m and are are equivalent matchers.
+func (m matcher) Equal(a matcher) bool {
+	if m.key != a.key {
+		return false
+	}
+	if (m.names == nil) != (a.names == nil) {
+		return false
+	}
+	if len(m.names) != len(a.names) {
+		return false
+	}
+	for i := 0; i < len(m.names); i++ {
+		if m.names[i] != a.names[i] {
+			return false
+		}
+	}
+	return true
+}
+
 func (b builder) keys(md metadata.MD) KeyMap {
 	kvMap := make(map[string]string)
 	for _, m := range b.matchers {

balancer/rls/internal/keys/builder_test.go

@@ -85,7 +85,7 @@ func TestMakeBuilderMap(t *testing.T) {
 	for _, test := range tests {
 		t.Run(test.desc, func(t *testing.T) {
 			builderMap, err := MakeBuilderMap(test.cfg)
-			if err != nil || !BuilderMapEqual(builderMap, test.wantBuilderMap) {
+			if err != nil || !builderMap.Equal(test.wantBuilderMap) {
 				t.Errorf("MakeBuilderMap(%+v) returned {%v, %v}, want: {%v, nil}", test.cfg, builderMap, err, test.wantBuilderMap)
 			}
 		})
@@ -330,3 +330,201 @@ func TestMapToString(t *testing.T) {
 		})
 	}
 }
+
+func TestBuilderMapEqual(t *testing.T) {
+	tests := []struct {
+		desc      string
+		a         BuilderMap
+		b         BuilderMap
+		wantEqual bool
+	}{
+		{
+			desc:      "nil builder maps",
+			a:         nil,
+			b:         nil,
+			wantEqual: true,
+		},
+		{
+			desc:      "empty builder maps",
+			a:         make(map[string]builder),
+			b:         make(map[string]builder),
+			wantEqual: true,
+		},
+		{
+			desc:      "nil and non-nil builder maps",
+			a:         nil,
+			b:         map[string]builder{"/gFoo/": {matchers: []matcher{{key: "k1", names: []string{"n1"}}}}},
+			wantEqual: false,
+		},
+		{
+			desc:      "empty and non-empty builder maps",
+			a:         make(map[string]builder),
+			b:         map[string]builder{"/gFoo/": {matchers: []matcher{{key: "k1", names: []string{"n1"}}}}},
+			wantEqual: false,
+		},
+		{
+			desc: "different number of map keys",
+			a: map[string]builder{
+				"/gFoo/": {matchers: []matcher{{key: "k1", names: []string{"n1"}}}},
+				"/gBar/": {matchers: []matcher{{key: "k1", names: []string{"n1"}}}},
+			},
+			b: map[string]builder{
+				"/gFoo/": {matchers: []matcher{{key: "k1", names: []string{"n1"}}}},
+			},
+			wantEqual: false,
+		},
+		{
+			desc: "different map keys",
+			a: map[string]builder{
+				"/gBar/": {matchers: []matcher{{key: "k1", names: []string{"n1"}}}},
+			},
+			b: map[string]builder{
+				"/gFoo/": {matchers: []matcher{{key: "k1", names: []string{"n1"}}}},
+			},
+			wantEqual: false,
+		},
+		{
+			desc: "equal keys different values",
+			a: map[string]builder{
+				"/gBar/": {matchers: []matcher{{key: "k1", names: []string{"n1"}}}},
+				"/gFoo/": {matchers: []matcher{{key: "k1", names: []string{"n1", "n2"}}}},
+			},
+			b: map[string]builder{
+				"/gBar/": {matchers: []matcher{{key: "k1", names: []string{"n1"}}}},
+				"/gFoo/": {matchers: []matcher{{key: "k1", names: []string{"n1"}}}},
+			},
+			wantEqual: false,
+		},
+		{
+			desc: "good match",
+			a: map[string]builder{
+				"/gBar/": {matchers: []matcher{{key: "k1", names: []string{"n1"}}}},
+				"/gFoo/": {matchers: []matcher{{key: "k1", names: []string{"n1"}}}},
+			},
+			b: map[string]builder{
+				"/gBar/": {matchers: []matcher{{key: "k1", names: []string{"n1"}}}},
+				"/gFoo/": {matchers: []matcher{{key: "k1", names: []string{"n1"}}}},
+			},
+			wantEqual: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			if gotEqual := test.a.Equal(test.b); gotEqual != test.wantEqual {
+				t.Errorf("BuilderMap.Equal(%v, %v) = %v, want %v", test.a, test.b, gotEqual, test.wantEqual)
+			}
+		})
+	}
+}
+
+func TestBuilderEqual(t *testing.T) {
+	tests := []struct {
+		desc      string
+		a         builder
+		b         builder
+		wantEqual bool
+	}{
+		{
+			desc:      "nil builders",
+			a:         builder{matchers: nil},
+			b:         builder{matchers: nil},
+			wantEqual: true,
+		},
+		{
+			desc:      "empty builders",
+			a:         builder{matchers: []matcher{}},
+			b:         builder{matchers: []matcher{}},
+			wantEqual: true,
+		},
+		{
+			desc:      "nil and non-nil builders",
+			a:         builder{matchers: nil},
+			b:         builder{matchers: []matcher{}},
+			wantEqual: false,
+		},
+		{
+			desc:      "empty and non-empty builders",
+			a:         builder{matchers: []matcher{}},
+			b:         builder{matchers: []matcher{{key: "foo"}}},
+			wantEqual: false,
+		},
+		{
+			desc:      "different number of matchers",
+			a:         builder{matchers: []matcher{{key: "foo"}, {key: "bar"}}},
+			b:         builder{matchers: []matcher{{key: "foo"}}},
+			wantEqual: false,
+		},
+		{
+			desc:      "equal number but differing matchers",
+			a:         builder{matchers: []matcher{{key: "bar"}}},
+			b:         builder{matchers: []matcher{{key: "foo"}}},
+			wantEqual: false,
+		},
+		{
+			desc:      "good match",
+			a:         builder{matchers: []matcher{{key: "foo"}}},
+			b:         builder{matchers: []matcher{{key: "foo"}}},
+			wantEqual: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			t.Run(test.desc, func(t *testing.T) {
+				if gotEqual := test.a.Equal(test.b); gotEqual != test.wantEqual {
+					t.Errorf("builder.Equal(%v, %v) = %v, want %v", test.a, test.b, gotEqual, test.wantEqual)
+				}
+			})
+		})
+	}
+}
+
+// matcher helps extract a key from request headers based on a given name.
+func TestMatcherEqual(t *testing.T) {
+	tests := []struct {
+		desc      string
+		a         matcher
+		b         matcher
+		wantEqual bool
+	}{
+		{
+			desc:      "different keys",
+			a:         matcher{key: "foo"},
+			b:         matcher{key: "bar"},
+			wantEqual: false,
+		},
+		{
+			desc:      "different number of names",
+			a:         matcher{key: "foo", names: []string{"v1", "v2"}},
+			b:         matcher{key: "foo", names: []string{"v1"}},
+			wantEqual: false,
+		},
+		{
+			desc:      "equal number but differing names",
+			a:         matcher{key: "foo", names: []string{"v1", "v2"}},
+			b:         matcher{key: "foo", names: []string{"v1", "v22"}},
+			wantEqual: false,
+		},
+		{
+			desc:      "same names in different order",
+			a:         matcher{key: "foo", names: []string{"v2", "v1"}},
+			b:         matcher{key: "foo", names: []string{"v1", "v3"}},
+			wantEqual: false,
+		},
+		{
+			desc:      "good match",
+			a:         matcher{key: "foo", names: []string{"v1", "v2"}},
+			b:         matcher{key: "foo", names: []string{"v1", "v2"}},
+			wantEqual: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			if gotEqual := test.a.Equal(test.b); gotEqual != test.wantEqual {
+				t.Errorf("matcher.Equal(%v, %v) = %v, want %v", test.a, test.b, gotEqual, test.wantEqual)
+			}
+		})
+	}
+}