consul/connect: Fix bug where connect sidecar services would be unnecessarily re-registered

This PR fixes a bug where sidecar services would be re-registered into Consul every ~30
seconds, caused by the parent service having its tags field set and the sidecar_service
tags unset. Nomad would directly compare the tags between its copy of the sidecar service
definition and the tags of the sidecar service reported by Consul. This does not work,
because Consul will under-the-hood set the sidecar service tags to inherit the parent
service tags if the sidecar service tags are unset. The comparison then done by Nomad
would not match, if the parent sidecar tags are set.

Fixes #10025

CHANGELOG.md

@@ -21,6 +21,7 @@ BUG FIXES:
  * consul: Fixed a bug where failing tasks with group services would only cause the allocation to restart once instead of respecting the `restart` field. [[GH-9869](https://github.com/hashicorp/nomad/issues/9869)]
  * consul/connect: Fixed a bug where gateway proxy connection default timeout not set [[GH-9851](https://github.com/hashicorp/nomad/pull/9851)]
  * consul/connect: Fixed a bug preventing more than one connect gateway per Nomad client [[GH-9849](https://github.com/hashicorp/nomad/pull/9849)]
+ * consul/connect: Fixed a bug where connect sidecar services would be re-registered unnecessarily. [[GH-10059](https://github.com/hashicorp/nomad/pull/10059)]
  * consul/connect: Fixed a bug where the sidecar health checks would fail if `host_network` was defined. [[GH-9975](https://github.com/hashicorp/nomad/issues/9975)]
  * drivers/docker: Fixed a bug preventing multiple ports to be mapped to the same container port [[GH-9951](https://github.com/hashicorp/nomad/issues/9951)]
  * driver/qemu: Fixed a bug where network namespaces were not supported for QEMU workloads [[GH-9861](https://github.com/hashicorp/nomad/pull/9861)]

command/agent/consul/service_client.go

@@ -189,7 +189,6 @@ func maybeTweakTags(wanted *api.AgentServiceRegistration, existing *api.AgentSer
 // (cached) state of the service registration reported by Consul. If any of the
 // critical fields are not deeply equal, they considered different.
 func different(wanted *api.AgentServiceRegistration, existing *api.AgentService, sidecar *api.AgentService) bool {
-
 	switch {
 	case wanted.Kind != existing.Kind:
 		return true
@@ -205,12 +204,11 @@ func different(wanted *api.AgentServiceRegistration, existing *api.AgentService,
 		return true
 	case !reflect.DeepEqual(wanted.Meta, existing.Meta):
 		return true
-	case !reflect.DeepEqual(wanted.Tags, existing.Tags):
+	case tagsDifferent(wanted.Tags, existing.Tags):
 		return true
 	case connectSidecarDifferent(wanted, sidecar):
 		return true
 	}
-
 	return false
 }
 
@@ -228,20 +226,36 @@ func tagsDifferent(a, b []string) bool {
 	return false
 }
 
+// sidecarTagsDifferent includes the special logic for comparing sidecar tags
+// from Nomad vs. Consul perspective. Because Consul forces the sidecar tags
+// to inherit the parent service tags if the sidecar tags are unset, we need to
+// take that into consideration when Nomad's sidecar tags are unset by instead
+// comparing them to the parent service tags.
+func sidecarTagsDifferent(parent, wanted, sidecar []string) bool {
+	if len(wanted) == 0 {
+		return tagsDifferent(parent, sidecar)
+	}
+	return tagsDifferent(wanted, sidecar)
+}
+
+// connectSidecarDifferent returns true if Nomad expects there to be a sidecar
+// hanging off the desired parent service definition on the Consul side, and does
+// not match with what Consul has.
 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 tagsDifferent(wanted.Connect.SidecarService.Tags, sidecar.Tags) {
+
+		if sidecarTagsDifferent(wanted.Tags, wanted.Connect.SidecarService.Tags, sidecar.Tags) {
 			// tags on the nomad definition have been modified
 			return true
 		}
 	}
 
-	// There is no connect sidecar the nomad side; let consul anti-entropy worry
+	// Either Nomad does not expect there to be a sidecar_service, or there is
-	// about any registration on the consul side.
+	// no actionable difference from the Consul sidecar_service definition.
 	return false
 }
 

command/agent/consul/service_client_test.go

@@ -230,6 +230,32 @@ func TestSyncLogic_tagsDifferent(t *testing.T) {
 	})
 }
 
+func TestSyncLogic_sidecarTagsDifferent(t *testing.T) {
+	type tc struct {
+		parent, wanted, sidecar []string
+		expect                  bool
+	}
+
+	try := func(t *testing.T, test tc) {
+		result := sidecarTagsDifferent(test.parent, test.wanted, test.sidecar)
+		require.Equal(t, test.expect, result)
+	}
+
+	try(t, tc{parent: nil, wanted: nil, sidecar: nil, expect: false})
+
+	// wanted is nil, compare sidecar to parent
+	try(t, tc{parent: []string{"foo"}, wanted: nil, sidecar: nil, expect: true})
+	try(t, tc{parent: []string{"foo"}, wanted: nil, sidecar: []string{"foo"}, expect: false})
+	try(t, tc{parent: []string{"foo"}, wanted: nil, sidecar: []string{"bar"}, expect: true})
+	try(t, tc{parent: nil, wanted: nil, sidecar: []string{"foo"}, expect: true})
+
+	// wanted is non-nil, compare sidecar to wanted
+	try(t, tc{parent: nil, wanted: []string{"foo"}, sidecar: nil, expect: true})
+	try(t, tc{parent: nil, wanted: []string{"foo"}, sidecar: []string{"foo"}, expect: false})
+	try(t, tc{parent: nil, wanted: []string{"foo"}, sidecar: []string{"bar"}, expect: true})
+	try(t, tc{parent: []string{"foo"}, wanted: []string{"foo"}, sidecar: []string{"bar"}, expect: true})
+}
+
 func TestSyncLogic_maybeTweakTags(t *testing.T) {
 	t.Parallel()