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

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
 		}

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