agent: clean up defaulting of proxy configuration

This cleans up and unifies how proxy settings defaults are applied.

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
+}

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

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

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)

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.

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{}
 

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{