feat: support BackendRef HTTP filters #3246

cnvergence · 2024-04-22T17:26:17Z

What type of PR is this?

What this PR does / why we need it:

Support HTTP Filters in the BackendRef and translate them to WeightedCluster in the XDS.
Currently, only support for RequestHeaderModifiier and ResponseHeaderModifier type, as it is the only filter with
straightforward support in
https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#config-route-v3-weightedcluster-clusterweight
Validate other types of filters

Which issue(s) this PR fixes:

Fixes #2572 #3338

zirain · 2024-05-07T23:06:46Z

this may conflict with #3212, should decide which go first.
cc @envoyproxy/gateway-maintainers

cnvergence · 2024-05-09T09:08:24Z

/retest

codecov · 2024-05-10T09:30:09Z

Codecov Report

Attention: Patch coverage is 78.88889% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 67.33%. Comparing base (8ca2675) to head (cf2bbcf).

Files	Patch %	Lines
internal/gatewayapi/route.go	70.58%	6 Missing and 4 partials ⚠️
internal/xds/translator/route.go	85.36%	3 Missing and 3 partials ⚠️
internal/gatewayapi/validate.go	72.72%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3246   +/-   ##
=======================================
  Coverage   67.32%   67.33%           
=======================================
  Files         166      166           
  Lines       19211    19275   +64     
=======================================
+ Hits        12934    12979   +45     
- Misses       5347     5358   +11     
- Partials      930      938    +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cnvergence · 2024-05-15T08:34:50Z

/retest

cnvergence · 2024-05-15T09:06:18Z

@zirain rebased the PR, as your changes went first 😄

internal/gatewayapi/route.go

internal/xds/translator/route.go

test/conformance/conformance_test.go

arkodg · 2024-05-16T02:49:37Z

hey @cnvergence small nits, overall LGTM, thanks for adding this feature !
you will also want to undo parts of #2620 in this PR, or even modify that code to add the status if any of the filters are the ones from below

Path rewrites
Redirect
Mirroring

cnvergence · 2024-05-19T06:26:18Z

/retest

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

cnvergence · 2024-05-20T09:04:04Z

/retest

arkodg

LGTM thanks !

zhaohuabing

This PR looks great! Just a few non-blocking nits, which can be addressed in a follow-up PR.

zhaohuabing · 2024-05-20T17:51:25Z

internal/gatewayapi/route.go

@@ -1225,17 +1225,71 @@ func (t *Translator) processDestination(backendRefContext BackendRefContext,
 			gwapiv1a2.RouteReasonResolvedRefs,
 			"Mixed endpointslice address type for the same backendRef is not supported")
 	}
-
+	destinationFilters := t.processDestinationFilters(routeType, backendRefContext, parentRef, route, resources)


Could we remove route parameter here? Passing an unused parameter into this function could be confusing.

Suggested change

destinationFilters := t.processDestinationFilters(routeType, backendRefContext, parentRef, route, resources)

destinationFilters := t.processDestinationFilters(routeType, backendRefContext, parentRef, resources)

zhaohuabing · 2024-05-20T17:55:29Z

internal/gatewayapi/route.go

+
+	switch filters := backendFilters.(type) {
+	case []gwapiv1.HTTPRouteFilter:
+		httpFiltersContext = t.ProcessHTTPFilters(parentRef, route, filters, 0, resources)


This is probably not an ideal solution, but it makes it clear that route is not needed in this case.

Suggested change

httpFiltersContext = t.ProcessHTTPFilters(parentRef, route, filters, 0, resources)

httpFiltersContext = t.ProcessHTTPFilters(parentRef, nil, filters, 0, resources)

zhaohuabing · 2024-05-20T17:56:05Z

internal/gatewayapi/route.go

+		httpFiltersContext = t.ProcessHTTPFilters(parentRef, route, filters, 0, resources)
+
+	case []gwapiv1.GRPCRouteFilter:
+		httpFiltersContext = t.ProcessGRPCFilters(parentRef, route, filters, resources)


Suggested change

httpFiltersContext = t.ProcessGRPCFilters(parentRef, route, filters, resources)

httpFiltersContext = t.ProcessGRPCFilters(parentRef, nil, filters, resources)

zhaohuabing · 2024-05-20T18:15:03Z

internal/xds/translator/route.go

@@ -222,41 +222,70 @@ func buildXdsStringMatcher(irMatch *ir.StringMatch) *matcherv3.StringMatcher {
 	return stringMatcher
 }

-func buildXdsRouteAction(backendWeights *ir.BackendWeights) *routev3.RouteAction {
+func buildXdsRouteAction(backendWeights *ir.BackendWeights, settings []*ir.DestinationSetting) *routev3.RouteAction {
 	// only use weighted cluster when there are invalid weights


Nit: this comment should be updated. Adding a few lines to explain the reason would help people who come later.

cnvergence force-pushed the backendref-filters branch 4 times, most recently from 274ac15 to 1188ea7 Compare April 22, 2024 18:40

cnvergence force-pushed the backendref-filters branch 2 times, most recently from 53163a5 to 15f536d Compare May 3, 2024 12:39

cnvergence self-assigned this May 6, 2024

cnvergence force-pushed the backendref-filters branch from 8618b87 to 632b748 Compare May 7, 2024 11:53

cnvergence force-pushed the backendref-filters branch 6 times, most recently from 2285eb6 to 458ced2 Compare May 9, 2024 08:29

cnvergence marked this pull request as ready for review May 9, 2024 08:32

cnvergence requested a review from a team as a code owner May 9, 2024 08:32

cnvergence force-pushed the backendref-filters branch from 5ea22af to 11bf097 Compare May 15, 2024 08:13

shawnh2 reviewed May 15, 2024

View reviewed changes

internal/gatewayapi/route.go Show resolved Hide resolved