From c2a28ba2686866bb79886b4470ac6e11838aeb1e Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Wed, 2 Sep 2020 10:47:19 -0500 Subject: [PATCH] connect: fix bug in preventing some namespaced config entry modifications (#8601) Whenever an upsert/deletion of a config entry happens, within the open state store transaction we speculatively test compile all discovery chains that may be affected by the pending modification to verify that the write would not create an erroneous scenario (such as splitting traffic to a subset that did not exist). If a single discovery chain evaluation references two config entries with the same kind and name in different namespaces then sometimes the upsert/deletion would be falsely rejected. It does not appear as though this bug would've let invalid writes through to the state store so the correction does not require a cleanup phase. --- .changelog/8601.txt | 3 + agent/consul/state/config_entry.go | 7 +- agent/consul/state/config_entry_test.go | 96 +++++++++++++------------ agent/structs/config_entry.go | 15 ++++ 4 files changed, 71 insertions(+), 50 deletions(-) create mode 100644 .changelog/8601.txt diff --git a/.changelog/8601.txt b/.changelog/8601.txt new file mode 100644 index 0000000000..f791fe2efe --- /dev/null +++ b/.changelog/8601.txt @@ -0,0 +1,3 @@ +```release-note:bug +connect: fix bug in preventing some namespaced config entry modifications +``` diff --git a/agent/consul/state/config_entry.go b/agent/consul/state/config_entry.go index 7e947d749a..0e5c30f4f1 100644 --- a/agent/consul/state/config_entry.go +++ b/agent/consul/state/config_entry.go @@ -485,7 +485,7 @@ func (s *Store) validateProposedConfigEntryInServiceGraph( } overrides := map[structs.ConfigEntryKindName]structs.ConfigEntry{ - {Kind: kind, Name: name}: next, + structs.NewConfigEntryKindName(kind, name, entMeta): next, } var ( @@ -927,9 +927,8 @@ func (s *Store) configEntryWithOverridesTxn( entMeta *structs.EnterpriseMeta, ) (uint64, structs.ConfigEntry, error) { if len(overrides) > 0 { - entry, ok := overrides[structs.ConfigEntryKindName{ - Kind: kind, Name: name, - }] + kn := structs.NewConfigEntryKindName(kind, name, entMeta) + entry, ok := overrides[kn] if ok { return 0, entry, nil // a nil entry implies it should act like it is erased } diff --git a/agent/consul/state/config_entry_test.go b/agent/consul/state/config_entry_test.go index dcacf2f1a9..0bfeed7719 100644 --- a/agent/consul/state/config_entry_test.go +++ b/agent/consul/state/config_entry_test.go @@ -880,10 +880,10 @@ func TestStore_ReadDiscoveryChainConfigEntries_Overrides(t *testing.T) { }, }, expectBefore: []structs.ConfigEntryKindName{ - {Kind: structs.ServiceDefaults, Name: "main"}, + structs.NewConfigEntryKindName(structs.ServiceDefaults, "main", nil), }, overrides: map[structs.ConfigEntryKindName]structs.ConfigEntry{ - {Kind: structs.ServiceDefaults, Name: "main"}: nil, + structs.NewConfigEntryKindName(structs.ServiceDefaults, "main", nil): nil, }, expectAfter: []structs.ConfigEntryKindName{ // nothing @@ -899,17 +899,17 @@ func TestStore_ReadDiscoveryChainConfigEntries_Overrides(t *testing.T) { }, }, expectBefore: []structs.ConfigEntryKindName{ - {Kind: structs.ServiceDefaults, Name: "main"}, + structs.NewConfigEntryKindName(structs.ServiceDefaults, "main", nil), }, overrides: map[structs.ConfigEntryKindName]structs.ConfigEntry{ - {Kind: structs.ServiceDefaults, Name: "main"}: &structs.ServiceConfigEntry{ + structs.NewConfigEntryKindName(structs.ServiceDefaults, "main", nil): &structs.ServiceConfigEntry{ Kind: structs.ServiceDefaults, Name: "main", Protocol: "grpc", }, }, expectAfter: []structs.ConfigEntryKindName{ - {Kind: structs.ServiceDefaults, Name: "main"}, + structs.NewConfigEntryKindName(structs.ServiceDefaults, "main", nil), }, checkAfter: func(t *testing.T, entrySet *structs.DiscoveryChainConfigEntries) { defaults := entrySet.GetService(structs.NewServiceID("main", nil)) @@ -932,14 +932,14 @@ func TestStore_ReadDiscoveryChainConfigEntries_Overrides(t *testing.T) { }, }, expectBefore: []structs.ConfigEntryKindName{ - {Kind: structs.ServiceDefaults, Name: "main"}, - {Kind: structs.ServiceRouter, Name: "main"}, + structs.NewConfigEntryKindName(structs.ServiceDefaults, "main", nil), + structs.NewConfigEntryKindName(structs.ServiceRouter, "main", nil), }, overrides: map[structs.ConfigEntryKindName]structs.ConfigEntry{ - {Kind: structs.ServiceRouter, Name: "main"}: nil, + structs.NewConfigEntryKindName(structs.ServiceRouter, "main", nil): nil, }, expectAfter: []structs.ConfigEntryKindName{ - {Kind: structs.ServiceDefaults, Name: "main"}, + structs.NewConfigEntryKindName(structs.ServiceDefaults, "main", nil), }, }, { @@ -977,12 +977,12 @@ func TestStore_ReadDiscoveryChainConfigEntries_Overrides(t *testing.T) { }, }, expectBefore: []structs.ConfigEntryKindName{ - {Kind: structs.ServiceDefaults, Name: "main"}, - {Kind: structs.ServiceResolver, Name: "main"}, - {Kind: structs.ServiceRouter, Name: "main"}, + structs.NewConfigEntryKindName(structs.ServiceDefaults, "main", nil), + structs.NewConfigEntryKindName(structs.ServiceResolver, "main", nil), + structs.NewConfigEntryKindName(structs.ServiceRouter, "main", nil), }, overrides: map[structs.ConfigEntryKindName]structs.ConfigEntry{ - {Kind: structs.ServiceRouter, Name: "main"}: &structs.ServiceRouterConfigEntry{ + structs.NewConfigEntryKindName(structs.ServiceRouter, "main", nil): &structs.ServiceRouterConfigEntry{ Kind: structs.ServiceRouter, Name: "main", Routes: []structs.ServiceRoute{ @@ -1000,9 +1000,9 @@ func TestStore_ReadDiscoveryChainConfigEntries_Overrides(t *testing.T) { }, }, expectAfter: []structs.ConfigEntryKindName{ - {Kind: structs.ServiceDefaults, Name: "main"}, - {Kind: structs.ServiceResolver, Name: "main"}, - {Kind: structs.ServiceRouter, Name: "main"}, + structs.NewConfigEntryKindName(structs.ServiceDefaults, "main", nil), + structs.NewConfigEntryKindName(structs.ServiceResolver, "main", nil), + structs.NewConfigEntryKindName(structs.ServiceRouter, "main", nil), }, checkAfter: func(t *testing.T, entrySet *structs.DiscoveryChainConfigEntries) { router := entrySet.GetRouter(structs.NewServiceID("main", nil)) @@ -1040,14 +1040,14 @@ func TestStore_ReadDiscoveryChainConfigEntries_Overrides(t *testing.T) { }, }, expectBefore: []structs.ConfigEntryKindName{ - {Kind: structs.ServiceDefaults, Name: "main"}, - {Kind: structs.ServiceSplitter, Name: "main"}, + structs.NewConfigEntryKindName(structs.ServiceDefaults, "main", nil), + structs.NewConfigEntryKindName(structs.ServiceSplitter, "main", nil), }, overrides: map[structs.ConfigEntryKindName]structs.ConfigEntry{ - {Kind: structs.ServiceSplitter, Name: "main"}: nil, + structs.NewConfigEntryKindName(structs.ServiceSplitter, "main", nil): nil, }, expectAfter: []structs.ConfigEntryKindName{ - {Kind: structs.ServiceDefaults, Name: "main"}, + structs.NewConfigEntryKindName(structs.ServiceDefaults, "main", nil), }, }, { @@ -1067,11 +1067,11 @@ func TestStore_ReadDiscoveryChainConfigEntries_Overrides(t *testing.T) { }, }, expectBefore: []structs.ConfigEntryKindName{ - {Kind: structs.ServiceDefaults, Name: "main"}, - {Kind: structs.ServiceSplitter, Name: "main"}, + structs.NewConfigEntryKindName(structs.ServiceDefaults, "main", nil), + structs.NewConfigEntryKindName(structs.ServiceSplitter, "main", nil), }, overrides: map[structs.ConfigEntryKindName]structs.ConfigEntry{ - {Kind: structs.ServiceSplitter, Name: "main"}: &structs.ServiceSplitterConfigEntry{ + structs.NewConfigEntryKindName(structs.ServiceSplitter, "main", nil): &structs.ServiceSplitterConfigEntry{ Kind: structs.ServiceSplitter, Name: "main", Splits: []structs.ServiceSplit{ @@ -1081,8 +1081,8 @@ func TestStore_ReadDiscoveryChainConfigEntries_Overrides(t *testing.T) { }, }, expectAfter: []structs.ConfigEntryKindName{ - {Kind: structs.ServiceDefaults, Name: "main"}, - {Kind: structs.ServiceSplitter, Name: "main"}, + structs.NewConfigEntryKindName(structs.ServiceDefaults, "main", nil), + structs.NewConfigEntryKindName(structs.ServiceSplitter, "main", nil), }, checkAfter: func(t *testing.T, entrySet *structs.DiscoveryChainConfigEntries) { splitter := entrySet.GetSplitter(structs.NewServiceID("main", nil)) @@ -1106,10 +1106,10 @@ func TestStore_ReadDiscoveryChainConfigEntries_Overrides(t *testing.T) { }, }, expectBefore: []structs.ConfigEntryKindName{ - {Kind: structs.ServiceResolver, Name: "main"}, + structs.NewConfigEntryKindName(structs.ServiceResolver, "main", nil), }, overrides: map[structs.ConfigEntryKindName]structs.ConfigEntry{ - {Kind: structs.ServiceResolver, Name: "main"}: nil, + structs.NewConfigEntryKindName(structs.ServiceResolver, "main", nil): nil, }, expectAfter: []structs.ConfigEntryKindName{ // nothing @@ -1124,17 +1124,17 @@ func TestStore_ReadDiscoveryChainConfigEntries_Overrides(t *testing.T) { }, }, expectBefore: []structs.ConfigEntryKindName{ - {Kind: structs.ServiceResolver, Name: "main"}, + structs.NewConfigEntryKindName(structs.ServiceResolver, "main", nil), }, overrides: map[structs.ConfigEntryKindName]structs.ConfigEntry{ - {Kind: structs.ServiceResolver, Name: "main"}: &structs.ServiceResolverConfigEntry{ + structs.NewConfigEntryKindName(structs.ServiceResolver, "main", nil): &structs.ServiceResolverConfigEntry{ Kind: structs.ServiceResolver, Name: "main", ConnectTimeout: 33 * time.Second, }, }, expectAfter: []structs.ConfigEntryKindName{ - {Kind: structs.ServiceResolver, Name: "main"}, + structs.NewConfigEntryKindName(structs.ServiceResolver, "main", nil), }, checkAfter: func(t *testing.T, entrySet *structs.DiscoveryChainConfigEntries) { resolver := entrySet.GetResolver(structs.NewServiceID("main", nil)) @@ -1181,28 +1181,32 @@ func TestStore_ReadDiscoveryChainConfigEntries_Overrides(t *testing.T) { func entrySetToKindNames(entrySet *structs.DiscoveryChainConfigEntries) []structs.ConfigEntryKindName { var out []structs.ConfigEntryKindName for _, entry := range entrySet.Routers { - out = append(out, structs.ConfigEntryKindName{ - Kind: entry.Kind, - Name: entry.Name, - }) + out = append(out, structs.NewConfigEntryKindName( + entry.Kind, + entry.Name, + &entry.EnterpriseMeta, + )) } for _, entry := range entrySet.Splitters { - out = append(out, structs.ConfigEntryKindName{ - Kind: entry.Kind, - Name: entry.Name, - }) + out = append(out, structs.NewConfigEntryKindName( + entry.Kind, + entry.Name, + &entry.EnterpriseMeta, + )) } for _, entry := range entrySet.Resolvers { - out = append(out, structs.ConfigEntryKindName{ - Kind: entry.Kind, - Name: entry.Name, - }) + out = append(out, structs.NewConfigEntryKindName( + entry.Kind, + entry.Name, + &entry.EnterpriseMeta, + )) } for _, entry := range entrySet.Services { - out = append(out, structs.ConfigEntryKindName{ - Kind: entry.Kind, - Name: entry.Name, - }) + out = append(out, structs.NewConfigEntryKindName( + entry.Kind, + entry.Name, + &entry.EnterpriseMeta, + )) } return out } diff --git a/agent/structs/config_entry.go b/agent/structs/config_entry.go index d377f83a72..4cafb9b292 100644 --- a/agent/structs/config_entry.go +++ b/agent/structs/config_entry.go @@ -666,4 +666,19 @@ func (c *ConfigEntryResponse) UnmarshalBinary(data []byte) error { type ConfigEntryKindName struct { Kind string Name string + EnterpriseMeta +} + +func NewConfigEntryKindName(kind, name string, entMeta *EnterpriseMeta) ConfigEntryKindName { + ret := ConfigEntryKindName{ + Kind: kind, + Name: name, + } + if entMeta == nil { + entMeta = DefaultEnterpriseMeta() + } + + ret.EnterpriseMeta = *entMeta + ret.EnterpriseMeta.Normalize() + return ret }