etcdserver: move peer URLs check to config

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 {

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

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
 }
 

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)