diff --git a/agent/agent.go b/agent/agent.go index bfee4d32f8..b52c480c21 100644 --- a/agent/agent.go +++ b/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) } diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index b2d68e3044..e758d4bbf3 100644 --- a/agent/agent_endpoint.go +++ b/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)} } diff --git a/agent/agent_test.go b/agent/agent_test.go index 8bae81ce45..b0da44d6e7 100644 --- a/agent/agent_test.go +++ b/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) diff --git a/agent/sidecar_service.go b/agent/sidecar_service.go index a41d73d80a..72e868e773 100644 --- a/agent/sidecar_service.go +++ b/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, }, { diff --git a/agent/sidecar_service_test.go b/agent/sidecar_service_test.go index 39ab854a6b..ac30901ad6 100644 --- a/agent/sidecar_service_test.go +++ b/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{ - { - Name: "Connect Sidecar Listening", - TCP: "127.0.0.1:2222", - Interval: 10 * time.Second, - }, - { - Name: "Connect Sidecar Aliasing web1", - AliasService: "web1", - }, - }, - wantToken: "foo", + wantChecks: nil, + 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{ - { - Name: "Connect Sidecar Listening", - TCP: "127.0.0.1:2222", - Interval: 10 * time.Second, - }, - { - Name: "Connect Sidecar Aliasing web1", - AliasService: "web1", - }, - }, + wantChecks: nil, }, { 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) + }) + } +} diff --git a/agent/structs/structs.go b/agent/structs/structs.go index 8301688886..b3b8325674 100644 --- a/agent/structs/structs.go +++ b/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