EP-10625: Invalid Route Replacement #11227

Sodman · 2025-05-16T16:22:33Z

Description

Enhancement proposal to bring invalid route replacement to kgateway. This is a feature the 1.x project previously had. Although not natively supported by the k8s Gateway API spec currently, it has proven to be an invaluable feature for many gateway users over the years, and has prevented production outages and security incidents. This feature results in improved UX/QoL for gateway admins and route creators, as well as improved guarantees for route atomicity - important for multi-tenant or shared-gateway environments.

Design to address route replacement feature identified in #10625.

Change Type

/kind design

Changelog

NONE

Additional Notes

Copilot

Pull Request Overview

This pull request introduces a design proposal for implementing invalid route replacement to prevent misrouted requests and accidental exposure of secure routes.

Introduces the invalid route replacement feature with clear scenarios and motivations.
Details configuration options and implementation details for the control plane, controllers, monitoring, and telemetry.
Provides a comprehensive test plan and discusses alternatives and open questions.

design/10625.md

yuval-k · 2025-05-16T16:51:32Z

design/10625.md

+- Invalid response code - The HTTP code returned to the end user hitting an invalid route
+  - Default: 404
+  - TODO: Making this code configurable may break compliance with the current k8s gateway API Spec (See "K8s Gateway API Spec" section above)
+- Enable/Disable feature


i think the alternative is don't update envoy at all vs disable route replacement and send the bad config?

maybe remove disable, pending CS feedback

design/10625.md

yuval-k · 2025-05-16T16:53:02Z

design/10625.md

+
+### Monitoring & Telemetry
+
+#### Metrics


100% need metrics and we should make sure these are well documented in our production recommendations

yuval-k · 2025-05-16T16:55:49Z

design/10625.md

+
+#### Metrics
+When an invalid route is replaced, we should increment a metric (Gauge) so that this can be piped into a monitoring dashboard.
+TODO: Should we have CR-level labels on this metric, or does that result in problems with high cardinality? Would help identify problematic resources easily, if it's feasible.


good question re-label and cardinality; if we don't do it as labels, maybe we can do it in the admin page? and expose it via check command in our future CLI?

yuval-k · 2025-05-16T16:56:38Z

design/10625.md

+
+#### Logs
+When an invalid route is replaced, it should be logged.
+TODO: Confirm we're OK with this, could end up being verbose in larger environments. General problem with translation errors, particularly if we consider this a 'warning'.


if this is disabled, and we instead delay sending route updates to envoy, this should be Error

lgadban · 2025-05-16T16:55:55Z

design/10625.md

+When handling the translation logic for HTTPRoutes, any configuration which would make the route invalid - previously resulting in the route being ellided from the resulting configuration sent to Envoy - will instead result in a direct response route being configured, with the settings above.
+
+Examples of things that could result in the route being invalid:
+- Missing destination for route target


I think the details here will be extremely important.
A missing route target, meaning an invalid BackendRef I assume, has well-defined behavior in the upstream Gateway API spec.
Do we want the kgw route replacement feature to supersede behavior in the spec?

lgadban · 2025-05-16T16:58:27Z

design/10625.md

+
+## Background
+
+Currently, when a user adds an invalid route - for example, a route to an upstream which does not currently exist - we ellide that route from the configuration sent to Envoy. In many scenarios, this results in users making requests that would otherwise match that route, to instead receive a "No Healthy Upstream" message along with a HTTP 404.


I think we need to define and document the current behavior for the following questions:

what is an invalid route from a Gateway API perspective?

what is an invalid route from a kgw policy perspective?

are there any scenarios other than these 2?

An Invalid route is:

A route we know for sure Envoy would Nack.

A route, which may otherwise be valid, but which has an invalid policy attached

Which subset of total policies is still TBD. Any policy that's security-based or request-impacting.

For routes which are missing a backend:

We are not using the route replacement as outlined in this doc

We are leaning on Envoy's built-in handling of missing cluster(s).

a policy can return a warning or an error. if a route only has errors we update status but let it go through to envoy.
if a route has errors we handle as described below

a policy can return a warning or an error. if a route only has errors we update status but let it go through to envoy.

Expanded on the error handling for policies in the last couple of commits. Right now, any errors encountered during IR construction, e.g. referential errors from attachment issues or structural issues highlighted by xDS validation, are treated the same. We can explore adding sentinel errors that delineate between a warning and terminal errors, but that doesn't seem immediately necessary.

We are leaning on Envoy's built-in handling of missing cluster(s).

I'm less familiar with this mechanism. Any documentation?

We can explore adding sentinel errors that delineate between a warning and terminal errors, but that doesn't seem immediately necessary.

Agreed that it is not immediately necessary, but for any extensions built around the route replacement feature, this may become important.

We are leaning on Envoy's built-in handling of missing cluster(s).

I'm less familiar with this mechanism. Any documentation?

That is referring to the clusterNotFound functionality to satisfy upstream Gateway API standards on response codes etc:

kgateway/internal/kgateway/translator/irtranslator/route.go

Lines 428 to 433 in e4cc62a

action := outRoute.GetRoute()

if action == nil {

action = &envoy_config_route_v3.RouteAction{

ClusterNotFoundResponseCode: envoy_config_route_v3.RouteAction_INTERNAL_SERVER_ERROR,

}

}

Lastly, given that a backend not found results in this 500 and NOT route replacement, I believe the opening sentence is not correct --

Currently, when a user adds an invalid route - for example, a route to an upstream which does not currently exist - we ellide that route from the configuration sent to Envoy.

We do not elide the route, but rely on the aforementioned behavior to return a 500

yuval-k · 2025-05-16T20:05:34Z

design/10625.md

+
+### Configuration
+
+The following settings need to be user-configurable:


have each policy call envoy-validate during translation to IR

if envoy validate fails, we set a EnvoyValidateFailed condition on the status

we only call envoy mode validate if the condition is missing for the current generation

during route translation we call envoy validate only to the route parts (not the per filter config) and LRU cache it.

policies can return a special error of type Warning. if route errors are all warnings, it will not trigger route replacement, just status message.

if envoy validate fails, we set a EnvoyValidateFailed condition on the status

I'm not sure this is something we can do with our approach to policy status as we're using v2alpha1 GW API policy types internally. Introducing another reason would be inconsistent with how policy errors are handled in the route translator today.

Signed-off-by: timflannagan <[email protected]>

AI typo fixes Co-authored-by: Copilot <[email protected]> Signed-off-by: Shane O'Donnell <[email protected]>

Signed-off-by: timflannagan <[email protected]>

lgadban

Looking mostly solid to me.
My only real concern is with treating all errors as the same within the system.

lgadban · 2025-06-09T21:36:39Z

design/10625.md

+
+## Background
+
+Currently, when a user adds an invalid route - for example, a route to an upstream which does not currently exist - we ellide that route from the configuration sent to Envoy. In many scenarios, this results in users making requests that would otherwise match that route, to instead receive a "No Healthy Upstream" message along with a HTTP 404.


We can explore adding sentinel errors that delineate between a warning and terminal errors, but that doesn't seem immediately necessary.

Agreed that it is not immediately necessary, but for any extensions built around the route replacement feature, this may become important.

We are leaning on Envoy's built-in handling of missing cluster(s).

I'm less familiar with this mechanism. Any documentation?

That is referring to the clusterNotFound functionality to satisfy upstream Gateway API standards on response codes etc:

kgateway/internal/kgateway/translator/irtranslator/route.go

Lines 428 to 433 in e4cc62a

action := outRoute.GetRoute()

if action == nil {

action = &envoy_config_route_v3.RouteAction{

ClusterNotFoundResponseCode: envoy_config_route_v3.RouteAction_INTERNAL_SERVER_ERROR,

}

}

Lastly, given that a backend not found results in this 500 and NOT route replacement, I believe the opening sentence is not correct --

Currently, when a user adds an invalid route - for example, a route to an upstream which does not currently exist - we ellide that route from the configuration sent to Envoy.

We do not elide the route, but rely on the aforementioned behavior to return a 500

lgadban · 2025-06-09T21:39:40Z

design/10625.md

+### Goals
+
+- Implement invalid route replacement, for the following scenarios:
+  - Configured target destination does not exist


Regarding my earlier comment, is this actually a scenario we want route replacement to handle?
IIUC this scenario is explicitly handled already?

lgadban · 2025-06-09T21:46:34Z

design/10625.md

+
+- Replace any `nil` route with a direct-response route when the feature gate is enabled
+- Consolidate all error handling at the IR level; plugins no longer bubble errors via `ApplyForX` methods
+- Referential errors for policy, e.g. non-existent backing GatewayExtension provider, should be treated the same as structural errors that are highlighted by xDS validation


Mentioned above, but this means that "errors" that can be fixed via "eventual consistency" can not be delineated from other errors. This will become a challenge for extensions that rely on this behavior.

Signed-off-by: timflannagan <[email protected]>

Copilot AI review requested due to automatic review settings May 16, 2025 16:22

Copilot AI reviewed May 16, 2025

View reviewed changes

design/10625.md Outdated Show resolved Hide resolved

design/10625.md Outdated Show resolved Hide resolved

design/10625.md Outdated Show resolved Hide resolved

github-actions bot added kind/design Categorizes issue or PR as related to design. release-note-none labels May 16, 2025

Sodman marked this pull request as draft May 16, 2025 16:23

yuval-k reviewed May 16, 2025

View reviewed changes

design/10625.md Outdated Show resolved Hide resolved

yuval-k reviewed May 16, 2025

View reviewed changes

lgadban reviewed May 16, 2025

View reviewed changes

yuval-k reviewed May 16, 2025

View reviewed changes

timflannagan mentioned this pull request Jun 2, 2025

WIP: Invalid route replacement #11322

Open

Sodman and others added 5 commits June 7, 2025 22:32

Initial draft of Invalid Route Replacement EP

3396d22

Signed-off-by: timflannagan <[email protected]>

Apply suggestions from code review

a5ae0a8

AI typo fixes Co-authored-by: Copilot <[email protected]> Signed-off-by: Shane O'Donnell <[email protected]>

Minor formatting updates

e02c463

Signed-off-by: timflannagan <[email protected]>

Flesh out implementation details

ff86f89

Signed-off-by: timflannagan <[email protected]>

Minor self review

23fa224

Signed-off-by: timflannagan <[email protected]>

timflannagan force-pushed the ep-10625-invalid-route-replacement branch from 1026eec to ff86f89 Compare June 9, 2025 15:53

timflannagan changed the title ~~(WIP) EP-10625: Invalid Route Replacement~~ EP-10625: Invalid Route Replacement Jun 9, 2025

timflannagan marked this pull request as ready for review June 9, 2025 15:53

lgadban requested a review from DuncanDoyle June 9, 2025 18:53

lgadban reviewed Jun 9, 2025

View reviewed changes

timflannagan added 6 commits June 11, 2025 17:46

WIP: A little bit of everything

b0e447d

Signed-off-by: timflannagan <[email protected]>

WIP: Checkpoint

c7b0cc8

Signed-off-by: timflannagan <[email protected]>

WIP: Consolidate motivation section

59be21d

Signed-off-by: timflannagan <[email protected]>

WIP: Re-org sections

d53e33e

Signed-off-by: timflannagan <[email protected]>

Re-add missing alternatives section

8be75e1

Signed-off-by: timflannagan <[email protected]>

Add non-Envoy data plane as non-goal, minor fix, add open questions

2896046

Signed-off-by: timflannagan <[email protected]>

timflannagan mentioned this pull request Jun 13, 2025

Invalid route replacement #11404

Open


		## Background

		Currently, when a user adds an invalid route - for example, a route to an upstream which does not currently exist - we ellide that route from the configuration sent to Envoy. In many scenarios, this results in users making requests that would otherwise match that route, to instead receive a "No Healthy Upstream" message along with a HTTP 404.

	action := outRoute.GetRoute()
	if action == nil {
	action = &envoy_config_route_v3.RouteAction{
	ClusterNotFoundResponseCode: envoy_config_route_v3.RouteAction_INTERNAL_SERVER_ERROR,
	}
	}


		### Configuration

		The following settings need to be user-configurable:

EP-10625: Invalid Route Replacement #11227

Are you sure you want to change the base?

EP-10625: Invalid Route Replacement #11227

Conversation

Sodman commented May 16, 2025 • edited by timflannagan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Change Type

Changelog

Additional Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yuval-k May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgadban left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sodman commented May 16, 2025 •

edited by timflannagan

Loading

yuval-k May 16, 2025 •

edited

Loading