Merge pull request #1221 from philips/fixing-stuff

pkg/types: introduce a URLs type

etcdserver/cluster.go

@@ -6,6 +6,9 @@ import (
 	"net/url"
 	"sort"
 	"strings"
+
+	"github.com/coreos/etcd/pkg/flags"
+	"github.com/coreos/etcd/pkg/types"
 )
 
 // Cluster is a list of Members that belong to the same raft cluster
@@ -71,7 +74,8 @@ func (c *Cluster) Set(s string) error {
 		if len(urls) == 0 || urls[0] == "" {
 			return fmt.Errorf("Empty URL given for %q", name)
 		}
-		m := newMember(name, urls, nil)
+
+		m := newMember(name, types.URLs(*flags.NewURLsValue(strings.Join(urls, ","))), nil)
 		err := c.Add(*m)
 		if err != nil {
 			return err

etcdserver/member.go

@@ -5,9 +5,10 @@ import (
 	"encoding/binary"
 	"fmt"
 	"path"
-	"sort"
 	"strconv"
 	"time"
+
+	"github.com/coreos/etcd/pkg/types"
 )
 
 const machineKVPrefix = "/_etcd/machines/"
@@ -22,9 +23,8 @@ type Member struct {
 
 // newMember creates a Member without an ID and generates one based on the
 // name, peer URLs. This is used for bootstrapping.
-func newMember(name string, peerURLs []string, now *time.Time) *Member {
+func newMember(name string, peerURLs types.URLs, now *time.Time) *Member {
-	sort.Strings(peerURLs)
+	m := &Member{Name: name, PeerURLs: peerURLs.StringSlice()}
-	m := &Member{Name: name, PeerURLs: peerURLs}
 
 	b := []byte(m.Name)
 	for _, p := range m.PeerURLs {

etcdserver/member_test.go

@@ -1,6 +1,7 @@
 package etcdserver
 
 import (
+	"net/url"
 	"testing"
 	"time"
 )
@@ -18,8 +19,8 @@ func TestMemberTime(t *testing.T) {
 		mem *Member
 		id  int64
 	}{
-		{newMember("mem1", []string{"http://10.0.0.8:2379"}, nil), 7206348984215161146},
+		{newMember("mem1", []url.URL{{Scheme: "http", Host: "10.0.0.8:2379"}}, nil), 7206348984215161146},
-		{newMember("mem1", []string{"http://10.0.0.1:2379"}, timeParse("1984-12-23T15:04:05Z")), 5483967913615174889},
+		{newMember("mem1", []url.URL{{Scheme: "http", Host: "10.0.0.1:2379"}}, timeParse("1984-12-23T15:04:05Z")), 5483967913615174889},
 	}
 	for i, tt := range tests {
 		if tt.mem.ID != tt.id {

etcdserver/server.go

@@ -9,6 +9,7 @@ import (
 	"time"
 
 	pb "github.com/coreos/etcd/etcdserver/etcdserverpb"
+	"github.com/coreos/etcd/pkg/types"
 	"github.com/coreos/etcd/raft"
 	"github.com/coreos/etcd/raft/raftpb"
 	"github.com/coreos/etcd/store"
@@ -81,7 +82,7 @@ type EtcdServer struct {
 	done chan struct{}
 
 	Name       string
-	ClientURLs []string
+	ClientURLs types.URLs
 
 	Node  raft.Node
 	Store store.Store
@@ -340,7 +341,7 @@ func (s *EtcdServer) sync(timeout time.Duration) {
 // TODO: take care of info fetched from cluster store after having reconfig.
 func (s *EtcdServer) publish(retryInterval time.Duration) {
 	m := *s.ClusterStore.Get().FindName(s.Name)
-	m.ClientURLs = s.ClientURLs
+	m.ClientURLs = s.ClientURLs.StringSlice()
 	b, err := json.Marshal(m)
 	if err != nil {
 		log.Printf("etcdserver: json marshal error: %v", err)

etcdserver/server_test.go

@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"math/rand"
+	"net/url"
 	"reflect"
 	"sync"
 	"testing"
@@ -836,7 +837,7 @@ func TestPublish(t *testing.T) {
 	w := &waitWithResponse{ch: ch}
 	srv := &EtcdServer{
 		Name:         "node1",
-		ClientURLs:   []string{"a", "b"},
+		ClientURLs:   []url.URL{{Scheme: "http", Host: "a"}, {Scheme: "http", Host: "b"}},
 		Node:         n,
 		ClusterStore: cs,
 		w:            w,
@@ -854,7 +855,7 @@ func TestPublish(t *testing.T) {
 	if r.Method != "PUT" {
 		t.Errorf("method = %s, want PUT", r.Method)
 	}
-	wm := Member{ID: 1, Name: "node1", ClientURLs: []string{"a", "b"}}
+	wm := Member{ID: 1, Name: "node1", ClientURLs: []string{"http://a", "http://b"}}
 	if r.Path != wm.storeKey() {
 		t.Errorf("path = %s, want %s", r.Path, wm.storeKey())
 	}

main.go

@@ -64,10 +64,10 @@ func init() {
 	flag.Var(cluster, "bootstrap-config", "Initial cluster configuration for bootstrapping")
 	cluster.Set("default=http://localhost:2380,default=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.NewURLsValue("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.NewURLsValue("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.NewURLsValue("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(flagtypes.NewURLsValue("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).")
 
@@ -188,9 +188,14 @@ func startEtcd() {
 
 	cls := etcdserver.NewClusterStore(st, *cluster)
 
+	acurls, err := pkg.URLsFromFlags(flag.CommandLine, "advertise-client-urls", "addr", clientTLSInfo)
+	if err != nil {
+		log.Fatal(err.Error())
+	}
+
 	s := &etcdserver.EtcdServer{
 		Name:       *name,
-		ClientURLs: strings.Split(acurls.String(), ","),
+		ClientURLs: acurls,
 		Store:      st,
 		Node:       n,
 		Storage: struct {
@@ -211,7 +216,7 @@ func startEtcd() {
 	}
 	ph := etcdhttp.NewPeerHandler(s)
 
-	lpurls, err := pkg.URLsFromFlags(flag.CommandLine, "listen-peer-urls", "peer-bind-addr", clientTLSInfo)
+	lpurls, err := pkg.URLsFromFlags(flag.CommandLine, "listen-peer-urls", "peer-bind-addr", peerTLSInfo)
 	if err != nil {
 		log.Fatal(err.Error())
 	}

pkg/flag.go

@@ -101,5 +101,5 @@ func URLsFromFlags(fs *flag.FlagSet, urlsFlagName string, addrFlagName string, t
 		return []url.URL{addrURL}, nil
 	}
 
-	return []url.URL(*fs.Lookup(urlsFlagName).Value.(*flags.URLs)), nil
+	return []url.URL(*fs.Lookup(urlsFlagName).Value.(*flags.URLsValue)), nil
 }

pkg/flag_test.go

@@ -77,8 +77,8 @@ func TestURLsFromFlags(t *testing.T) {
 			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"},
+				url.URL{Scheme: "https", Host: "192.0.3.17:2930"},
 			},
 			wantFail: false,
 		},
@@ -117,7 +117,7 @@ func TestURLsFromFlags(t *testing.T) {
 
 	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.NewURLsValue("http://127.0.0.1:2379"), "urls", "")
 		fs.Var(&flags.IPAddressPort{}, "addr", "")
 
 		if err := fs.Parse(tt.args); err != nil {

pkg/flags/urls.go

@@ -1,47 +1,27 @@
 package flags
 
 import (
-	"errors"
-	"fmt"
-	"net"
-	"net/url"
 	"strings"
+
+	"github.com/coreos/etcd/pkg/types"
 )
 
-// URLs implements the flag.Value interface to allow users to define multiple
+type URLsValue types.URLs
-// URLs on the command-line
-type URLs []url.URL
 
 // Set parses a command line set of URLs formatted like:
 // http://127.0.0.1:7001,http://10.1.1.2:80
-func (us *URLs) Set(s string) error {
+func (us *URLsValue) Set(s string) error {
 	strs := strings.Split(s, ",")
-	all := make([]url.URL, len(strs))
+	nus, err := types.NewURLs(strs)
-	if len(all) == 0 {
-		return errors.New("no valid URLs given")
-	}
-	for i, in := range strs {
-		in = strings.TrimSpace(in)
-		u, err := url.Parse(in)
 	if err != nil {
 		return err
 	}
-		if u.Scheme != "http" && u.Scheme != "https" {
+
-			return fmt.Errorf("URL scheme must be http or https: %s", s)
+	*us = URLsValue(nus)
-		}
-		if _, _, err := net.SplitHostPort(u.Host); err != nil {
-			return fmt.Errorf(`URL address does not have the form "host:port": %s`, s)
-		}
-		if u.Path != "" {
-			return fmt.Errorf("URL must not contain a path: %s", s)
-		}
-		all[i] = *u
-	}
-	*us = all
 	return nil
 }
 
-func (us *URLs) String() string {
+func (us *URLsValue) String() string {
 	all := make([]string, len(*us))
 	for i, u := range *us {
 		all[i] = u.String()
@@ -49,8 +29,8 @@ func (us *URLs) String() string {
 	return strings.Join(all, ",")
 }
 
-func NewURLs(init string) *URLs {
+func NewURLsValue(init string) *URLsValue {
-	v := &URLs{}
+	v := &URLsValue{}
 	v.Set(init)
 	return v
 }

pkg/flags/urls_test.go

@@ -4,7 +4,7 @@ import (
 	"testing"
 )
 
-func TestValidateURLsBad(t *testing.T) {
+func TestValidateURLsValueBad(t *testing.T) {
 	tests := []string{
 		// bad IP specification
 		":4001",
@@ -24,14 +24,14 @@ func TestValidateURLsBad(t *testing.T) {
 		"http://10.1.1.1",
 	}
 	for i, in := range tests {
-		u := URLs{}
+		u := URLsValue{}
 		if err := u.Set(in); err == nil {
 			t.Errorf(`#%d: unexpected nil error for in=%q`, i, in)
 		}
 	}
 }
 
-func TestValidateURLsGood(t *testing.T) {
+func TestValidateURLsValueGood(t *testing.T) {
 	tests := []string{
 		"https://1.2.3.4:8080",
 		"http://10.1.1.1:80",
@@ -39,7 +39,7 @@ func TestValidateURLsGood(t *testing.T) {
 		"http://:80",
 	}
 	for i, in := range tests {
-		u := URLs{}
+		u := URLsValue{}
 		if err := u.Set(in); err != nil {
 			t.Errorf("#%d: err=%v, want nil for in=%q", i, err, in)
 		}

pkg/types/urls.go

@@ -0,0 +1,56 @@
+package types
+
+import (
+	"errors"
+	"fmt"
+	"net"
+	"net/url"
+	"sort"
+	"strings"
+)
+
+type URLs []url.URL
+
+func (us *URLs) Sort() {
+	sort.Sort(us)
+}
+func (us URLs) Len() int           { return len(us) }
+func (us URLs) Less(i, j int) bool { return us[i].String() < us[j].String() }
+func (us URLs) Swap(i, j int)      { us[i], us[j] = us[j], us[i] }
+
+func (us URLs) StringSlice() []string {
+	out := make([]string, len(us))
+	for i := range us {
+		out[i] = us[i].String()
+	}
+
+	return out
+}
+
+func NewURLs(strs []string) (URLs, error) {
+	all := make([]url.URL, len(strs))
+	if len(all) == 0 {
+		return nil, errors.New("no valid URLs given")
+	}
+	for i, in := range strs {
+		in = strings.TrimSpace(in)
+		u, err := url.Parse(in)
+		if err != nil {
+			return nil, err
+		}
+		if u.Scheme != "http" && u.Scheme != "https" {
+			return nil, fmt.Errorf("URL scheme must be http or https: %s", in)
+		}
+		if _, _, err := net.SplitHostPort(u.Host); err != nil {
+			return nil, fmt.Errorf(`URL address does not have the form "host:port": %s`, in)
+		}
+		if u.Path != "" {
+			return nil, fmt.Errorf("URL must not contain a path: %s", in)
+		}
+		all[i] = *u
+	}
+	us := URLs(all)
+	us.Sort()
+
+	return us, nil
+}