From 67b7bc1e90cbb1e33cdd89871cb2f67675c1b3dc Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Fri, 30 Aug 2019 11:05:30 -0700 Subject: [PATCH] consul: ignore connect services when syncing Consul registers Connect services automatically, however Nomad thinks it owns them due to the _nomad prefix. Since the services are managed by Consul, Nomad needs to explicitly ignore them or otherwies they will be removed. --- command/agent/consul/client.go | 32 ++++++++++++ command/agent/consul/group_test.go | 78 ++++++++++++++++++++---------- 2 files changed, 84 insertions(+), 26 deletions(-) diff --git a/command/agent/consul/client.go b/command/agent/consul/client.go index ac05d6bf3..3f3739067 100644 --- a/command/agent/consul/client.go +++ b/command/agent/consul/client.go @@ -499,6 +499,11 @@ func (c *ServiceClient) sync() error { continue } + // Ignore if this is a service for a Nomad managed sidecar proxy. + if isNomadSidecar(id, c.services) { + continue + } + // Unknown Nomad managed service; kill if err := c.client.ServiceDeregister(id); err != nil { if isOldNomadService(id) { @@ -556,6 +561,11 @@ func (c *ServiceClient) sync() error { continue } + // Ignore if this is a check for a Nomad managed sidecar proxy. + if isNomadSidecar(check.ServiceID, c.services) { + continue + } + // Unknown Nomad managed check; remove if err := c.client.CheckDeregister(id); err != nil { if isOldNomadService(check.ServiceID) { @@ -1370,6 +1380,28 @@ func isOldNomadService(id string) bool { return strings.HasPrefix(id, prefix) } +// isNomadSidecar returns true if the ID matches a sidecar proxy for a Nomad +// managed service. +// +// For example if you have a Connect enabled service with the ID: +// +// _nomad-task-5229c7f8-376b-3ccc-edd9-981e238f7033-cache-redis-cache-db +// +// Consul will create a service for the sidecar proxy with the ID: +// +// _nomad-task-5229c7f8-376b-3ccc-edd9-981e238f7033-cache-redis-cache-db-sidecar-proxy +// +func isNomadSidecar(id string, services map[string]*api.AgentServiceRegistration) bool { + const suffix = "-sidecar-proxy" + if !strings.HasSuffix(id, suffix) { + return false + } + + // Make sure the Nomad managed service for this proxy still exists. + _, ok := services[id[:len(id)-len(suffix)]] + return ok +} + // getAddress returns the IP and port to use for a service or check. If no port // label is specified (an empty value), zero values are returned because no // address could be resolved. diff --git a/command/agent/consul/group_test.go b/command/agent/consul/group_test.go index 7cb708ba1..e6e42c522 100644 --- a/command/agent/consul/group_test.go +++ b/command/agent/consul/group_test.go @@ -5,6 +5,7 @@ import ( "testing" "time" + "github.com/hashicorp/consul/api" consulapi "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/testutil" "github.com/hashicorp/nomad/helper/testlog" @@ -32,7 +33,17 @@ func TestConsul_Connect(t *testing.T) { consulClient, err := consulapi.NewClient(consulConfig) require.NoError(t, err) serviceClient := NewServiceClient(consulClient.Agent(), testlog.HCLogger(t), true) + + // Lower periodicInterval to ensure periodic syncing doesn't improperly + // remove Connect services. + const interval = 50 * time.Millisecond + serviceClient.periodicInterval = interval + + // Disable deregistration probation to test syncing + serviceClient.deregisterProbationExpiry = time.Time{} + go serviceClient.Run() + defer serviceClient.Shutdown() alloc := mock.Alloc() alloc.AllocatedResources.Shared.Networks = []*structs.NetworkResource{ @@ -59,6 +70,11 @@ func TestConsul_Connect(t *testing.T) { }, } + // required by isNomadSidecar assertion below + serviceRegMap := map[string]*api.AgentServiceRegistration{ + MakeTaskServiceID(alloc.ID, "group-"+alloc.TaskGroup, tg.Services[0], false): nil, + } + require.NoError(t, serviceClient.RegisterGroup(alloc)) require.Eventually(t, func() bool { @@ -67,33 +83,43 @@ func TestConsul_Connect(t *testing.T) { return len(services) == 2 }, 3*time.Second, 100*time.Millisecond) - services, err := consulClient.Agent().Services() - require.NoError(t, err) - require.Len(t, services, 2) + // Test a few times to ensure Nomad doesn't improperly deregister + // Connect services. + for i := 10; i > 0; i-- { + services, err := consulClient.Agent().Services() + require.NoError(t, err) + require.Len(t, services, 2) - serviceID := MakeTaskServiceID(alloc.ID, "group-"+alloc.TaskGroup, tg.Services[0], false) - connectID := serviceID + "-sidecar-proxy" + serviceID := MakeTaskServiceID(alloc.ID, "group-"+alloc.TaskGroup, tg.Services[0], false) + connectID := serviceID + "-sidecar-proxy" - require.Contains(t, services, serviceID) - agentService := services[serviceID] - require.Equal(t, agentService.Service, "testconnect") - require.Equal(t, agentService.Address, "10.0.0.1") - require.Equal(t, agentService.Port, 9999) - require.Nil(t, agentService.Connect) - require.Nil(t, agentService.Proxy) + require.Contains(t, services, serviceID) + require.True(t, isNomadService(serviceID)) + require.False(t, isNomadSidecar(serviceID, serviceRegMap)) + agentService := services[serviceID] + require.Equal(t, agentService.Service, "testconnect") + require.Equal(t, agentService.Address, "10.0.0.1") + require.Equal(t, agentService.Port, 9999) + require.Nil(t, agentService.Connect) + require.Nil(t, agentService.Proxy) - require.Contains(t, services, connectID) - connectService := services[connectID] - require.Equal(t, connectService.Service, "testconnect-sidecar-proxy") - require.Equal(t, connectService.Address, "10.0.0.1") - require.Equal(t, connectService.Port, 9999) - require.Nil(t, connectService.Connect) - require.Equal(t, connectService.Proxy.DestinationServiceName, "testconnect") - require.Equal(t, connectService.Proxy.DestinationServiceID, serviceID) - require.Equal(t, connectService.Proxy.LocalServiceAddress, "127.0.0.1") - require.Equal(t, connectService.Proxy.LocalServicePort, 9999) - require.Equal(t, connectService.Proxy.Config, map[string]interface{}{ - "bind_address": "0.0.0.0", - "bind_port": float64(9998), - }) + require.Contains(t, services, connectID) + require.True(t, isNomadService(connectID)) + require.True(t, isNomadSidecar(connectID, serviceRegMap)) + connectService := services[connectID] + require.Equal(t, connectService.Service, "testconnect-sidecar-proxy") + require.Equal(t, connectService.Address, "10.0.0.1") + require.Equal(t, connectService.Port, 9999) + require.Nil(t, connectService.Connect) + require.Equal(t, connectService.Proxy.DestinationServiceName, "testconnect") + require.Equal(t, connectService.Proxy.DestinationServiceID, serviceID) + require.Equal(t, connectService.Proxy.LocalServiceAddress, "127.0.0.1") + require.Equal(t, connectService.Proxy.LocalServicePort, 9999) + require.Equal(t, connectService.Proxy.Config, map[string]interface{}{ + "bind_address": "0.0.0.0", + "bind_port": float64(9998), + }) + + time.Sleep(interval >> 2) + } }