From e4a992c58173ae52309b93ef23c85b3edd45a1e0 Mon Sep 17 00:00:00 2001 From: Andrew Stucki Date: Fri, 17 Feb 2023 13:18:11 -0500 Subject: [PATCH] Fix hostname alignment checks for HTTPRoutes (#16300) * Fix hostname alignment checks for HTTPRoutes --- agent/consul/discoverychain/gateway.go | 13 +- agent/consul/discoverychain/gateway_test.go | 3 + agent/consul/gateways/controller_gateways.go | 8 + agent/proxycfg/snapshot.go | 15 +- agent/structs/config_entry_gateways.go | 7 + agent/structs/config_entry_routes.go | 26 +++ .../capture.sh | 3 + .../service_gateway.hcl | 4 + .../case-api-gateway-http-hostnames/setup.sh | 156 ++++++++++++++++++ .../case-api-gateway-http-hostnames/vars.sh | 3 + .../verify.bats | 66 ++++++++ 11 files changed, 291 insertions(+), 13 deletions(-) create mode 100644 test/integration/connect/envoy/case-api-gateway-http-hostnames/capture.sh create mode 100644 test/integration/connect/envoy/case-api-gateway-http-hostnames/service_gateway.hcl create mode 100644 test/integration/connect/envoy/case-api-gateway-http-hostnames/setup.sh create mode 100644 test/integration/connect/envoy/case-api-gateway-http-hostnames/vars.sh create mode 100644 test/integration/connect/envoy/case-api-gateway-http-hostnames/verify.bats diff --git a/agent/consul/discoverychain/gateway.go b/agent/consul/discoverychain/gateway.go index cd582f1ec0..c9acdbbfda 100644 --- a/agent/consul/discoverychain/gateway.go +++ b/agent/consul/discoverychain/gateway.go @@ -17,6 +17,7 @@ type GatewayChainSynthesizer struct { trustDomain string suffix string gateway *structs.APIGatewayConfigEntry + hostname string matchesByHostname map[string][]hostnameMatch tcpRoutes []structs.TCPRouteConfigEntry } @@ -44,17 +45,17 @@ func (l *GatewayChainSynthesizer) AddTCPRoute(route structs.TCPRouteConfigEntry) l.tcpRoutes = append(l.tcpRoutes, route) } +// SetHostname sets the base hostname for a listener that this is being synthesized for +func (l *GatewayChainSynthesizer) SetHostname(hostname string) { + l.hostname = hostname +} + // AddHTTPRoute takes a new route and flattens its rule matches out per hostname. // This is required since a single route can specify multiple hostnames, and a // single hostname can be specified in multiple routes. Routing for a given // hostname must behave based on the aggregate of all rules that apply to it. func (l *GatewayChainSynthesizer) AddHTTPRoute(route structs.HTTPRouteConfigEntry) { - hostnames := route.Hostnames - if len(route.Hostnames) == 0 { - // add a wildcard if there are no explicit hostnames set - hostnames = append(hostnames, "*") - } - + hostnames := route.FilteredHostnames(l.hostname) for _, host := range hostnames { matches, ok := l.matchesByHostname[host] if !ok { diff --git a/agent/consul/discoverychain/gateway_test.go b/agent/consul/discoverychain/gateway_test.go index 1d6ec78d24..1c44f680b7 100644 --- a/agent/consul/discoverychain/gateway_test.go +++ b/agent/consul/discoverychain/gateway_test.go @@ -459,6 +459,7 @@ func TestGatewayChainSynthesizer_AddHTTPRoute(t *testing.T) { gatewayChainSynthesizer := NewGatewayChainSynthesizer(datacenter, "domain", "suffix", gateway) + gatewayChainSynthesizer.SetHostname("*") gatewayChainSynthesizer.AddHTTPRoute(tc.route) require.Equal(t, tc.expectedMatchesByHostname, gatewayChainSynthesizer.matchesByHostname) @@ -621,6 +622,8 @@ func TestGatewayChainSynthesizer_Synthesize(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { + tc.synthesizer.SetHostname("*") + for _, tcpRoute := range tc.tcpRoutes { tc.synthesizer.AddTCPRoute(*tcpRoute) } diff --git a/agent/consul/gateways/controller_gateways.go b/agent/consul/gateways/controller_gateways.go index 53d8c9a888..f06b48581f 100644 --- a/agent/consul/gateways/controller_gateways.go +++ b/agent/consul/gateways/controller_gateways.go @@ -701,6 +701,14 @@ func (g *gatewayMeta) bindRoute(listener *structs.APIGatewayListener, bound *str return false, nil } + if route, ok := route.(*structs.HTTPRouteConfigEntry); ok { + // check our hostnames + hostnames := route.FilteredHostnames(listener.GetHostname()) + if len(hostnames) == 0 { + return false, fmt.Errorf("failed to bind route to gateway %s: listener %s is does not have any hostnames that match the route", route.GetName(), g.Gateway.Name) + } + } + if listener.Protocol == route.GetProtocol() && bound.BindRoute(structs.ResourceReference{ Kind: route.GetKind(), Name: route.GetName(), diff --git a/agent/proxycfg/snapshot.go b/agent/proxycfg/snapshot.go index 6ce5a30083..f60e62319e 100644 --- a/agent/proxycfg/snapshot.go +++ b/agent/proxycfg/snapshot.go @@ -774,7 +774,7 @@ func (c *configSnapshotAPIGateway) ToIngress(datacenter string) (configSnapshotI } // Create a synthesized discovery chain for each service. - services, upstreams, compiled, err := c.synthesizeChains(datacenter, listener.Protocol, listener.Port, listener.Name, boundListener) + services, upstreams, compiled, err := c.synthesizeChains(datacenter, listener, boundListener) if err != nil { return configSnapshotIngressGateway{}, err } @@ -836,7 +836,7 @@ func (c *configSnapshotAPIGateway) ToIngress(datacenter string) (configSnapshotI }, nil } -func (c *configSnapshotAPIGateway) synthesizeChains(datacenter string, protocol structs.APIGatewayListenerProtocol, port int, name string, boundListener structs.BoundAPIGatewayListener) ([]structs.IngressService, structs.Upstreams, []*structs.CompiledDiscoveryChain, error) { +func (c *configSnapshotAPIGateway) synthesizeChains(datacenter string, listener structs.APIGatewayListener, boundListener structs.BoundAPIGatewayListener) ([]structs.IngressService, structs.Upstreams, []*structs.CompiledDiscoveryChain, error) { chains := []*structs.CompiledDiscoveryChain{} trustDomain := "" @@ -852,12 +852,13 @@ DOMAIN_LOOP: } } - synthesizer := discoverychain.NewGatewayChainSynthesizer(datacenter, trustDomain, name, c.GatewayConfig) + synthesizer := discoverychain.NewGatewayChainSynthesizer(datacenter, trustDomain, listener.Name, c.GatewayConfig) + synthesizer.SetHostname(listener.GetHostname()) for _, routeRef := range boundListener.Routes { switch routeRef.Kind { case structs.HTTPRoute: route, ok := c.HTTPRoutes.Get(routeRef) - if !ok || protocol != structs.ListenerProtocolHTTP { + if !ok || listener.Protocol != structs.ListenerProtocolHTTP { continue } synthesizer.AddHTTPRoute(*route) @@ -869,7 +870,7 @@ DOMAIN_LOOP: } case structs.TCPRoute: route, ok := c.TCPRoutes.Get(routeRef) - if !ok || protocol != structs.ListenerProtocolTCP { + if !ok || listener.Protocol != structs.ListenerProtocolTCP { continue } synthesizer.AddTCPRoute(*route) @@ -901,9 +902,9 @@ DOMAIN_LOOP: DestinationNamespace: service.NamespaceOrDefault(), DestinationPartition: service.PartitionOrDefault(), IngressHosts: service.Hosts, - LocalBindPort: port, + LocalBindPort: listener.Port, Config: map[string]interface{}{ - "protocol": string(protocol), + "protocol": string(listener.Protocol), }, }) } diff --git a/agent/structs/config_entry_gateways.go b/agent/structs/config_entry_gateways.go index 83bbcfb159..7d4ffd4479 100644 --- a/agent/structs/config_entry_gateways.go +++ b/agent/structs/config_entry_gateways.go @@ -897,6 +897,13 @@ type APIGatewayListener struct { TLS APIGatewayTLSConfiguration } +func (l APIGatewayListener) GetHostname() string { + if l.Hostname != "" { + return l.Hostname + } + return "*" +} + // APIGatewayTLSConfiguration specifies the configuration of a listener’s // TLS settings. type APIGatewayTLSConfiguration struct { diff --git a/agent/structs/config_entry_routes.go b/agent/structs/config_entry_routes.go index 7c959d79b0..26e50b3f2d 100644 --- a/agent/structs/config_entry_routes.go +++ b/agent/structs/config_entry_routes.go @@ -2,6 +2,7 @@ package structs import ( "fmt" + "strings" "github.com/hashicorp/consul/acl" ) @@ -121,6 +122,31 @@ func (e *HTTPRouteConfigEntry) GetRaftIndex() *RaftIndex { return &e.RaftIndex } +func (e *HTTPRouteConfigEntry) FilteredHostnames(listenerHostname string) []string { + if len(e.Hostnames) == 0 { + // we have no hostnames specified here, so treat it like a wildcard + return []string{listenerHostname} + } + + wildcardHostname := strings.ContainsRune(listenerHostname, '*') || listenerHostname == "*" + listenerHostname = strings.TrimPrefix(strings.TrimPrefix(listenerHostname, "*"), ".") + + hostnames := []string{} + for _, hostname := range e.Hostnames { + if wildcardHostname { + if strings.HasSuffix(hostname, listenerHostname) { + hostnames = append(hostnames, hostname) + } + continue + } + + if hostname == listenerHostname { + hostnames = append(hostnames, hostname) + } + } + return hostnames +} + // HTTPMatch specifies the criteria that should be // used in determining whether or not a request should // be routed to a given set of services. diff --git a/test/integration/connect/envoy/case-api-gateway-http-hostnames/capture.sh b/test/integration/connect/envoy/case-api-gateway-http-hostnames/capture.sh new file mode 100644 index 0000000000..8ba0e0ddab --- /dev/null +++ b/test/integration/connect/envoy/case-api-gateway-http-hostnames/capture.sh @@ -0,0 +1,3 @@ +#!/bin/bash + +snapshot_envoy_admin localhost:20000 api-gateway primary || true \ No newline at end of file diff --git a/test/integration/connect/envoy/case-api-gateway-http-hostnames/service_gateway.hcl b/test/integration/connect/envoy/case-api-gateway-http-hostnames/service_gateway.hcl new file mode 100644 index 0000000000..486c25c59e --- /dev/null +++ b/test/integration/connect/envoy/case-api-gateway-http-hostnames/service_gateway.hcl @@ -0,0 +1,4 @@ +services { + name = "api-gateway" + kind = "api-gateway" +} diff --git a/test/integration/connect/envoy/case-api-gateway-http-hostnames/setup.sh b/test/integration/connect/envoy/case-api-gateway-http-hostnames/setup.sh new file mode 100644 index 0000000000..f4963a2a24 --- /dev/null +++ b/test/integration/connect/envoy/case-api-gateway-http-hostnames/setup.sh @@ -0,0 +1,156 @@ +#!/bin/bash + +set -euo pipefail + +upsert_config_entry primary ' +kind = "api-gateway" +name = "api-gateway" +listeners = [ + { + name = "listener-one" + port = 9999 + protocol = "http" + hostname = "*.consul.example" + }, + { + name = "listener-two" + port = 9998 + protocol = "http" + hostname = "foo.bar.baz" + }, + { + name = "listener-three" + port = 9997 + protocol = "http" + hostname = "*.consul.example" + }, + { + name = "listener-four" + port = 9996 + protocol = "http" + hostname = "*.consul.example" + }, + { + name = "listener-five" + port = 9995 + protocol = "http" + hostname = "foo.bar.baz" + } +] +' + +upsert_config_entry primary ' +Kind = "proxy-defaults" +Name = "global" +Config { + protocol = "http" +} +' + +upsert_config_entry primary ' +kind = "http-route" +name = "api-gateway-route-one" +hostnames = ["test.consul.example"] +rules = [ + { + services = [ + { + name = "s1" + } + ] + } +] +parents = [ + { + name = "api-gateway" + sectionName = "listener-one" + }, +] +' + +upsert_config_entry primary ' +kind = "http-route" +name = "api-gateway-route-two" +hostnames = ["foo.bar.baz"] +rules = [ + { + services = [ + { + name = "s1" + } + ] + } +] +parents = [ + { + name = "api-gateway" + sectionName = "listener-two" + }, +] +' + +upsert_config_entry primary ' +kind = "http-route" +name = "api-gateway-route-three" +hostnames = ["foo.bar.baz"] +rules = [ + { + services = [ + { + name = "s1" + } + ] + } +] +parents = [ + { + name = "api-gateway" + sectionName = "listener-three" + }, +] +' + +upsert_config_entry primary ' +kind = "http-route" +name = "api-gateway-route-four" +rules = [ + { + services = [ + { + name = "s1" + } + ] + } +] +parents = [ + { + name = "api-gateway" + sectionName = "listener-four" + }, +] +' + +upsert_config_entry primary ' +kind = "http-route" +name = "api-gateway-route-five" +rules = [ + { + services = [ + { + name = "s1" + } + ] + } +] +parents = [ + { + name = "api-gateway" + sectionName = "listener-five" + }, +] +' + +register_services primary + +gen_envoy_bootstrap api-gateway 20000 primary true +gen_envoy_bootstrap s1 19000 \ No newline at end of file diff --git a/test/integration/connect/envoy/case-api-gateway-http-hostnames/vars.sh b/test/integration/connect/envoy/case-api-gateway-http-hostnames/vars.sh new file mode 100644 index 0000000000..38a47d8527 --- /dev/null +++ b/test/integration/connect/envoy/case-api-gateway-http-hostnames/vars.sh @@ -0,0 +1,3 @@ +#!/bin/bash + +export REQUIRED_SERVICES="$DEFAULT_REQUIRED_SERVICES api-gateway-primary" diff --git a/test/integration/connect/envoy/case-api-gateway-http-hostnames/verify.bats b/test/integration/connect/envoy/case-api-gateway-http-hostnames/verify.bats new file mode 100644 index 0000000000..ba109ea6f9 --- /dev/null +++ b/test/integration/connect/envoy/case-api-gateway-http-hostnames/verify.bats @@ -0,0 +1,66 @@ +#!/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 have 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 be bound to route one" { + 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 bound to route two" { + assert_config_entry_status Bound True Bound primary http-route api-gateway-route-two +} + +@test "api gateway should be unbound to route three" { + assert_config_entry_status Bound False FailedToBind primary http-route api-gateway-route-three +} + +@test "api gateway should be bound to route four" { + assert_config_entry_status Bound True Bound primary http-route api-gateway-route-four +} + +@test "api gateway should be bound to route five" { + assert_config_entry_status Bound True Bound primary http-route api-gateway-route-five +} + +@test "api gateway should be able to connect to s1 via route one with the proper host" { + run retry_long curl -H "Host: test.consul.example" -s -f -d hello localhost:9999 + [ "$status" -eq 0 ] + [[ "$output" == *"hello"* ]] +} + +@test "api gateway should not be able to connect to s1 via route one with a mismatched host" { + run retry_default sh -c "curl -H \"Host: foo.consul.example\" -sI -o /dev/null -w \"%{http_code}\" localhost:9999 | grep 404" + [ "$status" -eq 0 ] + [[ "$output" == "404" ]] +} + +@test "api gateway should be able to connect to s1 via route two with the proper host" { + run retry_long curl -H "Host: foo.bar.baz" -s -f -d hello localhost:9998 + [ "$status" -eq 0 ] + [[ "$output" == *"hello"* ]] +} + +@test "api gateway should be able to connect to s1 via route four with any subdomain of the listener host" { + run retry_long curl -H "Host: test.consul.example" -s -f -d hello localhost:9996 + [ "$status" -eq 0 ] + [[ "$output" == *"hello"* ]] + run retry_long curl -H "Host: foo.consul.example" -s -f -d hello localhost:9996 + [ "$status" -eq 0 ] + [[ "$output" == *"hello"* ]] +} + +@test "api gateway should be able to connect to s1 via route five with the proper host" { + run retry_long curl -H "Host: foo.bar.baz" -s -f -d hello localhost:9995 + [ "$status" -eq 0 ] + [[ "$output" == *"hello"* ]] +} \ No newline at end of file