From 498080653a500d31c73aea58dfc2441ea9c04729 Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Tue, 26 Nov 2024 21:25:35 -0500 Subject: [PATCH] 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 | 3 +- images/router/haproxy/check-haproxy | 7 ++++ pkg/cmd/infra/router/template.go | 47 +++++++++++++++---------- pkg/router/controller/status.go | 15 ++++---- pkg/router/controller/status_test.go | 42 ++++++++--------------- pkg/router/router_test.go | 6 ++-- pkg/router/template/plugin.go | 51 ++++++++++++++++++++++------ pkg/router/template/plugin_test.go | 14 +++++--- pkg/router/template/router.go | 50 ++++++++++++++++++++++++--- 9 files changed, 159 insertions(+), 76 deletions(-) create mode 100755 images/router/haproxy/check-haproxy 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..fcf630652 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", "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) } 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..4c4aa15f4 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 @@ -204,15 +216,20 @@ func (p *TemplatePlugin) HandleEndpoints(eventType watch.EventType, endpoints *k log.V(4).Info("deleting endpoints", "key", key) p.Router.DeleteEndpoints(key) } - - return nil + if p.Plugin == nil { + 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 + if p.Plugin == nil { + return nil + } + return p.Plugin.HandleNode(eventType, node) } // HandleRoute processes watch events on the Route resource. @@ -223,24 +240,38 @@ 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 + if p.Plugin == nil { + 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 + if p.Plugin == nil { + return nil + } + return p.Plugin.HandleNamespaces(namespaces) } func (p *TemplatePlugin) Commit() error { p.Router.Commit() - return nil + if p.Plugin == nil { + return nil + } + return p.Plugin.Commit() } // endpointsKey returns the internal router key to use for the given Endpoints. 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