hashicorp / hashicorp/consul

Large KVs using chunked raft messages can fail verification

Open
#23,926 0 comments 1 reaction 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.