[API Gateway] Fix targeting service splitters in HTTPRoutes (#16350)

* [API Gateway] Fix targeting service splitters in HTTPRoutes

* Fix test description

agent/consul/discoverychain/gateway.go

@@ -5,6 +5,7 @@ import (
 	"hash/crc32"
 	"sort"
 	"strconv"
+	"strings"
 
 	"github.com/hashicorp/consul/agent/configentry"
 	"github.com/hashicorp/consul/agent/structs"
@@ -126,6 +127,23 @@ func (l *GatewayChainSynthesizer) Synthesize(chains ...*structs.CompiledDiscover
 		if err != nil {
 			return nil, nil, err
 		}
+
+		// fix up the nodes for the terminal targets to either be a splitter or resolver if there is no splitter present
+		for name, node := range compiled.Nodes {
+			switch node.Type {
+			// we should only have these two types
+			case structs.DiscoveryGraphNodeTypeRouter:
+				for i, route := range node.Routes {
+					node.Routes[i].NextNode = targetForResolverNode(route.NextNode, chains)
+				}
+			case structs.DiscoveryGraphNodeTypeSplitter:
+				for i, split := range node.Splits {
+					node.Splits[i].NextNode = targetForResolverNode(split.NextNode, chains)
+				}
+			}
+			compiled.Nodes[name] = node
+		}
+
 		for _, c := range chains {
 			for id, target := range c.Targets {
 				compiled.Targets[id] = target
@@ -177,6 +195,27 @@ func (l *GatewayChainSynthesizer) consolidateHTTPRoutes() []structs.HTTPRouteCon
 	return routes
 }
 
+func targetForResolverNode(nodeName string, chains []*structs.CompiledDiscoveryChain) string {
+	resolverPrefix := structs.DiscoveryGraphNodeTypeResolver + ":"
+	splitterPrefix := structs.DiscoveryGraphNodeTypeSplitter + ":"
+
+	if !strings.HasPrefix(nodeName, resolverPrefix) {
+		return nodeName
+	}
+
+	splitterName := splitterPrefix + strings.TrimPrefix(nodeName, resolverPrefix)
+
+	for _, c := range chains {
+		for name, node := range c.Nodes {
+			if node.IsSplitter() && strings.HasPrefix(splitterName, name) {
+				return name
+			}
+		}
+	}
+
+	return nodeName
+}
+
 func hostsKey(hosts ...string) string {
 	sort.Strings(hosts)
 	hostsHash := crc32.NewIEEE()

agent/consul/discoverychain/gateway_test.go

@@ -3,6 +3,7 @@ package discoverychain
 import (
 	"testing"
 
+	"github.com/hashicorp/consul/agent/configentry"
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/stretchr/testify/require"
 )
@@ -640,3 +641,256 @@ func TestGatewayChainSynthesizer_Synthesize(t *testing.T) {
 		})
 	}
 }
+
+func TestGatewayChainSynthesizer_ComplexChain(t *testing.T) {
+	t.Parallel()
+
+	cases := map[string]struct {
+		synthesizer            *GatewayChainSynthesizer
+		route                  *structs.HTTPRouteConfigEntry
+		entries                []structs.ConfigEntry
+		expectedDiscoveryChain *structs.CompiledDiscoveryChain
+	}{
+		"HTTP-Route with nested splitters": {
+			synthesizer: NewGatewayChainSynthesizer("dc1", "domain", "suffix", &structs.APIGatewayConfigEntry{
+				Kind: structs.APIGateway,
+				Name: "gateway",
+			}),
+			route: &structs.HTTPRouteConfigEntry{
+				Kind: structs.HTTPRoute,
+				Name: "test",
+				Rules: []structs.HTTPRouteRule{{
+					Services: []structs.HTTPService{{
+						Name: "splitter-one",
+					}},
+				}},
+			},
+			entries: []structs.ConfigEntry{
+				&structs.ServiceSplitterConfigEntry{
+					Kind: structs.ServiceSplitter,
+					Name: "splitter-one",
+					Splits: []structs.ServiceSplit{{
+						Service: "service-one",
+						Weight:  50,
+					}, {
+						Service: "splitter-two",
+						Weight:  50,
+					}},
+				},
+				&structs.ServiceSplitterConfigEntry{
+					Kind: structs.ServiceSplitter,
+					Name: "splitter-two",
+					Splits: []structs.ServiceSplit{{
+						Service: "service-two",
+						Weight:  50,
+					}, {
+						Service: "service-three",
+						Weight:  50,
+					}},
+				},
+				&structs.ProxyConfigEntry{
+					Kind: structs.ProxyConfigGlobal,
+					Name: "global",
+					Config: map[string]interface{}{
+						"protocol": "http",
+					},
+				},
+			},
+			expectedDiscoveryChain: &structs.CompiledDiscoveryChain{
+				ServiceName: "gateway-suffix-9b9265b",
+				Namespace:   "default",
+				Partition:   "default",
+				Datacenter:  "dc1",
+				Protocol:    "http",
+				StartNode:   "router:gateway-suffix-9b9265b.default.default",
+				Nodes: map[string]*structs.DiscoveryGraphNode{
+					"resolver:gateway-suffix-9b9265b.default.default.dc1": {
+						Type: "resolver",
+						Name: "gateway-suffix-9b9265b.default.default.dc1",
+						Resolver: &structs.DiscoveryResolver{
+							Target:         "gateway-suffix-9b9265b.default.default.dc1",
+							Default:        true,
+							ConnectTimeout: 5000000000,
+						},
+					},
+					"resolver:service-one.default.default.dc1": {
+						Type: "resolver",
+						Name: "service-one.default.default.dc1",
+						Resolver: &structs.DiscoveryResolver{
+							Target:         "service-one.default.default.dc1",
+							Default:        true,
+							ConnectTimeout: 5000000000,
+						},
+					},
+					"resolver:service-three.default.default.dc1": {
+						Type: "resolver",
+						Name: "service-three.default.default.dc1",
+						Resolver: &structs.DiscoveryResolver{
+							Target:         "service-three.default.default.dc1",
+							Default:        true,
+							ConnectTimeout: 5000000000,
+						},
+					},
+					"resolver:service-two.default.default.dc1": {
+						Type: "resolver",
+						Name: "service-two.default.default.dc1",
+						Resolver: &structs.DiscoveryResolver{
+							Target:         "service-two.default.default.dc1",
+							Default:        true,
+							ConnectTimeout: 5000000000,
+						},
+					},
+					"resolver:splitter-one.default.default.dc1": {
+						Type: "resolver",
+						Name: "splitter-one.default.default.dc1",
+						Resolver: &structs.DiscoveryResolver{
+							Target:         "splitter-one.default.default.dc1",
+							Default:        true,
+							ConnectTimeout: 5000000000,
+						},
+					},
+					"router:gateway-suffix-9b9265b.default.default": {
+						Type: "router",
+						Name: "gateway-suffix-9b9265b.default.default",
+						Routes: []*structs.DiscoveryRoute{{
+							Definition: &structs.ServiceRoute{
+								Match: &structs.ServiceRouteMatch{
+									HTTP: &structs.ServiceRouteHTTPMatch{
+										PathPrefix: "/",
+									},
+								},
+								Destination: &structs.ServiceRouteDestination{
+									Service:   "splitter-one",
+									Partition: "default",
+									Namespace: "default",
+									RequestHeaders: &structs.HTTPHeaderModifiers{
+										Add: make(map[string]string),
+										Set: make(map[string]string),
+									},
+								},
+							},
+							NextNode: "splitter:splitter-one.default.default",
+						}, {
+							Definition: &structs.ServiceRoute{
+								Match: &structs.ServiceRouteMatch{
+									HTTP: &structs.ServiceRouteHTTPMatch{
+										PathPrefix: "/",
+									},
+								},
+								Destination: &structs.ServiceRouteDestination{
+									Service:   "gateway-suffix-9b9265b",
+									Partition: "default",
+									Namespace: "default",
+								},
+							},
+							NextNode: "resolver:gateway-suffix-9b9265b.default.default.dc1",
+						}},
+					},
+					"splitter:splitter-one.default.default": {
+						Type: structs.DiscoveryGraphNodeTypeSplitter,
+						Name: "splitter-one.default.default",
+						Splits: []*structs.DiscoverySplit{{
+							Definition: &structs.ServiceSplit{
+								Weight:  50,
+								Service: "service-one",
+							},
+							Weight:   50,
+							NextNode: "resolver:service-one.default.default.dc1",
+						}, {
+							Definition: &structs.ServiceSplit{
+								Weight:  50,
+								Service: "service-two",
+							},
+							Weight:   25,
+							NextNode: "resolver:service-two.default.default.dc1",
+						}, {
+							Definition: &structs.ServiceSplit{
+								Weight:  50,
+								Service: "service-three",
+							},
+							Weight:   25,
+							NextNode: "resolver:service-three.default.default.dc1",
+						}},
+					},
+				}, Targets: map[string]*structs.DiscoveryTarget{
+					"gateway-suffix-9b9265b.default.default.dc1": {
+						ID:             "gateway-suffix-9b9265b.default.default.dc1",
+						Service:        "gateway-suffix-9b9265b",
+						Datacenter:     "dc1",
+						Partition:      "default",
+						Namespace:      "default",
+						ConnectTimeout: 5000000000,
+						SNI:            "gateway-suffix-9b9265b.default.dc1.internal.domain",
+						Name:           "gateway-suffix-9b9265b.default.dc1.internal.domain",
+					},
+					"service-one.default.default.dc1": {
+						ID:             "service-one.default.default.dc1",
+						Service:        "service-one",
+						Datacenter:     "dc1",
+						Partition:      "default",
+						Namespace:      "default",
+						ConnectTimeout: 5000000000,
+						SNI:            "service-one.default.dc1.internal.domain",
+						Name:           "service-one.default.dc1.internal.domain",
+					},
+					"service-three.default.default.dc1": {
+						ID:             "service-three.default.default.dc1",
+						Service:        "service-three",
+						Datacenter:     "dc1",
+						Partition:      "default",
+						Namespace:      "default",
+						ConnectTimeout: 5000000000,
+						SNI:            "service-three.default.dc1.internal.domain",
+						Name:           "service-three.default.dc1.internal.domain",
+					},
+					"service-two.default.default.dc1": {
+						ID:             "service-two.default.default.dc1",
+						Service:        "service-two",
+						Datacenter:     "dc1",
+						Partition:      "default",
+						Namespace:      "default",
+						ConnectTimeout: 5000000000,
+						SNI:            "service-two.default.dc1.internal.domain",
+						Name:           "service-two.default.dc1.internal.domain",
+					},
+					"splitter-one.default.default.dc1": {
+						ID:             "splitter-one.default.default.dc1",
+						Service:        "splitter-one",
+						Datacenter:     "dc1",
+						Partition:      "default",
+						Namespace:      "default",
+						ConnectTimeout: 5000000000,
+						SNI:            "splitter-one.default.dc1.internal.domain",
+						Name:           "splitter-one.default.dc1.internal.domain",
+					},
+				}},
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			service := tc.entries[0]
+			entries := configentry.NewDiscoveryChainSet()
+			entries.AddEntries(tc.entries...)
+			compiled, err := Compile(CompileRequest{
+				ServiceName:           service.GetName(),
+				EvaluateInNamespace:   service.GetEnterpriseMeta().NamespaceOrDefault(),
+				EvaluateInPartition:   service.GetEnterpriseMeta().PartitionOrDefault(),
+				EvaluateInDatacenter:  "dc1",
+				EvaluateInTrustDomain: "domain",
+				Entries:               entries,
+			})
+			require.NoError(t, err)
+
+			tc.synthesizer.SetHostname("*")
+			tc.synthesizer.AddHTTPRoute(*tc.route)
+
+			chains := []*structs.CompiledDiscoveryChain{compiled}
+			_, discoveryChains, err := tc.synthesizer.Synthesize(chains...)
+
+			require.NoError(t, err)
+			require.Len(t, discoveryChains, 1)
+			require.Equal(t, tc.expectedDiscoveryChain, discoveryChains[0])
+		})
+	}
+}

agent/structs/config_entry_routes.go

@@ -79,10 +79,10 @@ func (e *HTTPRouteConfigEntry) Normalize() error {
 	for i, parent := range e.Parents {
 		if parent.Kind == "" {
 			parent.Kind = APIGateway
+		}
 		parent.EnterpriseMeta.Normalize()
 		e.Parents[i] = parent
 	}
-	}
 
 	for i, rule := range e.Rules {
 		for j, match := range rule.Matches {
@@ -505,10 +505,10 @@ func (e *TCPRouteConfigEntry) Normalize() error {
 	for i, parent := range e.Parents {
 		if parent.Kind == "" {
 			parent.Kind = APIGateway
+		}
 		parent.EnterpriseMeta.Normalize()
 		e.Parents[i] = parent
 	}
-	}
 
 	for i, service := range e.Services {
 		service.EnterpriseMeta.Normalize()

test/integration/connect/envoy/case-api-gateway-http-splitter-targets/capture.sh

@@ -0,0 +1,3 @@
+#!/bin/bash
+
+snapshot_envoy_admin localhost:20000 api-gateway primary || true

test/integration/connect/envoy/case-api-gateway-http-splitter-targets/service_gateway.hcl

@@ -0,0 +1,4 @@
+services {
+  name = "api-gateway"
+  kind = "api-gateway"
+}

test/integration/connect/envoy/case-api-gateway-http-splitter-targets/service_s3.hcl

@@ -0,0 +1,9 @@
+services {
+  id   = "s3"
+  name = "s3"
+  port = 8182
+
+  connect {
+    sidecar_service {}
+  }
+}

test/integration/connect/envoy/case-api-gateway-http-splitter-targets/setup.sh

@@ -0,0 +1,80 @@
+#!/bin/bash
+
+set -euo pipefail
+
+upsert_config_entry primary '
+kind = "api-gateway"
+name = "api-gateway"
+listeners = [
+  {
+    name = "listener-one"
+    port = 9999
+    protocol = "http"
+  }
+]
+'
+
+upsert_config_entry primary '
+Kind      = "proxy-defaults"
+Name      = "global"
+Config {
+  protocol = "http"
+}
+'
+
+upsert_config_entry primary '
+kind = "http-route"
+name = "api-gateway-route-one"
+rules = [
+  {
+    services = [
+      {
+        name = "splitter-one"
+      }
+    ]
+  }
+]
+parents = [
+  {
+    name = "api-gateway"
+    sectionName = "listener-one"
+  }
+]
+'
+
+upsert_config_entry primary '
+kind = "service-splitter"
+name = "splitter-one"
+splits = [
+  {
+    weight = 50,
+    service = "s1"
+  },
+  {
+    weight = 50,
+    service = "splitter-two"
+  },
+]
+'
+
+upsert_config_entry primary '
+kind = "service-splitter"
+name = "splitter-two"
+splits = [
+  {
+    weight = 50,
+    service = "s2"
+  },
+  {
+    weight = 50,
+    service = "s3"
+  },
+]
+'
+
+register_services primary
+
+gen_envoy_bootstrap api-gateway 20000 primary true
+gen_envoy_bootstrap s1 19000
+gen_envoy_bootstrap s2 19001
+gen_envoy_bootstrap s3 19002

test/integration/connect/envoy/case-api-gateway-http-splitter-targets/vars.sh

@@ -0,0 +1,3 @@
+#!/bin/bash
+
+export REQUIRED_SERVICES="$DEFAULT_REQUIRED_SERVICES api-gateway-primary"

test/integration/connect/envoy/case-api-gateway-http-splitter-targets/verify.bats

@@ -0,0 +1,23 @@
+#!/usr/bin/env bats
+
+load helpers
+
+@test "api gateway proxy admin is up on :20000" {
+  retry_default curl -f -s localhost:20000/stats -o /dev/null
+}
+
+@test "api gateway should be accepted and not conflicted" {
+  assert_config_entry_status Accepted True Accepted primary api-gateway api-gateway
+  assert_config_entry_status Conflicted False NoConflict primary api-gateway api-gateway
+}
+
+@test "api gateway should have healthy endpoints for s1" {
+  assert_config_entry_status Bound True Bound primary http-route api-gateway-route-one
+  assert_upstream_has_endpoints_in_status 127.0.0.1:20000 s1 HEALTHY 1
+}
+
+@test "api gateway should be able to connect to s1, s2, and s3 via configured port" {
+  run retry_default assert_expected_fortio_name_pattern ^FORTIO_NAME=s1$
+  run retry_default assert_expected_fortio_name_pattern ^FORTIO_NAME=s2$
+  run retry_default assert_expected_fortio_name_pattern ^FORTIO_NAME=s3$
+}