luxolus
/
open-consul
2
0
Fork 0
Code Issues 1 Pull Requests Packages Releases Wiki Activity

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

Refactor sidecar_service method to separate port assignment
This commit is contained in:
skpratt 2022-08-10 09:46:29 -05:00 committed by GitHub
parent d35dd36cbf 7f1f095b2f
commit 070ed3738d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 134 additions and 120 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

84
agent/sidecar_service.go
View File

@ -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
}
// 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 sidecar, checks, token, nil
return sidecarPort, nil
}
func sidecarDefaultChecks(serviceID string, localServiceAddress string, port int) []*structs.CheckType {
// Setup default check if none given
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,
},
}
}

170
agent/sidecar_service_test.go
View File

@ -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{},
// 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",
},
},
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",
},
},
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)
})
}
}