Merge pull request #1627 from xiangli-cmu/validate_peer_url

etcdserver: validate peerurl when adding members

etcdserver/cluster.go

@@ -242,7 +242,6 @@ func (c *Cluster) SetStore(st store.Store) { c.store = st }
 
 func (c *Cluster) ValidateConfigurationChange(cc raftpb.ConfChange) error {
 	appliedMembers, appliedRemoved := membersFromStore(c.store)
-
 	if appliedRemoved[types.ID(cc.NodeID)] {
 		return ErrIDRemoved
 	}
@@ -251,6 +250,21 @@ func (c *Cluster) ValidateConfigurationChange(cc raftpb.ConfChange) error {
 		if appliedMembers[types.ID(cc.NodeID)] != nil {
 			return ErrIDExists
 		}
+		urls := make(map[string]bool)
+		for _, m := range appliedMembers {
+			for _, u := range m.PeerURLs {
+				urls[u] = true
+			}
+		}
+		m := new(Member)
+		if err := json.Unmarshal(cc.Context, m); err != nil {
+			log.Panicf("unmarshal member should never fail: %v", err)
+		}
+		for _, u := range m.PeerURLs {
+			if urls[u] {
+				return ErrPeerURLexists
+			}
+		}
 	case raftpb.ConfChangeRemoveNode:
 		if appliedMembers[types.ID(cc.NodeID)] == nil {
 			return ErrIDNotFound

etcdserver/cluster_test.go

@@ -17,11 +17,14 @@
 package etcdserver
 
 import (
+	"encoding/json"
+	"fmt"
 	"path"
 	"reflect"
 	"testing"
 
 	"github.com/coreos/etcd/pkg/types"
+	"github.com/coreos/etcd/raft/raftpb"
 	"github.com/coreos/etcd/store"
 )
 
@@ -351,6 +354,77 @@ func TestClusterValidateAndAssignIDs(t *testing.T) {
 	}
 }
 
+func TestClusterValidateConfigurationChange(t *testing.T) {
+	cl := newCluster("")
+	cl.SetStore(store.New())
+	for i := 1; i <= 4; i++ {
+		attr := RaftAttributes{PeerURLs: []string{fmt.Sprintf("http://127.0.0.1:%d", i)}}
+		cl.AddMember(&Member{ID: types.ID(i), RaftAttributes: attr})
+	}
+	cl.RemoveMember(4)
+
+	attr := RaftAttributes{PeerURLs: []string{fmt.Sprintf("http://127.0.0.1:%d", 1)}}
+	cxt, err := json.Marshal(&Member{ID: types.ID(5), RaftAttributes: attr})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	tests := []struct {
+		cc   raftpb.ConfChange
+		werr error
+	}{
+		{
+			raftpb.ConfChange{
+				Type:   raftpb.ConfChangeRemoveNode,
+				NodeID: 3,
+			},
+			nil,
+		},
+		{
+			raftpb.ConfChange{
+				Type:   raftpb.ConfChangeAddNode,
+				NodeID: 4,
+			},
+			ErrIDRemoved,
+		},
+		{
+			raftpb.ConfChange{
+				Type:   raftpb.ConfChangeRemoveNode,
+				NodeID: 4,
+			},
+			ErrIDRemoved,
+		},
+		{
+			raftpb.ConfChange{
+				Type:   raftpb.ConfChangeAddNode,
+				NodeID: 1,
+			},
+			ErrIDExists,
+		},
+		{
+			raftpb.ConfChange{
+				Type:    raftpb.ConfChangeAddNode,
+				NodeID:  5,
+				Context: cxt,
+			},
+			ErrPeerURLexists,
+		},
+		{
+			raftpb.ConfChange{
+				Type:   raftpb.ConfChangeRemoveNode,
+				NodeID: 5,
+			},
+			ErrIDNotFound,
+		},
+	}
+	for i, tt := range tests {
+		err := cl.ValidateConfigurationChange(tt.cc)
+		if err != tt.werr {
+			t.Errorf("#%d: validateConfigurationChange error = %v, want %v", i, err, tt.werr)
+		}
+	}
+}
+
 func TestClusterGenID(t *testing.T) {
 	cs := newTestCluster([]Member{
 		newTestMember(1, nil, "", nil),

etcdserver/etcdhttp/client.go

@@ -186,12 +186,16 @@ func (h *membersHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 		now := h.clock.Now()
 		m := etcdserver.NewMember("", req.PeerURLs, "", &now)
-		if err := h.server.AddMember(ctx, *m); err != nil {
+		err = h.server.AddMember(ctx, *m)
+		switch {
+		case err == etcdserver.ErrIDExists || err == etcdserver.ErrPeerURLexists:
+			writeError(w, httptypes.NewHTTPError(http.StatusPreconditionFailed, err.Error()))
+			return
+		case err != nil:
 			log.Printf("etcdhttp: error adding node %s: %v", m.ID, err)
 			writeError(w, err)
 			return
 		}
-
 		res := newMember(m)
 		w.Header().Set("Content-Type", "application/json")
 		w.WriteHeader(http.StatusCreated)

etcdserver/server.go

@@ -64,6 +64,7 @@ var (
 	ErrIDRemoved     = errors.New("etcdserver: ID removed")
 	ErrIDExists      = errors.New("etcdserver: ID exists")
 	ErrIDNotFound    = errors.New("etcdserver: ID not found")
+	ErrPeerURLexists = errors.New("etcdserver: peerURL exists")
 	ErrCanceled      = errors.New("etcdserver: request cancelled")
 	ErrTimeout       = errors.New("etcdserver: request timed out")