Skip to content

Commit bccae62

Browse files
The unique_host router filter can lose routes
The unique_host filter grew over time to add additional functionality and scenarios. As part of that growth the guarantees for how claiming routes evicted and restored other routes that shared the same host or path were weakened and eventually broken. Two routes that conflict by host and path created at the same time where one is deleted will result in a 50% chance that the host remains clamed by the deleted route. Refactor unique_host to use a separate data structure - a host index - to track the routes that are assigned to each host. Split the logic for deciding which routes for a given host to use into side-effect free functions that can be individually tested. In the core unique_host code guarantee that when routes cover other routes that deletions are propagated down to lower level plugins and that no route is left behind. Add much stronger tests on the core data structure to ensure the logic of unique_host is not broken.
1 parent 2173ae3 commit bccae62

File tree

5 files changed

+1240
-133
lines changed

5 files changed

+1240
-133
lines changed
Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
package hostindex
2+
3+
import (
4+
"sort"
5+
6+
routeapi "github.com/openshift/origin/pkg/route/apis/route"
7+
)
8+
9+
// Changed allows a route activation function to record which routes moved from inactive
10+
// to active or vice versa.
11+
type Changed interface {
12+
// Activated should be invoked if the route was previously inactive and is now active.
13+
Activated(route *routeapi.Route)
14+
// Displaced should be invoked if the route was previously active and is now inactive.
15+
Displaced(route *routeapi.Route)
16+
}
17+
18+
// RouteActivationFunc attempts to add routes from inactive into the active set. Any routes that are not
19+
// valid must be returned in the displaced list. All routes that are added into active should be passed to
20+
// Changed.Activated() and any route in active that ends up in the displaced list should be passed to
21+
// Changed.Displaced(). It is the caller's responsiblity to invoke Activated or Displaced for inactive routes.
22+
// Routes must be provided to the function in lowest to highest order and the ordering should be preserved
23+
// in activated.
24+
type RouteActivationFunc func(changed Changed, active []*routeapi.Route, inactive ...*routeapi.Route) (activated, displaced []*routeapi.Route)
25+
26+
// OldestFirst identifies all unique host+port combinations in active and inactive, ordered by oldest
27+
// first. Duplicates are returned in displaced in order. It assumes all provided route have the same
28+
// spec.host value.
29+
func OldestFirst(changed Changed, active []*routeapi.Route, inactive ...*routeapi.Route) (updated, displaced []*routeapi.Route) {
30+
if len(inactive) == 0 {
31+
return active, nil
32+
}
33+
34+
// Routes must be unique in the updated list (no duplicate spec.host / spec.path combination).
35+
return zipperMerge(active, inactive, changed, func(route *routeapi.Route) bool { return true })
36+
}
37+
38+
// SameNamespace identifies all unique host+port combinations in active and inactive that are from
39+
// the same namespace as the oldest route (creation timestamp and then uid ordering). Duplicates and
40+
// non matching routes are returned in displaced and are unordered. It assumes all provided routes
41+
// have the same spec.host value.
42+
func SameNamespace(changed Changed, active []*routeapi.Route, inactive ...*routeapi.Route) (updated, displaced []*routeapi.Route) {
43+
if len(inactive) == 0 {
44+
return active, nil
45+
}
46+
ns := inactive[0].Namespace
47+
48+
// if:
49+
// * there was no previous claimant
50+
// * we're newer than the oldest claimant
51+
// * or we're in the same namespace as the oldest claimant
52+
// add the new routes to the existing active set
53+
if len(active) == 0 || routeapi.RouteLessThan(active[0], inactive[0]) {
54+
if len(active) > 0 {
55+
ns = active[0].Namespace
56+
}
57+
updated = active
58+
for _, route := range inactive {
59+
updated, displaced = appendRoute(changed, updated, displaced, route, ns == route.Namespace, false)
60+
}
61+
sort.Slice(updated, func(i, j int) bool { return routeapi.RouteLessThan(updated[i], updated[j]) })
62+
return updated, displaced
63+
}
64+
65+
// We're claiming the host and in a different namespace than the previous holder so we must recalculate
66+
// everything. We do that with a zipper merge of the two sorted lists, appending routes as we go.
67+
// Routes must be unique in the updated list (no duplicate spec.host / spec.path combination).
68+
ns = ""
69+
return zipperMerge(active, inactive, changed, func(route *routeapi.Route) bool {
70+
if len(ns) == 0 {
71+
ns = route.Namespace
72+
return true
73+
}
74+
return ns == route.Namespace
75+
})
76+
}
77+
78+
// zipperMerge assumes both active and inactive are in order and takes the oldest route from either
79+
// list until all items are processed. If fn returns false the item will be skipped.
80+
func zipperMerge(active, inactive []*routeapi.Route, changed Changed, fn func(*routeapi.Route) bool) (updated, displaced []*routeapi.Route) {
81+
i, j := 0, 0
82+
for {
83+
switch {
84+
case j >= len(active):
85+
for ; i < len(inactive); i++ {
86+
updated, displaced = appendRoute(changed, updated, displaced, inactive[i], fn(inactive[i]), false)
87+
}
88+
return updated, displaced
89+
case i >= len(inactive):
90+
for ; j < len(active); j++ {
91+
updated, displaced = appendRoute(changed, updated, displaced, active[j], fn(active[j]), true)
92+
}
93+
return updated, displaced
94+
default:
95+
a, b := inactive[i], active[j]
96+
if routeapi.RouteLessThan(a, b) {
97+
updated, displaced = appendRoute(changed, updated, displaced, a, fn(a), false)
98+
i++
99+
} else {
100+
updated, displaced = appendRoute(changed, updated, displaced, b, fn(b), true)
101+
j++
102+
}
103+
}
104+
}
105+
}
106+
107+
// appendRoute adds the route to the end of the appropriate list if matches is true and no route already exists in the list
108+
// with the same path.
109+
func appendRoute(changed Changed, updated, displaced []*routeapi.Route, route *routeapi.Route, matches bool, isActive bool) ([]*routeapi.Route, []*routeapi.Route) {
110+
if matches && !hasExistingMatch(updated, route) {
111+
if !isActive {
112+
changed.Activated(route)
113+
}
114+
return append(updated, route), displaced
115+
}
116+
if isActive {
117+
changed.Displaced(route)
118+
}
119+
return updated, append(displaced, route)
120+
}
121+
122+
// hasExistingMatch returns true if a route is in exists with the same path.
123+
func hasExistingMatch(exists []*routeapi.Route, route *routeapi.Route) bool {
124+
for _, existing := range exists {
125+
if existing.Spec.Path == route.Spec.Path {
126+
return true
127+
}
128+
}
129+
return false
130+
}

0 commit comments

Comments
 (0)