Fix KVSGet method to handle QueryOptions properly

[kisunji]

Fixes #13303

Description

Originally reported by @wjordan here, I requested the bugfix be removed from #11500 assuming it was a separate issue. Unfortunately it turns out that broke KVS.Get RPC operations in 1.12.1.

Cause

The KV.Get endpoint was being passed **structs.KeyRequest, which when handled by a client's RPC method, was not fulfilling this assertion:

consul/agent/pool/pool.go

Lines 632 to 635 in c48120d

	// Use the zero value if the request doesn't implement RPCInfo
	if info, ok := args.(structs.RPCInfo); ok {
	sc.stream.FirstReadTimeout = info.Timeout(p.Timeout, p.MaxQueryTime, p.DefaultQueryTime)
	}

causing the ReadTimeout to be set to the default value of rpc_hold_timeout (source).

Testing & Reproduction steps

Set up a cluster with server and client using vagrant.

# 1.12.1
$ consul join 192.168.56.10
Successfully joined cluster by contacting 1 nodes.

$ consul members
Node       Address             Status  Type    Build   Protocol  DC   Partition  Segment
agent-one  192.168.56.10:8301  alive   server  1.12.1  2         dc1  default    <all>
agent-two  192.168.56.11:8301  alive   client  1.12.1  2         dc1  default    <default>

$ consul kv put test foo
Success! Data written to: test

$ time curl "http://localhost:8500/v1/kv/test"
[{"LockIndex":0,"Key":"test","Flags":0,"Value":"Zm9v","CreateIndex":11,"ModifyIndex":11}]
real	0m0.013s
user	0m0.000s
sys	0m0.004s

$ time curl "http://localhost:8500/v1/kv/test?index=99&stale=&wait=60000ms"
rpc error making call: i/o deadline reached
real	0m7.013s <- deadline of rpc_hold_timeout
user	0m0.004s
sys	0m0.000s

# 1.13.0dev patch (version comes from main; ignore)
$ consul join 192.168.56.10
Successfully joined cluster by contacting 1 nodes.

$ consul members
Node       Address             Status  Type    Build      Protocol  DC   Partition  Segment
agent-one  192.168.56.10:8301  alive   server  1.12.1     2         dc1  default    <all>
agent-two  192.168.56.11:8301  alive   client  1.13.0dev  2         dc1  default    <default>

$ consul kv put test foo
Success! Data written to: test

$ time curl "http://localhost:8500/v1/kv/test"
[{"LockIndex":0,"Key":"test","Flags":0,"Value":"Zm9v","CreateIndex":12,"ModifyIndex":12}]
real	0m0.044s
user	0m0.012s
sys	0m0.004s

$ time curl "http://localhost:8500/v1/kv/test?index=99&stale=&wait=60000ms"
[{"LockIndex":0,"Key":"test","Flags":0,"Value":"Zm9v","CreateIndex":12,"ModifyIndex":12}]
real	1m2.708s <- wait=60000ms correctly applied
user	0m0.008s
sys	0m0.000s

[dhiaayachi]

🚀
It's likely not for the scope of this fix, but I'm wondering why we don't transition to an RPCInfo arg type for the RPC interface?

[kisunji]

🚀 It's likely not for the scope of this fix, but I'm wondering why we don't transition to an RPCInfo arg type for the RPC interface?

Do you mean instead of interface{}? RPCInfo is embedded by a lot of RPC request type structs to hold RPC metadata but won't hold the data itself (unless we give it a payload method)