Fix filter namespaces in template router

Remove endpoints key format and route key format assumptions in the template plugin and router code.
Get the endpoints and route key related information from one place, this will be less prone to
errors and easy to modify in the future if needed.

Author: Ravi Sankar Penta

pkg/router/template/plugin.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"path/filepath"
 	"strconv"
+	"strings"
 	"text/template"
 	"time"
 
@@ -19,6 +20,10 @@ import (
 	unidlingapi "github.com/openshift/origin/pkg/unidling/api"
 )
 
+const (
+	endpointsKeySeparator = "/"
+)
+
 // TemplatePlugin implements the router.Plugin interface to provide
 // a template based, backend-agnostic router.
 type TemplatePlugin struct {
@@ -197,14 +202,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 fmt.Sprintf("%s%s%s", endpoints.Namespace, endpointsKeySeparator, 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 fmt.Sprintf("%s%s%s", namespacedName.Namespace, endpointsKeySeparator, namespacedName.Name)
 }
 
 // createRouterEndpoints creates openshift router endpoints based on k8s endpoints

pkg/router/template/plugin_test.go

@@ -1,9 +1,7 @@
 package templaterouter
 
 import (
-	"fmt"
 	"reflect"
-	"strings"
 	"testing"
 	"time"
 
@@ -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,
@@ -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
@@ -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 route.Spec.Host + routeKeySeparator + route.Spec.Path
 }
 
 func (r *TestRouter) Commit() {
@@ -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)
 
@@ -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)
@@ -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" {
@@ -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" {
@@ -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)
 	}
@@ -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)
@@ -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 {
@@ -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)
 
@@ -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)
@@ -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)
 		}
 	}
@@ -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)
 	}
 }