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

@@ -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)]

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
 		}

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()