Skip to content

Commit

Permalink
WIP: Add HAProxy Config Validator to the TemplatePlugin
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
gcs278 committed Nov 27, 2024
1 parent a405953 commit 8d03c79
Show file tree
Hide file tree
Showing 9 changed files with 143 additions and 74 deletions.
3 changes: 2 additions & 1 deletion images/router/haproxy/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
7 changes: 7 additions & 0 deletions images/router/haproxy/check-haproxy
Original file line number Diff line number Diff line change
@@ -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
47 changes: 30 additions & 17 deletions pkg/cmd/infra/router/template.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.")
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
}
Expand Down
15 changes: 6 additions & 9 deletions pkg/router/controller/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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

Expand All @@ -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,

Expand Down Expand Up @@ -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.
Expand Down
42 changes: 14 additions & 28 deletions pkg/router/controller/status_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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)
Expand Down Expand Up @@ -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{
Expand All @@ -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)
Expand All @@ -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" {
Expand Down Expand Up @@ -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")
Expand All @@ -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" {
Expand Down Expand Up @@ -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 {
Expand All @@ -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{
Expand All @@ -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 {
Expand All @@ -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{
Expand All @@ -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 {
Expand All @@ -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{
Expand All @@ -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 {
Expand All @@ -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" {
Expand Down Expand Up @@ -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 {
Expand All @@ -569,7 +560,6 @@ func TestStatusRecordRejectionConflict(t *testing.T) {
}

func TestStatusFightBetweenReplicas(t *testing.T) {
p := &fakePlugin{}
stopCh := make(chan struct{})
defer close(stopCh)

Expand All @@ -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)
Expand All @@ -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)

Expand Down Expand Up @@ -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 }
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)

Expand All @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down
Loading

0 comments on commit 8d03c79

Please sign in to comment.