Merge pull request #2552 from yichengq/fix-2396

etcdserver: check -initial-cluster in join case

etcdserver/config.go

@@ -46,9 +46,39 @@ type ServerConfig struct {
 	ElectionTicks int
 }
 
-// VerifyBootstrapConfig sanity-checks the initial config and returns an error
-// for things that should never happen.
-func (c *ServerConfig) VerifyBootstrapConfig() error {
+// 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 {
+		return err
+	}
+	if err := c.Cluster.Validate(); err != nil {
+		return err
+	}
+	if c.Cluster.String() == "" && c.DiscoveryURL == "" {
+		return fmt.Errorf("initial cluster unset and no discovery URL found")
+	}
+	return nil
+}
+
+// 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 {
+		return err
+	}
+	if err := c.Cluster.Validate(); err != nil {
+		return err
+	}
+	if c.DiscoveryURL != "" {
+		return fmt.Errorf("discovery URL should not be set when joining existing initial cluster")
+	}
+	return nil
+}
+
+// verifyLocalMember verifies that the local member is valid and is listed
+// in the cluster correctly.
+func (c *ServerConfig) verifyLocalMember() error {
 	m := c.Cluster.MemberByName(c.Name)
 	// Make sure the cluster at least contains the local server.
 	if m == nil {
@@ -58,14 +88,6 @@ func (c *ServerConfig) VerifyBootstrapConfig() error {
 		return fmt.Errorf("cannot use %x as member id", raft.None)
 	}
 
-	if c.DiscoveryURL == "" && !c.NewCluster {
-		return fmt.Errorf("initial cluster state unset and no wal or discovery URL found")
-	}
-
-	if err := c.Cluster.Validate(); err != nil {
-		return err
-	}
-
 	// Advertised peer URLs must match those in the cluster peer list
 	// TODO: Remove URLStringsEqual after improvement of using hostnames #2150 #2123
 	apurls := c.PeerURLs.StringSlice()

etcdserver/config_test.go

@@ -29,74 +29,76 @@ func mustNewURLs(t *testing.T, urls []string) []url.URL {
 	return u
 }
 
-func TestBootstrapConfigVerify(t *testing.T) {
+func TestConfigVerifyBootstrapWithoutClusterAndDiscoveryURLFail(t *testing.T) {
+	cluster, err := NewClusterFromString("", "")
+	if err != nil {
+		t.Fatalf("NewClusterFromString error: %v", err)
+	}
+	c := &ServerConfig{
+		Name:         "node1",
+		DiscoveryURL: "",
+		Cluster:      cluster,
+	}
+	if err := c.VerifyBootstrap(); err == nil {
+		t.Errorf("err = nil, want not nil")
+	}
+}
+
+func TestConfigVerifyExistingWithDiscoveryURLFail(t *testing.T) {
+	cluster, err := NewClusterFromString("", "node1=http://127.0.0.1:2380")
+	if err != nil {
+		t.Fatalf("NewClusterFromString error: %v", err)
+	}
+	c := &ServerConfig{
+		Name:         "node1",
+		DiscoveryURL: "http://127.0.0.1:4001/abcdefg",
+		PeerURLs:     mustNewURLs(t, []string{"http://127.0.0.1:2380"}),
+		Cluster:      cluster,
+		NewCluster:   false,
+	}
+	if err := c.VerifyJoinExisting(); err == nil {
+		t.Errorf("err = nil, want not nil")
+	}
+}
+
+func TestConfigVerifyLocalMember(t *testing.T) {
 	tests := []struct {
 		clusterSetting string
-		newclst        bool
 		apurls         []string
-		disc           string
 		shouldError    bool
 	}{
 		{
 			// 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
+			// Initial cluster set
 			"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",
+			// Default initial cluster
+			"node1=http://localhost:2380,node1=http://localhost:7001",
+			[]string{"http://localhost:2380", "http://localhost:7001"},
 
 			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,
 		},
@@ -108,15 +110,13 @@ func TestBootstrapConfigVerify(t *testing.T) {
 			t.Fatalf("#%d: Got unexpected error: %v", i, err)
 		}
 		cfg := ServerConfig{
-			Name:         "node1",
-			DiscoveryURL: tt.disc,
-			Cluster:      cluster,
-			NewCluster:   tt.newclst,
+			Name:    "node1",
+			Cluster: cluster,
 		}
 		if tt.apurls != nil {
 			cfg.PeerURLs = mustNewURLs(t, tt.apurls)
 		}
-		err = cfg.VerifyBootstrapConfig()
+		err = cfg.verifyLocalMember()
 		if (err == nil) && tt.shouldError {
 			t.Errorf("%#v", *cluster)
 			t.Errorf("#%d: Got no error where one was expected", i)

etcdserver/server.go

@@ -155,6 +155,9 @@ func NewServer(cfg *ServerConfig) (*EtcdServer, error) {
 
 	switch {
 	case !haveWAL && !cfg.NewCluster:
+		if err := cfg.VerifyJoinExisting(); err != nil {
+			return nil, err
+		}
 		existingCluster, err := GetClusterFromRemotePeers(getRemotePeerURLs(cfg.Cluster, cfg.Name), cfg.Transport)
 		if err != nil {
 			return nil, fmt.Errorf("cannot fetch cluster info from peer urls: %v", err)
@@ -168,7 +171,7 @@ func NewServer(cfg *ServerConfig) (*EtcdServer, error) {
 		cfg.Print()
 		id, n, s, w = startNode(cfg, nil)
 	case !haveWAL && cfg.NewCluster:
-		if err := cfg.VerifyBootstrapConfig(); err != nil {
+		if err := cfg.VerifyBootstrap(); err != nil {
 			return nil, err
 		}
 		m := cfg.Cluster.MemberByName(cfg.Name)