From b938ff5916effe84c0f63bc0fac1e74d45a1fa3d Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Wed, 10 May 2023 16:55:37 -0400 Subject: [PATCH 01/26] Add directed leadership transfer func --- nomad/leader.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/nomad/leader.go b/nomad/leader.go index 719160fa589..42010666024 100644 --- a/nomad/leader.go +++ b/nomad/leader.go @@ -149,6 +149,34 @@ func (s *Server) monitorLeadership() { } } +func (s *Server) leadershipTransferToServer(id raft.ServerID, addr raft.ServerAddress) error { + if lAddr, lID := s.raft.LeaderWithID(); id == lID && addr == lAddr { + s.logger.Debug("leadership transfer to current leader is a no-op") + return nil + } + retryCount := 3 + for i := 0; i < retryCount; i++ { + err := s.raft.LeadershipTransferToServer(id, addr).Error() + if err == nil { + s.logger.Info("successfully transferred leadership") + return nil + } + + // Don't retry if the Raft version doesn't support leadership transfer + // since this will never succeed. + if err == raft.ErrUnsupportedProtocol { + return fmt.Errorf("leadership transfer not supported with Raft version lower than 3") + } + + s.logger.Error("failed to transfer leadership attempt, will retry", + "attempt", i, + "retry_limit", retryCount, + "error", err, + ) + } + return fmt.Errorf("failed to transfer leadership in %d attempts", retryCount) +} + func (s *Server) leadershipTransfer() error { retryCount := 3 for i := 0; i < retryCount; i++ { From cc3c075b05465dec6eacc561649bf8c5d5ab8eec Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Wed, 10 May 2023 16:57:05 -0400 Subject: [PATCH 02/26] Add leadership transfer RPC endpoint --- nomad/operator_endpoint.go | 132 ++++++++++++++++++++++++++++++++++++- 1 file changed, 130 insertions(+), 2 deletions(-) diff --git a/nomad/operator_endpoint.go b/nomad/operator_endpoint.go index e6b018537e2..4395e4926e9 100644 --- a/nomad/operator_endpoint.go +++ b/nomad/operator_endpoint.go @@ -124,7 +124,7 @@ func (op *Operator) RaftRemovePeerByAddress(args *structs.RaftPeerByAddressReque // Since this is an operation designed for humans to use, we will return // an error if the supplied address isn't among the peers since it's - // likely they screwed up. + // likely a mistake. { future := op.srv.raft.GetConfiguration() if err := future.Error(); err != nil { @@ -182,7 +182,7 @@ func (op *Operator) RaftRemovePeerByID(args *structs.RaftPeerByIDRequest, reply // Since this is an operation designed for humans to use, we will return // an error if the supplied id isn't among the peers since it's - // likely they screwed up. + // likely a mistake. var address raft.ServerAddress { future := op.srv.raft.GetConfiguration() @@ -228,6 +228,134 @@ REMOVE: return nil } +// TransferLeadershipToServerAddress is used to transfer leadership away from the +// current leader to a specific target peer. This can help prevent leadership +// flapping during a rolling upgrade by allowing the cluster operator to target +// an already upgraded node before upgrading the remainder of the cluster. +func (op *Operator) TransferLeadershipToServerAddress(args *structs.RaftPeerByAddressRequest, reply *struct{}) error { + authErr := op.srv.Authenticate(op.ctx, args) + + if done, err := op.srv.forward("Operator.TransferLeadershipToServerAddress", args, args, reply); done { + return err + } + op.srv.MeasureRPCRate("operator", structs.RateMetricWrite, args) + if authErr != nil { + return structs.ErrPermissionDenied + } + + // Check management permissions + if aclObj, err := op.srv.ResolveACL(args); err != nil { + return err + } else if aclObj != nil && !aclObj.IsManagement() { + return structs.ErrPermissionDenied + } + + return op.doTransfer(args) +} + +// TransferLeadershipToServerID is used to transfer leadership away from the +// current leader to a specific target peer. This can help prevent leadership +// flapping during a rolling upgrade by allowing the cluster operator to target +// an already upgraded node before upgrading the remainder of the cluster. +func (op *Operator) TransferLeadershipToServerID(args *structs.RaftPeerByIDRequest, reply *struct{}) error { + authErr := op.srv.Authenticate(op.ctx, args) + + if done, err := op.srv.forward("Operator.TransferLeadershipToServerID", args, args, reply); done { + return err + } + op.srv.MeasureRPCRate("operator", structs.RateMetricWrite, args) + if authErr != nil { + return structs.ErrPermissionDenied + } + + // Check management permissions + if aclObj, err := op.srv.ResolveACL(args); err != nil { + return err + } else if aclObj != nil && !aclObj.IsManagement() { + return structs.ErrPermissionDenied + } + + return op.doTransfer(args) +} + +func (op *Operator) doTransfer(args any) error { + var id raft.ServerID + var addr raft.ServerAddress + var kind, testedVal string // to be used in making error strings + + if lAddr, lID := op.srv.raft.LeaderWithID(); id == lID && addr == lAddr { + op.logger.Debug("leadership transfer to current leader is a no-op") + return nil + } + + minRaftProtocol, err := op.srv.MinRaftProtocol() + if err != nil { + return err + } + + // TransferLeadership is not supported until Raft protocol v3 or greater. + if minRaftProtocol < 3 { + op.logger.Warn("unsupported minimum common raft protocol version", "required", "3", "current", minRaftProtocol) + return fmt.Errorf("unsupported minimum common raft protocol version") + } + + // This is in a scope so that future will go out of scope once we're done + // checking the configuration, enabling us to reuse the name later. + { + // Get the raft configuration + future := op.srv.raft.GetConfiguration() + if err := future.Error(); err != nil { + return err + } + + // Since this is an operation designed for humans to use, we will return + // an error if the supplied id isn't among the peers since it's + // likely a mistake. + switch tArg := args.(type) { + case *structs.RaftPeerByAddressRequest: + kind = "address" + testedVal = string(tArg.Address) + for _, s := range future.Configuration().Servers { + if s.Address == tArg.Address { + id = s.ID + addr = s.Address + goto TRANSFER + } + } + case *structs.RaftPeerByIDRequest: + kind = "id" + testedVal = string(tArg.ID) + for _, s := range future.Configuration().Servers { + if s.ID == tArg.ID { + id = s.ID + addr = s.Address + goto TRANSFER + } + } + default: + // This should never happen, since this function's callers ensure that the + // type is either a RaftPeerByAddressRequest or RaftPeerByIDRequest via + // their argument types; however, it can't hurt to be defensive for the + // future and it feels better to make an error than to panic for missing + // (but non-critical) functionality. + op.logger.Error("doTransfer: invalid argument", "type", fmt.Sprintf("%T", args)) + return fmt.Errorf("doTransfer: invalid argument type (%T)", args) + } + return fmt.Errorf("%s %q was not found in the Raft configuration", + kind, testedVal) + } + +TRANSFER: + log := op.logger.With("peer_id", id, "peer_addr", addr) + if err = op.srv.leadershipTransferToServer(id, addr); err != nil { + log.Error("failed transferring leadership", "error", err) + return err + } + + log.Info("transferred leadership") + return nil +} + // AutopilotGetConfiguration is used to retrieve the current Autopilot configuration. func (op *Operator) AutopilotGetConfiguration(args *structs.GenericRequest, reply *structs.AutopilotConfig) error { From 3e0ce48833acf7e6e11f225d4bb5d733c6e1ce76 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Wed, 10 May 2023 16:57:33 -0400 Subject: [PATCH 03/26] Add ACL tests for leadership-transfer endpoint --- nomad/operator_endpoint_test.go | 113 ++++++++++++++++++++++++++++++++ 1 file changed, 113 insertions(+) diff --git a/nomad/operator_endpoint_test.go b/nomad/operator_endpoint_test.go index fdd9a62a496..72e13aa76d2 100644 --- a/nomad/operator_endpoint_test.go +++ b/nomad/operator_endpoint_test.go @@ -27,6 +27,7 @@ import ( "github.com/hashicorp/nomad/nomad/structs" "github.com/hashicorp/nomad/testutil" "github.com/hashicorp/raft" + "github.com/shoenig/test/must" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -368,6 +369,118 @@ func TestOperator_RaftRemovePeerByID_ACL(t *testing.T) { } } +func TestOperator_TransferLeadershipToServerAddress_ACL(t *testing.T) { + ci.Parallel(t) + + s1, root, cleanupS1 := TestACLServer(t, func(c *Config) { + c.RaftConfig.ProtocolVersion = raft.ProtocolVersion(3) + }) + + defer cleanupS1() + + codec := rpcClient(t, s1) + testutil.WaitForLeader(t, s1.RPC) + state := s1.fsm.State() + + // Create ACL token + invalidToken := mock.CreatePolicyAndToken(t, state, 1001, "test-invalid", mock.NodePolicy(acl.PolicyWrite)) + + ports := ci.PortAllocator.Grab(1) + + id := raft.ServerID("e35bde83-4e9c-434f-a6ef-453f44ee21ea") + addr := raft.ServerAddress(fmt.Sprintf("127.0.0.1:%d", ports[0])) + arg := structs.RaftPeerByAddressRequest{ + Address: addr, + WriteRequest: structs.WriteRequest{Region: s1.config.Region}, + } + + // Add peer manually to Raft. + { + future := s1.raft.AddVoter(id, addr, 0, 0) + must.NoError(t, future.Error()) + } + + var reply struct{} + + t.Run("no-token", func(t *testing.T) { + // Try with no token and expect permission denied + err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToServerAddress", &arg, &reply) + must.Error(t, err) + must.Eq(t, structs.ErrPermissionDenied.Error(), err.Error()) + }) + + t.Run("invalid-token", func(t *testing.T) { + // Try with an invalid token and expect permission denied + arg.AuthToken = invalidToken.SecretID + err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToServerAddress", &arg, &reply) + must.Error(t, err) + must.Eq(t, structs.ErrPermissionDenied.Error(), err.Error()) + }) + + t.Run("good-token", func(t *testing.T) { + // Try with a management token + arg.AuthToken = root.SecretID + err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToServerAddress", &arg, &reply) + must.NoError(t, err) + }) +} + +func TestOperator_TransferLeadershipToServerID_ACL(t *testing.T) { + ci.Parallel(t) + + s1, root, cleanupS1 := TestACLServer(t, func(c *Config) { + c.RaftConfig.ProtocolVersion = raft.ProtocolVersion(3) + }) + + defer cleanupS1() + + codec := rpcClient(t, s1) + testutil.WaitForLeader(t, s1.RPC) + state := s1.fsm.State() + + // Create ACL token + invalidToken := mock.CreatePolicyAndToken(t, state, 1001, "test-invalid", mock.NodePolicy(acl.PolicyWrite)) + + ports := ci.PortAllocator.Grab(1) + + id := raft.ServerID("e35bde83-4e9c-434f-a6ef-453f44ee21ea") + addr := raft.ServerAddress(fmt.Sprintf("127.0.0.1:%d", ports[0])) + arg := structs.RaftPeerByIDRequest{ + ID: id, + WriteRequest: structs.WriteRequest{Region: s1.config.Region}, + } + + // Add peer manually to Raft. + { + future := s1.raft.AddVoter(id, addr, 0, 0) + must.NoError(t, future.Error()) + } + + var reply struct{} + + t.Run("no-token", func(t *testing.T) { + // Try with no token and expect permission denied + err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToServerAddress", &arg, &reply) + must.Error(t, err) + must.Eq(t, structs.ErrPermissionDenied.Error(), err.Error()) + }) + + t.Run("invalid-token", func(t *testing.T) { + // Try with an invalid token and expect permission denied + arg.AuthToken = invalidToken.SecretID + err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToServerAddress", &arg, &reply) + must.Error(t, err) + must.Eq(t, structs.ErrPermissionDenied.Error(), err.Error()) + }) + + t.Run("good-token", func(t *testing.T) { + // Try with a management token + arg.AuthToken = root.SecretID + err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToServerAddress", &arg, &reply) + must.NoError(t, err) + }) +} + func TestOperator_SchedulerGetConfiguration(t *testing.T) { ci.Parallel(t) From ebc49eab683a445ef21b8f73440260d4f3c39106 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Wed, 10 May 2023 16:58:48 -0400 Subject: [PATCH 04/26] Add HTTP API route and implementation --- command/agent/operator_endpoint.go | 45 ++++++++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 2 deletions(-) diff --git a/command/agent/operator_endpoint.go b/command/agent/operator_endpoint.go index 113ac56150d..ee25fb5e5f9 100644 --- a/command/agent/operator_endpoint.go +++ b/command/agent/operator_endpoint.go @@ -30,6 +30,8 @@ func (s *HTTPServer) OperatorRequest(resp http.ResponseWriter, req *http.Request return s.OperatorRaftConfiguration(resp, req) case strings.HasPrefix(path, "peer"): return s.OperatorRaftPeer(resp, req) + case strings.HasPrefix(path, "transfer-leadership"): + return s.OperatorRaftTransferLeadership(resp, req) default: return nil, CodedError(404, ErrInvalidMethod) } @@ -56,8 +58,7 @@ func (s *HTTPServer) OperatorRaftConfiguration(resp http.ResponseWriter, req *ht return reply, nil } -// OperatorRaftPeer supports actions on Raft peers. Currently we only support -// removing peers by address. +// OperatorRaftPeer supports actions on Raft peers. func (s *HTTPServer) OperatorRaftPeer(resp http.ResponseWriter, req *http.Request) (interface{}, error) { if req.Method != http.MethodDelete { return nil, CodedError(404, ErrInvalidMethod) @@ -97,6 +98,46 @@ func (s *HTTPServer) OperatorRaftPeer(resp http.ResponseWriter, req *http.Reques return nil, nil } +// OperatorRaftTransferLeadership supports actions on Raft peers. +func (s *HTTPServer) OperatorRaftTransferLeadership(resp http.ResponseWriter, req *http.Request) (interface{}, error) { + if req.Method != "PUT" && req.Method != "POST" { + return nil, CodedError(http.StatusMethodNotAllowed, ErrInvalidMethod) + } + + params := req.URL.Query() + _, hasID := params["id"] + _, hasAddress := params["address"] + + if !hasID && !hasAddress { + return nil, CodedError(http.StatusBadRequest, "Must specify either ?id with the destination server's ID or ?address with IP:port of the destination peer") + } + if hasID && hasAddress { + return nil, CodedError(http.StatusBadRequest, "Must specify only one of ?id or ?address") + } + + if hasID { + 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.TransferLeadershipToServerID", &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.TransferLeadershipToServerAddress", &args, &reply); err != nil { + return nil, err + } + } + + return nil, nil +} + // OperatorAutopilotConfiguration is used to inspect the current Autopilot configuration. // This supports the stale query mode in case the cluster doesn't have a leader. func (s *HTTPServer) OperatorAutopilotConfiguration(resp http.ResponseWriter, req *http.Request) (interface{}, error) { From 796d8fe436e0ddeea6aa914082181b7e08aa5102 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Wed, 10 May 2023 16:59:27 -0400 Subject: [PATCH 05/26] Add to Go API client --- api/operator.go | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/api/operator.go b/api/operator.go index 32faf354661..19f14ffb68c 100644 --- a/api/operator.go +++ b/api/operator.go @@ -120,6 +120,46 @@ func (op *Operator) RaftRemovePeerByID(id string, q *WriteOptions) error { return nil } +// RaftTransferLeadershipByAddress is used to transfer leadership to a +// different peer using its address in the form of "IP:port". +func (op *Operator) RaftTransferLeadershipByAddress(address string, q *WriteOptions) error { + r, err := op.c.newRequest("PUT", "/v1/operator/raft/transfer-leadership") + if err != nil { + return err + } + r.setWriteOptions(q) + + r.params.Set("address", address) + + _, resp, err := requireOK(op.c.doRequest(r)) + if err != nil { + return err + } + + resp.Body.Close() + return nil +} + +// RaftTransferLeadershipByID is used to transfer leadership to a +// different peer using its Raft ID. +func (op *Operator) RaftTransferLeadershipByID(id string, q *WriteOptions) error { + r, err := op.c.newRequest("PUT", "/v1/operator/raft/transfer-leadership") + if err != nil { + return err + } + r.setWriteOptions(q) + + r.params.Set("id", id) + + _, resp, err := requireOK(op.c.doRequest(r)) + if err != nil { + return err + } + + resp.Body.Close() + return nil +} + // SchedulerConfiguration is the config for controlling scheduler behavior type SchedulerConfiguration struct { // SchedulerAlgorithm lets you select between available scheduling algorithms. @@ -363,3 +403,12 @@ func (op *Operator) LicenseGet(q *QueryOptions) (*LicenseReply, *QueryMeta, erro return &reply, qm, nil } + +type LeadershipTransferResponse struct { + From RaftServer + To RaftServer + Noop bool + Err error + + QueryMeta +} From dec242cd20c5363bea9365950450a16773a638c9 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Wed, 10 May 2023 17:00:06 -0400 Subject: [PATCH 06/26] Implement CLI command --- command/commands.go | 5 + command/operator_raft_leadership_transfer.go | 122 +++++++++++++++++++ 2 files changed, 127 insertions(+) create mode 100644 command/operator_raft_leadership_transfer.go diff --git a/command/commands.go b/command/commands.go index f833b1847e2..d665c80e5e8 100644 --- a/command/commands.go +++ b/command/commands.go @@ -749,6 +749,11 @@ func Commands(metaPtr *Meta, agentUi cli.Ui) map[string]cli.CommandFactory { Meta: meta, }, nil }, + "operator raft transfer-leadership": func() (cli.Command, error) { + return &OperatorRaftTransferLeadershipCommand{ + Meta: meta, + }, nil + }, "operator raft info": func() (cli.Command, error) { return &OperatorRaftInfoCommand{ Meta: meta, diff --git a/command/operator_raft_leadership_transfer.go b/command/operator_raft_leadership_transfer.go new file mode 100644 index 00000000000..5b435d424c9 --- /dev/null +++ b/command/operator_raft_leadership_transfer.go @@ -0,0 +1,122 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package command + +import ( + "fmt" + "strings" + + "github.com/hashicorp/nomad/api" + "github.com/posener/complete" +) + +type OperatorRaftTransferLeadershipCommand struct { + Meta +} + +func (c *OperatorRaftTransferLeadershipCommand) Help() string { + helpText := ` +Usage: nomad operator raft transfer-leadership [options] + + Transfer leadership to the Nomad server with given -peer-address or + -peer-id in the Raft configuration. All server nodes in the cluster + must be running at least Raft protocol v3 in order to use this command. + + There are cases where you might desire transferring leadership from one + cluster member to another, for example, during a rolling upgrade. This + command allows you to designate a new server to be cluster leader. + + If ACLs are enabled, this command requires a management token. + +General Options: + + ` + generalOptionsUsage(usageOptsDefault|usageOptsNoNamespace) + ` + +Remove Peer Options: + + -peer-address="IP:port" + Transfer leadership to the Nomad server with given Raft address. + + -peer-id="id" + Transfer leadership to the Nomad server with given Raft ID. +` + return strings.TrimSpace(helpText) +} + +func (c *OperatorRaftTransferLeadershipCommand) AutocompleteFlags() complete.Flags { + return mergeAutocompleteFlags(c.Meta.AutocompleteFlags(FlagSetClient), + complete.Flags{ + "-peer-address": complete.PredictAnything, + "-peer-id": complete.PredictAnything, + }) +} + +func (c *OperatorRaftTransferLeadershipCommand) AutocompleteArgs() complete.Predictor { + return complete.PredictNothing +} + +func (c *OperatorRaftTransferLeadershipCommand) Synopsis() string { + return "Transfer leadership to a specified Nomad server" +} + +func (c *OperatorRaftTransferLeadershipCommand) Name() string { + return "operator raft transfer-leadership" +} + +func (c *OperatorRaftTransferLeadershipCommand) 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 + } + + // Set up a client. + client, err := c.Meta.Client() + if err != nil { + c.Ui.Error(fmt.Sprintf("Error initializing client: %s", err)) + return 1 + } + operator := client.Operator() + + if err := raftTransferLeadership(peerAddress, peerID, operator); err != nil { + c.Ui.Error(fmt.Sprintf("Error transferring leadership to peer: %v", err)) + return 1 + } + if peerAddress != "" { + c.Ui.Output(fmt.Sprintf("Transferred leadership to peer with address %q", peerAddress)) + } else { + c.Ui.Output(fmt.Sprintf("Transferred leadership to peer with id %q", peerID)) + } + + return 0 +} + +func raftTransferLeadership(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 destination peer") + } + 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.RaftTransferLeadershipByAddress(address, nil); err != nil { + return err + } + } else { + if err := operator.RaftTransferLeadershipByID(id, nil); err != nil { + return err + } + } + + return nil +} From a11663cbc988af1e585cc0761222e4dcd246b98a Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Thu, 1 Jun 2023 13:29:07 -0400 Subject: [PATCH 07/26] Add documentation --- website/content/api-docs/operator/raft.mdx | 148 +++++++++++++++--- .../operator/raft/transfer-leadership.mdx | 57 +++++++ website/data/docs-nav-data.json | 4 + 3 files changed, 190 insertions(+), 19 deletions(-) create mode 100644 website/content/docs/commands/operator/raft/transfer-leadership.mdx diff --git a/website/content/api-docs/operator/raft.mdx b/website/content/api-docs/operator/raft.mdx index b6313223f07..84d673bd0c0 100644 --- a/website/content/api-docs/operator/raft.mdx +++ b/website/content/api-docs/operator/raft.mdx @@ -2,7 +2,7 @@ layout: api page_title: Raft - Operator - HTTP API description: |- - The /operator/raft endpoints provide tools for management of the Raft subsystem. + The /operator/raft endpoints provide tools for management of the Raft subsystem. --- # Raft Operator HTTP API @@ -34,26 +34,56 @@ The table below shows this endpoint's support for ### Sample Request + + + +```shell-session +$ nomad operator api /v1/operator/raft/configuration +``` + + + + ```shell-session $ curl \ https://localhost:4646/v1/operator/raft/configuration ``` + + + ### Sample Response ```json { - "Index": 1, - "Servers": [ - { - "Address": "127.0.0.1:4647", - "ID": "127.0.0.1:4647", - "Leader": true, - "Node": "bacon-mac.global", - "RaftProtocol": 2, - "Voter": true - } - ] + "Index": 0, + "Servers": [ + { + "Address": "10.1.0.10:4647", + "ID": "c13f9998-a0f3-d765-0b52-55a0b3ce5f88", + "Leader": false, + "Node": "node1.global", + "RaftProtocol": "3", + "Voter": true + }, + { + "Address": "10.1.0.20:4647", + "ID": "d7927f2b-067f-45a4-6266-af8bb84de082", + "Leader": true, + "Node": "node2.global", + "RaftProtocol": "3", + "Voter": true + }, + { + "Address": "10.1.0.30:4647", + "ID": "00d56ef8-938e-abc3-6f8a-f8ac80a80fb9", + "Leader": false, + "Node": "node3.global", + "RaftProtocol": "3", + "Voter": true + } + + ] } ``` @@ -66,8 +96,8 @@ $ curl \ - `Servers` `(array: Server)` - The returned `Servers` array has information about the servers in the Raft peer configuration. - - `ID` `(string)` - The ID of the server. This is the same as the `Address` - but may be upgraded to a GUID in a future version of Nomad. + - `ID` `(string)` - The ID of the server. For Raft protocol v2, this is the + same as the `Address`. Raft protocol v3 uses GUIDs as the ID. - `Node` `(string)` - The node name of the server, as known to Nomad, or `"(unknown)"` if the node is stale and not known. @@ -100,18 +130,98 @@ The table below shows this endpoint's support for ### Parameters -- `address` `(string: )` - Specifies the server to remove as - `ip:port`. This cannot be provided along with the `id` parameter. +- `address` `(string: )` - Specifies the Raft **Address** of the + server to remove as provided in the output of `/v1/operator/raft/configuration` + API endpoint or the `nomad operator raft list-peers` command. -- `id` `(string: )` - Specifies the server to remove as - `id`. This cannot be provided along with the `address` parameter. +- `id` `(string: )` - Specifies the Raft **ID** of the server to + remove as provided in the output of `/v1/operator/raft/configuration` + API endpoint or the `nomad operator raft list-peers` command. + + + + Either `address` or `id` must be provided, but not both. + + ### Sample Request + + + + +```shell-session +$ nomad operator api -X DELETE \ + /v1/operator/raft/peer?address=1.2.3.4:4647 +``` + + + + ```shell-session $ curl \ --request DELETE \ - https://localhost:4646/v1/operator/raft/peer?address=1.2.3.4:4646 + --header "X-Nomad-Token: ${NOMAD_TOKEN}" + https://127.0.0.1:4646/v1/operator/raft/peer?address=1.2.3.4:4647 ``` + + + +## Transfer Leadership to another Raft Peer + +This endpoint tells the current cluster leader to transfer leadership +to the Nomad server with given address or ID in the Raft +configuration. The return code signifies success or failure. + +| Method | Path | Produces | +| ------------------- | --------------------------------------- | ------------------ | +| `PUT`
`POST` | `/v1/operator/raft/transfer-leadership` | `application/json` | + +The table below shows this endpoint's support for +[blocking queries](/nomad/api-docs#blocking-queries) and +[required ACLs](/nomad/api-docs#acls). + +| Blocking Queries | ACL Required | +| ---------------- | ------------ | +| `NO` | `management` | + +### Parameters + +- `address` `(string: )` - Specifies the Raft **Address** of the + target server as provided in the output of `/v1/operator/raft/configuration` + API endpoint or the `nomad operator raft list-peers` command. + +- `id` `(string: )` - Specifies the Raft **ID** of the target server + as provided in the output of `/v1/operator/raft/configuration` API endpoint or + the `nomad operator raft list-peers` command. + + + + Either `address` or `id` must be provided, but not both. + + + +### Sample Requests + + + + +```shell-session +$ nomad operator api -X PUT \ + "/v1/operator/raft/transfer-leadership?address=1.2.3.4:4647" +``` + + + + +```shell-session +$ curl --request PUT \ + --header "X-Nomad-Token: ${NOMAD_TOKEN}" + "https://127.0.0.1:4646/v1/operator/raft/transfer-leadership?address=1.2.3.4:4647" +``` + + + + [consensus protocol guide]: /nomad/docs/concepts/consensus diff --git a/website/content/docs/commands/operator/raft/transfer-leadership.mdx b/website/content/docs/commands/operator/raft/transfer-leadership.mdx new file mode 100644 index 00000000000..0520b530b9b --- /dev/null +++ b/website/content/docs/commands/operator/raft/transfer-leadership.mdx @@ -0,0 +1,57 @@ +--- +layout: docs +page_title: 'Commands: operator raft transfer-leadership' +description: | + Transfer leadership to a specific a Nomad server. +--- + +# Command: operator raft transfer-leadership + +Transfer leadership from the current leader to the given server member. + +While performing a [rolling upgrade][] of your Nomad cluster, it might be +advisable to transfer leadership to a specific node in the cluster. For example, +setting the leader to the first upgraded server in the cluster can prevent +leadership churn as you upgrade the remaining server nodes. + +The target server's ID or address:port are required and can be obtained by +running the [`nomad operator raft list-peers`][] command or by calling the +[Read Raft Configuration][] API endpoint. + +For an API to perform these operations programmatically, please see the +documentation for the [Operator][] endpoint. + +## Usage + +```plaintext +nomad operator raft transfer-leadership [options] +``` + + +If ACLs are enabled, this command requires a management token. + + +## General Options + +@include 'general_options_no_namespace.mdx' + +## Transfer Leadership Options + +- `-peer-address`: Specifies the Raft **Address** of the target server as + provided in the output of the [`nomad operator raft list-peers`][] command or + the [Read Raft Configuration] API endpoint. + +- `-peer-id`: Specifies the Raft **ID** of the target server as provided in the + output of the [`nomad operator raft list-peers`][] command or the + [Read Raft Configuration] API endpoint. + + + + Either `-peer-address` or `-peer-id` must be provided, but not both. + + + +[`nomad operator raft list-peers`]: /nomad/docs/commands/operator/raft/list-peers 'Nomad operator raft list-peers command' +[operator]: /nomad/api-docs/operator 'Nomad Operator API' +[rolling upgrade]: /nomad/docs/upgrade#upgrade-process +[Read Raft Configuration]: /nomad/api-docs/operator/raft#read-raft-configuration diff --git a/website/data/docs-nav-data.json b/website/data/docs-nav-data.json index 5a1acc2a430..25afbf5482e 100644 --- a/website/data/docs-nav-data.json +++ b/website/data/docs-nav-data.json @@ -800,6 +800,10 @@ { "title": "state", "path": "commands/operator/raft/state" + }, + { + "title": "transfer-leadership", + "path": "commands/operator/raft/transfer-leadership" } ] }, From c07f94aa9fa757ed606d4321528919820d7b3169 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Thu, 1 Jun 2023 13:44:05 -0400 Subject: [PATCH 08/26] Add changelog --- .changelog/17383.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/17383.txt diff --git a/.changelog/17383.txt b/.changelog/17383.txt new file mode 100644 index 00000000000..bfe3b594329 --- /dev/null +++ b/.changelog/17383.txt @@ -0,0 +1,3 @@ +```release-note:improvement +server: Added transfer-leadership API and CLI +``` From a93ce06d1a8b979f6c489390604b1b458baf2e92 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Wed, 23 Aug 2023 12:22:05 -0400 Subject: [PATCH 09/26] fix spacing in command output --- command/operator_raft_leadership_transfer.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/command/operator_raft_leadership_transfer.go b/command/operator_raft_leadership_transfer.go index 5b435d424c9..80281101db3 100644 --- a/command/operator_raft_leadership_transfer.go +++ b/command/operator_raft_leadership_transfer.go @@ -36,10 +36,10 @@ General Options: Remove Peer Options: -peer-address="IP:port" - Transfer leadership to the Nomad server with given Raft address. + Transfer leadership to the Nomad server with given Raft address. -peer-id="id" - Transfer leadership to the Nomad server with given Raft ID. + Transfer leadership to the Nomad server with given Raft ID. ` return strings.TrimSpace(helpText) } From e3051de0b7a9613afe3cb13e4b0114d6d0126261 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Wed, 23 Aug 2023 12:22:35 -0400 Subject: [PATCH 10/26] Add raft peer request --- nomad/structs/operator.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/nomad/structs/operator.go b/nomad/structs/operator.go index 70a3704d93f..347bf475918 100644 --- a/nomad/structs/operator.go +++ b/nomad/structs/operator.go @@ -67,6 +67,24 @@ type RaftPeerByIDRequest struct { WriteRequest } +// RaftPeerRequest is used by the Operator endpoint to apply a Raft +// operation on a specific Raft peer by its peer ID or address in the form of +// "IP:port". +type RaftPeerRequest struct { + // ID is the peer ID to remove + ID raft.ServerID + + // Address is the peer to target, in the form "IP:port". + Address raft.ServerAddress + + // WriteRequest holds the Region for this request. + WriteRequest +} + +func (r *RaftPeerRequest) IsValid() bool { + return r.ID == "" || r.Address == "" +} + // AutopilotSetConfigRequest is used by the Operator endpoint to update the // current Autopilot configuration of the cluster. type AutopilotSetConfigRequest struct { From ab1dd971a98a0d1630a2d2a9d091ce2c3780c8f2 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Wed, 23 Aug 2023 12:44:14 -0400 Subject: [PATCH 11/26] fixup operator endpoint: don't change message --- nomad/operator_endpoint.go | 128 +++++++++++++------------------------ 1 file changed, 43 insertions(+), 85 deletions(-) diff --git a/nomad/operator_endpoint.go b/nomad/operator_endpoint.go index 4395e4926e9..bc98c2d5ab5 100644 --- a/nomad/operator_endpoint.go +++ b/nomad/operator_endpoint.go @@ -228,126 +228,84 @@ REMOVE: return nil } -// TransferLeadershipToServerAddress is used to transfer leadership away from the +// TransferLeadershipToServerID is used to transfer leadership away from the // current leader to a specific target peer. This can help prevent leadership // flapping during a rolling upgrade by allowing the cluster operator to target // an already upgraded node before upgrading the remainder of the cluster. -func (op *Operator) TransferLeadershipToServerAddress(args *structs.RaftPeerByAddressRequest, reply *struct{}) error { - authErr := op.srv.Authenticate(op.ctx, args) +func (op *Operator) TransferLeadershipToPeer(req *structs.RaftPeerRequest, reply *struct{}) error { + authErr := op.srv.Authenticate(op.ctx, req) - if done, err := op.srv.forward("Operator.TransferLeadershipToServerAddress", args, args, reply); done { + if done, err := op.srv.forward("Operator.TransferLeadershipToPeer", req, req, reply); done { return err } - op.srv.MeasureRPCRate("operator", structs.RateMetricWrite, args) + op.srv.MeasureRPCRate("operator", structs.RateMetricWrite, req) if authErr != nil { return structs.ErrPermissionDenied } // Check management permissions - if aclObj, err := op.srv.ResolveACL(args); err != nil { + if aclObj, err := op.srv.ResolveACL(req); err != nil { return err } else if aclObj != nil && !aclObj.IsManagement() { return structs.ErrPermissionDenied } - return op.doTransfer(args) -} - -// TransferLeadershipToServerID is used to transfer leadership away from the -// current leader to a specific target peer. This can help prevent leadership -// flapping during a rolling upgrade by allowing the cluster operator to target -// an already upgraded node before upgrading the remainder of the cluster. -func (op *Operator) TransferLeadershipToServerID(args *structs.RaftPeerByIDRequest, reply *struct{}) error { - authErr := op.srv.Authenticate(op.ctx, args) - - if done, err := op.srv.forward("Operator.TransferLeadershipToServerID", args, args, reply); done { + // while this is a somewhat more expensive test than later ones, if this + // test fails, they will _never_ be able to do a transfer. We do this after + // ACL checks though, so as to not leak cluster info to unvalidated users. + minRaftProtocol, err := op.srv.MinRaftProtocol() + if err != nil { return err } - op.srv.MeasureRPCRate("operator", structs.RateMetricWrite, args) - if authErr != nil { - return structs.ErrPermissionDenied - } - // Check management permissions - if aclObj, err := op.srv.ResolveACL(args); err != nil { - return err - } else if aclObj != nil && !aclObj.IsManagement() { - return structs.ErrPermissionDenied + // TransferLeadership is not supported until Raft protocol v3 or greater. + if minRaftProtocol < 3 { + op.logger.Warn("unsupported minimum common raft protocol version", "required", "3", "current", minRaftProtocol) + return fmt.Errorf("unsupported minimum common raft protocol version") } - return op.doTransfer(args) -} + var lID, tgtID raft.ServerID + var lAddr, tgtAddr raft.ServerAddress + var kind, testedVal string -func (op *Operator) doTransfer(args any) error { - var id raft.ServerID - var addr raft.ServerAddress - var kind, testedVal string // to be used in making error strings + switch { + case req.ID != "": + kind, testedVal = "ID", string(req.ID) + case req.Address != "": + kind, testedVal = "address", string(req.Address) + default: + return fmt.Errorf("invalid request: must provide peer id or address") + } - if lAddr, lID := op.srv.raft.LeaderWithID(); id == lID && addr == lAddr { + if lAddr, lID := op.srv.raft.LeaderWithID(); tgtID == lID && tgtAddr == lAddr { op.logger.Debug("leadership transfer to current leader is a no-op") return nil } - minRaftProtocol, err := op.srv.MinRaftProtocol() - if err != nil { + // Get the raft configuration + future := op.srv.raft.GetConfiguration() + if err := future.Error(); err != nil { return err } - // TransferLeadership is not supported until Raft protocol v3 or greater. - if minRaftProtocol < 3 { - op.logger.Warn("unsupported minimum common raft protocol version", "required", "3", "current", minRaftProtocol) - return fmt.Errorf("unsupported minimum common raft protocol version") - } - - // This is in a scope so that future will go out of scope once we're done - // checking the configuration, enabling us to reuse the name later. - { - // Get the raft configuration - future := op.srv.raft.GetConfiguration() - if err := future.Error(); err != nil { - return err - } - - // Since this is an operation designed for humans to use, we will return - // an error if the supplied id isn't among the peers since it's - // likely a mistake. - switch tArg := args.(type) { - case *structs.RaftPeerByAddressRequest: - kind = "address" - testedVal = string(tArg.Address) - for _, s := range future.Configuration().Servers { - if s.Address == tArg.Address { - id = s.ID - addr = s.Address - goto TRANSFER - } - } - case *structs.RaftPeerByIDRequest: - kind = "id" - testedVal = string(tArg.ID) - for _, s := range future.Configuration().Servers { - if s.ID == tArg.ID { - id = s.ID - addr = s.Address - goto TRANSFER - } - } - default: - // This should never happen, since this function's callers ensure that the - // type is either a RaftPeerByAddressRequest or RaftPeerByIDRequest via - // their argument types; however, it can't hurt to be defensive for the - // future and it feels better to make an error than to panic for missing - // (but non-critical) functionality. - op.logger.Error("doTransfer: invalid argument", "type", fmt.Sprintf("%T", args)) - return fmt.Errorf("doTransfer: invalid argument type (%T)", args) + // Since this is an operation designed for humans to use, we will return + // an error if the supplied ID or address isn't among the peers since it's + // likely a mistake. + var found bool + for _, s := range future.Configuration().Servers { + if s.ID == req.ID || s.Address == req.Address { + tgtID = s.ID + tgtAddr = s.Address + found = true } + } + if !found { return fmt.Errorf("%s %q was not found in the Raft configuration", kind, testedVal) } -TRANSFER: - log := op.logger.With("peer_id", id, "peer_addr", addr) - if err = op.srv.leadershipTransferToServer(id, addr); err != nil { + log := op.logger.With("to_peer_id", tgtID, "to_peer_addr", tgtAddr, "from_peer_id", lID, "from_peer_addr", lAddr) + if err = op.srv.leadershipTransferToServer(tgtID, tgtAddr); err != nil { log.Error("failed transferring leadership", "error", err) return err } From 9b3aeb6e4afbc346d13fbe6af90da590d681f34d Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Mon, 28 Aug 2023 10:38:42 -0400 Subject: [PATCH 12/26] Make text updates to command help --- command/operator_raft_leadership_transfer.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/command/operator_raft_leadership_transfer.go b/command/operator_raft_leadership_transfer.go index 80281101db3..4e22e1b1152 100644 --- a/command/operator_raft_leadership_transfer.go +++ b/command/operator_raft_leadership_transfer.go @@ -24,8 +24,8 @@ Usage: nomad operator raft transfer-leadership [options] must be running at least Raft protocol v3 in order to use this command. There are cases where you might desire transferring leadership from one - cluster member to another, for example, during a rolling upgrade. This - command allows you to designate a new server to be cluster leader. + cluster member to another, for example, during a rolling upgrade. This + command allows you to designate a new server to be cluster leader. If ACLs are enabled, this command requires a management token. @@ -107,7 +107,7 @@ func raftTransferLeadership(address, id string, operator *api.Operator) error { return fmt.Errorf("cannot give both an address and id") } - // Try to kick the peer. + // Try to perform the leadership transfer. if len(address) > 0 { if err := operator.RaftTransferLeadershipByAddress(address, nil); err != nil { return err From 613965590b95fd94d296dd37f88cc072f5ca363e Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Mon, 28 Aug 2023 10:42:55 -0400 Subject: [PATCH 13/26] Begin adding Validation on request object --- nomad/structs/operator.go | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/nomad/structs/operator.go b/nomad/structs/operator.go index 347bf475918..fe2f359b537 100644 --- a/nomad/structs/operator.go +++ b/nomad/structs/operator.go @@ -4,9 +4,11 @@ package structs import ( + "errors" "fmt" "time" + "github.com/hashicorp/go-uuid" "github.com/hashicorp/raft" ) @@ -81,8 +83,25 @@ type RaftPeerRequest struct { WriteRequest } -func (r *RaftPeerRequest) IsValid() bool { - return r.ID == "" || r.Address == "" +func (r *RaftPeerRequest) Validate() error { + if (r.ID == "" && r.Address == "") || (r.ID != "" && r.Address == "") { + return errors.New("either ID or Address must be set") + } + if r.ID != "" { + return r.validateID() + } + return r.validateAddress() +} + +func (r *RaftPeerRequest) validateID() error { + if _, err := uuid.ParseUUID(string(r.ID)); err != nil { + return fmt.Errorf("id must be a uuid: %w", err) + } + return nil +} + +func (r *RaftPeerRequest) validateAddress() error { + return nil } // AutopilotSetConfigRequest is used by the Operator endpoint to update the From f76e663e532704903c570a9b244581ca7241365f Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Mon, 28 Aug 2023 10:45:35 -0400 Subject: [PATCH 14/26] Add RPC endpoint validations; populate response --- nomad/operator_endpoint.go | 64 +++++++++++++++++++++++++++++++------- 1 file changed, 53 insertions(+), 11 deletions(-) diff --git a/nomad/operator_endpoint.go b/nomad/operator_endpoint.go index bc98c2d5ab5..fd9caead47c 100644 --- a/nomad/operator_endpoint.go +++ b/nomad/operator_endpoint.go @@ -8,6 +8,7 @@ import ( "fmt" "io" "net" + "net/http" "time" "github.com/hashicorp/go-hclog" @@ -15,6 +16,7 @@ import ( "github.com/hashicorp/raft" "github.com/hashicorp/serf/serf" + "github.com/hashicorp/nomad/api" cstructs "github.com/hashicorp/nomad/client/structs" "github.com/hashicorp/nomad/helper/snapshot" "github.com/hashicorp/nomad/nomad/structs" @@ -232,59 +234,96 @@ REMOVE: // current leader to a specific target peer. This can help prevent leadership // flapping during a rolling upgrade by allowing the cluster operator to target // an already upgraded node before upgrading the remainder of the cluster. -func (op *Operator) TransferLeadershipToPeer(req *structs.RaftPeerRequest, reply *struct{}) error { +func (op *Operator) TransferLeadershipToPeer(req *structs.RaftPeerRequest, reply *api.LeadershipTransferResponse) error { + reply.To.Address, reply.To.ID = string(req.Address), string(req.ID) + tgtAddr, tgtID := req.Address, req.ID + authErr := op.srv.Authenticate(op.ctx, req) if done, err := op.srv.forward("Operator.TransferLeadershipToPeer", req, req, reply); done { - return err + reply.Err = err + return reply.Err } op.srv.MeasureRPCRate("operator", structs.RateMetricWrite, req) if authErr != nil { + reply.Err = structs.ErrPermissionDenied return structs.ErrPermissionDenied } - // Check management permissions + // Check ACL permissions if aclObj, err := op.srv.ResolveACL(req); err != nil { return err } else if aclObj != nil && !aclObj.IsManagement() { + reply.Err = structs.ErrPermissionDenied return structs.ErrPermissionDenied } + // Technically, this code will be running on the leader becuase of the RPC + // forwarding, but a leadership change could happen at any moment while we're + // running. We need the leader's raft info to populate the response struct + // anyway, so we have a chance to check again here + lAddr, lID := op.srv.raft.LeaderWithID() + reply.From.Address, reply.From.ID = string(lAddr), string(lID) + + // If the leader information comes back empty, that signals that there is + // currently no leader. + if lAddr == "" || lID == "" { + reply.Err = structs.ErrNoLeader + return structs.NewErrRPCCoded(http.StatusServiceUnavailable, structs.ErrNoLeader.Error()) + } + // while this is a somewhat more expensive test than later ones, if this // test fails, they will _never_ be able to do a transfer. We do this after // ACL checks though, so as to not leak cluster info to unvalidated users. minRaftProtocol, err := op.srv.MinRaftProtocol() if err != nil { + reply.Err = err return err } // TransferLeadership is not supported until Raft protocol v3 or greater. if minRaftProtocol < 3 { op.logger.Warn("unsupported minimum common raft protocol version", "required", "3", "current", minRaftProtocol) - return fmt.Errorf("unsupported minimum common raft protocol version") + reply.Err = errors.New("unsupported minimum common raft protocol version") + return structs.NewErrRPCCoded(http.StatusBadRequest, reply.Err.Error()) } - var lID, tgtID raft.ServerID - var lAddr, tgtAddr raft.ServerAddress var kind, testedVal string + // The request must provide either an ID or an Address, this lets us validate + // the request + req.Validate() switch { case req.ID != "": - kind, testedVal = "ID", string(req.ID) + kind, testedVal = "id", string(req.ID) case req.Address != "": kind, testedVal = "address", string(req.Address) default: - return fmt.Errorf("invalid request: must provide peer id or address") + reply.Err = errors.New("must provide peer id or address") + return structs.NewErrRPCCoded(http.StatusBadRequest, reply.Err.Error()) } - if lAddr, lID := op.srv.raft.LeaderWithID(); tgtID == lID && tgtAddr == lAddr { + // Fetching lAddr and lID again close to use so we can + if lAddr, lID := op.srv.raft.LeaderWithID(); lAddr == "" || lID == "" || + (tgtID == lID && tgtAddr == lAddr) { + + // If the leader info is empty, return a ErrNoLeader + if lAddr == "" || lID == "" { + reply.Err = structs.ErrNoLeader + return structs.NewErrRPCCoded(http.StatusServiceUnavailable, structs.ErrNoLeader.Error()) + } + + // Otherwise, this is a no-op, respond accordingly. + reply.From.Address, reply.From.ID = string(lAddr), string(lID) op.logger.Debug("leadership transfer to current leader is a no-op") + reply.Noop = true return nil } // Get the raft configuration future := op.srv.raft.GetConfiguration() if err := future.Error(); err != nil { + reply.Err = err return err } @@ -299,14 +338,17 @@ func (op *Operator) TransferLeadershipToPeer(req *structs.RaftPeerRequest, reply found = true } } + if !found { - return fmt.Errorf("%s %q was not found in the Raft configuration", + reply.Err = fmt.Errorf("%s %q was not found in the Raft configuration", kind, testedVal) + return structs.NewErrRPCCoded(http.StatusBadRequest, reply.Err.Error()) } log := op.logger.With("to_peer_id", tgtID, "to_peer_addr", tgtAddr, "from_peer_id", lID, "from_peer_addr", lAddr) if err = op.srv.leadershipTransferToServer(tgtID, tgtAddr); err != nil { - log.Error("failed transferring leadership", "error", err) + reply.Err = err + log.Error("failed transferring leadership", "error", reply.Err.Error()) return err } From 3f5acc5608f7fbbd4936a8fef96af5d380606f1b Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Mon, 28 Aug 2023 10:47:08 -0400 Subject: [PATCH 15/26] Update to build a cluster to test against --- nomad/operator_endpoint_test.go | 197 ++++++++++++++++++++++++-------- 1 file changed, 147 insertions(+), 50 deletions(-) diff --git a/nomad/operator_endpoint_test.go b/nomad/operator_endpoint_test.go index 72e13aa76d2..a712aa72d31 100644 --- a/nomad/operator_endpoint_test.go +++ b/nomad/operator_endpoint_test.go @@ -9,6 +9,7 @@ import ( "fmt" "io" "net" + "net/rpc" "os" "path" "reflect" @@ -32,6 +33,11 @@ import ( "github.com/stretchr/testify/require" ) +var ( + // RPC Permission Denied Errors - currently `rpc error: Permission denied` + rpcPermDeniedErr = rpc.ServerError(structs.ErrPermissionDenied.Error()) +) + func TestOperator_RaftGetConfiguration(t *testing.T) { ci.Parallel(t) @@ -369,115 +375,206 @@ func TestOperator_RaftRemovePeerByID_ACL(t *testing.T) { } } -func TestOperator_TransferLeadershipToServerAddress_ACL(t *testing.T) { - ci.Parallel(t) +type testcluster struct { + t *testing.T + server []*Server + cleanup []func() + token *structs.ACLToken + rpc func(string, any, any) error +} - s1, root, cleanupS1 := TestACLServer(t, func(c *Config) { - c.RaftConfig.ProtocolVersion = raft.ProtocolVersion(3) - }) +func (tc testcluster) Cleanup() { + for _, cFn := range tc.cleanup { + cFn() + } +} - defer cleanupS1() +type tcArgs struct { + size int + enableACL bool +} + +func newTestCluster(t *testing.T, args tcArgs) (tc testcluster) { + // handle the zero case reasonably for count + if args.size == 0 { + args.size = 3 + } + if args.size < 1 { + t.Fatal("newTestCluster must have size greater than zero") + } + cSize := args.size + out := testcluster{ + t: t, + server: make([]*Server, cSize), + cleanup: make([]func(), cSize), + } + + for i := 0; i < cSize; i += 1 { + out.server[i], out.cleanup[i] = TestServer(t, func(c *Config) { + c.NodeName = fmt.Sprintf("node-%v", i+1) + c.RaftConfig.ProtocolVersion = raft.ProtocolVersion(3) + c.BootstrapExpect = cSize + c.ACLEnabled = args.enableACL + }) + } + t.Cleanup(out.Cleanup) + out.rpc = out.server[0].RPC + + TestJoin(t, out.server...) + out.WaitForLeader() + if args.enableACL { + // Bootstrap the ACL subsystem + token := mock.ACLManagementToken() + err := out.server[0].State().BootstrapACLTokens(structs.MsgTypeTestSetup, 1, 0, token) + if err != nil { + t.Fatalf("failed to bootstrap ACL token: %v", err) + } + t.Logf("bootstrap token: %v", *token) + out.token = token + } + return out +} + +func (tc testcluster) WaitForLeader() { + testutil.WaitForLeader(tc.t, tc.rpc) +} + +func (tc testcluster) leader() *Server { + tc.WaitForLeader() + for _, s := range tc.server { + if isLeader, _ := s.getLeader(); isLeader { + return s + } + } + return nil +} + +func (tc testcluster) anyFollower() *Server { + if len(tc.server) < 2 { + return nil + } + + testutil.WaitForLeader(tc.t, tc.rpc) + for _, s := range tc.server { + if isLeader, _ := s.getLeader(); !isLeader { + return s + } + } + // something weird happened. + return nil +} + +func TestOperator_TransferLeadershipToServerAddress_ACL(t *testing.T) { + ci.Parallel(t) + + tc := newTestCluster(t, tcArgs{enableACL: true}) + s1 := tc.leader() codec := rpcClient(t, s1) - testutil.WaitForLeader(t, s1.RPC) state := s1.fsm.State() + lAddr, _ := s1.raft.LeaderWithID() + + var addr raft.ServerAddress + // Find the first non-leader server in the list. + for a := range s1.localPeers { + addr = a + if addr != lAddr { + break + } + } + // Create ACL token invalidToken := mock.CreatePolicyAndToken(t, state, 1001, "test-invalid", mock.NodePolicy(acl.PolicyWrite)) - ports := ci.PortAllocator.Grab(1) - - id := raft.ServerID("e35bde83-4e9c-434f-a6ef-453f44ee21ea") - addr := raft.ServerAddress(fmt.Sprintf("127.0.0.1:%d", ports[0])) - arg := structs.RaftPeerByAddressRequest{ + arg := structs.RaftPeerRequest{ Address: addr, WriteRequest: structs.WriteRequest{Region: s1.config.Region}, } - // Add peer manually to Raft. - { - future := s1.raft.AddVoter(id, addr, 0, 0) - must.NoError(t, future.Error()) - } - var reply struct{} t.Run("no-token", func(t *testing.T) { // Try with no token and expect permission denied - err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToServerAddress", &arg, &reply) + err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToPeer", &arg, &reply) must.Error(t, err) - must.Eq(t, structs.ErrPermissionDenied.Error(), err.Error()) + must.ErrorIs(t, err, rpcPermDeniedErr) }) t.Run("invalid-token", func(t *testing.T) { // Try with an invalid token and expect permission denied arg.AuthToken = invalidToken.SecretID - err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToServerAddress", &arg, &reply) + err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToPeer", &arg, &reply) must.Error(t, err) - must.Eq(t, structs.ErrPermissionDenied.Error(), err.Error()) + must.ErrorIs(t, err, rpcPermDeniedErr) }) t.Run("good-token", func(t *testing.T) { // Try with a management token - arg.AuthToken = root.SecretID - err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToServerAddress", &arg, &reply) + arg.AuthToken = tc.token.SecretID + err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToPeer", &arg, &reply) must.NoError(t, err) + + // Is the expected leader the new one? + tc.WaitForLeader() + lAddrNew, _ := s1.raft.LeaderWithID() + must.Eq(t, addr, lAddrNew) }) } func TestOperator_TransferLeadershipToServerID_ACL(t *testing.T) { ci.Parallel(t) - - s1, root, cleanupS1 := TestACLServer(t, func(c *Config) { - c.RaftConfig.ProtocolVersion = raft.ProtocolVersion(3) - }) - - defer cleanupS1() - + tc := newTestCluster(t, tcArgs{enableACL: true}) + s1 := tc.leader() codec := rpcClient(t, s1) - testutil.WaitForLeader(t, s1.RPC) state := s1.fsm.State() + _, ldrID := s1.raft.LeaderWithID() + + var tgtID raft.ServerID + // Find the first non-leader server in the list. + for _, sp := range s1.localPeers { + tgtID = raft.ServerID(sp.ID) + if tgtID != ldrID { + break + } + } + // Create ACL token invalidToken := mock.CreatePolicyAndToken(t, state, 1001, "test-invalid", mock.NodePolicy(acl.PolicyWrite)) - ports := ci.PortAllocator.Grab(1) - - id := raft.ServerID("e35bde83-4e9c-434f-a6ef-453f44ee21ea") - addr := raft.ServerAddress(fmt.Sprintf("127.0.0.1:%d", ports[0])) - arg := structs.RaftPeerByIDRequest{ - ID: id, + arg := structs.RaftPeerRequest{ + ID: tgtID, WriteRequest: structs.WriteRequest{Region: s1.config.Region}, } - // Add peer manually to Raft. - { - future := s1.raft.AddVoter(id, addr, 0, 0) - must.NoError(t, future.Error()) - } - var reply struct{} t.Run("no-token", func(t *testing.T) { // Try with no token and expect permission denied - err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToServerAddress", &arg, &reply) + err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToPeer", &arg, &reply) must.Error(t, err) - must.Eq(t, structs.ErrPermissionDenied.Error(), err.Error()) + must.ErrorIs(t, err, rpcPermDeniedErr) }) t.Run("invalid-token", func(t *testing.T) { // Try with an invalid token and expect permission denied arg.AuthToken = invalidToken.SecretID - err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToServerAddress", &arg, &reply) + err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToPeer", &arg, &reply) must.Error(t, err) - must.Eq(t, structs.ErrPermissionDenied.Error(), err.Error()) + must.ErrorIs(t, err, rpcPermDeniedErr) }) t.Run("good-token", func(t *testing.T) { // Try with a management token - arg.AuthToken = root.SecretID - err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToServerAddress", &arg, &reply) + arg.AuthToken = tc.token.SecretID + err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToPeer", &arg, &reply) must.NoError(t, err) + + // Is the expected leader the new one? + tc.WaitForLeader() + _, ldrID := s1.raft.LeaderWithID() + must.Eq(t, tgtID, ldrID) }) } From bb8283c6bc7a650d76cb173add0c5d4dc5ce2b9b Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Mon, 28 Aug 2023 10:47:57 -0400 Subject: [PATCH 16/26] Add validation to HTTP API --- command/agent/operator_endpoint.go | 63 +++++++++++++++++++----------- 1 file changed, 40 insertions(+), 23 deletions(-) diff --git a/command/agent/operator_endpoint.go b/command/agent/operator_endpoint.go index ee25fb5e5f9..b5d74348f28 100644 --- a/command/agent/operator_endpoint.go +++ b/command/agent/operator_endpoint.go @@ -9,6 +9,7 @@ import ( "io" "net" "net/http" + "net/netip" "strconv" "strings" "time" @@ -100,42 +101,58 @@ func (s *HTTPServer) OperatorRaftPeer(resp http.ResponseWriter, req *http.Reques // OperatorRaftTransferLeadership supports actions on Raft peers. func (s *HTTPServer) OperatorRaftTransferLeadership(resp http.ResponseWriter, req *http.Request) (interface{}, error) { - if req.Method != "PUT" && req.Method != "POST" { + if req.Method != http.MethodPost && req.Method != http.MethodPut { return nil, CodedError(http.StatusMethodNotAllowed, ErrInvalidMethod) } params := req.URL.Query() - _, hasID := params["id"] - _, hasAddress := params["address"] - if !hasID && !hasAddress { - return nil, CodedError(http.StatusBadRequest, "Must specify either ?id with the destination server's ID or ?address with IP:port of the destination peer") - } - if hasID && hasAddress { - return nil, CodedError(http.StatusBadRequest, "Must specify only one of ?id or ?address") - } + // Using the params map directly + id, hasID := params["id"] + addr, hasAddress := params["address"] + // There are some items that we can parse for here that are more unwieldy in + // the Validate() func on the RPC request object, like repeated query params. if hasID { - 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.TransferLeadershipToServerID", &args, &reply); err != nil { - return nil, err + if len(id) > 1 { + return nil, CodedError(http.StatusBadRequest, "must specify only one id") + } + if id[0] == "" { + return nil, CodedError(http.StatusBadRequest, "id must be non-empty") + } + } else if hasAddress { + if len(addr) > 1 { + return nil, CodedError(http.StatusBadRequest, "must specify only one address") + } + if addr[0] == "" { + return nil, CodedError(http.StatusBadRequest, "address must be non-empty") } } else { - var args structs.RaftPeerByAddressRequest - s.parseWriteRequest(req, &args.WriteRequest) + return nil, CodedError(http.StatusBadRequest, "must specify id or address") + } - var reply struct{} - args.Address = raft.ServerAddress(params.Get("address")) - if err := s.agent.RPC("Operator.TransferLeadershipToServerAddress", &args, &reply); err != nil { - return nil, err + var reply api.LeadershipTransferResponse + args := &structs.RaftPeerRequest{} + s.parseWriteRequest(req, &args.WriteRequest) + + if hasID { + args.ID = raft.ServerID(id[0]) + } else { + args.Address = raft.ServerAddress(addr[0]) + if args.Address == "" { + return nil, CodedError(http.StatusBadRequest, "address must be non-empty") + } + if _, err := netip.ParseAddrPort(string(args.Address)); err != nil { + return nil, CodedError(http.StatusBadRequest, fmt.Sprintf("address must be in IP:port format: %s", err.Error())) } } - return nil, nil + if err := args.Validate(); err != nil { + return nil, CodedError(http.StatusBadRequest, err.Error()) + } + + err := s.agent.RPC("Operator.TransferLeadershipToPeer", &args, &reply) + return nil, err } // OperatorAutopilotConfiguration is used to inspect the current Autopilot configuration. From 3487ac5f39d7ecc269f95a1c5bde2411ff4e410d Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Mon, 28 Aug 2023 10:48:18 -0400 Subject: [PATCH 17/26] Add test for OperatorTransfer HTTP endpoint --- command/agent/operator_endpoint_test.go | 140 ++++++++++++++++++++++++ 1 file changed, 140 insertions(+) diff --git a/command/agent/operator_endpoint_test.go b/command/agent/operator_endpoint_test.go index 12535366c57..2d5590ffd06 100644 --- a/command/agent/operator_endpoint_test.go +++ b/command/agent/operator_endpoint_test.go @@ -20,6 +20,8 @@ import ( "github.com/hashicorp/nomad/api" "github.com/hashicorp/nomad/ci" + "github.com/hashicorp/nomad/helper/pointer" + "github.com/hashicorp/nomad/helper/uuid" "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" "github.com/shoenig/test/must" @@ -91,6 +93,144 @@ func TestHTTP_OperatorRaftPeer(t *testing.T) { }) } +func TestHTTP_OperatorRaftTransferLeadership(t *testing.T) { + ci.Parallel(t) + configCB := func(c *Config) { + c.Client.Enabled = false + c.Server.NumSchedulers = pointer.Of(0) + } + + httpTest(t, configCB, func(s *TestAgent) { + body := bytes.NewBuffer(nil) + badMethods := []string{ + http.MethodConnect, + http.MethodDelete, + http.MethodGet, + http.MethodHead, + http.MethodOptions, + http.MethodPatch, + http.MethodTrace, + } + for _, tc := range badMethods { + tc := tc + t.Run(tc+" method errors", func(t *testing.T) { + req, err := http.NewRequest(tc, "/v1/operator/raft/transfer-leadership?address=nope", body) + must.NoError(t, err) + + resp := httptest.NewRecorder() + _, err = s.Server.OperatorRaftTransferLeadership(resp, req) + + must.Error(t, err) + must.ErrorContains(t, err, "Invalid method") + body.Reset() + }) + } + + apiErrTCs := []struct { + name string + qs string + expected string + }{ + { + name: "URL with id and address errors", + qs: `?id=foo&address=bar`, + expected: "must specify either id or address", + }, + { + name: "URL without id and address errors", + qs: ``, + expected: "must specify id or address", + }, + { + name: "URL with multiple id errors", + qs: `?id=foo&id=bar`, + expected: "must specify only one id", + }, + { + name: "URL with multiple address errors", + qs: `?address=foo&address=bar`, + expected: "must specify only one address", + }, + { + name: "URL with an empty id errors", + qs: `?id`, + expected: "id must be non-empty", + }, + { + name: "URL with an empty address errors", + qs: `?address`, + expected: "address must be non-empty", + }, + { + name: "an invalid id errors", + qs: `?id=foo`, + expected: "id must be a uuid", + }, + { + name: "URL with an empty address errors", + qs: `?address=bar`, + expected: "address must be in IP:port format", + }, + } + for _, tc := range apiErrTCs { + tc := tc + t.Run(tc.name, func(t *testing.T) { + req, err := http.NewRequest( + http.MethodPut, + "/v1/operator/raft/transfer-leadership"+tc.qs, + body, + ) + must.NoError(t, err) + + resp := httptest.NewRecorder() + _, err = s.Server.OperatorRaftTransferLeadership(resp, req) + + must.Error(t, err) + must.ErrorContains(t, err, tc.expected) + body.Reset() + }) + } + }) + + testID := uuid.Generate() + apiOkTCs := []struct { + name string + qs string + expected string + }{ + { + "id", + "?id=" + testID, + `id "` + testID + `" was not found in the Raft configuration`, + }, + { + "address", + "?address=9.9.9.9:8000", + `address "9.9.9.9:8000" was not found in the Raft configuration`, + }, + } + for _, tc := range apiOkTCs { + tc := tc + t.Run(tc.name+" can roundtrip", func(t *testing.T) { + httpTest(t, configCB, func(s *TestAgent) { + body := bytes.NewBuffer(nil) + req, err := http.NewRequest( + http.MethodPut, + "/v1/operator/raft/transfer-leadership"+tc.qs, + body, + ) + must.NoError(t, err) + + // If we get this error, it proves we sent the parameter all the + // way through. + resp := httptest.NewRecorder() + _, err = s.Server.OperatorRaftTransferLeadership(resp, req) + must.ErrorContains(t, err, tc.expected) + }) + }) + } +} + func TestOperator_AutopilotGetConfiguration(t *testing.T) { ci.Parallel(t) httpTest(t, nil, func(s *TestAgent) { From 3bbbe814c55b3ddb2f612facf83c0cc7c0f45659 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Fri, 1 Sep 2023 11:58:18 -0400 Subject: [PATCH 18/26] typo fixes --- nomad/operator_endpoint.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/nomad/operator_endpoint.go b/nomad/operator_endpoint.go index fd9caead47c..1dc4431703c 100644 --- a/nomad/operator_endpoint.go +++ b/nomad/operator_endpoint.go @@ -258,7 +258,7 @@ func (op *Operator) TransferLeadershipToPeer(req *structs.RaftPeerRequest, reply return structs.ErrPermissionDenied } - // Technically, this code will be running on the leader becuase of the RPC + // Technically, this code will be running on the leader because of the RPC // forwarding, but a leadership change could happen at any moment while we're // running. We need the leader's raft info to populate the response struct // anyway, so we have a chance to check again here @@ -274,7 +274,7 @@ func (op *Operator) TransferLeadershipToPeer(req *structs.RaftPeerRequest, reply // while this is a somewhat more expensive test than later ones, if this // test fails, they will _never_ be able to do a transfer. We do this after - // ACL checks though, so as to not leak cluster info to unvalidated users. + // ACL checks though, so as to not leak cluster info to non-validated users. minRaftProtocol, err := op.srv.MinRaftProtocol() if err != nil { reply.Err = err @@ -412,7 +412,7 @@ func (op *Operator) AutopilotSetConfiguration(args *structs.AutopilotSetConfigRe return structs.ErrPermissionDenied } - // All servers should be at or above 0.8.0 to apply this operatation + // All servers should be at or above 0.8.0 to apply this operation if !ServersMeetMinimumVersion(op.srv.Members(), op.srv.Region(), minAutopilotVersion, false) { return fmt.Errorf("All servers should be running version %v to update autopilot config", minAutopilotVersion) } From 56b2ced4c7d4e6b51f0d27ec4b1b9a39839072fc Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Fri, 1 Sep 2023 13:39:58 -0400 Subject: [PATCH 19/26] fix error in Validate condition --- nomad/structs/operator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nomad/structs/operator.go b/nomad/structs/operator.go index fe2f359b537..7f454395932 100644 --- a/nomad/structs/operator.go +++ b/nomad/structs/operator.go @@ -84,7 +84,7 @@ type RaftPeerRequest struct { } func (r *RaftPeerRequest) Validate() error { - if (r.ID == "" && r.Address == "") || (r.ID != "" && r.Address == "") { + if (r.ID == "" && r.Address == "") || (r.ID != "" && r.Address != "") { return errors.New("either ID or Address must be set") } if r.ID != "" { From e1a354e99f9b7bd9fa9688a4fa110038cf6a44f6 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Fri, 1 Sep 2023 13:49:14 -0400 Subject: [PATCH 20/26] move address validation to the struct --- command/agent/operator_endpoint.go | 7 ------- nomad/structs/operator.go | 4 ++++ 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/command/agent/operator_endpoint.go b/command/agent/operator_endpoint.go index b5d74348f28..ed0f9edd17a 100644 --- a/command/agent/operator_endpoint.go +++ b/command/agent/operator_endpoint.go @@ -9,7 +9,6 @@ import ( "io" "net" "net/http" - "net/netip" "strconv" "strings" "time" @@ -139,12 +138,6 @@ func (s *HTTPServer) OperatorRaftTransferLeadership(resp http.ResponseWriter, re args.ID = raft.ServerID(id[0]) } else { args.Address = raft.ServerAddress(addr[0]) - if args.Address == "" { - return nil, CodedError(http.StatusBadRequest, "address must be non-empty") - } - if _, err := netip.ParseAddrPort(string(args.Address)); err != nil { - return nil, CodedError(http.StatusBadRequest, fmt.Sprintf("address must be in IP:port format: %s", err.Error())) - } } if err := args.Validate(); err != nil { diff --git a/nomad/structs/operator.go b/nomad/structs/operator.go index 7f454395932..6b81d4319c3 100644 --- a/nomad/structs/operator.go +++ b/nomad/structs/operator.go @@ -6,6 +6,7 @@ package structs import ( "errors" "fmt" + "net/netip" "time" "github.com/hashicorp/go-uuid" @@ -101,6 +102,9 @@ func (r *RaftPeerRequest) validateID() error { } func (r *RaftPeerRequest) validateAddress() error { + if _, err := netip.ParseAddrPort(string(r.Address)); err != nil { + return fmt.Errorf("address must be in IP:port format: %w", err) + } return nil } From 7f34aae03457e9f1222f15705bda00afd0623749 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Fri, 1 Sep 2023 13:51:55 -0400 Subject: [PATCH 21/26] refactor `if` to `switch` I think this makes the validations a little easier to parse as a reader and eliminates nesting --- command/agent/operator_endpoint.go | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/command/agent/operator_endpoint.go b/command/agent/operator_endpoint.go index ed0f9edd17a..365c25b778c 100644 --- a/command/agent/operator_endpoint.go +++ b/command/agent/operator_endpoint.go @@ -112,22 +112,19 @@ func (s *HTTPServer) OperatorRaftTransferLeadership(resp http.ResponseWriter, re // There are some items that we can parse for here that are more unwieldy in // the Validate() func on the RPC request object, like repeated query params. - if hasID { - if len(id) > 1 { - return nil, CodedError(http.StatusBadRequest, "must specify only one id") - } - if id[0] == "" { - return nil, CodedError(http.StatusBadRequest, "id must be non-empty") - } - } else if hasAddress { - if len(addr) > 1 { - return nil, CodedError(http.StatusBadRequest, "must specify only one address") - } - if addr[0] == "" { - return nil, CodedError(http.StatusBadRequest, "address must be non-empty") - } - } else { + switch { + case !hasID && !hasAddress: return nil, CodedError(http.StatusBadRequest, "must specify id or address") + case hasID && hasAddress: + return nil, CodedError(http.StatusBadRequest, "must specify either id or address") + case hasID && id[0] == "": + return nil, CodedError(http.StatusBadRequest, "id must be non-empty") + case hasID && len(id) > 1: + return nil, CodedError(http.StatusBadRequest, "must specify only one id") + case hasAddress && addr[0] == "": + return nil, CodedError(http.StatusBadRequest, "address must be non-empty") + case hasAddress && len(addr) > 1: + return nil, CodedError(http.StatusBadRequest, "must specify only one address") } var reply api.LeadershipTransferResponse From b8cf935bfca0f572eaedec61f0f3564ad924a4a3 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Mon, 18 Sep 2023 21:39:18 -0400 Subject: [PATCH 22/26] Code review feedback --- api/operator.go | 2 +- command/operator_raft_leadership_transfer.go | 5 ++++- nomad/operator_endpoint.go | 9 ++++----- nomad/structs/operator.go | 13 +++++++++++++ website/content/api-docs/operator/raft.mdx | 8 +++++--- 5 files changed, 27 insertions(+), 10 deletions(-) diff --git a/api/operator.go b/api/operator.go index 19f14ffb68c..507ea5cad2d 100644 --- a/api/operator.go +++ b/api/operator.go @@ -410,5 +410,5 @@ type LeadershipTransferResponse struct { Noop bool Err error - QueryMeta + WriteMeta } diff --git a/command/operator_raft_leadership_transfer.go b/command/operator_raft_leadership_transfer.go index 4e22e1b1152..121c91574de 100644 --- a/command/operator_raft_leadership_transfer.go +++ b/command/operator_raft_leadership_transfer.go @@ -21,12 +21,14 @@ Usage: nomad operator raft transfer-leadership [options] Transfer leadership to the Nomad server with given -peer-address or -peer-id in the Raft configuration. All server nodes in the cluster - must be running at least Raft protocol v3 in order to use this command. + must be running at least Raft protocol v3 in order to use this command. There are cases where you might desire transferring leadership from one cluster member to another, for example, during a rolling upgrade. This command allows you to designate a new server to be cluster leader. + Note: This command requires a currently established leader to function. + If ACLs are enabled, this command requires a management token. General Options: @@ -41,6 +43,7 @@ Remove Peer Options: -peer-id="id" Transfer leadership to the Nomad server with given Raft ID. ` + return strings.TrimSpace(helpText) } diff --git a/nomad/operator_endpoint.go b/nomad/operator_endpoint.go index 1dc4431703c..fe5d72e22fb 100644 --- a/nomad/operator_endpoint.go +++ b/nomad/operator_endpoint.go @@ -16,7 +16,6 @@ import ( "github.com/hashicorp/raft" "github.com/hashicorp/serf/serf" - "github.com/hashicorp/nomad/api" cstructs "github.com/hashicorp/nomad/client/structs" "github.com/hashicorp/nomad/helper/snapshot" "github.com/hashicorp/nomad/nomad/structs" @@ -234,8 +233,8 @@ REMOVE: // current leader to a specific target peer. This can help prevent leadership // flapping during a rolling upgrade by allowing the cluster operator to target // an already upgraded node before upgrading the remainder of the cluster. -func (op *Operator) TransferLeadershipToPeer(req *structs.RaftPeerRequest, reply *api.LeadershipTransferResponse) error { - reply.To.Address, reply.To.ID = string(req.Address), string(req.ID) +func (op *Operator) TransferLeadershipToPeer(req *structs.RaftPeerRequest, reply *structs.LeadershipTransferResponse) error { + reply.To.Address, reply.To.ID = raft.ServerAddress(req.Address), raft.ServerID(req.ID) tgtAddr, tgtID := req.Address, req.ID authErr := op.srv.Authenticate(op.ctx, req) @@ -263,7 +262,7 @@ func (op *Operator) TransferLeadershipToPeer(req *structs.RaftPeerRequest, reply // running. We need the leader's raft info to populate the response struct // anyway, so we have a chance to check again here lAddr, lID := op.srv.raft.LeaderWithID() - reply.From.Address, reply.From.ID = string(lAddr), string(lID) + reply.From.Address, reply.From.ID = lAddr, lID // If the leader information comes back empty, that signals that there is // currently no leader. @@ -314,7 +313,7 @@ func (op *Operator) TransferLeadershipToPeer(req *structs.RaftPeerRequest, reply } // Otherwise, this is a no-op, respond accordingly. - reply.From.Address, reply.From.ID = string(lAddr), string(lID) + reply.From.Address, reply.From.ID = lAddr, lID op.logger.Debug("leadership transfer to current leader is a no-op") reply.Noop = true return nil diff --git a/nomad/structs/operator.go b/nomad/structs/operator.go index 6b81d4319c3..f3d4d459096 100644 --- a/nomad/structs/operator.go +++ b/nomad/structs/operator.go @@ -52,6 +52,8 @@ type RaftConfigurationResponse struct { // RaftPeerByAddressRequest is used by the Operator endpoint to apply a Raft // operation on a specific Raft peer by address in the form of "IP:port". +// +// Deprecated: Use RaftPeerRequest with an Address instead. type RaftPeerByAddressRequest struct { // Address is the peer to remove, in the form "IP:port". Address raft.ServerAddress @@ -62,6 +64,8 @@ type RaftPeerByAddressRequest struct { // RaftPeerByIDRequest is used by the Operator endpoint to apply a Raft // operation on a specific Raft peer by ID. +// +// Deprecated: Use RaftPeerRequest with an ID instead. type RaftPeerByIDRequest struct { // ID is the peer ID to remove. ID raft.ServerID @@ -108,6 +112,15 @@ func (r *RaftPeerRequest) validateAddress() error { return nil } +type LeadershipTransferResponse struct { + From RaftServer // Server yielding leadership + To RaftServer // Server obtaining leadership + Noop bool // Was the transfer a non-operation + Err error // Non-nil if there was an error while transferring leadership + + WriteMeta +} + // AutopilotSetConfigRequest is used by the Operator endpoint to update the // current Autopilot configuration of the cluster. type AutopilotSetConfigRequest struct { diff --git a/website/content/api-docs/operator/raft.mdx b/website/content/api-docs/operator/raft.mdx index 84d673bd0c0..fb77f6d8e60 100644 --- a/website/content/api-docs/operator/raft.mdx +++ b/website/content/api-docs/operator/raft.mdx @@ -196,11 +196,13 @@ The table below shows this endpoint's support for as provided in the output of `/v1/operator/raft/configuration` API endpoint or the `nomad operator raft list-peers` command. - + - Either `address` or `id` must be provided, but not both. +- The cluster must be running Raft protocol v3 or greater on all server members. - +- Either `address` or `id` must be provided, but not both. + + ### Sample Requests From 96cc82ab2364c44f68097365385548fa468a5993 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Tue, 19 Sep 2023 14:34:12 -0400 Subject: [PATCH 23/26] fix returned object --- command/agent/operator_endpoint.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/command/agent/operator_endpoint.go b/command/agent/operator_endpoint.go index 365c25b778c..e20e3b225b5 100644 --- a/command/agent/operator_endpoint.go +++ b/command/agent/operator_endpoint.go @@ -127,7 +127,7 @@ func (s *HTTPServer) OperatorRaftTransferLeadership(resp http.ResponseWriter, re return nil, CodedError(http.StatusBadRequest, "must specify only one address") } - var reply api.LeadershipTransferResponse + var out structs.LeadershipTransferResponse args := &structs.RaftPeerRequest{} s.parseWriteRequest(req, &args.WriteRequest) @@ -141,8 +141,13 @@ func (s *HTTPServer) OperatorRaftTransferLeadership(resp http.ResponseWriter, re return nil, CodedError(http.StatusBadRequest, err.Error()) } - err := s.agent.RPC("Operator.TransferLeadershipToPeer", &args, &reply) - return nil, err + err := s.agent.RPC("Operator.TransferLeadershipToPeer", &args, &out) + if err != nil { + return nil, err + } + + setIndex(resp, out.Index) + return out, nil } // OperatorAutopilotConfiguration is used to inspect the current Autopilot configuration. From a557c7dbdcea3bb6d708a552738159e8efbd9f22 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Tue, 3 Oct 2023 16:31:19 -0400 Subject: [PATCH 24/26] Make RaftIDAddress type --- nomad/leader.go | 28 +++++++++++++++---- nomad/operator_endpoint.go | 49 +++++++++++++++------------------ nomad/operator_endpoint_test.go | 8 ++++-- nomad/structs/operator.go | 27 ++++++++++-------- 4 files changed, 65 insertions(+), 47 deletions(-) diff --git a/nomad/leader.go b/nomad/leader.go index 42010666024..8d43d4ea9b2 100644 --- a/nomad/leader.go +++ b/nomad/leader.go @@ -149,32 +149,48 @@ func (s *Server) monitorLeadership() { } } -func (s *Server) leadershipTransferToServer(id raft.ServerID, addr raft.ServerAddress) error { - if lAddr, lID := s.raft.LeaderWithID(); id == lID && addr == lAddr { +func (s *Server) leadershipTransferToServer(to structs.RaftIDAddress) error { + if l := structs.NewRaftIDAddress(s.raft.LeaderWithID()); l == to { s.logger.Debug("leadership transfer to current leader is a no-op") return nil } retryCount := 3 + var lastError error for i := 0; i < retryCount; i++ { - err := s.raft.LeadershipTransferToServer(id, addr).Error() + err := s.raft.LeadershipTransferToServer(to.ID, to.Address).Error() if err == nil { s.logger.Info("successfully transferred leadership") return nil } - // Don't retry if the Raft version doesn't support leadership transfer - // since this will never succeed. + // "cannot transfer leadership to itself" + // Handled at top of function, but reapplied here to prevent retrying if + // it occurs while we are retrying + if err.Error() == "cannot transfer leadership to itself" { + s.logger.Debug("leadership transfer to current leader is a no-op") + return nil + } + + // ErrRaftShutdown: Don't retry if raft is shut down. + if err == raft.ErrRaftShutdown { + return err + } + + // ErrUnsupportedProtocol: Don't retry if the Raft version doesn't + // support leadership transfer since this will never succeed. if err == raft.ErrUnsupportedProtocol { return fmt.Errorf("leadership transfer not supported with Raft version lower than 3") } + // ErrEnqueueTimeout: This seems to be the valid time to retry. s.logger.Error("failed to transfer leadership attempt, will retry", "attempt", i, "retry_limit", retryCount, "error", err, ) + lastError = err } - return fmt.Errorf("failed to transfer leadership in %d attempts", retryCount) + return fmt.Errorf("failed to transfer leadership in %d attempts. last error: %w", retryCount, lastError) } func (s *Server) leadershipTransfer() error { diff --git a/nomad/operator_endpoint.go b/nomad/operator_endpoint.go index fe5d72e22fb..05c2855bfd7 100644 --- a/nomad/operator_endpoint.go +++ b/nomad/operator_endpoint.go @@ -234,8 +234,10 @@ REMOVE: // flapping during a rolling upgrade by allowing the cluster operator to target // an already upgraded node before upgrading the remainder of the cluster. func (op *Operator) TransferLeadershipToPeer(req *structs.RaftPeerRequest, reply *structs.LeadershipTransferResponse) error { - reply.To.Address, reply.To.ID = raft.ServerAddress(req.Address), raft.ServerID(req.ID) - tgtAddr, tgtID := req.Address, req.ID + // Populate the reply's `To` with the arguments. Only one of them is likely + // to be filled. We don't get any additional information until after auth + // to prevent leaking cluster details vis the error response. + reply.To = structs.NewRaftIDAddress(req.Address, req.ID) authErr := op.srv.Authenticate(op.ctx, req) @@ -261,12 +263,12 @@ func (op *Operator) TransferLeadershipToPeer(req *structs.RaftPeerRequest, reply // forwarding, but a leadership change could happen at any moment while we're // running. We need the leader's raft info to populate the response struct // anyway, so we have a chance to check again here - lAddr, lID := op.srv.raft.LeaderWithID() - reply.From.Address, reply.From.ID = lAddr, lID + + reply.From = structs.NewRaftIDAddress(op.srv.raft.LeaderWithID()) // If the leader information comes back empty, that signals that there is // currently no leader. - if lAddr == "" || lID == "" { + if reply.From.Address == "" || reply.From.ID == "" { reply.Err = structs.ErrNoLeader return structs.NewErrRPCCoded(http.StatusServiceUnavailable, structs.ErrNoLeader.Error()) } @@ -277,7 +279,7 @@ func (op *Operator) TransferLeadershipToPeer(req *structs.RaftPeerRequest, reply minRaftProtocol, err := op.srv.MinRaftProtocol() if err != nil { reply.Err = err - return err + return structs.NewErrRPCCoded(http.StatusInternalServerError, err.Error()) } // TransferLeadership is not supported until Raft protocol v3 or greater. @@ -302,23 +304,6 @@ func (op *Operator) TransferLeadershipToPeer(req *structs.RaftPeerRequest, reply return structs.NewErrRPCCoded(http.StatusBadRequest, reply.Err.Error()) } - // Fetching lAddr and lID again close to use so we can - if lAddr, lID := op.srv.raft.LeaderWithID(); lAddr == "" || lID == "" || - (tgtID == lID && tgtAddr == lAddr) { - - // If the leader info is empty, return a ErrNoLeader - if lAddr == "" || lID == "" { - reply.Err = structs.ErrNoLeader - return structs.NewErrRPCCoded(http.StatusServiceUnavailable, structs.ErrNoLeader.Error()) - } - - // Otherwise, this is a no-op, respond accordingly. - reply.From.Address, reply.From.ID = lAddr, lID - op.logger.Debug("leadership transfer to current leader is a no-op") - reply.Noop = true - return nil - } - // Get the raft configuration future := op.srv.raft.GetConfiguration() if err := future.Error(); err != nil { @@ -332,9 +317,9 @@ func (op *Operator) TransferLeadershipToPeer(req *structs.RaftPeerRequest, reply var found bool for _, s := range future.Configuration().Servers { if s.ID == req.ID || s.Address == req.Address { - tgtID = s.ID - tgtAddr = s.Address + reply.To = structs.NewRaftIDAddress(s.Address, s.ID) found = true + break } } @@ -344,8 +329,18 @@ func (op *Operator) TransferLeadershipToPeer(req *structs.RaftPeerRequest, reply return structs.NewErrRPCCoded(http.StatusBadRequest, reply.Err.Error()) } - log := op.logger.With("to_peer_id", tgtID, "to_peer_addr", tgtAddr, "from_peer_id", lID, "from_peer_addr", lAddr) - if err = op.srv.leadershipTransferToServer(tgtID, tgtAddr); err != nil { + // Otherwise, this is a no-op, respond accordingly. + if reply.From == reply.To { + op.logger.Debug("leadership transfer to current leader is a no-op") + reply.Noop = true + return nil + } + + log := op.logger.With( + "to_peer_id", reply.To.ID, "to_peer_addr", reply.To.Address, + "from_peer_id", reply.From.ID, "from_peer_addr", reply.From.Address, + ) + if err = op.srv.leadershipTransferToServer(reply.To); err != nil { reply.Err = err log.Error("failed transferring leadership", "error", reply.Err.Error()) return err diff --git a/nomad/operator_endpoint_test.go b/nomad/operator_endpoint_test.go index a712aa72d31..6899a1d0b48 100644 --- a/nomad/operator_endpoint_test.go +++ b/nomad/operator_endpoint_test.go @@ -488,8 +488,8 @@ func TestOperator_TransferLeadershipToServerAddress_ACL(t *testing.T) { invalidToken := mock.CreatePolicyAndToken(t, state, 1001, "test-invalid", mock.NodePolicy(acl.PolicyWrite)) arg := structs.RaftPeerRequest{ - Address: addr, - WriteRequest: structs.WriteRequest{Region: s1.config.Region}, + RaftIDAddress: structs.RaftIDAddress{Address: addr}, + WriteRequest: structs.WriteRequest{Region: s1.config.Region}, } var reply struct{} @@ -544,7 +544,9 @@ func TestOperator_TransferLeadershipToServerID_ACL(t *testing.T) { invalidToken := mock.CreatePolicyAndToken(t, state, 1001, "test-invalid", mock.NodePolicy(acl.PolicyWrite)) arg := structs.RaftPeerRequest{ - ID: tgtID, + RaftIDAddress: structs.RaftIDAddress{ + ID: tgtID, + }, WriteRequest: structs.WriteRequest{Region: s1.config.Region}, } diff --git a/nomad/structs/operator.go b/nomad/structs/operator.go index f3d4d459096..c68bd668d90 100644 --- a/nomad/structs/operator.go +++ b/nomad/structs/operator.go @@ -78,12 +78,8 @@ type RaftPeerByIDRequest struct { // operation on a specific Raft peer by its peer ID or address in the form of // "IP:port". type RaftPeerRequest struct { - // ID is the peer ID to remove - ID raft.ServerID - - // Address is the peer to target, in the form "IP:port". - Address raft.ServerAddress - + // RaftIDAddress contains an ID and Address field to identify the target + RaftIDAddress // WriteRequest holds the Region for this request. WriteRequest } @@ -113,12 +109,21 @@ func (r *RaftPeerRequest) validateAddress() error { } type LeadershipTransferResponse struct { - From RaftServer // Server yielding leadership - To RaftServer // Server obtaining leadership - Noop bool // Was the transfer a non-operation - Err error // Non-nil if there was an error while transferring leadership + From RaftIDAddress // Server yielding leadership + To RaftIDAddress // Server obtaining leadership + Noop bool // Was the transfer a non-operation + Err error // Non-nil if there was an error while transferring leadership +} - WriteMeta +type RaftIDAddress struct { + Address raft.ServerAddress + ID raft.ServerID +} + +// NewRaftIDAddress takes parameters in the order provided by raft's +// LeaderWithID func and returns a RaftIDAddress +func NewRaftIDAddress(a raft.ServerAddress, id raft.ServerID) RaftIDAddress { + return RaftIDAddress{ID: id, Address: a} } // AutopilotSetConfigRequest is used by the Operator endpoint to update the From 9e27072e62aed2b9c606a957f7698b050f3ba728 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Tue, 3 Oct 2023 16:55:27 -0400 Subject: [PATCH 25/26] Remove call to setIndex --- command/agent/operator_endpoint.go | 1 - 1 file changed, 1 deletion(-) diff --git a/command/agent/operator_endpoint.go b/command/agent/operator_endpoint.go index e20e3b225b5..c79641e02eb 100644 --- a/command/agent/operator_endpoint.go +++ b/command/agent/operator_endpoint.go @@ -146,7 +146,6 @@ func (s *HTTPServer) OperatorRaftTransferLeadership(resp http.ResponseWriter, re return nil, err } - setIndex(resp, out.Index) return out, nil } From e46d1e49ba4722e8a031334d11a5cf62cb268d93 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Wed, 4 Oct 2023 09:39:12 -0400 Subject: [PATCH 26/26] Apply suggestions from code review Co-authored-by: Tim Gross --- nomad/operator_endpoint.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nomad/operator_endpoint.go b/nomad/operator_endpoint.go index 05c2855bfd7..dd20c084636 100644 --- a/nomad/operator_endpoint.go +++ b/nomad/operator_endpoint.go @@ -229,14 +229,14 @@ REMOVE: return nil } -// TransferLeadershipToServerID is used to transfer leadership away from the +// TransferLeadershipToPeer is used to transfer leadership away from the // current leader to a specific target peer. This can help prevent leadership // flapping during a rolling upgrade by allowing the cluster operator to target // an already upgraded node before upgrading the remainder of the cluster. func (op *Operator) TransferLeadershipToPeer(req *structs.RaftPeerRequest, reply *structs.LeadershipTransferResponse) error { // Populate the reply's `To` with the arguments. Only one of them is likely // to be filled. We don't get any additional information until after auth - // to prevent leaking cluster details vis the error response. + // to prevent leaking cluster details via the error response. reply.To = structs.NewRaftIDAddress(req.Address, req.ID) authErr := op.srv.Authenticate(op.ctx, req)