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.

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

fix: gernerate a catch-all route for rules with no matches and no hostnames in HTTPRoute and GRPCRoute #4528

Merged
merged 2 commits into from
Aug 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,14 @@ Adding a new version? You'll need three changes:

## Unreleased


### Changes

- Generate a wildcard routes to match all `HTTP` or `GRPC` requests for rules
in `HTTPRoute` or `GRPCRoute` if there are no matches in the rule and no
hostnames in their parent objects.
[#4526](https://github.com/Kong/kubernetes-ingress-controller/pull/4528)

### Fixed

- Display Service ports on generated Kong services, instead of a static default
Expand Down
7 changes: 0 additions & 7 deletions internal/dataplane/parser/translate_grpcroute.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,13 +168,6 @@ func validateGRPCRoute(grpcRoute *gatewayv1alpha2.GRPCRoute) error {
if len(grpcRoute.Spec.Rules) == 0 {
return translators.ErrRouteValidationNoRules
}
// REVIEW: remove this to generate a "catch-all" route from rule with no matches and no hostnames in its parent GRPCRoute.
for _, rule := range grpcRoute.Spec.Rules {
if len(rule.Matches) == 0 {
return translators.ErrRouteValidationNoMatchRulesOrHostnamesSpecified
}
}
}

return nil
}
27 changes: 19 additions & 8 deletions internal/dataplane/parser/translate_grpcroute_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,17 @@ func TestIngressRulesFromGRPCRoutesUsingExpressionRoutes(t *testing.T) {
},
},
},
{
Service: kong.Service{
Name: kong.String("grpcroute.default.grpcroute-no-hostnames-no-matches._.0"),
},
Backends: []kongstate.ServiceBackend{
{
Name: "service0",
PortDef: kongstate.PortDef{Mode: kongstate.PortModeByNumber, Number: int32(80)},
},
},
},
},
expectedKongRoutes: map[string][]kongstate.Route{
"grpcroute.default.grpcroute-1._.0": {
Expand All @@ -369,6 +380,14 @@ func TestIngressRulesFromGRPCRoutesUsingExpressionRoutes(t *testing.T) {
},
},
},
"grpcroute.default.grpcroute-no-hostnames-no-matches._.0": {
{
Route: kong.Route{
Name: kong.String("grpcroute.default.grpcroute-no-hostnames-no-matches._.0.0"),
Expression: kong.String(translators.CatchAllHTTPExpression),
},
},
},
},
expectedFailures: []failures.ResourceFailure{
newResourceFailure(translators.ErrRouteValidationNoRules.Error(),
Expand All @@ -379,14 +398,6 @@ func TestIngressRulesFromGRPCRoutesUsingExpressionRoutes(t *testing.T) {
Name: "grpcroute-no-rules",
},
}),
newResourceFailure(translators.ErrRouteValidationNoMatchRulesOrHostnamesSpecified.Error(),
&gatewayv1alpha2.GRPCRoute{
TypeMeta: grpcRouteTypeMeta,
ObjectMeta: metav1.ObjectMeta{
Namespace: "default",
Name: "grpcroute-no-hostnames-no-matches",
},
}),
},
},
}
Expand Down
9 changes: 1 addition & 8 deletions internal/dataplane/parser/translate_httproute.go
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,7 @@ func generateKongRoutesFromHTTPRouteMatches(
// but only backends as long as there are hostnames. In this case, we
// match all traffic based on the hostname and leave all other routing
// options default.
// for rules with no hostnames, we generate a "catch-all" route for it.
r := kongstate.Route{
Ingress: ingressObjectInfo,
Route: kong.Route{
Expand All @@ -348,14 +349,6 @@ func generateKongRoutesFromHTTPRouteMatches(
Tags: tags,
},
}

// however in this case there must actually be some present hostnames
// configured for the HTTPRoute or else it's not valid.
if len(hostnames) == 0 {
return []kongstate.Route{}, translators.ErrRouteValidationNoMatchRulesOrHostnamesSpecified
}

// otherwise apply the hostnames to the route
r.Hosts = append(r.Hosts, hostnames...)

return []kongstate.Route{r}, nil
Expand Down
153 changes: 145 additions & 8 deletions internal/dataplane/parser/translate_httproute_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ func getIngressRulesFromHTTPRoutesCommonTestCases() []testCaseIngressRulesFromHT
},
},
{
msg: "an HTTPRoute rule with no matches and no hostnames can't be routed",
msg: "an HTTPRoute rule with no matches and no hostnames produces a catch-all rule",
routes: []*gatewayv1beta1.HTTPRoute{{
ObjectMeta: metav1.ObjectMeta{
Name: "basic-httproute",
Expand All @@ -141,14 +141,48 @@ func getIngressRulesFromHTTPRoutesCommonTestCases() []testCaseIngressRulesFromHT
}},
expected: func(routes []*gatewayv1beta1.HTTPRoute) ingressRules {
return ingressRules{
SecretNameToSNIs: newSecretNameToSNIs(),
ServiceNameToParent: map[string]client.Object{},
ServiceNameToServices: make(map[string]kongstate.Service),
SecretNameToSNIs: newSecretNameToSNIs(),
ServiceNameToParent: map[string]client.Object{
"httproute.default.basic-httproute.0": routes[0],
},
ServiceNameToServices: map[string]kongstate.Service{
"httproute.default.basic-httproute.0": {
Service: kong.Service{ // only 1 service should be created
ConnectTimeout: kong.Int(60000),
Host: kong.String("httproute.default.basic-httproute.0"),
Name: kong.String("httproute.default.basic-httproute.0"),
Protocol: kong.String("http"),
ReadTimeout: kong.Int(60000),
Retries: kong.Int(5),
WriteTimeout: kong.Int(60000),
},
Backends: kongstate.ServiceBackends{
builder.NewKongstateServiceBackend("fake-service").WithPortNumber(80).Build(),
},
Namespace: "default",
Routes: []kongstate.Route{{ // only 1 route should be created
Route: kong.Route{
Name: kong.String("httproute.default.basic-httproute.0.0"),
PreserveHost: kong.Bool(true),
Protocols: []*string{
kong.String("http"),
kong.String("https"),
},
Tags: []*string{
kong.String("k8s-name:basic-httproute"),
kong.String("k8s-namespace:default"),
kong.String("k8s-kind:HTTPRoute"),
kong.String("k8s-group:gateway.networking.k8s.io"),
kong.String("k8s-version:v1beta1"),
},
},
Ingress: k8sObjectInfoOfHTTPRoute(routes[0]),
}},
Parent: routes[0],
},
},
}
},
errs: []error{
translators.ErrRouteValidationNoMatchRulesOrHostnamesSpecified,
},
},
{
msg: "a single HTTPRoute with one match and one backendRef results in a single service",
Expand Down Expand Up @@ -2062,6 +2096,109 @@ func TestIngressRulesFromSplitHTTPRouteMatchWithPriority(t *testing.T) {
ExpressionRoutes: true,
},
},
{
name: "precise hostname and no match",
matchWithPriority: translators.SplitHTTPRouteMatchToKongRoutePriority{
Match: translators.SplitHTTPRouteMatch{
Source: &gatewayv1beta1.HTTPRoute{
TypeMeta: httpRouteTypeMeta,
ObjectMeta: metav1.ObjectMeta{
Namespace: "default",
Name: "httproute-1",
},
Spec: gatewayv1beta1.HTTPRouteSpec{
Hostnames: []gatewayv1beta1.Hostname{
"a.foo.com",
},
Rules: []gatewayv1beta1.HTTPRouteRule{
{
BackendRefs: []gatewayv1beta1.HTTPBackendRef{
builder.NewHTTPBackendRef("service1").WithPort(80).Build(),
},
},
},
},
},
Hostname: "a.foo.com",
Match: builder.NewHTTPRouteMatch().Build(),
RuleIndex: 0,
MatchIndex: 0,
},
Priority: 1024,
},
expectedKongService: kongstate.Service{
Service: kong.Service{
Name: kong.String("httproute.default.httproute-1.a.foo.com.0"),
},
Backends: []kongstate.ServiceBackend{
{
Name: "service1",
PortDef: kongstate.PortDef{Mode: kongstate.PortModeByNumber, Number: int32(80)},
},
},
},
expectedKongRoute: kongstate.Route{
Route: kong.Route{
Name: kong.String("httproute.default.httproute-1.a.foo.com.0.0"),
Expression: kong.String(`http.host == "a.foo.com"`),
PreserveHost: kong.Bool(true),
StripPath: kong.Bool(false),
Priority: kong.Int(1024),
},
Plugins: []kong.Plugin{},
ExpressionRoutes: true,
},
},
{
name: "no hostname and no match",
matchWithPriority: translators.SplitHTTPRouteMatchToKongRoutePriority{
Match: translators.SplitHTTPRouteMatch{
Source: &gatewayv1beta1.HTTPRoute{
TypeMeta: httpRouteTypeMeta,
ObjectMeta: metav1.ObjectMeta{
Namespace: "default",
Name: "httproute-1",
},
Spec: gatewayv1beta1.HTTPRouteSpec{
Rules: []gatewayv1beta1.HTTPRouteRule{
{
BackendRefs: []gatewayv1beta1.HTTPBackendRef{
builder.NewHTTPBackendRef("service1").WithPort(80).Build(),
},
},
},
},
},
Hostname: "",
Match: builder.NewHTTPRouteMatch().Build(),
RuleIndex: 0,
MatchIndex: 0,
},
Priority: 1024,
},
expectedKongService: kongstate.Service{
Service: kong.Service{
Name: kong.String("httproute.default.httproute-1._.0"),
},
Backends: []kongstate.ServiceBackend{
{
Name: "service1",
PortDef: kongstate.PortDef{Mode: kongstate.PortModeByNumber, Number: int32(80)},
},
},
},
expectedKongRoute: kongstate.Route{
Route: kong.Route{
Name: kong.String("httproute.default.httproute-1._.0.0"),
Expression: kong.String(translators.CatchAllHTTPExpression),
PreserveHost: kong.Bool(true),
StripPath: kong.Bool(false),
Priority: kong.Int(1024),
},
Plugins: []kong.Plugin{},
ExpressionRoutes: true,
},
},
}

for _, tc := range testCases {
Expand All @@ -2079,7 +2216,7 @@ func TestIngressRulesFromSplitHTTPRouteMatchWithPriority(t *testing.T) {
}
require.NoError(t, err)
kongService, ok := res.ServiceNameToServices[*tc.expectedKongService.Name]
require.True(t, ok)
require.Truef(t, ok, "should find service %s", *tc.expectedKongService.Name)
require.Equal(t, tc.expectedKongService.Backends, kongService.Backends)
require.Len(t, kongService.Routes, 1)
require.Equal(t, tc.expectedKongRoute, kongService.Routes[0])
Expand Down
10 changes: 10 additions & 0 deletions internal/dataplane/parser/translators/atc_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,16 @@ const (
ResourceKindBitsGRPCRoute = 1
)

const (
// CatchAllHTTPExpression is the expression to match all HTTP/HTTPS requests.
// For rules with no matches and no hostnames in its parent HTTPRoute or GRPCRoute,
// we need to generate a "catch-all" route for the rule:
// https://github.com/Kong/kubernetes-ingress-controller/issues/4526
// but Kong does not allow empty expression in expression router,
// so we define this is we need a route to match all HTTP/HTTPS requests.
CatchAllHTTPExpression = `(net.protocol == "http") || (net.protocol == "https")`
)

// -----------------------------------------------------------------------------
// Translator - common functions in translating expression(ATC) routes from multiple kinds of k8s objects.
// -----------------------------------------------------------------------------
Expand Down
4 changes: 4 additions & 0 deletions internal/dataplane/parser/translators/grpcroute_atc.go
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,10 @@ func KongExpressionRouteFromSplitGRPCRouteMatchWithPriority(
grpcRoute.Annotations,
)
atc.ApplyExpression(&r.Route, matcher, matchWithPriority.Priority)
if r.Expression == nil || len(*r.Expression) == 0 {
r.Expression = kong.String(CatchAllHTTPExpression)
r.Priority = &matchWithPriority.Priority
}

return r
}
Expand Down
19 changes: 17 additions & 2 deletions internal/dataplane/parser/translators/httproute_atc.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@ func GenerateKongExpressionRoutesFromHTTPRouteMatches(

if len(translation.Matches) == 0 {
if len(hostnames) == 0 {
return []kongstate.Route{}, ErrRouteValidationNoMatchRulesOrHostnamesSpecified
r.Expression = kong.String(CatchAllHTTPExpression)
return []kongstate.Route{r}, nil
}

hostMatcher := hostMatcherFromHosts(hostnames)
atc.ApplyExpression(&r.Route, hostMatcher, 1)
return []kongstate.Route{r}, nil
Expand Down Expand Up @@ -238,6 +238,15 @@ func SplitHTTPRoute(httproute *gatewayv1beta1.HTTPRoute) []SplitHTTPRouteMatch {
splitHTTPRouteByMatch := func(hostname string) []SplitHTTPRouteMatch {
ret := []SplitHTTPRouteMatch{}
for ruleIndex, rule := range httproute.Spec.Rules {
if len(rule.Matches) == 0 {
ret = append(ret, SplitHTTPRouteMatch{
Source: httproute,
Hostname: hostname,
Match: gatewayv1beta1.HTTPRouteMatch{},
RuleIndex: ruleIndex,
MatchIndex: 0,
})
}
for matchIndex, match := range rule.Matches {
ret = append(ret, SplitHTTPRouteMatch{
Source: httproute,
Expand Down Expand Up @@ -537,6 +546,12 @@ func KongExpressionRouteFromHTTPRouteMatchWithPriority(

atc.ApplyExpression(&r.Route, atc.And(matchers...), httpRouteMatchWithPriority.Priority)

// generate a "catch-all" route if the generated expression is empty.
if r.Expression == nil || len(*r.Expression) == 0 {
r.Expression = kong.String(CatchAllHTTPExpression)
r.Priority = kong.Int(httpRouteMatchWithPriority.Priority)
}

// translate filters in the rule.
if match.RuleIndex < len(httproute.Spec.Rules) {
rule := httproute.Spec.Rules[match.RuleIndex]
Expand Down
13 changes: 11 additions & 2 deletions internal/dataplane/parser/translators/httproute_atc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,17 @@ func TestGenerateKongExpressionRoutesFromHTTPRouteMatches(t *testing.T) {
name: "no hostnames and no matches",
routeName: "empty_route.default.0.0",
ingressObjectInfo: util.K8sObjectInfo{},
expectedRoutes: []kongstate.Route{},
expectedError: ErrRouteValidationNoMatchRulesOrHostnamesSpecified,
expectedRoutes: []kongstate.Route{
{
Route: kong.Route{
Name: kong.String("empty_route.default.0.0"),
PreserveHost: kong.Bool(true),
StripPath: kong.Bool(false),
Expression: kong.String(CatchAllHTTPExpression),
},
ExpressionRoutes: true,
},
},
},
{
name: "no matches but have hostnames",
Expand Down