From 84b0071346861a46d109dfdce53ada9c0a4198c1 Mon Sep 17 00:00:00 2001 From: Greg Dubicki Date: Thu, 4 Jun 2020 15:57:53 +0200 Subject: [PATCH] Keep original server names in HAproxy backends to make the logs more useful --- consul/config.go | 9 ++-- consul/watcher.go | 16 ++++--- haproxy/dataplane/backend.go | 8 ++-- haproxy/state/apply.go | 2 +- haproxy/state/apply_backend_test.go | 40 ++++++++--------- haproxy/state/fake_ha_test.go | 4 +- haproxy/state/snapshot_test.go | 70 ++++++++++++++++------------- haproxy/state/states.go | 2 +- haproxy/state/upstream.go | 9 ++-- 9 files changed, 86 insertions(+), 74 deletions(-) diff --git a/consul/config.go b/consul/config.go index d504f4c..320bd41 100644 --- a/consul/config.go +++ b/consul/config.go @@ -32,13 +32,14 @@ func (n Upstream) Equal(o Upstream) bool { } type UpstreamNode struct { - Host string - Port int - Weight int + Name string + Address string + Port int + Weight int } func (n UpstreamNode) ID() string { - return fmt.Sprintf("%s:%d", n.Host, n.Port) + return fmt.Sprintf("%s:%d", n.Address, n.Port) } func (n UpstreamNode) Equal(o UpstreamNode) bool { diff --git a/consul/watcher.go b/consul/watcher.go index 6211007..fc848ad 100644 --- a/consul/watcher.go +++ b/consul/watcher.go @@ -454,9 +454,12 @@ func (w *Watcher) genCfg() Config { } for _, s := range up.Nodes { serviceInstancesTotal++ - host := s.Service.Address - if host == "" { - host = s.Node.Address + + name := s.Node.Node + + address := s.Service.Address + if address == "" { + address = s.Node.Address } weight := 1 @@ -474,9 +477,10 @@ func (w *Watcher) genCfg() Config { serviceInstancesAlive++ upstream.Nodes = append(upstream.Nodes, UpstreamNode{ - Host: host, - Port: s.Service.Port, - Weight: weight, + Name: name, + Address: address, + Port: s.Service.Port, + Weight: weight, }) } diff --git a/haproxy/dataplane/backend.go b/haproxy/dataplane/backend.go index 3606c04..6fbfd86 100644 --- a/haproxy/dataplane/backend.go +++ b/haproxy/dataplane/backend.go @@ -58,15 +58,15 @@ func (t *tnx) CreateServer(beName string, srv models.Server) error { return t.client.makeReq(http.MethodPost, fmt.Sprintf("/v1/services/haproxy/configuration/servers?backend=%s&transaction_id=%s", beName, t.txID), srv, nil) } -func (t *tnx) ReplaceServer(beName string, srv models.Server) error { +func (t *tnx) ReplaceServer(beName string, oldSrvName string, newSrv models.Server) error { t.After(func() error { - return t.client.ReplaceServer(beName, srv) + return t.client.ReplaceServer(beName, oldSrvName, newSrv) }) return nil } -func (c *Dataplane) ReplaceServer(beName string, srv models.Server) error { - err := c.makeReq(http.MethodPut, fmt.Sprintf("/v1/services/haproxy/configuration/servers/%s?backend=%s&version=%d", srv.Name, beName, c.version), srv, nil) +func (c *Dataplane) ReplaceServer(beName string, oldSrvName string, newSrv models.Server) error { + err := c.makeReq(http.MethodPut, fmt.Sprintf("/v1/services/haproxy/configuration/servers/%s?backend=%s&version=%d", oldSrvName, beName, c.version), newSrv, nil) if err != nil { return err } diff --git a/haproxy/state/apply.go b/haproxy/state/apply.go index 58710a9..3deef0c 100644 --- a/haproxy/state/apply.go +++ b/haproxy/state/apply.go @@ -151,7 +151,7 @@ func applyBackends(ha HAProxy, old, new []Backend) error { continue } - err := ha.ReplaceServer(newBack.Backend.Name, s) + err := ha.ReplaceServer(newBack.Backend.Name, oldServers[i].Name, s) if err != nil { return err } diff --git a/haproxy/state/apply_backend_test.go b/haproxy/state/apply_backend_test.go index 0129f6e..a00203b 100644 --- a/haproxy/state/apply_backend_test.go +++ b/haproxy/state/apply_backend_test.go @@ -65,7 +65,7 @@ func TestNoChangeBackend(t *testing.T) { }, Servers: []models.Server{ models.Server{ - Name: "srv_0", + Name: "some-server", Address: "1.2.3.4", Port: int64p(8080), Maintenance: models.ServerMaintenanceDisabled, @@ -82,7 +82,7 @@ func TestNoChangeBackend(t *testing.T) { }, Servers: []models.Server{ models.Server{ - Name: "srv_0", + Name: "some-server", Address: "1.2.3.4", Port: int64p(8080), Maintenance: models.ServerMaintenanceDisabled, @@ -138,7 +138,7 @@ func TestAddServerDifferentSize(t *testing.T) { }, Servers: []models.Server{ models.Server{ - Name: "srv_0", + Name: "some-server", Address: "1.2.3.4", Port: int64p(8080), Maintenance: models.ServerMaintenanceDisabled, @@ -156,7 +156,7 @@ func TestAddServerDifferentSize(t *testing.T) { ha.RequireOps(t, RequireOp(haOpDeleteBackend, "back"), RequireOp(haOpCreateBackend, "back"), - RequireOp(haOpCreateServer, "srv_0"), + RequireOp(haOpCreateServer, "some-server"), ) } @@ -169,13 +169,13 @@ func TestAddServerSameSize(t *testing.T) { }, Servers: []models.Server{ models.Server{ - Name: "srv_0", + Name: "some-server", Address: "1.2.3.4", Port: int64p(8080), Maintenance: models.ServerMaintenanceDisabled, }, models.Server{ - Name: "srv_1", + Name: "disabled_server_0", Address: "127.0.0.1", Port: int64p(1), Maintenance: models.ServerMaintenanceEnabled, @@ -192,13 +192,13 @@ func TestAddServerSameSize(t *testing.T) { }, Servers: []models.Server{ models.Server{ - Name: "srv_0", + Name: "some-server", Address: "1.2.3.4", Port: int64p(8080), Maintenance: models.ServerMaintenanceDisabled, }, models.Server{ - Name: "srv_1", + Name: "different-server", Address: "1.2.3.5", Port: int64p(8081), Maintenance: models.ServerMaintenanceDisabled, @@ -214,7 +214,7 @@ func TestAddServerSameSize(t *testing.T) { require.Nil(t, err) ha.RequireOps(t, - RequireOp(haOpReplaceServer, "srv_1"), + RequireOp(haOpReplaceServer, "disabled_server_0"), ) } @@ -227,13 +227,13 @@ func TestRemoveServerSameSize(t *testing.T) { }, Servers: []models.Server{ models.Server{ - Name: "srv_0", + Name: "some-server", Address: "1.2.3.4", Port: int64p(8080), Maintenance: models.ServerMaintenanceDisabled, }, models.Server{ - Name: "srv_1", + Name: "different-server", Address: "1.2.3.5", Port: int64p(8081), Maintenance: models.ServerMaintenanceDisabled, @@ -250,13 +250,13 @@ func TestRemoveServerSameSize(t *testing.T) { }, Servers: []models.Server{ models.Server{ - Name: "srv_0", + Name: "disabled_server_0", Address: "127.0.0.1", Port: int64p(1), Maintenance: models.ServerMaintenanceEnabled, }, models.Server{ - Name: "srv_1", + Name: "different-server", Address: "1.2.3.5", Port: int64p(8081), Maintenance: models.ServerMaintenanceDisabled, @@ -272,7 +272,7 @@ func TestRemoveServerSameSize(t *testing.T) { require.Nil(t, err) ha.RequireOps(t, - RequireOp(haOpReplaceServer, "srv_0"), + RequireOp(haOpReplaceServer, "some-server"), ) } @@ -285,7 +285,7 @@ func TestDifferentCerts(t *testing.T) { }, Servers: []models.Server{ models.Server{ - Name: "srv_0", + Name: "some-server", Address: "1.2.3.4", Port: int64p(8080), Maintenance: models.ServerMaintenanceDisabled, @@ -304,7 +304,7 @@ func TestDifferentCerts(t *testing.T) { }, Servers: []models.Server{ models.Server{ - Name: "srv_0", + Name: "some-server", Address: "1.2.3.4", Port: int64p(8080), Maintenance: models.ServerMaintenanceDisabled, @@ -324,7 +324,7 @@ func TestDifferentCerts(t *testing.T) { ha.RequireOps(t, RequireOp(haOpDeleteBackend, "back"), RequireOp(haOpCreateBackend, "back"), - RequireOp(haOpCreateServer, "srv_0"), + RequireOp(haOpCreateServer, "some-server"), ) } @@ -337,7 +337,7 @@ func TestBackendChange(t *testing.T) { }, Servers: []models.Server{ models.Server{ - Name: "srv_0", + Name: "some-server", Address: "1.2.3.4", Port: int64p(8080), Maintenance: models.ServerMaintenanceDisabled, @@ -355,7 +355,7 @@ func TestBackendChange(t *testing.T) { }, Servers: []models.Server{ models.Server{ - Name: "srv_0", + Name: "some-server", Address: "1.2.3.4", Port: int64p(8080), Maintenance: models.ServerMaintenanceDisabled, @@ -373,6 +373,6 @@ func TestBackendChange(t *testing.T) { ha.RequireOps(t, RequireOp(haOpDeleteBackend, "back"), RequireOp(haOpCreateBackend, "back"), - RequireOp(haOpCreateServer, "srv_0"), + RequireOp(haOpCreateServer, "some-server"), ) } diff --git a/haproxy/state/fake_ha_test.go b/haproxy/state/fake_ha_test.go index 25b6bde..7d1ce86 100644 --- a/haproxy/state/fake_ha_test.go +++ b/haproxy/state/fake_ha_test.go @@ -98,10 +98,10 @@ func (h *fakeHA) CreateServer(beName string, srv models.Server) error { return nil } -func (h *fakeHA) ReplaceServer(beName string, srv models.Server) error { +func (h *fakeHA) ReplaceServer(beName string, oldSrvName string, newSrv models.Server) error { h.ops = append(h.ops, fakeHAOp{ Type: haOpReplaceServer, - Name: srv.Name, + Name: oldSrvName, }) return nil } diff --git a/haproxy/state/snapshot_test.go b/haproxy/state/snapshot_test.go index 636910a..fbd937c 100644 --- a/haproxy/state/snapshot_test.go +++ b/haproxy/state/snapshot_test.go @@ -26,14 +26,16 @@ func GetTestConsulConfig() consul.Config { LocalBindPort: 10000, Nodes: []consul.UpstreamNode{ consul.UpstreamNode{ - Host: "1.2.3.4", - Port: 8080, - Weight: 5, + Name: "some-server", + Address: "1.2.3.4", + Port: 8080, + Weight: 5, }, consul.UpstreamNode{ - Host: "1.2.3.5", - Port: 8081, - Weight: 8, + Name: "different-server", + Address: "1.2.3.5", + Port: 8081, + Weight: 8, }, }, }, @@ -155,7 +157,7 @@ func GetTestHAConfig(baseCfg string) State { }, Servers: []models.Server{ models.Server{ - Name: "srv_0", + Name: "some-server", Address: "1.2.3.4", Port: int64p(8080), Weight: int64p(5), @@ -166,7 +168,7 @@ func GetTestHAConfig(baseCfg string) State { Maintenance: models.ServerMaintenanceDisabled, }, models.Server{ - Name: "srv_1", + Name: "different-server", Address: "1.2.3.5", Port: int64p(8081), Weight: int64p(8), @@ -234,36 +236,40 @@ func TestServerUpdate(t *testing.T) { consulCfg := GetTestConsulConfig() consulCfg.Upstreams[0].Nodes = consulCfg.Upstreams[0].Nodes[1:] - oldState := GetTestHAConfig("/") - // remove first server - expectedNewState := GetTestHAConfig("/") - expectedNewState.Backends[1].Servers[0].Maintenance = models.ServerMaintenanceEnabled - expectedNewState.Backends[1].Servers[0].Address = "127.0.0.1" - expectedNewState.Backends[1].Servers[0].Port = int64p(1) - expectedNewState.Backends[1].Servers[0].Weight = int64p(1) + expectedRemovedFirstServer := GetTestHAConfig("/") + expectedRemovedFirstServer.Backends[1].Servers[0].Maintenance = models.ServerMaintenanceEnabled + expectedRemovedFirstServer.Backends[1].Servers[0].Name = "disabled_server_0" + expectedRemovedFirstServer.Backends[1].Servers[0].Address = "127.0.0.1" + expectedRemovedFirstServer.Backends[1].Servers[0].Port = int64p(1) + expectedRemovedFirstServer.Backends[1].Servers[0].Weight = int64p(1) + + originalState := GetTestHAConfig("/") + actualRemovedFirstServer, err := Generate(TestOpts, TestCertStore, originalState, consulCfg) - generated, err := Generate(TestOpts, TestCertStore, oldState, consulCfg) require.Nil(t, err) - require.Equal(t, expectedNewState, generated) + require.Equal(t, expectedRemovedFirstServer, actualRemovedFirstServer) // re-add first server - generated, err = Generate(TestOpts, TestCertStore, generated, GetTestConsulConfig()) - require.Nil(t, err) - require.Equal(t, GetTestHAConfig("/"), generated) + originalConsulCfg := GetTestConsulConfig() + readdedFirstServer, err2 := Generate(TestOpts, TestCertStore, actualRemovedFirstServer, originalConsulCfg) + + require.Nil(t, err2) + require.Equal(t, originalState, readdedFirstServer) // add another one - consulCfg = GetTestConsulConfig() - consulCfg.Upstreams[0].Nodes = append(consulCfg.Upstreams[0].Nodes, consul.UpstreamNode{ - Host: "1.2.3.6", - Port: 8082, - Weight: 10, + addedOneServerConsulCfg := GetTestConsulConfig() + addedOneServerConsulCfg.Upstreams[0].Nodes = append(addedOneServerConsulCfg.Upstreams[0].Nodes, consul.UpstreamNode{ + Name: "even-different-server", + Address: "1.2.3.6", + Port: 8082, + Weight: 10, }) - expectedNewState = GetTestHAConfig("/") - expectedNewState.Backends[1].Servers = append(expectedNewState.Backends[1].Servers, + expectedAddedOneServer := GetTestHAConfig("/") + expectedAddedOneServer.Backends[1].Servers = append(expectedAddedOneServer.Backends[1].Servers, models.Server{ - Name: "srv_2", + Name: "even-different-server", Address: "1.2.3.6", Port: int64p(8082), Weight: int64p(10), @@ -273,8 +279,8 @@ func TestServerUpdate(t *testing.T) { Verify: models.BindVerifyRequired, Maintenance: models.ServerMaintenanceDisabled, }, - models.Server{ - Name: "srv_3", + models.Server{ // because we always make slot for one more + Name: "disabled_server_3", Address: "127.0.0.1", Port: int64p(1), Weight: int64p(1), @@ -286,9 +292,9 @@ func TestServerUpdate(t *testing.T) { }, ) - generated, err = Generate(TestOpts, TestCertStore, generated, consulCfg) + actualAddedOneServer, err := Generate(TestOpts, TestCertStore, expectedAddedOneServer, addedOneServerConsulCfg) require.Nil(t, err) - require.Equal(t, expectedNewState, generated) + require.Equal(t, expectedAddedOneServer, actualAddedOneServer) } type fakeCertStore struct{} diff --git a/haproxy/state/states.go b/haproxy/state/states.go index b85720d..a374909 100644 --- a/haproxy/state/states.go +++ b/haproxy/state/states.go @@ -28,7 +28,7 @@ type HAProxy interface { DeleteBackend(name string) error CreateBackend(be models.Backend) error CreateServer(beName string, srv models.Server) error - ReplaceServer(beName string, srv models.Server) error + ReplaceServer(beName string, oldSrvName string, newSrv models.Server) error DeleteServer(beName string, name string) error CreateFilter(parentType, parentName string, filter models.Filter) error CreateTCPRequestRule(parentType, parentName string, rule models.TCPRequestRule) error diff --git a/haproxy/state/upstream.go b/haproxy/state/upstream.go index 3509967..4ab3c29 100644 --- a/haproxy/state/upstream.go +++ b/haproxy/state/upstream.go @@ -85,7 +85,7 @@ func generateUpstreamServers(opts Options, certStore CertificateStore, cfg consu return fmt.Sprintf("%s:%d", s.Address, *s.Port) } idxConsulNode := func(s consul.UpstreamNode) string { - return fmt.Sprintf("%s:%d", s.Host, s.Port) + return fmt.Sprintf("%s:%d", s.Address, s.Port) } servers := make([]models.Server, len(oldBackend.Servers)) @@ -124,7 +124,7 @@ func generateUpstreamServers(opts Options, certStore CertificateStore, cfg consu } servers[i] = disabledServer - servers[i].Name = fmt.Sprintf("srv_%d", i) + servers[i].Name = fmt.Sprintf("disabled_server_%d", i) emptyServerSlots = append(emptyServerSlots, i) } @@ -143,7 +143,7 @@ func generateUpstreamServers(opts Options, certStore CertificateStore, cfg consu } for i := 0; i < add; i++ { server := disabledServer - server.Name = fmt.Sprintf("srv_%d", i+l) + server.Name = fmt.Sprintf("disabled_server_%d", i+l) servers = append(servers, server) emptyServerSlots = append(emptyServerSlots, i+l) } @@ -152,7 +152,8 @@ func generateUpstreamServers(opts Options, certStore CertificateStore, cfg consu i := emptyServerSlots[0] emptyServerSlots = emptyServerSlots[1:] - servers[i].Address = s.Host + servers[i].Name = s.Name + servers[i].Address = s.Address servers[i].Port = int64p(s.Port) servers[i].Weight = int64p(s.Weight) servers[i].Maintenance = models.ServerMaintenanceDisabled