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.

Sign up for GitHub

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

Jump to bottom

Enable HTTPRouteRewritePath test #2112

Merged
merged 9 commits into from
Nov 1, 2023
Merged

Enable HTTPRouteRewritePath test #2112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a0c286d
Enable HTTPRouteRewritePath test
arkodg Oct 29, 2023
2e01a87
fix prefix match
arkodg Oct 30, 2023
e6e7a9e
make testdata
arkodg Oct 30, 2023
713120a
fix path match
arkodg Oct 30, 2023
71c21a3
rm trailing /
arkodg Oct 30, 2023
9acd678
sort on path match type
arkodg Oct 30, 2023
177bdc6
make testdata
arkodg Oct 30, 2023
1c26b15
temp var
arkodg Oct 30, 2023
aaa93e4
fix url rewrite
arkodg Oct 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/cmd/egctl/testdata/translate/out/multiple-xds.route.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
"routes": [
{
"match": {
"prefix": "/v2/"
"pathSeparatedPrefix": "/v2"
},
"name": "httproute/default/backend/rule/0/match/0/www_example2_com",
"route": {
Expand Down
41 changes: 38 additions & 3 deletions internal/gatewayapi/sort.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,42 @@
func (x XdsIRRoutes) Len() int { return len(x) }
func (x XdsIRRoutes) Swap(i, j int) { x[i], x[j] = x[j], x[i] }
func (x XdsIRRoutes) Less(i, j int) bool {
// 1. Sort based on characters in a matching path.

// 1. Sort based on path match type
// Exact > PathPrefix > RegularExpression
if x[i].PathMatch != nil && x[i].PathMatch.Exact != nil {
if x[j].PathMatch != nil {
if x[j].PathMatch.Prefix != nil {
return false
}
if x[j].PathMatch.SafeRegex != nil {
return false
}
}
}
if x[i].PathMatch != nil && x[i].PathMatch.Prefix != nil {
if x[j].PathMatch != nil {
if x[j].PathMatch.Exact != nil {
return true
}

Check warning on line 36 in internal/gatewayapi/sort.go

View check run for this annotation

Codecov / codecov/patch

internal/gatewayapi/sort.go#L35-L36 

Added lines #L35 - L36 were not covered by tests
if x[j].PathMatch.SafeRegex != nil {
return false
}
}
}
if x[i].PathMatch != nil && x[i].PathMatch.SafeRegex != nil {
if x[j].PathMatch != nil {
if x[j].PathMatch.Exact != nil {
return true
}
if x[j].PathMatch.Prefix != nil {
return true
}

Check warning on line 49 in internal/gatewayapi/sort.go

View check run for this annotation

Codecov / codecov/patch

internal/gatewayapi/sort.go#L43-L49 

Added lines #L43 - L49 were not covered by tests
}
}
// Equal case

// 2. Sort based on characters in a matching path.
pCountI := pathMatchCount(x[i].PathMatch)
pCountJ := pathMatchCount(x[j].PathMatch)
if pCountI < pCountJ {
Expand All @@ -27,7 +62,7 @@
}
// Equal case

// 2. Sort based on the number of Header matches.
// 3. Sort based on the number of Header matches.
hCountI := len(x[i].HeaderMatches)
hCountJ := len(x[j].HeaderMatches)
if hCountI < hCountJ {
Expand All @@ -38,7 +73,7 @@
}
// Equal case

// 3. Sort based on the number of Query param matches.
// 4. Sort based on the number of Query param matches.
qCountI := len(x[i].QueryParamMatches)
qCountJ := len(x[j].QueryParamMatches)
return qCountI < qCountJ
Expand Down
8 changes: 4 additions & 4 deletions internal/gatewayapi/testdata/grpcroute-with-service-match.out.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,11 +119,11 @@ xdsIR:
port: 8080
weight: 1
hostname: '*'
name: grpcroute/default/grpcroute-1/rule/0/match/1/*
name: grpcroute/default/grpcroute-1/rule/0/match/0/*
pathMatch:
distinct: false
name: ""
safeRegex: /com.[A-Z]+/[A-Za-z_][A-Za-z_0-9]*
prefix: /com.ExampleExact
- backendWeights:
invalid: 0
valid: 0
Expand All @@ -135,8 +135,8 @@ xdsIR:
port: 8080
weight: 1
hostname: '*'
name: grpcroute/default/grpcroute-1/rule/0/match/0/*
name: grpcroute/default/grpcroute-1/rule/0/match/1/*
pathMatch:
distinct: false
name: ""
prefix: /com.ExampleExact
safeRegex: /com.[A-Z]+/[A-Za-z_][A-Za-z_0-9]*
20 changes: 10 additions & 10 deletions internal/gatewayapi/testdata/httproute-with-single-rule-with-multiple-rules.out.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,26 +137,26 @@ xdsIR:
invalid: 0
valid: 0
destination:
name: httproute/default/httproute-1/rule/2
name: httproute/default/httproute-1/rule/0
settings:
- endpoints:
- host: 7.7.7.7
port: 8080
weight: 1
headerMatches:
- distinct: false
exact: exact
name: Header-1
safeRegex: '*regex*'
hostname: '*'
name: httproute/default/httproute-1/rule/2/match/0/*
name: httproute/default/httproute-1/rule/0/match/0/*
pathMatch:
distinct: false
exact: /exact
name: ""
safeRegex: '*regex*'
queryParamMatches:
- distinct: false
exact: exact
name: QueryParam-1
safeRegex: '*regex*'
- backendWeights:
invalid: 0
valid: 0
Expand All @@ -177,23 +177,23 @@ xdsIR:
invalid: 0
valid: 0
destination:
name: httproute/default/httproute-1/rule/0
name: httproute/default/httproute-1/rule/2
settings:
- endpoints:
- host: 7.7.7.7
port: 8080
weight: 1
headerMatches:
- distinct: false
exact: exact
name: Header-1
safeRegex: '*regex*'
hostname: '*'
name: httproute/default/httproute-1/rule/0/match/0/*
name: httproute/default/httproute-1/rule/2/match/0/*
pathMatch:
distinct: false
exact: /exact
name: ""
safeRegex: '*regex*'
queryParamMatches:
- distinct: false
exact: exact
name: QueryParam-1
safeRegex: '*regex*'
29 changes: 22 additions & 7 deletions internal/xds/translator/route.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func buildXdsRoute(httpRoute *ir.HTTPRoute) *routev3.Route {
case httpRoute.Redirect != nil:
router.Action = &routev3.Route_Redirect{Redirect: buildXdsRedirectAction(httpRoute.Redirect)}
case httpRoute.URLRewrite != nil:
routeAction := buildXdsURLRewriteAction(httpRoute.Destination.Name, httpRoute.URLRewrite)
routeAction := buildXdsURLRewriteAction(httpRoute.Destination.Name, httpRoute.URLRewrite, httpRoute.PathMatch)
if httpRoute.Mirrors != nil {
routeAction.RequestMirrorPolicies = buildXdsRequestMirrorPolicies(httpRoute.Mirrors)
}
Expand Down Expand Up @@ -101,14 +101,15 @@ func buildXdsRouteMatch(pathMatch *ir.StringMatch, headerMatches []*ir.StringMat
Path: *pathMatch.Exact,
}
} else if pathMatch.Prefix != nil {
// when the prefix ends with "/", use RouteMatch_Prefix
if strings.HasSuffix(*pathMatch.Prefix, "/") {
if *pathMatch.Prefix == "/" {
Copy link
Member

@qicz qicz Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

IMO, HasSuffix can cover Prefix = '/' and suffix with /. if we remove the trailing / can not cover the use case that suffix with / like /example/abc/. /example/abc and /example/abc/ are different case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

according to the gateway API a trailing slash in prefix can be ignored, ptal at
https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io%2fv1.PathMatchType

Matches based on a URL path prefix split by /. Matching is case sensitive and done on a path element by element basis. A path element refers to the list of labels in the path split by the / separator. When specified, a trailing / is ignored.

For example, the paths /abc, /abc/, and /abc/def would all match the prefix /abc, but the path /abcd would not.

outMatch.PathSpecifier = &routev3.RouteMatch_Prefix{
Prefix: *pathMatch.Prefix,
Prefix: "/",
}
} else {
// Remove trailing /
trimmedPrefix := strings.TrimSuffix(*pathMatch.Prefix, "/")
outMatch.PathSpecifier = &routev3.RouteMatch_PathSeparatedPrefix{
PathSeparatedPrefix: *pathMatch.Prefix,
Copy link
Member

@qicz qicz Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

if the path prefix is not a suffix with /, the PathSpecifier use the PathSeparatedPrefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this related to your above comment, if so ptal at #2112 (comment)

PathSeparatedPrefix: trimmedPrefix,
}
}
} else if pathMatch.SafeRegex != nil {
Expand Down Expand Up @@ -252,7 +253,7 @@ func buildXdsRedirectAction(redirection *ir.Redirect) *routev3.RedirectAction {
return routeAction
}

func buildXdsURLRewriteAction(destName string, urlRewrite *ir.URLRewrite) *routev3.RouteAction {
func buildXdsURLRewriteAction(destName string, urlRewrite *ir.URLRewrite, pathMatch *ir.StringMatch) *routev3.RouteAction {
routeAction := &routev3.RouteAction{
ClusterSpecifier: &routev3.RouteAction_Cluster{
Cluster: destName,
Expand All @@ -268,7 +269,21 @@ func buildXdsURLRewriteAction(destName string, urlRewrite *ir.URLRewrite) *route
Substitution: *urlRewrite.Path.FullReplace,
}
} else if urlRewrite.Path.PrefixMatchReplace != nil {
routeAction.PrefixRewrite = *urlRewrite.Path.PrefixMatchReplace
// Circumvent the case of "//" when the replace string is "/"
// An empty replace string does not seem to solve the issue so we are using
// a regex match and replace instead
// Remove this workaround once https://github.com/envoyproxy/envoy/issues/26055 is fixed
if pathMatch != nil && pathMatch.Prefix != nil &&
(*urlRewrite.Path.PrefixMatchReplace == "" || *urlRewrite.Path.PrefixMatchReplace == "/") {
routeAction.RegexRewrite = &matcherv3.RegexMatchAndSubstitute{
Pattern: &matcherv3.RegexMatcher{
Regex: "^" + *pathMatch.Prefix + `\/*`,
},
Substitution: "/",
}
} else {
routeAction.PrefixRewrite = *urlRewrite.Path.PrefixMatchReplace
}
}
}

Expand Down
7 changes: 5 additions & 2 deletions ...al/xds/translator/testdata/out/xds-ir/http-route-rewrite-root-path-url-prefix.routes.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,11 @@
- name: :authority
stringMatch:
exact: gateway.envoyproxy.io
prefix: /origin/
pathSeparatedPrefix: /origin
name: rewrite-route
route:
cluster: rewrite-route-dest
prefixRewrite: /
regexRewrite:
pattern:
regex: ^/origin/\/*
substitution: /
1 change: 0 additions & 1 deletion test/conformance/conformance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ func TestGatewayAPIConformance(t *testing.T) {
CleanupBaseResources: *flags.CleanupBaseResources,
SupportedFeatures: suite.AllFeatures,
SkipTests: []string{
tests.HTTPRouteRewritePath.ShortName,
tests.GatewayStaticAddresses.ShortName,
tests.GatewayWithAttachedRoutes.ShortName,
tests.HTTPRouteBackendProtocolH2C.ShortName,
Expand Down
1 change: 0 additions & 1 deletion test/conformance/experimental_conformance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@ func experimentalConformance(t *testing.T) {
CleanupBaseResources: *flags.CleanupBaseResources,
SupportedFeatures: suite.AllFeatures,
SkipTests: []string{
tests.HTTPRouteRewritePath.ShortName,
tests.GatewayStaticAddresses.ShortName,
tests.GatewayWithAttachedRoutes.ShortName,
tests.HTTPRouteBackendProtocolH2C.ShortName,
Expand Down