xds: generate routes directly from API gateway snapshot #17392
Conversation
sarahalsmiller
changed the base branch from
main
to
NET-3673-endpointsFromSnapshotAPIGateway
sarahalsmiller
added
pr/no-changelog
sarahalsmiller
added
pr/no-backport
and removed
backport/1.15
nathancoleman
changed the title
| // 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) |
There was a problem hiding this comment.
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 🎉
| } | ||
|
|
||
| // 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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I added a comment in the code explaining this - see 072aa45
There was a problem hiding this comment.
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:
- 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
- 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.
- 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
SynthesizedRoutesso that we're not leaking abstractions even more than we currently are into the xDS code
There was a problem hiding this comment.
Changed flattenHTTPRoutes into a function called ReformatHTTPRoute and reverted the gateway discoverychain logic back to how it was before.
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| // 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 { |
There was a problem hiding this comment.
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.
Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com>
sarahalsmiller
force-pushed
the
NET-3672-routesForAPIGateway
branch
2 times, most recently
from
a7a387b to
072aa45
Compare
Description
In order to move the gateway off of using an underlying ingress gateway, add functionality to generate API Gateway natively. Previously this code was all in a single branch, but for ease of tracking and reviewing, breaking this code into 4 separate PRs.
NOTE- this code relies on a helper function introduced in this PR #17390 and tests will not pass until that code is merged.
Testing & Reproduction steps
Links
PR Checklist