connect: fix bug in preventing some namespaced config entry modificat…

…ions (#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.
hashicorp · Sep 2, 2020 · d0f74cd · d0f74cd
1 parent f61649f
commit d0f74cd
Show file tree

Hide file tree

Showing 4 changed files with 71 additions and 50 deletions.
diff --git a/.changelog/8601.txt 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
@@ -467,7 +467,7 @@ func validateProposedConfigEntryInServiceGraph(
 	}
 
 	overrides := map[structs.ConfigEntryKindName]structs.ConfigEntry{
-		{Kind: kind, Name: name}: next,
+		structs.NewConfigEntryKindName(kind, name, entMeta): next,
 	}
 
 	var (
@@ -909,9 +909,8 @@ func 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
@@ -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
@@ -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
 }