Skip to content

xds: generate routes directly from API gateway snapshot #17392

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sarahalsmiller merged 17 commits into from
May 25, 2023
Merged

xds: generate routes directly from API gateway snapshot #17392

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
ce47cb3
xds generation for routes api gateway
sarahalsmiller May 17, 2023
7c1c1ff
Update gateway.go
sarahalsmiller May 17, 2023
11ab02f
Merge branch 'main' into NET-3672-routesForAPIGateway
sarahalsmiller May 19, 2023
2779953
move buildHttpRoute into xds package
sarahalsmiller May 19, 2023
9ca8c7e
Update agent/consul/discoverychain/gateway.go
sarahalsmiller May 19, 2023
2dd8700
remove unneeded function
sarahalsmiller May 22, 2023
d782497
convert http route code to only run for http protocol to future proof…
sarahalsmiller May 22, 2023
d9bd811
Update agent/consul/discoverychain/gateway.go
sarahalsmiller May 22, 2023
ecbe243
fix tests, clean up http check logic
sarahalsmiller May 22, 2023
e13d041
clean up todo
sarahalsmiller May 22, 2023
2f07a6d
Fix casing in docstring
nathancoleman May 22, 2023
ecf5659
Merge branch 'main' into NET-3672-routesForAPIGateway
nathancoleman May 22, 2023
072aa45
Fix import block, adjust docstrings
nathancoleman May 22, 2023
8d3c354
Merge branch 'main' into NET-3672-routesForAPIGateway
sarahalsmiller May 24, 2023
3534acc
update name and comment
sarahalsmiller May 24, 2023
221816c
use constant value
sarahalsmiller May 24, 2023
71b808b
use constant
sarahalsmiller May 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 45 additions & 10 deletions agent/consul/discoverychain/gateway.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,13 @@ func (l *GatewayChainSynthesizer) SetHostname(hostname string) {
// 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.FilteredHostnames(l.hostname)
l.matchesByHostname = getHostMatches(l.hostname, &route, l.matchesByHostname)
}

func getHostMatches(hostname string, route *structs.HTTPRouteConfigEntry, currentMatches map[string][]hostnameMatch) map[string][]hostnameMatch {
Comment thread
sarahalsmiller marked this conversation as resolved.
Outdated
hostnames := route.FilteredHostnames(hostname)
for _, host := range hostnames {
matches, ok := l.matchesByHostname[host]
matches, ok := currentMatches[host]
if !ok {
matches = []hostnameMatch{}
}
Expand Down Expand Up @@ -90,8 +94,10 @@ func (l *GatewayChainSynthesizer) AddHTTPRoute(route structs.HTTPRouteConfigEntr
}
}

l.matchesByHostname[host] = matches
currentMatches[host] = matches
}
//TODO def don't think this is needed just testing for now, remove if not needed
Comment thread
sarahalsmiller marked this conversation as resolved.
Outdated
return currentMatches
Comment thread
sarahalsmiller marked this conversation as resolved.
Outdated
}

// Synthesize assembles a synthetic discovery chain from multiple other discovery chains
Expand All @@ -116,6 +122,7 @@ func (l *GatewayChainSynthesizer) Synthesize(chains ...*structs.CompiledDiscover

compiledChains := make([]*structs.CompiledDiscoveryChain, 0, len(set))
for i, service := range services {

entries := set[i]

compiled, err := Compile(CompileRequest{
Expand All @@ -126,7 +133,6 @@ func (l *GatewayChainSynthesizer) Synthesize(chains ...*structs.CompiledDiscover
EvaluateInTrustDomain: l.trustDomain,
Entries: entries,
})

if err != nil {
return nil, nil, err
}
Expand Down Expand Up @@ -188,17 +194,44 @@ func (l *GatewayChainSynthesizer) Synthesize(chains ...*structs.CompiledDiscover
// consolidateHTTPRoutes combines all rules into the shortest possible list of routes
// with one route per hostname containing all rules for that hostname.
func (l *GatewayChainSynthesizer) consolidateHTTPRoutes() []structs.HTTPRouteConfigEntry {
return consolidateHTTPRoutes(l.matchesByHostname, l.suffix, l.gateway)
}

// FlattenHTTPRoute takes in a route and its parent config entries and returns a list of flattened routes
func FlattenHTTPRoute(route *structs.HTTPRouteConfigEntry, listener *structs.APIGatewayListener, gateway *structs.APIGatewayConfigEntry) []structs.HTTPRouteConfigEntry {

@andrewstucki andrewstucki May 19, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I'm slightly confused as to what this is being used for. the route consolidation/flatenning that was happening in the discoverychain compilation previously was to merge all compatible routes on a listener so that we can have some "uber routes" that combine disparate parts of multiple routes configuration. This was due to the way that we otherwise couldn't take into account things like two routes with the same hostname values that have different path routing rules. In order for our synthesized discoverychain primitives to generate proper xDS in that case, we pretty much had to have one "uber route" per-hostname.

Guess I'm confused because this looks like it's trying to take in a single route and generate multiple routes from it? Additionally why would we be trying to flatten/merge routes if all we have at this point is a single route we're operating on?

@sarahalsmiller sarahalsmiller May 19, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a bit confusing, and this was something I wrote out by directly tracing what the ToIngress function was previously doing to the route. It could be set up wrong here, but we do get test failures if we don't run this function since we appear to be relying on some of the transformation that happens in the consolidateHTTPRoute function. Would it make more sense if matches persisted outside of this function?

@mikemorris mikemorris May 19, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked with @nathancoleman and @sarahalsmiller about how this function would probably make more sense if it was returning a lower-level construct closer to the xDS, but the logic of splitting matches per hostname seems to make sense - this feels like a reasonable candidate to consider refactoring later.

@nathancoleman nathancoleman May 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The route consolidation logic uses a specific format for the route name here. The pre-existing logic for synthesizing discovery chains uses the list of consolidated routes, so the disco chains are indexed using this specific name format.

The code that we're discussing here needs to look up disco chains using the index built earlier. If it doesn't use the same name format that the route consolidation logic used in the indexing step, then we aren't able to find any disco chains and end up producing no xDS resources because of this.

We could potentially rework the discovery chain synthesizing code in the future to use a different naming strategy when indexing discovery chains, but that's outside the scope of this PR.

@nathancoleman nathancoleman May 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment in the code explaining this - see 072aa45

@andrewstucki andrewstucki May 23, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's fine if we end up doing this mostly in a refactor, but there are few things I think we should make sure of when doing this here, and probably additionally add comments about:

  1. we can only use the name/hostname of the route that is returned from this function here, any other used fields will be wrong due to the fact that we're not actually consolidating this route with any other routes
  2. I think even in this PR, given point 1 above, that we should likely just do something like make a function called something to the effect of "SynthesizedRouteNames" to do the CRC32 rename + hostname splitting for route iteration, because as it stands, the helper introduced as part of the discoverychain just isn't doing any of the route merging/flattening that its name indicates it's doing due to the fact that it's only taking in a single route.
  3. down the line, in a follow-up, it probably makes the most sense to just do something like set the compiled routes on another field in proxycfg at synthesize time -- something like SynthesizedRoutes so that we're not leaking abstractions even more than we currently are into the xDS code

@sarahalsmiller sarahalsmiller May 23, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed flattenHTTPRoutes into a function called ReformatHTTPRoute and reverted the gateway discoverychain logic back to how it was before.

@sarahalsmiller sarahalsmiller May 24, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I ended up needed to revert that because it caused the integration tests to blow up, but hopefully the changes I made to the naming/comment make whats happening clearer.

//build map[string][]hostnameMatch for route
matches := map[string][]hostnameMatch{}
matches = getHostMatches(listener.GetHostname(), route, matches)
return consolidateHTTPRoutes(matches, listener.Name, gateway)
}

func RebuildHTTPRouteUpstream(route structs.HTTPRouteConfigEntry, listener structs.APIGatewayListener) structs.Upstream {
return structs.Upstream{
DestinationName: route.GetName(),
DestinationNamespace: route.NamespaceOrDefault(),
DestinationPartition: route.PartitionOrDefault(),
IngressHosts: route.Hostnames,
LocalBindPort: listener.Port,
Config: map[string]interface{}{
"protocol": string(listener.Protocol),
},
}
}
Comment thread
nathancoleman marked this conversation as resolved.
Outdated

// ConsolidateHTTPRoutes combines all rules into the shortest possible list of routes
// with one route per hostname containing all rules for that hostname.
func consolidateHTTPRoutes(matchesByHostname map[string][]hostnameMatch, suffix string, gateway *structs.APIGatewayConfigEntry) []structs.HTTPRouteConfigEntry {
Comment thread
sarahalsmiller marked this conversation as resolved.
Outdated
var routes []structs.HTTPRouteConfigEntry

for hostname, rules := range l.matchesByHostname {
for hostname, rules := range matchesByHostname {
// Create route for this hostname
route := structs.HTTPRouteConfigEntry{
Kind: structs.HTTPRoute,
Name: fmt.Sprintf("%s-%s-%s", l.gateway.Name, l.suffix, hostsKey(hostname)),
Name: fmt.Sprintf("%s-%s-%s", gateway.Name, suffix, hostsKey(hostname)),
Hostnames: []string{hostname},
Rules: make([]structs.HTTPRouteRule, 0, len(rules)),
Meta: l.gateway.Meta,
EnterpriseMeta: l.gateway.EnterpriseMeta,
Meta: gateway.Meta,
EnterpriseMeta: gateway.EnterpriseMeta,
}

// Sort rules for this hostname in order of precedence
Expand Down Expand Up @@ -258,12 +291,14 @@ func (l *GatewayChainSynthesizer) synthesizeEntries() ([]structs.IngressService,
entries := []*configentry.DiscoveryChainSet{}

for _, route := range l.consolidateHTTPRoutes() {
entrySet := configentry.NewDiscoveryChainSet()
ingress, router, splitters, defaults := synthesizeHTTPRouteDiscoveryChain(route)

services = append(services, ingress)

entrySet := configentry.NewDiscoveryChainSet()
entrySet.AddRouters(router)
entrySet.AddSplitters(splitters...)
entrySet.AddServices(defaults...)
services = append(services, ingress)
entries = append(entries, entrySet)
}

Expand Down
2 changes: 1 addition & 1 deletion agent/consul/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1906,7 +1906,7 @@ func TestServer_ReloadConfig(t *testing.T) {
defaults := DefaultConfig()
got := s.raft.ReloadableConfig()
require.Equal(t, uint64(4321), got.SnapshotThreshold,
"should have be reloaded to new value")
"should have been reloaded to new value")
require.Equal(t, defaults.RaftConfig.SnapshotInterval, got.SnapshotInterval,
"should have remained the default interval")
require.Equal(t, defaults.RaftConfig.TrailingLogs, got.TrailingLogs,
Expand Down
93 changes: 86 additions & 7 deletions agent/xds/routes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package xds
import (
"errors"
"fmt"
"github.com/hashicorp/consul/agent/consul/discoverychain"
"net"
"sort"
"strings"
Expand Down Expand Up @@ -36,13 +37,7 @@ func (s *ResourceGenerator) routesFromSnapshot(cfgSnap *proxycfg.ConfigSnapshot)
case structs.ServiceKindIngressGateway:
return s.routesForIngressGateway(cfgSnap)
case structs.ServiceKindAPIGateway:
// TODO Find a cleaner solution, can't currently pass unexported property types
var err error
cfgSnap.IngressGateway, err = cfgSnap.APIGateway.ToIngress(cfgSnap.Datacenter)
if err != nil {
return nil, err
}
return s.routesForIngressGateway(cfgSnap)
return s.routesForAPIGateway(cfgSnap)
Comment on lines -39 to +40

@nathancoleman nathancoleman May 19, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the crux of the whole change that we're making: instead of converting to an ingress gateway snapshot and generating xDS resources from that, we generate xDS resources directly from our API gateway snapshot 🎉

case structs.ServiceKindTerminatingGateway:
return s.routesForTerminatingGateway(cfgSnap)
case structs.ServiceKindMeshGateway:
Expand Down Expand Up @@ -430,6 +425,75 @@ func (s *ResourceGenerator) routesForIngressGateway(cfgSnap *proxycfg.ConfigSnap
return result, nil
}

// routesForAPIGateway returns the xDS API representation of the
// "routes" in the snapshot.
func (s *ResourceGenerator) routesForAPIGateway(cfgSnap *proxycfg.ConfigSnapshot) ([]proto.Message, error) {
var result []proto.Message

readyUpstreamsList := getReadyUpstreams(cfgSnap)

for _, readyUpstreams := range readyUpstreamsList {
listenerCfg := readyUpstreams.listenerCfg
// Do not create any route configuration for TCP listeners
if listenerCfg.Protocol == "tcp" {
continue
}
Comment thread
sarahalsmiller marked this conversation as resolved.

routeRef := readyUpstreams.routeReference
listenerKey := readyUpstreams.listenerKey

// Depending on their TLS config, upstreams are either attached to the
// default route or have their own routes. We'll add any upstreams that
// don't have custom filter chains and routes to this.
defaultRoute := &envoy_route_v3.RouteConfiguration{
Name: listenerKey.RouteName(),
// ValidateClusters defaults to true when defined statically and false
// when done via RDS. Re-set the reasonable value of true to prevent
// null-routing traffic.
ValidateClusters: makeBoolValue(true),
}

route, ok := cfgSnap.APIGateway.HTTPRoutes.Get(routeRef)
Comment thread
nathancoleman marked this conversation as resolved.
if !ok {
return nil, fmt.Errorf("missing route for route reference %s:%s", routeRef.Name, routeRef.Kind)
}

flattenedRoutes := discoverychain.FlattenHTTPRoute(route, &listenerCfg, cfgSnap.APIGateway.GatewayConfig)

for _, flattenedRoute := range flattenedRoutes {
flattenedRoute := flattenedRoute

upstream := discoverychain.RebuildHTTPRouteUpstream(flattenedRoute, listenerCfg)
uid := proxycfg.NewUpstreamID(&upstream)
chain := cfgSnap.APIGateway.DiscoveryChain[uid]
if chain == nil {
s.Logger.Debug("Discovery chain not found for flattened route", "discovery chain ID", uid)
continue
}

domains := generateUpstreamAPIsDomains(listenerKey, upstream, flattenedRoute.Hostnames)

virtualHost, err := s.makeUpstreamRouteForDiscoveryChain(cfgSnap, uid, chain, domains, false)
if err != nil {
return nil, err
}

injectHeaderManipToVirtualHostAPIGateway(&flattenedRoute, virtualHost)

// TODO Handle TLS config and add new route if appropriate
// We need something analogous to routeNameForUpstream used below
// But currently ToIngress is not handeling this usecase
defaultRoute.VirtualHosts = append(defaultRoute.VirtualHosts, virtualHost)
Comment thread
nathancoleman marked this conversation as resolved.
Outdated
}

if len(defaultRoute.VirtualHosts) > 0 {
result = append(result, defaultRoute)
}
}

return result, nil
}

func makeHeadersValueOptions(vals map[string]string, add bool) []*envoy_core_v3.HeaderValueOption {
opts := make([]*envoy_core_v3.HeaderValueOption, 0, len(vals))
for k, v := range vals {
Expand Down Expand Up @@ -516,6 +580,11 @@ func generateUpstreamIngressDomains(listenerKey proxycfg.IngressListenerKey, u s
return domains
}

func generateUpstreamAPIsDomains(listenerKey proxycfg.APIGatewayListenerKey, u structs.Upstream, hosts []string) []string {
u.IngressHosts = hosts
return generateUpstreamIngressDomains(listenerKey, u)
}

func (s *ResourceGenerator) makeUpstreamRouteForDiscoveryChain(
cfgSnap *proxycfg.ConfigSnapshot,
uid proxycfg.UpstreamID,
Expand Down Expand Up @@ -1019,6 +1088,16 @@ func injectHeaderManipToRoute(dest *structs.ServiceRouteDestination, r *envoy_ro
return nil
}

func injectHeaderManipToVirtualHostAPIGateway(dest *structs.HTTPRouteConfigEntry, vh *envoy_route_v3.VirtualHost) {
for _, rule := range dest.Rules {
for _, header := range rule.Filters.Headers {
vh.RequestHeadersToAdd = append(vh.RequestHeadersToAdd, makeHeadersValueOptions(header.Add, true)...)
vh.RequestHeadersToAdd = append(vh.RequestHeadersToAdd, makeHeadersValueOptions(header.Set, false)...)
vh.RequestHeadersToRemove = append(vh.RequestHeadersToRemove, header.Remove...)
}
}
}

func injectHeaderManipToVirtualHost(dest *structs.IngressService, vh *envoy_route_v3.VirtualHost) error {
if !dest.RequestHeaders.IsZero() {
vh.RequestHeadersToAdd = append(
Expand Down