From 685caee3dfcf0475f6aa90adf6f0cfd09fa145dd Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Wed, 11 Nov 2020 16:43:14 -0600 Subject: [PATCH] consul: prevent re-registration churn by correctly comparing sidecar tags Previously, connect sidecars would be re-registered with consul every cycle of Nomad's reconciliation loop around Consul service registrations. This is because part of the comparison used `reflect.DeepEqual` on []string objects, which returns false when one object is `[]string{}` and the other is `[]string{}(nil)`. Unforunately, this was always the case, and every Connect sidecar service would be re-registered on every iteration, which happens every 30 seconds. --- CHANGELOG.md | 1 + command/agent/consul/service_client.go | 47 ++++++++++++++++----- command/agent/consul/service_client_test.go | 31 ++++++++++++++ 3 files changed, 69 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8148563523f..97036dd0a50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,7 @@ BUG FIXES: * client: Fixed an in-place upgrade bug, where a Nomad client may fail to manage tasks that were started with pre-0.9 Nomad client. [[GH-9304](https://github.com/hashicorp/nomad/pull/9304)] * consul: Fixed a bug where canary_meta was not being interpolated with environment variables [[GH-9096](https://github.com/hashicorp/nomad/pull/9096)] * consul: Fixed a bug to correctly validate task when using script-checks in group-level services [[GH-8952](https://github.com/hashicorp/nomad/issues/8952)] + * consul: Fixed a bug that caused connect sidecars to be re-registered in Consul every 30 seconds [[GH-9330](https://github.com/hashicorp/nomad/pull/9330)] * consul/connect: Fixed a bug to correctly trigger updates on jobspec changes [[GH-9029](https://github.com/hashicorp/nomad/pull/9029)] * csi: Fixed a bug where multi-writer volumes were allowed only 1 write claim. [[GH-9040](https://github.com/hashicorp/nomad/issues/9040)] * csi: Fixed a bug where `nomad volume detach` would not accept prefixes for the node ID parameter. [[GH-9041](https://github.com/hashicorp/nomad/issues/9041)] diff --git a/command/agent/consul/service_client.go b/command/agent/consul/service_client.go index cefcc81e246..532547124e9 100644 --- a/command/agent/consul/service_client.go +++ b/command/agent/consul/service_client.go @@ -190,15 +190,42 @@ func maybeTweakTags(wanted *api.AgentServiceRegistration, existing *api.AgentSer // critical fields are not deeply equal, they considered different. func different(wanted *api.AgentServiceRegistration, existing *api.AgentService, sidecar *api.AgentService) bool { - return !(wanted.Kind == existing.Kind && - wanted.ID == existing.ID && - wanted.Port == existing.Port && - wanted.Address == existing.Address && - wanted.Name == existing.Service && - wanted.EnableTagOverride == existing.EnableTagOverride && - reflect.DeepEqual(wanted.Meta, existing.Meta) && - reflect.DeepEqual(wanted.Tags, existing.Tags) && - !connectSidecarDifferent(wanted, sidecar)) + switch { + case wanted.Kind != existing.Kind: + return true + case wanted.ID != existing.ID: + return true + case wanted.Port != existing.Port: + return true + case wanted.Address != existing.Address: + return true + case wanted.Name != existing.Service: + return true + case wanted.EnableTagOverride != existing.EnableTagOverride: + return true + case !reflect.DeepEqual(wanted.Meta, existing.Meta): + return true + case !reflect.DeepEqual(wanted.Tags, existing.Tags): + return true + case connectSidecarDifferent(wanted, sidecar): + return true + } + + return false +} + +func tagsDifferent(a, b []string) bool { + if len(a) != len(b) { + return true + } + + for i, valueA := range a { + if b[i] != valueA { + return true + } + } + + return false } func connectSidecarDifferent(wanted *api.AgentServiceRegistration, sidecar *api.AgentService) bool { @@ -207,7 +234,7 @@ func connectSidecarDifferent(wanted *api.AgentServiceRegistration, sidecar *api. // consul lost our sidecar (?) return true } - if !reflect.DeepEqual(wanted.Connect.SidecarService.Tags, sidecar.Tags) { + if tagsDifferent(wanted.Connect.SidecarService.Tags, sidecar.Tags) { // tags on the nomad definition have been modified return true } diff --git a/command/agent/consul/service_client_test.go b/command/agent/consul/service_client_test.go index 68bde388c76..bb90313c22d 100644 --- a/command/agent/consul/service_client_test.go +++ b/command/agent/consul/service_client_test.go @@ -199,6 +199,37 @@ func TestSyncLogic_agentServiceUpdateRequired(t *testing.T) { }) } +func TestSyncLogic_tagsDifferent(t *testing.T) { + t.Run("nil nil", func(t *testing.T) { + require.False(t, tagsDifferent(nil, nil)) + }) + + t.Run("empty nil", func(t *testing.T) { + // where reflect.DeepEqual does not work + require.False(t, tagsDifferent([]string{}, nil)) + }) + + t.Run("empty empty", func(t *testing.T) { + require.False(t, tagsDifferent([]string{}, []string{})) + }) + + t.Run("set empty", func(t *testing.T) { + require.True(t, tagsDifferent([]string{"A"}, []string{})) + }) + + t.Run("set nil", func(t *testing.T) { + require.True(t, tagsDifferent([]string{"A"}, nil)) + }) + + t.Run("different content", func(t *testing.T) { + require.True(t, tagsDifferent([]string{"A"}, []string{"B"})) + }) + + t.Run("different lengths", func(t *testing.T) { + require.True(t, tagsDifferent([]string{"A"}, []string{"A", "B"})) + }) +} + func TestSyncLogic_maybeTweakTags(t *testing.T) { t.Parallel()