Skip to content
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.

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

xds: generate listeners directly from API gateway snapshot #17398

Merged
merged 40 commits into from
May 22, 2023

Conversation

sarahalsmiller
Copy link
Member

@sarahalsmiller sarahalsmiller commented May 17, 2023

Description

Instead of converting each API gateway config snapshot into an ingress gateway config snapshot and then generating xDS resources from that, generate xDS resources directly from the API gateway config snapshot.

Testing & Reproduction steps

Links

PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

sarahalsmiller and others added 11 commits April 26, 2023 10:17
* XDS primitive generation for endpoints and clusters

Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>

* server_test

* deleted extra file

* add missing parents to test

---------

Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>
* XDS primitive generation for endpoints and clusters

Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>

* server_test

* deleted extra file

* add missing parents to test

* checkpoint

* delete extra file

* httproute flattening code

* linting issue

* so close on this, calling for tonight

* unit test passing

* add in header manip to virtual host

* upstream rebuild commented out

* Use consistent upstream name whether or not we're rebuilding

* Start working through route naming logic

* Fix typos in test descriptions

* Simplify route naming logic

* Simplify RebuildHTTPRouteUpstream

* Merge additional compiled discovery chains instead of overwriting

* Use correct chain for flattened route, clean up + add TODOs

* Remove empty conditional branch

* Restore previous variable declaration

Limit the scope of this PR

* Clean up, improve TODO

* add logging, clean up todos

* clean up function

---------

Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>
Co-authored-by: John Maguire <john.maguire@hashicorp.com>
@github-actions github-actions bot added the theme/envoy/xds Related to Envoy support label May 17, 2023
sarahalsmiller and others added 6 commits May 18, 2023 10:07
Each listener would previously have all upstreams from any route that bound to the listener. This is problematic when a route bound to one listener also binds to other listeners and so includes upstreams for multiple listeners. The list for a given listener would then wind up including upstreams for other listeners.
@sarahalsmiller sarahalsmiller changed the base branch from NET-2063-Implementation-API-GW-Use-XDS-primitives-instead-of-Ingress-GW-primitives to NET-3673-endpointsFromSnapshotAPIGateway May 18, 2023 19:43
@nathancoleman nathancoleman changed the title WIP- Net 3671 make api gateway listeners xds: generate listeners directly from API gateway snapshot May 18, 2023
return consolidateHTTPRoutes(matches, listener.Name, gateway)
}

func RebuildHTTPRouteUpstream(route structs.HTTPRouteConfigEntry, listener structs.APIGatewayListener) structs.Upstream {
Copy link
Member

Choose a reason for hiding this comment

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

"rebuild" makes me feel like we're doing something a lot more intense than just initializing a struct. Thoughts on changing to just "build"?

Suggested change
func RebuildHTTPRouteUpstream(route structs.HTTPRouteConfigEntry, listener structs.APIGatewayListener) structs.Upstream {
func BuildHTTPRouteUpstream(route structs.HTTPRouteConfigEntry, listener structs.APIGatewayListener) structs.Upstream {

Copy link
Contributor

@t-eckert t-eckert left a comment

Choose a reason for hiding this comment

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

I love how much more clear the logic reads now! There are still some TODOs which I'm not sure are for after this PR or not. I'll approve anyway so you can move forward.

agent/consul/discoverychain/gateway.go Outdated Show resolved Hide resolved
agent/structs/config_entry_gateways.go Outdated Show resolved Hide resolved
agent/xds/listeners_apigateway.go Outdated Show resolved Hide resolved
agent/xds/listeners_apigateway.go Outdated Show resolved Hide resolved
Copy link
Member

@nathancoleman nathancoleman left a comment

Choose a reason for hiding this comment

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

This is looking great! I think there's still a little cleanup that you and I can do to make it polished and shiny

agent/consul/discoverychain/gateway.go Outdated Show resolved Hide resolved
agent/consul/discoverychain/gateway.go Outdated Show resolved Hide resolved
agent/consul/discoverychain/gateway.go Outdated Show resolved Hide resolved
agent/proxycfg/snapshot.go Outdated Show resolved Hide resolved
// 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
}
listeners, err := s.makeIngressGatewayListeners(a.Address, cfgSnap)
listeners, err := s.makeAPIGatewayListeners(a.Address, cfgSnap)
Copy link
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 🎉

sarahalsmiller and others added 4 commits May 19, 2023 13:29
Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>
Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>
…rp/consul into NET-3671-makeAPIGatewayListeners
Base automatically changed from NET-3673-endpointsFromSnapshotAPIGateway to main May 19, 2023 18:51
@sarahalsmiller sarahalsmiller merged commit e2a81aa into main May 22, 2023
@sarahalsmiller sarahalsmiller deleted the NET-3671-makeAPIGatewayListeners branch May 22, 2023 21:36
nickethier pushed a commit that referenced this pull request May 26, 2023
* API Gateway XDS Primitives, endpoints and clusters (#17002)

* XDS primitive generation for endpoints and clusters

Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>

* server_test

* deleted extra file

* add missing parents to test

---------

Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>

* Routes for API Gateway (#17158)

* XDS primitive generation for endpoints and clusters

Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>

* server_test

* deleted extra file

* add missing parents to test

* checkpoint

* delete extra file

* httproute flattening code

* linting issue

* so close on this, calling for tonight

* unit test passing

* add in header manip to virtual host

* upstream rebuild commented out

* Use consistent upstream name whether or not we're rebuilding

* Start working through route naming logic

* Fix typos in test descriptions

* Simplify route naming logic

* Simplify RebuildHTTPRouteUpstream

* Merge additional compiled discovery chains instead of overwriting

* Use correct chain for flattened route, clean up + add TODOs

* Remove empty conditional branch

* Restore previous variable declaration

Limit the scope of this PR

* Clean up, improve TODO

* add logging, clean up todos

* clean up function

---------

Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>

* checkpoint, skeleton, tests not passing

* checkpoint

* endpoints xds cluster configuration

* resources test fix

* fix reversion in resources_test

* checkpoint

* Update agent/proxycfg/api_gateway.go

Co-authored-by: John Maguire <john.maguire@hashicorp.com>

* unit tests passing

* gofmt

* add deterministic sorting to appease the unit test gods

* remove panic

* Find ready upstream matching listener instead of first in list

* Clean up, improve TODO

* Modify getReadyUpstreams to filter upstreams by listener (#17410)

Each listener would previously have all upstreams from any route that bound to the listener. This is problematic when a route bound to one listener also binds to other listeners and so includes upstreams for multiple listeners. The list for a given listener would then wind up including upstreams for other listeners.

* clean up todos, references to api gateway in listeners_ingress

* merge in Nathan's fix

* Update agent/consul/discoverychain/gateway.go

* cleanup current todos, remove snapshot manipulation from generation code

* Update agent/structs/config_entry_gateways.go

Co-authored-by: Thomas Eckert <teckert@hashicorp.com>

* Update agent/consul/discoverychain/gateway.go

Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>

* Update agent/consul/discoverychain/gateway.go

Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>

* Update agent/proxycfg/snapshot.go

Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>

* clarified header comment for FlattenHTTPRoute, changed RebuildHTTPRouteUpstream to BuildHTTPRouteUpstream

* simplify cert logic

* Delete scratch

* revert route related changes in listener PR

* Update agent/consul/discoverychain/gateway.go

* Update agent/proxycfg/snapshot.go

* clean up uneeded extra lines in endpoints

---------

Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>
Co-authored-by: John Maguire <john.maguire@hashicorp.com>
Co-authored-by: Thomas Eckert <teckert@hashicorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/no-backport pr/no-changelog PR does not need a corresponding .changelog entry theme/envoy/xds Related to Envoy support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants