Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix filter namespaces in template router #15916

Merged
merged 3 commits into from
Aug 30, 2017
Merged

Fix filter namespaces in template router #15916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
86a8ea0
Fix filter namespaces in template router
Aug 24, 2017
29e84f8
Remove endpoints and route key format assumptions in the template plu…
Aug 23, 2017
f088be6
Added test cases for router FilterNamespaces()
Aug 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions pkg/router/template/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"path/filepath"
"strconv"
"strings"
"text/template"
"time"

Expand All @@ -19,6 +20,11 @@ import (
unidlingapi "github.com/openshift/origin/pkg/unidling/api"
)

const (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a comment that says what this is used for please

// endpointsKeySeparator is used to uniquely generate key/ID for endpoints
endpointsKeySeparator = "/"
)

// TemplatePlugin implements the router.Plugin interface to provide
// a template based, backend-agnostic router.
type TemplatePlugin struct {
Expand Down Expand Up @@ -197,14 +203,28 @@ func (p *TemplatePlugin) Commit() error {

// endpointsKey returns the internal router key to use for the given Endpoints.
func endpointsKey(endpoints *kapi.Endpoints) string {
return fmt.Sprintf("%s/%s", endpoints.Namespace, endpoints.Name)
return endpointsKeyFromParts(endpoints.Namespace, endpoints.Name)
}

func endpointsKeyFromParts(namespace, name string) string {
return fmt.Sprintf("%s%s%s", namespace, endpointsKeySeparator, name)
}

func getPartsFromEndpointsKey(key string) (string, string) {
tokens := strings.SplitN(key, endpointsKeySeparator, 2)
if len(tokens) != 2 {
glog.Errorf("Expected separator %q not found in endpoints key %q", endpointsKeySeparator, key)
}
namespace := tokens[0]
name := tokens[1]
return namespace, name
}

// peerServiceKey may be used by the underlying router when handling endpoints to identify
// endpoints that belong to its peers. THIS MUST FOLLOW THE KEY STRATEGY OF endpointsKey. It
// receives a NamespacedName that is created from the service that is added by the oadm command
func peerEndpointsKey(namespacedName ktypes.NamespacedName) string {
return fmt.Sprintf("%s/%s", namespacedName.Namespace, namespacedName.Name)
return endpointsKeyFromParts(namespacedName.Namespace, namespacedName.Name)
}

// createRouterEndpoints creates openshift router endpoints based on k8s endpoints
Expand Down
56 changes: 27 additions & 29 deletions pkg/router/template/plugin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
package templaterouter

import (
"fmt"
"reflect"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -173,7 +171,7 @@ func (r *TestRouter) DeleteEndpoints(id string) {

// AddRoute adds a ServiceAliasConfig and associated ServiceUnits for the route
func (r *TestRouter) AddRoute(route *routeapi.Route) {
routeKey := r.routeKey(route)
routeKey := getKey(route)

config := ServiceAliasConfig{
Host: route.Spec.Host,
Expand All @@ -195,7 +193,7 @@ func (r *TestRouter) numberOfEndpoints(key string) int32 {

// RemoveRoute removes the service alias config for Route
func (r *TestRouter) RemoveRoute(route *routeapi.Route) {
routeKey := r.routeKey(route)
routeKey := getKey(route)
_, ok := r.State[routeKey]
if !ok {
return
Expand All @@ -222,25 +220,25 @@ func (r *TestRouter) FilterNamespaces(namespaces sets.String) {
for k := range r.ServiceUnits {
// TODO: the id of a service unit should be defined inside this class, not passed in from the outside
// remove the leak of the abstraction when we refactor this code
ns := strings.SplitN(k, "/", 2)[0]
ns, _ := getPartsFromEndpointsKey(k)
if namespaces.Has(ns) {
continue
}
delete(r.ServiceUnits, k)
}

for k := range r.State {
ns := strings.SplitN(k, "-", 2)[0]
ns, _ := getPartsFromRouteKey(k)
if namespaces.Has(ns) {
continue
}
delete(r.State, k)
}
}

// routeKey create an identifier for the route consisting of host-path
func (r *TestRouter) routeKey(route *routeapi.Route) string {
return route.Spec.Host + "-" + route.Spec.Path
// getKey create an identifier for the route consisting of host-path
func getKey(route *routeapi.Route) string {
return routeKeyFromParts(route.Spec.Host, route.Spec.Path)
}

func (r *TestRouter) Commit() {
Expand Down Expand Up @@ -499,7 +497,7 @@ func TestHandleRoute(t *testing.T) {
},
},
}
serviceUnitKey := fmt.Sprintf("%s/%s", route.Namespace, route.Spec.To.Name)
serviceUnitKey := endpointsKeyFromParts(route.Namespace, route.Spec.To.Name)

plugin.HandleRoute(watch.Added, route)

Expand All @@ -508,10 +506,10 @@ func TestHandleRoute(t *testing.T) {
if !ok {
t.Errorf("TestHandleRoute was unable to find the service unit %s after HandleRoute was called", route.Spec.To.Name)
} else {
serviceAliasCfg, ok := router.State[router.routeKey(route)]
serviceAliasCfg, ok := router.State[getKey(route)]

if !ok {
t.Errorf("TestHandleRoute expected route key %s", router.routeKey(route))
t.Errorf("TestHandleRoute expected route key %s", getKey(route))
} else {
if serviceAliasCfg.Host != route.Spec.Host || serviceAliasCfg.Path != route.Spec.Path {
t.Errorf("Expected route did not match service alias config %v : %v", route, serviceAliasCfg)
Expand Down Expand Up @@ -541,7 +539,7 @@ func TestHandleRoute(t *testing.T) {
if err := plugin.HandleRoute(watch.Added, duplicateRoute); err == nil {
t.Fatal("unexpected non-error")
}
if _, ok := router.FindServiceUnit("foo/TestService2"); ok {
if _, ok := router.FindServiceUnit(endpointsKeyFromParts("foo", "TestService2")); ok {
t.Fatalf("unexpected second unit: %#v", router)
}
if r, ok := plugin.RoutesForHost("www.example.com"); !ok || r[0].Name != "test" {
Expand All @@ -559,10 +557,10 @@ func TestHandleRoute(t *testing.T) {
if err := plugin.HandleRoute(watch.Deleted, duplicateRoute); err == nil {
t.Fatal("unexpected non-error")
}
if _, ok := router.FindServiceUnit("foo/TestService2"); ok {
if _, ok := router.FindServiceUnit(endpointsKeyFromParts("foo", "TestService2")); ok {
t.Fatalf("unexpected second unit: %#v", router)
}
if _, ok := router.FindServiceUnit("foo/TestService"); !ok {
if _, ok := router.FindServiceUnit(endpointsKeyFromParts("foo", "TestService")); !ok {
t.Fatalf("unexpected first unit: %#v", router)
}
if r, ok := plugin.RoutesForHost("www.example.com"); !ok || r[0].Name != "test" {
Expand All @@ -581,7 +579,7 @@ func TestHandleRoute(t *testing.T) {
if err := plugin.HandleRoute(watch.Added, duplicateRoute); err != nil {
t.Fatal("unexpected error")
}
_, ok = router.FindServiceUnit("foo/TestService2")
_, ok = router.FindServiceUnit(endpointsKeyFromParts("foo", "TestService2"))
if !ok {
t.Fatalf("missing second unit: %#v", router)
}
Expand All @@ -602,10 +600,10 @@ func TestHandleRoute(t *testing.T) {
if !ok {
t.Errorf("TestHandleRoute was unable to find the service unit %s after HandleRoute was called", route.Spec.To.Name)
} else {
serviceAliasCfg, ok := router.State[router.routeKey(route)]
serviceAliasCfg, ok := router.State[getKey(route)]

if !ok {
t.Errorf("TestHandleRoute expected route key %s", router.routeKey(route))
t.Errorf("TestHandleRoute expected route key %s", getKey(route))
} else {
if serviceAliasCfg.Host != route.Spec.Host || serviceAliasCfg.Path != route.Spec.Path {
t.Errorf("Expected route did not match service alias config %v : %v", route, serviceAliasCfg)
Expand All @@ -627,10 +625,10 @@ func TestHandleRoute(t *testing.T) {
if !ok {
t.Errorf("TestHandleRoute was unable to find the service unit %s after HandleRoute was called", route.Spec.To.Name)
} else {
_, ok := router.State[router.routeKey(route)]
_, ok := router.State[getKey(route)]

if ok {
t.Errorf("TestHandleRoute did not expect route key %s", router.routeKey(route))
t.Errorf("TestHandleRoute did not expect route key %s", getKey(route))
}
}
if plugin.HostLen() != 0 {
Expand Down Expand Up @@ -668,7 +666,7 @@ func TestHandleRouteExtendedValidation(t *testing.T) {
},
},
}
serviceUnitKey := fmt.Sprintf("%s/%s", route.Namespace, route.Spec.To.Name)
serviceUnitKey := endpointsKeyFromParts(route.Namespace, route.Spec.To.Name)

plugin.HandleRoute(watch.Added, route)

Expand All @@ -677,10 +675,10 @@ func TestHandleRouteExtendedValidation(t *testing.T) {
if !ok {
t.Errorf("TestHandleRoute was unable to find the service unit %s after HandleRoute was called", route.Spec.To.Name)
} else {
serviceAliasCfg, ok := router.State[router.routeKey(route)]
serviceAliasCfg, ok := router.State[getKey(route)]

if !ok {
t.Errorf("TestHandleRoute expected route key %s", router.routeKey(route))
t.Errorf("TestHandleRoute expected route key %s", getKey(route))
} else {
if serviceAliasCfg.Host != route.Spec.Host || serviceAliasCfg.Path != route.Spec.Path {
t.Errorf("Expected route did not match service alias config %v : %v", route, serviceAliasCfg)
Expand Down Expand Up @@ -1017,7 +1015,7 @@ func TestNamespaceScopingFromEmpty(t *testing.T) {
// ignores all events for namespace that doesn't match
for _, s := range []watch.EventType{watch.Added, watch.Modified, watch.Deleted} {
plugin.HandleRoute(s, route)
if _, ok := router.FindServiceUnit("foo/TestService"); ok || plugin.HostLen() != 0 {
if _, ok := router.FindServiceUnit(endpointsKeyFromParts("foo", "TestService")); ok || plugin.HostLen() != 0 {
t.Errorf("unexpected router state %#v", router)
}
}
Expand All @@ -1026,29 +1024,29 @@ func TestNamespaceScopingFromEmpty(t *testing.T) {
plugin.HandleNamespaces(sets.NewString("bar"))
for _, s := range []watch.EventType{watch.Added, watch.Modified, watch.Deleted} {
plugin.HandleRoute(s, route)
if _, ok := router.FindServiceUnit("foo/TestService"); ok || plugin.HostLen() != 0 {
if _, ok := router.FindServiceUnit(endpointsKeyFromParts("foo", "TestService")); ok || plugin.HostLen() != 0 {
t.Errorf("unexpected router state %#v", router)
}
}

// allow foo
plugin.HandleNamespaces(sets.NewString("foo", "bar"))
plugin.HandleRoute(watch.Added, route)
if _, ok := router.FindServiceUnit("foo/TestService"); !ok || plugin.HostLen() != 1 {
if _, ok := router.FindServiceUnit(endpointsKeyFromParts("foo", "TestService")); !ok || plugin.HostLen() != 1 {
t.Errorf("unexpected router state %#v", router)
}

// forbid foo, and make sure it's cleared
plugin.HandleNamespaces(sets.NewString("bar"))
if _, ok := router.FindServiceUnit("foo/TestService"); ok || plugin.HostLen() != 0 {
if _, ok := router.FindServiceUnit(endpointsKeyFromParts("foo", "TestService")); ok || plugin.HostLen() != 0 {
t.Errorf("unexpected router state %#v", router)
}
plugin.HandleRoute(watch.Modified, route)
if _, ok := router.FindServiceUnit("foo/TestService"); ok || plugin.HostLen() != 0 {
if _, ok := router.FindServiceUnit(endpointsKeyFromParts("foo", "TestService")); ok || plugin.HostLen() != 0 {
t.Errorf("unexpected router state %#v", router)
}
plugin.HandleRoute(watch.Added, route)
if _, ok := router.FindServiceUnit("foo/TestService"); ok || plugin.HostLen() != 0 {
if _, ok := router.FindServiceUnit(endpointsKeyFromParts("foo", "TestService")); ok || plugin.HostLen() != 0 {
t.Errorf("unexpected router state %#v", router)
}
}
Loading