Merge pull request #1220 from bcwaldon/bkcompat

Backwards-compatibility with v0.4.6 addr-related flags
2014-10-01 12:53:50 -07:00 · 2014-10-01 12:53:50 -07:00 · 619c7f9fdb
parent f84b5b1071 dd88a08f8e
commit 619c7f9fdb
8 changed files with 263 additions and 114 deletions
--- a/main.go
+++ b/main.go
@ -5,7 +5,6 @@ import (
 	"fmt"
 	"log"
 	"net/http"
-	"net/url"
 	"os"
 	"path"
 	"strings"
@ -38,10 +37,6 @@ var (
 	printVersion = flag.Bool("version", false, "Print the version and exit")

 	cluster   = &etcdserver.Cluster{}
-	lcurls    = &flagtypes.URLs{}
-	acurls    = &flagtypes.URLs{}
-	lpurls    = &flagtypes.URLs{}
-	apurls    = &flagtypes.URLs{}
 	cors      = &pkg.CORSInfo{}
 	proxyFlag = new(flagtypes.Proxy)

@ -49,7 +44,6 @@ var (
 	peerTLSInfo   = transport.TLSInfo{}

 	deprecated = []string{
-		"addr",
 		"cluster-active-size",
 		"cluster-remove-delay",
 		"cluster-sync-interval",
@ -57,7 +51,6 @@ var (
 		"force",
 		"max-result-buffer",
 		"max-retry-attempts",
-		"peer-addr",
 		"peer-heartbeat-interval",
 		"peer-election-timeout",
 		"retry-interval",
@ -69,19 +62,16 @@ var (

 func init() {
 	flag.Var(cluster, "bootstrap-config", "Initial cluster configuration for bootstrapping")
-	flag.Var(apurls, "advertise-peer-urls", "List of this member's peer URLs to advertise to the rest of the cluster")
-	flag.Var(acurls, "advertise-client-urls", "List of this member's client URLs to advertise to the rest of the cluster")
-	flag.Var(lpurls, "listen-peer-urls", "List of this URLs to listen on for peer traffic")
-	flag.Var(lcurls, "listen-client-urls", "List of this URLs to listen on for client traffic")
-	flag.Var(cors, "cors", "Comma-separated white list of origins for CORS (cross-origin resource sharing).")
-	flag.Var(proxyFlag, "proxy", fmt.Sprintf("Valid values include %s", strings.Join(flagtypes.ProxyValues, ", ")))
-
 	cluster.Set("default=http://localhost:2380,default=http://localhost:7001")
-	lcurls.Set("http://localhost:2379,http://localhost:4001")
-	acurls.Set("http://localhost:2379,http://localhost:4001")
-	lpurls.Set("http://localhost:2380,http://localhost:7001")
-	apurls.Set("http://localhost:2380,http://localhost:7001")

+	flag.Var(flagtypes.NewURLs("http://localhost:2380,http://localhost:7001"), "advertise-peer-urls", "List of this member's peer URLs to advertise to the rest of the cluster")
+	flag.Var(flagtypes.NewURLs("http://localhost:2379,http://localhost:4001"), "advertise-client-urls", "List of this member's client URLs to advertise to the rest of the cluster")
+	flag.Var(flagtypes.NewURLs("http://localhost:2380,http://localhost:7001"), "listen-peer-urls", "List of this URLs to listen on for peer traffic")
+	flag.Var(flagtypes.NewURLs("http://localhost:2379,http://localhost:4001"), "listen-client-urls", "List of this URLs to listen on for client traffic")
+
+	flag.Var(cors, "cors", "Comma-separated white list of origins for CORS (cross-origin resource sharing).")
+
+	flag.Var(proxyFlag, "proxy", fmt.Sprintf("Valid values include %s", strings.Join(flagtypes.ProxyValues, ", ")))
 	proxyFlag.Set(flagtypes.ProxyValueOff)

 	flag.StringVar(&clientTLSInfo.CAFile, "ca-file", "", "Path to the client server TLS CA file.")
@ -92,6 +82,12 @@ func init() {
 	flag.StringVar(&peerTLSInfo.CertFile, "peer-cert-file", "", "Path to the peer server TLS cert file.")
 	flag.StringVar(&peerTLSInfo.KeyFile, "peer-key-file", "", "Path to the peer server TLS key file.")

+	// backwards-compatibility with v0.4.6
+	flag.Var(&flagtypes.IPAddressPort{}, "addr", "DEPRECATED: Use -advertise-client-urls instead.")
+	flag.Var(&flagtypes.IPAddressPort{}, "bind-addr", "DEPRECATED: Use -listen-client-urls instead.")
+	flag.Var(&flagtypes.IPAddressPort{}, "peer-addr", "DEPRECATED: Use -advertise-peer-urls instead.")
+	flag.Var(&flagtypes.IPAddressPort{}, "peer-bind-addr", "DEPRECATED: Use -listen-peer-urls instead.")
+
 	for _, f := range deprecated {
 		flag.Var(&pkg.DeprecatedFlag{f}, f, "")
 	}
@ -215,7 +211,12 @@ func startEtcd() {
 	}
 	ph := etcdhttp.NewPeerHandler(s)

-	for _, u := range []url.URL(*lpurls) {
+	lpurls, err := pkg.URLsFromFlags(flag.CommandLine, "listen-peer-urls", "peer-bind-addr", clientTLSInfo)
+	if err != nil {
+		log.Fatal(err.Error())
+	}
+
+	for _, u := range lpurls {
 		l, err := transport.NewListener(u.Host, peerTLSInfo)
 		if err != nil {
 			log.Fatal(err)
@ -229,8 +230,13 @@ func startEtcd() {
 		}()
 	}

+	lcurls, err := pkg.URLsFromFlags(flag.CommandLine, "listen-client-urls", "bind-addr", clientTLSInfo)
+	if err != nil {
+		log.Fatal(err.Error())
+	}
+
 	// Start a client server goroutine for each listen address
-	for _, u := range []url.URL(*lcurls) {
+	for _, u := range lcurls {
 		l, err := transport.NewListener(u.Host, clientTLSInfo)
 		if err != nil {
 			log.Fatal(err)
@ -265,8 +271,12 @@ func startProxy() {
 		ph = proxy.NewReadonlyHandler(ph)
 	}

+	lcurls, err := pkg.URLsFromFlags(flag.CommandLine, "listen-client-urls", "bind-addr", clientTLSInfo)
+	if err != nil {
+		log.Fatal(err.Error())
+	}
 	// Start a proxy server goroutine for each listen address
-	for _, u := range []url.URL(*lcurls) {
+	for _, u := range lcurls {
 		l, err := transport.NewListener(u.Host, clientTLSInfo)
 		if err != nil {
 			log.Fatal(err)
--- a/pkg/flag.go
+++ b/pkg/flag.go
@ -4,8 +4,12 @@ import (
 	"flag"
 	"fmt"
 	"log"
+	"net/url"
 	"os"
 	"strings"
+
+	"github.com/coreos/etcd/pkg/flags"
+	"github.com/coreos/etcd/pkg/transport"
 )

 type DeprecatedFlag struct {
@ -64,3 +68,38 @@ func SetFlagsFromEnv(fs *flag.FlagSet) {
 		}
 	})
 }
+
+// URLsFromFlags decides what URLs should be using two different flags
+// as datasources. The first flag's Value must be of type URLs, while
+// the second must be of type IPAddressPort. If both of these flags
+// are set, an error will be returned. If only the first flag is set,
+// the underlying url.URL objects will be returned unmodified. If the
+// second flag happens to be set, the underlying IPAddressPort will be
+// converted to a url.URL and returned. The Scheme of the returned
+// url.URL will be http unless the provided TLSInfo object is non-empty.
+// If neither of the flags have been explicitly set, the default value
+// of the first flag will be returned unmodified.
+func URLsFromFlags(fs *flag.FlagSet, urlsFlagName string, addrFlagName string, tlsInfo transport.TLSInfo) ([]url.URL, error) {
+	visited := make(map[string]struct{})
+	fs.Visit(func(f *flag.Flag) {
+		visited[f.Name] = struct{}{}
+	})
+
+	_, urlsFlagIsSet := visited[urlsFlagName]
+	_, addrFlagIsSet := visited[addrFlagName]
+
+	if addrFlagIsSet {
+		if urlsFlagIsSet {
+			return nil, fmt.Errorf("Set only one of flags -%s and -%s", urlsFlagName, addrFlagName)
+		}
+
+		addr := *fs.Lookup(addrFlagName).Value.(*flags.IPAddressPort)
+		addrURL := url.URL{Scheme: "http", Host: addr.String()}
+		if !tlsInfo.Empty() {
+			addrURL.Scheme = "https"
+		}
+		return []url.URL{addrURL}, nil
+	}
+
+	return []url.URL(*fs.Lookup(urlsFlagName).Value.(*flags.URLs)), nil
+}
--- a/pkg/flag_test.go
+++ b/pkg/flag_test.go
@ -2,8 +2,13 @@ package pkg

 import (
 	"flag"
+	"net/url"
 	"os"
+	"reflect"
 	"testing"
+
+	"github.com/coreos/etcd/pkg/flags"
+	"github.com/coreos/etcd/pkg/transport"
 )

 func TestSetFlagsFromEnv(t *testing.T) {
@ -49,3 +54,85 @@ func TestSetFlagsFromEnv(t *testing.T) {
 		}
 	}
 }
+
+func TestURLsFromFlags(t *testing.T) {
+	tests := []struct {
+		args     []string
+		tlsInfo  transport.TLSInfo
+		wantURLs []url.URL
+		wantFail bool
+	}{
+		// use -urls default when no flags defined
+		{
+			args:    []string{},
+			tlsInfo: transport.TLSInfo{},
+			wantURLs: []url.URL{
+				url.URL{Scheme: "http", Host: "127.0.0.1:2379"},
+			},
+			wantFail: false,
+		},
+
+		// explicitly setting -urls should carry through
+		{
+			args:    []string{"-urls=https://192.0.3.17:2930,http://127.0.0.1:1024"},
+			tlsInfo: transport.TLSInfo{},
+			wantURLs: []url.URL{
+				url.URL{Scheme: "https", Host: "192.0.3.17:2930"},
+				url.URL{Scheme: "http", Host: "127.0.0.1:1024"},
+			},
+			wantFail: false,
+		},
+
+		// explicitly setting -addr should carry through
+		{
+			args:    []string{"-addr=192.0.2.3:1024"},
+			tlsInfo: transport.TLSInfo{},
+			wantURLs: []url.URL{
+				url.URL{Scheme: "http", Host: "192.0.2.3:1024"},
+			},
+			wantFail: false,
+		},
+
+		// scheme prepended to -addr should be https if TLSInfo non-empty
+		{
+			args: []string{"-addr=192.0.2.3:1024"},
+			tlsInfo: transport.TLSInfo{
+				CertFile: "/tmp/foo",
+				KeyFile:  "/tmp/bar",
+			},
+			wantURLs: []url.URL{
+				url.URL{Scheme: "https", Host: "192.0.2.3:1024"},
+			},
+			wantFail: false,
+		},
+
+		// explicitly setting both -urls and -addr should fail
+		{
+			args:     []string{"-urls=https://127.0.0.1:1024", "-addr=192.0.2.3:1024"},
+			tlsInfo:  transport.TLSInfo{},
+			wantURLs: nil,
+			wantFail: true,
+		},
+	}
+
+	for i, tt := range tests {
+		fs := flag.NewFlagSet("test", flag.PanicOnError)
+		fs.Var(flags.NewURLs("http://127.0.0.1:2379"), "urls", "")
+		fs.Var(&flags.IPAddressPort{}, "addr", "")
+
+		if err := fs.Parse(tt.args); err != nil {
+			t.Errorf("#%d: failed to parse flags: %v", i, err)
+			continue
+		}
+
+		gotURLs, err := URLsFromFlags(fs, "urls", "addr", tt.tlsInfo)
+		if tt.wantFail != (err != nil) {
+			t.Errorf("#%d: wantFail=%t, got err=%v", i, tt.wantFail, err)
+			continue
+		}
+
+		if !reflect.DeepEqual(tt.wantURLs, gotURLs) {
+			t.Errorf("#%d: incorrect URLs\nwant=%#v\ngot=%#v", i, tt.wantURLs, gotURLs)
+		}
+	}
+}
--- a/pkg/flags/addrs.go
+++ b/pkg/flags/addrs.go
@ -1,51 +0,0 @@
-package flags
-
-import (
-	"errors"
-	"net"
-	"strconv"
-	"strings"
-)
-
-// Addrs implements the flag.Value interface to allow users to define multiple
-// listen addresses on the command-line
-type Addrs []string
-
-// Set parses a command line set of listen addresses, formatted like:
-// 127.0.0.1:7001,10.1.1.2:80
-func (as *Addrs) Set(s string) error {
-	parsed := make([]string, 0)
-	for _, in := range strings.Split(s, ",") {
-		a := strings.TrimSpace(in)
-		if err := validateAddr(a); err != nil {
-			return err
-		}
-		parsed = append(parsed, a)
-	}
-	if len(parsed) == 0 {
-		return errors.New("no valid addresses given!")
-	}
-	*as = parsed
-	return nil
-}
-
-func (as *Addrs) String() string {
-	return strings.Join(*as, ",")
-}
-
-// validateAddr ensures that the provided string is a valid address. Valid
-// addresses are of the form IP:port.
-// Returns an error if the address is invalid, else nil.
-func validateAddr(s string) error {
-	parts := strings.SplitN(s, ":", 2)
-	if len(parts) != 2 {
-		return errors.New("bad format in address specification")
-	}
-	if net.ParseIP(parts[0]) == nil {
-		return errors.New("bad IP in address specification")
-	}
-	if _, err := strconv.Atoi(parts[1]); err != nil {
-		return errors.New("bad port in address specification")
-	}
-	return nil
-}
--- a/pkg/flags/addrs_test.go
+++ b/pkg/flags/addrs_test.go
@ -1,42 +0,0 @@
-package flags
-
-import (
-	"testing"
-)
-
-func TestBadValidateAddr(t *testing.T) {
-	tests := []string{
-		// bad IP specification
-		":4001",
-		"127.0:8080",
-		"123:456",
-		// bad port specification
-		"127.0.0.1:foo",
-		"127.0.0.1:",
-		// unix sockets not supported
-		"unix://",
-		"unix://tmp/etcd.sock",
-		// bad strings
-		"somewhere",
-		"234#$",
-		"file://foo/bar",
-		"http://hello",
-	}
-	for i, in := range tests {
-		if err := validateAddr(in); err == nil {
-			t.Errorf(`#%d: unexpected nil error for in=%q`, i, in)
-		}
-	}
-}
-
-func TestValidateAddr(t *testing.T) {
-	tests := []string{
-		"1.2.3.4:8080",
-		"10.1.1.1:80",
-	}
-	for i, in := range tests {
-		if err := validateAddr(in); err != nil {
-			t.Errorf("#%d: err=%v, want nil for in=%q", i, err, in)
-		}
-	}
-}
--- a/pkg/flags/ipaddressport.go
+++ b/pkg/flags/ipaddressport.go
@ -0,0 +1,43 @@
+package flags
+
+import (
+	"errors"
+	"fmt"
+	"net"
+	"strconv"
+	"strings"
+)
+
+// IPAddressPort implements the flag.Value interface. The argument
+// is validated as "ip:port".
+type IPAddressPort struct {
+	IP   string
+	Port int
+}
+
+func (a *IPAddressPort) Set(arg string) error {
+	arg = strings.TrimSpace(arg)
+
+	parts := strings.SplitN(arg, ":", 2)
+	if len(parts) != 2 {
+		return errors.New("bad format in address specification")
+	}
+
+	if net.ParseIP(parts[0]) == nil {
+		return errors.New("bad IP in address specification")
+	}
+
+	port, err := strconv.Atoi(parts[1])
+	if err != nil {
+		return errors.New("bad port in address specification")
+	}
+
+	a.IP = parts[0]
+	a.Port = port
+
+	return nil
+}
+
+func (a *IPAddressPort) String() string {
+	return fmt.Sprintf("%s:%d", a.IP, a.Port)
+}
--- a/pkg/flags/ipaddressport_test.go
+++ b/pkg/flags/ipaddressport_test.go
@ -0,0 +1,57 @@
+package flags
+
+import (
+	"testing"
+)
+
+func TestIPAddressPortSet(t *testing.T) {
+	pass := []string{
+		"1.2.3.4:8080",
+		"10.1.1.1:80",
+	}
+
+	fail := []string{
+		// bad IP specification
+		":4001",
+		"127.0:8080",
+		"123:456",
+		// bad port specification
+		"127.0.0.1:foo",
+		"127.0.0.1:",
+		// unix sockets not supported
+		"unix://",
+		"unix://tmp/etcd.sock",
+		// bad strings
+		"somewhere",
+		"234#$",
+		"file://foo/bar",
+		"http://hello",
+	}
+
+	for i, tt := range pass {
+		f := &IPAddressPort{}
+		if err := f.Set(tt); err != nil {
+			t.Errorf("#%d: unexpected error from IPAddressPort.Set(%q): %v", i, tt, err)
+		}
+	}
+
+	for i, tt := range fail {
+		f := &IPAddressPort{}
+		if err := f.Set(tt); err == nil {
+			t.Errorf("#%d: expected error from IPAddressPort.Set(%q)", i, tt, err)
+		}
+	}
+}
+
+func TestIPAddressPortString(t *testing.T) {
+	f := &IPAddressPort{}
+	if err := f.Set("127.0.0.1:4001"); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	want := "127.0.0.1:4001"
+	got := f.String()
+	if want != got {
+		t.Fatalf("IPAddressPort.String() value should be %q, got %q", want, got)
+	}
+}
--- a/pkg/flags/urls.go
+++ b/pkg/flags/urls.go
@ -48,3 +48,9 @@ func (us *URLs) String() string {
 	}
 	return strings.Join(all, ",")
 }
+
+func NewURLs(init string) *URLs {
+	v := &URLs{}
+	v.Set(init)
+	return v
+}