Large KVs using chunked raft messages can fail verification
- Dominant language
- Go
- Stars
- 30.1k
- Forks
- 4.6k
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 43
Description
#### Overview of the Issue
When a large (close to 512kB) KV is applied via raft, it can fail verification and be rejected.
1. A large KV is applied by `KVS.Apply()`, which calls `server.raftApply()`:
https://github.com/hashicorp/consul/blob/b0296e557a0584c933a723c94be25c35086d12d8/agent/consul/kvs_endpoint.go#L129
1. `raftApply()` ends up calling `raftApplyWithEncoder(..., structs.Encoder)`
1. `structs.Encoder()` encodes messages with the type in the first byte, followed by the msgpack encoded blob.
https://github.com/hashicorp/consul/blob/b0296e557a0584c933a723c94be25c35086d12d8/agent/structs/structs.go#L3135-L3137
1. Once encoded, the 512 kB KV will exceed `raft.SuggestedMaxDataSize`. `raftApplyEncoded` will "chunk" it into several messages:
https://github.com/hashicorp/consul/blob/b0296e557a0584c933a723c94be25c35086d12d8/agent/consul/rpc.go#L999-L1004
1. `ChunkingApply` generates a `raft.Log` per chunk, with:
a. `Data` set to part of the raw message passed in.
b. `Extensions` set to a `ChunkInfo` describing the number of chunks.
https://github.com/hashicorp/go-raftchunking/blob/1e61ed476ac8d41cdb2ef2ca74630becfc1afab7/api.go#L92
1. When verification is enabled, a `verifier.NewLogStore()` is constructed with `isLogVerifyCheckpoint` as the `IsCheckpointFn` callback.
1. The verifier passes the whole `raft.Log` to `isLogVerifyCheckpoint`:
https://github.com/hashicorp/raft-wal/blob/ebffec3619e9330066b00ebd42135a68ef404421/verifier/store.go#L121
1. `isLogVerifyCheckpoint` only checks `log.Data[0]` to determine if the message is a checkpoint or not:
https://github.com/hashicorp/consul/blob/b0296e557a0584c933a723c94be25c35086d12d8/agent/consul/server_log_verification.go#L28-L32
That's correct for the first chunk: the first byte of data will be a type encoded by `structs.Encode()`.
But for subsequent chunks, `data[0]` will be an arbitrary byte from the KV payload! If it happens to be `RaftLogVerifierCheckpoint`, `41`, the chunk will be parsed as a checkpoint.
https://github.com/hashicorp/consul/blob/b0296e557a0584c933a723c94be25c35086d12d8/agent/structs/structs.go#L91
This fails (I haven't dug further into what specifically happens to it) with:
```
rpc error making call: raft apply failed: unable to store logs within log store, err: "failed updating verifier state: short buffer"
```
#### Reproduction Steps
On a cluster with verification enabled, writing a 512kB KV of only `41` / `0x29`:
```
python3 -c 'import sys; sys.stdout.buffer.write(b"\x29" * (512 * 1024))' | consul kv put test -
```
will error with:
```
Error! Failed writing data: Unexpected response code: 500 (rpc error making call: rpc error making call: raft apply failed: unable to store logs within log store, err: "failed updating verifier state: short buffer")
```
### Inconsistent Verification
This is particularly fun if not all servers in the cluster have verification enabled. In one case in our environment, in a 5 server cluster, we had two servers using:
```
raft_logstore {
backend = "boltdb"
}
```
which doesn't enable verification.
And three servers without a `backend` setting. That defaults to `WAL` with verification enabled:
https://github.com/hashicorp/consul/blob/b0296e557a0584c933a723c94be25c35086d12d8/agent/consul/server.go#L1020-L1026
If the leader is one of the two servers without verification, it will apply the raft updates for big KVs with `41` at the right offset. But the three peers with verification will reject it, and the whole cluster becomes unusable without wiping it.
Could there be some kind of warning for clusters with inconsistent verification settings? It makes this situation much worse.
Contributor guide
Research direction
Reproduce the failure with the provided 512kB command, then trace chunk handling from agent/consul/rpc.go through agent/consul/server_log_verification.go and agent/structs/structs.go. Inspect verifier.NewLogStore and the chunk metadata to define the correct checkpoint behavior, and add regression coverage for large KVs whose chunks contain 0x29. Done means the reproduction no longer rejects the KV; separately assess whether inconsistent verification settings need a warning.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- distributed-systems
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 68/100