Move route host updating out of the unique_host plugin

Because we are now using the informer cache, having the plugins mutate
the passed in object is incorrect (the cache doesn't have that
modification). Instead, mutate the cache from the very beginning so that
we always have the router's preferred spec.host set.

Author: smarterclayton

pkg/cmd/infra/router/f5.go

@@ -233,6 +233,7 @@ func (o *F5RouterOptions) Run() error {
 	}
 
 	factory := o.RouterSelection.NewFactory(routeclient, projectclient.Project().Projects(), kc)
+	factory.RouteModifierFn = o.RouteUpdate
 
 	var plugin router.Plugin = f5Plugin
 	var recorder controller.RejectionRecorder = controller.LogRejections
@@ -250,7 +251,7 @@ func (o *F5RouterOptions) Run() error {
 	if o.ExtendedValidation {
 		plugin = controller.NewExtendedValidator(plugin, recorder)
 	}
-	plugin = controller.NewUniqueHost(plugin, o.RouteSelectionFunc(), o.RouterSelection.DisableNamespaceOwnershipCheck, recorder)
+	plugin = controller.NewUniqueHost(plugin, o.RouterSelection.DisableNamespaceOwnershipCheck, recorder)
 	plugin = controller.NewHostAdmitter(plugin, o.F5RouteAdmitterFunc(), o.AllowWildcardRoutes, o.RouterSelection.DisableNamespaceOwnershipCheck, recorder)
 
 	watchNodes := (len(o.InternalAddress) != 0 && len(o.VxlanGateway) != 0)

pkg/cmd/infra/router/router.go

@@ -86,30 +86,31 @@ func (o *RouterSelection) Bind(flag *pflag.FlagSet) {
 	flag.StringVar(&o.ListenAddr, "listen-addr", cmdutil.Env("ROUTER_LISTEN_ADDR", ""), "The name of an interface to listen on to expose metrics and health checking. If not specified, will not listen. Overrides stats port.")
 }
 
-// RouteSelectionFunc returns a func that identifies the host for a route.
-func (o *RouterSelection) RouteSelectionFunc() controller.RouteHostFunc {
+// RouteUpdate updates the route before it is seen by the cache.
+func (o *RouterSelection) RouteUpdate(route *routeapi.Route) {
 	if len(o.HostnameTemplate) == 0 {
-		return controller.HostForRoute
+		return
 	}
-	return func(route *routeapi.Route) string {
-		if !o.OverrideHostname && len(route.Spec.Host) > 0 {
-			return route.Spec.Host
-		}
-		s, err := variable.ExpandStrict(o.HostnameTemplate, func(key string) (string, bool) {
-			switch key {
-			case "name":
-				return route.Name, true
-			case "namespace":
-				return route.Namespace, true
-			default:
-				return "", false
-			}
-		})
-		if err != nil {
-			return ""
+	if !o.OverrideHostname && len(route.Spec.Host) > 0 {
+		return
+	}
+	s, err := variable.ExpandStrict(o.HostnameTemplate, func(key string) (string, bool) {
+		switch key {
+		case "name":
+			return route.Name, true
+		case "namespace":
+			return route.Namespace, true
+		default:
+			return "", false
 		}
-		return strings.Trim(s, "\"'")
+	})
+	if err != nil {
+		return
 	}
+
+	s = strings.Trim(s, "\"'")
+	glog.V(4).Infof("changing route %s to %s", route.Spec.Host, s)
+	route.Spec.Host = s
 }
 
 func (o *RouterSelection) AdmissionCheck(route *routeapi.Route) error {

pkg/cmd/infra/router/template.go

@@ -414,6 +414,7 @@ func (o *TemplateRouterOptions) Run() error {
 	}
 
 	factory := o.RouterSelection.NewFactory(routeclient, projectclient.Project().Projects(), kc)
+	factory.RouteModifierFn = o.RouteUpdate
 
 	var plugin router.Plugin = templatePlugin
 	var recorder controller.RejectionRecorder = controller.LogRejections
@@ -431,7 +432,7 @@ func (o *TemplateRouterOptions) Run() error {
 	if o.ExtendedValidation {
 		plugin = controller.NewExtendedValidator(plugin, recorder)
 	}
-	plugin = controller.NewUniqueHost(plugin, o.RouteSelectionFunc(), o.RouterSelection.DisableNamespaceOwnershipCheck, recorder)
+	plugin = controller.NewUniqueHost(plugin, o.RouterSelection.DisableNamespaceOwnershipCheck, recorder)
 	plugin = controller.NewHostAdmitter(plugin, o.RouteAdmissionFunc(), o.AllowWildcardRoutes, o.RouterSelection.DisableNamespaceOwnershipCheck, recorder)
 
 	controller := factory.Create(plugin, false)

pkg/router/controller/factory/factory.go

@@ -45,6 +45,7 @@ type RouterControllerFactory struct {
 	FieldSelector   string
 	NamespaceLabels labels.Selector
 	ProjectLabels   labels.Selector
+	RouteModifierFn func(route *routeapi.Route)
 
 	informers map[reflect.Type]kcache.SharedIndexInformer
 }
@@ -122,6 +123,7 @@ func (f *RouterControllerFactory) registerInformerEventHandlers(rc *routercontro
 		f.registerSharedInformerEventHandlers(&kapi.Namespace{}, rc.HandleNamespace)
 	}
 	f.registerSharedInformerEventHandlers(&kapi.Endpoints{}, rc.HandleEndpoints)
+
 	f.registerSharedInformerEventHandlers(&routeapi.Route{}, rc.HandleRoute)
 
 	if rc.WatchNodes {
@@ -218,13 +220,30 @@ func (f *RouterControllerFactory) CreateRoutesSharedInformer() kcache.SharedInde
 			if err != nil {
 				return nil, err
 			}
+			if f.RouteModifierFn != nil {
+				for i := range routeList.Items {
+					f.RouteModifierFn(&routeList.Items[i])
+				}
+			}
 			// Return routes in order of age to avoid rejections during resync
 			sort.Sort(routeAge(routeList.Items))
 			return routeList, nil
 		},
 		WatchFunc: func(options v1.ListOptions) (watch.Interface, error) {
 			f.setSelectors(&options)
-			return f.RClient.Route().Routes(f.Namespace).Watch(options)
+			w, err := f.RClient.Route().Routes(f.Namespace).Watch(options)
+			if err != nil {
+				return nil, err
+			}
+			if f.RouteModifierFn != nil {
+				watch.Filter(w, func(in watch.Event) (watch.Event, bool) {
+					if route, ok := in.Object.(*routeapi.Route); ok {
+						f.RouteModifierFn(route)
+					}
+					return in, true
+				})
+			}
+			return w, nil
 		},
 	}
 	indexers := kcache.Indexers{kcache.NamespaceIndex: kcache.MetaNamespaceIndexFunc}

pkg/router/controller/host_admitter_test.go

@@ -854,7 +854,7 @@ func TestDisableOwnershipChecksFuzzing(t *testing.T) {
 
 	admitAll := func(route *routeapi.Route) error { return nil }
 	recorder := rejectionRecorder{rejections: make(map[string]string)}
-	uniqueHostPlugin := NewUniqueHost(p, HostForRoute, true, recorder)
+	uniqueHostPlugin := NewUniqueHost(p, true, recorder)
 	admitter := NewHostAdmitter(uniqueHostPlugin, RouteAdmissionFunc(admitAll), true, true, recorder)
 
 	oldest := metav1.Time{Time: time.Now()}

pkg/router/controller/status_test.go

@@ -235,7 +235,23 @@ func TestStatusResetsHost(t *testing.T) {
 	admitter := NewStatusAdmitter(p, c.Route(), lister, "test", "", noopLease{}, tracker)
 	err := admitter.HandleRoute(watch.Added, route)
 
-	checkResult(t, err, c, admitter, "route1.test.local", now, &now.Time, 0, 0)
+	route = checkResult(t, err, c, admitter, "route1.test.local", now, &now.Time, 0, 0)
+	ingress := findIngressForRoute(route, "test")
+	if ingress == nil {
+		t.Fatalf("no ingress found: %#v", route)
+	}
+	if ingress.Host != "route1.test.local" {
+		t.Fatalf("incorrect ingress: %#v", ingress)
+	}
+}
+
+func findIngressForRoute(route *routeapi.Route, routerName string) *routeapi.RouteIngress {
+	for i := range route.Status.Ingress {
+		if route.Status.Ingress[i].RouterName == routerName {
+			return &route.Status.Ingress[i]
+		}
+	}
+	return nil
 }
 
 func TestStatusAdmitsRouteOnForbidden(t *testing.T) {
@@ -273,7 +289,14 @@ func TestStatusAdmitsRouteOnForbidden(t *testing.T) {
 	lister := &routeLister{items: []*routeapi.Route{route}}
 	admitter := NewStatusAdmitter(p, c.Route(), lister, "test", "", noopLease{}, tracker)
 	err := admitter.HandleRoute(watch.Added, route)
-	checkResult(t, err, c, admitter, "route1.test.local", now, &touched.Time, 0, 0)
+	route = checkResult(t, err, c, admitter, "route1.test.local", now, &touched.Time, 0, 0)
+	ingress := findIngressForRoute(route, "test")
+	if ingress == nil {
+		t.Fatalf("no ingress found: %#v", route)
+	}
+	if ingress.Host != "route1.test.local" {
+		t.Fatalf("incorrect ingress: %#v", ingress)
+	}
 }
 
 func TestStatusBackoffOnConflict(t *testing.T) {

pkg/router/controller/unique_host.go

@@ -27,8 +27,7 @@ func HostForRoute(route *routeapi.Route) string {
 // UniqueHost implements the router.Plugin interface to provide
 // a template based, backend-agnostic router.
 type UniqueHost struct {
-	plugin       router.Plugin
-	hostForRoute RouteHostFunc
+	plugin router.Plugin
 
 	recorder RejectionRecorder
 
@@ -43,14 +42,13 @@ type UniqueHost struct {
 // NewUniqueHost creates a plugin wrapper that ensures only unique routes are passed into
 // the underlying plugin. Recorder is an interface for indicating why a route was
 // rejected.
-func NewUniqueHost(plugin router.Plugin, fn RouteHostFunc, disableOwnershipCheck bool, recorder RejectionRecorder) *UniqueHost {
+func NewUniqueHost(plugin router.Plugin, disableOwnershipCheck bool, recorder RejectionRecorder) *UniqueHost {
 	routeActivationFn := hostindex.SameNamespace
 	if disableOwnershipCheck {
 		routeActivationFn = hostindex.OldestFirst
 	}
 	return &UniqueHost{
-		plugin:       plugin,
-		hostForRoute: fn,
+		plugin: plugin,
 
 		recorder: recorder,
 
@@ -92,15 +90,14 @@ func (p *UniqueHost) HandleRoute(eventType watch.EventType, route *routeapi.Rout
 	}
 
 	routeName := routeNameKey(route)
+	host := route.Spec.Host
 
-	host := p.hostForRoute(route)
 	if len(host) == 0 {
 		glog.V(4).Infof("Route %s has no host value", routeName)
 		p.recorder.RecordRouteRejection(route, "NoHostValue", "no host value was defined for the route")
 		p.plugin.HandleRoute(watch.Deleted, route)
 		return nil
 	}
-	route.Spec.Host = host
 
 	// Validate that the route host name conforms to DNS requirements.
 	// Defends against routes created before validation rules were added for host names.

pkg/router/template/plugin_test.go

@@ -332,7 +332,7 @@ func TestHandleEndpoints(t *testing.T) {
 	templatePlugin := newDefaultTemplatePlugin(router, true, nil)
 	// TODO: move tests that rely on unique hosts to pkg/router/controller and remove them from
 	// here
-	plugin := controller.NewUniqueHost(templatePlugin, controller.HostForRoute, false, controller.LogRejections)
+	plugin := controller.NewUniqueHost(templatePlugin, false, controller.LogRejections)
 
 	for _, tc := range testCases {
 		plugin.HandleEndpoints(tc.eventType, tc.endpoints)
@@ -442,7 +442,7 @@ func TestHandleTCPEndpoints(t *testing.T) {
 	templatePlugin := newDefaultTemplatePlugin(router, false, nil)
 	// TODO: move tests that rely on unique hosts to pkg/router/controller and remove them from
 	// here
-	plugin := controller.NewUniqueHost(templatePlugin, controller.HostForRoute, false, controller.LogRejections)
+	plugin := controller.NewUniqueHost(templatePlugin, false, controller.LogRejections)
 
 	for _, tc := range testCases {
 		plugin.HandleEndpoints(tc.eventType, tc.endpoints)
@@ -484,7 +484,7 @@ func TestHandleRoute(t *testing.T) {
 	templatePlugin := newDefaultTemplatePlugin(router, true, nil)
 	// TODO: move tests that rely on unique hosts to pkg/router/controller and remove them from
 	// here
-	plugin := controller.NewUniqueHost(templatePlugin, controller.HostForRoute, false, rejections)
+	plugin := controller.NewUniqueHost(templatePlugin, false, rejections)
 
 	uidCount := 0
 	nextUID := func() types.UID {
@@ -1033,7 +1033,7 @@ func TestNamespaceScopingFromEmpty(t *testing.T) {
 	templatePlugin := newDefaultTemplatePlugin(router, true, nil)
 	// TODO: move tests that rely on unique hosts to pkg/router/controller and remove them from
 	// here
-	plugin := controller.NewUniqueHost(templatePlugin, controller.HostForRoute, false, controller.LogRejections)
+	plugin := controller.NewUniqueHost(templatePlugin, false, controller.LogRejections)
 
 	// no namespaces allowed
 	plugin.HandleNamespaces(sets.String{})