diff --git a/.changelog/10240.txt b/.changelog/10240.txt new file mode 100644 index 000000000..fa786ea1d --- /dev/null +++ b/.changelog/10240.txt @@ -0,0 +1,3 @@ +```release-note:bug +agent: ensure we hash the non-deprecated upstream fields on ServiceConfigRequest +``` diff --git a/agent/structs/config_entry.go b/agent/structs/config_entry.go index 3f7145ee2..04917e942 100644 --- a/agent/structs/config_entry.go +++ b/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 diff --git a/agent/structs/config_entry_test.go b/agent/structs/config_entry_test.go index cace510bb..e37cf49cd 100644 --- a/agent/structs/config_entry_test.go +++ b/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.