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

kgateway-waypoint: initial implementation #10783

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

stevenctl
Copy link
Contributor

@stevenctl stevenctl commented Mar 7, 2025

Description

#10453

This PR adds support for a new GatewayClass kgateway-waypoint, which can be use in place of Istio's default Waypoint implementation.

There are some sample YAMLs included, but the basic flow is:

  1. Deploy a Gateway with this class
  2. To route traffic through the gateway, label either a Namespace or a Service with istio.io/use-waypoint=<gateway name (and optionally istio.io/use-waypoint-namespace=<ns the gw is in> for out-of-namespace waypoints)
  3. Attach policy like HTTPRoute by parenting either to the Gateway (standard) or the Service (GAMMA-style)

This PR only supports the most basic features of a Waypoint:

  • Capturing to-service traffic in a single cluster
  • Attaching HTTPRoute to the Service or Gateway

Potential follow ups include:

  • TCPRoute
  • AuthorizationPolicy
  • Multi-Cluster
  • ServiceEntry capture and backends

API changes

  • Add new GatewayClass named kgateway-waypoint
  • (Helm) Move service.kind out of gateway and onto the gatewayClass and waypointClass values.
  • (Helm) Allow adding labels/annotation to GatewayClass and the generated podTemplate for each gateway class

Code changes

Helm

  • Change values to allow per-class customizations
  • Make templates for GatewayClass and GatewayParameters support passing in values for each class type (standard kgateway and the kgateway-waypoint)

Core

  • Refactor initialization of Routes and Gateway indexes to happen within CommonCollections rather than in proxy_syncer.
  • Move some collections into CommonCollections
  • Make RouteIndex handle the parent group/kind so we can do service-targeted policy.
  • Remove isOurGw and ExtraGatewayClasses in favor of a statically defined Set of known classes. This can be expanded later to read from an env var, but we shouldn't be passing a closure around for this.
  • Start actually applying the NetworkFilter plugins on TCP filter chains.
  • Gateway Queries just hold a reference to CommonCollections so that plugins can reference GatewayQueries (the routes field on it's own won't be ready till after plugins are initialized).

Waypoint Specific

  • Add a waypoint plugin that provides a new translator
  • Add some additional queries for waypoints for waypoint attachment and service-attached routes
  • Add a sandwich plugin that can be added at the Listener level to enable PROXY protocol inbound, and restore the client identity authenticated by zTunnel (Waypoint Sandwiches)

CI changes

  • E2E added to cluster-two job

Docs changes

  • Added some samples
  • README for the samples

Context

#10453

Testing steps

  • Unit tests against translation in/out
  • E2E tests for simple traffic flows and basic policy
  • To test on your own, see the examples/waypoint directory.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@stevenctl stevenctl force-pushed the stevenctl/waypoint-basic branch 5 times, most recently from a349fb4 to 229f833 Compare March 10, 2025 23:22
@nfuden nfuden requested review from timflannagan and lgadban March 10, 2025 23:23
@stevenctl stevenctl force-pushed the stevenctl/waypoint-basic branch from 257a5cb to 77c095d Compare March 11, 2025 04:16
func (s Service) IsHeadless() bool {
switch o := s.Object.(type) {
case *corev1.Service:
return o.Spec.ClusterIP == "None"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return o.Spec.ClusterIP == "None"
return o.Spec.ClusterIP == corev1.ClusterIPNone

@yuval-k
Copy link
Contributor

yuval-k commented Mar 11, 2025

Remove isOurGw and ExtraGatewayClasses in favor of a statically defined Set of known classes. This can be expanded later to read from an env var, but we shouldn't be passing a closure around for this.

For context, our goal is to eventually use a the controller name on the gateway class to know if we own a gateway class (instead of the gateway class name); so with that in mind a closure seemed like an easier way to get there

@stevenctl
Copy link
Contributor Author

eventually use a the controller name on the gateway class to know if we own a gateway class

Sounds good. Assuming we supported other gateway classes, I'd imagine it would be:

  1. Helm install time adds a new class controlled by our controller
  2. We have a plugin that contributes a translator for that class

Do we really need to have a closure? At the very least, we could probably move where that closure starts getting passed down; it seemed like it was several layers deep but only used in the GatewayIndex. If the list of (or single) controller name is env-driven, it could just be a func in wellknown. If settings driven I guess a closure or the list could be passed around.

@stevenctl
Copy link
Contributor Author

so with that in mind a closure seemed like an easier way to get there

Should I revert this in the short-term?

@stevenctl stevenctl force-pushed the stevenctl/waypoint-basic branch from 7dc87fb to 1747e25 Compare March 11, 2025 18:28
@stevenctl
Copy link
Contributor Author

stevenctl commented Mar 11, 2025

@yuval-k you can check 1747e25 for the controller name thing, happy to revert and split it out if it's too much for this one PR.

@stevenctl stevenctl linked an issue Mar 11, 2025 that may be closed by this pull request
3 tasks
@stevenctl stevenctl requested a review from PetrMc March 11, 2025 20:38
Copy link
Member

@PetrMc PetrMc 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 a huge PR. LGTM

waypointClass:
enabled: true
name: "kgateway-waypoint"
description: "KGateway Waypoint Controller"
Copy link
Member

Choose a reason for hiding this comment

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

according to the docs site it seems that kgateway is always lower-case unless the sentence starting with it. Should we unify?

Copy link
Contributor

@yuval-k yuval-k left a comment

Choose a reason for hiding this comment

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

comments re-init code

Comment on lines +150 to +157
gwl := &apiv1.GatewayList{}
if err := cli.List(ctx, gwl); err != nil {
return nil
}

var reqs []reconcile.Request
for _, gw := range gwl.Items {
if string(gw.Spec.GatewayClassName) == gc.Name {
Copy link
Contributor

Choose a reason for hiding this comment

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

consider: index gw by gateway class name; though not sure it is a big deal in terms of perf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done; if we have a ton of Istio gateways/waypoints then it will matter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#10829

This and the related pieces are moved into this smaller PR.

@stevenctl
Copy link
Contributor Author

@PetrMc

this is a huge PR

You're right!

I've opened #10822 and #10829 to shrink it a bit. I also plan on peeling off the changes to InitCollections/InitPlugins on common collections

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Basic Waypoint Support
3 participants