From c2266b134ae03f29ef2ebac156359e58823b2900 Mon Sep 17 00:00:00 2001 From: Paul Banks Date: Tue, 17 Apr 2018 13:29:02 +0100 Subject: [PATCH] HTTP agent registration allows proxy to be defined. --- agent/agent.go | 12 +++-- agent/agent_endpoint.go | 14 +++++ agent/agent_endpoint_test.go | 79 +++++++++++++++++++++++++++-- agent/config/builder.go | 50 ++++-------------- agent/config/runtime.go | 4 -- agent/config/runtime_test.go | 28 +++++----- agent/structs/service_definition.go | 62 ++++++++++++++++++++++ 7 files changed, 182 insertions(+), 67 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index b988029ce..03f7677d0 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -2426,9 +2426,15 @@ func (a *Agent) unloadChecks() error { // loadProxies will load connect proxy definitions from configuration and // persisted definitions on disk, and load them into the local agent. func (a *Agent) loadProxies(conf *config.RuntimeConfig) error { - for _, proxy := range conf.ConnectProxies { - if err := a.AddProxy(proxy, false); err != nil { - return fmt.Errorf("failed adding proxy: %s", err) + for _, svc := range conf.Services { + if svc.Connect != nil { + proxy, err := svc.ConnectManagedProxy() + if err != nil { + return fmt.Errorf("failed adding proxy: %s", err) + } + if err := a.AddProxy(proxy, false); err != nil { + return fmt.Errorf("failed adding proxy: %s", err) + } } } diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index 722909467..43013785f 100644 --- a/agent/agent_endpoint.go +++ b/agent/agent_endpoint.go @@ -589,10 +589,24 @@ func (s *HTTPServer) AgentRegisterService(resp http.ResponseWriter, req *http.Re return nil, err } + // Get any proxy registrations + proxy, err := args.ConnectManagedProxy() + if err != nil { + resp.WriteHeader(http.StatusBadRequest) + fmt.Fprintf(resp, err.Error()) + return nil, nil + } + // Add the service. if err := s.agent.AddService(ns, chkTypes, true, token); err != nil { return nil, err } + // Add proxy (which will add proxy service so do it before we trigger sync) + if proxy != nil { + if err := s.agent.AddProxy(proxy, true); 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 1c6f7d830..9d8591126 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -26,6 +26,7 @@ import ( "github.com/hashicorp/serf/serf" "github.com/pascaldekloe/goe/verify" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func makeReadOnlyAgentACL(t *testing.T, srv *HTTPServer) string { @@ -1369,10 +1370,78 @@ func TestAgent_RegisterService_InvalidAddress(t *testing.T) { } } -// This tests local agent service registration of a connect proxy. This -// verifies that it is put in the local state store properly for syncing -// later. -func TestAgent_RegisterService_ConnectProxy(t *testing.T) { +// This tests local agent service registration with a managed proxy. +func TestAgent_RegisterService_ManagedConnectProxy(t *testing.T) { + t.Parallel() + + assert := assert.New(t) + require := require.New(t) + a := NewTestAgent(t.Name(), "") + defer a.Shutdown() + + // Register a proxy. Note that the destination doesn't exist here on + // this agent or in the catalog at all. This is intended and part + // of the design. + args := &structs.ServiceDefinition{ + Name: "web", + Port: 8000, + // This is needed just because empty check struct (not pointer) get json + // encoded as object with zero values and then decoded back to object with + // zero values _except that the header map is an empty map not a nil map_. + // So our check to see if s.Check.Empty() returns false since DeepEqual + // considers empty maps and nil maps to be different types. Then the request + // fails validation because the Check definition isn't valid... This is jank + // we should fix but it's another yak I don't want to shave right now. + Check: structs.CheckType{ + TTL: 15 * time.Second, + }, + Connect: &structs.ServiceDefinitionConnect{ + Proxy: &structs.ServiceDefinitionConnectProxy{ + ExecMode: "script", + Command: "proxy.sh", + Config: map[string]interface{}{ + "foo": "bar", + }, + }, + }, + } + + req, _ := http.NewRequest("PUT", "/v1/agent/service/register?token=abc123", jsonReader(args)) + resp := httptest.NewRecorder() + obj, err := a.srv.AgentRegisterService(resp, req) + assert.NoError(err) + assert.Nil(obj) + require.Equal(200, resp.Code, "request failed with body: %s", + resp.Body.String()) + + // Ensure the target service + _, ok := a.State.Services()["web"] + assert.True(ok, "has service") + + // Ensure the proxy service was registered + proxySvc, ok := a.State.Services()["web-proxy"] + require.True(ok, "has proxy service") + assert.Equal(structs.ServiceKindConnectProxy, proxySvc.Kind) + assert.Equal("web", proxySvc.ProxyDestination) + assert.NotEmpty(proxySvc.Port, "a port should have been assigned") + + // Ensure proxy itself was registered + proxy := a.State.Proxy("web-proxy") + require.NotNil(proxy) + assert.Equal(structs.ProxyExecModeScript, proxy.ExecMode) + assert.Equal("proxy.sh", proxy.Command) + assert.Equal(args.Connect.Proxy.Config, proxy.Config) + + // Ensure the token was configured + assert.Equal("abc123", a.State.ServiceToken("web")) + assert.Equal("abc123", a.State.ServiceToken("web-proxy")) +} + +// This tests local agent service registration of a unmanaged connect proxy. +// This verifies that it is put in the local state store properly for syncing +// later. Note that _managed_ connect proxies are registered as part of the +// target service's registration. +func TestAgent_RegisterService_UnmanagedConnectProxy(t *testing.T) { t.Parallel() assert := assert.New(t) @@ -1411,7 +1480,7 @@ func TestAgent_RegisterService_ConnectProxy(t *testing.T) { // 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. -func TestAgent_RegisterService_ConnectProxyInvalid(t *testing.T) { +func TestAgent_RegisterService_UnmanagedConnectProxyInvalid(t *testing.T) { t.Parallel() assert := assert.New(t) diff --git a/agent/config/builder.go b/agent/config/builder.go index a6338ae14..ec36e9ab0 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -322,15 +322,8 @@ func (b *Builder) Build() (rt RuntimeConfig, err error) { } var services []*structs.ServiceDefinition - var proxies []*structs.ConnectManagedProxy for _, service := range c.Services { services = append(services, b.serviceVal(&service)) - // Register any connect proxies requested - if proxy := b.connectManagedProxyVal(&service); proxy != nil { - proxies = append(proxies, proxy) - } - // TODO(banks): support connect-native registrations (v.Connect.Enabled == - // true) } if c.Service != nil { services = append(services, b.serviceVal(c.Service)) @@ -648,7 +641,6 @@ func (b *Builder) Build() (rt RuntimeConfig, err error) { CheckUpdateInterval: b.durationVal("check_update_interval", c.CheckUpdateInterval), Checks: checks, ClientAddrs: clientAddrs, - ConnectProxies: proxies, ConnectProxyBindMinPort: proxyBindMinPort, ConnectProxyBindMaxPort: proxyBindMaxPort, DataDir: b.stringVal(c.DataDir), @@ -1020,46 +1012,26 @@ func (b *Builder) serviceVal(v *ServiceDefinition) *structs.ServiceDefinition { Token: b.stringVal(v.Token), EnableTagOverride: b.boolVal(v.EnableTagOverride), Checks: checks, + Connect: b.serviceConnectVal(v.Connect), } } -func (b *Builder) connectManagedProxyVal(v *ServiceDefinition) *structs.ConnectManagedProxy { - if v.Connect == nil || v.Connect.Proxy == nil { +func (b *Builder) serviceConnectVal(v *ServiceConnect) *structs.ServiceDefinitionConnect { + if v == nil { return nil } - p := v.Connect.Proxy - - targetID := b.stringVal(v.ID) - if targetID == "" { - targetID = b.stringVal(v.Name) - } - - execMode := structs.ProxyExecModeDaemon - if p.ExecMode != nil { - switch *p.ExecMode { - case "daemon": - execMode = structs.ProxyExecModeDaemon - case "script": - execMode = structs.ProxyExecModeScript - default: - b.err = multierror.Append(fmt.Errorf( - "service[%s]: invalid connect proxy exec_mode: %s", targetID, - *p.ExecMode)) - return nil + var proxy *structs.ServiceDefinitionConnectProxy + if v.Proxy != nil { + proxy = &structs.ServiceDefinitionConnectProxy{ + ExecMode: b.stringVal(v.Proxy.ExecMode), + Command: b.stringVal(v.Proxy.Command), + Config: v.Proxy.Config, } } - return &structs.ConnectManagedProxy{ - ExecMode: execMode, - Command: b.stringVal(p.Command), - Config: p.Config, - // ProxyService will be setup when the agent registers the configured - // proxies and starts them etc. We could do it here but we may need to do - // things like probe the OS for a free port etc. And we have enough info to - // resolve all this later. - ProxyService: nil, - TargetServiceID: targetID, + return &structs.ServiceDefinitionConnect{ + Proxy: proxy, } } diff --git a/agent/config/runtime.go b/agent/config/runtime.go index 55c15d14e..b31630d27 100644 --- a/agent/config/runtime.go +++ b/agent/config/runtime.go @@ -621,10 +621,6 @@ type RuntimeConfig struct { // that. ConnectEnabled bool - // ConnectProxies is a list of configured proxies taken from the "connect" - // block of service registrations. - ConnectProxies []*structs.ConnectManagedProxy - // ConnectProxyBindMinPort is the inclusive start of the range of ports // allocated to the agent for starting proxy listeners on where no explicit // port is specified. diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index e990f0689..773b7a036 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -3403,21 +3403,8 @@ func TestFullConfig(t *testing.T) { DeregisterCriticalServiceAfter: 13209 * time.Second, }, }, - CheckUpdateInterval: 16507 * time.Second, - ClientAddrs: []*net.IPAddr{ipAddr("93.83.18.19")}, - ConnectProxies: []*structs.ConnectManagedProxy{ - { - ExecMode: structs.ProxyExecModeDaemon, - Command: "awesome-proxy", - Config: map[string]interface{}{ - "foo": "qux", // Overriden by service - // Note globals are not merged here but on rendering to the proxy - // endpoint. That's because proxies can be added later too so merging - // at config time is redundant if we have to do it later anyway. - }, - TargetServiceID: "MRHVMZuD", - }, - }, + CheckUpdateInterval: 16507 * time.Second, + ClientAddrs: []*net.IPAddr{ipAddr("93.83.18.19")}, ConnectProxyBindMinPort: 2000, ConnectProxyBindMaxPort: 3000, DNSAddrs: []net.Addr{tcpAddr("93.95.95.81:7001"), udpAddr("93.95.95.81:7001")}, @@ -3592,6 +3579,15 @@ func TestFullConfig(t *testing.T) { DeregisterCriticalServiceAfter: 68482 * time.Second, }, }, + Connect: &structs.ServiceDefinitionConnect{ + Proxy: &structs.ServiceDefinitionConnectProxy{ + ExecMode: "daemon", + Command: "awesome-proxy", + Config: map[string]interface{}{ + "foo": "qux", + }, + }, + }, }, { ID: "dLOXpSCI", @@ -4082,7 +4078,6 @@ func TestSanitize(t *testing.T) { ], "ClientAddrs": [], "ConnectEnabled": false, - "ConnectProxies": [], "ConnectProxyBindMaxPort": 0, "ConnectProxyBindMinPort": 0, "ConnectProxyDefaultConfig": {}, @@ -4219,6 +4214,7 @@ func TestSanitize(t *testing.T) { "Timeout": "0s" }, "Checks": [], + "Connect": null, "EnableTagOverride": false, "ID": "", "Kind": "", diff --git a/agent/structs/service_definition.go b/agent/structs/service_definition.go index a10f1527f..ad77d8e3b 100644 --- a/agent/structs/service_definition.go +++ b/agent/structs/service_definition.go @@ -1,5 +1,9 @@ package structs +import ( + "fmt" +) + // ServiceDefinition is used to JSON decode the Service definitions. For // documentation on specific fields see NodeService which is better documented. type ServiceDefinition struct { @@ -15,6 +19,7 @@ type ServiceDefinition struct { Token string EnableTagOverride bool ProxyDestination string + Connect *ServiceDefinitionConnect } func (s *ServiceDefinition) NodeService() *NodeService { @@ -35,6 +40,45 @@ func (s *ServiceDefinition) NodeService() *NodeService { return ns } +// ConnectManagedProxy returns a ConnectManagedProxy from the ServiceDefinition +// if one is configured validly. Note that is may return nil if no proxy is +// configured and will also return nil error in this case too as it's an +// expected case. The error returned indicates that there was an attempt to +// configure a proxy made but that it was invalid input, e.g. invalid +// "exec_mode". +func (s *ServiceDefinition) ConnectManagedProxy() (*ConnectManagedProxy, error) { + if s.Connect == nil || s.Connect.Proxy == nil { + return nil, nil + } + + // NodeService performs some simple normalization like copying ID from Name + // which we shouldn't hard code ourselves here... + ns := s.NodeService() + + execMode := ProxyExecModeDaemon + switch s.Connect.Proxy.ExecMode { + case "": + execMode = ProxyExecModeDaemon + case "daemon": + execMode = ProxyExecModeDaemon + case "script": + execMode = ProxyExecModeScript + default: + return nil, fmt.Errorf("invalid exec mode: %s", s.Connect.Proxy.ExecMode) + } + + p := &ConnectManagedProxy{ + ExecMode: execMode, + Command: s.Connect.Proxy.Command, + Config: s.Connect.Proxy.Config, + // ProxyService will be setup when the agent registers the configured + // proxies and starts them etc. + TargetServiceID: ns.ID, + } + + return p, nil +} + func (s *ServiceDefinition) CheckTypes() (checks CheckTypes, err error) { if !s.Check.Empty() { err := s.Check.Validate() @@ -51,3 +95,21 @@ func (s *ServiceDefinition) CheckTypes() (checks CheckTypes, err error) { } return checks, nil } + +// ServiceDefinitionConnect is the connect block within a service registration. +// Note this is duplicated in config.ServiceConnect and needs to be kept in +// sync. +type ServiceDefinitionConnect struct { + // TODO(banks) add way to specify that the app is connect-native + // Proxy configures a connect proxy instance for the service + Proxy *ServiceDefinitionConnectProxy `json:"proxy,omitempty" hcl:"proxy" mapstructure:"proxy"` +} + +// ServiceDefinitionConnectProxy is the connect proxy config within a service +// registration. Note this is duplicated in config.ServiceConnectProxy and needs +// to be kept in sync. +type ServiceDefinitionConnectProxy struct { + Command string `json:"command,omitempty" hcl:"command" mapstructure:"command"` + ExecMode string `json:"exec_mode,omitempty" hcl:"exec_mode" mapstructure:"exec_mode"` + Config map[string]interface{} `json:"config,omitempty" hcl:"config" mapstructure:"config"` +}