From 83137f9eba183b454264b9902f00739b42ba49a8 Mon Sep 17 00:00:00 2001 From: Brandon Philips Date: Wed, 1 Oct 2014 13:12:24 -0700 Subject: [PATCH] pkg/types: introduce a URLs type Cleanup the usage of URLs into its own type so we don't have to use a FlagValue everywhere we have a list of URLs. --- etcdserver/cluster.go | 6 ++++- etcdserver/member.go | 8 +++--- etcdserver/member_test.go | 5 ++-- etcdserver/server.go | 5 ++-- etcdserver/server_test.go | 5 ++-- main.go | 17 +++++++----- pkg/flag.go | 2 +- pkg/flag_test.go | 4 +-- pkg/flags/urls.go | 44 +++++++++--------------------- pkg/flags/urls_test.go | 8 +++--- pkg/types/urls.go | 56 +++++++++++++++++++++++++++++++++++++++ 11 files changed, 104 insertions(+), 56 deletions(-) create mode 100644 pkg/types/urls.go diff --git a/etcdserver/cluster.go b/etcdserver/cluster.go index c31e2e93c..eaace6217 100644 --- a/etcdserver/cluster.go +++ b/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 diff --git a/etcdserver/member.go b/etcdserver/member.go index 6a65adc29..ec648fba4 100644 --- a/etcdserver/member.go +++ b/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 { - sort.Strings(peerURLs) - m := &Member{Name: name, PeerURLs: peerURLs} +func newMember(name string, peerURLs types.URLs, now *time.Time) *Member { + m := &Member{Name: name, PeerURLs: peerURLs.StringSlice()} b := []byte(m.Name) for _, p := range m.PeerURLs { diff --git a/etcdserver/member_test.go b/etcdserver/member_test.go index cc077d25a..87d5ec6af 100644 --- a/etcdserver/member_test.go +++ b/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", []string{"http://10.0.0.1:2379"}, timeParse("1984-12-23T15:04:05Z")), 5483967913615174889}, + {newMember("mem1", []url.URL{{Scheme: "http", Host: "10.0.0.8:2379"}}, nil), 7206348984215161146}, + {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 { diff --git a/etcdserver/server.go b/etcdserver/server.go index ccfaef427..ed979bf53 100644 --- a/etcdserver/server.go +++ b/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) diff --git a/etcdserver/server_test.go b/etcdserver/server_test.go index 105548833..a91bda504 100644 --- a/etcdserver/server_test.go +++ b/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()) } diff --git a/main.go b/main.go index 38268f6cb..44e5cf879 100644 --- a/main.go +++ b/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.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(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.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.NewURLsValue("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: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()) } diff --git a/pkg/flag.go b/pkg/flag.go index 3eb66bef5..eecc46d7f 100644 --- a/pkg/flag.go +++ b/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 } diff --git a/pkg/flag_test.go b/pkg/flag_test.go index 32fd84a67..b913a4d91 100644 --- a/pkg/flag_test.go +++ b/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 { diff --git a/pkg/flags/urls.go b/pkg/flags/urls.go index 7ed69ff3c..bc3cf1e63 100644 --- a/pkg/flags/urls.go +++ b/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 -// URLs on the command-line -type URLs []url.URL +type URLsValue types.URLs // 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)) - if len(all) == 0 { - return errors.New("no valid URLs given") + nus, err := types.NewURLs(strs) + if err != nil { + return err } - 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) - } - 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 + + *us = URLsValue(nus) 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 { - v := &URLs{} +func NewURLsValue(init string) *URLsValue { + v := &URLsValue{} v.Set(init) return v } diff --git a/pkg/flags/urls_test.go b/pkg/flags/urls_test.go index caab40c0e..f400dc285 100644 --- a/pkg/flags/urls_test.go +++ b/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) } diff --git a/pkg/types/urls.go b/pkg/types/urls.go new file mode 100644 index 000000000..afa44ba17 --- /dev/null +++ b/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 +}