diff --git a/agent/agent.go b/agent/agent.go index fb4d6c4a7a..6f77547e64 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -2051,20 +2051,11 @@ func (a *Agent) AddProxy(proxy *structs.ConnectManagedProxy, persist bool) error // Lookup the target service token in state if there is one. token := a.State.ServiceToken(proxy.TargetServiceID) - // Determine if we need to default the command - if proxy.ExecMode == structs.ProxyExecModeDaemon && len(proxy.Command) == 0 { - // We use the globally configured default command. If it is empty - // then we need to determine the subcommand for this agent. - cmd := a.config.ConnectProxyDefaultDaemonCommand - if len(cmd) == 0 { - var err error - cmd, err = a.defaultProxyCommand() - if err != nil { - return err - } - } - - proxy.CommandDefault = cmd + // Copy the basic proxy structure so it isn't modified w/ defaults + proxyCopy := *proxy + proxy = &proxyCopy + if err := a.applyProxyDefaults(proxy); err != nil { + return err } // Add the proxy to local state first since we may need to assign a port which @@ -2090,6 +2081,47 @@ func (a *Agent) AddProxy(proxy *structs.ConnectManagedProxy, persist bool) error return nil } +// applyProxyDefaults modifies the given proxy by applying any configured +// defaults, such as the default execution mode, command, etc. +func (a *Agent) applyProxyDefaults(proxy *structs.ConnectManagedProxy) error { + // Set the default exec mode + if proxy.ExecMode == structs.ProxyExecModeUnspecified { + mode, err := structs.NewProxyExecMode(a.config.ConnectProxyDefaultExecMode) + if err != nil { + return err + } + + proxy.ExecMode = mode + } + if proxy.ExecMode == structs.ProxyExecModeUnspecified { + proxy.ExecMode = structs.ProxyExecModeDaemon + } + + // Set the default command to the globally configured default + if len(proxy.Command) == 0 { + switch proxy.ExecMode { + case structs.ProxyExecModeDaemon: + proxy.Command = a.config.ConnectProxyDefaultDaemonCommand + + case structs.ProxyExecModeScript: + proxy.Command = a.config.ConnectProxyDefaultScriptCommand + } + } + + // If there is no globally configured default we need to get the + // default command so we can do "consul connect proxy" + if len(proxy.Command) == 0 { + command, err := defaultProxyCommand() + if err != nil { + return err + } + + proxy.Command = command + } + + return nil +} + // RemoveProxy stops and removes a local proxy instance. func (a *Agent) RemoveProxy(proxyID string, persist bool) error { // Validate proxyID @@ -2107,19 +2139,6 @@ func (a *Agent) RemoveProxy(proxyID string, persist bool) error { return nil } -// defaultProxyCommand returns the default Connect managed proxy command. -func (a *Agent) defaultProxyCommand() ([]string, error) { - // Get the path to the current exectuable. This is cached once by the - // library so this is effectively just a variable read. - execPath, err := os.Executable() - if err != nil { - return nil, err - } - - // "consul connect proxy" default value for managed daemon proxy - return []string{execPath, "connect", "proxy"}, nil -} - func (a *Agent) cancelCheckMonitors(checkID types.CheckID) { // Stop any monitors delete(a.checkReapAfter, checkID) @@ -2751,3 +2770,16 @@ func (a *Agent) registerCache() { RefreshTimeout: 10 * time.Minute, }) } + +// defaultProxyCommand returns the default Connect managed proxy command. +func defaultProxyCommand() ([]string, error) { + // Get the path to the current exectuable. This is cached once by the + // library so this is effectively just a variable read. + execPath, err := os.Executable() + if err != nil { + return nil, err + } + + // "consul connect proxy" default value for managed daemon proxy + return []string{execPath, "connect", "proxy"}, nil +} diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index ea81ba0b1d..8f080ea7a1 100644 --- a/agent/agent_endpoint.go +++ b/agent/agent_endpoint.go @@ -1007,33 +1007,6 @@ func (s *HTTPServer) AgentConnectProxyConfig(resp http.ResponseWriter, req *http } } - execMode := "daemon" - // If there is a global default mode use that instead - if s.agent.config.ConnectProxyDefaultExecMode != "" { - execMode = s.agent.config.ConnectProxyDefaultExecMode - } - // If it's actually set though, use the one set - if proxy.Proxy.ExecMode != structs.ProxyExecModeUnspecified { - execMode = proxy.Proxy.ExecMode.String() - } - - // TODO(banks): default the binary to current binary. Probably needs to be - // done deeper though as it will be needed for actually managing proxy - // lifecycle. - command := proxy.Proxy.Command - if len(command) == 0 { - if execMode == "daemon" { - command = s.agent.config.ConnectProxyDefaultDaemonCommand - } - if execMode == "script" { - command = s.agent.config.ConnectProxyDefaultScriptCommand - } - } - // No global defaults set either... - if len(command) == 0 { - command = []string{"consul", "connect", "proxy"} - } - // Set defaults for anything that is still not specified but required. // Note that these are not included in the content hash. Since we expect // them to be static in general but some like the default target service @@ -1061,8 +1034,8 @@ func (s *HTTPServer) AgentConnectProxyConfig(resp http.ResponseWriter, req *http TargetServiceID: target.ID, TargetServiceName: target.Service, ContentHash: contentHash, - ExecMode: api.ProxyExecMode(execMode), - Command: command, + ExecMode: api.ProxyExecMode(proxy.Proxy.ExecMode.String()), + Command: proxy.Proxy.Command, Config: config, } return contentHash, reply, nil diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index 98f95e69e2..fa01eab89a 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -2334,6 +2334,7 @@ func TestAgentConnectProxyConfig_Blocking(t *testing.T) { }, Connect: &structs.ServiceDefinitionConnect{ Proxy: &structs.ServiceDefinitionConnectProxy{ + Command: []string{"tubes.sh"}, Config: map[string]interface{}{ "bind_port": 1234, "connect_timeout_ms": 500, @@ -2352,9 +2353,9 @@ func TestAgentConnectProxyConfig_Blocking(t *testing.T) { ProxyServiceID: "test-proxy", TargetServiceID: "test", TargetServiceName: "test", - ContentHash: "365a50cbb9a748b6", + ContentHash: "4662e51e78609569", ExecMode: "daemon", - Command: []string{"consul", "connect", "proxy"}, + Command: []string{"tubes.sh"}, Config: map[string]interface{}{ "upstreams": []interface{}{ map[string]interface{}{ @@ -2372,7 +2373,7 @@ func TestAgentConnectProxyConfig_Blocking(t *testing.T) { ur, err := copystructure.Copy(expectedResponse) require.NoError(t, err) updatedResponse := ur.(*api.ConnectProxyConfig) - updatedResponse.ContentHash = "538d0366b7b1dc3e" + updatedResponse.ContentHash = "23b5b6b3767601e1" upstreams := updatedResponse.Config["upstreams"].([]interface{}) upstreams = append(upstreams, map[string]interface{}{ @@ -2519,6 +2520,10 @@ func TestAgentConnectProxyConfig_Blocking(t *testing.T) { func TestAgentConnectProxyConfig_ConfigHandling(t *testing.T) { t.Parallel() + // Get the default command to compare below + defaultCommand, err := defaultProxyCommand() + require.NoError(t, err) + // Define a local service with a managed proxy. It's registered in the test // loop to make sure agent state is predictable whatever order tests execute // since some alter this service config. @@ -2555,7 +2560,7 @@ func TestAgentConnectProxyConfig_ConfigHandling(t *testing.T) { `, proxy: structs.ServiceDefinitionConnectProxy{}, wantMode: api.ProxyExecModeDaemon, - wantCommand: []string{"consul", "connect", "proxy"}, + wantCommand: defaultCommand, wantConfig: map[string]interface{}{ "bind_address": "0.0.0.0", "bind_port": 10000, // "randomly" chosen from our range of 1 @@ -2629,7 +2634,7 @@ func TestAgentConnectProxyConfig_ConfigHandling(t *testing.T) { }, }, wantMode: api.ProxyExecModeDaemon, - wantCommand: []string{"consul", "connect", "proxy"}, + wantCommand: defaultCommand, wantConfig: map[string]interface{}{ "bind_address": "0.0.0.0", "bind_port": 10000, // "randomly" chosen from our range of 1 diff --git a/agent/agent_test.go b/agent/agent_test.go index 768d1c9512..d0fcbff5e2 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -2389,6 +2389,7 @@ func TestAgent_AddProxy(t *testing.T) { // Test the ID was created as we expect. got := a.State.Proxy("web-proxy") + tt.proxy.ProxyService = got.Proxy.ProxyService require.Equal(tt.proxy, got.Proxy) }) } @@ -2412,12 +2413,14 @@ func TestAgent_RemoveProxy(t *testing.T) { // Add a proxy for web pReg := &structs.ConnectManagedProxy{ TargetServiceID: "web", + ExecMode: structs.ProxyExecModeDaemon, Command: []string{"foo"}, } require.NoError(a.AddProxy(pReg, false)) // Test the ID was created as we expect. gotProxy := a.State.Proxy("web-proxy") + gotProxy.Proxy.ProxyService = nil require.Equal(pReg, gotProxy.Proxy) err := a.RemoveProxy("web-proxy", false) diff --git a/agent/proxy/manager.go b/agent/proxy/manager.go index 4e45d22a83..4b9971d4c8 100644 --- a/agent/proxy/manager.go +++ b/agent/proxy/manager.go @@ -332,9 +332,6 @@ func (m *Manager) newProxy(mp *local.ManagedProxy) (Proxy, error) { switch p.ExecMode { case structs.ProxyExecModeDaemon: command := p.Command - if len(command) == 0 { - command = p.CommandDefault - } // This should never happen since validation should happen upstream // but verify it because the alternative is to panic below. diff --git a/agent/structs/connect.go b/agent/structs/connect.go index aca9764fa9..e08d016464 100644 --- a/agent/structs/connect.go +++ b/agent/structs/connect.go @@ -1,6 +1,8 @@ package structs import ( + "fmt" + "github.com/mitchellh/mapstructure" ) @@ -40,6 +42,20 @@ const ( ProxyExecModeTest ) +// NewProxyExecMode returns the proper ProxyExecMode for the given string value. +func NewProxyExecMode(raw string) (ProxyExecMode, error) { + switch raw { + case "": + return ProxyExecModeUnspecified, nil + case "daemon": + return ProxyExecModeDaemon, nil + case "script": + return ProxyExecModeScript, nil + default: + return 0, fmt.Errorf("invalid exec mode: %s", raw) + } +} + // String implements Stringer func (m ProxyExecMode) String() string { switch m { @@ -73,9 +89,6 @@ type ConnectManagedProxy struct { // for ProxyExecModeScript. Command []string - // CommandDefault is the default command to execute if Command is empty. - CommandDefault []string `json:"-" hash:"ignore"` - // Config is the arbitrary configuration data provided with the registration. Config map[string]interface{} diff --git a/agent/structs/service_definition.go b/agent/structs/service_definition.go index 7163b55494..be69a7b57e 100644 --- a/agent/structs/service_definition.go +++ b/agent/structs/service_definition.go @@ -1,9 +1,5 @@ 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 { @@ -55,17 +51,9 @@ func (s *ServiceDefinition) ConnectManagedProxy() (*ConnectManagedProxy, error) // which we shouldn't hard code ourselves here... ns := s.NodeService() - execMode := ProxyExecModeUnspecified - switch s.Connect.Proxy.ExecMode { - case "": - // Use default - break - case "daemon": - execMode = ProxyExecModeDaemon - case "script": - execMode = ProxyExecModeScript - default: - return nil, fmt.Errorf("invalid exec mode: %s", s.Connect.Proxy.ExecMode) + execMode, err := NewProxyExecMode(s.Connect.Proxy.ExecMode) + if err != nil { + return nil, err } p := &ConnectManagedProxy{