Merge pull request #7498 from hashicorp/dnephin/small-cleanup

envoy: small cleanup in cmd and server

agent/agent.go

@@ -291,9 +291,6 @@ type Agent struct {
 	// the centrally configured proxy/service defaults.
 	serviceManager *ServiceManager
 
-	// xdsServer is the Server instance that serves xDS gRPC API.
-	xdsServer *xds.Server
-
 	// grpcServer is the server instance used currently to serve xDS API for
 	// Envoy.
 	grpcServer *grpc.Server
@@ -736,7 +733,7 @@ func (a *Agent) listenAndServeGRPC() error {
 		return nil
 	}
 
-	a.xdsServer = &xds.Server{
+	xdsServer := &xds.Server{
 		Logger:       a.logger,
 		CfgMgr:       a.proxyConfig,
 		Authz:        a,
@@ -744,15 +741,15 @@ func (a *Agent) listenAndServeGRPC() error {
 		CheckFetcher: a,
 		CfgFetcher:   a,
 	}
-	a.xdsServer.Initialize()
+	xdsServer.Initialize()
 
 	var err error
 	if a.config.HTTPSPort > 0 {
 		// gRPC uses the same TLS settings as the HTTPS API. If HTTPS is
 		// enabled then gRPC will require HTTPS as well.
-		a.grpcServer, err = a.xdsServer.GRPCServer(a.tlsConfigurator)
+		a.grpcServer, err = xdsServer.GRPCServer(a.tlsConfigurator)
 	} else {
-		a.grpcServer, err = a.xdsServer.GRPCServer(nil)
+		a.grpcServer, err = xdsServer.GRPCServer(nil)
 	}
 	if err != nil {
 		return err

command/connect/envoy/envoy.go

@@ -74,13 +74,13 @@ const defaultEnvoyVersion = "1.13.1"
 func (c *cmd) init() {
 	c.flags = flag.NewFlagSet("", flag.ContinueOnError)
 
-	c.flags.StringVar(&c.proxyID, "proxy-id", "",
+	c.flags.StringVar(&c.proxyID, "proxy-id", os.Getenv("CONNECT_PROXY_ID"),
 		"The proxy's ID on the local agent.")
 
 	c.flags.BoolVar(&c.meshGateway, "mesh-gateway", false,
 		"Configure Envoy as a Mesh Gateway.")
 
-	c.flags.StringVar(&c.sidecarFor, "sidecar-for", "",
+	c.flags.StringVar(&c.sidecarFor, "sidecar-for", os.Getenv("CONNECT_SIDECAR_FOR"),
 		"The ID of a service instance on the local agent that this proxy should "+
 			"become a sidecar for. It requires that the proxy service is registered "+
 			"with the agent as a connect-proxy with Proxy.DestinationServiceID set "+
@@ -110,7 +110,7 @@ func (c *cmd) init() {
 			"cases where either assumption is violated this flag will prevent the "+
 			"command attempting to resolve config from the local agent.")
 
-	c.flags.StringVar(&c.grpcAddr, "grpc-addr", "",
+	c.flags.StringVar(&c.grpcAddr, "grpc-addr", os.Getenv(api.GRPCAddrEnvName),
 		"Set the agent's gRPC address and port (in http(s)://host:port format). "+
 			"Alternatively, you can specify CONSUL_GRPC_ADDR in ENV.")
 
@@ -208,7 +208,6 @@ func canBindInternal(addr string, ifAddrs []net.Addr) bool {
 
 func canBind(addr string) bool {
 	ifAddrs, err := net.InterfaceAddrs()
-
 	if err != nil {
 		return false
 	}
@@ -222,17 +221,6 @@ func (c *cmd) Run(args []string) int {
 	}
 	passThroughArgs := c.flags.Args()
 
-	// Load the proxy ID and token from env vars if they're set
-	if c.proxyID == "" {
-		c.proxyID = os.Getenv("CONNECT_PROXY_ID")
-	}
-	if c.sidecarFor == "" {
-		c.sidecarFor = os.Getenv("CONNECT_SIDECAR_FOR")
-	}
-	if c.grpcAddr == "" {
-		c.grpcAddr = os.Getenv(api.GRPCAddrEnvName)
-	}
-
 	// Setup Consul client
 	client, err := c.http.APIClient()
 	if err != nil {
@@ -355,14 +343,14 @@ func (c *cmd) Run(args []string) int {
 
 	// See if we need to lookup proxyID
 	if c.proxyID == "" && c.sidecarFor != "" {
-		proxyID, err := c.lookupProxyIDForSidecar()
+		proxyID, err := proxyCmd.LookupProxyIDForSidecar(c.client, c.sidecarFor)
 		if err != nil {
 			c.UI.Error(err.Error())
 			return 1
 		}
 		c.proxyID = proxyID
 	} else if c.proxyID == "" && c.meshGateway {
-		gatewaySvc, err := c.lookupGatewayProxy()
+		gatewaySvc, err := proxyCmd.LookupGatewayProxy(c.client)
 		if err != nil {
 			c.UI.Error(err.Error())
 			return 1
@@ -372,8 +360,7 @@ func (c *cmd) Run(args []string) int {
 	}
 
 	if c.proxyID == "" {
-		c.UI.Error("No proxy ID specified. One of -proxy-id or -sidecar-for/-mesh-gateway is " +
+		c.UI.Error("No proxy ID specified. One of -proxy-id or -sidecar-for/-mesh-gateway is required")
-			"required")
 		return 1
 	}
 
@@ -583,14 +570,6 @@ func (c *cmd) generateConfig() ([]byte, error) {
 	return bsCfg.GenerateJSON(args)
 }
 
-func (c *cmd) lookupProxyIDForSidecar() (string, error) {
-	return proxyCmd.LookupProxyIDForSidecar(c.client, c.sidecarFor)
-}
-
-func (c *cmd) lookupGatewayProxy() (*api.AgentService, error) {
-	return proxyCmd.LookupGatewayProxy(c.client)
-}
-
 func (c *cmd) lookupGRPCPort() (int, error) {
 	self, err := c.client.Agent().Self()
 	if err != nil {

command/connect/envoy/envoy_test.go

@@ -81,7 +81,6 @@ func TestGenerateConfig(t *testing.T) {
 		{
 			Name:  "defaults",
 			Flags: []string{"-proxy-id", "test-proxy"},
-			Env:   []string{},
 			WantArgs: BootstrapTplArgs{
 				EnvoyVersion:          defaultEnvoyVersion,
 				ProxyCluster:          "test-proxy",
@@ -98,7 +97,6 @@ func TestGenerateConfig(t *testing.T) {
 			Name: "token-arg",
 			Flags: []string{"-proxy-id", "test-proxy",
 				"-token", "c9a52720-bf6c-4aa6-b8bc-66881a5ade95"},
-			Env: []string{},
 			WantArgs: BootstrapTplArgs{
 				EnvoyVersion:          defaultEnvoyVersion,
 				ProxyCluster:          "test-proxy",
@@ -136,7 +134,6 @@ func TestGenerateConfig(t *testing.T) {
 			Flags: []string{"-proxy-id", "test-proxy",
 				"-token-file", "@@TEMPDIR@@token.txt",
 			},
-			Env: []string{},
 			Files: map[string]string{
 				"token.txt": "c9a52720-bf6c-4aa6-b8bc-66881a5ade95",
 			},
@@ -179,7 +176,6 @@ func TestGenerateConfig(t *testing.T) {
 			Name: "grpc-addr-flag",
 			Flags: []string{"-proxy-id", "test-proxy",
 				"-grpc-addr", "localhost:9999"},
-			Env: []string{},
 			WantArgs: BootstrapTplArgs{
 				EnvoyVersion: defaultEnvoyVersion,
 				ProxyCluster: "test-proxy",
@@ -220,7 +216,6 @@ func TestGenerateConfig(t *testing.T) {
 			Name: "grpc-addr-unix",
 			Flags: []string{"-proxy-id", "test-proxy",
 				"-grpc-addr", "unix:///var/run/consul.sock"},
-			Env: []string{},
 			WantArgs: BootstrapTplArgs{
 				EnvoyVersion:          defaultEnvoyVersion,
 				ProxyCluster:          "test-proxy",
@@ -254,7 +249,6 @@ func TestGenerateConfig(t *testing.T) {
 		{
 			Name:  "access-log-path",
 			Flags: []string{"-proxy-id", "test-proxy", "-admin-access-log-path", "/some/path/access.log"},
-			Env:   []string{},
 			WantArgs: BootstrapTplArgs{
 				EnvoyVersion: defaultEnvoyVersion,
 				ProxyCluster: "test-proxy",
@@ -273,7 +267,6 @@ func TestGenerateConfig(t *testing.T) {
 		{
 			Name:  "missing-ca-file",
 			Flags: []string{"-proxy-id", "test-proxy", "-ca-file", "some/path"},
-			Env:   []string{},
 			WantArgs: BootstrapTplArgs{
 				EnvoyVersion: defaultEnvoyVersion,
 				ProxyCluster: "test-proxy",
@@ -310,7 +303,6 @@ func TestGenerateConfig(t *testing.T) {
 		{
 			Name:  "custom-bootstrap",
 			Flags: []string{"-proxy-id", "test-proxy"},
-			Env:   []string{},
 			ProxyConfig: map[string]interface{}{
 				// Add a completely custom bootstrap template. Never mind if this is
 				// invalid envoy config just as long as it works and gets the variables
@@ -348,7 +340,6 @@ func TestGenerateConfig(t *testing.T) {
 		{
 			Name:  "extra_-single",
 			Flags: []string{"-proxy-id", "test-proxy"},
-			Env:   []string{},
 			ProxyConfig: map[string]interface{}{
 				// Add a custom sections with interpolated variables. These are all
 				// invalid config syntax too but we are just testing they have the right
@@ -381,7 +372,6 @@ func TestGenerateConfig(t *testing.T) {
 		{
 			Name:  "extra_-multiple",
 			Flags: []string{"-proxy-id", "test-proxy"},
-			Env:   []string{},
 			ProxyConfig: map[string]interface{}{
 				// Add a custom sections with interpolated variables. These are all
 				// invalid config syntax too but we are just testing they have the right
@@ -419,7 +409,6 @@ func TestGenerateConfig(t *testing.T) {
 		{
 			Name:  "stats-config-override",
 			Flags: []string{"-proxy-id", "test-proxy"},
-			Env:   []string{},
 			ProxyConfig: map[string]interface{}{
 				// Add a custom sections with interpolated variables. These are all
 				// invalid config syntax too but we are just testing they have the right
@@ -444,7 +433,6 @@ func TestGenerateConfig(t *testing.T) {
 		{
 			Name:  "zipkin-tracing-config",
 			Flags: []string{"-proxy-id", "test-proxy"},
-			Env:   []string{},
 			ProxyConfig: map[string]interface{}{
 				// Add a custom sections with interpolated variables. These are all
 				// invalid config syntax too but we are just testing they have the right
@@ -520,9 +508,6 @@ func TestGenerateConfig(t *testing.T) {
 				}
 			}
 
-			ui := cli.NewMockUi()
-			c := New(ui)
-
 			// Run a mock agent API that just always returns the proxy config in the
 			// test.
 			srv := httptest.NewServer(testMockAgent(tc.ProxyConfig, tc.GRPCPort))
@@ -530,15 +515,15 @@ func TestGenerateConfig(t *testing.T) {
 
 			// Set the agent HTTP address in ENV to be our mock
 			tc.Env = append(tc.Env, "CONSUL_HTTP_ADDR="+srv.URL)
-
 			testDirPrefix := testDir + string(filepath.Separator)
-
-			myFlags := copyAndReplaceAll(tc.Flags, "@@TEMPDIR@@", testDirPrefix)
 			myEnv := copyAndReplaceAll(tc.Env, "@@TEMPDIR@@", testDirPrefix)
-
 			defer testSetAndResetEnv(t, myEnv)()
 
+			ui := cli.NewMockUi()
+			c := New(ui)
+
 			// Run the command
+			myFlags := copyAndReplaceAll(tc.Flags, "@@TEMPDIR@@", testDirPrefix)
 			args := append([]string{"-bootstrap"}, myFlags...)
 			code := c.Run(args)
 			if tc.WantErr == "" {