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

API Gateway XDS Primitives, endpoints and clusters #17002

Conversation

sarahalsmiller
Copy link
Member

@sarahalsmiller sarahalsmiller commented Apr 14, 2023

Description

Work to move APIGateway away from using prexisting ingress gateway functionality.

Testing & Reproduction steps

Existing gateway behavior should be unchanged with this change, so existing CI tests continuing to pass should be sufficient.

Links

Relevant RFC: https://docs.google.com/document/d/15kzB5wqhzzPOMWCD8SphpcYUAigUmJvV6R6y2MZUnJU/edit?usp=sharing

PR Checklist

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

@sarahalsmiller sarahalsmiller added the backport/1.15 This release series is no longer active on CE. Use backport/ent/1.15. label Apr 14, 2023
@github-actions github-actions bot added the theme/envoy/xds Related to Envoy support label Apr 14, 2023
@sarahalsmiller sarahalsmiller marked this pull request as draft April 14, 2023 18:52
continue
}

chain, ok := cfgSnap.IngressGateway.DiscoveryChain[uid]
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey Sarah, so I'll need to review some of the RFC to see the full approach suggested, but part of our desire is to get fully rid of the translation that happens from the APIGateway portion of a proxycfg snapshot into the IngressGateway portion, which means that ideally none of our code would use any of the IngressGateway fields. That would allow us to drop all of the transformation logic found here:

func (c *configSnapshotAPIGateway) ToIngress(datacenter string) (configSnapshotIngressGateway, error) {
and in the corresponding discovery chain "synthesizing" code that exists in the gateway*.go files in this subpackage.

But it does mean that we'll need to come up with the direct HTTPRoute --> xDS route synthesis code (which may or may not also leak to listeners, and seemingly here into clusters as well).

Maybe we can set up some time next week to chat about this synchronously to get on the same page?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi Andrew, I would be happy to hop on a call with you sometime this week, but I just accidentally missed that IngressGateway field in my initial pass. Part of the trouble I'm having with this is how much of the preexisting Xds generation/helper codes seem to rely heavily on the preexisting IngressGateway struct, so I do think it'd be helpful to talk through how our architecture is set up and what we want this to look like in the end.

sarahalsmiller and others added 2 commits April 19, 2023 15:08
Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>
@sarahalsmiller sarahalsmiller force-pushed the NET-3673-endpointsFromSnapshotAPIGateway branch from e030b94 to e082ec4 Compare April 19, 2023 20:12
@sarahalsmiller sarahalsmiller marked this pull request as ready for review April 19, 2023 20:45
@sarahalsmiller sarahalsmiller changed the title WIP- API Gateway XDS Primitives, endpoints and clusters API Gateway XDS Primitives, endpoints and clusters Apr 19, 2023
@sarahalsmiller sarahalsmiller changed the base branch from main to NET-2063-Implementation-API-GW-Use-XDS-primitives-instead-of-Ingress-GW-primitives April 26, 2023 14:49
…ad-of-Ingress-GW-primitives' into NET-3673-endpointsFromSnapshotAPIGateway
@sarahalsmiller sarahalsmiller merged commit af41a67 into NET-2063-Implementation-API-GW-Use-XDS-primitives-instead-of-Ingress-GW-primitives Apr 26, 2023
@sarahalsmiller sarahalsmiller deleted the NET-3673-endpointsFromSnapshotAPIGateway branch April 26, 2023 15:17
sarahalsmiller added a commit that referenced this pull request May 22, 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>
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
backport/1.15 This release series is no longer active on CE. Use backport/ent/1.15. 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.

2 participants