From 3c411b167c5fa1994b3a6a50e64cb18c34f70b50 Mon Sep 17 00:00:00 2001
From: Seth Hoenig <shoenig@hashicorp.com>
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      | 16 ++++++++++-
 command/agent/consul/service_client_test.go | 31 +++++++++++++++++++++
 3 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8148563523fa..a0762eac8037 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 [[TBD](TBD)]
  * 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 cefcc81e246c..0a9241bdb097 100644
--- a/command/agent/consul/service_client.go
+++ b/command/agent/consul/service_client.go
@@ -201,13 +201,27 @@ func different(wanted *api.AgentServiceRegistration, existing *api.AgentService,
 		!connectSidecarDifferent(wanted, sidecar))
 }
 
+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 {
 	if wanted.Connect != nil && wanted.Connect.SidecarService != nil {
 		if sidecar == nil {
 			// 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 68bde388c76a..bb90313c22dc 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()