diff --git a/agent/acl.go b/agent/acl.go index 5c4deb7cc8..dec738174e 100644 --- a/agent/acl.go +++ b/agent/acl.go @@ -277,6 +277,14 @@ func (a *Agent) vetServiceRegister(token string, service *structs.NodeService) e } } + // If the service is a proxy, ensure that it has write on the destination too + // since it can be discovered as an instance of that service. + if service.Kind == structs.ServiceKindConnectProxy { + if !rule.ServiceWrite(service.Proxy.DestinationServiceName, nil) { + return acl.ErrPermissionDenied + } + } + return nil } diff --git a/agent/agent.go b/agent/agent.go index cec9612993..b6f3d6a9ae 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1795,6 +1795,7 @@ func (a *Agent) AddService(service *structs.NodeService, chkTypes []*structs.Che return err } } + return nil } @@ -1839,6 +1840,20 @@ func (a *Agent) RemoveService(serviceID string, persist bool) error { } a.logger.Printf("[DEBUG] agent: removed service %q", serviceID) + + // If any Sidecar services exist for the removed service ID, remove them too. + if sidecar := a.State.Service(a.sidecarServiceID(serviceID)); sidecar != nil { + // Double check that it's not just an ID collision and we actually added + // this from a sidecar. + if sidecar.LocallyRegisteredAsSidecar { + // Remove it! + err := a.RemoveService(a.sidecarServiceID(serviceID), persist) + if err != nil { + return err + } + } + } + return nil } @@ -2718,9 +2733,27 @@ func (a *Agent) loadServices(conf *config.RuntimeConfig) error { if err != nil { return fmt.Errorf("Failed to validate checks for service %q: %v", service.Name, err) } + + // Grab and validate sidecar if there is one too + sidecar, sidecarChecks, sidecarToken, err := a.sidecarServiceFromNodeService(ns, service.Token) + if err != nil { + return fmt.Errorf("Failed to validate sidecar for service %q: %v", service.Name, err) + } + + // Remove sidecar from NodeService now it's done it's job it's just a config + // syntax sugar and shouldn't be persisted in local or server state. + ns.Connect.SidecarService = nil + if err := a.AddService(ns, chkTypes, false, service.Token); err != nil { return fmt.Errorf("Failed to register service %q: %v", service.Name, err) } + + // If there is a sidecar service, register that too. + if sidecar != nil { + if err := a.AddService(sidecar, sidecarChecks, false, sidecarToken); err != nil { + return fmt.Errorf("Failed to register sidecar for service %q: %v", service.Name, err) + } + } } // Load any persisted services diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index 6caeb0155f..e3038d57f9 100644 --- a/agent/agent_endpoint.go +++ b/agent/agent_endpoint.go @@ -559,30 +559,40 @@ func (s *HTTPServer) AgentRegisterService(resp http.ResponseWriter, req *http.Re // and why we should get rid of it. config.TranslateKeys(rawMap, map[string]string{ "enable_tag_override": "EnableTagOverride", - }) + // Managed Proxy Config + "exec_mode": "ExecMode", + // Proxy Upstreams + "destination_name": "DestinationName", + "destination_type": "DestinationType", + "destination_namespace": "DestinationNamespace", + "local_bind_port": "LocalBindPort", + "local_bind_address": "LocalBindAddress", + // Proxy Config + "destination_service_name": "DestinationServiceName", + "destination_service_id": "DestinationServiceID", + "local_service_port": "LocalServicePort", + "local_service_address": "LocalServiceAddress", + // SidecarService + "sidecar_service": "SidecarService", - // Translate upstream keys - we have the same upstream format in two - // possible places. - translateUpstreams := func(rawMap map[string]interface{}) { - var upstreams []interface{} - if us, ok := rawMap["upstreams"].([]interface{}); ok { - upstreams = us - } - if us, ok := rawMap["Upstreams"].([]interface{}); ok { - upstreams = us - } - for _, u := range upstreams { - if uMap, ok := u.(map[string]interface{}); ok { - config.TranslateKeys(uMap, map[string]string{ - "destination_name": "DestinationName", - "destination_type": "DestinationType", - "destination_namespace": "DestinationNamespace", - "local_bind_port": "LocalBindPort", - "local_bind_address": "LocalBindAddress", - }) - } - } - } + // DON'T Recurse into these opaque config maps or we might mangle user's + // keys. Note empty canonical is a special sentinel to prevent recursion. + "Meta": "", + // upstreams is an array but this prevents recursion into config field of + // any item in the array. + "Proxy.Config": "", + "Proxy.Upstreams.Config": "", + "Connect.Proxy.Config": "", + "Connect.Proxy.Upstreams.Config": "", + + // Same exceptions as above, but for a nested sidecar_service note we use + // the canonical form SidecarService since that is translated by the time + // the lookup here happens. Note that sidecar service doesn't support + // managed proxies (connect.proxy). + "Connect.SidecarService.Meta": "", + "Connect.SidecarService.Proxy.Config": "", + "Connect.SidecarService.Proxy.Upstreams.config": "", + }) for k, v := range rawMap { switch strings.ToLower(k) { @@ -600,32 +610,6 @@ func (s *HTTPServer) AgentRegisterService(resp http.ResponseWriter, req *http.Re return err } } - case "proxy": - if valMap, ok := v.(map[string]interface{}); ok { - config.TranslateKeys(valMap, map[string]string{ - "destination_service_name": "DestinationServiceName", - "destination_service_id": "DestinationServiceID", - "local_service_port": "LocalServicePort", - "local_service_address": "LocalServiceAddress", - }) - translateUpstreams(valMap) - } - case "connect": - if connectMap, ok := v.(map[string]interface{}); ok { - var proxyMap map[string]interface{} - if pMap, ok := connectMap["Proxy"].(map[string]interface{}); ok { - proxyMap = pMap - } - if pMap, ok := connectMap["proxy"].(map[string]interface{}); ok { - proxyMap = pMap - } - if proxyMap != nil { - config.TranslateKeys(proxyMap, map[string]string{ - "exec_mode": "ExecMode", - }) - translateUpstreams(proxyMap) - } - } } } return nil @@ -689,6 +673,23 @@ func (s *HTTPServer) AgentRegisterService(resp http.ResponseWriter, req *http.Re } } + // Verify the sidecar check types + if args.Connect != nil && args.Connect.SidecarService != nil { + chkTypes, err := args.Connect.SidecarService.CheckTypes() + if err != nil { + return nil, &BadRequestError{ + Reason: fmt.Sprintf("Invalid check in sidecar_service: %v", err), + } + } + for _, check := range chkTypes { + if check.Status != "" && !structs.ValidStatus(check.Status) { + return nil, &BadRequestError{ + Reason: "Status for checks must 'passing', 'warning', 'critical'", + } + } + } + } + // Get the provided token, if any, and vet against any ACL policies. var token string s.parseToken(req, &token) @@ -696,6 +697,26 @@ func (s *HTTPServer) AgentRegisterService(resp http.ResponseWriter, req *http.Re return nil, err } + // See if we have a sidecar to register too + sidecar, sidecarChecks, sidecarToken, err := s.agent.sidecarServiceFromNodeService(ns, token) + if err != nil { + return nil, &BadRequestError{ + Reason: fmt.Sprintf("Invalid SidecarService: %s", err)} + } + if sidecar != nil { + // Make sure we are allowed to register the side car using the token + // specified (might be specific to sidecar or the same one as the overall + // request). + if err := s.agent.vetServiceRegister(sidecarToken, sidecar); err != nil { + return nil, err + } + // We parsed the sidecar registration, now remove it from the NodeService + // for the actual service since it's done it's job and we don't want to + // persist it in the actual state/catalog. SidecarService is meant to be a + // registration syntax sugar so don't propagate it any further. + ns.Connect.SidecarService = nil + } + // Get any proxy registrations proxy, err := args.ConnectManagedProxy() if err != nil { @@ -720,6 +741,12 @@ func (s *HTTPServer) AgentRegisterService(resp http.ResponseWriter, req *http.Re return nil, err } } + // Add sidecar. + if sidecar != nil { + if err := s.agent.AddService(sidecar, sidecarChecks, true, sidecarToken); err != nil { + return nil, err + } + } s.syncChanges() return nil, nil } diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index 95bbae563e..c77090d1b3 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -4,6 +4,7 @@ import ( "bytes" "crypto/tls" "crypto/x509" + "encoding/json" "fmt" "io" "io/ioutil" @@ -19,6 +20,7 @@ import ( "github.com/hashicorp/consul/agent/checks" "github.com/hashicorp/consul/agent/config" "github.com/hashicorp/consul/agent/connect" + "github.com/hashicorp/consul/agent/local" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/lib" @@ -133,6 +135,57 @@ func TestAgent_Services_ExternalConnectProxy(t *testing.T) { assert.Equal(srv1.Proxy.DestinationServiceName, actual.ProxyDestination) } +// Thie tests that a sidecar-registered service is returned as expected. +func TestAgent_Services_Sidecar(t *testing.T) { + t.Parallel() + + require := require.New(t) + assert := assert.New(t) + a := NewTestAgent(t.Name(), "") + defer a.Shutdown() + + testrpc.WaitForLeader(t, a.RPC, "dc1") + srv1 := &structs.NodeService{ + Kind: structs.ServiceKindConnectProxy, + ID: "db-sidecar-proxy", + Service: "db-sidecar-proxy", + Port: 5000, + // Set this internal state that we expect sidecar registrations to have. + LocallyRegisteredAsSidecar: true, + Proxy: structs.ConnectProxyConfig{ + DestinationServiceName: "db", + Upstreams: structs.TestUpstreams(t), + }, + } + a.State.AddService(srv1, "") + + req, _ := http.NewRequest("GET", "/v1/agent/services", nil) + obj, err := a.srv.AgentServices(nil, req) + require.NoError(err) + val := obj.(map[string]*api.AgentService) + assert.Len(val, 1) + actual := val["db-sidecar-proxy"] + require.NotNil(actual) + assert.Equal(api.ServiceKindConnectProxy, actual.Kind) + assert.Equal(srv1.Proxy.ToAPI(), actual.Proxy) + + // DEPRECATED (ProxyDestination) - remove the next comment and assertion + // Should still have deprecated ProxyDestination filled in until we remove it + // completely at a major version bump. + assert.Equal(srv1.Proxy.DestinationServiceName, actual.ProxyDestination) + + // Sanity check that LocalRegisteredAsSidecar is not in the output (assuming + // JSON encoding). Right now this is not the case becuase the services + // endpoint happens to use the api struct which doesn't include that field, + // but this test serves as a regression test incase we change the endpoint to + // return the internal struct later and accidentally expose some "internal" + // state. + output, err := json.Marshal(obj) + require.NoError(err) + assert.NotContains(string(output), "LocallyRegisteredAsSidecar") + assert.NotContains(string(output), "locally_registered_as_sidecar") +} + func TestAgent_Services_ACLFilter(t *testing.T) { t.Parallel() a := NewTestAgent(t.Name(), TestACLConfig()) @@ -1401,36 +1454,80 @@ func TestAgent_RegisterService_TranslateKeys(t *testing.T) { "port":8000, "enable_tag_override": true, "meta": { - "some": "meta" + "some": "meta", + "enable_tag_override": "meta is 'opaque' so should not get translated" }, - "kind": "connect-proxy", - "proxy": { + "kind": "connect-proxy",` + + // Note the uppercase P is important here - it ensures translation works + // correctly in case-insensitive way. Without it this test can pass even + // when translation is broken for other valid inputs. + `"Proxy": { "destination_service_name": "web", "destination_service_id": "web", "local_service_port": 1234, "local_service_address": "127.0.0.1", + "config": { + "destination_type": "proxy.config is 'opaque' so should not get translated" + }, "upstreams": [ { "destination_type": "service", "destination_namespace": "default", "destination_name": "db", "local_bind_address": "127.0.0.1", - "local_bind_port": 1234 + "local_bind_port": 1234, + "config": { + "destination_type": "proxy.upstreams.config is 'opaque' so should not get translated" + } } ] }, "connect": { "proxy": { "exec_mode": "script", + "config": { + "destination_type": "connect.proxy.config is 'opaque' so should not get translated" + }, "upstreams": [ { "destination_type": "service", "destination_namespace": "default", "destination_name": "db", "local_bind_address": "127.0.0.1", - "local_bind_port": 1234 + "local_bind_port": 1234, + "config": { + "destination_type": "connect.proxy.upstreams.config is 'opaque' so should not get translated" + } } ] + }, + "sidecar_service": { + "name":"test-proxy", + "port":8001, + "enable_tag_override": true, + "meta": { + "some": "meta", + "enable_tag_override": "sidecar_service.meta is 'opaque' so should not get translated" + }, + "kind": "connect-proxy", + "proxy": { + "destination_service_name": "test", + "destination_service_id": "test", + "local_service_port": 4321, + "local_service_address": "127.0.0.1", + "upstreams": [ + { + "destination_type": "service", + "destination_namespace": "default", + "destination_name": "db", + "local_bind_address": "127.0.0.1", + "local_bind_port": 1234, + "config": { + "destination_type": "sidecar_service.proxy.upstreams.config is 'opaque' so should not get translated" + } + } + ] + } } }, "weights":{ @@ -1446,9 +1543,12 @@ func TestAgent_RegisterService_TranslateKeys(t *testing.T) { require.Equal(t, 200, rr.Code, "body: %s", rr.Body) svc := &structs.NodeService{ - ID: "test", - Service: "test", - Meta: map[string]string{"some": "meta"}, + ID: "test", + Service: "test", + Meta: map[string]string{ + "some": "meta", + "enable_tag_override": "meta is 'opaque' so should not get translated", + }, Port: 8000, EnableTagOverride: true, Weights: &structs.Weights{Passing: 16, Warning: 0}, @@ -1458,6 +1558,9 @@ func TestAgent_RegisterService_TranslateKeys(t *testing.T) { DestinationServiceID: "web", LocalServiceAddress: "127.0.0.1", LocalServicePort: 1234, + Config: map[string]interface{}{ + "destination_type": "proxy.config is 'opaque' so should not get translated", + }, Upstreams: structs.Upstreams{ { DestinationType: structs.UpstreamDestTypeService, @@ -1465,12 +1568,18 @@ func TestAgent_RegisterService_TranslateKeys(t *testing.T) { DestinationNamespace: "default", LocalBindAddress: "127.0.0.1", LocalBindPort: 1234, + Config: map[string]interface{}{ + "destination_type": "proxy.upstreams.config is 'opaque' so should not get translated", + }, }, }, }, Connect: structs.ServiceConnect{ Proxy: &structs.ServiceDefinitionConnectProxy{ ExecMode: "script", + Config: map[string]interface{}{ + "destination_type": "connect.proxy.config is 'opaque' so should not get translated", + }, Upstreams: structs.Upstreams{ { DestinationType: structs.UpstreamDestTypeService, @@ -1478,6 +1587,36 @@ func TestAgent_RegisterService_TranslateKeys(t *testing.T) { DestinationNamespace: "default", LocalBindAddress: "127.0.0.1", LocalBindPort: 1234, + Config: map[string]interface{}{ + "destination_type": "connect.proxy.upstreams.config is 'opaque' so should not get translated", + }, + }, + }, + }, + SidecarService: &structs.ServiceDefinition{ + Name: "test-proxy", + Meta: map[string]string{ + "some": "meta", + "enable_tag_override": "sidecar_service.meta is 'opaque' so should not get translated", + }, Port: 8001, + EnableTagOverride: true, + Kind: structs.ServiceKindConnectProxy, + Proxy: &structs.ConnectProxyConfig{ + DestinationServiceName: "test", + DestinationServiceID: "test", + LocalServiceAddress: "127.0.0.1", + LocalServicePort: 4321, + Upstreams: structs.Upstreams{ + { + DestinationType: structs.UpstreamDestTypeService, + DestinationName: "db", + DestinationNamespace: "default", + LocalBindAddress: "127.0.0.1", + LocalBindPort: 1234, + Config: map[string]interface{}{ + "destination_type": "sidecar_service.proxy.upstreams.config is 'opaque' so should not get translated", + }, + }, }, }, }, @@ -1815,6 +1954,504 @@ func TestAgent_RegisterService_UnmanagedConnectProxy(t *testing.T) { assert.Equal("abc123", a.State.ServiceToken("connect-proxy")) } +func testDefaultSidecar(svc string, port int, fns ...func(*structs.NodeService)) *structs.NodeService { + ns := &structs.NodeService{ + ID: svc + "-sidecar-proxy", + Kind: structs.ServiceKindConnectProxy, + Service: svc + "-sidecar-proxy", + Port: 2222, + // Note that LocallyRegisteredAsSidecar should be true on the internal + // NodeService, but that we never want to see it in the HTTP response as + // it's internal only state. This is being compared directly to local state + // so should be present here. + LocallyRegisteredAsSidecar: true, + Proxy: structs.ConnectProxyConfig{ + DestinationServiceName: svc, + DestinationServiceID: svc, + LocalServiceAddress: "127.0.0.1", + LocalServicePort: port, + }, + } + for _, fn := range fns { + fn(ns) + } + return ns +} + +// This tests local agent service registration with a sidecar service. Note we +// only test simple defaults for the sidecar here since the actual logic for +// handling sidecar defaults and port assignment is tested thoroughly in +// TestAgent_sidecarServiceFromNodeService. Note it also tests Deregister +// explicitly too since setup is identical. +func TestAgent_RegisterServiceDeregisterService_Sidecar(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + preRegister, preRegister2 *structs.NodeService + // Use raw JSON payloads rather than encoding to avoid subtleties with some + // internal representations and different ways they encode and decode. We + // rely on the payload being Unmarshalable to structs.ServiceDefinition + // directly. + json string + enableACL bool + tokenRules string + wantNS *structs.NodeService + wantErr string + wantSidecarIDLeftAfterDereg bool + assertStateFn func(t *testing.T, state *local.State) + }{ + { + name: "sanity check no sidecar case", + json: ` + { + "name": "web", + "port": 1111 + } + `, + wantNS: nil, + wantErr: "", + }, + { + name: "default sidecar", + json: ` + { + "name": "web", + "port": 1111, + "connect": { + "SidecarService": {} + } + } + `, + wantNS: testDefaultSidecar("web", 1111), + wantErr: "", + }, + { + name: "ACL OK defaults", + json: ` + { + "name": "web", + "port": 1111, + "connect": { + "SidecarService": {} + } + } + `, + enableACL: true, + tokenRules: ` + service "web" { + policy = "write" + }`, + wantNS: testDefaultSidecar("web", 1111), + wantErr: "", + }, + { + name: "ACL denied", + json: ` + { + "name": "web", + "port": 1111, + "connect": { + "SidecarService": {} + } + } + `, + enableACL: true, + tokenRules: ``, // No token rules means no valid token + wantNS: nil, + wantErr: "Permission denied", + }, + { + name: "ACL OK for service but not for sidecar", + json: ` + { + "name": "web", + "port": 1111, + "connect": { + "SidecarService": {} + } + } + `, + enableACL: true, + // This will become more common/reasonable when ACLs support exact match. + tokenRules: ` + service "web-sidecar-proxy" { + policy = "deny" + } + service "web" { + policy = "write" + }`, + wantNS: nil, + wantErr: "Permission denied", + }, + { + name: "ACL OK for service and sidecar but not sidecar's overriden destination", + json: ` + { + "name": "web", + "port": 1111, + "connect": { + "SidecarService": { + "proxy": { + "DestinationServiceName": "foo" + } + } + } + } + `, + enableACL: true, + tokenRules: ` + service "web" { + policy = "write" + }`, + wantNS: nil, + wantErr: "Permission denied", + }, + { + name: "ACL OK for service but not for overridden sidecar", + json: ` + { + "name": "web", + "port": 1111, + "connect": { + "SidecarService": { + "name": "foo-sidecar-proxy" + } + } + } + `, + enableACL: true, + tokenRules: ` + service "web" { + policy = "write" + }`, + wantNS: nil, + wantErr: "Permission denied", + }, + { + name: "ACL OK for service but and overridden for sidecar", + // This test ensures that if the sidecar embeds it's own token with + // differnt privs from the main request token it will be honoured for the + // sidecar registration. We use the test root token since that should have + // permission. + json: ` + { + "name": "web", + "port": 1111, + "connect": { + "SidecarService": { + "name": "foo", + "token": "root" + } + } + } + `, + enableACL: true, + tokenRules: ` + service "web" { + policy = "write" + }`, + wantNS: testDefaultSidecar("web", 1111, func(ns *structs.NodeService) { + ns.Service = "foo" + }), + wantErr: "", + }, + { + name: "invalid check definition in sidecar", + // Note no interval in the TCP check should fail validation + json: ` + { + "name": "web", + "port": 1111, + "connect": { + "SidecarService": { + "check": { + "TCP": "foo" + } + } + } + } + `, + wantNS: nil, + wantErr: "invalid check in sidecar_service", + }, + { + name: "invalid checks definitions in sidecar", + // Note no interval in the TCP check should fail validation + json: ` + { + "name": "web", + "port": 1111, + "connect": { + "SidecarService": { + "checks": [{ + "TCP": "foo" + }] + } + } + } + `, + wantNS: nil, + wantErr: "invalid check in sidecar_service", + }, + { + name: "invalid check status in sidecar", + // Note no interval in the TCP check should fail validation + json: ` + { + "name": "web", + "port": 1111, + "connect": { + "SidecarService": { + "check": { + "TCP": "foo", + "Interval": 10, + "Status": "unsupported-status" + } + } + } + } + `, + wantNS: nil, + wantErr: "Status for checks must 'passing', 'warning', 'critical'", + }, + { + name: "invalid checkS status in sidecar", + // Note no interval in the TCP check should fail validation + json: ` + { + "name": "web", + "port": 1111, + "connect": { + "SidecarService": { + "checks": [{ + "TCP": "foo", + "Interval": 10, + "Status": "unsupported-status" + }] + } + } + } + `, + wantNS: nil, + wantErr: "Status for checks must 'passing', 'warning', 'critical'", + }, + { + name: "another service registered with same ID as a sidecar should not be deregistered", + // Add another service with the same ID that a sidecar for web would have + preRegister: &structs.NodeService{ + ID: "web-sidecar-proxy", + Service: "fake-sidecar", + Port: 9999, + }, + // Register web with NO SIDECAR + json: ` + { + "name": "web", + "port": 1111 + } + `, + // Note here that although the registration here didn't register it, we + // should still see the NodeService we pre-registered here. + wantNS: &structs.NodeService{ + ID: "web-sidecar-proxy", + Service: "fake-sidecar", + Port: 9999, + }, + // After we deregister the web service above, the fake sidecar with + // clashing ID SHOULD NOT have been removed since it wasn't part of the + // original registration. + wantSidecarIDLeftAfterDereg: true, + }, + { + name: "updates to sidecar should work", + // Add a valid sidecar already registered + preRegister: &structs.NodeService{ + ID: "web-sidecar-proxy", + Service: "web-sidecar-proxy", + LocallyRegisteredAsSidecar: true, + Port: 9999, + }, + // Register web with Sidecar on different port + json: ` + { + "name": "web", + "port": 1111, + "connect": { + "SidecarService": { + "Port": 6666 + } + } + } + `, + // Note here that although the registration here didn't register it, we + // should still see the NodeService we pre-registered here. + wantNS: &structs.NodeService{ + Kind: "connect-proxy", + ID: "web-sidecar-proxy", + Service: "web-sidecar-proxy", + LocallyRegisteredAsSidecar: true, + Port: 6666, + Proxy: structs.ConnectProxyConfig{ + DestinationServiceName: "web", + DestinationServiceID: "web", + LocalServiceAddress: "127.0.0.1", + LocalServicePort: 1111, + }, + }, + }, + { + name: "update that removes sidecar should NOT deregister it", + // Add web with a valid sidecar already registered + preRegister: &structs.NodeService{ + ID: "web", + Service: "web", + Port: 1111, + }, + preRegister2: testDefaultSidecar("web", 1111), + // Register (update) web and remove sidecar (and port for sanity check) + json: ` + { + "name": "web", + "port": 2222 + } + `, + // Sidecar should still be there such that API can update registration + // without accidentally removing a sidecar. This is equivalent to embedded + // checks which are not removed by just not being included in an update. + // We will document that sidecar registrations via API must be explicitiy + // deregistered. + wantNS: testDefaultSidecar("web", 1111), + // Sanity check the rest of the update happened though. + assertStateFn: func(t *testing.T, state *local.State) { + svcs := state.Services() + svc, ok := svcs["web"] + require.True(t, ok) + require.Equal(t, 2222, svc.Port) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert := assert.New(t) + require := require.New(t) + + // Constrain auto ports to 1 available to make it deterministic + hcl := `ports { + sidecar_min_port = 2222 + sidecar_max_port = 2222 + } + ` + if tt.enableACL { + hcl = hcl + TestACLConfig() + } + + a := NewTestAgent(t.Name(), hcl) + defer a.Shutdown() + testrpc.WaitForLeader(t, a.RPC, "dc1") + + if tt.preRegister != nil { + require.NoError(a.AddService(tt.preRegister, nil, false, "")) + } + if tt.preRegister2 != nil { + require.NoError(a.AddService(tt.preRegister2, nil, false, "")) + } + + // Create an ACL token with require policy + var token string + if tt.enableACL && tt.tokenRules != "" { + args := map[string]interface{}{ + "Name": "User Token", + "Type": "client", + "Rules": tt.tokenRules, + } + req, _ := http.NewRequest("PUT", "/v1/acl/create?token=root", jsonReader(args)) + resp := httptest.NewRecorder() + obj, err := a.srv.ACLCreate(resp, req) + require.NoError(err) + require.NotNil(obj) + aclResp := obj.(aclCreateResponse) + token = aclResp.ID + } + + br := bytes.NewBufferString(tt.json) + + req, _ := http.NewRequest("PUT", "/v1/agent/service/register?token="+token, br) + resp := httptest.NewRecorder() + obj, err := a.srv.AgentRegisterService(resp, req) + if tt.wantErr != "" { + require.Error(err, "response code=%d, body:\n%s", + resp.Code, resp.Body.String()) + require.Contains(strings.ToLower(err.Error()), strings.ToLower(tt.wantErr)) + return + } + require.NoError(err) + assert.Nil(obj) + require.Equal(200, resp.Code, "request failed with body: %s", + resp.Body.String()) + + // Sanity the target service registration + svcs := a.State.Services() + + // Parse the expected definition into a ServiceDefinition + var sd structs.ServiceDefinition + err = json.Unmarshal([]byte(tt.json), &sd) + require.NoError(err) + require.NotEmpty(sd.Name) + + svcID := sd.ID + if svcID == "" { + svcID = sd.Name + } + svc, ok := svcs[svcID] + require.True(ok, "has service "+svcID) + assert.Equal(sd.Name, svc.Service) + assert.Equal(sd.Port, svc.Port) + // Ensure that the actual registered service _doesn't_ still have it's + // sidecar info since it's duplicate and we don't want that synced up to + // the catalog or included in responses particulary - it's just + // registration syntax sugar. + assert.Nil(svc.Connect.SidecarService) + + if tt.wantNS == nil { + // Sanity check that there was no service registered, we rely on there + // being no services at start of test so we can just use the count. + assert.Len(svcs, 1, "should be no sidecar registered") + return + } + + // Ensure sidecar + svc, ok = svcs[tt.wantNS.ID] + require.True(ok, "no sidecar registered at "+tt.wantNS.ID) + assert.Equal(tt.wantNS, svc) + + if tt.assertStateFn != nil { + tt.assertStateFn(t, a.State) + } + + // Now verify deregistration also removes sidecar (if there was one and it + // was added via sidecar not just coincidental ID clash) + { + req := httptest.NewRequest("PUT", + "/v1/agent/service/deregister/"+svcID+"?token="+token, nil) + resp := httptest.NewRecorder() + obj, err := a.srv.AgentDeregisterService(resp, req) + require.NoError(err) + require.Nil(obj) + + svcs := a.State.Services() + svc, ok = svcs[tt.wantNS.ID] + if tt.wantSidecarIDLeftAfterDereg { + require.True(ok, "removed non-sidecar service at "+tt.wantNS.ID) + } else { + require.False(ok, "sidecar not deregistered with service "+svcID) + } + } + }) + } +} + // This tests that connect proxy validation is done for local agent // registration. This doesn't need to test validation exhaustively since // that is done via a table test in the structs package. diff --git a/agent/agent_test.go b/agent/agent_test.go index 7b1bc6c0bc..6456fa846a 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -1987,6 +1987,72 @@ func TestAgent_loadServices_token(t *testing.T) { } } +func TestAgent_loadServices_sidecar(t *testing.T) { + t.Parallel() + a := NewTestAgent(t.Name(), ` + service = { + id = "rabbitmq" + name = "rabbitmq" + port = 5672 + token = "abc123" + connect = { + sidecar_service {} + } + } + `) + defer a.Shutdown() + + services := a.State.Services() + if _, ok := services["rabbitmq"]; !ok { + t.Fatalf("missing service") + } + if token := a.State.ServiceToken("rabbitmq"); token != "abc123" { + t.Fatalf("bad: %s", token) + } + if _, ok := services["rabbitmq-sidecar-proxy"]; !ok { + t.Fatalf("missing service") + } + if token := a.State.ServiceToken("rabbitmq-sidecar-proxy"); token != "abc123" { + t.Fatalf("bad: %s", token) + } + + // 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, services["rabbitmq"].Connect.SidecarService) +} + +func TestAgent_loadServices_sidecarSeparateToken(t *testing.T) { + t.Parallel() + a := NewTestAgent(t.Name(), ` + service = { + id = "rabbitmq" + name = "rabbitmq" + port = 5672 + token = "abc123" + connect = { + sidecar_service { + token = "789xyz" + } + } + } + `) + defer a.Shutdown() + + services := a.State.Services() + if _, ok := services["rabbitmq"]; !ok { + t.Fatalf("missing service") + } + if token := a.State.ServiceToken("rabbitmq"); token != "abc123" { + t.Fatalf("bad: %s", token) + } + if _, ok := services["rabbitmq-sidecar-proxy"]; !ok { + t.Fatalf("missing service") + } + if token := a.State.ServiceToken("rabbitmq-sidecar-proxy"); token != "789xyz" { + t.Fatalf("bad: %s", token) + } +} + func TestAgent_unloadServices(t *testing.T) { t.Parallel() a := NewTestAgent(t.Name(), "") @@ -2194,8 +2260,8 @@ func TestAgent_Service_Reap(t *testing.T) { } chkTypes := []*structs.CheckType{ &structs.CheckType{ - Status: api.HealthPassing, - TTL: 25 * time.Millisecond, + Status: api.HealthPassing, + TTL: 25 * time.Millisecond, DeregisterCriticalServiceAfter: 200 * time.Millisecond, }, } diff --git a/agent/config/builder.go b/agent/config/builder.go index 177e540d8d..7537ceb003 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -344,10 +344,16 @@ func (b *Builder) Build() (rt RuntimeConfig, err error) { serfPortWAN := b.portVal("ports.serf_wan", c.Ports.SerfWAN) proxyMinPort := b.portVal("ports.proxy_min_port", c.Ports.ProxyMinPort) proxyMaxPort := b.portVal("ports.proxy_max_port", c.Ports.ProxyMaxPort) + sidecarMinPort := b.portVal("ports.sidecar_min_port", c.Ports.SidecarMinPort) + sidecarMaxPort := b.portVal("ports.sidecar_max_port", c.Ports.SidecarMaxPort) if proxyMaxPort < proxyMinPort { return RuntimeConfig{}, fmt.Errorf( "proxy_min_port must be less than proxy_max_port. To disable, set both to zero.") } + if sidecarMaxPort < sidecarMinPort { + return RuntimeConfig{}, fmt.Errorf( + "sidecar_min_port must be less than sidecar_max_port. To disable, set both to zero.") + } // determine the default bind and advertise address // @@ -689,6 +695,8 @@ func (b *Builder) Build() (rt RuntimeConfig, err error) { ConnectProxyAllowManagedAPIRegistration: b.boolVal(c.Connect.Proxy.AllowManagedAPIRegistration), ConnectProxyBindMinPort: proxyMinPort, ConnectProxyBindMaxPort: proxyMaxPort, + ConnectSidecarMinPort: sidecarMinPort, + ConnectSidecarMaxPort: sidecarMaxPort, ConnectProxyDefaultExecMode: proxyDefaultExecMode, ConnectProxyDefaultDaemonCommand: proxyDefaultDaemonCommand, ConnectProxyDefaultScriptCommand: proxyDefaultScriptCommand, @@ -1184,9 +1192,29 @@ func (b *Builder) serviceConnectVal(v *ServiceConnect) *structs.ServiceConnect { } } + sidecar := b.serviceVal(v.SidecarService) + if sidecar != nil { + // Sanity checks + if sidecar.ID != "" { + b.err = multierror.Append(b.err, fmt.Errorf("sidecar_service can't speficy an ID")) + sidecar.ID = "" + } + if sidecar.Connect != nil { + if sidecar.Connect.SidecarService != nil { + b.err = multierror.Append(b.err, fmt.Errorf("sidecar_service can't have a nested sidecar_service")) + sidecar.Connect.SidecarService = nil + } + if sidecar.Connect.Proxy != nil { + b.err = multierror.Append(b.err, fmt.Errorf("sidecar_service can't have a managed proxy")) + sidecar.Connect.Proxy = nil + } + } + } + return &structs.ServiceConnect{ - Native: b.boolVal(v.Native), - Proxy: proxy, + Native: b.boolVal(v.Native), + Proxy: proxy, + SidecarService: sidecar, } } diff --git a/agent/config/config.go b/agent/config/config.go index 3a4e75e5b1..89f34d5584 100644 --- a/agent/config/config.go +++ b/agent/config/config.go @@ -90,6 +90,13 @@ func Parse(data string, format string) (c Config, err error) { "services.connect.proxy.upstreams", "service.proxy.upstreams", "services.proxy.upstreams", + + // Need all the service(s) exceptions also for nested sidecar service except + // managed proxy which is explicitly not supported there. + "service.connect.sidecar_service.checks", + "services.connect.sidecar_service.checks", + "service.connect.sidecar_service.proxy.upstreams", + "services.connect.sidecar_service.proxy.upstreams", }) // There is a difference of representation of some fields depending on @@ -383,6 +390,15 @@ type ServiceConnect struct { // Proxy configures a connect proxy instance for the service Proxy *ServiceConnectProxy `json:"proxy,omitempty" hcl:"proxy" mapstructure:"proxy"` + + // SidecarService is a nested Service Definition to register at the same time. + // It's purely a convenience mechanism to allow specifying a sidecar service + // along with the application service definition. It's nested nature allows + // all of the fields to be defaulted which can reduce the amount of + // boilerplate needed to register a sidecar service separately, but the end + // result is identical to just making a second service registration via any + // other means. + SidecarService *ServiceDefinition `json:"sidecar_service,omitempty" hcl:"sidecar_service" mapstructure:"sidecar_service"` } type ServiceConnectProxy struct { @@ -553,14 +569,16 @@ type Telemetry struct { } type Ports struct { - DNS *int `json:"dns,omitempty" hcl:"dns" mapstructure:"dns"` - HTTP *int `json:"http,omitempty" hcl:"http" mapstructure:"http"` - HTTPS *int `json:"https,omitempty" hcl:"https" mapstructure:"https"` - SerfLAN *int `json:"serf_lan,omitempty" hcl:"serf_lan" mapstructure:"serf_lan"` - SerfWAN *int `json:"serf_wan,omitempty" hcl:"serf_wan" mapstructure:"serf_wan"` - Server *int `json:"server,omitempty" hcl:"server" mapstructure:"server"` - ProxyMinPort *int `json:"proxy_min_port,omitempty" hcl:"proxy_min_port" mapstructure:"proxy_min_port"` - ProxyMaxPort *int `json:"proxy_max_port,omitempty" hcl:"proxy_max_port" mapstructure:"proxy_max_port"` + DNS *int `json:"dns,omitempty" hcl:"dns" mapstructure:"dns"` + HTTP *int `json:"http,omitempty" hcl:"http" mapstructure:"http"` + HTTPS *int `json:"https,omitempty" hcl:"https" mapstructure:"https"` + SerfLAN *int `json:"serf_lan,omitempty" hcl:"serf_lan" mapstructure:"serf_lan"` + SerfWAN *int `json:"serf_wan,omitempty" hcl:"serf_wan" mapstructure:"serf_wan"` + Server *int `json:"server,omitempty" hcl:"server" mapstructure:"server"` + ProxyMinPort *int `json:"proxy_min_port,omitempty" hcl:"proxy_min_port" mapstructure:"proxy_min_port"` + ProxyMaxPort *int `json:"proxy_max_port,omitempty" hcl:"proxy_max_port" mapstructure:"proxy_max_port"` + SidecarMinPort *int `json:"sidecar_min_port,omitempty" hcl:"sidecar_min_port" mapstructure:"sidecar_min_port"` + SidecarMaxPort *int `json:"sidecar_max_port,omitempty" hcl:"sidecar_max_port" mapstructure:"sidecar_max_port"` } type UnixSocket struct { diff --git a/agent/config/default.go b/agent/config/default.go index 017120d8ee..f737363638 100644 --- a/agent/config/default.go +++ b/agent/config/default.go @@ -107,6 +107,8 @@ func DefaultSource() Source { server = ` + strconv.Itoa(consul.DefaultRPCPort) + ` proxy_min_port = 20000 proxy_max_port = 20255 + sidecar_min_port = 21000 + sidecar_max_port = 21255 } telemetry = { metrics_prefix = "consul" diff --git a/agent/config/runtime.go b/agent/config/runtime.go index 7a81c8e54d..2b342c125e 100644 --- a/agent/config/runtime.go +++ b/agent/config/runtime.go @@ -462,6 +462,16 @@ type RuntimeConfig struct { // port is specified. ConnectProxyBindMaxPort int + // ConnectSidecarMinPort is the inclusive start of the range of ports + // allocated to the agent for asigning to sidecar services where no port is + // specified. + ConnectSidecarMinPort int + + // ConnectSidecarMaxPort is the inclusive end of the range of ports + // allocated to the agent for asigning to sidecar services where no port is + // specified + ConnectSidecarMaxPort int + // ConnectProxyAllowManagedRoot is true if Consul can execute managed // proxies when running as root (EUID == 0). ConnectProxyAllowManagedRoot bool diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index c87f57da89..b59000ddc3 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -1863,6 +1863,103 @@ func TestConfigFlagsAndEdgecases(t *testing.T) { `}, err: "Serf Advertise WAN address 10.0.0.1:1000 already configured for RPC Advertise", }, + { + desc: "sidecar_service can't have ID", + args: []string{ + `-data-dir=` + dataDir, + }, + json: []string{`{ + "service": { + "name": "web", + "port": 1234, + "connect": { + "sidecar_service": { + "ID": "random-sidecar-id" + } + } + } + }`}, + hcl: []string{` + service { + name = "web" + port = 1234 + connect { + sidecar_service { + ID = "random-sidecar-id" + } + } + } + `}, + err: "sidecar_service can't speficy an ID", + }, + { + desc: "sidecar_service can't have nested sidecar", + args: []string{ + `-data-dir=` + dataDir, + }, + json: []string{`{ + "service": { + "name": "web", + "port": 1234, + "connect": { + "sidecar_service": { + "connect": { + "sidecar_service": {} + } + } + } + } + }`}, + hcl: []string{` + service { + name = "web" + port = 1234 + connect { + sidecar_service { + connect { + sidecar_service { + } + } + } + } + } + `}, + err: "sidecar_service can't have a nested sidecar_service", + }, + { + desc: "sidecar_service can't have managed proxy", + args: []string{ + `-data-dir=` + dataDir, + }, + json: []string{`{ + "service": { + "name": "web", + "port": 1234, + "connect": { + "sidecar_service": { + "connect": { + "proxy": {} + } + } + } + } + }`}, + hcl: []string{` + service { + name = "web" + port = 1234 + connect { + sidecar_service { + connect { + proxy { + } + } + } + } + } + `}, + err: "sidecar_service can't have a managed proxy", + }, { desc: "telemetry.prefix_filter cannot be empty", args: []string{ @@ -2308,6 +2405,181 @@ func TestConfigFlagsAndEdgecases(t *testing.T) { rt.ConnectProxyAllowManagedAPIRegistration = true }, }, + + { + // This tests that we correct added the nested paths to arrays of objects + // to the exceptions in patchSliceOfMaps in config.go (for single service) + desc: "service.connectsidecar_service with checks and upstreams", + args: []string{ + `-data-dir=` + dataDir, + }, + json: []string{`{ + "service": { + "name": "web", + "port": 1234, + "connect": { + "sidecar_service": { + "port": 2345, + "checks": [ + { + "TCP": "127.0.0.1:2345", + "Interval": "10s" + } + ], + "proxy": { + "upstreams": [ + { + "destination_name": "db", + "local_bind_port": 7000 + } + ] + } + } + } + } + }`}, + hcl: []string{` + service { + name = "web" + port = 1234 + connect { + sidecar_service { + port = 2345 + checks = [ + { + tcp = "127.0.0.1:2345" + interval = "10s" + } + ] + proxy { + upstreams = [ + { + destination_name = "db" + local_bind_port = 7000 + }, + ] + } + } + } + } + `}, + patch: func(rt *RuntimeConfig) { + rt.DataDir = dataDir + rt.Services = []*structs.ServiceDefinition{ + { + Name: "web", + Port: 1234, + Connect: &structs.ServiceConnect{ + SidecarService: &structs.ServiceDefinition{ + Port: 2345, + Checks: structs.CheckTypes{ + { + TCP: "127.0.0.1:2345", + Interval: 10 * time.Second, + }, + }, + Proxy: &structs.ConnectProxyConfig{ + Upstreams: structs.Upstreams{ + structs.Upstream{ + DestinationType: "service", + DestinationName: "db", + LocalBindPort: 7000, + }, + }, + }, + }, + }, + }, + } + }, + }, + { + // This tests that we correct added the nested paths to arrays of objects + // to the exceptions in patchSliceOfMaps in config.go (for service*s*) + desc: "services.connect.sidecar_service with checks and upstreams", + args: []string{ + `-data-dir=` + dataDir, + }, + json: []string{`{ + "services": [{ + "name": "web", + "port": 1234, + "connect": { + "sidecar_service": { + "port": 2345, + "checks": [ + { + "TCP": "127.0.0.1:2345", + "Interval": "10s" + } + ], + "proxy": { + "upstreams": [ + { + "destination_name": "db", + "local_bind_port": 7000 + } + ] + } + } + } + }] + }`}, + hcl: []string{` + services = [{ + name = "web" + port = 1234 + connect { + sidecar_service { + port = 2345 + checks = [ + { + tcp = "127.0.0.1:2345" + interval = "10s" + } + ] + proxy { + upstreams = [ + { + destination_name = "db" + local_bind_port = 7000 + }, + ] + } + } + } + }] + `}, + patch: func(rt *RuntimeConfig) { + rt.DataDir = dataDir + rt.Services = []*structs.ServiceDefinition{ + { + Name: "web", + Port: 1234, + Connect: &structs.ServiceConnect{ + SidecarService: &structs.ServiceDefinition{ + Port: 2345, + Checks: structs.CheckTypes{ + { + TCP: "127.0.0.1:2345", + Interval: 10 * time.Second, + }, + }, + Proxy: &structs.ConnectProxyConfig{ + Upstreams: structs.Upstreams{ + structs.Upstream{ + DestinationType: "service", + DestinationName: "db", + LocalBindPort: 7000, + }, + }, + }, + }, + }, + }, + } + }, + }, } testConfig(t, tests, dataDir) @@ -2700,7 +2972,9 @@ func TestFullConfig(t *testing.T) { "https": 15127, "server": 3757, "proxy_min_port": 2000, - "proxy_max_port": 3000 + "proxy_max_port": 3000, + "sidecar_min_port": 8888, + "sidecar_max_port": 9999 }, "protocol": 30793, "raft_protocol": 19016, @@ -2850,6 +3124,9 @@ func TestFullConfig(t *testing.T) { "timeout": "38333s", "ttl": "57201s", "deregister_critical_service_after": "44214s" + }, + "connect": { + "sidecar_service": {} } }, { @@ -3224,6 +3501,8 @@ func TestFullConfig(t *testing.T) { server = 3757 proxy_min_port = 2000 proxy_max_port = 3000 + sidecar_min_port = 8888 + sidecar_max_port = 9999 } protocol = 30793 raft_protocol = 19016 @@ -3374,6 +3653,9 @@ func TestFullConfig(t *testing.T) { ttl = "57201s" deregister_critical_service_after = "44214s" } + connect { + sidecar_service {} + } }, { id = "MRHVMZuD" @@ -3756,6 +4038,8 @@ func TestFullConfig(t *testing.T) { ConnectEnabled: true, ConnectProxyBindMinPort: 2000, ConnectProxyBindMaxPort: 3000, + ConnectSidecarMinPort: 8888, + ConnectSidecarMaxPort: 9999, ConnectCAProvider: "consul", ConnectCAConfig: map[string]interface{}{ "RotationPeriod": "90h", @@ -3896,6 +4180,13 @@ func TestFullConfig(t *testing.T) { DeregisterCriticalServiceAfter: 44214 * time.Second, }, }, + // Note that although this SidecarService is only syntax sugar for + // registering another service, that has to happen in the agent code so + // it can make intelligent decisions about automatic port assignments + // etc. So we expect config just to pass it through verbatim. + Connect: &structs.ServiceConnect{ + SidecarService: &structs.ServiceDefinition{}, + }, }, { ID: "MRHVMZuD", @@ -4511,6 +4802,8 @@ func TestSanitize(t *testing.T) { "ConnectProxyDefaultDaemonCommand": [], "ConnectProxyDefaultExecMode": "", "ConnectProxyDefaultScriptCommand": [], + "ConnectSidecarMaxPort": 0, + "ConnectSidecarMinPort": 0, "ConnectTestDisableManagedProxies": false, "ConsulCoordinateUpdateBatchSize": 0, "ConsulCoordinateUpdateMaxBatches": 0, diff --git a/agent/config/translate.go b/agent/config/translate.go index 201df9d21e..51752b4527 100644 --- a/agent/config/translate.go +++ b/agent/config/translate.go @@ -1,6 +1,8 @@ package config -import "strings" +import ( + "strings" +) // TranslateKeys recursively translates all keys from m in-place to their // canonical form as defined in dict which maps an alias name to the canonical @@ -10,21 +12,64 @@ import "strings" // // Example: // -// m = TranslateKeys(m, map[string]string{"CamelCase": "snake_case"}) +// m = TranslateKeys(m, map[string]string{"snake_case": "CamelCase"}) // +// If the canonical string provided is the empty string, the effect is to stop +// recursing into any key matching the left hand side. In this case the left +// hand side must use periods to specify a full path e.g. +// `connect.proxy.config`. The path must be the canonical key names (i.e. +// CamelCase) AFTER translation so ExecMode not exec_mode. These are still match +// in a case-insensitive way. +// +// This is needed for example because parts of the Service Definition are +// "opaque" maps of metadata or config passed to another process or component. +// If we allow translation to recurse we might mangle the "opaque" keys given +// where the clash with key names in other parts of the definition (and they do +// in practice with deprecated managed proxy upstreams) :sob: +// +// Example: +// m - TranslateKeys(m, map[string]string{ +// "foo_bar": "FooBar", +// "widget.config": "", +// // Assume widgets is an array, this will prevent recursing into any +// // item's config field +// "widgets.config": "", +// }) func TranslateKeys(v map[string]interface{}, dict map[string]string) { - ck(v, dict) + // Convert all dict keys for exclusions to lower. so we can match against them + // unambiguously with a single lookup. + for k, v := range dict { + if v == "" { + dict[strings.ToLower(k)] = "" + } + } + ck(v, dict, "") } -func ck(v interface{}, dict map[string]string) interface{} { +func ck(v interface{}, dict map[string]string, pathPfx string) interface{} { + // In array case we don't add a path segment for the item as they are all + // assumed to be same which is why we check the prefix doesn't already end in + // a . + if pathPfx != "" && !strings.HasSuffix(pathPfx, ".") { + pathPfx += "." + } switch x := v.(type) { case map[string]interface{}: for k, v := range x { - canonKey := dict[strings.ToLower(k)] + lowerK := strings.ToLower(k) + + // Check if this path has been excluded + val, ok := dict[pathPfx+lowerK] + if ok && val == "" { + // Don't recurse into this key + continue + } + + canonKey, ok := dict[lowerK] // no canonical key? -> use this key - if canonKey == "" { - x[k] = ck(v, dict) + if !ok { + x[k] = ck(v, dict, pathPfx+lowerK) continue } @@ -37,14 +82,14 @@ func ck(v interface{}, dict map[string]string) interface{} { } // otherwise translate to the canonical key - x[canonKey] = ck(v, dict) + x[canonKey] = ck(v, dict, pathPfx+strings.ToLower(canonKey)) } return x case []interface{}: var a []interface{} for _, xv := range x { - a = append(a, ck(xv, dict)) + a = append(a, ck(xv, dict, pathPfx)) } return a diff --git a/agent/sidecar_service.go b/agent/sidecar_service.go new file mode 100644 index 0000000000..f8c3c03d5b --- /dev/null +++ b/agent/sidecar_service.go @@ -0,0 +1,146 @@ +package agent + +import ( + "fmt" + "math/rand" + "time" + + "github.com/hashicorp/consul/agent/structs" +) + +func (a *Agent) sidecarServiceID(serviceID string) string { + return serviceID + "-sidecar-proxy" +} + +// sidecarServiceFromNodeService returns a *structs.NodeService representing a +// sidecar service with all defaults populated based on the current agent +// config. +// +// It assumes the ns has been validated already which means the nested +// SidecarService is also already validated.It also assumes that any check +// definitions within the sidecar service definition have been validated if +// necessary. If no sidecar service is defined in ns, then nil is returned with +// nil error. +// +// The second return argument is a list of CheckTypes to register along with the +// service. +// +// The third return argument is the effective Token to use for the sidecar +// registration. This will be the same as the token parameter passed unless the +// SidecarService definition contains a distint one. +func (a *Agent) sidecarServiceFromNodeService(ns *structs.NodeService, token string) (*structs.NodeService, []*structs.CheckType, string, error) { + if ns.Connect.SidecarService == nil { + return nil, nil, "", nil + } + + // Start with normal conversion from service definition + sidecar := ns.Connect.SidecarService.NodeService() + + // Override the ID which must always be consistent for a given outer service + // ID. We rely on this for lifecycle management of the nested definition. + sidecar.ID = a.sidecarServiceID(ns.ID) + + // Set some meta we can use to disambiguate between service instances we added + // later and are responsible for deregistering. + if sidecar.Meta != nil { + // Meta is non-nil validate it before we add the special key so we can + // enforce that user cannot add a consul- prefix one. + if err := structs.ValidateMetadata(sidecar.Meta, false); err != nil { + return nil, nil, "", err + } + } + + // Flag this as a sidecar - this is not persisted in catalog but only needed + // in local agent state to disambiguate lineage when deregistereing the parent + // service later. + sidecar.LocallyRegisteredAsSidecar = true + + // See if there is a more specific token for the sidecar registration + if ns.Connect.SidecarService.Token != "" { + token = ns.Connect.SidecarService.Token + } + + // Setup some sane connect proxy defaults. + if sidecar.Kind == "" { + sidecar.Kind = structs.ServiceKindConnectProxy + } + if sidecar.Service == "" { + sidecar.Service = ns.Service + "-sidecar-proxy" + } + if sidecar.Address == "" { + // Inherit address from the service if it's provided + sidecar.Address = ns.Address + } + // Proxy defaults + if sidecar.Proxy.DestinationServiceName == "" { + sidecar.Proxy.DestinationServiceName = ns.Service + } + if sidecar.Proxy.DestinationServiceID == "" { + sidecar.Proxy.DestinationServiceID = ns.ID + } + if sidecar.Proxy.LocalServiceAddress == "" { + sidecar.Proxy.LocalServiceAddress = "127.0.0.1" + } + if sidecar.Proxy.LocalServicePort < 1 { + sidecar.Proxy.LocalServicePort = ns.Port + } + + // 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 { + // This should be a really short list so don't bother optimising lookup yet. + OUTER: + for _, offset := range rand.Perm(rangeLen) { + p := a.config.ConnectSidecarMinPort + offset + // See if this port was already allocated to another service + for _, otherNS := range a.State.Services() { + if otherNS.Port == p { + // already taken, skip to next random pick in the range + continue OUTER + } + } + // We made it through all existing proxies without a match so claim this one + sidecar.Port = p + break + } + } + // If no ports left (or auto ports disabled) fail + if sidecar.Port < 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 " + + "and auto-assignement disabled in config") + } + return nil, nil, "", 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{ + &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: fmt.Sprintf("127.0.0.1:%d", sidecar.Port), + Interval: 10 * time.Second, + }, + &structs.CheckType{ + Name: "Connect Sidecar Aliasing " + ns.ID, + AliasService: ns.ID, + }, + } + } + + return sidecar, checks, token, nil +} diff --git a/agent/sidecar_service_test.go b/agent/sidecar_service_test.go new file mode 100644 index 0000000000..3f3e7a7734 --- /dev/null +++ b/agent/sidecar_service_test.go @@ -0,0 +1,248 @@ +package agent + +import ( + "testing" + "time" + + "github.com/hashicorp/consul/agent/structs" + "github.com/stretchr/testify/require" +) + +func TestAgent_sidecarServiceFromNodeService(t *testing.T) { + tests := []struct { + name string + preRegister *structs.ServiceDefinition + sd *structs.ServiceDefinition + token string + autoPortsDisabled bool + wantNS *structs.NodeService + wantChecks []*structs.CheckType + wantToken string + wantErr string + }{ + { + name: "no sidecar", + sd: &structs.ServiceDefinition{ + Name: "web", + Port: 1111, + }, + token: "foo", + wantNS: nil, + wantChecks: nil, + wantToken: "", + wantErr: "", // Should NOT error + }, + { + name: "all the defaults", + sd: &structs.ServiceDefinition{ + ID: "web1", + Name: "web", + Port: 1111, + Connect: &structs.ServiceConnect{ + SidecarService: &structs.ServiceDefinition{}, + }, + }, + token: "foo", + wantNS: &structs.NodeService{ + Kind: structs.ServiceKindConnectProxy, + ID: "web1-sidecar-proxy", + Service: "web-sidecar-proxy", + Port: 2222, + LocallyRegisteredAsSidecar: true, + Proxy: structs.ConnectProxyConfig{ + DestinationServiceName: "web", + DestinationServiceID: "web1", + LocalServiceAddress: "127.0.0.1", + LocalServicePort: 1111, + }, + }, + wantChecks: []*structs.CheckType{ + &structs.CheckType{ + Name: "Connect Sidecar Listening", + TCP: "127.0.0.1:2222", + Interval: 10 * time.Second, + }, + &structs.CheckType{ + Name: "Connect Sidecar Aliasing web1", + AliasService: "web1", + }, + }, + wantToken: "foo", + }, + { + name: "all the allowed overrides", + sd: &structs.ServiceDefinition{ + ID: "web1", + Name: "web", + Port: 1111, + Connect: &structs.ServiceConnect{ + SidecarService: &structs.ServiceDefinition{ + Name: "motorbike1", + Port: 3333, + Tags: []string{"foo", "bar"}, + Address: "127.127.127.127", + Meta: map[string]string{"foo": "bar"}, + Check: structs.CheckType{ + ScriptArgs: []string{"sleep", "1"}, + Interval: 999 * time.Second, + }, + Token: "custom-token", + EnableTagOverride: true, + Proxy: &structs.ConnectProxyConfig{ + DestinationServiceName: "web", + DestinationServiceID: "web1", + LocalServiceAddress: "127.0.127.0", + LocalServicePort: 9999, + Config: map[string]interface{}{"baz": "qux"}, + Upstreams: structs.TestUpstreams(t), + }, + }, + }, + }, + token: "foo", + wantNS: &structs.NodeService{ + Kind: structs.ServiceKindConnectProxy, + ID: "web1-sidecar-proxy", + Service: "motorbike1", + Port: 3333, + Tags: []string{"foo", "bar"}, + Address: "127.127.127.127", + Meta: map[string]string{ + "foo": "bar", + }, + LocallyRegisteredAsSidecar: true, + EnableTagOverride: true, + Proxy: structs.ConnectProxyConfig{ + DestinationServiceName: "web", + DestinationServiceID: "web1", + LocalServiceAddress: "127.0.127.0", + LocalServicePort: 9999, + Config: map[string]interface{}{"baz": "qux"}, + Upstreams: structs.TestAddDefaultsToUpstreams(t, structs.TestUpstreams(t)), + }, + }, + wantChecks: []*structs.CheckType{ + &structs.CheckType{ + ScriptArgs: []string{"sleep", "1"}, + Interval: 999 * time.Second, + }, + }, + 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-assignement disabled in config", + }, + { + name: "invalid check type", + sd: &structs.ServiceDefinition{ + ID: "web1", + Name: "web", + Port: 1111, + Connect: &structs.ServiceConnect{ + SidecarService: &structs.ServiceDefinition{ + Check: structs.CheckType{ + TCP: "foo", + // Invalid since no interval specified + }, + }, + }, + }, + token: "foo", + wantErr: "Interval must be > 0", + }, + { + name: "invalid meta", + sd: &structs.ServiceDefinition{ + ID: "web1", + Name: "web", + Port: 1111, + Connect: &structs.ServiceConnect{ + SidecarService: &structs.ServiceDefinition{ + Meta: map[string]string{ + "consul-reserved-key-should-be-rejected": "true", + }, + }, + }, + }, + token: "foo", + wantErr: "reserved for internal use", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Set port range to make it deterministic. This allows a single assigned + // port at 2222 thanks to being inclusive at both ends. + hcl := ` + ports { + sidecar_min_port = 2222 + sidecar_max_port = 2222 + } + ` + if tt.autoPortsDisabled { + hcl = ` + ports { + sidecar_min_port = 0 + sidecar_max_port = 0 + } + ` + } + + require := require.New(t) + a := NewTestAgent("jones", hcl) + + if tt.preRegister != nil { + err := a.AddService(tt.preRegister.NodeService(), nil, false, "") + require.NoError(err) + } + + ns := tt.sd.NodeService() + err := ns.Validate() + require.NoError(err, "Invalid test case - NodeService must validate") + + gotNS, gotChecks, gotToken, err := a.sidecarServiceFromNodeService(ns, tt.token) + if tt.wantErr != "" { + require.Error(err) + require.Contains(err.Error(), tt.wantErr) + return + } + + require.NoError(err) + require.Equal(tt.wantNS, gotNS) + require.Equal(tt.wantChecks, gotChecks) + require.Equal(tt.wantToken, gotToken) + }) + } +} diff --git a/agent/structs/connect_proxy_config.go b/agent/structs/connect_proxy_config.go index 1297d2c0be..80cada307c 100644 --- a/agent/structs/connect_proxy_config.go +++ b/agent/structs/connect_proxy_config.go @@ -14,7 +14,7 @@ import ( type ConnectProxyConfig struct { // DestinationServiceName is required and is the name of the service to accept // traffic for. - DestinationServiceName string + DestinationServiceName string `json:",omitempty"` // DestinationServiceID is optional and should only be specified for // "side-car" style proxies where the proxy is in front of just a single @@ -22,27 +22,27 @@ type ConnectProxyConfig struct { // being represented which must be registered to the same agent. It's valid to // provide a service ID that does not yet exist to avoid timing issues when // bootstrapping a service with a proxy. - DestinationServiceID string + DestinationServiceID string `json:",omitempty"` // LocalServiceAddress is the address of the local service instance. It is // optional and should only be specified for "side-car" style proxies. It will // default to 127.0.0.1 if the proxy is a "side-car" (DestinationServiceID is // set) but otherwise will be ignored. - LocalServiceAddress string + LocalServiceAddress string `json:",omitempty"` // LocalServicePort is the port of the local service instance. It is optional // and should only be specified for "side-car" style proxies. It will default // to the registered port for the instance if the proxy is a "side-car" // (DestinationServiceID is set) but otherwise will be ignored. - LocalServicePort int + LocalServicePort int `json:",omitempty"` // Config is the arbitrary configuration data provided with the proxy // registration. - Config map[string]interface{} + Config map[string]interface{} `json:",omitempty"` // Upstreams describes any upstream dependencies the proxy instance should // setup. - Upstreams Upstreams + Upstreams Upstreams `json:",omitempty"` } // ToAPI returns the api struct with the same fields. We have duplicates to diff --git a/agent/structs/structs.go b/agent/structs/structs.go index f5f9d13c98..0a940fe516 100644 --- a/agent/structs/structs.go +++ b/agent/structs/structs.go @@ -658,6 +658,25 @@ type NodeService struct { // a pointer so that we never have to nil-check this. Connect ServiceConnect + // 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 + // 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 + // deregister regular services if they happen to be registered using the same + // ID scheme as our sidecars do by default. We could use meta but that gets + // unpleasant because we can't use the consul- prefix from an agent (reserved + // for use internally but in practice that means within the state store or in + // responses only), and it leaks the detail publically which people might rely + // on which is a bit unpleasant for something that is meant to be config-file + // syntax sugar. Note this is not translated to ServiceNode and friends and + // may not be set on a NodeService that isn't the one the agent registered and + // keeps in it's local state. We never want this rendered in JSON as it's + // internal only. Right now our agent endpoints return api structs which don't + // include it but this is a safety net incase we change that or there is + // somewhere this is used in API output. + LocallyRegisteredAsSidecar bool `json:"-"` + RaftIndex } @@ -665,12 +684,21 @@ type NodeService struct { // definitions from the agent to the state store. type ServiceConnect struct { // Native is true when this service can natively understand Connect. - Native bool + Native bool `json:",omitempty"` // Proxy configures a connect proxy instance for the service. This is // only used for agent service definitions and is invalid for non-agent // (catalog API) definitions. - Proxy *ServiceDefinitionConnectProxy + Proxy *ServiceDefinitionConnectProxy `json:",omitempty"` + + // SidecarService is a nested Service Definition to register at the same time. + // It's purely a convenience mechanism to allow specifying a sidecar service + // along with the application service definition. It's nested nature allows + // all of the fields to be defaulted which can reduce the amount of + // boilerplate needed to register a sidecar service separately, but the end + // result is identical to just making a second service registration via any + // other means. + SidecarService *ServiceDefinition `json:",omitempty"` } // Validate validates the node service configuration. @@ -708,6 +736,25 @@ func (s *NodeService) Validate() error { } } + // Nested sidecar validation + if s.Connect.SidecarService != nil { + if s.Connect.SidecarService.ID != "" { + result = multierror.Append(result, fmt.Errorf( + "A SidecarService cannot specify an ID as this is managed by the "+ + "agent")) + } + if s.Connect.SidecarService.Connect != nil { + if s.Connect.SidecarService.Connect.SidecarService != nil { + result = multierror.Append(result, fmt.Errorf( + "A SidecarService cannot have a nested SidecarService")) + } + if s.Connect.SidecarService.Connect.Proxy != nil { + result = multierror.Append(result, fmt.Errorf( + "A SidecarService cannot have a managed proxy")) + } + } + } + return result } diff --git a/agent/structs/structs_test.go b/agent/structs/structs_test.go index f78658a1a2..903f9386ea 100644 --- a/agent/structs/structs_test.go +++ b/agent/structs/structs_test.go @@ -284,6 +284,62 @@ func TestStructs_NodeService_ValidateConnectProxy(t *testing.T) { } } +func TestStructs_NodeService_ValidateSidecarService(t *testing.T) { + cases := []struct { + Name string + Modify func(*NodeService) + Err string + }{ + { + "valid", + func(x *NodeService) {}, + "", + }, + + { + "ID can't be set", + func(x *NodeService) { x.Connect.SidecarService.ID = "foo" }, + "SidecarService cannot specify an ID", + }, + + { + "Nested sidecar can't be set", + func(x *NodeService) { + x.Connect.SidecarService.Connect = &ServiceConnect{ + SidecarService: &ServiceDefinition{}, + } + }, + "SidecarService cannot have a nested SidecarService", + }, + + { + "Sidecar can't have managed proxy", + func(x *NodeService) { + x.Connect.SidecarService.Connect = &ServiceConnect{ + Proxy: &ServiceDefinitionConnectProxy{}, + } + }, + "SidecarService cannot have a managed proxy", + }, + } + + for _, tc := range cases { + t.Run(tc.Name, func(t *testing.T) { + assert := assert.New(t) + ns := TestNodeServiceSidecar(t) + tc.Modify(ns) + + err := ns.Validate() + assert.Equal(err != nil, tc.Err != "", err) + if err == nil { + return + } + + assert.Contains(strings.ToLower(err.Error()), strings.ToLower(tc.Err)) + }) + } +} + func TestStructs_NodeService_IsSame(t *testing.T) { ns := &NodeService{ ID: "node1", diff --git a/agent/structs/testing_catalog.go b/agent/structs/testing_catalog.go index a0884465ff..f5586aaaae 100644 --- a/agent/structs/testing_catalog.go +++ b/agent/structs/testing_catalog.go @@ -48,3 +48,15 @@ func TestNodeServiceProxy(t testing.T) *NodeService { Proxy: TestConnectProxyConfig(t), } } + +// TestNodeServiceSidecar returns a *NodeService representing a service +// registration with a nested Sidecar registration. +func TestNodeServiceSidecar(t testing.T) *NodeService { + return &NodeService{ + Service: "web", + Port: 2222, + Connect: ServiceConnect{ + SidecarService: &ServiceDefinition{}, + }, + } +} diff --git a/agent/structs/testing_connect_proxy_config.go b/agent/structs/testing_connect_proxy_config.go index 21a30251a2..aa058f58ff 100644 --- a/agent/structs/testing_connect_proxy_config.go +++ b/agent/structs/testing_connect_proxy_config.go @@ -34,3 +34,19 @@ func TestUpstreams(t testing.T) Upstreams { }, } } + +// TestAddDefaultsToUpstreams takes an array of upstreams (such as that from +// TestUpstreams) and adds default values that are populated during +// refigistration. Use this for generating the expected Upstreams value after +// registration. +func TestAddDefaultsToUpstreams(t testing.T, upstreams []Upstream) []Upstream { + ups := make([]Upstream, len(upstreams)) + for i := range upstreams { + ups[i] = upstreams[i] + // Fill in default fields we expect to have back explicitly in a response + if ups[i].DestinationType == "" { + ups[i].DestinationType = UpstreamDestTypeService + } + } + return ups +} diff --git a/api/agent.go b/api/agent.go index 8c0cc4be0a..9c6665ffe4 100644 --- a/api/agent.go +++ b/api/agent.go @@ -90,8 +90,9 @@ type AgentService struct { // AgentServiceConnect represents the Connect configuration of a service. type AgentServiceConnect struct { - Native bool `json:",omitempty"` - Proxy *AgentServiceConnectProxy `json:",omitempty"` + Native bool `json:",omitempty"` + Proxy *AgentServiceConnectProxy `json:",omitempty"` + SidecarService *AgentServiceRegistration `json:",omitempty"` } // AgentServiceConnectProxy represents the Connect Proxy configuration of a diff --git a/api/agent_test.go b/api/agent_test.go index 9c7e647098..02fb0f0881 100644 --- a/api/agent_test.go +++ b/api/agent_test.go @@ -323,6 +323,52 @@ func TestAPI_AgentServices_ManagedConnectProxyDeprecatedUpstreams(t *testing.T) } } +func TestAPI_AgentServices_SidecarService(t *testing.T) { + t.Parallel() + c, s := makeClient(t) + defer s.Stop() + + agent := c.Agent() + + // Register service + reg := &AgentServiceRegistration{ + Name: "foo", + Port: 8000, + Connect: &AgentServiceConnect{ + SidecarService: &AgentServiceRegistration{}, + }, + } + if err := agent.ServiceRegister(reg); err != nil { + t.Fatalf("err: %v", err) + } + + services, err := agent.Services() + if err != nil { + t.Fatalf("err: %v", err) + } + if _, ok := services["foo"]; !ok { + t.Fatalf("missing service: %v", services) + } + if _, ok := services["foo-sidecar-proxy"]; !ok { + t.Fatalf("missing sidecar service: %v", services) + } + + if err := agent.ServiceDeregister("foo"); err != nil { + t.Fatalf("err: %v", err) + } + + // Deregister should have removed both service and it's sidecar + services, err = agent.Services() + require.NoError(t, err) + + if _, ok := services["foo"]; ok { + t.Fatalf("didn't remove service: %v", services) + } + if _, ok := services["foo-sidecar-proxy"]; ok { + t.Fatalf("didn't remove sidecar service: %v", services) + } +} + func TestAPI_AgentServices_ExternalConnectProxy(t *testing.T) { t.Parallel() c, s := makeClient(t)