agent: ensure we hash the non-deprecated upstream fields on ServiceCo…

…nfigRequest (#10240)

.changelog/10240.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+agent: ensure we hash the non-deprecated upstream fields on ServiceConfigRequest
+```

agent/structs/config_entry.go

@@ -641,11 +641,13 @@ func (r *ServiceConfigRequest) CacheInfo() cache.RequestInfo {
 	v, err := hashstructure.Hash(struct {
 		Name           string
 		EnterpriseMeta EnterpriseMeta
-		Upstreams      []string `hash:"set"`
+		Upstreams      []string    `hash:"set"`
+		UpstreamIDs    []ServiceID `hash:"set"`
 	}{
 		Name:           r.Name,
 		EnterpriseMeta: r.EnterpriseMeta,
 		Upstreams:      r.Upstreams,
+		UpstreamIDs:    r.UpstreamIDs,
 	}, nil)
 	if err == nil {
 		// If there is an error, we don't set the key. A blank key forces

agent/structs/config_entry_test.go

@@ -11,6 +11,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/hashicorp/consul/agent/cache"
 	"github.com/hashicorp/consul/sdk/testutil"
 )
 
@@ -1366,6 +1367,118 @@ func TestDecodeConfigEntry(t *testing.T) {
 	}
 }
 
+func TestServiceConfigRequest(t *testing.T) {
+	tests := []struct {
+		name     string
+		req      ServiceConfigRequest
+		mutate   func(req *ServiceConfigRequest)
+		want     *cache.RequestInfo
+		wantSame bool
+	}{
+		{
+			name: "basic params",
+			req: ServiceConfigRequest{
+				QueryOptions: QueryOptions{Token: "foo"},
+				Datacenter:   "dc1",
+			},
+			want: &cache.RequestInfo{
+				Token:      "foo",
+				Datacenter: "dc1",
+			},
+			wantSame: true,
+		},
+		{
+			name: "name should be considered",
+			req: ServiceConfigRequest{
+				Name: "web",
+			},
+			mutate: func(req *ServiceConfigRequest) {
+				req.Name = "db"
+			},
+			wantSame: false,
+		},
+		{
+			name: "legacy upstreams should be different",
+			req: ServiceConfigRequest{
+				Name:      "web",
+				Upstreams: []string{"foo"},
+			},
+			mutate: func(req *ServiceConfigRequest) {
+				req.Upstreams = []string{"foo", "bar"}
+			},
+			wantSame: false,
+		},
+		{
+			name: "legacy upstreams should not depend on order",
+			req: ServiceConfigRequest{
+				Name:      "web",
+				Upstreams: []string{"bar", "foo"},
+			},
+			mutate: func(req *ServiceConfigRequest) {
+				req.Upstreams = []string{"foo", "bar"}
+			},
+			wantSame: true,
+		},
+		{
+			name: "upstreams should be different",
+			req: ServiceConfigRequest{
+				Name: "web",
+				UpstreamIDs: []ServiceID{
+					NewServiceID("foo", nil),
+				},
+			},
+			mutate: func(req *ServiceConfigRequest) {
+				req.UpstreamIDs = []ServiceID{
+					NewServiceID("foo", nil),
+					NewServiceID("bar", nil),
+				}
+			},
+			wantSame: false,
+		},
+		{
+			name: "upstreams should not depend on order",
+			req: ServiceConfigRequest{
+				Name: "web",
+				UpstreamIDs: []ServiceID{
+					NewServiceID("bar", nil),
+					NewServiceID("foo", nil),
+				},
+			},
+			mutate: func(req *ServiceConfigRequest) {
+				req.UpstreamIDs = []ServiceID{
+					NewServiceID("foo", nil),
+					NewServiceID("bar", nil),
+				}
+			},
+			wantSame: true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			info := tc.req.CacheInfo()
+			if tc.mutate != nil {
+				tc.mutate(&tc.req)
+			}
+			afterInfo := tc.req.CacheInfo()
+
+			// Check key matches or not
+			if tc.wantSame {
+				require.Equal(t, info, afterInfo)
+			} else {
+				require.NotEqual(t, info, afterInfo)
+			}
+
+			if tc.want != nil {
+				// Reset key since we don't care about the actual hash value as long as
+				// it does/doesn't change appropriately (asserted with wantSame above).
+				info.Key = ""
+				require.Equal(t, *tc.want, info)
+			}
+		})
+	}
+}
+
 func TestServiceConfigResponse_MsgPack(t *testing.T) {
 	// TODO(banks) lib.MapWalker doesn't actually fix the map[interface{}] issue
 	// it claims to in docs yet. When it does uncomment those cases below.