diff --git a/agent/sidecar_service.go b/agent/sidecar_service.go index 673a02252e..e0cb24a0ea 100644 --- a/agent/sidecar_service.go +++ b/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 } diff --git a/agent/sidecar_service_test.go b/agent/sidecar_service_test.go index 7ced9720a7..f095670ff5 100644 --- a/agent/sidecar_service_test.go +++ b/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) }) } }