diff --git a/images/router/haproxy/Dockerfile b/images/router/haproxy/Dockerfile index c254d75d9..2c54cd321 100644 --- a/images/router/haproxy/Dockerfile +++ b/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"] diff --git a/images/router/haproxy/check-haproxy b/images/router/haproxy/check-haproxy new file mode 100755 index 000000000..11b63d235 --- /dev/null +++ b/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 diff --git a/pkg/cmd/infra/router/template.go b/pkg/cmd/infra/router/template.go index 398a07f08..da7f30aa2 100644 --- a/pkg/cmd/infra/router/template.go +++ b/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", "")), "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) } diff --git a/pkg/router/controller/status.go b/pkg/router/controller/status.go index 9703c63ea..791ecd8f6 100644 --- a/pkg/router/controller/status.go +++ b/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. diff --git a/pkg/router/controller/status_test.go b/pkg/router/controller/status_test.go index 2eb739c39..6a4367592 100644 --- a/pkg/router/controller/status_test.go +++ b/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,7 +339,7 @@ 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) } @@ -351,7 +347,6 @@ func TestStatusBackoffOnConflict(t *testing.T) { 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{ @@ -359,7 +354,7 @@ func TestStatusRecordRejection(t *testing.T) { 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 { diff --git a/pkg/router/router_test.go b/pkg/router/router_test.go index dc188c0a3..aecbd6715 100644 --- a/pkg/router/router_test.go +++ b/pkg/router/router_test.go @@ -113,9 +113,13 @@ func TestMain(m *testing.M) { createRouterDirs() + statusPlugin := controller.NewStatusAdmitter(routeClient.RouteV1(), routeLister, "default", "example.com", lease, tracker) + // The template plugin which is wrapped svcFetcher := templateplugin.NewListWatchServiceLookup(client.CoreV1(), 60*time.Second, namespace) pluginCfg := templateplugin.TemplatePluginConfig{ + Plugin: statusPlugin, + Recorder: statusPlugin, WorkingDir: workdir, DefaultCertificate: `-----BEGIN CERTIFICATE----- MIIDIjCCAgqgAwIBAgIBBjANBgkqhkiG9w0BAQUFADCBoTELMAkGA1UEBhMCVVMx @@ -176,8 +180,6 @@ u3YLAbyW/lHhOCiZu2iAI8AbmXem9lW6Tr7p/97s0w== } // Wrap the template plugin with other stuff - statusPlugin := controller.NewStatusAdmitter(plugin, routeClient.RouteV1(), routeLister, "default", "example.com", lease, tracker) - plugin = statusPlugin plugin = controller.NewUniqueHost(plugin, routerSelection.DisableNamespaceOwnershipCheck, statusPlugin) plugin = controller.NewHostAdmitter(plugin, routerSelection.RouteAdmissionFunc(), false, false, statusPlugin) diff --git a/pkg/router/template/plugin.go b/pkg/router/template/plugin.go index 8d592ca28..47bcbb068 100644 --- a/pkg/router/template/plugin.go +++ b/pkg/router/template/plugin.go @@ -16,6 +16,8 @@ import ( "k8s.io/apimachinery/pkg/watch" routev1 "github.com/openshift/api/route/v1" + "github.com/openshift/router/pkg/router" + "github.com/openshift/router/pkg/router/controller" "github.com/openshift/library-go/pkg/route/secretmanager" unidlingapi "github.com/openshift/router/pkg/router/unidling" @@ -29,22 +31,30 @@ const ( // TemplatePlugin implements the router.Plugin interface to provide // a template based, backend-agnostic router. type TemplatePlugin struct { + Plugin router.Plugin Router RouterInterface IncludeUDP bool ServiceFetcher ServiceLookup + Recorder controller.RouteStatusRecorder } -func newDefaultTemplatePlugin(router RouterInterface, includeUDP bool, lookupSvc ServiceLookup) *TemplatePlugin { +func newDefaultTemplatePlugin(plugin router.Plugin, router RouterInterface, includeUDP bool, lookupSvc ServiceLookup, recorder controller.RouteStatusRecorder) *TemplatePlugin { return &TemplatePlugin{ + Plugin: plugin, Router: router, IncludeUDP: includeUDP, ServiceFetcher: lookupSvc, + Recorder: recorder, } } type TemplatePluginConfig struct { + Plugin router.Plugin + Recorder controller.RouteStatusRecorder WorkingDir string TemplatePath string + RouteConfigCheck bool + ConfigCheckScriptPath string ReloadScriptPath string ReloadFn func(shutdown bool) error ReloadInterval time.Duration @@ -93,7 +103,7 @@ type RouterInterface interface { DeleteEndpoints(id ServiceUnitKey) // AddRoute attempts to add a route to the router. - AddRoute(route *routev1.Route) + AddRoute(route *routev1.Route) error // RemoveRoute removes the given route RemoveRoute(route *routev1.Route) // HasRoute indicates whether the router is configured with the given route @@ -146,6 +156,8 @@ func NewTemplatePlugin(cfg TemplatePluginConfig, lookupSvc ServiceLookup) (*Temp templateRouterCfg := templateRouterCfg{ dir: cfg.WorkingDir, templates: templates, + routeConfigCheck: cfg.RouteConfigCheck, + checkScriptPath: cfg.ConfigCheckScriptPath, reloadScriptPath: cfg.ReloadScriptPath, reloadFn: cfg.ReloadFn, reloadInterval: cfg.ReloadInterval, @@ -169,7 +181,7 @@ func NewTemplatePlugin(cfg TemplatePluginConfig, lookupSvc ServiceLookup) (*Temp secretManager: cfg.SecretManager, } router, err := newTemplateRouter(templateRouterCfg) - return newDefaultTemplatePlugin(router, cfg.IncludeUDP, lookupSvc), err + return newDefaultTemplatePlugin(cfg.Plugin, router, cfg.IncludeUDP, lookupSvc, cfg.Recorder), err } // Stop instructs the router plugin to stop invoking the reload method, and waits until no further @@ -205,14 +217,14 @@ func (p *TemplatePlugin) HandleEndpoints(eventType watch.EventType, endpoints *k p.Router.DeleteEndpoints(key) } - return nil + return p.Plugin.HandleEndpoints(eventType, endpoints) } // HandleNode processes watch events on the Node resource // The template type of plugin currently does not need to act on such events // so the implementation just returns without error func (p *TemplatePlugin) HandleNode(eventType watch.EventType, node *kapi.Node) error { - return nil + return p.Plugin.HandleNode(eventType, node) } // HandleRoute processes watch events on the Route resource. @@ -223,19 +235,24 @@ func (p *TemplatePlugin) HandleRoute(eventType watch.EventType, route *routev1.R log.V(10).Info("HandleRoute: TemplatePlugin") switch eventType { case watch.Added, watch.Modified: - p.Router.AddRoute(route) + // Add route + if err := p.Router.AddRoute(route); err != nil { + log.V(0).Error(err, "failed to validate HAProxy config", "name", route.Name, "namespace", route.Namespace) + p.Recorder.RecordRouteRejection(route, "HAProxyCheckConfigFailed", "Failed to validate HAProxy config with route. Review the openshift-router logs for more details on the validation failure.") + return err + } case watch.Deleted: log.V(4).Info("deleting route", "namespace", route.Namespace, "name", route.Name) p.Router.RemoveRoute(route) } - return nil + return p.Plugin.HandleRoute(eventType, route) } // HandleNamespaces limits the scope of valid routes to only those that match // the provided namespace list. func (p *TemplatePlugin) HandleNamespaces(namespaces sets.String) error { p.Router.FilterNamespaces(namespaces) - return nil + return p.Plugin.HandleNamespaces(namespaces) } func (p *TemplatePlugin) Commit() error { diff --git a/pkg/router/template/plugin_test.go b/pkg/router/template/plugin_test.go index c61d09069..983f497ce 100644 --- a/pkg/router/template/plugin_test.go +++ b/pkg/router/template/plugin_test.go @@ -230,7 +230,7 @@ func (r *TestRouter) calculateServiceWeights(serviceUnits map[ServiceUnitKey]int } // AddRoute adds a ServiceAliasConfig and associated ServiceUnits for the route -func (r *TestRouter) AddRoute(route *routev1.Route) { +func (r *TestRouter) AddRoute(route *routev1.Route) error { routeKey := getKey(route) config := ServiceAliasConfig{ @@ -245,6 +245,7 @@ func (r *TestRouter) AddRoute(route *routev1.Route) { } r.State[routeKey] = config + return nil } // RemoveRoute removes the service alias config for Route @@ -300,6 +301,9 @@ func getKey(route *routev1.Route) ServiceAliasConfigKey { func (r *TestRouter) Commit() { // No op } +func (r *TestRouter) CheckConfig() error { + return nil +} // TestHandleEndpoints test endpoint watch events func TestHandleEndpoints(t *testing.T) { @@ -379,7 +383,7 @@ func TestHandleEndpoints(t *testing.T) { } router := newTestRouter(make(map[ServiceAliasConfigKey]ServiceAliasConfig)) - templatePlugin := newDefaultTemplatePlugin(router, true, nil) + templatePlugin := newDefaultTemplatePlugin(&fakePlugin{}, router, true, nil, nil) // TODO: move tests that rely on unique hosts to pkg/router/controller and remove them from // here plugin := controller.NewUniqueHost(templatePlugin, false, controller.LogRejections) @@ -489,7 +493,7 @@ func TestHandleTCPEndpoints(t *testing.T) { } router := newTestRouter(make(map[ServiceAliasConfigKey]ServiceAliasConfig)) - templatePlugin := newDefaultTemplatePlugin(router, false, nil) + templatePlugin := newDefaultTemplatePlugin(&fakePlugin{}, router, false, nil, nil) // TODO: move tests that rely on unique hosts to pkg/router/controller and remove them from // here plugin := controller.NewUniqueHost(templatePlugin, false, controller.LogRejections) @@ -553,7 +557,7 @@ func (r *fakeStatusRecorder) isUnservableInFutureVersions(route *routev1.Route) func TestHandleRoute(t *testing.T) { rejections := &fakeStatusRecorder{} router := newTestRouter(make(map[ServiceAliasConfigKey]ServiceAliasConfig)) - templatePlugin := newDefaultTemplatePlugin(router, true, nil) + templatePlugin := newDefaultTemplatePlugin(&fakePlugin{}, router, true, nil, rejections) // TODO: move tests that rely on unique hosts to pkg/router/controller and remove them from // here plugin := controller.NewUniqueHost(templatePlugin, false, rejections) @@ -1197,7 +1201,7 @@ func TestHandleRouteUpgradeValidation(t *testing.T) { func TestNamespaceScopingFromEmpty(t *testing.T) { router := newTestRouter(make(map[ServiceAliasConfigKey]ServiceAliasConfig)) - templatePlugin := newDefaultTemplatePlugin(router, true, nil) + templatePlugin := newDefaultTemplatePlugin(&fakePlugin{}, router, true, nil, nil) // TODO: move tests that rely on unique hosts to pkg/router/controller and remove them from // here plugin := controller.NewUniqueHost(templatePlugin, false, controller.LogRejections) diff --git a/pkg/router/template/router.go b/pkg/router/template/router.go index aeb968068..ae212cc2c 100644 --- a/pkg/router/template/router.go +++ b/pkg/router/template/router.go @@ -58,6 +58,8 @@ type templateRouter struct { // the directory to write router output to dir string templates map[string]*template.Template + routeConfigCheck bool + checkScriptPath string reloadScriptPath string reloadFn func(shutdown bool) error reloadInterval time.Duration @@ -136,6 +138,8 @@ type templateRouter struct { type templateRouterCfg struct { dir string templates map[string]*template.Template + routeConfigCheck bool + checkScriptPath string reloadScriptPath string reloadFn func(shutdown bool) error reloadInterval time.Duration @@ -249,6 +253,8 @@ func newTemplateRouter(cfg templateRouterCfg) (*templateRouter, error) { router := &templateRouter{ dir: dir, templates: cfg.templates, + routeConfigCheck: cfg.routeConfigCheck, + checkScriptPath: cfg.checkScriptPath, reloadScriptPath: cfg.reloadScriptPath, reloadInterval: cfg.reloadInterval, reloadCallbacks: cfg.reloadCallbacks, @@ -561,6 +567,24 @@ func (r *templateRouter) commitAndReload() error { return nil } +// configCheck writes the current config, then runs the config check script +// and returns an error if config fails validation. +func (r *templateRouter) configCheck() error { + if err := r.writeConfig(); err != nil { + // Fail open since we don't want to misinterpret write failures as validation failures. + log.V(0).Error(err, "failed to write config to run check") + return nil + } + + cmd := exec.Command(r.checkScriptPath, r.dir+"/conf/haproxy.config") + out, err := cmd.CombinedOutput() + if err != nil { + return fmt.Errorf("error validating haproxy config: %v\n%s", err, string(out)) + } + log.V(4).Info("router passed haproxy config check") + return nil +} + // writeConfig writes the config to disk // Must be called while holding r.lock func (r *templateRouter) writeConfig() error { @@ -1083,7 +1107,7 @@ func SanitizeHeaderValue(headerValue string) string { // AddRoute adds the given route to the router state if the route // hasn't been seen before or has changed since it was last seen. -func (r *templateRouter) AddRoute(route *routev1.Route) { +func (r *templateRouter) AddRoute(route *routev1.Route) error { backendKey := routeKey(route) newConfig := r.createServiceAliasConfig(route, backendKey) @@ -1095,7 +1119,7 @@ func (r *templateRouter) AddRoute(route *routev1.Route) { if existingConfig, exists := r.state[backendKey]; exists { if configsAreEqual(newConfig, &existingConfig) { - return + return nil } log.V(4).Info("updating route", "namespace", route.Namespace, "name", route.Name) @@ -1122,10 +1146,28 @@ func (r *templateRouter) AddRoute(route *routev1.Route) { } configChanged := r.dynamicallyAddRoute(backendKey, route, newConfig) - r.state[backendKey] = *newConfig - r.stateChanged = true r.dynamicallyConfigured = r.dynamicallyConfigured && configChanged + + // Perform a config check by writing the config file with the newly added route and executing the config check + // script. Since we have the lock, the router must wait before reloading. If validation fails, the route is + // removed, and the next reload restores the configuration to its previous working state and reloads HAProxy. + if r.routeConfigCheck && !r.dynamicallyConfigured { + if err := r.configCheck(); err != nil { + // HAProxy Validation failed with new route, remove the route. + r.removeRouteInternal(route) + if r.secretManager != nil { + if r.secretManager.IsRouteRegistered(route.Namespace, route.Name) { + if err := r.secretManager.UnregisterRoute(route.Namespace, route.Name); err != nil { + log.Error(err, "failed to unregister route") + } + } + } + return err + } + } + r.stateChanged = true + return nil } // RemoveRoute removes the given route