AlexanderYastrebov
commented
1 year ago 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
What needs to be done and why is it needed?
We go through all routes and see if
routeIDand 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 misleadingWhat 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.