api: make attempting to remove peer by address a no-op (#25599)

In Nomad 1.4.0 we removed support for Raft Protocol v2 entirely. But the
`Operator.RemoveRaftPeerByAddress` RPC handler was left in place, along with its
supporting HTTP API and command line flags. Using this API will always result in
the Raft library error "operation not supported with current protocol version".

Unfortunately it's still possible in unit tests to exercise this code path, and
these tests are quite flaky. This changeset turns the RPC handler and HTTP API
into a no-op, removes the associated command line flags, and removes the flaky
tests. I've also cleaned up the test for `RemoveRaftPeerByID` to consolidate
test servers and use `shoenig/test`.

Fixes: https://hashicorp.atlassian.net/browse/NET-12413
Ref: https://github.com/hashicorp/nomad/pull/13467
Ref: https://developer.hashicorp.com/nomad/docs/upgrade/upgrade-specific#raft-protocol-version-2-unsupported
Ref: https://github.com/hashicorp/nomad-enterprise/actions/runs/13201513025/job/36855234398?pr=2302

command/agent/operator_endpoint.go

@@ -68,31 +68,20 @@ func (s *HTTPServer) OperatorRaftPeer(resp http.ResponseWriter, req *http.Reques
 	_, hasID := params["id"]
 	_, hasAddress := params["address"]
 
-	if !hasID && !hasAddress {
-		return nil, CodedError(http.StatusBadRequest, "Must specify either ?id with the server's ID or ?address with IP:port of peer to remove")
+	if hasAddress {
+		return nil, CodedError(http.StatusBadRequest, "Removing a peer by address is not supported in the current raft protocol version")
 	}
-	if hasID && hasAddress {
-		return nil, CodedError(http.StatusBadRequest, "Must specify only one of ?id or ?address")
+	if !hasID {
+		return nil, CodedError(http.StatusBadRequest, "Must specify the peer's ID to remove")
 	}
 
-	if hasID {
-		var args structs.RaftPeerByIDRequest
-		s.parseWriteRequest(req, &args.WriteRequest)
+	var args structs.RaftPeerByIDRequest
+	s.parseWriteRequest(req, &args.WriteRequest)
 
-		var reply struct{}
-		args.ID = raft.ServerID(params.Get("id"))
-		if err := s.agent.RPC("Operator.RaftRemovePeerByID", &args, &reply); err != nil {
-			return nil, err
-		}
-	} else {
-		var args structs.RaftPeerByAddressRequest
-		s.parseWriteRequest(req, &args.WriteRequest)
-
-		var reply struct{}
-		args.Address = raft.ServerAddress(params.Get("address"))
-		if err := s.agent.RPC("Operator.RaftRemovePeerByAddress", &args, &reply); err != nil {
-			return nil, err
-		}
+	var reply struct{}
+	args.ID = raft.ServerID(params.Get("id"))
+	if err := s.agent.RPC("Operator.RaftRemovePeerByID", &args, &reply); err != nil {
+		return nil, err
 	}
 
 	return nil, nil

command/agent/operator_endpoint_test.go

@@ -14,7 +14,6 @@ import (
 	"os"
 	"path"
 	"path/filepath"
-	"strings"
 	"testing"
 	"time"
 
@@ -26,7 +25,6 @@ import (
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/shoenig/test/must"
 	"github.com/shoenig/test/wait"
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -61,35 +59,18 @@ func TestHTTP_OperatorRaftConfiguration(t *testing.T) {
 
 func TestHTTP_OperatorRaftPeer(t *testing.T) {
 	ci.Parallel(t)
-	assert := assert.New(t)
-	httpTest(t, nil, func(s *TestAgent) {
-		body := bytes.NewBuffer(nil)
-		req, err := http.NewRequest(http.MethodDelete, "/v1/operator/raft/peer?address=nope", body)
-		assert.Nil(err)
-
-		// If we get this error, it proves we sent the address all the
-		// way through.
-		resp := httptest.NewRecorder()
-		_, err = s.Server.OperatorRaftPeer(resp, req)
-		if err == nil || !strings.Contains(err.Error(),
-			"address \"nope\" was not found in the Raft configuration") {
-			t.Fatalf("err: %v", err)
-		}
-	})
 
 	httpTest(t, nil, func(s *TestAgent) {
 		body := bytes.NewBuffer(nil)
 		req, err := http.NewRequest(http.MethodDelete, "/v1/operator/raft/peer?id=nope", body)
-		assert.Nil(err)
+		must.NoError(t, err)
 
 		// If we get this error, it proves we sent the address all the
 		// way through.
 		resp := httptest.NewRecorder()
 		_, err = s.Server.OperatorRaftPeer(resp, req)
-		if err == nil || !strings.Contains(err.Error(),
-			"id \"nope\" was not found in the Raft configuration") {
-			t.Fatalf("err: %v", err)
-		}
+		must.ErrorContains(t, err,
+			"id \"nope\" was not found in the Raft configuration")
 	})
 }
 

command/operator_raft_remove.go

@@ -7,7 +7,6 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/hashicorp/nomad/api"
 	"github.com/posener/complete"
 )
 
@@ -19,7 +18,7 @@ func (c *OperatorRaftRemoveCommand) Help() string {
 	helpText := `
 Usage: nomad operator raft remove-peer [options]
 
-  Remove the Nomad server with given -peer-address from the Raft configuration.
+  Remove the Nomad server with given -peer-id from the Raft configuration.
 
   There are rare cases where a peer may be left behind in the Raft quorum even
   though the server is no longer present and known to the cluster. This command
@@ -36,9 +35,6 @@ General Options:
 
 Remove Peer Options:
 
-  -peer-address="IP:port"
-	Remove a Nomad server with given address from the Raft configuration.
-
   -peer-id="id"
 	Remove a Nomad server with the given ID from the Raft configuration.
 `
@@ -48,8 +44,7 @@ Remove Peer Options:
 func (c *OperatorRaftRemoveCommand) AutocompleteFlags() complete.Flags {
 	return mergeAutocompleteFlags(c.Meta.AutocompleteFlags(FlagSetClient),
 		complete.Flags{
-			"-peer-address": complete.PredictAnything,
-			"-peer-id":      complete.PredictAnything,
+			"-peer-id": complete.PredictAnything,
 		})
 }
 
@@ -64,18 +59,20 @@ func (c *OperatorRaftRemoveCommand) Synopsis() string {
 func (c *OperatorRaftRemoveCommand) Name() string { return "operator raft remove-peer" }
 
 func (c *OperatorRaftRemoveCommand) Run(args []string) int {
-	var peerAddress string
 	var peerID string
 
 	flags := c.Meta.FlagSet("raft", FlagSetClient)
 	flags.Usage = func() { c.Ui.Output(c.Help()) }
 
-	flags.StringVar(&peerAddress, "peer-address", "", "")
 	flags.StringVar(&peerID, "peer-id", "", "")
 	if err := flags.Parse(args); err != nil {
 		c.Ui.Error(fmt.Sprintf("Failed to parse args: %v", err))
 		return 1
 	}
+	if peerID == "" {
+		c.Ui.Error("Missing peer id required")
+		return 1
+	}
 
 	// Set up a client.
 	client, err := c.Meta.Client()
@@ -85,37 +82,11 @@ func (c *OperatorRaftRemoveCommand) Run(args []string) int {
 	}
 	operator := client.Operator()
 
-	if err := raftRemovePeers(peerAddress, peerID, operator); err != nil {
+	if err := operator.RaftRemovePeerByID(peerID, nil); err != nil {
 		c.Ui.Error(fmt.Sprintf("Error removing peer: %v", err))
 		return 1
 	}
-	if peerAddress != "" {
-		c.Ui.Output(fmt.Sprintf("Removed peer with address %q", peerAddress))
-	} else {
-		c.Ui.Output(fmt.Sprintf("Removed peer with id %q", peerID))
-	}
 
+	c.Ui.Output(fmt.Sprintf("Removed peer with id %q", peerID))
 	return 0
 }
-
-func raftRemovePeers(address, id string, operator *api.Operator) error {
-	if len(address) == 0 && len(id) == 0 {
-		return fmt.Errorf("an address or id is required for the peer to remove")
-	}
-	if len(address) > 0 && len(id) > 0 {
-		return fmt.Errorf("cannot give both an address and id")
-	}
-
-	// Try to kick the peer.
-	if len(address) > 0 {
-		if err := operator.RaftRemovePeerByAddress(address, nil); err != nil {
-			return err
-		}
-	} else {
-		if err := operator.RaftRemovePeerByID(id, nil); err != nil {
-			return err
-		}
-	}
-
-	return nil
-}

command/operator_raft_remove_test.go

@@ -11,66 +11,24 @@ import (
 	"github.com/shoenig/test/must"
 )
 
-func TestOperator_Raft_RemovePeers_Implements(t *testing.T) {
-	ci.Parallel(t)
-	var _ cli.Command = &OperatorRaftRemoveCommand{}
-}
-
 func TestOperator_Raft_RemovePeer(t *testing.T) {
 	ci.Parallel(t)
+	var _ cli.Command = &OperatorRaftRemoveCommand{}
 
 	s, _, addr := testServer(t, false, nil)
 	defer s.Shutdown()
 
 	ui := cli.NewMockUi()
 	c := &OperatorRaftRemoveCommand{Meta: Meta{Ui: ui}}
-	args := []string{"-address=" + addr, "-peer-address=nope", "-peer-id=nope"}
-
-	// Give both an address and ID
+	args := []string{"-address=" + addr}
 	code := c.Run(args)
-	if code != 1 {
-		t.Fatalf("bad: %d. %#v", code, ui.ErrorWriter.String())
-	}
+	must.One(t, code)
+	must.StrContains(t, ui.ErrorWriter.String(), "Missing peer id required")
 
-	must.StrContains(t, ui.ErrorWriter.String(), "cannot give both an address and id")
-
-	// Neither address nor ID present
-	args = args[:1]
+	ui.ErrorWriter.Reset()
+	args = []string{"-address=" + addr, "-peer-id=nope"}
 	code = c.Run(args)
 	must.One(t, code)
-	must.StrContains(t, ui.ErrorWriter.String(), "an address or id is required for the peer to remove")
-}
-
-func TestOperator_Raft_RemovePeerAddress(t *testing.T) {
-	ci.Parallel(t)
-
-	s, _, addr := testServer(t, false, nil)
-	defer s.Shutdown()
-
-	ui := cli.NewMockUi()
-	c := &OperatorRaftRemoveCommand{Meta: Meta{Ui: ui}}
-	args := []string{"-address=" + addr, "-peer-address=nope"}
-
-	code := c.Run(args)
-	must.One(t, code)
-
-	// If we get this error, it proves we sent the address all they through.
-	must.StrContains(t, ui.ErrorWriter.String(), "address \"nope\" was not found in the Raft configuration")
-}
-
-func TestOperator_Raft_RemovePeerID(t *testing.T) {
-	ci.Parallel(t)
-
-	s, _, addr := testServer(t, false, nil)
-	defer s.Shutdown()
-
-	ui := cli.NewMockUi()
-	c := &OperatorRaftRemoveCommand{Meta: Meta{Ui: ui}}
-	args := []string{"-address=" + addr, "-peer-id=nope"}
-
-	code := c.Run(args)
-	must.One(t, code)
-
-	// If we get this error, it proves we sent the address all they through.
+	// If we get this error, it proves we sent the peer ID all the way thru
 	must.StrContains(t, ui.ErrorWriter.String(), "id \"nope\" was not found in the Raft configuration")
 }