diff --git a/cluster.go b/cluster.go index d9575b58e..c7f5c4193 100644 --- a/cluster.go +++ b/cluster.go @@ -1690,6 +1690,16 @@ func (c *Cluster) NodeLeave(node *Node) error { return fmt.Errorf("Cluster must be in state %s to remove a node. Current state: %s", ClusterStateNormal, c.State()) } + // Ensure that node is in the cluster. + if c.nodeByID(node.ID) == nil { + return fmt.Errorf("Node is not a member of the cluster: %s", node.ID) + } + + // Prevent removing the coordinator node (this node). + if node.ID == c.Node.ID { + return fmt.Errorf("The coordinator node cannot be removed. First, make a different node the new coordinator.") + } + return c.nodeLeave(node) } diff --git a/handler.go b/handler.go index 5d7af3987..fc1771128 100644 --- a/handler.go +++ b/handler.go @@ -2042,20 +2042,13 @@ func (h *Handler) handlePostClusterResizeRemoveNode(w http.ResponseWriter, r *ht removeNode := h.Cluster.nodeByID(req.ID) if removeNode == nil { - http.Error(w, err.Error(), http.StatusBadRequest) + http.Error(w, fmt.Sprintf("Node is not a member of the cluster: %s", req.ID), http.StatusBadRequest) return } - if err := func() error { - // TODO: prevent removing the coordinator node - // Start the resize process (similar to NodeJoin) - err := h.Cluster.NodeLeave(removeNode) - if err != nil { - return err - } - - return nil - }(); err != nil { + // Start the resize process (similar to NodeJoin) + err = h.Cluster.NodeLeave(removeNode) + if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return } @@ -2208,7 +2201,6 @@ func GetTimeStamp(data map[string]interface{}, timeField string) (int64, error) func (h *Handler) handlePostClusterMessage(w http.ResponseWriter, r *http.Request) { // Verify that request is only communicating over protobufs. if r.Header.Get("Content-Type") != "application/x-protobuf" { - fmt.Println("**unsupported media type**") http.Error(w, "Unsupported media type", http.StatusUnsupportedMediaType) return } diff --git a/handler_test.go b/handler_test.go index c8030a9ea..419cbcecd 100644 --- a/handler_test.go +++ b/handler_test.go @@ -1075,9 +1075,6 @@ func TestHandler_Frame_GetFields(t *testing.T) { t.Fatal(err) } resp, err := http.Get(s.URL + "/index/i/frame/f/fields") - if err != nil { - t.Fatal(err) - } if err != nil { t.Fatal(err) } else if resp.StatusCode != http.StatusOK { diff --git a/server/cluster_test.go b/server/cluster_test.go index 23284bb00..c2af1d3c0 100644 --- a/server/cluster_test.go +++ b/server/cluster_test.go @@ -16,7 +16,10 @@ package server_test import ( "context" + "fmt" + "net/http" "reflect" + "strings" "testing" "time" @@ -435,3 +438,50 @@ func TestClusterResize_AddNode(t *testing.T) { } }) } + +func TestClusterResize_RemoveNode(t *testing.T) { + cluster := test.MustRunMainWithCluster(t, 3) + m0 := cluster[0] + m1 := cluster[1] + + t.Run("ErrorRemoveInvalidNode", func(t *testing.T) { + resp := test.MustDo("POST", m0.URL()+fmt.Sprintf("/cluster/resize/remove-node"), `{"id": "invalid-node-id"}`) + expBody := "Node is not a member of the cluster: invalid-node-id" + if resp.StatusCode != http.StatusBadRequest { + t.Fatalf("expected StatusCode %d but got %d", http.StatusBadRequest, resp.StatusCode) + } else if strings.TrimSpace(resp.Body) != expBody { + t.Fatalf("expected Body '%s' but got '%s'", expBody, strings.TrimSpace(resp.Body)) + } + }) + + t.Run("ErrorRemoveCoordinator", func(t *testing.T) { + resp := test.MustDo("GET", m0.URL()+fmt.Sprintf("/id"), "") + nodeID := resp.Body + + resp = test.MustDo("POST", m0.URL()+fmt.Sprintf("/cluster/resize/remove-node"), fmt.Sprintf(`{"id": "%s"}`, nodeID)) + + expBody := "The coordinator node cannot be removed. First, make a different node the new coordinator." + if resp.StatusCode != http.StatusInternalServerError { + t.Fatalf("expected StatusCode %d but got %d", http.StatusInternalServerError, resp.StatusCode) + } else if strings.TrimSpace(resp.Body) != expBody { + t.Fatalf("expected Body '%s' but got '%s'", expBody, strings.TrimSpace(resp.Body)) + } + }) + + t.Run("ErrorRemoveOnNonCoordinator", func(t *testing.T) { + resp := test.MustDo("GET", m0.URL()+fmt.Sprintf("/id"), "") + coordinatorNodeID := resp.Body + + resp = test.MustDo("GET", m1.URL()+fmt.Sprintf("/id"), "") + nodeID := resp.Body + + resp = test.MustDo("POST", m1.URL()+fmt.Sprintf("/cluster/resize/remove-node"), fmt.Sprintf(`{"id": "%s"}`, nodeID)) + + expBody := fmt.Sprintf("Node removal requests are only valid on the Coordinator node: %s", coordinatorNodeID) + if resp.StatusCode != http.StatusInternalServerError { + t.Fatalf("expected StatusCode %d but got %d", http.StatusInternalServerError, resp.StatusCode) + } else if strings.TrimSpace(resp.Body) != expBody { + t.Fatalf("expected Body '%s' but got '%s'", expBody, strings.TrimSpace(resp.Body)) + } + }) +}