This fixes an issue where TCP services that are exported cannot be configured to failover. (#17469)

This will likely happen frequently with sameness groups. Relaxing this constraint is harmless for failover because xds/endpoints exludes cross partition and peer endpoints.
2023-05-25 12:50:20 -04:00 · 2023-05-25 12:50:20 -04:00 · 17a280d51b
parent 1c80892717
commit 17a280d51b
3 changed files with 332 additions and 17 deletions
--- a/agent/consul/state/config_entry.go
+++ b/agent/consul/state/config_entry.go
@ -1121,20 +1121,23 @@ func validateProposedConfigEntryInServiceGraph(
 		svcTopNodeType              = make(map[structs.ServiceID]string)
 		exportedServicesByPartition = make(map[string]map[structs.ServiceName]struct{})
 	)
-	for chain := range checkChains {
-		protocol, topNode, newTargets, err := testCompileDiscoveryChain(tx, chain.ID, overrides, &chain.EnterpriseMeta)
+	for serviceID := range checkChains {
+		chain, err := testCompileDiscoveryChain(tx, serviceID.ID, overrides, &serviceID.EnterpriseMeta)
 		if err != nil {
 			return err
 		}
-		svcProtocols[chain] = protocol
-		svcTopNodeType[chain] = topNode.Type
+		topNode := chain.Nodes[chain.StartNode]
+		protocol := chain.Protocol

-		chainSvc := structs.NewServiceName(chain.ID, &chain.EnterpriseMeta)
+		svcProtocols[serviceID] = protocol
+		svcTopNodeType[serviceID] = topNode.Type
+
+		chainSvc := structs.NewServiceName(serviceID.ID, &serviceID.EnterpriseMeta)

 		// Validate that we aren't adding a cross-datacenter or cross-partition
 		// reference to a peer-exported service's discovery chain by this pending
 		// edit.
-		partition := chain.PartitionOrDefault()
+		partition := serviceID.PartitionOrDefault()
 		exportedServices, ok := exportedServicesByPartition[partition]
 		if !ok {
 			entMeta := structs.NodeEnterpriseMetaInPartition(partition)
@ -1152,15 +1155,15 @@ func validateProposedConfigEntryInServiceGraph(
 			// If a TCP (L4) discovery chain is peer exported we have to take
 			// care to prohibit certain edits to service-resolvers.
 			if !structs.IsProtocolHTTPLike(protocol) {
-				_, _, oldTargets, err := testCompileDiscoveryChain(tx, chain.ID, nil, &chain.EnterpriseMeta)
+				oldChain, err := testCompileDiscoveryChain(tx, serviceID.ID, nil, &serviceID.EnterpriseMeta)
 				if err != nil {
 					return fmt.Errorf("error compiling current discovery chain for %q: %w", chainSvc, err)
 				}

 				// Ensure that you can't introduce any new targets that would
 				// produce a new SpiffeID for this L4 service.
-				oldSpiffeIDs := convertTargetsToTestSpiffeIDs(oldTargets)
-				newSpiffeIDs := convertTargetsToTestSpiffeIDs(newTargets)
+				oldSpiffeIDs := convertTargetsToTestSpiffeIDs(oldChain)
+				newSpiffeIDs := convertTargetsToTestSpiffeIDs(chain)
 				for id, targetID := range newSpiffeIDs {
 					if _, exists := oldSpiffeIDs[id]; !exists {
 						return fmt.Errorf("peer exported service %q uses protocol=%q and cannot introduce new discovery chain targets like %q",
@ -1270,6 +1273,11 @@ func validateChainIsPeerExportSafe(
 			if e.Redirect.Datacenter != "" {
 				return fmt.Errorf("peer exported service %q contains cross-datacenter resolver redirect", exportedSvc)
 			}
+
+			if e.Redirect.Peer != "" {
+				return fmt.Errorf("peer exported service %q contains cross-peer resolver redirect", exportedSvc)
+			}
+
 			if !emptyOrMatchesEntryPartition(e, e.Redirect.Partition) {
 				return fmt.Errorf("peer exported service %q contains cross-partition resolver redirect", exportedSvc)
 			}
@ -1279,6 +1287,14 @@ func validateChainIsPeerExportSafe(
 				if len(failover.Datacenters) > 0 {
 					return fmt.Errorf("peer exported service %q contains cross-datacenter failover", exportedSvc)
 				}
+
+				// We don't exclude Peer and Partition here because it's equivalent to sameness group failover behavior.
+				// Cross-peer and partition behavior is ignored by mesh gateways in xds/endpoints.
+				for _, t := range failover.Targets {
+					if t.Datacenter != "" {
+						return fmt.Errorf("peer exported service %q contains cross-datacenter failover", exportedSvc)
+					}
+				}
 			}
 		}
 	}
@ -1303,10 +1319,10 @@ func testCompileDiscoveryChain(
 	chainName string,
 	overrides map[configentry.KindName]structs.ConfigEntry,
 	entMeta *acl.EnterpriseMeta,
-) (string, *structs.DiscoveryGraphNode, map[string]*structs.DiscoveryTarget, error) {
+) (*structs.CompiledDiscoveryChain, error) {
 	_, speculativeEntries, err := readDiscoveryChainConfigEntriesTxn(tx, nil, chainName, overrides, entMeta)
 	if err != nil {
-		return "", nil, nil, err
+		return nil, err
 	}

 	// Note we use an arbitrary namespace and datacenter as those would not
@ -1323,10 +1339,10 @@ func testCompileDiscoveryChain(
 	}
 	chain, err := discoverychain.Compile(req)
 	if err != nil {
-		return "", nil, nil, err
+		return nil, err
 	}

-	return chain.Protocol, chain.Nodes[chain.StartNode], chain.Targets, nil
+	return chain, nil
 }

 func (s *Store) ServiceDiscoveryChain(
@ -2053,6 +2069,7 @@ func protocolForService(
 }

 const dummyTrustDomain = "b6fc9da3-03d4-4b5a-9134-c045e9b20152.consul"
+const dummyPeerTrustDomain = "4945bd44-b427-4cf5-b19a-5f54b195bf6e.consul"

 func newConfigEntryQuery(c structs.ConfigEntry) configentry.KindName {
 	return configentry.NewKindName(c.GetKind(), c.GetName(), c.GetEnterpriseMeta())
@ -2079,9 +2096,24 @@ func (q ConfigEntryKindQuery) PartitionOrDefault() string {
 // convertTargetsToTestSpiffeIDs indexes the provided targets by their eventual
 // spiffeid values using a dummy trust domain. Returns a map of SpiffeIDs to
 // targetID values which can be used for error output.
-func convertTargetsToTestSpiffeIDs(targets map[string]*structs.DiscoveryTarget) map[string]string {
+func convertTargetsToTestSpiffeIDs(chain *structs.CompiledDiscoveryChain) map[string]string {
+	excludedTargets := make(map[string]struct{})
+
+	for _, n := range chain.Nodes {
+		if n != nil && n.Resolver != nil && n.Resolver.Failover != nil {
+			failover := n.Resolver.Failover
+			for _, t := range failover.Targets {
+				excludedTargets[t] = struct{}{}
+			}
+		}
+	}
+
 	out := make(map[string]string)
-	for tid, t := range targets {
+	for tid, t := range chain.Targets {
+		if _, ok := excludedTargets[tid]; ok {
+			continue
+		}
+
 		testSpiffeID := connect.SpiffeIDService{
 			Host:       dummyTrustDomain,
 			Partition:  t.Partition,
@ -2089,6 +2121,15 @@ func convertTargetsToTestSpiffeIDs(targets map[string]*structs.DiscoveryTarget)
 			Datacenter: t.Datacenter,
 			Service:    t.Service,
 		}
+
+		// Setting peer to dc allows spiffe ids to compare peered services to non-peered services.
+		// We also have to use a distinct trust domain for peering services so we handle
+		// clashes between peers and datacenters.
+		if t.Peer != "" {
+			testSpiffeID.Datacenter = t.Peer
+			testSpiffeID.Host = dummyPeerTrustDomain
+		}
+
 		uri := testSpiffeID.URI().String()
 		if _, ok := out[uri]; !ok {
 			out[uri] = tid
--- a/agent/consul/state/config_entry_test.go
+++ b/agent/consul/state/config_entry_test.go
@ -1663,6 +1663,280 @@ func TestStore_ConfigEntry_GraphValidation(t *testing.T) {
 			},
 			expectErr: `cannot introduce new discovery chain targets like`,
 		},
+		"can redirect a peer exported http service to another service": {
+			entries: []structs.ConfigEntry{
+				&structs.ServiceConfigEntry{
+					Kind:     structs.ServiceDefaults,
+					Name:     "main",
+					Protocol: "http",
+				},
+				&structs.ServiceConfigEntry{
+					Kind:     structs.ServiceDefaults,
+					Name:     "other",
+					Protocol: "http",
+				},
+				&structs.ExportedServicesConfigEntry{
+					Name: "default",
+					Services: []structs.ExportedService{{
+						Name:      "main",
+						Consumers: []structs.ServiceConsumer{{Peer: "my-peer"}},
+					}},
+				},
+			},
+			opAdd: &structs.ServiceResolverConfigEntry{
+				Kind: structs.ServiceResolver,
+				Name: "main",
+				Redirect: &structs.ServiceResolverRedirect{
+					Service: "other",
+				},
+			},
+		},
+		"cannot redirect a peer exported http service to another peer service": {
+			entries: []structs.ConfigEntry{
+				&structs.ServiceConfigEntry{
+					Kind:     structs.ServiceDefaults,
+					Name:     "main",
+					Protocol: "http",
+				},
+				&structs.ExportedServicesConfigEntry{
+					Name: "default",
+					Services: []structs.ExportedService{{
+						Name:      "main",
+						Consumers: []structs.ServiceConsumer{{Peer: "my-peer"}},
+					}},
+				},
+			},
+			opAdd: &structs.ServiceResolverConfigEntry{
+				Kind: structs.ServiceResolver,
+				Name: "main",
+				Redirect: &structs.ServiceResolverRedirect{
+					Service: "other",
+					Peer:    "something",
+				},
+			},
+			expectErr: `contains cross-peer resolver redirect`,
+		},
+		"cannot redirect a peer exported http service to a service in another datacenter": {
+			entries: []structs.ConfigEntry{
+				&structs.ServiceConfigEntry{
+					Kind:     structs.ServiceDefaults,
+					Name:     "main",
+					Protocol: "http",
+				},
+				&structs.ServiceConfigEntry{
+					Kind:     structs.ServiceDefaults,
+					Name:     "other",
+					Protocol: "http",
+				},
+				&structs.ExportedServicesConfigEntry{
+					Name: "default",
+					Services: []structs.ExportedService{{
+						Name:      "main",
+						Consumers: []structs.ServiceConsumer{{Peer: "my-peer"}},
+					}},
+				},
+			},
+			opAdd: &structs.ServiceResolverConfigEntry{
+				Kind: structs.ServiceResolver,
+				Name: "main",
+				Redirect: &structs.ServiceResolverRedirect{
+					Service:    "other",
+					Datacenter: "dc12",
+				},
+			},
+			expectErr: `contains cross-datacenter resolver redirect`,
+		},
+		"can failover a peer exported http service to another service": {
+			entries: []structs.ConfigEntry{
+				&structs.ServiceConfigEntry{
+					Kind:     structs.ServiceDefaults,
+					Name:     "main",
+					Protocol: "http",
+				},
+				&structs.ServiceConfigEntry{
+					Kind:     structs.ServiceDefaults,
+					Name:     "other",
+					Protocol: "http",
+				},
+				&structs.ExportedServicesConfigEntry{
+					Name: "default",
+					Services: []structs.ExportedService{{
+						Name:      "main",
+						Consumers: []structs.ServiceConsumer{{Peer: "my-peer"}},
+					}},
+				},
+			},
+			opAdd: &structs.ServiceResolverConfigEntry{
+				Kind: structs.ServiceResolver,
+				Name: "main",
+				Failover: map[string]structs.ServiceResolverFailover{
+					"*": {
+						Service: "other",
+					},
+				},
+			},
+		},
+		"can failover a peer exported http service to another peer service": {
+			entries: []structs.ConfigEntry{
+				&structs.ServiceConfigEntry{
+					Kind:     structs.ServiceDefaults,
+					Name:     "main",
+					Protocol: "http",
+				},
+				&structs.ExportedServicesConfigEntry{
+					Name: "default",
+					Services: []structs.ExportedService{{
+						Name:      "main",
+						Consumers: []structs.ServiceConsumer{{Peer: "my-peer"}},
+					}},
+				},
+			},
+			opAdd: &structs.ServiceResolverConfigEntry{
+				Kind: structs.ServiceResolver,
+				Name: "main",
+				Failover: map[string]structs.ServiceResolverFailover{
+					"*": {
+						Targets: []structs.ServiceResolverFailoverTarget{
+							{
+								Service: "other",
+								Peer:    "some-peer",
+							},
+						},
+					},
+				},
+			},
+		},
+		"can't failover a peer exported http service to another service in a different datacenter": {
+			entries: []structs.ConfigEntry{
+				&structs.ServiceConfigEntry{
+					Kind:     structs.ServiceDefaults,
+					Name:     "main",
+					Protocol: "http",
+				},
+				&structs.ServiceConfigEntry{
+					Kind:     structs.ServiceDefaults,
+					Name:     "other",
+					Protocol: "http",
+				},
+				&structs.ExportedServicesConfigEntry{
+					Name: "default",
+					Services: []structs.ExportedService{{
+						Name:      "main",
+						Consumers: []structs.ServiceConsumer{{Peer: "my-peer"}},
+					}},
+				},
+			},
+			opAdd: &structs.ServiceResolverConfigEntry{
+				Kind: structs.ServiceResolver,
+				Name: "main",
+				Failover: map[string]structs.ServiceResolverFailover{
+					"*": {
+						Service:     "other",
+						Datacenters: []string{"dc12"},
+					},
+				},
+			},
+			expectErr: `contains cross-datacenter failover`,
+		},
+		"can't failover a peer exported http service to another service in a different datacenter using targets": {
+			entries: []structs.ConfigEntry{
+				&structs.ServiceConfigEntry{
+					Kind:     structs.ServiceDefaults,
+					Name:     "main",
+					Protocol: "http",
+				},
+				&structs.ServiceConfigEntry{
+					Kind:     structs.ServiceDefaults,
+					Name:     "other",
+					Protocol: "http",
+				},
+				&structs.ExportedServicesConfigEntry{
+					Name: "default",
+					Services: []structs.ExportedService{{
+						Name:      "main",
+						Consumers: []structs.ServiceConsumer{{Peer: "my-peer"}},
+					}},
+				},
+			},
+			opAdd: &structs.ServiceResolverConfigEntry{
+				Kind: structs.ServiceResolver,
+				Name: "main",
+				Failover: map[string]structs.ServiceResolverFailover{
+					"*": {
+						Targets: []structs.ServiceResolverFailoverTarget{
+							{
+								Service:    "other",
+								Datacenter: "dc12",
+							},
+						},
+					},
+				},
+			},
+			expectErr: `contains cross-datacenter failover`,
+		},
+		"can failover a peer exported tcp service": {
+			entries: []structs.ConfigEntry{
+				&structs.ExportedServicesConfigEntry{
+					Name: "default",
+					Services: []structs.ExportedService{{
+						Name:      "main",
+						Consumers: []structs.ServiceConsumer{{Peer: "my-peer"}},
+					}},
+				},
+			},
+			opAdd: &structs.ServiceResolverConfigEntry{
+				Kind: structs.ServiceResolver,
+				Name: "main",
+				Failover: map[string]structs.ServiceResolverFailover{
+					"*": {
+						Targets: []structs.ServiceResolverFailoverTarget{
+							{
+								Service: "other",
+							},
+							{
+								Service: "other",
+								Peer:    "cluster-01",
+							},
+						},
+					},
+				},
+			},
+		},
+		"can failover a peer exported tcp service from a redirect": {
+			entries: []structs.ConfigEntry{
+				&structs.ExportedServicesConfigEntry{
+					Name: "default",
+					Services: []structs.ExportedService{{
+						Name:      "main",
+						Consumers: []structs.ServiceConsumer{{Peer: "my-peer"}},
+					}},
+				},
+				&structs.ServiceResolverConfigEntry{
+					Kind: structs.ServiceResolver,
+					Name: "other",
+					Redirect: &structs.ServiceResolverRedirect{
+						Service: "main",
+					},
+				},
+			},
+			opAdd: &structs.ServiceResolverConfigEntry{
+				Kind: structs.ServiceResolver,
+				Name: "main",
+				Failover: map[string]structs.ServiceResolverFailover{
+					"*": {
+						Targets: []structs.ServiceResolverFailoverTarget{
+							{
+								Service: "another",
+							},
+							{
+								Service: "other",
+								Peer:    "cluster-01",
+							},
+						},
+					},
+				},
+			},
+		},
 	}

 	for name, tc := range cases {
--- a/agent/xds/endpoints.go
+++ b/agent/xds/endpoints.go
@ -776,8 +776,8 @@ func (s *ResourceGenerator) makeExportedUpstreamEndpointsForMeshGateway(cfgSnap

 		chainEndpoints := make(map[string]structs.CheckServiceNodes)
 		for _, target := range chain.Targets {
-			if !cfgSnap.Locality.Matches(target.Datacenter, target.Partition) {
-				s.Logger.Warn("ignoring discovery chain target that crosses a datacenter or partition boundary in a mesh gateway",
+			if !cfgSnap.Locality.Matches(target.Datacenter, target.Partition) || target.Peer != "" {
+				s.Logger.Warn("ignoring discovery chain target that crosses a datacenter, peer, or partition boundary in a mesh gateway",
 					"target", target,
 					"gatewayLocality", cfgSnap.Locality,
 				)