From e3817adb5b406d6672b92b01555a945ce064fc4e Mon Sep 17 00:00:00 2001
From: Xiang Li <xiang.li@coreos.com>
Date: Wed, 25 Mar 2015 11:48:51 -0700
Subject: [PATCH] etcdserver: loose member validation for joining existing
 cluster

---
 etcdserver/config.go      | 20 +++++++++++++-------
 etcdserver/config_test.go | 27 ++++++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/etcdserver/config.go b/etcdserver/config.go
index e06c498cd..c90692150 100644
--- a/etcdserver/config.go
+++ b/etcdserver/config.go
@@ -49,7 +49,7 @@ type ServerConfig struct {
 // VerifyBootstrapConfig sanity-checks the initial config for bootstrap case
 // and returns an error for things that should never happen.
 func (c *ServerConfig) VerifyBootstrap() error {
-	if err := c.verifyLocalMember(); err != nil {
+	if err := c.verifyLocalMember(true); err != nil {
 		return err
 	}
 	if err := c.Cluster.Validate(); err != nil {
@@ -64,7 +64,10 @@ func (c *ServerConfig) VerifyBootstrap() error {
 // VerifyJoinExisting sanity-checks the initial config for join existing cluster
 // case and returns an error for things that should never happen.
 func (c *ServerConfig) VerifyJoinExisting() error {
-	if err := c.verifyLocalMember(); err != nil {
+	// no need for strict checking since the member have announced its
+	// peer urls to the cluster before starting and do not have to set
+	// it in the configuration again.
+	if err := c.verifyLocalMember(false); err != nil {
 		return err
 	}
 	if err := c.Cluster.Validate(); err != nil {
@@ -76,9 +79,10 @@ func (c *ServerConfig) VerifyJoinExisting() error {
 	return nil
 }
 
-// verifyLocalMember verifies that the local member is valid and is listed
-// in the cluster correctly.
-func (c *ServerConfig) verifyLocalMember() error {
+// verifyLocalMember verifies the configured member is in configured
+// cluster. If strict is set, it also verifies the configured member
+// has the same peer urls as configured advertised peer urls.
+func (c *ServerConfig) verifyLocalMember(strict bool) error {
 	m := c.Cluster.MemberByName(c.Name)
 	// Make sure the cluster at least contains the local server.
 	if m == nil {
@@ -92,8 +96,10 @@ func (c *ServerConfig) verifyLocalMember() error {
 	// TODO: Remove URLStringsEqual after improvement of using hostnames #2150 #2123
 	apurls := c.PeerURLs.StringSlice()
 	sort.Strings(apurls)
-	if !netutil.URLStringsEqual(apurls, m.PeerURLs) {
-		return fmt.Errorf("%s has different advertised URLs in the cluster and advertised peer URLs list", c.Name)
+	if strict {
+		if !netutil.URLStringsEqual(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 6ee7f5db8..eb301515b 100644
--- a/etcdserver/config_test.go
+++ b/etcdserver/config_test.go
@@ -22,6 +22,9 @@ import (
 )
 
 func mustNewURLs(t *testing.T, urls []string) []url.URL {
+	if len(urls) == 0 {
+		return nil
+	}
 	u, err := types.NewURLs(urls)
 	if err != nil {
 		t.Fatalf("error creating new URLs from %q: %v", urls, err)
@@ -65,12 +68,14 @@ func TestConfigVerifyLocalMember(t *testing.T) {
 	tests := []struct {
 		clusterSetting string
 		apurls         []string
+		strict         bool
 		shouldError    bool
 	}{
 		{
 			// Node must exist in cluster
 			"",
 			nil,
+			true,
 
 			true,
 		},
@@ -78,6 +83,7 @@ func TestConfigVerifyLocalMember(t *testing.T) {
 			// Initial cluster set
 			"node1=http://localhost:7001,node2=http://localhost:7002",
 			[]string{"http://localhost:7001"},
+			true,
 
 			false,
 		},
@@ -85,6 +91,7 @@ func TestConfigVerifyLocalMember(t *testing.T) {
 			// Default initial cluster
 			"node1=http://localhost:2380,node1=http://localhost:7001",
 			[]string{"http://localhost:2380", "http://localhost:7001"},
+			true,
 
 			false,
 		},
@@ -92,6 +99,7 @@ func TestConfigVerifyLocalMember(t *testing.T) {
 			// Advertised peer URLs must match those in cluster-state
 			"node1=http://localhost:7001",
 			[]string{"http://localhost:12345"},
+			true,
 
 			true,
 		},
@@ -99,9 +107,26 @@ func TestConfigVerifyLocalMember(t *testing.T) {
 			// Advertised peer URLs must match those in cluster-state
 			"node1=http://localhost:7001,node1=http://localhost:12345",
 			[]string{"http://localhost:12345"},
+			true,
 
 			true,
 		},
+		{
+			// Advertised peer URLs must match those in cluster-state
+			"node1=http://localhost:7001",
+			[]string{},
+			true,
+
+			true,
+		},
+		{
+			// do not care about the urls if strict is not set
+			"node1=http://localhost:7001",
+			[]string{},
+			false,
+
+			false,
+		},
 	}
 
 	for i, tt := range tests {
@@ -116,7 +141,7 @@ func TestConfigVerifyLocalMember(t *testing.T) {
 		if tt.apurls != nil {
 			cfg.PeerURLs = mustNewURLs(t, tt.apurls)
 		}
-		err = cfg.verifyLocalMember()
+		err = cfg.verifyLocalMember(tt.strict)
 		if (err == nil) && tt.shouldError {
 			t.Errorf("%#v", *cluster)
 			t.Errorf("#%d: Got no error where one was expected", i)