From 830b4a15f62d9039037560ec43bfa7000767e15b Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 7 Apr 2020 15:53:12 -0400 Subject: [PATCH 1/4] Step 1: move all the grpcAddr logic into the same spot There is no reason a reader should have to jump around to find this value. It is only used in 1 place --- command/connect/envoy/envoy.go | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/command/connect/envoy/envoy.go b/command/connect/envoy/envoy.go index a917856f7f..cdceda2970 100644 --- a/command/connect/envoy/envoy.go +++ b/command/connect/envoy/envoy.go @@ -343,21 +343,6 @@ func (c *cmd) Run(args []string) int { return 1 } - // See if we need to lookup grpcAddr - if c.grpcAddr == "" { - port, err := c.lookupGRPCPort() - if err != nil { - c.UI.Error(fmt.Sprintf("Error connecting to Consul agent: %s", err)) - } - if port <= 0 { - // This is the dev mode default and recommended production setting if - // enabled. - port = 8502 - c.UI.Info(fmt.Sprintf("Defaulting to grpc port = %d", port)) - } - c.grpcAddr = fmt.Sprintf("localhost:%v", port) - } - // Generate config bootstrapJson, err := c.generateConfig() if err != nil { @@ -411,6 +396,21 @@ func (c *cmd) templateArgs() (*BootstrapTplArgs, error) { return nil, err } + // See if we need to lookup grpcAddr + if c.grpcAddr == "" { + port, err := c.lookupGRPCPort() + if err != nil { + c.UI.Error(fmt.Sprintf("Error connecting to Consul agent: %s", err)) + } + if port <= 0 { + // This is the dev mode default and recommended production setting if + // enabled. + port = 8502 + c.UI.Info(fmt.Sprintf("Defaulting to grpc port = %d", port)) + } + c.grpcAddr = fmt.Sprintf("localhost:%v", port) + } + // Decide on TLS if the scheme is provided and indicates it, if the HTTP env // suggests TLS is supported explicitly (CONSUL_HTTP_SSL) or implicitly // (CONSUL_HTTP_ADDR) is https:// From 1a8ffec6a7fcb1f7bca67240e03a58db3f4ab6da Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 7 Apr 2020 16:33:22 -0400 Subject: [PATCH 2/4] Step 2: extract the grpc address logic and a new type The new grpcAddress function contains all of the logic to translate the command line options into the values used in the template. The new type has two advantages. 1. It introduces a logical grouping of values in the BootstrapTplArgs struct which is exceptionally large. This grouping makes the struct easier to understand because each set of nested values can be seen as a single entity. 2. It gives us a reasonable return value for this new function. --- command/connect/envoy/bootstrap_tpl.go | 37 +++--- command/connect/envoy/envoy.go | 140 +++++++++++---------- command/connect/envoy/envoy_test.go | 168 +++++++++++++++---------- 3 files changed, 196 insertions(+), 149 deletions(-) diff --git a/command/connect/envoy/bootstrap_tpl.go b/command/connect/envoy/bootstrap_tpl.go index 54287069be..bbd87a23cc 100644 --- a/command/connect/envoy/bootstrap_tpl.go +++ b/command/connect/envoy/bootstrap_tpl.go @@ -3,6 +3,8 @@ package envoy // BootstrapTplArgs is the set of arguments that may be interpolated into the // Envoy bootstrap template. type BootstrapTplArgs struct { + GRPC + // ProxyCluster is the cluster name for the the Envoy `node` specification and // is typically the same as the ProxyID. ProxyCluster string @@ -12,26 +14,10 @@ type BootstrapTplArgs struct { // the agent to deliver the correct configuration. ProxyID string - // AgentAddress is the IP address of the local agent where the proxy instance - // is registered. - AgentAddress string - - // AgentPort is the gRPC port exposed on the local agent. - AgentPort string - - // AgentTLS is true if the local agent gRPC service should be accessed over - // TLS. - AgentTLS bool - // AgentCAPEM is the CA to use to verify the local agent gRPC service if // TLS is enabled. AgentCAPEM string - // AgentSocket is the path to a Unix Socket for communicating with the - // local agent's gRPC endpoint. Disabled if the empty (the default), - // but overrides AgentAddress and AgentPort if set. - AgentSocket string - // AdminAccessLogPath The path to write the access log for the // administration server. If no access log is desired specify // "/dev/null". By default it will use "/dev/null". @@ -102,6 +88,25 @@ type BootstrapTplArgs struct { EnvoyVersion string } +// GRPC settings used in the bootstrap template. +type GRPC struct { + // AgentAddress is the IP address of the local agent where the proxy instance + // is registered. + AgentAddress string + + // AgentPort is the gRPC port exposed on the local agent. + AgentPort string + + // AgentTLS is true if the local agent gRPC service should be accessed over + // TLS. + AgentTLS bool + + // AgentSocket is the path to a Unix Socket for communicating with the + // local agent's gRPC endpoint. Disabled if the empty (the default), + // but overrides AgentAddress and AgentPort if set. + AgentSocket string +} + const bootstrapTemplate = `{ "admin": { "access_log_path": "{{ .AdminAccessLogPath }}", diff --git a/command/connect/envoy/envoy.go b/command/connect/envoy/envoy.go index cdceda2970..13824d39b5 100644 --- a/command/connect/envoy/envoy.go +++ b/command/connect/envoy/envoy.go @@ -396,68 +396,9 @@ func (c *cmd) templateArgs() (*BootstrapTplArgs, error) { return nil, err } - // See if we need to lookup grpcAddr - if c.grpcAddr == "" { - port, err := c.lookupGRPCPort() - if err != nil { - c.UI.Error(fmt.Sprintf("Error connecting to Consul agent: %s", err)) - } - if port <= 0 { - // This is the dev mode default and recommended production setting if - // enabled. - port = 8502 - c.UI.Info(fmt.Sprintf("Defaulting to grpc port = %d", port)) - } - c.grpcAddr = fmt.Sprintf("localhost:%v", port) - } - - // Decide on TLS if the scheme is provided and indicates it, if the HTTP env - // suggests TLS is supported explicitly (CONSUL_HTTP_SSL) or implicitly - // (CONSUL_HTTP_ADDR) is https:// - useTLS := false - if strings.HasPrefix(strings.ToLower(c.grpcAddr), "https://") { - useTLS = true - } else if useSSLEnv := os.Getenv(api.HTTPSSLEnvName); useSSLEnv != "" { - if enabled, err := strconv.ParseBool(useSSLEnv); err == nil { - useTLS = enabled - } - } else if strings.HasPrefix(strings.ToLower(httpCfg.Address), "https://") { - useTLS = true - } - - // We want to allow grpcAddr set as host:port with no scheme but if the host - // is an IP this will fail to parse as a URL with "parse 127.0.0.1:8500: first - // path segment in URL cannot contain colon". On the other hand we also - // support both http(s)://host:port and unix:///path/to/file. - var agentAddr, agentPort, agentSock string - if grpcAddr := strings.TrimPrefix(c.grpcAddr, "unix://"); grpcAddr != c.grpcAddr { - // Path to unix socket - agentSock = grpcAddr - } else { - // Parse as host:port with option http prefix - grpcAddr = strings.TrimPrefix(c.grpcAddr, "http://") - grpcAddr = strings.TrimPrefix(c.grpcAddr, "https://") - - var err error - agentAddr, agentPort, err = net.SplitHostPort(grpcAddr) - if err != nil { - return nil, fmt.Errorf("Invalid Consul HTTP address: %s", err) - } - if agentAddr == "" { - agentAddr = "127.0.0.1" - } - - // We use STATIC for agent which means we need to resolve DNS names like - // `localhost` ourselves. We could use STRICT_DNS or LOGICAL_DNS with envoy - // but Envoy resolves `localhost` differently to go on macOS at least which - // causes paper cuts like default dev agent (which binds specifically to - // 127.0.0.1) isn't reachable since Envoy resolves localhost to `[::]` and - // can't connect. - agentIP, err := net.ResolveIPAddr("ip", agentAddr) - if err != nil { - return nil, fmt.Errorf("Failed to resolve agent address: %s", err) - } - agentAddr = agentIP.String() + grpcAddr, err := c.grpcAddress(httpCfg) + if err != nil { + return nil, err } adminAddr, adminPort, err := net.SplitHostPort(c.adminBind) @@ -499,12 +440,9 @@ func (c *cmd) templateArgs() (*BootstrapTplArgs, error) { } return &BootstrapTplArgs{ + GRPC: grpcAddr, ProxyCluster: cluster, ProxyID: c.proxyID, - AgentAddress: agentAddr, - AgentPort: agentPort, - AgentSocket: agentSock, - AgentTLS: useTLS, AgentCAPEM: caPEM, AdminAccessLogPath: adminAccessLogPath, AdminBindAddress: adminBindIP.String(), @@ -549,6 +487,76 @@ func (c *cmd) generateConfig() ([]byte, error) { return bsCfg.GenerateJSON(args) } +// TODO: make method a function +func (c *cmd) grpcAddress(httpCfg *api.Config) (GRPC, error) { + g := GRPC{} + + addr := c.grpcAddr + // See if we need to lookup grpcAddr + if addr == "" { + port, err := c.lookupGRPCPort() + if err != nil { + c.UI.Error(fmt.Sprintf("Error connecting to Consul agent: %s", err)) + } + if port <= 0 { + // This is the dev mode default and recommended production setting if + // enabled. + port = 8502 + c.UI.Info(fmt.Sprintf("Defaulting to grpc port = %d", port)) + } + addr = fmt.Sprintf("localhost:%v", port) + } + + // Decide on TLS if the scheme is provided and indicates it, if the HTTP env + // suggests TLS is supported explicitly (CONSUL_HTTP_SSL) or implicitly + // (CONSUL_HTTP_ADDR) is https:// + if strings.HasPrefix(strings.ToLower(addr), "https://") { + g.AgentTLS = true + } else if useSSLEnv := os.Getenv(api.HTTPSSLEnvName); useSSLEnv != "" { + if enabled, err := strconv.ParseBool(useSSLEnv); err == nil { + g.AgentTLS = enabled + } + } else if strings.HasPrefix(strings.ToLower(httpCfg.Address), "https://") { + g.AgentTLS = true + } + + // We want to allow grpcAddr set as host:port with no scheme but if the host + // is an IP this will fail to parse as a URL with "parse 127.0.0.1:8500: first + // path segment in URL cannot contain colon". On the other hand we also + // support both http(s)://host:port and unix:///path/to/file. + if grpcAddr := strings.TrimPrefix(addr, "unix://"); grpcAddr != addr { + // Path to unix socket + g.AgentSocket = grpcAddr + } else { + // Parse as host:port with option http prefix + grpcAddr = strings.TrimPrefix(addr, "http://") + grpcAddr = strings.TrimPrefix(addr, "https://") + + var err error + g.AgentAddress, g.AgentPort, err = net.SplitHostPort(grpcAddr) + if err != nil { + return g, fmt.Errorf("Invalid Consul HTTP address: %s", err) + } + // TODO: isn't this case impossible because we have already set a default value + if g.AgentAddress == "" { + g.AgentAddress = "127.0.0.1" + } + + // We use STATIC for agent which means we need to resolve DNS names like + // `localhost` ourselves. We could use STRICT_DNS or LOGICAL_DNS with envoy + // but Envoy resolves `localhost` differently to go on macOS at least which + // causes paper cuts like default dev agent (which binds specifically to + // 127.0.0.1) isn't reachable since Envoy resolves localhost to `[::]` and + // can't connect. + agentIP, err := net.ResolveIPAddr("ip", g.AgentAddress) + if err != nil { + return g, fmt.Errorf("Failed to resolve agent address: %s", err) + } + g.AgentAddress = agentIP.String() + } + return g, nil +} + func (c *cmd) lookupGRPCPort() (int, error) { self, err := c.client.Agent().Self() if err != nil { diff --git a/command/connect/envoy/envoy_test.go b/command/connect/envoy/envoy_test.go index d5f0e5d159..32fe3dd7e3 100644 --- a/command/connect/envoy/envoy_test.go +++ b/command/connect/envoy/envoy_test.go @@ -125,11 +125,13 @@ func TestGenerateConfig(t *testing.T) { Name: "defaults", Flags: []string{"-proxy-id", "test-proxy"}, WantArgs: BootstrapTplArgs{ - EnvoyVersion: defaultEnvoyVersion, - ProxyCluster: "test-proxy", - ProxyID: "test-proxy", - AgentAddress: "127.0.0.1", - AgentPort: "8502", // Note this is the gRPC port + EnvoyVersion: defaultEnvoyVersion, + ProxyCluster: "test-proxy", + ProxyID: "test-proxy", + GRPC: GRPC{ + AgentAddress: "127.0.0.1", + AgentPort: "8502", // Note this is the gRPC port + }, AdminAccessLogPath: "/dev/null", AdminBindAddress: "127.0.0.1", AdminBindPort: "19000", @@ -141,11 +143,13 @@ func TestGenerateConfig(t *testing.T) { Flags: []string{"-proxy-id", "test-proxy", "-token", "c9a52720-bf6c-4aa6-b8bc-66881a5ade95"}, WantArgs: BootstrapTplArgs{ - EnvoyVersion: defaultEnvoyVersion, - ProxyCluster: "test-proxy", - ProxyID: "test-proxy", - AgentAddress: "127.0.0.1", - AgentPort: "8502", // Note this is the gRPC port + EnvoyVersion: defaultEnvoyVersion, + ProxyCluster: "test-proxy", + ProxyID: "test-proxy", + GRPC: GRPC{ + AgentAddress: "127.0.0.1", + AgentPort: "8502", // Note this is the gRPC port + }, AdminAccessLogPath: "/dev/null", AdminBindAddress: "127.0.0.1", AdminBindPort: "19000", @@ -160,11 +164,13 @@ func TestGenerateConfig(t *testing.T) { "CONSUL_HTTP_TOKEN=c9a52720-bf6c-4aa6-b8bc-66881a5ade95", }, WantArgs: BootstrapTplArgs{ - EnvoyVersion: defaultEnvoyVersion, - ProxyCluster: "test-proxy", - ProxyID: "test-proxy", - AgentAddress: "127.0.0.1", - AgentPort: "8502", // Note this is the gRPC port + EnvoyVersion: defaultEnvoyVersion, + ProxyCluster: "test-proxy", + ProxyID: "test-proxy", + GRPC: GRPC{ + AgentAddress: "127.0.0.1", + AgentPort: "8502", // Note this is the gRPC port + }, AdminAccessLogPath: "/dev/null", AdminBindAddress: "127.0.0.1", AdminBindPort: "19000", @@ -181,11 +187,13 @@ func TestGenerateConfig(t *testing.T) { "token.txt": "c9a52720-bf6c-4aa6-b8bc-66881a5ade95", }, WantArgs: BootstrapTplArgs{ - EnvoyVersion: defaultEnvoyVersion, - ProxyCluster: "test-proxy", - ProxyID: "test-proxy", - AgentAddress: "127.0.0.1", - AgentPort: "8502", // Note this is the gRPC port + EnvoyVersion: defaultEnvoyVersion, + ProxyCluster: "test-proxy", + ProxyID: "test-proxy", + GRPC: GRPC{ + AgentAddress: "127.0.0.1", + AgentPort: "8502", // Note this is the gRPC port + }, AdminAccessLogPath: "/dev/null", AdminBindAddress: "127.0.0.1", AdminBindPort: "19000", @@ -203,11 +211,13 @@ func TestGenerateConfig(t *testing.T) { "token.txt": "c9a52720-bf6c-4aa6-b8bc-66881a5ade95", }, WantArgs: BootstrapTplArgs{ - EnvoyVersion: defaultEnvoyVersion, - ProxyCluster: "test-proxy", - ProxyID: "test-proxy", - AgentAddress: "127.0.0.1", - AgentPort: "8502", // Note this is the gRPC port + EnvoyVersion: defaultEnvoyVersion, + ProxyCluster: "test-proxy", + ProxyID: "test-proxy", + GRPC: GRPC{ + AgentAddress: "127.0.0.1", + AgentPort: "8502", // Note this is the gRPC port + }, AdminAccessLogPath: "/dev/null", AdminBindAddress: "127.0.0.1", AdminBindPort: "19000", @@ -226,8 +236,10 @@ func TestGenerateConfig(t *testing.T) { // Should resolve IP, note this might not resolve the same way // everywhere which might make this test brittle but not sure what else // to do. - AgentAddress: "127.0.0.1", - AgentPort: "9999", + GRPC: GRPC{ + AgentAddress: "127.0.0.1", + AgentPort: "9999", + }, AdminAccessLogPath: "/dev/null", AdminBindAddress: "127.0.0.1", AdminBindPort: "19000", @@ -247,8 +259,10 @@ func TestGenerateConfig(t *testing.T) { // Should resolve IP, note this might not resolve the same way // everywhere which might make this test brittle but not sure what else // to do. - AgentAddress: "127.0.0.1", - AgentPort: "9999", + GRPC: GRPC{ + AgentAddress: "127.0.0.1", + AgentPort: "9999", + }, AdminAccessLogPath: "/dev/null", AdminBindAddress: "127.0.0.1", AdminBindPort: "19000", @@ -260,10 +274,12 @@ func TestGenerateConfig(t *testing.T) { Flags: []string{"-proxy-id", "test-proxy", "-grpc-addr", "unix:///var/run/consul.sock"}, WantArgs: BootstrapTplArgs{ - EnvoyVersion: defaultEnvoyVersion, - ProxyCluster: "test-proxy", - ProxyID: "test-proxy", - AgentSocket: "/var/run/consul.sock", + EnvoyVersion: defaultEnvoyVersion, + ProxyCluster: "test-proxy", + ProxyID: "test-proxy", + GRPC: GRPC{ + AgentSocket: "/var/run/consul.sock", + }, AdminAccessLogPath: "/dev/null", AdminBindAddress: "127.0.0.1", AdminBindPort: "19000", @@ -281,8 +297,10 @@ func TestGenerateConfig(t *testing.T) { // Should resolve IP, note this might not resolve the same way // everywhere which might make this test brittle but not sure what else // to do. - AgentAddress: "127.0.0.1", - AgentPort: "9999", + GRPC: GRPC{ + AgentAddress: "127.0.0.1", + AgentPort: "9999", + }, AdminAccessLogPath: "/dev/null", AdminBindAddress: "127.0.0.1", AdminBindPort: "19000", @@ -299,8 +317,10 @@ func TestGenerateConfig(t *testing.T) { // Should resolve IP, note this might not resolve the same way // everywhere which might make this test brittle but not sure what else // to do. - AgentAddress: "127.0.0.1", - AgentPort: "8502", + GRPC: GRPC{ + AgentAddress: "127.0.0.1", + AgentPort: "8502", + }, AdminAccessLogPath: "/some/path/access.log", AdminBindAddress: "127.0.0.1", AdminBindPort: "19000", @@ -317,8 +337,10 @@ func TestGenerateConfig(t *testing.T) { // Should resolve IP, note this might not resolve the same way // everywhere which might make this test brittle but not sure what else // to do. - AgentAddress: "127.0.0.1", - AgentPort: "8502", + GRPC: GRPC{ + AgentAddress: "127.0.0.1", + AgentPort: "8502", + }, }, WantErr: "Error loading CA File: open some/path: no such file or directory", }, @@ -333,9 +355,11 @@ func TestGenerateConfig(t *testing.T) { // Should resolve IP, note this might not resolve the same way // everywhere which might make this test brittle but not sure what else // to do. - AgentAddress: "127.0.0.1", - AgentPort: "8502", - AgentTLS: true, + GRPC: GRPC{ + AgentAddress: "127.0.0.1", + AgentPort: "8502", + AgentTLS: true, + }, AgentCAPEM: `-----BEGIN CERTIFICATE-----\nMIIEtzCCA5+gAwIBAgIJAIewRMI8OnvTMA0GCSqGSIb3DQEBBQUAMIGYMQswCQYD\nVQQGEwJVUzELMAkGA1UECBMCQ0ExFjAUBgNVBAcTDVNhbiBGcmFuY2lzY28xHDAa\nBgNVBAoTE0hhc2hpQ29ycCBUZXN0IENlcnQxDDAKBgNVBAsTA0RldjEWMBQGA1UE\nAxMNdGVzdC5pbnRlcm5hbDEgMB4GCSqGSIb3DQEJARYRdGVzdEBpbnRlcm5hbC5j\nb20wHhcNMTQwNDA3MTkwMTA4WhcNMjQwNDA0MTkwMTA4WjCBmDELMAkGA1UEBhMC\nVVMxCzAJBgNVBAgTAkNBMRYwFAYDVQQHEw1TYW4gRnJhbmNpc2NvMRwwGgYDVQQK\nExNIYXNoaUNvcnAgVGVzdCBDZXJ0MQwwCgYDVQQLEwNEZXYxFjAUBgNVBAMTDXRl\nc3QuaW50ZXJuYWwxIDAeBgkqhkiG9w0BCQEWEXRlc3RAaW50ZXJuYWwuY29tMIIB\nIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAxrs6JK4NpiOItxrpNR/1ppUU\nmH7p2BgLCBZ6eHdclle9J56i68adt8J85zaqphCfz6VDP58DsFx+N50PZyjQaDsU\nd0HejRqfHRMtg2O+UQkv4Z66+Vo+gc6uGuANi2xMtSYDVTAqqzF48OOPQDgYkzcG\nxcFZzTRFFZt2vPnyHj8cHcaFo/NMNVh7C3yTXevRGNm9u2mrbxCEeiHzFC2WUnvg\nU2jQuC7Fhnl33Zd3B6d3mQH6O23ncmwxTcPUJe6xZaIRrDuzwUcyhLj5Z3faag/f\npFIIcHSiHRfoqHLGsGg+3swId/zVJSSDHr7pJUu7Cre+vZa63FqDaooqvnisrQID\nAQABo4IBADCB/TAdBgNVHQ4EFgQUo/nrOfqvbee2VklVKIFlyQEbuJUwgc0GA1Ud\nIwSBxTCBwoAUo/nrOfqvbee2VklVKIFlyQEbuJWhgZ6kgZswgZgxCzAJBgNVBAYT\nAlVTMQswCQYDVQQIEwJDQTEWMBQGA1UEBxMNU2FuIEZyYW5jaXNjbzEcMBoGA1UE\nChMTSGFzaGlDb3JwIFRlc3QgQ2VydDEMMAoGA1UECxMDRGV2MRYwFAYDVQQDEw10\nZXN0LmludGVybmFsMSAwHgYJKoZIhvcNAQkBFhF0ZXN0QGludGVybmFsLmNvbYIJ\nAIewRMI8OnvTMAwGA1UdEwQFMAMBAf8wDQYJKoZIhvcNAQEFBQADggEBADa9fV9h\ngjapBlkNmu64WX0Ufub5dsJrdHS8672P30S7ILB7Mk0W8sL65IezRsZnG898yHf9\n2uzmz5OvNTM9K380g7xFlyobSVq+6yqmmSAlA/ptAcIIZT727P5jig/DB7fzJM3g\njctDlEGOmEe50GQXc25VKpcpjAsNQi5ER5gowQ0v3IXNZs+yU+LvxLHc0rUJ/XSp\nlFCAMOqd5uRoMOejnT51G6krvLNzPaQ3N9jQfNVY4Q0zfs0M+6dRWvqfqB9Vyq8/\nPOLMld+HyAZEBk9zK3ZVIXx6XS4dkDnSNR91njLq7eouf6M7+7s/oMQZZRtAfQ6r\nwlW975rYa1ZqEdA=\n-----END CERTIFICATE-----\n`, AdminAccessLogPath: "/dev/null", AdminBindAddress: "127.0.0.1", @@ -369,11 +393,13 @@ func TestGenerateConfig(t *testing.T) { }`, }, WantArgs: BootstrapTplArgs{ - EnvoyVersion: defaultEnvoyVersion, - ProxyCluster: "test-proxy", - ProxyID: "test-proxy", - AgentAddress: "127.0.0.1", - AgentPort: "8502", + EnvoyVersion: defaultEnvoyVersion, + ProxyCluster: "test-proxy", + ProxyID: "test-proxy", + GRPC: GRPC{ + AgentAddress: "127.0.0.1", + AgentPort: "8502", + }, AdminAccessLogPath: "/dev/null", AdminBindAddress: "127.0.0.1", AdminBindPort: "19000", @@ -401,11 +427,13 @@ func TestGenerateConfig(t *testing.T) { }`, }, WantArgs: BootstrapTplArgs{ - EnvoyVersion: defaultEnvoyVersion, - ProxyCluster: "test-proxy", - ProxyID: "test-proxy", - AgentAddress: "127.0.0.1", - AgentPort: "8502", + EnvoyVersion: defaultEnvoyVersion, + ProxyCluster: "test-proxy", + ProxyID: "test-proxy", + GRPC: GRPC{ + AgentAddress: "127.0.0.1", + AgentPort: "8502", + }, AdminAccessLogPath: "/dev/null", AdminBindAddress: "127.0.0.1", AdminBindPort: "19000", @@ -438,11 +466,13 @@ func TestGenerateConfig(t *testing.T) { } , { "name": "fake_sink_2" }`, }, WantArgs: BootstrapTplArgs{ - EnvoyVersion: defaultEnvoyVersion, - ProxyCluster: "test-proxy", - ProxyID: "test-proxy", - AgentAddress: "127.0.0.1", - AgentPort: "8502", + EnvoyVersion: defaultEnvoyVersion, + ProxyCluster: "test-proxy", + ProxyID: "test-proxy", + GRPC: GRPC{ + AgentAddress: "127.0.0.1", + AgentPort: "8502", + }, AdminAccessLogPath: "/dev/null", AdminBindAddress: "127.0.0.1", AdminBindPort: "19000", @@ -462,11 +492,13 @@ func TestGenerateConfig(t *testing.T) { }`, }, WantArgs: BootstrapTplArgs{ - EnvoyVersion: defaultEnvoyVersion, - ProxyCluster: "test-proxy", - ProxyID: "test-proxy", - AgentAddress: "127.0.0.1", - AgentPort: "8502", + EnvoyVersion: defaultEnvoyVersion, + ProxyCluster: "test-proxy", + ProxyID: "test-proxy", + GRPC: GRPC{ + AgentAddress: "127.0.0.1", + AgentPort: "8502", + }, AdminAccessLogPath: "/dev/null", AdminBindAddress: "127.0.0.1", AdminBindPort: "19000", @@ -516,11 +548,13 @@ func TestGenerateConfig(t *testing.T) { }`, }, WantArgs: BootstrapTplArgs{ - EnvoyVersion: defaultEnvoyVersion, - ProxyCluster: "test-proxy", - ProxyID: "test-proxy", - AgentAddress: "127.0.0.1", - AgentPort: "8502", + EnvoyVersion: defaultEnvoyVersion, + ProxyCluster: "test-proxy", + ProxyID: "test-proxy", + GRPC: GRPC{ + AgentAddress: "127.0.0.1", + AgentPort: "8502", + }, AdminAccessLogPath: "/dev/null", AdminBindAddress: "127.0.0.1", AdminBindPort: "19000", From 0888c6575b0ba6408663294d951c2173141b6c9e Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 7 Apr 2020 18:02:56 -0400 Subject: [PATCH 3/4] Step 3: fix a bug in api.NewClient and fix the tests The api client should never rever to HTTP if the user explicitly requested TLS. This change broke some tests because the tests always use an non-TLS http server, but some tests explicitly enable TLS. --- api/api.go | 6 +++--- command/connect/envoy/envoy.go | 13 ++++++++----- command/connect/envoy/envoy_test.go | 10 +++++++--- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/api/api.go b/api/api.go index a42a110bce..7b00be967a 100644 --- a/api/api.go +++ b/api/api.go @@ -551,11 +551,11 @@ func NewClient(config *Config) (*Client, error) { // bootstrap the config defConfig := DefaultConfig() - if len(config.Address) == 0 { + if config.Address == "" { config.Address = defConfig.Address } - if len(config.Scheme) == 0 { + if config.Scheme == "" { config.Scheme = defConfig.Scheme } @@ -599,7 +599,7 @@ func NewClient(config *Config) (*Client, error) { if len(parts) == 2 { switch parts[0] { case "http": - config.Scheme = "http" + // Never revert to http if TLS was explicitly requested. case "https": config.Scheme = "https" case "unix": diff --git a/command/connect/envoy/envoy.go b/command/connect/envoy/envoy.go index 13824d39b5..fd5177638d 100644 --- a/command/connect/envoy/envoy.go +++ b/command/connect/envoy/envoy.go @@ -199,16 +199,19 @@ func (c *cmd) Run(args []string) int { if err := c.flags.Parse(args); err != nil { return 1 } - passThroughArgs := c.flags.Args() // Setup Consul client - client, err := c.http.APIClient() + var err error + c.client, err = c.http.APIClient() if err != nil { c.UI.Error(fmt.Sprintf("Error connecting to Consul agent: %s", err)) return 1 } - c.client = client + // TODO: refactor + return c.run(c.flags.Args()) +} +func (c *cmd) run(args []string) int { // Fixup for deprecated mesh-gateway flag if c.meshGateway && c.gateway != "" { c.UI.Error("The mesh-gateway flag is deprecated and cannot be used alongside the gateway flag") @@ -311,7 +314,7 @@ func (c *cmd) Run(args []string) int { }, } - if err := client.Agent().ServiceRegister(&svc); err != nil { + if err := c.client.Agent().ServiceRegister(&svc); err != nil { c.UI.Error(fmt.Sprintf("Error registering service %q: %s", svc.Name, err)) return 1 } @@ -363,7 +366,7 @@ func (c *cmd) Run(args []string) int { return 1 } - err = execEnvoy(binary, nil, passThroughArgs, bootstrapJson) + err = execEnvoy(binary, nil, args, bootstrapJson) if err == errUnsupportedOS { c.UI.Error("Directly running Envoy is only supported on linux and macOS " + "since envoy itself doesn't build on other platforms currently.") diff --git a/command/connect/envoy/envoy_test.go b/command/connect/envoy/envoy_test.go index 32fe3dd7e3..2b45f8a3d3 100644 --- a/command/connect/envoy/envoy_test.go +++ b/command/connect/envoy/envoy_test.go @@ -589,20 +589,24 @@ func TestGenerateConfig(t *testing.T) { // test. srv := httptest.NewServer(testMockAgent(tc.ProxyConfig, tc.GRPCPort)) defer srv.Close() + client, err := api.NewClient(&api.Config{Address: srv.URL}) + require.NoError(err) - // 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) myEnv := copyAndReplaceAll(tc.Env, "@@TEMPDIR@@", testDirPrefix) defer testSetAndResetEnv(t, myEnv)() ui := cli.NewMockUi() c := New(ui) + // explicitly set the client to one which can connect to the httptest.Server + c.client = client // Run the command myFlags := copyAndReplaceAll(tc.Flags, "@@TEMPDIR@@", testDirPrefix) args := append([]string{"-bootstrap"}, myFlags...) - code := c.Run(args) + + require.NoError(c.flags.Parse(args)) + code := c.run(c.flags.Args()) if tc.WantErr == "" { require.Equal(0, code, ui.ErrorWriter.String()) } else { From 8b6861518fd3ef3a17f4db27d4b74265e0a032d8 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 7 Apr 2020 18:16:34 -0400 Subject: [PATCH 4/4] Fix CONSUL_HTTP_ADDR=https not enabling TLS Use the config instead of attempting to reparse the env var. --- command/connect/envoy/envoy.go | 23 ++-- command/connect/envoy/envoy_test.go | 22 +++ ..._ADDR-with-https-scheme-enables-tls.golden | 125 ++++++++++++++++++ 3 files changed, 156 insertions(+), 14 deletions(-) create mode 100644 command/connect/envoy/testdata/CONSUL_HTTP_ADDR-with-https-scheme-enables-tls.golden diff --git a/command/connect/envoy/envoy.go b/command/connect/envoy/envoy.go index fd5177638d..123c82f4ce 100644 --- a/command/connect/envoy/envoy.go +++ b/command/connect/envoy/envoy.go @@ -8,7 +8,6 @@ import ( "net" "os" "os/exec" - "strconv" "strings" "github.com/mitchellh/cli" @@ -394,7 +393,7 @@ func (c *cmd) templateArgs() (*BootstrapTplArgs, error) { httpCfg := api.DefaultConfig() c.http.MergeOntoConfig(httpCfg) - // Trigger the Client init to do any last-minute updates to the Config. + // api.NewClient normalizes some values (Token, Scheme) on the Config. if _, err := api.NewClient(httpCfg); err != nil { return nil, err } @@ -510,16 +509,15 @@ func (c *cmd) grpcAddress(httpCfg *api.Config) (GRPC, error) { addr = fmt.Sprintf("localhost:%v", port) } + // TODO: parse addr as a url instead of strings.HasPrefix/TrimPrefix + // Decide on TLS if the scheme is provided and indicates it, if the HTTP env // suggests TLS is supported explicitly (CONSUL_HTTP_SSL) or implicitly // (CONSUL_HTTP_ADDR) is https:// - if strings.HasPrefix(strings.ToLower(addr), "https://") { + switch { + case strings.HasPrefix(strings.ToLower(addr), "https://"): g.AgentTLS = true - } else if useSSLEnv := os.Getenv(api.HTTPSSLEnvName); useSSLEnv != "" { - if enabled, err := strconv.ParseBool(useSSLEnv); err == nil { - g.AgentTLS = enabled - } - } else if strings.HasPrefix(strings.ToLower(httpCfg.Address), "https://") { + case httpCfg.Scheme == "https": g.AgentTLS = true } @@ -536,14 +534,11 @@ func (c *cmd) grpcAddress(httpCfg *api.Config) (GRPC, error) { grpcAddr = strings.TrimPrefix(addr, "https://") var err error - g.AgentAddress, g.AgentPort, err = net.SplitHostPort(grpcAddr) + var host string + host, g.AgentPort, err = net.SplitHostPort(grpcAddr) if err != nil { return g, fmt.Errorf("Invalid Consul HTTP address: %s", err) } - // TODO: isn't this case impossible because we have already set a default value - if g.AgentAddress == "" { - g.AgentAddress = "127.0.0.1" - } // We use STATIC for agent which means we need to resolve DNS names like // `localhost` ourselves. We could use STRICT_DNS or LOGICAL_DNS with envoy @@ -551,7 +546,7 @@ func (c *cmd) grpcAddress(httpCfg *api.Config) (GRPC, error) { // causes paper cuts like default dev agent (which binds specifically to // 127.0.0.1) isn't reachable since Envoy resolves localhost to `[::]` and // can't connect. - agentIP, err := net.ResolveIPAddr("ip", g.AgentAddress) + agentIP, err := net.ResolveIPAddr("ip", host) if err != nil { return g, fmt.Errorf("Failed to resolve agent address: %s", err) } diff --git a/command/connect/envoy/envoy_test.go b/command/connect/envoy/envoy_test.go index 2b45f8a3d3..e92fad3408 100644 --- a/command/connect/envoy/envoy_test.go +++ b/command/connect/envoy/envoy_test.go @@ -561,6 +561,28 @@ func TestGenerateConfig(t *testing.T) { LocalAgentClusterName: xds.LocalAgentClusterName, }, }, + { + Name: "CONSUL_HTTP_ADDR-with-https-scheme-enables-tls", + Flags: []string{"-proxy-id", "test-proxy"}, + Env: []string{"CONSUL_HTTP_ADDR=https://127.0.0.1:8888"}, + WantArgs: BootstrapTplArgs{ + EnvoyVersion: defaultEnvoyVersion, + ProxyCluster: "test-proxy", + ProxyID: "test-proxy", + // Should resolve IP, note this might not resolve the same way + // everywhere which might make this test brittle but not sure what else + // to do. + GRPC: GRPC{ + AgentAddress: "127.0.0.1", + AgentPort: "8502", + AgentTLS: true, + }, + AdminAccessLogPath: "/dev/null", + AdminBindAddress: "127.0.0.1", + AdminBindPort: "19000", + LocalAgentClusterName: xds.LocalAgentClusterName, + }, + }, } copyAndReplaceAll := func(s []string, old, new string) []string { diff --git a/command/connect/envoy/testdata/CONSUL_HTTP_ADDR-with-https-scheme-enables-tls.golden b/command/connect/envoy/testdata/CONSUL_HTTP_ADDR-with-https-scheme-enables-tls.golden new file mode 100644 index 0000000000..0488995626 --- /dev/null +++ b/command/connect/envoy/testdata/CONSUL_HTTP_ADDR-with-https-scheme-enables-tls.golden @@ -0,0 +1,125 @@ +{ + "admin": { + "access_log_path": "/dev/null", + "address": { + "socket_address": { + "address": "127.0.0.1", + "port_value": 19000 + } + } + }, + "node": { + "cluster": "test-proxy", + "id": "test-proxy", + "metadata": { + "namespace": "default", + "envoy_version": "1.13.1" + } + }, + "static_resources": { + "clusters": [ + { + "name": "local_agent", + "connect_timeout": "1s", + "type": "STATIC", + "tls_context": { + "common_tls_context": { + "validation_context": { + "trusted_ca": { + "inline_string": "" + } + } + } + }, + "http2_protocol_options": {}, + "hosts": [ + { + "socket_address": { + "address": "127.0.0.1", + "port_value": 8502 + } + } + ] + } + ] + }, + "stats_config": { + "stats_tags": [ + { + "regex": "^cluster\\.((?:([^.]+)~)?(?:[^.]+\\.)?[^.]+\\.[^.]+\\.[^.]+\\.[^.]+\\.[^.]+\\.consul\\.)", + "tag_name": "consul.custom_hash" + }, + { + "regex": "^cluster\\.((?:[^.]+~)?(?:([^.]+)\\.)?[^.]+\\.[^.]+\\.[^.]+\\.[^.]+\\.[^.]+\\.consul\\.)", + "tag_name": "consul.service_subset" + }, + { + "regex": "^cluster\\.((?:[^.]+~)?(?:[^.]+\\.)?([^.]+)\\.[^.]+\\.[^.]+\\.[^.]+\\.[^.]+\\.consul\\.)", + "tag_name": "consul.service" + }, + { + "regex": "^cluster\\.((?:[^.]+~)?(?:[^.]+\\.)?[^.]+\\.([^.]+)\\.[^.]+\\.[^.]+\\.[^.]+\\.consul\\.)", + "tag_name": "consul.namespace" + }, + { + "regex": "^cluster\\.((?:[^.]+~)?(?:[^.]+\\.)?[^.]+\\.[^.]+\\.([^.]+)\\.[^.]+\\.[^.]+\\.consul\\.)", + "tag_name": "consul.datacenter" + }, + { + "regex": "^cluster\\.((?:[^.]+~)?(?:[^.]+\\.)?[^.]+\\.[^.]+\\.[^.]+\\.([^.]+)\\.[^.]+\\.consul\\.)", + "tag_name": "consul.routing_type" + }, + { + "regex": "^cluster\\.((?:[^.]+~)?(?:[^.]+\\.)?[^.]+\\.[^.]+\\.[^.]+\\.[^.]+\\.([^.]+)\\.consul\\.)", + "tag_name": "consul.trust_domain" + }, + { + "regex": "^cluster\\.(((?:[^.]+~)?(?:[^.]+\\.)?[^.]+\\.[^.]+\\.[^.]+)\\.[^.]+\\.[^.]+\\.consul\\.)", + "tag_name": "consul.target" + }, + { + "regex": "^cluster\\.(((?:[^.]+~)?(?:[^.]+\\.)?[^.]+\\.[^.]+\\.[^.]+\\.[^.]+\\.[^.]+)\\.consul\\.)", + "tag_name": "consul.full_target" + }, + { + "tag_name": "local_cluster", + "fixed_value": "test-proxy" + } + ], + "use_all_default_tags": true + }, + "dynamic_resources": { + "lds_config": { + "ads": {} + }, + "cds_config": { + "ads": {} + }, + "ads_config": { + "api_type": "GRPC", + "grpc_services": { + "initial_metadata": [ + { + "key": "x-consul-token", + "value": "" + } + ], + "envoy_grpc": { + "cluster_name": "local_agent" + } + } + } + }, + "layered_runtime": { + "layers": [ + { + "name": "static_layer", + "static_layer": { + "envoy.deprecated_features:envoy.api.v2.Cluster.tls_context": true, + "envoy.deprecated_features:envoy.config.trace.v2.ZipkinConfig.HTTP_JSON_V1": true, + "envoy.deprecated_features:envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager.Tracing.operation_name": true + } + } + ] + } +}