Skip to content

fix: httproute matching #121

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

Merged
merged 2 commits into from
May 6, 2025
Merged

fix: httproute matching #121

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c0b7a5
fix: httproute matching
ronething May 6, 2025
215991f
fix: r
ronething May 6, 2025
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
32 changes: 27 additions & 5 deletions internal/provider/adc/translator/httproute.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,14 @@ func (t *Translator) translateGatewayHTTPRouteMatch(match *gatewayv1.HTTPRouteMa
case gatewayv1.PathMatchExact:
route.Uris = []string{*match.Path.Value}
case gatewayv1.PathMatchPathPrefix:
route.Uris = []string{*match.Path.Value + "*"}
pathValue := *match.Path.Value
route.Uris = []string{pathValue}

Copy link

Copilot AI May 6, 2025

Choose a reason for hiding this comment

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

The new logic separates the original path and its wildcard extension into two URIs. Please add inline documentation to clarify the intent and ensure that this change aligns with the expected backend matching behavior.

Suggested change
// Append wildcard extensions to ensure the route matches both the base path
// and any subpaths. For example, a path "/example" will match "/example"
// and "/example/*". This behavior aligns with the backend's expectation
// for prefix-based path matching.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI May 6, 2025

Choose a reason for hiding this comment

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

[nitpick] The logic for constructing route URIs based on whether the path ends with a slash is not immediately obvious; consider adding an inline comment to clarify why a different wildcard is appended depending on the trailing slash.

Suggested change
// If the path ends with a slash, append "*" to match all subpaths.
// Otherwise, append "/*" to match the path itself and all its subpaths.

Copilot uses AI. Check for mistakes.
if strings.HasSuffix(pathValue, "/") {
route.Uris = append(route.Uris, pathValue+"*")
} else {
route.Uris = append(route.Uris, pathValue+"/*")
}
case gatewayv1.PathMatchRegularExpression:
var this []adctypes.StringOrSlice
this = append(this, adctypes.StringOrSlice{
Expand All @@ -439,6 +446,11 @@ func (t *Translator) translateGatewayHTTPRouteMatch(match *gatewayv1.HTTPRouteMa
default:
return nil, errors.New("unknown path match type " + string(*match.Path.Type))
}
} else {
/* If no matches are specified, the default is a prefix
path match on "/", which has the effect of matching every
HTTP request. */
route.Uris = []string{"/", "/*"}
}

if len(match.Headers) > 0 {
Expand All @@ -451,7 +463,12 @@ func (t *Translator) translateGatewayHTTPRouteMatch(match *gatewayv1.HTTPRouteMa
StrVal: "http_" + name,
})

Copy link

Copilot AI May 6, 2025

Choose a reason for hiding this comment

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

[nitpick] The default assignment of the header match type improves clarity, but adding a brief comment explaining this default value could help future maintainers understand the decision process.

Suggested change
// Default to HeaderMatchExact if no specific header match type is provided.

Copilot uses AI. Check for mistakes.
switch *header.Type {
matchType := gatewayv1.HeaderMatchExact
if header.Type != nil {
matchType = *header.Type
}

switch matchType {
case gatewayv1.HeaderMatchExact:
this = append(this, adctypes.StringOrSlice{
StrVal: "==",
Expand All @@ -461,7 +478,7 @@ func (t *Translator) translateGatewayHTTPRouteMatch(match *gatewayv1.HTTPRouteMa
StrVal: "~~",
})
default:
return nil, errors.New("unknown header match type " + string(*header.Type))
return nil, errors.New("unknown header match type " + string(matchType))
}

this = append(this, adctypes.StringOrSlice{
Expand All @@ -479,7 +496,12 @@ func (t *Translator) translateGatewayHTTPRouteMatch(match *gatewayv1.HTTPRouteMa
StrVal: "arg_" + strings.ToLower(fmt.Sprintf("%v", query.Name)),
})

switch *query.Type {
queryType := gatewayv1.QueryParamMatchExact
if query.Type != nil {
queryType = *query.Type
}

switch queryType {
case gatewayv1.QueryParamMatchExact:
this = append(this, adctypes.StringOrSlice{
StrVal: "==",
Expand All @@ -489,7 +511,7 @@ func (t *Translator) translateGatewayHTTPRouteMatch(match *gatewayv1.HTTPRouteMa
StrVal: "~~",
})
default:
return nil, errors.New("unknown query match type " + string(*query.Type))
return nil, errors.New("unknown query match type " + string(queryType))
}

this = append(this, adctypes.StringOrSlice{
Expand Down
2 changes: 1 addition & 1 deletion test/conformance/conformance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ var skippedTestsForTraditionalRoutes = []string{
tests.HTTPRouteHostnameIntersection.ShortName,
tests.HTTPRouteListenerHostnameMatching.ShortName,

tests.HTTPRouteMatching.ShortName,
// tests.HTTPRouteMatching.ShortName,
tests.HTTPRouteMatchingAcrossRoutes.ShortName,

tests.GatewayInvalidTLSConfiguration.ShortName,
Expand Down
Loading