move port and default check logic to locked step (#14057)

agent/agent.go

@@ -2104,6 +2104,21 @@ func (a *Agent) AddService(req AddServiceRequest) error {
 // addServiceLocked adds a service entry to the service manager if enabled, or directly
 // to the local state if it is not. This function assumes the state lock is already held.
 func (a *Agent) addServiceLocked(req addServiceLockedRequest) error {
+	// Must auto-assign the port and default checks (if needed) here to avoid race collisions.
+	if req.Service.LocallyRegisteredAsSidecar {
+		if req.Service.Port < 1 {
+			port, err := a.sidecarPortFromServiceIDLocked(req.Service.CompoundServiceID())
+			if err != nil {
+				return err
+			}
+			req.Service.Port = port
+		}
+		// Setup default check if none given.
+		if len(req.chkTypes) < 1 {
+			req.chkTypes = sidecarDefaultChecks(req.Service.ID, req.Service.Address, req.Service.Proxy.LocalServiceAddress, req.Service.Port)
+		}
+	}
+
 	req.Service.EnterpriseMeta.Normalize()
 
 	if err := a.validateService(req.Service, req.chkTypes); err != nil {
@@ -3368,7 +3383,7 @@ func (a *Agent) loadServices(conf *config.RuntimeConfig, snap map[structs.CheckI
 		}
 
 		// Grab and validate sidecar if there is one too
-		sidecar, sidecarChecks, sidecarToken, err := a.sidecarServiceFromNodeService(ns, service.Token)
+		sidecar, sidecarChecks, sidecarToken, err := sidecarServiceFromNodeService(ns, service.Token)
 		if err != nil {
 			return fmt.Errorf("Failed to validate sidecar for service %q: %v", service.Name, err)
 		}

agent/agent_endpoint.go

@@ -1159,7 +1159,7 @@ func (s *HTTPHandlers) AgentRegisterService(resp http.ResponseWriter, req *http.
 	}
 
 	// See if we have a sidecar to register too
-	sidecar, sidecarChecks, sidecarToken, err := s.agent.sidecarServiceFromNodeService(ns, token)
+	sidecar, sidecarChecks, sidecarToken, err := sidecarServiceFromNodeService(ns, token)
 	if err != nil {
 		return nil, HTTPError{StatusCode: http.StatusBadRequest, Reason: fmt.Sprintf("Invalid SidecarService: %s", err)}
 	}

agent/agent_test.go

@@ -2786,7 +2786,7 @@ func TestAgent_DeregisterPersistedSidecarAfterRestart(t *testing.T) {
 		},
 	}
 
-	connectSrv, _, _, err := a.sidecarServiceFromNodeService(srv, "")
+	connectSrv, _, _, err := sidecarServiceFromNodeService(srv, "")
 	require.NoError(t, err)
 
 	// First persist the check
@@ -2959,11 +2959,24 @@ func testAgent_loadServices_sidecar(t *testing.T, extraHCL string) {
 	if token := a.State.ServiceToken(structs.NewServiceID("rabbitmq", nil)); token != "abc123" {
 		t.Fatalf("bad: %s", token)
 	}
-	requireServiceExists(t, a, "rabbitmq-sidecar-proxy")
+	sidecarSvc := requireServiceExists(t, a, "rabbitmq-sidecar-proxy")
 	if token := a.State.ServiceToken(structs.NewServiceID("rabbitmq-sidecar-proxy", nil)); token != "abc123" {
 		t.Fatalf("bad: %s", token)
 	}
 
+	// Verify default checks have been added
+	wantChecks := sidecarDefaultChecks(sidecarSvc.ID, sidecarSvc.Address, sidecarSvc.Proxy.LocalServiceAddress, sidecarSvc.Port)
+	gotChecks := a.State.ChecksForService(sidecarSvc.CompoundServiceID(), true)
+	gotChkNames := make(map[string]types.CheckID)
+	for _, check := range gotChecks {
+		requireCheckExists(t, a, check.CheckID)
+		gotChkNames[check.Name] = check.CheckID
+	}
+	for _, check := range wantChecks {
+		chkName := check.Name
+		require.NotNil(t, gotChkNames[chkName])
+	}
+
 	// Sanity check rabbitmq service should NOT have sidecar info in state since
 	// it's done it's job and should be a registration syntax sugar only.
 	assert.Nil(t, svc.Connect.SidecarService)

agent/sidecar_service.go

@@ -2,6 +2,7 @@ package agent
 
 import (
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/hashicorp/consul/ipaddr"
@@ -13,6 +14,10 @@ func sidecarServiceID(serviceID string) string {
 	return serviceID + "-sidecar-proxy"
 }
 
+func serviceIDFromSidecarID(sidecarServiceID string) string {
+	return strings.Split(sidecarServiceID, "-")[0]
+}
+
 // sidecarServiceFromNodeService returns a *structs.NodeService representing a
 // sidecar service with all defaults populated based on the current agent
 // config.
@@ -30,7 +35,7 @@ func sidecarServiceID(serviceID string) string {
 // registration. This will be the same as the token parameter passed unless the
 // SidecarService definition contains a distinct one.
 // TODO: return AddServiceRequest
-func (a *Agent) sidecarServiceFromNodeService(ns *structs.NodeService, token string) (*structs.NodeService, []*structs.CheckType, string, error) {
+func sidecarServiceFromNodeService(ns *structs.NodeService, token string) (*structs.NodeService, []*structs.CheckType, string, error) {
 	if ns.Connect.SidecarService == nil {
 		return nil, nil, "", nil
 	}
@@ -114,41 +119,18 @@ func (a *Agent) sidecarServiceFromNodeService(ns *structs.NodeService, token str
 		}
 	}
 
-	if sidecar.Port < 1 {
-		port, err := a.sidecarPortFromServiceID(sidecar.CompoundServiceID())
-		if err != nil {
-			return nil, nil, "", err
-		}
-		sidecar.Port = port
-	}
-
 	// Setup checks
 	checks, err := ns.Connect.SidecarService.CheckTypes()
 	if err != nil {
 		return nil, nil, "", err
 	}
-	// Setup default check if none given.
-	if len(checks) < 1 {
-		// The check should use the sidecar's address because it makes a request to the sidecar.
-		// If the sidecar's address is empty, we fall back to the address of the local service, as set in
-		// sidecar.Proxy.LocalServiceAddress, in the hope that the proxy is also accessible on that address
-		// (which in most cases it is because it's running as a sidecar in the same network).
-		// We could instead fall back to the address of the service as set by (ns.Address), but I've kept it using
-		// sidecar.Proxy.LocalServiceAddress so as to not change things too much in the
-		// process of fixing #14433.
-		checkAddress := sidecar.Address
-		if checkAddress == "" {
-			checkAddress = sidecar.Proxy.LocalServiceAddress
-		}
-		checks = sidecarDefaultChecks(ns.ID, checkAddress, sidecar.Port)
-	}
 
 	return sidecar, checks, token, nil
 }
 
-// sidecarPortFromServiceID is used to allocate a unique port for a sidecar proxy.
+// sidecarPortFromServiceIDLocked is used to allocate a unique port for a sidecar proxy.
 // This is called immediately before registration to avoid value collisions. This function assumes the state lock is already held.
-func (a *Agent) sidecarPortFromServiceID(sidecarCompoundServiceID structs.ServiceID) (int, error) {
+func (a *Agent) sidecarPortFromServiceIDLocked(sidecarCompoundServiceID structs.ServiceID) (int, error) {
 	sidecarPort := 0
 
 	// Allocate port if needed (min and max inclusive).
@@ -213,11 +195,23 @@ func (a *Agent) sidecarPortFromServiceID(sidecarCompoundServiceID structs.Servic
 	return sidecarPort, nil
 }
 
-func sidecarDefaultChecks(serviceID string, address string, port int) []*structs.CheckType {
+func sidecarDefaultChecks(sidecarID string, sidecarAddress string, proxyServiceAddress string, port int) []*structs.CheckType {
+	// The check should use the sidecar's address because it makes a request to the sidecar.
+	// If the sidecar's address is empty, we fall back to the address of the local service, as set in
+	// sidecar.Proxy.LocalServiceAddress, in the hope that the proxy is also accessible on that address
+	// (which in most cases it is because it's running as a sidecar in the same network).
+	// We could instead fall back to the address of the service as set by (ns.Address), but I've kept it using
+	// sidecar.Proxy.LocalServiceAddress so as to not change things too much in the
+	// process of fixing #14433.
+	checkAddress := sidecarAddress
+	if checkAddress == "" {
+		checkAddress = proxyServiceAddress
+	}
+	serviceID := serviceIDFromSidecarID(sidecarID)
 	return []*structs.CheckType{
 		{
 			Name:     "Connect Sidecar Listening",
-			TCP:      ipaddr.FormatAddressPort(address, port),
+			TCP:      ipaddr.FormatAddressPort(checkAddress, port),
 			Interval: 10 * time.Second,
 		},
 		{

agent/sidecar_service_test.go

@@ -54,7 +54,7 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) {
 				Kind:                       structs.ServiceKindConnectProxy,
 				ID:                         "web1-sidecar-proxy",
 				Service:                    "web-sidecar-proxy",
-				Port:                       2222,
+				Port:                       0,
 				LocallyRegisteredAsSidecar: true,
 				Proxy: structs.ConnectProxyConfig{
 					DestinationServiceName: "web",
@@ -63,18 +63,8 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) {
 					LocalServicePort:       1111,
 				},
 			},
-			wantChecks: []*structs.CheckType{
+			wantChecks: nil,
-				{
+			wantToken:  "foo",
-					Name:     "Connect Sidecar Listening",
-					TCP:      "127.0.0.1:2222",
-					Interval: 10 * time.Second,
-				},
-				{
-					Name:         "Connect Sidecar Aliasing web1",
-					AliasService: "web1",
-				},
-			},
-			wantToken: "foo",
 		},
 		{
 			name: "all the allowed overrides",
@@ -157,7 +147,7 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) {
 				Kind:                       structs.ServiceKindConnectProxy,
 				ID:                         "web1-sidecar-proxy",
 				Service:                    "web-sidecar-proxy",
-				Port:                       2222,
+				Port:                       0,
 				Tags:                       []string{"foo"},
 				Meta:                       map[string]string{"foo": "bar"},
 				LocallyRegisteredAsSidecar: true,
@@ -168,17 +158,7 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) {
 					LocalServicePort:       1111,
 				},
 			},
-			wantChecks: []*structs.CheckType{
+			wantChecks: nil,
-				{
-					Name:     "Connect Sidecar Listening",
-					TCP:      "127.0.0.1:2222",
-					Interval: 10 * time.Second,
-				},
-				{
-					Name:         "Connect Sidecar Aliasing web1",
-					AliasService: "web1",
-				},
-			},
 		},
 		{
 			name: "invalid check type",
@@ -215,158 +195,14 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) {
 			token:   "foo",
 			wantErr: "reserved for internal use",
 		},
-		{
-			name: "uses proxy address for check",
-			sd: &structs.ServiceDefinition{
-				ID:   "web1",
-				Name: "web",
-				Port: 1111,
-				Connect: &structs.ServiceConnect{
-					SidecarService: &structs.ServiceDefinition{
-						Address: "123.123.123.123",
-						Proxy: &structs.ConnectProxyConfig{
-							LocalServiceAddress: "255.255.255.255",
-						},
-					},
-				},
-				Address: "255.255.255.255",
-			},
-			token: "foo",
-			wantNS: &structs.NodeService{
-				EnterpriseMeta:             *structs.DefaultEnterpriseMetaInDefaultPartition(),
-				Kind:                       structs.ServiceKindConnectProxy,
-				ID:                         "web1-sidecar-proxy",
-				Service:                    "web-sidecar-proxy",
-				Port:                       2222,
-				Address:                    "123.123.123.123",
-				LocallyRegisteredAsSidecar: true,
-				Proxy: structs.ConnectProxyConfig{
-					DestinationServiceName: "web",
-					DestinationServiceID:   "web1",
-					LocalServiceAddress:    "255.255.255.255",
-					LocalServicePort:       1111,
-				},
-			},
-			wantChecks: []*structs.CheckType{
-				{
-					Name:     "Connect Sidecar Listening",
-					TCP:      "123.123.123.123:2222",
-					Interval: 10 * time.Second,
-				},
-				{
-					Name:         "Connect Sidecar Aliasing web1",
-					AliasService: "web1",
-				},
-			},
-			wantToken: "foo",
-		},
-		{
-			name: "uses proxy.local_service_address for check if proxy address is empty",
-			sd: &structs.ServiceDefinition{
-				ID:   "web1",
-				Name: "web",
-				Port: 1111,
-				Connect: &structs.ServiceConnect{
-					SidecarService: &structs.ServiceDefinition{
-						Address: "", // Proxy address empty.
-						Proxy: &structs.ConnectProxyConfig{
-							LocalServiceAddress: "1.2.3.4",
-						},
-					},
-				},
-				Address: "", // Service address empty.
-			},
-			token: "foo",
-			wantNS: &structs.NodeService{
-				EnterpriseMeta:             *structs.DefaultEnterpriseMetaInDefaultPartition(),
-				Kind:                       structs.ServiceKindConnectProxy,
-				ID:                         "web1-sidecar-proxy",
-				Service:                    "web-sidecar-proxy",
-				Port:                       2222,
-				Address:                    "",
-				LocallyRegisteredAsSidecar: true,
-				Proxy: structs.ConnectProxyConfig{
-					DestinationServiceName: "web",
-					DestinationServiceID:   "web1",
-					LocalServiceAddress:    "1.2.3.4",
-					LocalServicePort:       1111,
-				},
-			},
-			wantChecks: []*structs.CheckType{
-				{
-					Name:     "Connect Sidecar Listening",
-					TCP:      "1.2.3.4:2222",
-					Interval: 10 * time.Second,
-				},
-				{
-					Name:         "Connect Sidecar Aliasing web1",
-					AliasService: "web1",
-				},
-			},
-			wantToken: "foo",
-		},
-		{
-			name: "uses 127.0.0.1 for check if proxy and proxy.local_service_address are empty",
-			sd: &structs.ServiceDefinition{
-				ID:   "web1",
-				Name: "web",
-				Port: 1111,
-				Connect: &structs.ServiceConnect{
-					SidecarService: &structs.ServiceDefinition{
-						Address: "",
-						Proxy: &structs.ConnectProxyConfig{
-							LocalServiceAddress: "",
-						},
-					},
-				},
-				Address: "",
-			},
-			token: "foo",
-			wantNS: &structs.NodeService{
-				EnterpriseMeta:             *structs.DefaultEnterpriseMetaInDefaultPartition(),
-				Kind:                       structs.ServiceKindConnectProxy,
-				ID:                         "web1-sidecar-proxy",
-				Service:                    "web-sidecar-proxy",
-				Port:                       2222,
-				Address:                    "",
-				LocallyRegisteredAsSidecar: true,
-				Proxy: structs.ConnectProxyConfig{
-					DestinationServiceName: "web",
-					DestinationServiceID:   "web1",
-					LocalServiceAddress:    "127.0.0.1",
-					LocalServicePort:       1111,
-				},
-			},
-			wantChecks: []*structs.CheckType{
-				{
-					Name:     "Connect Sidecar Listening",
-					TCP:      "127.0.0.1:2222",
-					Interval: 10 * time.Second,
-				},
-				{
-					Name:         "Connect Sidecar Aliasing web1",
-					AliasService: "web1",
-				},
-			},
-			wantToken: "foo",
-		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			hcl := `
-			ports {
-				sidecar_min_port = 2222
-				sidecar_max_port = 2222
-			}
-			`
-			a := StartTestAgent(t, TestAgent{Name: "jones", HCL: hcl})
-			defer a.Shutdown()
-
 			ns := tt.sd.NodeService()
 			err := ns.Validate()
 			require.NoError(t, err, "Invalid test case - NodeService must validate")
 
-			gotNS, gotChecks, gotToken, err := a.sidecarServiceFromNodeService(ns, tt.token)
+			gotNS, gotChecks, gotToken, err := sidecarServiceFromNodeService(ns, tt.token)
 			if tt.wantErr != "" {
 				require.Error(t, err)
 				require.Contains(t, err.Error(), tt.wantErr)
@@ -464,7 +300,7 @@ func TestAgent_SidecarPortFromServiceID(t *testing.T) {
 				}
 				`
 			}
-			a := StartTestAgent(t, TestAgent{Name: "jones", HCL: hcl})
+			a := NewTestAgent(t, hcl)
 			defer a.Shutdown()
 
 			if tt.preRegister != nil {
@@ -472,7 +308,7 @@ func TestAgent_SidecarPortFromServiceID(t *testing.T) {
 				require.NoError(t, err)
 			}
 
-			gotPort, err := a.sidecarPortFromServiceID(structs.ServiceID{ID: tt.serviceID, EnterpriseMeta: tt.enterpriseMeta})
+			gotPort, err := a.sidecarPortFromServiceIDLocked(structs.ServiceID{ID: tt.serviceID, EnterpriseMeta: tt.enterpriseMeta})
 
 			if tt.wantErr != "" {
 				require.Error(t, err)
@@ -485,3 +321,52 @@ func TestAgent_SidecarPortFromServiceID(t *testing.T) {
 		})
 	}
 }
+
+func TestAgent_SidecarDefaultChecks(t *testing.T) {
+	tests := []struct {
+		name                 string
+		svcAddress           string
+		proxyLocalSvcAddress string
+		port                 int
+		wantChecks           []*structs.CheckType
+	}{{
+		name:                 "uses proxy address for check",
+		svcAddress:           "123.123.123.123",
+		proxyLocalSvcAddress: "255.255.255.255",
+		port:                 2222,
+		wantChecks: []*structs.CheckType{
+			{
+				Name:     "Connect Sidecar Listening",
+				TCP:      "123.123.123.123:2222",
+				Interval: 10 * time.Second,
+			},
+			{
+				Name:         "Connect Sidecar Aliasing web1",
+				AliasService: "web1",
+			},
+		},
+	},
+		{
+			name:                 "uses proxy.local_service_address for check if proxy address is empty",
+			proxyLocalSvcAddress: "1.2.3.4",
+			port:                 2222,
+			wantChecks: []*structs.CheckType{
+				{
+					Name:     "Connect Sidecar Listening",
+					TCP:      "1.2.3.4:2222",
+					Interval: 10 * time.Second,
+				},
+				{
+					Name:         "Connect Sidecar Aliasing web1",
+					AliasService: "web1",
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotChecks := sidecarDefaultChecks("web1", tt.svcAddress, tt.proxyLocalSvcAddress, tt.port)
+			require.Equal(t, tt.wantChecks, gotChecks)
+		})
+	}
+}

agent/structs/structs.go

@@ -1257,8 +1257,9 @@ type NodeService struct {
 	// a pointer so that we never have to nil-check this.
 	Connect ServiceConnect
 
+	// TODO: rename to reflect that this is used to express future intent to register.
 	// LocallyRegisteredAsSidecar is private as it is only used by a local agent
-	// state to track if the service was registered from a nested sidecar_service
+	// state to track if the service was or will be registered from a nested sidecar_service
 	// block. We need to track that so we can know whether we need to deregister
 	// it automatically too if it's removed from the service definition or if the
 	// parent service is deregistered. Relying only on ID would cause us to