dataclients/kubernetes: allow disabling catchall routes (#2656)

Catchall routes are created for Ingresses and RouteGroups that
do not define predicates matching all requests for the host.

Initially catchall routes were introduced by #425 for #408.
Later it was discovered that multiple Ingresses for the same host
may create conflicting catchall routes which was fixed by #436.

Addition of RouteGroups #1180 introduced catchall routes for them as well
but did not take into account that and Ingress and RouteGroup
may use the same host which results in a catchall route created
e.g. for RouetGroup that interferes with routes created for Ingress.

The plan is to remove catchall route logic because it simply shunts the request
and responds with the default response status which matches the behaviour
when Skipper can not look up route at all.

This change introduces a flag to disable creation of catchall routes.

Updates #408
Updates #2394

Signed-off-by: Alexander Yastrebov <[email protected]>

config/config.go

@@ -155,6 +155,7 @@ type Config struct {
 	KubernetesHealthcheck                   bool                               `yaml:"kubernetes-healthcheck"`
 	KubernetesHTTPSRedirect                 bool                               `yaml:"kubernetes-https-redirect"`
 	KubernetesHTTPSRedirectCode             int                                `yaml:"kubernetes-https-redirect-code"`
+	KubernetesDisableCatchAllRoutes         bool                               `yaml:"kubernetes-disable-catchall-routes"`
 	KubernetesIngressClass                  string                             `yaml:"kubernetes-ingress-class"`
 	KubernetesRouteGroupClass               string                             `yaml:"kubernetes-routegroup-class"`
 	WhitelistedHealthCheckCIDR              string                             `yaml:"whitelisted-healthcheck-cidr"`
@@ -444,6 +445,7 @@ func NewConfig() *Config {
 	flag.BoolVar(&cfg.KubernetesHealthcheck, "kubernetes-healthcheck", true, "automatic healthcheck route for internal IPs with path /kube-system/healthz; valid only with kubernetes")
 	flag.BoolVar(&cfg.KubernetesHTTPSRedirect, "kubernetes-https-redirect", true, "automatic HTTP->HTTPS redirect route; valid only with kubernetes")
 	flag.IntVar(&cfg.KubernetesHTTPSRedirectCode, "kubernetes-https-redirect-code", 308, "overrides the default redirect code (308) when used together with -kubernetes-https-redirect")
+	flag.BoolVar(&cfg.KubernetesDisableCatchAllRoutes, "kubernetes-disable-catchall-routes", false, "disables creation of catchall routes")
 	flag.StringVar(&cfg.KubernetesIngressClass, "kubernetes-ingress-class", "", "ingress class regular expression used to filter ingress resources for kubernetes")
 	flag.StringVar(&cfg.KubernetesRouteGroupClass, "kubernetes-routegroup-class", "", "route group class regular expression used to filter route group resources for kubernetes")
 	flag.StringVar(&cfg.WhitelistedHealthCheckCIDR, "whitelisted-healthcheck-cidr", "", "sets the iprange/CIDRS to be whitelisted during healthcheck")
@@ -787,6 +789,7 @@ func (c *Config) ToOptions() skipper.Options {
 		KubernetesHealthcheck:                  c.KubernetesHealthcheck,
 		KubernetesHTTPSRedirect:                c.KubernetesHTTPSRedirect,
 		KubernetesHTTPSRedirectCode:            c.KubernetesHTTPSRedirectCode,
+		KubernetesDisableCatchAllRoutes:        c.KubernetesDisableCatchAllRoutes,
 		KubernetesIngressClass:                 c.KubernetesIngressClass,
 		KubernetesRouteGroupClass:              c.KubernetesRouteGroupClass,
 		WhitelistedHealthCheckCIDR:             whitelistCIDRS,

dataclients/kubernetes/definitions/routegroups.go

@@ -45,8 +45,7 @@ type RouteGroupItem struct {
 
 type RouteGroupSpec struct {
 	// Hosts specifies the host headers, that will be matched for
-	// all routes created by this route group. No hosts mean
-	// catchall.
+	// all routes created by this route group.
 	Hosts []string `json:"hosts,omitempty"`
 
 	// Backends specify the list of backends that can be
@@ -60,8 +59,7 @@ type RouteGroupSpec struct {
 	DefaultBackends BackendReferences `json:"defaultBackends,omitempty"`
 
 	// Routes specifies the list of route based on path, method
-	// and predicates. It defaults to catchall, if there are no
-	// routes.
+	// and predicates.
 	Routes []*RouteSpec `json:"routes,omitempty"`
 }
 

dataclients/kubernetes/ingress.go

@@ -54,6 +54,7 @@ type ingress struct {
 	httpsRedirectCode            int
 	kubernetesEnableEastWest     bool
 	provideHTTPSRedirect         bool
+	disableCatchAllRoutes        bool
 	forceKubernetesService       bool
 	backendTrafficAlgorithm      BackendTrafficAlgorithm
 	defaultLoadBalancerAlgorithm string
@@ -71,6 +72,7 @@ func newIngress(o Options) *ingress {
 	return &ingress{
 		provideHTTPSRedirect:         o.ProvideHTTPSRedirect,
 		httpsRedirectCode:            o.HTTPSRedirectCode,
+		disableCatchAllRoutes:        o.DisableCatchAllRoutes,
 		pathMode:                     o.PathMode,
 		kubernetesEnableEastWest:     o.KubernetesEnableEastWest,
 		kubernetesEastWestDomain:     o.KubernetesEastWestDomain,
@@ -388,10 +390,12 @@ func (ing *ingress) convert(state *clusterState, df defaultFilters, r *certregis
 		applyEastWestRange(ing.eastWestRangeDomains, ing.eastWestRangePredicates, host, rs)
 		routes = append(routes, rs...)
 
-		// if routes were configured, but there is no catchall route
-		// defined for the host name, create a route which returns 404
-		if !hasCatchAllRoutes(rs) {
-			routes = append(routes, ing.addCatchAllRoutes(host, rs[0], redirect)...)
+		if !ing.disableCatchAllRoutes {
+			// if routes were configured, but there is no catchall route
+			// defined for the host name, create a route which returns 404
+			if !hasCatchAllRoutes(rs) {
+				routes = append(routes, ing.addCatchAllRoutes(host, rs[0], redirect)...)
+			}
 		}
 	}
 

dataclients/kubernetes/kube.go

@@ -126,6 +126,9 @@ type Options struct {
 	// By default, 308 StatusPermanentRedirect is used.
 	HTTPSRedirectCode int
 
+	// DisableCatchAllRoutes, when set, tells the data client to not create catchall routes.
+	DisableCatchAllRoutes bool
+
 	// IngressClass is a regular expression to filter only those ingresses that match. If an ingress does
 	// not have a class annotation or the annotation is an empty string, skipper will load it. The default
 	// value for the ingress class is 'skipper'.

dataclients/kubernetes/kubernetestest/fixtures.go

@@ -39,6 +39,7 @@ type kubeOptionsParser struct {
 	EastWestRangePredicates        []*eskip.Predicate `yaml:"eastWestRangePredicatesAppend"`
 	HTTPSRedirect                  bool               `yaml:"httpsRedirect"`
 	HTTPSRedirectCode              int                `yaml:"httpsRedirectCode"`
+	DisableCatchAllRoutes          bool               `yaml:"disableCatchAllRoutes"`
 	BackendNameTracingTag          bool               `yaml:"backendNameTracingTag"`
 	OnlyAllowedExternalNames       bool               `yaml:"onlyAllowedExternalNames"`
 	AllowedExternalNames           []string           `yaml:"allowedExternalNames"`
@@ -230,6 +231,7 @@ func testFixture(t *testing.T, f fixtureSet) {
 		o.KubernetesEastWestRangePredicates = kop.EastWestRangePredicates
 		o.ProvideHTTPSRedirect = kop.HTTPSRedirect
 		o.HTTPSRedirectCode = kop.HTTPSRedirectCode
+		o.DisableCatchAllRoutes = kop.DisableCatchAllRoutes
 		o.BackendNameTracingTag = kop.BackendNameTracingTag
 		o.IngressClass = kop.IngressClass
 		o.CertificateRegistry = cr

dataclients/kubernetes/routegroup.go

@@ -538,11 +538,13 @@ func (r *routeGroups) convert(s *clusterState, df defaultFilters, loggingEnabled
 				continue
 			}
 
-			catchAll := hostCatchAllRoutes(ctx.hostRoutes, func(host string) string {
-				// "catchall" won't conflict with any HTTP method
-				return rgRouteID("", toSymbol(host), "catchall", 0, 0, false)
-			})
-			ri = append(ri, catchAll...)
+			if !r.options.DisableCatchAllRoutes {
+				catchAll := hostCatchAllRoutes(ctx.hostRoutes, func(host string) string {
+					// "catchall" won't conflict with any HTTP method
+					return rgRouteID("", toSymbol(host), "catchall", 0, 0, false)
+				})
+				ri = append(ri, catchAll...)
+			}
 
 			rs = append(rs, ri...)
 		}
@@ -572,11 +574,13 @@ func (r *routeGroups) convert(s *clusterState, df defaultFilters, loggingEnabled
 				continue
 			}
 
-			catchAll := hostCatchAllRoutes(internalCtx.hostRoutes, func(host string) string {
-				// "catchall" won't conflict with any HTTP method
-				return rgRouteID("", toSymbol(host), "catchall", 0, 0, true)
-			})
-			internalRi = append(internalRi, catchAll...)
+			if !r.options.DisableCatchAllRoutes {
+				catchAll := hostCatchAllRoutes(internalCtx.hostRoutes, func(host string) string {
+					// "catchall" won't conflict with any HTTP method
+					return rgRouteID("", toSymbol(host), "catchall", 0, 0, true)
+				})
+				internalRi = append(internalRi, catchAll...)
+			}
 
 			applyEastWestRangePredicates(internalRi, r.options.KubernetesEastWestRangePredicates)
 

dataclients/kubernetes/testdata/ingressV1/ingress-data/disabled-catchall.eskip

@@ -0,0 +1,9 @@
+kube_namespace1__ingress1__test_example_org___test1__service1:
+  Host(/^(test[.]example[.]org[.]?(:[0-9]+)?)$/)
+  && PathRegexp(/^(\/test1)/)
+  -> "http://42.0.1.2:8080";
+
+kube_namespace1__ingress1__test_ingress_cluster_local___test1__service1:
+  Host("^(test[.]ingress[.]cluster[.]local[.]?(:[0-9]+)?)$")
+  && PathRegexp("^(/test1)")
+  -> "http://42.0.1.2:8080";

dataclients/kubernetes/testdata/ingressV1/ingress-data/disabled-catchall.kube

@@ -0,0 +1,3 @@
+disableCatchAllRoutes: true
+eastWestRangeDomains:
+    - ingress.cluster.local

dataclients/kubernetes/testdata/ingressV1/ingress-data/disabled-catchall.yaml

@@ -0,0 +1,53 @@
+apiVersion: networking.k8s.io/v1
+kind: Ingress
+metadata:
+  namespace: namespace1
+  name: ingress1
+spec:
+  rules:
+    - host: test.example.org
+      http:
+        paths:
+          - path: "/test1"
+            pathType: ImplementationSpecific
+            backend:
+              service:
+                name: service1
+                port:
+                  name: port1
+    - host: test.ingress.cluster.local
+      http:
+        paths:
+          - path: "/test1"
+            pathType: ImplementationSpecific
+            backend:
+              service:
+                name: service1
+                port:
+                  name: port1
+---
+apiVersion: v1
+kind: Service
+metadata:
+  namespace: namespace1
+  name: service1
+spec:
+  clusterIP: 1.2.3.4
+  ports:
+    - name: port1
+      port: 8080
+      targetPort: 8080
+  type: ClusterIP
+---
+apiVersion: v1
+kind: Endpoints
+metadata:
+  namespace: namespace1
+  name: service1
+subsets:
+  - addresses:
+      - ip: 42.0.1.2
+    ports:
+      - name: port1
+        port: 8080
+        protocol: TCP

dataclients/kubernetes/testdata/routegroups/convert/disabled-catchall.eskip

@@ -0,0 +1,2 @@
+kube_rg__default__myapp__all__0_0: Host("^(example[.]org[.]?(:[0-9]+)?)$") && Path("/app") -> "http://10.2.4.8:80";
+kube_rg__internal_default__myapp__all__0_0: Host("^(example[.]ingress[.]cluster[.]local[.]?(:[0-9]+)?)$") && Path("/app") -> "http://10.2.4.8:80";

dataclients/kubernetes/testdata/routegroups/convert/disabled-catchall.kube

@@ -0,0 +1,3 @@
+disableCatchAllRoutes: true
+eastWestRangeDomains:
+    - ingress.cluster.local

dataclients/kubernetes/testdata/routegroups/convert/disabled-catchall.yaml

@@ -0,0 +1,40 @@
+apiVersion: zalando.org/v1
+kind: RouteGroup
+metadata:
+  name: myapp
+spec:
+  hosts:
+    - example.org
+    - example.ingress.cluster.local
+  backends:
+    - name: myapp
+      type: service
+      serviceName: myapp
+      servicePort: 80
+  defaultBackends:
+    - backendName: myapp
+  routes:
+    - path: /app
+---
+apiVersion: v1
+kind: Service
+metadata:
+  name: myapp
+spec:
+  ports:
+    - port: 80
+      protocol: TCP
+      targetPort: 80
+  selector:
+    application: myapp
+  type: ClusterIP
+---
+apiVersion: v1
+kind: Endpoints
+metadata:
+  name: myapp
+subsets:
+  - addresses:
+      - ip: 10.2.4.8
+    ports:
+      - port: 80

skipper.go

@@ -188,6 +188,9 @@ type Options struct {
 	// when used together with -kubernetes-https-redirect.
 	KubernetesHTTPSRedirectCode int
 
+	// KubernetesDisableCatchAllRoutes, when set, tells the data client to not create catchall routes.
+	KubernetesDisableCatchAllRoutes bool
+
 	// KubernetesIngressClass is a regular expression, that will make
 	// skipper load only the ingress resources that have a matching
 	// kubernetes.io/ingress.class annotation. For backwards compatibility,
@@ -930,6 +933,7 @@ func (o *Options) KubernetesDataClientOptions() kubernetes.Options {
 		KubernetesEastWestRangeDomains:    o.KubernetesEastWestRangeDomains,
 		KubernetesEastWestRangePredicates: o.KubernetesEastWestRangePredicates,
 		HTTPSRedirectCode:                 o.KubernetesHTTPSRedirectCode,
+		DisableCatchAllRoutes:             o.KubernetesDisableCatchAllRoutes,
 		IngressClass:                      o.KubernetesIngressClass,
 		IngressLabelSelectors:             o.KubernetesIngressLabelSelectors,
 		ServicesLabelSelectors:            o.KubernetesServicesLabelSelectors,