WIP: Add HAProxy Config Validator to the TemplatePlugin

Swap TemplatePlugin and StatusAdmitter ordering so that StatusAdmitter
is the last plugin so that the TemplatePlugin can reject a route based
on whether HAProxy rejects the configuration.

When TemplatePlugin adds a route, it also writes the config file without
reloading, and runs the configuration check script. If the configuration
check script fails with the new route, the route will be removed from the
state and it will be rejected.

images/router/haproxy/Dockerfile

@@ -17,5 +17,6 @@ USER 1001
 EXPOSE 80 443
 WORKDIR /var/lib/haproxy/conf
 ENV TEMPLATE_FILE=/var/lib/haproxy/conf/haproxy-config.template \
-    RELOAD_SCRIPT=/var/lib/haproxy/reload-haproxy
+    RELOAD_SCRIPT=/var/lib/haproxy/reload-haproxy \
+    CONFIG_CHECK_SCRIPT=/var/lib/haproxy/check-haproxy
 ENTRYPOINT ["/usr/bin/openshift-router", "--v=2"]

images/router/haproxy/check-haproxy

@@ -0,0 +1,7 @@
+#!/bin/bash
+
+config_file=/var/lib/haproxy/conf/haproxy.config
+if [[ "$1" != "" ]]; then
+  config_file=$1
+fi
+/usr/sbin/haproxy -c -f $config_file

pkg/cmd/infra/router/template.go

@@ -108,6 +108,8 @@ type TemplateRouterOptions struct {
 type TemplateRouter struct {
 	WorkingDir                          string
 	TemplateFile                        string
+	RouteConfigCheck                    bool
+	ConfigCheckScript                   string
 	ReloadScript                        string
 	ReloadInterval                      time.Duration
 	DefaultCertificate                  string
@@ -171,6 +173,8 @@ func (o *TemplateRouter) Bind(flag *pflag.FlagSet) {
 	flag.StringVar(&o.DefaultDestinationCAPath, "default-destination-ca-path", env("DEFAULT_DESTINATION_CA_PATH", ""), "A path to a PEM file containing the default CA bundle to use with re-encrypt routes. This CA should sign for certificates in the Kubernetes DNS space (service.namespace.svc).")
 	flag.StringVar(&o.TemplateFile, "template", env("TEMPLATE_FILE", ""), "The path to the template file to use")
 	flag.StringVar(&o.ReloadScript, "reload", env("RELOAD_SCRIPT", ""), "The path to the reload script to use")
+	flag.BoolVar(&o.RouteConfigCheck, "route-config-check", isTrue(env("ROUTE_CONFIG_CHECK", "true")), "Use configuration check script before adding routes")
+	flag.StringVar(&o.ConfigCheckScript, "check-script", env("CONFIG_CHECK_SCRIPT", ""), "The path to the config check script to use")
 	flag.DurationVar(&o.ReloadInterval, "interval", getIntervalFromEnv("RELOAD_INTERVAL", defaultReloadInterval), "Controls how often router reloads are invoked. Mutiple router reload requests are coalesced for the duration of this interval since the last reload time.")
 	flag.BoolVar(&o.BindPortsAfterSync, "bind-ports-after-sync", env("ROUTER_BIND_PORTS_AFTER_SYNC", "") == "true", "Bind ports only after route state has been synchronized")
 	flag.StringVar(&o.MaxConnections, "max-connections", env("ROUTER_MAX_CONNECTIONS", ""), "Specifies the maximum number of concurrent connections.")
@@ -552,6 +556,9 @@ func (o *TemplateRouterOptions) Validate() error {
 	if len(o.ReloadScript) == 0 {
 		return errors.New("reload script must be specified")
 	}
+	if o.RouteConfigCheck && len(o.ConfigCheckScript) == 0 {
+		return errors.New("config check script must be specified if route config checks are enabled")
+	}
 	return nil
 }
 
@@ -753,9 +760,31 @@ func (o *TemplateRouterOptions) Run(stopCh <-chan struct{}) error {
 
 	secretManager := secretmanager.NewManager(kc, nil)
 
+	factory := o.RouterSelection.NewFactory(routeclient, projectclient.ProjectV1().Projects(), kc)
+	factory.RouteModifierFn = o.RouteUpdate
+
+	var plugin router.Plugin
+	var recorder controller.RouteStatusRecorder = controller.LogRejections
+	if o.UpdateStatus {
+		lease := writerlease.New(time.Minute, 3*time.Second)
+		go lease.Run(stopCh)
+		informer := factory.CreateRoutesSharedInformer()
+		tracker := controller.NewSimpleContentionTracker(informer, o.RouterName, o.ResyncInterval/10)
+		tracker.SetConflictMessage(fmt.Sprintf("The router detected another process is writing conflicting updates to route status with name %q. Please ensure that the configuration of all routers is consistent. Route status will not be updated as long as conflicts are detected.", o.RouterName))
+		go tracker.Run(stopCh)
+		routeLister := routelisters.NewRouteLister(informer.GetIndexer())
+		status := controller.NewStatusAdmitter(routeclient.RouteV1(), routeLister, o.RouterName, o.RouterCanonicalHostname, lease, tracker)
+		recorder = status
+		plugin = status
+	}
+
 	pluginCfg := templateplugin.TemplatePluginConfig{
+		Plugin:                        plugin,
+		Recorder:                      recorder,
 		WorkingDir:                    o.WorkingDir,
 		TemplatePath:                  o.TemplateFile,
+		RouteConfigCheck:              o.RouteConfigCheck,
+		ConfigCheckScriptPath:         o.ConfigCheckScript,
 		ReloadScriptPath:              o.ReloadScript,
 		ReloadInterval:                o.ReloadInterval,
 		ReloadCallbacks:               reloadCallbacks,
@@ -789,25 +818,9 @@ func (o *TemplateRouterOptions) Run(stopCh <-chan struct{}) error {
 	if err != nil {
 		return err
 	}
+	plugin = templatePlugin
 	ptrTemplatePlugin = templatePlugin
 
-	factory := o.RouterSelection.NewFactory(routeclient, projectclient.ProjectV1().Projects(), kc)
-	factory.RouteModifierFn = o.RouteUpdate
-
-	var plugin router.Plugin = templatePlugin
-	var recorder controller.RouteStatusRecorder = controller.LogRejections
-	if o.UpdateStatus {
-		lease := writerlease.New(time.Minute, 3*time.Second)
-		go lease.Run(stopCh)
-		informer := factory.CreateRoutesSharedInformer()
-		tracker := controller.NewSimpleContentionTracker(informer, o.RouterName, o.ResyncInterval/10)
-		tracker.SetConflictMessage(fmt.Sprintf("The router detected another process is writing conflicting updates to route status with name %q. Please ensure that the configuration of all routers is consistent. Route status will not be updated as long as conflicts are detected.", o.RouterName))
-		go tracker.Run(stopCh)
-		routeLister := routelisters.NewRouteLister(informer.GetIndexer())
-		status := controller.NewStatusAdmitter(plugin, routeclient.RouteV1(), routeLister, o.RouterName, o.RouterCanonicalHostname, lease, tracker)
-		recorder = status
-		plugin = status
-	}
 	if o.UpgradeValidation {
 		plugin = controller.NewUpgradeValidation(plugin, recorder, o.UpgradeValidationForceAddCondition, o.UpgradeValidationForceRemoveCondition)
 	}

pkg/router/controller/status.go

@@ -17,7 +17,6 @@ import (
 	routev1 "github.com/openshift/api/route/v1"
 	client "github.com/openshift/client-go/route/clientset/versioned/typed/route/v1"
 	routelisters "github.com/openshift/client-go/route/listers/route/v1"
-	"github.com/openshift/router/pkg/router"
 	"github.com/openshift/router/pkg/router/writerlease"
 )
 
@@ -59,7 +58,6 @@ func (logRecorder) RecordRouteUnservableInFutureVersionsClear(route *routev1.Rou
 
 // StatusAdmitter ensures routes added to the plugin have status set.
 type StatusAdmitter struct {
-	plugin router.Plugin
 	client client.RoutesGetter
 	lister routelisters.RouteLister
 
@@ -74,9 +72,8 @@ type StatusAdmitter struct {
 // route has a status field set that matches this router. The admitter manages
 // an LRU of recently seen conflicting updates to handle when two router processes
 // with differing configurations are writing updates at the same time.
-func NewStatusAdmitter(plugin router.Plugin, client client.RoutesGetter, lister routelisters.RouteLister, name, hostName string, lease writerlease.Lease, tracker ContentionTracker) *StatusAdmitter {
+func NewStatusAdmitter(client client.RoutesGetter, lister routelisters.RouteLister, name, hostName string, lease writerlease.Lease, tracker ContentionTracker) *StatusAdmitter {
 	return &StatusAdmitter{
-		plugin: plugin,
 		client: client,
 		lister: lister,
 
@@ -107,23 +104,23 @@ func (a *StatusAdmitter) HandleRoute(eventType watch.EventType, route *routev1.R
 			Status: corev1.ConditionTrue,
 		})
 	}
-	return a.plugin.HandleRoute(eventType, route)
+	return nil
 }
 
 func (a *StatusAdmitter) HandleNode(eventType watch.EventType, node *kapi.Node) error {
-	return a.plugin.HandleNode(eventType, node)
+	return nil
 }
 
 func (a *StatusAdmitter) HandleEndpoints(eventType watch.EventType, route *kapi.Endpoints) error {
-	return a.plugin.HandleEndpoints(eventType, route)
+	return nil
 }
 
 func (a *StatusAdmitter) HandleNamespaces(namespaces sets.String) error {
-	return a.plugin.HandleNamespaces(namespaces)
+	return nil
 }
 
 func (a *StatusAdmitter) Commit() error {
-	return a.plugin.Commit()
+	return nil
 }
 
 // RecordRouteRejection attempts to update the route status with a reason for a route being rejected.

pkg/router/controller/status_test.go

@@ -150,7 +150,6 @@ func (t *fakeTracker) Clear(id contentionKey, ingress *routev1.RouteIngress) {
 func TestStatusNoOp(t *testing.T) {
 	now := nowFn()
 	touched := metav1.Time{Time: now.Add(-time.Minute)}
-	p := &fakePlugin{}
 	c := fake.NewSimpleClientset()
 	tracker := &fakeTracker{}
 	route := &routev1.Route{
@@ -174,7 +173,7 @@ func TestStatusNoOp(t *testing.T) {
 		},
 	}
 	lister := &routeLister{items: []*routev1.Route{route}}
-	admitter := NewStatusAdmitter(p, c.RouteV1(), lister, "test", "a.b.c.d", noopLease{}, tracker)
+	admitter := NewStatusAdmitter(c.RouteV1(), lister, "test", "a.b.c.d", noopLease{}, tracker)
 	err := admitter.HandleRoute(watch.Added, route)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -220,7 +219,6 @@ func TestStatusResetsHost(t *testing.T) {
 	now := metav1.Now()
 	nowFn = func() metav1.Time { return now }
 	touched := metav1.Time{Time: now.Add(-time.Minute)}
-	p := &fakePlugin{}
 	c := fake.NewSimpleClientset(&routev1.Route{ObjectMeta: metav1.ObjectMeta{Name: "route1", Namespace: "default", UID: types.UID("uid1")}})
 	tracker := &fakeTracker{}
 	route := &routev1.Route{
@@ -243,7 +241,7 @@ func TestStatusResetsHost(t *testing.T) {
 		},
 	}
 	lister := &routeLister{items: []*routev1.Route{route}}
-	admitter := NewStatusAdmitter(p, c.RouteV1(), lister, "test", "", noopLease{}, tracker)
+	admitter := NewStatusAdmitter(c.RouteV1(), lister, "test", "", noopLease{}, tracker)
 	err := admitter.HandleRoute(watch.Added, route)
 
 	route = checkResult(t, err, c, admitter, "route1.test.local", now, &now.Time, 0, 0)
@@ -269,7 +267,6 @@ func TestStatusAdmitsRouteOnForbidden(t *testing.T) {
 	now := nowFn()
 	nowFn = func() metav1.Time { return now }
 	touched := metav1.Time{Time: now.Add(-time.Minute)}
-	p := &fakePlugin{}
 	c := fake.NewSimpleClientset(&routev1.Route{ObjectMeta: metav1.ObjectMeta{Name: "route1", Namespace: "default", UID: types.UID("uid1")}})
 	c.PrependReactor("update", "routes", func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) {
 		if action.GetSubresource() != "status" {
@@ -298,7 +295,7 @@ func TestStatusAdmitsRouteOnForbidden(t *testing.T) {
 		},
 	}
 	lister := &routeLister{items: []*routev1.Route{route}}
-	admitter := NewStatusAdmitter(p, c.RouteV1(), lister, "test", "", noopLease{}, tracker)
+	admitter := NewStatusAdmitter(c.RouteV1(), lister, "test", "", noopLease{}, tracker)
 	err := admitter.HandleRoute(watch.Added, route)
 	route = checkResult(t, err, c, admitter, "route1.test.local", now, &touched.Time, 0, 0)
 	ingress := findIngressForRoute(route, "test")
@@ -314,7 +311,6 @@ func TestStatusBackoffOnConflict(t *testing.T) {
 	now := nowFn()
 	nowFn = func() metav1.Time { return now }
 	touched := metav1.Time{Time: now.Add(-time.Minute)}
-	p := &fakePlugin{}
 	c := fake.NewSimpleClientset(&routev1.Route{ObjectMeta: metav1.ObjectMeta{Name: "route1", Namespace: "default", UID: types.UID("uid1")}})
 	c.PrependReactor("update", "routes", func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) {
 		if action.GetSubresource() != "status" {
@@ -343,23 +339,22 @@ func TestStatusBackoffOnConflict(t *testing.T) {
 		},
 	}
 	lister := &routeLister{items: []*routev1.Route{route}}
-	admitter := NewStatusAdmitter(p, c.RouteV1(), lister, "test", "", noopLease{}, tracker)
+	admitter := NewStatusAdmitter(c.RouteV1(), lister, "test", "", noopLease{}, tracker)
 	err := admitter.HandleRoute(watch.Added, route)
 	checkResult(t, err, c, admitter, "route1.test.local", now, nil, 0, 0)
 }
 
 func TestStatusRecordRejection(t *testing.T) {
 	now := nowFn()
 	nowFn = func() metav1.Time { return now }
-	p := &fakePlugin{}
 	c := fake.NewSimpleClientset(&routev1.Route{ObjectMeta: metav1.ObjectMeta{Name: "route1", Namespace: "default", UID: types.UID("uid1")}})
 	tracker := &fakeTracker{}
 	route := &routev1.Route{
 		ObjectMeta: metav1.ObjectMeta{Name: "route1", Namespace: "default", UID: types.UID("uid1")},
 		Spec:       routev1.RouteSpec{Host: "route1.test.local"},
 	}
 	lister := &routeLister{items: []*routev1.Route{route}}
-	admitter := NewStatusAdmitter(p, c.RouteV1(), lister, "test", "", noopLease{}, tracker)
+	admitter := NewStatusAdmitter(c.RouteV1(), lister, "test", "", noopLease{}, tracker)
 	admitter.RecordRouteRejection(route, "Failed", "generic error")
 
 	if len(c.Actions()) != 1 {
@@ -383,7 +378,6 @@ func TestStatusRecordRejectionNoChange(t *testing.T) {
 	now := nowFn()
 	nowFn = func() metav1.Time { return now }
 	touched := metav1.Time{Time: now.Add(-time.Minute)}
-	p := &fakePlugin{}
 	c := fake.NewSimpleClientset(&routev1.Route{ObjectMeta: metav1.ObjectMeta{Name: "route1", Namespace: "default", UID: types.UID("uid1")}})
 	tracker := &fakeTracker{}
 	route := &routev1.Route{
@@ -408,7 +402,7 @@ func TestStatusRecordRejectionNoChange(t *testing.T) {
 		},
 	}
 	lister := &routeLister{items: []*routev1.Route{route}}
-	admitter := NewStatusAdmitter(p, c.RouteV1(), lister, "test", "", noopLease{}, tracker)
+	admitter := NewStatusAdmitter(c.RouteV1(), lister, "test", "", noopLease{}, tracker)
 	admitter.RecordRouteRejection(route, "Failed", "generic error")
 
 	if len(c.Actions()) != 0 {
@@ -420,7 +414,6 @@ func TestStatusRecordRejectionWithStatus(t *testing.T) {
 	now := nowFn()
 	nowFn = func() metav1.Time { return now }
 	touched := metav1.Time{Time: now.Add(-time.Minute)}
-	p := &fakePlugin{}
 	c := fake.NewSimpleClientset(&routev1.Route{ObjectMeta: metav1.ObjectMeta{Name: "route1", Namespace: "default", UID: types.UID("uid1")}})
 	tracker := &fakeTracker{}
 	route := &routev1.Route{
@@ -443,7 +436,7 @@ func TestStatusRecordRejectionWithStatus(t *testing.T) {
 		},
 	}
 	lister := &routeLister{items: []*routev1.Route{route}}
-	admitter := NewStatusAdmitter(p, c.RouteV1(), lister, "test", "", noopLease{}, tracker)
+	admitter := NewStatusAdmitter(c.RouteV1(), lister, "test", "", noopLease{}, tracker)
 	admitter.RecordRouteRejection(route, "Failed", "generic error")
 
 	if len(c.Actions()) != 1 {
@@ -467,7 +460,6 @@ func TestStatusRecordRejectionOnHostUpdateOnly(t *testing.T) {
 	now := nowFn()
 	nowFn = func() metav1.Time { return now }
 	touched := metav1.Time{Time: now.Add(-time.Minute)}
-	p := &fakePlugin{}
 	c := fake.NewSimpleClientset(&routev1.Route{ObjectMeta: metav1.ObjectMeta{Name: "route1", Namespace: "default", UID: types.UID("uid1")}})
 	tracker := &fakeTracker{}
 	route := &routev1.Route{
@@ -492,7 +484,7 @@ func TestStatusRecordRejectionOnHostUpdateOnly(t *testing.T) {
 		},
 	}
 	lister := &routeLister{items: []*routev1.Route{route}}
-	admitter := NewStatusAdmitter(p, c.RouteV1(), lister, "test", "", noopLease{}, tracker)
+	admitter := NewStatusAdmitter(c.RouteV1(), lister, "test", "", noopLease{}, tracker)
 	admitter.RecordRouteRejection(route, "Failed", "generic error")
 
 	if len(c.Actions()) != 1 {
@@ -519,7 +511,6 @@ func TestStatusRecordRejectionConflict(t *testing.T) {
 	now := nowFn()
 	nowFn = func() metav1.Time { return now }
 	touched := metav1.Time{Time: now.Add(-time.Minute)}
-	p := &fakePlugin{}
 	c := fake.NewSimpleClientset(&routev1.Route{ObjectMeta: metav1.ObjectMeta{Name: "route1", Namespace: "default", UID: types.UID("uid1")}})
 	c.PrependReactor("update", "routes", func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) {
 		if action.GetSubresource() != "status" {
@@ -548,7 +539,7 @@ func TestStatusRecordRejectionConflict(t *testing.T) {
 		},
 	}
 	lister := &routeLister{items: []*routev1.Route{route}}
-	admitter := NewStatusAdmitter(p, c.RouteV1(), lister, "test", "", noopLease{}, tracker)
+	admitter := NewStatusAdmitter(c.RouteV1(), lister, "test", "", noopLease{}, tracker)
 	admitter.RecordRouteRejection(route, "Failed", "generic error")
 
 	if len(c.Actions()) != 1 {
@@ -569,7 +560,6 @@ func TestStatusRecordRejectionConflict(t *testing.T) {
 }
 
 func TestStatusFightBetweenReplicas(t *testing.T) {
-	p := &fakePlugin{}
 	stopCh := make(chan struct{})
 	defer close(stopCh)
 
@@ -584,7 +574,7 @@ func TestStatusFightBetweenReplicas(t *testing.T) {
 		Status:     routev1.RouteStatus{},
 	}
 	lister1 := &routeLister{items: []*routev1.Route{route1}}
-	admitter1 := NewStatusAdmitter(p, c1.RouteV1(), lister1, "test", "", noopLease{}, tracker1)
+	admitter1 := NewStatusAdmitter(c1.RouteV1(), lister1, "test", "", noopLease{}, tracker1)
 	err := admitter1.HandleRoute(watch.Added, route1)
 
 	outObj1 := checkResult(t, err, c1, admitter1, "route1.test.local", now1, &now1.Time, 0, 0)
@@ -599,7 +589,7 @@ func TestStatusFightBetweenReplicas(t *testing.T) {
 	c2 := fake.NewSimpleClientset(&routev1.Route{ObjectMeta: metav1.ObjectMeta{Name: "route1", Namespace: "default", UID: types.UID("uid1")}})
 	tracker2 := &fakeTracker{}
 	lister2 := &routeLister{items: []*routev1.Route{outObj1}}
-	admitter2 := NewStatusAdmitter(p, c2.RouteV1(), lister2, "test", "", noopLease{}, tracker2)
+	admitter2 := NewStatusAdmitter(c2.RouteV1(), lister2, "test", "", noopLease{}, tracker2)
 	outObj1.Spec.Host = "route1.test-new.local"
 	err = admitter2.HandleRoute(watch.Added, outObj1)
 
@@ -628,8 +618,6 @@ func TestStatusFightBetweenReplicas(t *testing.T) {
 }
 
 func TestStatusFightBetweenRouters(t *testing.T) {
-	p := &fakePlugin{}
-
 	// initial try, results in conflict
 	now1 := metav1.Now()
 	nowFn = func() metav1.Time { return now1 }
@@ -678,7 +666,7 @@ func TestStatusFightBetweenRouters(t *testing.T) {
 		},
 	}
 	lister1 := &routeLister{items: []*routev1.Route{route1}}
-	admitter1 := NewStatusAdmitter(p, c1.RouteV1(), lister1, "test2", "", noopLease{}, tracker)
+	admitter1 := NewStatusAdmitter(c1.RouteV1(), lister1, "test2", "", noopLease{}, tracker)
 	err := admitter1.HandleRoute(watch.Added, route1)
 
 	checkResult(t, err, c1, admitter1, "route2.test-new.local", now1, nil, 1, 0)
@@ -771,7 +759,6 @@ func makePass(t *testing.T, host string, admitter *StatusAdmitter, srcObj *route
 }
 
 func TestRouterContention(t *testing.T) {
-	p := &fakePlugin{}
 	stopCh := make(chan struct{})
 	defer close(stopCh)
 
@@ -789,7 +776,7 @@ func TestRouterContention(t *testing.T) {
 	t1 := NewSimpleContentionTracker(i1, "test", time.Minute)
 	lister1 := &routeLister{}
 
-	r1 := NewStatusAdmitter(p, nil, lister1, "test", "", noopLease{}, t1)
+	r1 := NewStatusAdmitter(nil, lister1, "test", "", noopLease{}, t1)
 
 	// update
 	currObj := makePass(t, "route1.test.local", r1, initObj, true, false)
@@ -1352,11 +1339,10 @@ func TestStatusUnservableInFutureVersions(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			now := nowFn()
 			nowFn = func() metav1.Time { return now }
-			p := &fakePlugin{}
 			c := fake.NewSimpleClientset(tc.route)
 			tracker := &fakeTracker{}
 			lister := &routeLister{items: []*routev1.Route{tc.route}}
-			admitter := NewStatusAdmitter(p, c.RouteV1(), lister, tc.routerName, "", noopLease{}, tracker)
+			admitter := NewStatusAdmitter(c.RouteV1(), lister, tc.routerName, "", noopLease{}, tracker)
 			if tc.unservableInFutureVersions {
 				admitter.RecordRouteUnservableInFutureVersions(tc.route, unservableInFutureVersionsTrueCondition.Reason, unservableInFutureVersionsTrueCondition.Message)
 			} else {