From 40f463417efa79cdb2c919201ac43266242ee11e Mon Sep 17 00:00:00 2001 From: Alexander Yastrebov Date: Fri, 6 Oct 2023 13:29:52 +0200 Subject: [PATCH] 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 --- config/config.go | 3 ++ .../kubernetes/definitions/routegroups.go | 6 +-- dataclients/kubernetes/ingress.go | 12 +++-- dataclients/kubernetes/kube.go | 3 ++ .../kubernetes/kubernetestest/fixtures.go | 2 + dataclients/kubernetes/routegroup.go | 24 +++++---- .../ingress-data/disabled-catchall.eskip | 9 ++++ .../ingress-data/disabled-catchall.kube | 3 ++ .../ingress-data/disabled-catchall.yaml | 53 +++++++++++++++++++ .../convert/disabled-catchall.eskip | 2 + .../convert/disabled-catchall.kube | 3 ++ .../convert/disabled-catchall.yaml | 40 ++++++++++++++ skipper.go | 4 ++ 13 files changed, 146 insertions(+), 18 deletions(-) create mode 100644 dataclients/kubernetes/testdata/ingressV1/ingress-data/disabled-catchall.eskip create mode 100644 dataclients/kubernetes/testdata/ingressV1/ingress-data/disabled-catchall.kube create mode 100644 dataclients/kubernetes/testdata/ingressV1/ingress-data/disabled-catchall.yaml create mode 100644 dataclients/kubernetes/testdata/routegroups/convert/disabled-catchall.eskip create mode 100644 dataclients/kubernetes/testdata/routegroups/convert/disabled-catchall.kube create mode 100644 dataclients/kubernetes/testdata/routegroups/convert/disabled-catchall.yaml diff --git a/config/config.go b/config/config.go index 6102061afc..a64f379827 100644 --- a/config/config.go +++ b/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, diff --git a/dataclients/kubernetes/definitions/routegroups.go b/dataclients/kubernetes/definitions/routegroups.go index 874c0593a0..626880e7d5 100644 --- a/dataclients/kubernetes/definitions/routegroups.go +++ b/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"` } diff --git a/dataclients/kubernetes/ingress.go b/dataclients/kubernetes/ingress.go index aaec03360b..be7afcf3d6 100644 --- a/dataclients/kubernetes/ingress.go +++ b/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)...) + } } } diff --git a/dataclients/kubernetes/kube.go b/dataclients/kubernetes/kube.go index 536838b5a0..5ca47e3e66 100644 --- a/dataclients/kubernetes/kube.go +++ b/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'. diff --git a/dataclients/kubernetes/kubernetestest/fixtures.go b/dataclients/kubernetes/kubernetestest/fixtures.go index 0fb005c699..f43e706608 100644 --- a/dataclients/kubernetes/kubernetestest/fixtures.go +++ b/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 diff --git a/dataclients/kubernetes/routegroup.go b/dataclients/kubernetes/routegroup.go index d4df8e306d..d83c74554d 100644 --- a/dataclients/kubernetes/routegroup.go +++ b/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) diff --git a/dataclients/kubernetes/testdata/ingressV1/ingress-data/disabled-catchall.eskip b/dataclients/kubernetes/testdata/ingressV1/ingress-data/disabled-catchall.eskip new file mode 100644 index 0000000000..3931b28e8a --- /dev/null +++ b/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"; diff --git a/dataclients/kubernetes/testdata/ingressV1/ingress-data/disabled-catchall.kube b/dataclients/kubernetes/testdata/ingressV1/ingress-data/disabled-catchall.kube new file mode 100644 index 0000000000..40e1269695 --- /dev/null +++ b/dataclients/kubernetes/testdata/ingressV1/ingress-data/disabled-catchall.kube @@ -0,0 +1,3 @@ +disableCatchAllRoutes: true +eastWestRangeDomains: + - ingress.cluster.local diff --git a/dataclients/kubernetes/testdata/ingressV1/ingress-data/disabled-catchall.yaml b/dataclients/kubernetes/testdata/ingressV1/ingress-data/disabled-catchall.yaml new file mode 100644 index 0000000000..bb6a083dd4 --- /dev/null +++ b/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 diff --git a/dataclients/kubernetes/testdata/routegroups/convert/disabled-catchall.eskip b/dataclients/kubernetes/testdata/routegroups/convert/disabled-catchall.eskip new file mode 100644 index 0000000000..ef034f422b --- /dev/null +++ b/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"; diff --git a/dataclients/kubernetes/testdata/routegroups/convert/disabled-catchall.kube b/dataclients/kubernetes/testdata/routegroups/convert/disabled-catchall.kube new file mode 100644 index 0000000000..40e1269695 --- /dev/null +++ b/dataclients/kubernetes/testdata/routegroups/convert/disabled-catchall.kube @@ -0,0 +1,3 @@ +disableCatchAllRoutes: true +eastWestRangeDomains: + - ingress.cluster.local diff --git a/dataclients/kubernetes/testdata/routegroups/convert/disabled-catchall.yaml b/dataclients/kubernetes/testdata/routegroups/convert/disabled-catchall.yaml new file mode 100644 index 0000000000..a9075cf0f3 --- /dev/null +++ b/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 diff --git a/skipper.go b/skipper.go index 447d63bb10..e41ca73854 100644 --- a/skipper.go +++ b/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,