Merge pull request #14056 from hashicorp/proxy-register-port-race

Refactor sidecar_service method to separate port assignment

agent/sidecar_service.go

@@ -114,9 +114,35 @@ 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 {
+		checks = sidecarDefaultChecks(ns.ID, sidecar.Proxy.LocalServiceAddress, sidecar.Port)
+	}
+
+	return sidecar, checks, token, nil
+}
+
+// sidecarPortFromServiceID 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) {
+	sidecarPort := 0
+
 	// Allocate port if needed (min and max inclusive).
 	rangeLen := a.config.ConnectSidecarMaxPort - a.config.ConnectSidecarMinPort + 1
-	if sidecar.Port < 1 && a.config.ConnectSidecarMinPort > 0 && rangeLen > 0 {
+	if sidecarPort < 1 && a.config.ConnectSidecarMinPort > 0 && rangeLen > 0 {
 		// This did pick at random which was simpler but consul reload would assign
 		// new ports to all the sidecars since it unloads all state and
 		// re-populates. It also made this more difficult to test (have to pin the
@@ -130,11 +156,11 @@ func (a *Agent) sidecarServiceFromNodeService(ns *structs.NodeService, token str
 			// Check if other port is in auto-assign range
 			if otherNS.Port >= a.config.ConnectSidecarMinPort &&
 				otherNS.Port <= a.config.ConnectSidecarMaxPort {
-				if otherNS.CompoundServiceID() == sidecar.CompoundServiceID() {
+				if otherNS.CompoundServiceID() == sidecarCompoundServiceID {
 					// This sidecar is already registered with an auto-port and is just
 					// being updated so pick the same port as before rather than allocate
 					// a new one.
-					sidecar.Port = otherNS.Port
+					sidecarPort = otherNS.Port
 					break
 				}
 				usedPorts[otherNS.Port] = struct{}{}
@@ -147,54 +173,48 @@ func (a *Agent) sidecarServiceFromNodeService(ns *structs.NodeService, token str
 
 		// Check we still need to assign a port and didn't find we already had one
 		// allocated.
-		if sidecar.Port < 1 {
+		if sidecarPort < 1 {
 			// Iterate until we find lowest unused port
 			for p := a.config.ConnectSidecarMinPort; p <= a.config.ConnectSidecarMaxPort; p++ {
 				_, used := usedPorts[p]
 				if !used {
-					sidecar.Port = p
+					sidecarPort = p
 					break
 				}
 			}
 		}
 	}
 	// If no ports left (or auto ports disabled) fail
-	if sidecar.Port < 1 {
+	if sidecarPort < 1 {
 		// If ports are set to zero explicitly, config builder switches them to
 		// `-1`. In this case don't show the actual values since we don't know what
 		// was actually in config (zero or negative) and it might be confusing, we
 		// just know they explicitly disabled auto assignment.
 		if a.config.ConnectSidecarMinPort < 1 || a.config.ConnectSidecarMaxPort < 1 {
-			return nil, nil, "", fmt.Errorf("no port provided for sidecar_service " +
+			return 0, fmt.Errorf("no port provided for sidecar_service " +
 				"and auto-assignment disabled in config")
 		}
-		return nil, nil, "", fmt.Errorf("no port provided for sidecar_service and none "+
+		return 0, fmt.Errorf("no port provided for sidecar_service and none "+
 			"left in the configured range [%d, %d]", a.config.ConnectSidecarMinPort,
 			a.config.ConnectSidecarMaxPort)
 	}
 
-	// Setup checks
-	checks, err := ns.Connect.SidecarService.CheckTypes()
-	if err != nil {
-		return nil, nil, "", err
-	}
+	return sidecarPort, nil
+}
 
+func sidecarDefaultChecks(serviceID string, localServiceAddress string, port int) []*structs.CheckType {
 	// Setup default check if none given
-	if len(checks) < 1 {
-		checks = []*structs.CheckType{
-			{
-				Name: "Connect Sidecar Listening",
-				// Default to localhost rather than agent/service public IP. The checks
-				// can always be overridden if a non-loopback IP is needed.
-				TCP:      ipaddr.FormatAddressPort(sidecar.Proxy.LocalServiceAddress, sidecar.Port),
-				Interval: 10 * time.Second,
-			},
-			{
-				Name:         "Connect Sidecar Aliasing " + ns.ID,
-				AliasService: ns.ID,
-			},
-		}
+	return []*structs.CheckType{
+		{
+			Name: "Connect Sidecar Listening",
+			// Default to localhost rather than agent/service public IP. The checks
+			// can always be overridden if a non-loopback IP is needed.
+			TCP:      ipaddr.FormatAddressPort(localServiceAddress, port),
+			Interval: 10 * time.Second,
+		},
+		{
+			Name:         "Connect Sidecar Aliasing " + serviceID,
+			AliasService: serviceID,
+		},
 	}
-
-	return sidecar, checks, token, nil
 }

agent/sidecar_service_test.go

@@ -5,6 +5,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/hashicorp/consul/acl"
+
 	"github.com/stretchr/testify/require"
 
 	"github.com/hashicorp/consul/agent/structs"
@@ -16,16 +18,13 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) {
 	}
 
 	tests := []struct {
-		name              string
-		maxPort           int
-		preRegister       *structs.ServiceDefinition
-		sd                *structs.ServiceDefinition
-		token             string
-		autoPortsDisabled bool
-		wantNS            *structs.NodeService
-		wantChecks        []*structs.CheckType
-		wantToken         string
-		wantErr           string
+		name       string
+		sd         *structs.ServiceDefinition
+		token      string
+		wantNS     *structs.NodeService
+		wantChecks []*structs.CheckType
+		wantToken  string
+		wantErr    string
 	}{
 		{
 			name: "no sidecar",
@@ -141,42 +140,6 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) {
 			},
 			wantToken: "custom-token",
 		},
-		{
-			name: "no auto ports available",
-			// register another sidecar consuming our 1 and only allocated auto port.
-			preRegister: &structs.ServiceDefinition{
-				Kind: structs.ServiceKindConnectProxy,
-				Name: "api-proxy-sidecar",
-				Port: 2222, // Consume the one available auto-port
-				Proxy: &structs.ConnectProxyConfig{
-					DestinationServiceName: "api",
-				},
-			},
-			sd: &structs.ServiceDefinition{
-				ID:   "web1",
-				Name: "web",
-				Port: 1111,
-				Connect: &structs.ServiceConnect{
-					SidecarService: &structs.ServiceDefinition{},
-				},
-			},
-			token:   "foo",
-			wantErr: "none left in the configured range [2222, 2222]",
-		},
-		{
-			name:              "auto ports disabled",
-			autoPortsDisabled: true,
-			sd: &structs.ServiceDefinition{
-				ID:   "web1",
-				Name: "web",
-				Port: 1111,
-				Connect: &structs.ServiceConnect{
-					SidecarService: &structs.ServiceDefinition{},
-				},
-			},
-			token:   "foo",
-			wantErr: "auto-assignment disabled in config",
-		},
 		{
 			name: "inherit tags and meta",
 			sd: &structs.ServiceDefinition{
@@ -252,6 +215,58 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) {
 			token:   "foo",
 			wantErr: "reserved for internal use",
 		},
+	}
+	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)
+			if tt.wantErr != "" {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tt.wantErr)
+				return
+			}
+
+			require.NoError(t, err)
+			require.Equal(t, tt.wantNS, gotNS)
+			require.Equal(t, tt.wantChecks, gotChecks)
+			require.Equal(t, tt.wantToken, gotToken)
+		})
+	}
+}
+
+func TestAgent_SidecarPortFromServiceID(t *testing.T) {
+	if testing.Short() {
+		t.Skip("too slow for testing.Short")
+	}
+
+	tests := []struct {
+		name              string
+		autoPortsDisabled bool
+		enterpriseMeta    acl.EnterpriseMeta
+		maxPort           int
+		port              int
+		preRegister       *structs.ServiceDefinition
+		serviceID         string
+		wantPort          int
+		wantErr           string
+	}{
+		{
+			name:      "use auto ports",
+			serviceID: "web1",
+			wantPort:  2222,
+		},
 		{
 			name: "re-registering same sidecar with no port should pick same one",
 			// Allow multiple ports to be sure we get the right one
@@ -269,42 +284,27 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) {
 					LocalServicePort:       1111,
 				},
 			},
-			// Register same again but with different service port
-			sd: &structs.ServiceDefinition{
-				ID:   "web1",
-				Name: "web",
-				Port: 1112,
-				Connect: &structs.ServiceConnect{
-					SidecarService: &structs.ServiceDefinition{},
-				},
-			},
-			token: "foo",
-			wantNS: &structs.NodeService{
-				EnterpriseMeta:             *structs.DefaultEnterpriseMetaInDefaultPartition(),
-				Kind:                       structs.ServiceKindConnectProxy,
-				ID:                         "web1-sidecar-proxy",
-				Service:                    "web-sidecar-proxy",
-				Port:                       2222, // Should claim the same port as before
-				LocallyRegisteredAsSidecar: true,
-				Proxy: structs.ConnectProxyConfig{
-					DestinationServiceName: "web",
-					DestinationServiceID:   "web1",
-					LocalServiceAddress:    "127.0.0.1",
-					LocalServicePort:       1112,
-				},
-			},
-			wantChecks: []*structs.CheckType{
-				{
-					Name:     "Connect Sidecar Listening",
-					TCP:      "127.0.0.1:2222",
-					Interval: 10 * time.Second,
-				},
-				{
-					Name:         "Connect Sidecar Aliasing web1",
-					AliasService: "web1",
+			// Register same again
+			serviceID: "web1-sidecar-proxy",
+			wantPort:  2222, // Should claim the same port as before
+		},
+		{
+			name: "all auto ports already taken",
+			// register another sidecar consuming our 1 and only allocated auto port.
+			preRegister: &structs.ServiceDefinition{
+				Kind: structs.ServiceKindConnectProxy,
+				Name: "api-proxy-sidecar",
+				Port: 2222, // Consume the one available auto-port
+				Proxy: &structs.ConnectProxyConfig{
+					DestinationServiceName: "api",
 				},
 			},
-			wantToken: "foo",
+			wantErr: "none left in the configured range [2222, 2222]",
+		},
+		{
+			name:              "auto ports disabled",
+			autoPortsDisabled: true,
+			wantErr:           "auto-assignment disabled in config",
 		},
 	}
 	for _, tt := range tests {
@@ -329,7 +329,6 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) {
 				}
 				`
 			}
-
 			a := StartTestAgent(t, TestAgent{Name: "jones", HCL: hcl})
 			defer a.Shutdown()
 
@@ -338,11 +337,8 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) {
 				require.NoError(t, err)
 			}
 
-			ns := tt.sd.NodeService()
-			err := ns.ValidateForAgent()
-			require.NoError(t, err, "Invalid test case - NodeService must validate")
+			gotPort, err := a.sidecarPortFromServiceID(structs.ServiceID{ID: tt.serviceID, EnterpriseMeta: tt.enterpriseMeta})
 
-			gotNS, gotChecks, gotToken, err := a.sidecarServiceFromNodeService(ns, tt.token)
 			if tt.wantErr != "" {
 				require.Error(t, err)
 				require.Contains(t, err.Error(), tt.wantErr)
@@ -350,9 +346,7 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) {
 			}
 
 			require.NoError(t, err)
-			require.Equal(t, tt.wantNS, gotNS)
-			require.Equal(t, tt.wantChecks, gotChecks)
-			require.Equal(t, tt.wantToken, gotToken)
+			require.Equal(t, tt.wantPort, gotPort)
 		})
 	}
 }