client: don't use Status RPC for Consul discovery (#16490)

[tgross]

Fixes #16470

In #16217 we switched clients using Consul discovery to the Status.Members
endpoint for getting the list of servers so that we're using the correct
address. This endpoint has an authorization gate, so this fails if the anonymous
policy doesn't have node:read. We also can't check the AuthToken for the
request for the client secret, because the client hasn't yet registered so the
server doesn't have anything to compare against.

Instead of hitting the Status.Peers or Status.Members RPC endpoint, use the
Consul response directly. Update the registerNode method to handle the list of
servers we get back in the response; if we get a "no servers" or "no path to
region" response we'll kick off discovery again and retry immediately rather
than waiting 15s.

[tgross]

Moved back to draft, as this is still failing in E2E testing. I'm realizing the problem is a little more complex that I thought at first glance -- the Status.Members RPC call is happening before we register the node, so even if we pass the client secret as an auth token, the server has never seen that secret before and can't authenticate the node, because we haven't registered yet!

[tgross]

I've reworked this PR heavily based on the results of a discussion I had with @shoenig and @gulducat in order to get this working in the pre-registration phase. Because the new RPC handler is substantially the same as ee3e952 which we closed unmerged, I've added Ruslan as a co-author on this commit.

I need to do one more round of testing with this on E2E with the advertise addresses set correctly, and then I'll be ready for review.

[tgross]

Ok, I've tested this out on a cluster on AWS using HCP Consul for discovery and the networking tweaks discussed in #16507 to ensure that we're using the serf advertisement:

$ nomad operator api "/v1/status/peers"
["172.31.119.47:4647","172.31.112.244:4647","172.31.125.250:4647"]%

$ nomad operator api "/v1/agent/members" | jq -r '.Members[].Addr'
172.31.125.250
172.31.112.244
172.31.119.47

$ nomad operator api "/v1/agent/members" | jq -r '.Members[].Tags.rpc_addr'
172.31.92.121
172.31.93.95
172.31.88.76

There is no anonymous policy set. And both the servers and client end up happy:

$ nomad server members
Name                     Address         Port  Status  Leader  Raft Version  Build      Datacenter  Region
ip-172-31-88-76.global   172.31.119.47   4648  alive   false   3             1.5.2-dev  dc1         global
ip-172-31-92-121.global  172.31.125.250  4648  alive   true    3             1.5.2-dev  dc1         global
ip-172-31-93-95.global   172.31.112.244  4648  alive   false   3             1.5.2-dev  dc1         global

$ nomad node status
ID        DC   Name              Class   Drain  Eligibility  Status
700d45a8  dc1  ip-172-31-88-231  <none>  false  eligible     ready

[schmichael]

If we switch to Meta (see below) this will need to change.

However now I think we introduced ordering to our otherwise flexible upgrade procedure (at least one Server would need to be upgraded before any Clients for this to work). 🤔

But wait... what actually happens if a Client in Region A calls Node.Register on a Server in Region B?

• If the Regions are Federated, the Node.Register RPC is Forwarded and replies with the correct server list for Region A!
• If the Regions are not Federated, the Node.Register RPC errors and the Client will try the next Server, presumably until reaching a Server in its own Region.

...except I don't think that will happen for the following reasons: 😭

1. The Client.registerNode() code doesn't appear to use the response it gets from Node.Register! We pluck out the HeartbeatTTL but throw away the Server list. 👎
2. If the Regions aren't Federated, we wait up to 30s before retrying the Node.Register RPC. This means for clusters with <1,000 nodes we risk going down on Client agent restarts just from a single failed Node.Register call! 👎

Have I mentioned lately I hate all of this discovery/registration code and logic? I think it's too clever by half and makes the really critical operations like Restarting Clients really need to Register before HeartbeatTTL extremely difficult to determine/observe/tune. 😞

[tgross]

I've updated the PR (and the description above) to drop the extra tags/meta on Consul. And then I've updated the registerNode method to use the servers we get back from the response. If we get no servers or a "no path to region", we'll bubble that state up to the retry loop so that it triggers discovery again and hits the fast-path retry rather than the longer retry of 15s.