consul/connect: Avoid assumption of parent service when filtering con…

…nect proxies

This PR uses regex-based matching for sidecar proxy services and checks when syncing
with Consul. Previously we would check if the parent of the sidecar was still being
tracked in Nomad. This is a false invariant - one which we must not depend when we
make #10845 work.

Fixes #10843

command/agent/consul/service_client.go

@@ -6,6 +6,7 @@ import (
 	"net"
 	"net/url"
 	"reflect"
+	"regexp"
 	"strconv"
 	"strings"
 	"sync"
@@ -816,7 +817,7 @@ func (c *ServiceClient) sync(reason syncReason) error {
 		}
 
 		// Ignore if this is a service for a Nomad managed sidecar proxy.
-		if isNomadSidecar(id, c.services) {
+		if maybeConnectSidecar(id) {
 			continue
 		}
 
@@ -886,7 +887,7 @@ func (c *ServiceClient) sync(reason syncReason) error {
 		}
 
 		// Ignore if this is a check for a Nomad managed sidecar proxy.
-		if isNomadSidecar(check.ServiceID, c.services) {
+		if maybeSidecarProxyCheck(id) {
 			continue
 		}
 
@@ -1681,8 +1682,14 @@ const (
 	sidecarSuffix = "-sidecar-proxy"
 )
 
-// isNomadSidecar returns true if the ID matches a sidecar proxy for a Nomad
-// managed service.
+// maybeConnectSidecar returns true if the ID is likely of a Connect sidecar proxy.
+// This function should only be used to determine if Nomad should skip managing
+// service id; it could produce false negatives for non-Nomad managed services
+// (i.e. someone set the ID manually), but Nomad does not manage those anyway.
+//
+// It is important not to reference the parent service, which may or may not still
+// be tracked by Nomad internally.
+//
 //
 // For example if you have a Connect enabled service with the ID:
 //
@@ -1692,14 +1699,39 @@ const (
 //
 //	_nomad-task-5229c7f8-376b-3ccc-edd9-981e238f7033-cache-redis-cache-db-sidecar-proxy
 //
-func isNomadSidecar(id string, services map[string]*api.AgentServiceRegistration) bool {
-	if !strings.HasSuffix(id, sidecarSuffix) {
-		return false
-	}
+func maybeConnectSidecar(id string) bool {
+	return strings.HasSuffix(id, sidecarSuffix)
+}
 
-	// Make sure the Nomad managed service for this proxy still exists.
-	_, ok := services[id[:len(id)-len(sidecarSuffix)]]
-	return ok
+var (
+	sidecarProxyCheckRe = regexp.MustCompile(`service:_nomad-.+-sidecar-proxy(:v[\d]+)?`)
+)
+
+// maybeSidecarProxyCheck returns true if the ID likely matches a Nomad generated
+// check ID used in the context of a Nomad managed Connect sidecar proxy. This function
+// should only be used to determine if Nomad should skip managing a check; it can
+// produce false negatives for non-Nomad managed Connect sidecar proxy checks (i.e.
+// someone set the ID manually), but Nomad does not care about those anyway.
+//
+// For example if you have a Connect enabled service with the ID:
+//
+//	_nomad-task-5229c7f8-376b-3ccc-edd9-981e238f7033-cache-redis-cache-db
+//
+// Nomad will create a Connect sidecar proxy of ID:
+//
+// _nomad-task-5229c7f8-376b-3ccc-edd9-981e238f7033-cache-redis-cache-db-sidecar-proxy
+//
+// With default checks like:
+//
+// service:_nomad-task-2f5fb517-57d4-44ee-7780-dc1cb6e103cd-group-api-count-api-9001-sidecar-proxy:1
+// service:_nomad-task-2f5fb517-57d4-44ee-7780-dc1cb6e103cd-group-api-count-api-9001-sidecar-proxy:2
+//
+// Unless sidecar_service.disable_default_tcp_check is set, in which case the
+// default check is:
+//
+// service:_nomad-task-322616db-2680-35d8-0d10-b50a0a0aa4cd-group-api-count-api-9001-sidecar-proxy
+func maybeSidecarProxyCheck(id string) bool {
+	return sidecarProxyCheckRe.MatchString(id)
 }
 
 // getNomadSidecar returns the service registration of the sidecar for the managed

command/agent/consul/service_client_test.go

@@ -624,3 +624,17 @@ func TestSyncOps_empty(t *testing.T) {
 	try(&operations{}, true)
 	try(nil, true)
 }
+
+func TestSyncLogic_maybeSidecarProxyCheck(t *testing.T) {
+	try := func(input string, exp bool) {
+		result := maybeSidecarProxyCheck(input)
+		require.Equal(t, exp, result)
+	}
+
+	try("service:_nomad-task-2f5fb517-57d4-44ee-7780-dc1cb6e103cd-group-api-count-api-9001-sidecar-proxy", true)
+	try("service:_nomad-task-2f5fb517-57d4-44ee-7780-dc1cb6e103cd-group-api-count-api-9001-sidecar-proxy:1", true)
+	try("service:_nomad-task-2f5fb517-57d4-44ee-7780-dc1cb6e103cd-group-api-count-api-9001-sidecar-proxy:2", true)
+	try("service:_nomad-task-2f5fb517-57d4-44ee-7780-dc1cb6e103cd-group-api-count-api-9001", false)
+	try("_nomad-task-2f5fb517-57d4-44ee-7780-dc1cb6e103cd-group-api-count-api-9001-sidecar-proxy:1", false)
+	try("service", false)
+}