IPv6: Updating parsing of host/port for bootstrap options. (istio#4525)

Automatic merge from submit-queue.

IPv6: Updating parsing of host/port for bootstrap options.

Allow IPv6 addresses (with square brackets) to be specified in
addresses that are parsed into host/port for various bootstrap
file arguments.

Added unit tests to exercise the logic, which has been placed into
functions for use by multiple options. Tests cover failure cases,
which were not previously checked. Rely on the existing golden
master to test the happy path for all entries. Didn't feel we needed
to add a golden master for IPv6 addressess, as covered by the UTs.

/area networking
Fixes istio#4097 issue

pkg/bootstrap/bootstrap_config.go

@@ -18,10 +18,10 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	"net"
 	"os"
 	"os/exec"
 	"path"
-	"strings"
 	"text/template"
 	"time"
 
@@ -112,6 +112,22 @@ func RunProxy(config *meshconfig.ProxyConfig, node string, epoch int, configFnam
 	return cmd.Process, nil
 }
 
+// GetHostPort separates out the host and port portions of an address. The
+// host portion may be a name, IPv4 address, or IPv6 address (with square brackets).
+func GetHostPort(name, addr string) (host string, port string, err error) {
+	host, port, err = net.SplitHostPort(addr)
+	if err != nil {
+		return "", "", fmt.Errorf("unable to parse %s address %q: %v", name, addr, err)
+	}
+	return host, port, nil
+}
+
+// StoreHostPort encodes the host and port as key/value pair strings in
+// the provided map.
+func StoreHostPort(host, port, field string, opts map[string]interface{}) {
+	opts[field] = fmt.Sprintf("{\"address\": \"%s\", \"port_value\": %s}", host, port)
+}
+
 // WriteBootstrap generates an envoy config based on config and epoch, and returns the filename.
 // TODO: in v2 some of the LDS ports (port, http_port) should be configured in the bootstrap.
 func WriteBootstrap(config *meshconfig.ProxyConfig, epoch int, pilotSAN []string, opts map[string]interface{}) (string, error) {
@@ -153,37 +169,47 @@ func WriteBootstrap(config *meshconfig.ProxyConfig, epoch int, pilotSAN []string
 	opts["refresh_delay"] = fmt.Sprintf("{\"seconds\": %d, \"nanos\": %d}", config.DiscoveryRefreshDelay.Seconds, config.DiscoveryRefreshDelay.Nanos)
 	opts["connect_timeout"] = fmt.Sprintf("{\"seconds\": %d, \"nanos\": %d}", config.ConnectTimeout.Seconds, config.ConnectTimeout.Nanos)
 
-	addPort := strings.Split(config.DiscoveryAddress, ":")
-	pilotHost := addPort[0]
-	pilotPort := addPort[1]
-	opts["pilot_address"] = fmt.Sprintf("{\"address\": \"%s\", \"port_value\": %s}", pilotHost, pilotPort)
+	// Failsafe for EDSv2. In case of bugs of problems, the injection template can be modified to
+	// add this env variable. This is short lived, EDSv1 will be deprecated/removed.
+	if os.Getenv("USE_EDS_V1") == "1" {
+		opts["edsv1"] = "1"
+	}
+
+	h, p, err := GetHostPort("Discovery", config.DiscoveryAddress)
+	if err != nil {
+		return "", err
+	}
+	StoreHostPort(h, p, "pilot_address", opts)
 
 	grpcAddress := opts["pilot_grpc"]
 	// Default values for the grpc address.
 	grpcPort := "15010"
-	grpcHost := pilotHost
+	grpcHost := h // Use pilot host
 	// Default value
 	if grpcAddress != nil {
-		addPort = strings.Split(grpcAddress.(string), ":")
-		grpcHost = addPort[0]
-		grpcPort = addPort[1]
-	}
-	opts["pilot_grpc_address"] = fmt.Sprintf("{\"address\": \"%s\", \"port_value\": %s}", grpcHost, grpcPort)
-
-	// Failsafe for EDSv2. In case of bugs of problems, the injection template can be modified to
-	// add this env variable. This is short lived, EDSv1 will be deprecated/removed.
-	if os.Getenv("USE_EDS_V1") == "1" {
-		opts["edsv1"] = "1"
+		grpcHost, grpcPort, err = GetHostPort("gRPC", grpcAddress.(string))
+		if err != nil {
+			return "", err
+		}
 	}
+	StoreHostPort(grpcHost, grpcPort, "pilot_grpc_address", opts)
 
 	if config.ZipkinAddress != "" {
-		addPort = strings.Split(config.ZipkinAddress, ":")
-		opts["zipkin"] = fmt.Sprintf("{\"address\": \"%s\", \"port_value\": %s}", addPort[0], addPort[1])
+		h, p, err = GetHostPort("Zipkin", config.ZipkinAddress)
+		if err != nil {
+			return "", err
+		}
+		StoreHostPort(h, p, "zipkin", opts)
 	}
+
 	if config.StatsdUdpAddress != "" {
-		addPort = strings.Split(config.StatsdUdpAddress, ":")
-		opts["statsd"] = fmt.Sprintf("{\"address\": \"%s\", \"port_value\": %s}", addPort[0], addPort[1])
+		h, p, err = GetHostPort("statsd UDP", config.StatsdUdpAddress)
+		if err != nil {
+			return "", err
+		}
+		StoreHostPort(h, p, "statsd", opts)
 	}
+
 	fout, err := os.Create(fname)
 	if err != nil {
 		return "", err

pkg/bootstrap/bootstrap_config_test.go

@@ -97,3 +97,92 @@ func loadProxyConfig(base, out string, t *testing.T) (*meshconfig.ProxyConfig, e
 	cfg.CustomConfigFile = gobase + "/tools/deb/envoy_bootstrap_tmpl.json"
 	return cfg, nil
 }
+
+func TestGetHostPort(t *testing.T) {
+	var testCases = []struct {
+		name         string
+		addr         string
+		expectedHost string
+		expectedPort string
+		errStr       string
+	}{
+		{
+			name:         "Valid IPv4 host/port",
+			addr:         "127.0.0.1:5000",
+			expectedHost: "127.0.0.1",
+			expectedPort: "5000",
+			errStr:       "",
+		},
+		{
+			name:         "Valid IPv6 host/port",
+			addr:         "[2001:db8::100]:5000",
+			expectedHost: "2001:db8::100",
+			expectedPort: "5000",
+			errStr:       "",
+		},
+		{
+			name:         "Valid host/port",
+			addr:         "istio-pilot:15005",
+			expectedHost: "istio-pilot",
+			expectedPort: "15005",
+			errStr:       "",
+		},
+		{
+			name:         "No port specified",
+			addr:         "127.0.0.1:",
+			expectedHost: "127.0.0.1",
+			expectedPort: "",
+			errStr:       "",
+		},
+		{
+			name:         "Missing port",
+			addr:         "127.0.0.1",
+			expectedHost: "",
+			expectedPort: "",
+			errStr:       "unable to parse test address \"127.0.0.1\": address 127.0.0.1: missing port in address",
+		},
+		{
+			name:         "Missing brackets for IPv6",
+			addr:         "2001:db8::100:5000",
+			expectedHost: "",
+			expectedPort: "",
+			errStr:       "unable to parse test address \"2001:db8::100:5000\": address 2001:db8::100:5000: too many colons in address",
+		},
+		{
+			name:         "No address provided",
+			addr:         "",
+			expectedHost: "",
+			expectedPort: "",
+			errStr:       "unable to parse test address \"\": missing port in address",
+		},
+	}
+	for _, tc := range testCases {
+		h, p, err := GetHostPort("test", tc.addr)
+		if err == nil {
+			if tc.errStr != "" {
+				t.Errorf("[%s] expected error %q, but no error seen", tc.name, tc.errStr)
+			} else if h != tc.expectedHost || p != tc.expectedPort {
+				t.Errorf("[%s] expected %s:%s, got %s:%s", tc.name, tc.expectedHost, tc.expectedPort, h, p)
+			}
+		} else {
+			if tc.errStr == "" {
+				t.Errorf("[%s] expected no error but got %q", tc.name, err.Error())
+			} else if err.Error() != tc.errStr {
+				t.Errorf("[%s] expected error message %q, got %v", tc.name, tc.errStr, err)
+			}
+		}
+	}
+}
+
+func TestStoreHostPort(t *testing.T) {
+	opts := map[string]interface{}{}
+	StoreHostPort("istio-pilot", "15005", "foo", opts)
+	actual, ok := opts["foo"]
+	if !ok {
+		t.Fatalf("expected to have map entry foo populated")
+	}
+	expected := "{\"address\": \"istio-pilot\", \"port_value\": 15005}"
+	if actual != expected {
+		t.Errorf("expected value %q, got %q", expected, actual)
+	}
+}