skipper
skipper copied to clipboard
Better handling for duplicate routes in skipper
What needs to be done and why is it needed?
We go through all routes and see if routeID
and body of routes match we log a warning code
So if we have 2 catchall routes for same hostnames but different RG it will also log, which could be misleading
What needs to be changed?
For discussion
Option 1: Only log when RG/Ingress that has duplicate hostname. Option 2: Add ingress/rg name to routeID (If we add namespace+name it will make route ids different but we would still have duplicate per-host catchall routes.) Option 3: We should change main routegroup loop to only add catchall hostname routes once per hostname.
The same issue may also exist for ingress, we need to check that e.g. by adding a test case with two ingresses for the same hostname.
- [ ] Add test cases for 2 different ingress/routegroups that won't log
Also Skipper datasource neither detects duplicate routes from the same dataclient nor between multiple dataclients and overwrites existing route with the latest update which could be shown via:
diff --git a/routing/datasource.go b/routing/datasource.go
index cf006f6f..74cf4ad3 100644
--- a/routing/datasource.go
+++ b/routing/datasource.go
@@ -120,7 +120,7 @@ func receiveFromClient(c DataClient, o Options, out chan<- *incomingData, quit <
// applies incoming route definitions to key/route map, where
// the keys are the route ids.
-func applyIncoming(defs routeDefs, d *incomingData) routeDefs {
+func applyIncoming(defs routeDefs, d *incomingData, l logging.Logger) routeDefs {
if d.typ == incomingReset || defs == nil {
defs = make(routeDefs)
}
@@ -133,6 +133,9 @@ func applyIncoming(defs routeDefs, d *incomingData) routeDefs {
if d.typ == incomingReset || d.typ == incomingUpdate {
for _, def := range d.upsertedRoutes {
+ if _, ok := defs[def.Id]; ok {
+ l.Infof("overwriting %q route definition (same dataclient)", def.Id)
+ }
defs[def.Id] = def
}
}
@@ -141,10 +144,13 @@ func applyIncoming(defs routeDefs, d *incomingData) routeDefs {
}
// merges the route definitions from multiple data clients by route id
-func mergeDefs(defsByClient map[DataClient]routeDefs) []*eskip.Route {
+func mergeDefs(defsByClient map[DataClient]routeDefs, l logging.Logger) []*eskip.Route {
mergeByID := make(routeDefs)
for _, defs := range defsByClient {
for id, def := range defs {
+ if _, ok := mergeByID[id]; ok {
+ l.Infof("overwriting %q route definition (multiple dataclients)", id)
+ }
mergeByID[id] = def
}
}
@@ -183,10 +189,10 @@ func receiveRouteDefs(o Options, quit <-chan struct{}) <-chan []*eskip.Route {
incoming.log(o.Log, o.SuppressLogs)
c := incoming.client
- defsByClient[c] = applyIncoming(defsByClient[c], incoming)
+ defsByClient[c] = applyIncoming(defsByClient[c], incoming, o.Log)
select {
- case out <- mergeDefs(defsByClient):
+ case out <- mergeDefs(defsByClient, o.Log):
case <-quit:
return
}
and
# duplicate routes from one dataclient
$ bin/skipper -routes-file=<(cat ./eskipfile/fixtures/test.eskip ./eskipfile/fixtures/test.eskip)
[APP]INFO[0000] Expose metrics in codahale format
[APP]INFO[0000] enable swarm: false
[APP]INFO[0000] Replacing tee filter specification
[APP]INFO[0000] Replacing teenf filter specification
[APP]INFO[0000] Replacing lua filter specification
[APP]INFO[0000] support listener on :9911
[APP]INFO[0000] Dataclients are updated once, first load complete
[APP]INFO[0000] proxy listener on :9090
[APP]INFO[0000] TLS settings not found, defaulting to HTTP
[APP]INFO[0000] route settings, reset, route: foo: Path("/foo") -> setPath("/") -> "https://foo.example.org"
[APP]INFO[0000] route settings, reset, route: bar: Path("/bar") -> setPath("/") -> "https://bar.example.org"
[APP]INFO[0000] route settings, reset, route: foo: Path("/foo") -> setPath("/") -> "https://foo.example.org"
[APP]INFO[0000] route settings, reset, route: bar: Path("/bar") -> setPath("/") -> "https://bar.example.org"
[APP]INFO[0000] overwriting "foo" route definition (same dataclient)
[APP]INFO[0000] overwriting "bar" route definition (same dataclient)
[APP]INFO[0000] route settings received
[APP]INFO[0000] route settings applied
[APP]INFO[0003] route settings, update, deleted id: foo
[APP]INFO[0003] route settings, update, deleted id: bar
[APP]INFO[0003] route settings received
[APP]INFO[0003] route settings applied
and
# duplicate routes from multiple dataclients
$ bin/skipper -routes-file=./eskipfile/fixtures/test.eskip,./eskipfile/fixtures/test.eskip
[APP]INFO[0000] Expose metrics in codahale format
[APP]INFO[0000] enable swarm: false
[APP]INFO[0000] Replacing tee filter specification
[APP]INFO[0000] Replacing teenf filter specification
[APP]INFO[0000] Replacing lua filter specification
[APP]INFO[0000] support listener on :9911
[APP]INFO[0000] Dataclients are updated once, first load complete
[APP]INFO[0000] proxy listener on :9090
[APP]INFO[0000] TLS settings not found, defaulting to HTTP
[APP]INFO[0000] route settings, reset, route: foo: Path("/foo") -> setPath("/") -> "https://foo.example.org"
[APP]INFO[0000] route settings, reset, route: bar: Path("/bar") -> setPath("/") -> "https://bar.example.org"
[APP]INFO[0000] route settings, reset, route: foo: Path("/foo") -> setPath("/") -> "https://foo.example.org"
[APP]INFO[0000] route settings, reset, route: bar: Path("/bar") -> setPath("/") -> "https://bar.example.org"
[APP]INFO[0000] overwriting "foo" route definition (multiple dataclients)
[APP]INFO[0000] overwriting "bar" route definition (multiple dataclients)
[APP]INFO[0000] route settings received
[APP]INFO[0000] route settings received
[APP]INFO[0000] route settings applied
[APP]INFO[0000] route settings applied