From f7ed6addb7b9eb59064a0da75107b3374d4b9cb7 Mon Sep 17 00:00:00 2001 From: Joachim Bauch Date: Tue, 6 Oct 2020 15:58:05 +0200 Subject: [PATCH 1/4] Always load backends in the order they are configured. --- src/signaling/backend_configuration.go | 30 +++++++++++---------- src/signaling/backend_configuration_test.go | 24 +++++++++++++++++ 2 files changed, 40 insertions(+), 14 deletions(-) diff --git a/src/signaling/backend_configuration.go b/src/signaling/backend_configuration.go index ceaf506..ef577e4 100644 --- a/src/signaling/backend_configuration.go +++ b/src/signaling/backend_configuration.go @@ -71,7 +71,7 @@ func NewBackendConfiguration(config *goconf.ConfigFile) (*BackendConfiguration, compat: true, } } else if backendIds, _ := config.GetString("backend", "backends"); backendIds != "" { - for host, configuredBackends := range getConfiguredHosts(config) { + for host, configuredBackends := range getConfiguredHosts(backendIds, config) { backends[host] = append(backends[host], configuredBackends...) for _, be := range configuredBackends { log.Printf("Backend %s added for %s", be.id, be.url) @@ -146,26 +146,28 @@ func (b *BackendConfiguration) UpsertHost(host string, backends []*Backend) { b.backends[host] = append(b.backends[host], backends...) } -func getConfiguredBackendIDs(config *goconf.ConfigFile) (ids map[string]bool) { - ids = make(map[string]bool) +func getConfiguredBackendIDs(backendIds string) (ids []string) { + seen := make(map[string]bool) - if backendIds, _ := config.GetString("backend", "backends"); backendIds != "" { - for _, id := range strings.Split(backendIds, ",") { - id = strings.TrimSpace(id) - if id == "" { - continue - } - - ids[id] = true + for _, id := range strings.Split(backendIds, ",") { + id = strings.TrimSpace(id) + if id == "" { + continue } + + if seen[id] { + continue + } + ids = append(ids, id) + seen[id] = true } return ids } -func getConfiguredHosts(config *goconf.ConfigFile) (hosts map[string][]*Backend) { +func getConfiguredHosts(backendIds string, config *goconf.ConfigFile) (hosts map[string][]*Backend) { hosts = make(map[string][]*Backend) - for id := range getConfiguredBackendIDs(config) { + for _, id := range getConfiguredBackendIDs(backendIds) { u, _ := config.GetString(id, "url") if u == "" { log.Printf("Backend %s is missing or incomplete, skipping", id) @@ -199,7 +201,7 @@ func getConfiguredHosts(config *goconf.ConfigFile) (hosts map[string][]*Backend) func (b *BackendConfiguration) Reload(config *goconf.ConfigFile) { if backendIds, _ := config.GetString("backend", "backends"); backendIds != "" { - configuredHosts := getConfiguredHosts(config) + configuredHosts := getConfiguredHosts(backendIds, config) // remove backends that are no longer configured for hostname := range b.backends { diff --git a/src/signaling/backend_configuration_test.go b/src/signaling/backend_configuration_test.go index f1a16f5..225c475 100644 --- a/src/signaling/backend_configuration_test.go +++ b/src/signaling/backend_configuration_test.go @@ -165,6 +165,30 @@ func TestIsUrlAllowed_AllowAll(t *testing.T) { testUrls(t, cfg, valid_urls, invalid_urls) } +type ParseBackendIdsTestcase struct { + s string + ids []string +} + +func TestParseBackendIds(t *testing.T) { + testcases := []ParseBackendIdsTestcase{ + ParseBackendIdsTestcase{"", nil}, + ParseBackendIdsTestcase{"backend1", []string{"backend1"}}, + ParseBackendIdsTestcase{" backend1 ", []string{"backend1"}}, + ParseBackendIdsTestcase{"backend1,", []string{"backend1"}}, + ParseBackendIdsTestcase{"backend1,backend1", []string{"backend1"}}, + ParseBackendIdsTestcase{"backend1, backend2", []string{"backend1", "backend2"}}, + ParseBackendIdsTestcase{"backend1,backend2, backend1", []string{"backend1", "backend2"}}, + } + + for _, test := range testcases { + ids := getConfiguredBackendIDs(test.s) + if !reflect.DeepEqual(ids, test.ids) { + t.Errorf("List of ids differs, expected %+v, got %+v", test.ids, ids) + } + } +} + func TestBackendReloadChangeExistingURL(t *testing.T) { original_config := goconf.NewConfigFile() original_config.AddOption("backend", "backends", "backend1, backend2") From aa0191ef7e0ed0df7ac0fb6553c0d1abc68c452a Mon Sep 17 00:00:00 2001 From: Joachim Bauch Date: Tue, 6 Oct 2020 16:00:29 +0200 Subject: [PATCH 2/4] Don't allow reload with old-style backend configurations. --- src/signaling/backend_configuration.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/signaling/backend_configuration.go b/src/signaling/backend_configuration.go index ef577e4..255c1a3 100644 --- a/src/signaling/backend_configuration.go +++ b/src/signaling/backend_configuration.go @@ -200,6 +200,11 @@ func getConfiguredHosts(backendIds string, config *goconf.ConfigFile) (hosts map } func (b *BackendConfiguration) Reload(config *goconf.ConfigFile) { + if b.compatBackend != nil { + log.Println("Old-style configuration active, reload is not supported") + return + } + if backendIds, _ := config.GetString("backend", "backends"); backendIds != "" { configuredHosts := getConfiguredHosts(backendIds, config) From cd4d9308351b8ed1f3c906f0dd220188380feb48 Mon Sep 17 00:00:00 2001 From: Joachim Bauch Date: Tue, 6 Oct 2020 16:25:22 +0200 Subject: [PATCH 3/4] Update reload tests - Add test that reloads without changes. - Add test that removes backend from shared host. --- src/signaling/backend_configuration_test.go | 70 ++++++++++++++++++++- 1 file changed, 68 insertions(+), 2 deletions(-) diff --git a/src/signaling/backend_configuration_test.go b/src/signaling/backend_configuration_test.go index 225c475..208376e 100644 --- a/src/signaling/backend_configuration_test.go +++ b/src/signaling/backend_configuration_test.go @@ -189,6 +189,37 @@ func TestParseBackendIds(t *testing.T) { } } +func TestBackendReloadNoChange(t *testing.T) { + original_config := goconf.NewConfigFile() + original_config.AddOption("backend", "backends", "backend1, backend2") + original_config.AddOption("backend", "allowall", "false") + original_config.AddOption("backend1", "url", "http://domain1.invalid") + original_config.AddOption("backend1", "secret", string(testBackendSecret)+"-backend1") + original_config.AddOption("backend2", "url", "http://domain2.invalid") + original_config.AddOption("backend2", "secret", string(testBackendSecret)+"-backend2") + o_cfg, err := NewBackendConfiguration(original_config) + if err != nil { + t.Fatal(err) + } + + new_config := goconf.NewConfigFile() + new_config.AddOption("backend", "backends", "backend1, backend2") + new_config.AddOption("backend", "allowall", "false") + new_config.AddOption("backend1", "url", "http://domain1.invalid") + new_config.AddOption("backend1", "secret", string(testBackendSecret)+"-backend1") + new_config.AddOption("backend2", "url", "http://domain2.invalid") + new_config.AddOption("backend2", "secret", string(testBackendSecret)+"-backend2") + n_cfg, err := NewBackendConfiguration(new_config) + if err != nil { + t.Fatal(err) + } + + o_cfg.Reload(original_config) + if !reflect.DeepEqual(n_cfg, o_cfg) { + t.Error("BackendConfiguration should be equal after Reload") + } +} + func TestBackendReloadChangeExistingURL(t *testing.T) { original_config := goconf.NewConfigFile() original_config.AddOption("backend", "backends", "backend1, backend2") @@ -291,13 +322,13 @@ func TestBackendReloadAddBackend(t *testing.T) { } } -func TestBackendReloadRemove(t *testing.T) { +func TestBackendReloadRemoveHost(t *testing.T) { original_config := goconf.NewConfigFile() original_config.AddOption("backend", "backends", "backend1, backend2") original_config.AddOption("backend", "allowall", "false") original_config.AddOption("backend1", "url", "http://domain1.invalid") original_config.AddOption("backend1", "secret", string(testBackendSecret)+"-backend1") - original_config.AddOption("backend2", "url", "http://domain1.invalid") + original_config.AddOption("backend2", "url", "http://domain2.invalid") original_config.AddOption("backend2", "secret", string(testBackendSecret)+"-backend2") o_cfg, err := NewBackendConfiguration(original_config) if err != nil { @@ -314,6 +345,41 @@ func TestBackendReloadRemove(t *testing.T) { t.Fatal(err) } + original_config.RemoveOption("backend", "backends") + original_config.AddOption("backend", "backends", "backend1") + original_config.RemoveSection("backend2") + + o_cfg.Reload(original_config) + if !reflect.DeepEqual(n_cfg, o_cfg) { + t.Error("BackendConfiguration should be equal after Reload") + } +} + +func TestBackendReloadRemoveBackendFromSharedHost(t *testing.T) { + original_config := goconf.NewConfigFile() + original_config.AddOption("backend", "backends", "backend1, backend2") + original_config.AddOption("backend", "allowall", "false") + original_config.AddOption("backend1", "url", "http://domain1.invalid/foo/") + original_config.AddOption("backend1", "secret", string(testBackendSecret)+"-backend1") + original_config.AddOption("backend2", "url", "http://domain1.invalid/bar/") + original_config.AddOption("backend2", "secret", string(testBackendSecret)+"-backend2") + o_cfg, err := NewBackendConfiguration(original_config) + if err != nil { + t.Fatal(err) + } + + new_config := goconf.NewConfigFile() + new_config.AddOption("backend", "backends", "backend1") + new_config.AddOption("backend", "allowall", "false") + new_config.AddOption("backend1", "url", "http://domain1.invalid/foo/") + new_config.AddOption("backend1", "secret", string(testBackendSecret)+"-backend1") + n_cfg, err := NewBackendConfiguration(new_config) + if err != nil { + t.Fatal(err) + } + + original_config.RemoveOption("backend", "backends") + original_config.AddOption("backend", "backends", "backend1") original_config.RemoveSection("backend2") o_cfg.Reload(original_config) From 57971110334679d86fbcd5a091ce5cc4f05beccd Mon Sep 17 00:00:00 2001 From: Joachim Bauch Date: Tue, 6 Oct 2020 16:26:19 +0200 Subject: [PATCH 4/4] Log what backends are added/changed/removed on reload. --- src/signaling/backend_configuration.go | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/src/signaling/backend_configuration.go b/src/signaling/backend_configuration.go index 255c1a3..2d28d1e 100644 --- a/src/signaling/backend_configuration.go +++ b/src/signaling/backend_configuration.go @@ -120,13 +120,17 @@ func NewBackendConfiguration(config *goconf.ConfigFile) (*BackendConfiguration, }, nil } -func (b *BackendConfiguration) RemoveBackend(host string) { +func (b *BackendConfiguration) RemoveBackendsForHost(host string) { + if oldBackends := b.backends[host]; len(oldBackends) > 0 { + for _, backend := range oldBackends { + log.Printf("Backend %s removed for %s", backend.id, backend.url) + } + } delete(b.backends, host) } func (b *BackendConfiguration) UpsertHost(host string, backends []*Backend) { - existingIndex := 0 - for _, existingBackend := range b.backends[host] { + for existingIndex, existingBackend := range b.backends[host] { found := false index := 0 for _, newBackend := range backends { @@ -134,16 +138,26 @@ func (b *BackendConfiguration) UpsertHost(host string, backends []*Backend) { found = true backends = append(backends[:index], backends[index+1:]...) break + } else if newBackend.id == existingBackend.id { + found = true + b.backends[host][existingIndex] = newBackend + backends = append(backends[:index], backends[index+1:]...) + log.Printf("Backend %s updated for %s", newBackend.id, newBackend.url) + break } index++ } if !found { + removed := b.backends[host][existingIndex] + log.Printf("Backend %s removed for %s", removed.id, removed.url) b.backends[host] = append(b.backends[host][:existingIndex], b.backends[host][existingIndex+1:]...) } - existingIndex++ } b.backends[host] = append(b.backends[host], backends...) + for _, added := range backends { + log.Printf("Backend %s added for %s", added.id, added.url) + } } func getConfiguredBackendIDs(backendIds string) (ids []string) { @@ -211,7 +225,7 @@ func (b *BackendConfiguration) Reload(config *goconf.ConfigFile) { // remove backends that are no longer configured for hostname := range b.backends { if _, ok := configuredHosts[hostname]; !ok { - b.RemoveBackend(hostname) + b.RemoveBackendsForHost(hostname) } }