Skip to content

Commit

Permalink
internal/envoy: use RouteRegex in RouteMatch
Browse files Browse the repository at this point in the history
Updates #1351

internal/envoy offered two ways to construct a regex route matcher. The
first was from a *dag.Route via RouteMatch, the second was directly from
a string passed to RouteRegex. The latter was used by internal/e2e to
construct test fixtures, however the former didn't use RouteRegex, it
duplicated the RouteRegex logic. This makes refactoring this code to use
SafeRegex more problematic.

This PR cleans up RouteMatch to use RouteRegex directly. This isn't as
clean as I would like because of the way gRPC union types do no expose
an interface for the union value, this means we cannot pass from
RouteRegex or RoutePrefix a PathSpecifier value as the interface those
types implement is not exported -- thanks protoc, you're a pal.

Also, add a few tests for RouteRegex matching. These will be needed in
the next PR to switch to SafeRegex.

Lastly, note that the only way to create a regex dag.Route.PathCondition
is via the Ingress object whose value is actually specified to be a
ecmascript regex -- ffs.

Signed-off-by: Dave Cheney <dave@cheney.net>
  • Loading branch information
davecheney authored and jpeach committed Nov 13, 2019
1 parent b65947c commit 98510cb
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 11 deletions.
17 changes: 7 additions & 10 deletions internal/envoy/route.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,28 +41,25 @@ func Route(match *envoy_api_v2_route.RouteMatch, action *envoy_api_v2_route.Rout

// RouteMatch creates a *envoy_api_v2_route.RouteMatch for the supplied *dag.Route.
func RouteMatch(route *dag.Route) *envoy_api_v2_route.RouteMatch {
match := &envoy_api_v2_route.RouteMatch{
Headers: headerMatcher(route.HeaderConditions),
}
switch c := route.PathCondition.(type) {
case *dag.RegexCondition:
match.PathSpecifier = &envoy_api_v2_route.RouteMatch_Regex{
Regex: c.Regex,
}
return RouteRegex(c.Regex, route.HeaderConditions...)
case *dag.PrefixCondition:
match.PathSpecifier = &envoy_api_v2_route.RouteMatch_Prefix{
Prefix: c.Prefix,
return RoutePrefix(c.Prefix, route.HeaderConditions...)
default:
return &envoy_api_v2_route.RouteMatch{
Headers: headerMatcher(route.HeaderConditions),
}
}
return match
}

// RouteRegex returns a regex matcher.
func RouteRegex(regex string) *envoy_api_v2_route.RouteMatch {
func RouteRegex(regex string, headers ...dag.HeaderCondition) *envoy_api_v2_route.RouteMatch {
return &envoy_api_v2_route.RouteMatch{
PathSpecifier: &envoy_api_v2_route.RouteMatch_Regex{
Regex: regex,
},
Headers: headerMatcher(headers),
}
}

Expand Down
29 changes: 28 additions & 1 deletion internal/envoy/route_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -600,7 +600,7 @@ func TestUpgradeHTTPS(t *testing.T) {
assert.Equal(t, want, got)
}

func TestHeaderConditions(t *testing.T) {
func TestRouteMatch(t *testing.T) {
tests := map[string]struct {
route *dag.Route
want *envoy_api_v2_route.RouteMatch
Expand Down Expand Up @@ -662,6 +662,33 @@ func TestHeaderConditions(t *testing.T) {
}},
},
},
"path prefix": {
route: &dag.Route{
PathCondition: &dag.PrefixCondition{
Prefix: "/foo",
},
},
want: &envoy_api_v2_route.RouteMatch{
PathSpecifier: &envoy_api_v2_route.RouteMatch_Prefix{
Prefix: "/foo",
},
},
},
"path regex": {
route: &dag.Route{
PathCondition: &dag.RegexCondition{
Regex: "/v.1/*",
},
},
want: &envoy_api_v2_route.RouteMatch{
PathSpecifier: &envoy_api_v2_route.RouteMatch_Regex{
// note, unlike header conditions this is not a quoted regex because
// the value comes directly from the Ingress.Paths.Path value which
// is permitted to be a bare regex.
Regex: "/v.1/*",
},
},
},
}

for name, tc := range tests {
Expand Down

0 comments on commit 98510cb

Please sign in to comment.