From f520f6dd0fa11a4b29d855599b39addc0d428374 Mon Sep 17 00:00:00 2001 From: Sarah Pratt Date: Tue, 26 Jul 2022 13:31:06 -0500 Subject: [PATCH 1/2] Separate port and socket path requirement in case of local agent assignment --- agent/agent_endpoint.go | 8 ++++---- agent/sidecar_service_test.go | 2 +- agent/structs/structs.go | 25 +++++++++++++++++++++---- agent/structs/structs_test.go | 10 ++++++++++ 4 files changed, 36 insertions(+), 9 deletions(-) diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index 11ec5f9ca7..65d8af1dff 100644 --- a/agent/agent_endpoint.go +++ b/agent/agent_endpoint.go @@ -1123,9 +1123,9 @@ func (s *HTTPHandlers) AgentRegisterService(resp http.ResponseWriter, req *http. return nil, HTTPError{StatusCode: http.StatusBadRequest, Reason: fmt.Sprintf("Invalid Service Meta: %v", err)} } - // Run validation. This is the same validation that would happen on - // the catalog endpoint so it helps ensure the sync will work properly. - if err := ns.Validate(); err != nil { + // Run validation. This same validation would happen on the catalog endpoint, + // so it helps ensure the sync will work properly. + if err := ns.ValidateForAgent(); err != nil { return nil, HTTPError{StatusCode: http.StatusBadRequest, Reason: fmt.Sprintf("Validation failed: %v", err.Error())} } @@ -1164,7 +1164,7 @@ func (s *HTTPHandlers) AgentRegisterService(resp http.ResponseWriter, req *http. return nil, HTTPError{StatusCode: http.StatusBadRequest, Reason: fmt.Sprintf("Invalid SidecarService: %s", err)} } if sidecar != nil { - if err := sidecar.Validate(); err != nil { + if err := sidecar.ValidateForAgent(); err != nil { return nil, HTTPError{StatusCode: http.StatusBadRequest, Reason: fmt.Sprintf("Failed Validation: %v", err.Error())} } // Make sure we are allowed to register the sidecar using the token diff --git a/agent/sidecar_service_test.go b/agent/sidecar_service_test.go index a2ffe9af49..7ced9720a7 100644 --- a/agent/sidecar_service_test.go +++ b/agent/sidecar_service_test.go @@ -339,7 +339,7 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) { } ns := tt.sd.NodeService() - err := ns.Validate() + err := ns.ValidateForAgent() require.NoError(t, err, "Invalid test case - NodeService must validate") gotNS, gotChecks, gotToken, err := a.sidecarServiceFromNodeService(ns, tt.token) diff --git a/agent/structs/structs.go b/agent/structs/structs.go index 275bf4c18b..7a7bd93f5c 100644 --- a/agent/structs/structs.go +++ b/agent/structs/structs.go @@ -1411,6 +1411,27 @@ func (s *NodeService) IsGateway() bool { func (s *NodeService) Validate() error { var result error + if s.Kind == ServiceKindConnectProxy { + if s.Port == 0 && s.SocketPath == "" { + result = multierror.Append(result, fmt.Errorf("Port or SocketPath must be set for a %s", s.Kind)) + } + } + + commonValidation := s.ValidateForAgent() + if commonValidation != nil { + result = multierror.Append(result, commonValidation) + } + + return result +} + +// ValidateForAgent does a subset validation, with the assumption that a local agent can assist with missing values. +// +// I.e. in the catalog case, a local agent cannot be assumed to facilitate auto-assignment of port or socket path, +// so additional checks are needed. +func (s *NodeService) ValidateForAgent() error { + var result error + // TODO(partitions): remember to double check that this doesn't cross partition boundaries // ConnectProxy validation @@ -1426,10 +1447,6 @@ func (s *NodeService) Validate() error { "services")) } - if s.Port == 0 && s.SocketPath == "" { - result = multierror.Append(result, fmt.Errorf("Port or SocketPath must be set for a %s", s.Kind)) - } - if s.Connect.Native { result = multierror.Append(result, fmt.Errorf( "A Proxy cannot also be Connect Native, only typical services")) diff --git a/agent/structs/structs_test.go b/agent/structs/structs_test.go index 0b6efb3309..844189c2f8 100644 --- a/agent/structs/structs_test.go +++ b/agent/structs/structs_test.go @@ -1157,6 +1157,16 @@ func TestStructs_NodeService_ValidateConnectProxy(t *testing.T) { } } +func TestStructs_NodeService_ValidateConnectProxyWithAgentAutoAssign(t *testing.T) { + t.Run("connect-proxy: no port set", func(t *testing.T) { + ns := TestNodeServiceProxy(t) + ns.Port = 0 + + err := ns.ValidateForAgent() + assert.True(t, err == nil) + }) +} + func TestStructs_NodeService_ValidateConnectProxy_In_Partition(t *testing.T) { cases := []struct { Name string From a3ef6f016e9af3b8c3ebc7d29fbc0f193b68c23e Mon Sep 17 00:00:00 2001 From: Sarah Pratt Date: Wed, 27 Jul 2022 13:19:17 -0500 Subject: [PATCH 2/2] refactor sidecare_service method into parts --- agent/agent_endpoint.go | 2 +- agent/sidecar_service.go | 75 +++++++++------ agent/sidecar_service_test.go | 176 +++++++++++++++++----------------- 3 files changed, 134 insertions(+), 119 deletions(-) diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index 65d8af1dff..b2d68e3044 100644 --- a/agent/agent_endpoint.go +++ b/agent/agent_endpoint.go @@ -1125,7 +1125,7 @@ func (s *HTTPHandlers) AgentRegisterService(resp http.ResponseWriter, req *http. // Run validation. This same validation would happen on the catalog endpoint, // so it helps ensure the sync will work properly. - if err := ns.ValidateForAgent(); err != nil { + if err := ns.Validate(); err != nil { return nil, HTTPError{StatusCode: http.StatusBadRequest, Reason: fmt.Sprintf("Validation failed: %v", err.Error())} } diff --git a/agent/sidecar_service.go b/agent/sidecar_service.go index 673a02252e..ea58a7a676 100644 --- a/agent/sidecar_service.go +++ b/agent/sidecar_service.go @@ -114,9 +114,32 @@ func (a *Agent) sidecarServiceFromNodeService(ns *structs.NodeService, token str } } + port, err := a.sidecarPortFromServiceIDLocked(sidecar.Port, 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) sidecarPortFromServiceIDLocked(sidecarPort int, sidecarCompoundServiceID structs.ServiceID) (int, error) { + // 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 +153,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 +170,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..cffe054c21 100644 --- a/agent/sidecar_service_test.go +++ b/agent/sidecar_service_test.go @@ -2,6 +2,7 @@ package agent import ( "fmt" + "github.com/hashicorp/consul/acl" "testing" "time" @@ -16,16 +17,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 +139,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 +214,64 @@ 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_SidecarPortFromServiceIDLocked(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: "port pre-specified", + serviceID: "web1", + wantPort: 2222, + }, + { + name: "use auto ports", + serviceID: "web1", + port: 1111, + wantPort: 1111, + }, { 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 +289,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 +334,6 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) { } ` } - a := StartTestAgent(t, TestAgent{Name: "jones", HCL: hcl}) defer a.Shutdown() @@ -338,11 +342,7 @@ 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") - - gotNS, gotChecks, gotToken, err := a.sidecarServiceFromNodeService(ns, tt.token) + gotPort, err := a.sidecarPortFromServiceIDLocked(tt.port, structs.ServiceID{ID: tt.serviceID, EnterpriseMeta: tt.enterpriseMeta}) if tt.wantErr != "" { require.Error(t, err) require.Contains(t, err.Error(), tt.wantErr) @@ -350,9 +350,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) }) } }