From 1197c1f96500271d974bb4021ccc31f4ab4e960b Mon Sep 17 00:00:00 2001 From: Jonathan Boulle Date: Mon, 10 Nov 2014 13:45:29 -0800 Subject: [PATCH] etcdserver: move peer URLs check to config --- etcdmain/etcd.go | 44 ++++++++++---------------------- etcdmain/etcd_test.go | 53 --------------------------------------- etcdserver/config.go | 10 ++++++++ etcdserver/config_test.go | 48 +++++++++++++++++++++++++++++++++-- 4 files changed, 69 insertions(+), 86 deletions(-) diff --git a/etcdmain/etcd.go b/etcdmain/etcd.go index 8af5e432b..c5c603872 100644 --- a/etcdmain/etcd.go +++ b/etcdmain/etcd.go @@ -24,8 +24,6 @@ import ( "net/http" "net/url" "os" - "reflect" - "sort" "strings" "github.com/coreos/etcd/discovery" @@ -191,7 +189,11 @@ func Main() { // startEtcd launches the etcd server and HTTP handlers for client/server communication. func startEtcd() error { - cls, err := setupCluster() + apurls, err := flags.URLsFromFlags(fs, "initial-advertise-peer-urls", "addr", peerTLSInfo) + if err != nil { + return err + } + cls, err := setupCluster(apurls) if err != nil { return fmt.Errorf("error setting up initial cluster: %v", err) } @@ -268,6 +270,7 @@ func startEtcd() error { cfg := &etcdserver.ServerConfig{ Name: *name, ClientURLs: acurls, + PeerURLs: apurls, DataDir: *dir, SnapCount: *snapCount, Cluster: cls, @@ -306,7 +309,11 @@ func startEtcd() error { // startProxy launches an HTTP proxy for client communication which proxies to other etcd nodes. func startProxy() error { - cls, err := setupCluster() + apurls, err := flags.URLsFromFlags(fs, "initial-advertise-peer-urls", "addr", peerTLSInfo) + if err != nil { + return err + } + cls, err := setupCluster(apurls) if err != nil { return fmt.Errorf("error setting up initial cluster: %v", err) } @@ -367,7 +374,7 @@ func startProxy() error { } // setupCluster sets up an initial cluster definition for bootstrap or discovery. -func setupCluster() (*etcdserver.Cluster, error) { +func setupCluster(apurls []url.URL) (*etcdserver.Cluster, error) { set := make(map[string]bool) fs.Visit(func(f *flag.Flag) { set[f.Name] = true @@ -375,12 +382,8 @@ func setupCluster() (*etcdserver.Cluster, error) { if set["discovery"] && set["initial-cluster"] { return nil, fmt.Errorf("both discovery and bootstrap-config are set") } - apurls, err := flags.URLsFromFlags(fs, "initial-advertise-peer-urls", "addr", peerTLSInfo) - if err != nil { - return nil, err - } - var cls *etcdserver.Cluster + var err error switch { case set["discovery"]: // If using discovery, generate a temporary cluster based on @@ -392,31 +395,10 @@ func setupCluster() (*etcdserver.Cluster, error) { default: // We're statically configured, and cluster has appropriately been set. cls, err = etcdserver.NewClusterFromString(*initialClusterToken, *initialCluster) - // Ensure our own advertised peer URLs match those specified in cluster - if err == nil && !clusterPeerURLsMatch(*name, cls, apurls) { - cls = nil - err = fmt.Errorf("%s has different advertised URLs in the cluster and advertised peer URLs list", *name) - } } return cls, err } -// clusterPeerURLsMatch checks whether the peer URLs of the member by the given -// name in the given cluster match the provided set of URLs -func clusterPeerURLsMatch(name string, cls *etcdserver.Cluster, urls []url.URL) bool { - m := cls.MemberByName(name) - if m == nil { - // should never happen - log.Panicf("could not find %q in cluster!", name) - } - purls := make([]string, len(urls)) - for i, u := range urls { - purls[i] = u.String() - } - sort.Strings(purls) - return reflect.DeepEqual(purls, m.PeerURLs) -} - func genClusterString(name string, urls types.URLs) string { addrs := make([]string, 0) for _, u := range urls { diff --git a/etcdmain/etcd_test.go b/etcdmain/etcd_test.go index 147c6e05d..dff57f618 100644 --- a/etcdmain/etcd_test.go +++ b/etcdmain/etcd_test.go @@ -20,18 +20,9 @@ import ( "net/url" "testing" - "github.com/coreos/etcd/etcdserver" "github.com/coreos/etcd/pkg/types" ) -func mustClsFromString(t *testing.T, s string) *etcdserver.Cluster { - cls, err := etcdserver.NewClusterFromString("", s) - if err != nil { - t.Fatalf("error creating cluster from %q: %v", s, err) - } - return cls -} - func mustNewURLs(t *testing.T, urls []string) []url.URL { u, err := types.NewURLs(urls) if err != nil { @@ -40,50 +31,6 @@ func mustNewURLs(t *testing.T, urls []string) []url.URL { return u } -func TestClusterPeerURLsMatch(t *testing.T) { - tests := []struct { - name string - cls *etcdserver.Cluster - urls []url.URL - - w bool - }{ - { - name: "default", - cls: mustClsFromString(t, "default=http://localhost:12345"), - urls: mustNewURLs(t, []string{"http://localhost:12345"}), - - w: true, - }, - { - name: "default", - cls: mustClsFromString(t, "default=http://localhost:7001,other=http://192.168.0.1:7002,default=http://localhost:12345"), - urls: mustNewURLs(t, []string{"http://localhost:7001", "http://localhost:12345"}), - - w: true, - }, - { - name: "infra1", - cls: mustClsFromString(t, "infra1=http://localhost:7001"), - urls: mustNewURLs(t, []string{"http://localhost:12345"}), - - w: false, - }, - { - name: "infra1", - cls: mustClsFromString(t, "infra1=http://localhost:7001,infra2=http://localhost:12345"), - urls: mustNewURLs(t, []string{"http://localhost:12345"}), - - w: false, - }, - } - for i, tt := range tests { - if g := clusterPeerURLsMatch(tt.name, tt.cls, tt.urls); g != tt.w { - t.Errorf("#%d: clusterPeerURLsMatch=%t, want %t", i, g, tt.w) - } - } -} - func TestGenClusterString(t *testing.T) { tests := []struct { token string diff --git a/etcdserver/config.go b/etcdserver/config.go index 59d9486fe..6a434b0d3 100644 --- a/etcdserver/config.go +++ b/etcdserver/config.go @@ -20,6 +20,8 @@ import ( "fmt" "net/http" "path" + "reflect" + "sort" "github.com/coreos/etcd/pkg/types" "github.com/coreos/etcd/raft" @@ -31,6 +33,7 @@ type ServerConfig struct { DiscoveryURL string DiscoveryProxy string ClientURLs types.URLs + PeerURLs types.URLs DataDir string SnapCount uint64 Cluster *Cluster @@ -65,6 +68,13 @@ func (c *ServerConfig) VerifyBootstrapConfig() error { urlMap[url] = true } } + + // Advertised peer URLs must match those in the cluster peer list + apurls := c.PeerURLs.StringSlice() + sort.Strings(apurls) + if !reflect.DeepEqual(apurls, m.PeerURLs) { + return fmt.Errorf("%s has different advertised URLs in the cluster and advertised peer URLs list", c.Name) + } return nil } diff --git a/etcdserver/config_test.go b/etcdserver/config_test.go index d5bda587d..bbfb59882 100644 --- a/etcdserver/config_test.go +++ b/etcdserver/config_test.go @@ -16,12 +16,26 @@ package etcdserver -import "testing" +import ( + "net/url" + "testing" + + "github.com/coreos/etcd/pkg/types" +) + +func mustNewURLs(t *testing.T, urls []string) []url.URL { + u, err := types.NewURLs(urls) + if err != nil { + t.Fatalf("error creating new URLs from %q: %v", urls, err) + } + return u +} func TestBootstrapConfigVerify(t *testing.T) { tests := []struct { clusterSetting string newclst bool + apurls []string disc string shouldError bool }{ @@ -29,35 +43,63 @@ func TestBootstrapConfigVerify(t *testing.T) { // Node must exist in cluster "", true, + nil, "", + true, }, { // Cannot have duplicate URLs in cluster config "node1=http://localhost:7001,node2=http://localhost:7001,node2=http://localhost:7002", true, + nil, "", + true, }, { // Node defined, ClusterState OK "node1=http://localhost:7001,node2=http://localhost:7002", true, + []string{"http://localhost:7001"}, "", + false, }, { // Node defined, discovery OK "node1=http://localhost:7001", false, + []string{"http://localhost:7001"}, "http://discovery", + false, }, { // Cannot have ClusterState!=new && !discovery "node1=http://localhost:7001", false, + nil, "", + + true, + }, + { + // Advertised peer URLs must match those in cluster-state + "node1=http://localhost:7001", + true, + []string{"http://localhost:12345"}, + "", + + true, + }, + { + // Advertised peer URLs must match those in cluster-state + "node1=http://localhost:7001,node1=http://localhost:12345", + true, + []string{"http://localhost:12345"}, + "", + true, }, } @@ -67,13 +109,15 @@ func TestBootstrapConfigVerify(t *testing.T) { if err != nil { t.Fatalf("#%d: Got unexpected error: %v", i, err) } - cfg := ServerConfig{ Name: "node1", DiscoveryURL: tt.disc, Cluster: cluster, NewCluster: tt.newclst, } + if tt.apurls != nil { + cfg.PeerURLs = mustNewURLs(t, tt.apurls) + } err = cfg.VerifyBootstrapConfig() if (err == nil) && tt.shouldError { t.Errorf("%#v", *cluster)