diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d5ad3b06..5aae22c0a 100644 --- a/CHANGELOG.md +++ b/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)] diff --git a/command/agent/consul/service_client.go b/command/agent/consul/service_client.go index 8af946c46..68d013df8 100644 --- a/command/agent/consul/service_client.go +++ b/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 - // about any registration on the consul side. + // Either Nomad does not expect there to be a sidecar_service, or there is + // no actionable difference from the Consul sidecar_service definition. return false } diff --git a/command/agent/consul/service_client_test.go b/command/agent/consul/service_client_test.go index bb90313c2..a0522f99f 100644 --- a/command/agent/consul/service_client_test.go +++ b/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()