jepsen: run the existing suites against an encrypted cluster (§8.4) - #1232
jepsen: run the existing suites against an encrypted cluster (§8.4)#1232bootjp wants to merge 1 commit into
Conversation
The encryption design's Jepsen gate is explicit that NO new workload is required: encryption is consistency-transparent, so the acceptance gate is the existing Redis and DynamoDB workloads passing against an encrypted cluster. What was missing was the ability to stand that cluster up. --encryption provisions the §5.1 KEK file on each node with owner-only permissions and starts the server with --encryption-enabled, --kekFile and --encryptionSidecarPath. The three travel together because a sidecar path alone only enables read-only capability probing, while the mutating RPCs a bootstrap needs require the flag and a KEK source too. The KEK is a fixed test value rather than a generated one: every node must unwrap the same sidecar, and a per-node random KEK would fail startup with ErrKEKMismatch — a far more confusing failure than a hardcoded test key. The flag lives in common-cli-opts, so a future workload inherits the gate without opting in, and defaults off so existing runs measure the same thing they did before. server-args is extracted from start-node! as a pure function purely so the flag set is testable without SSH. That matters more than it looks: a --encryption run that silently produced an UNENCRYPTED cluster would report PASS and be recorded as evidence for this gate, which is worse than having no gate. The first version of these tests did not pin it — dropping the flags from db.clj left them all green. Full suite: 157 tests, 0 failures. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
|
@codex review |
|
@claude review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedNext included review available in 50 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @bootjp's task in 3m 39s —— View job Review:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 395f7d6259
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (when (:encryption opts) | ||
| (provision-kek! node)) |
There was a problem hiding this comment.
Bootstrap and activate encryption before running workloads
When --encryption is used with a fresh Jepsen data directory, this branch only provisions a KEK before starting the nodes. The server's --encryption-enabled flag merely enables the encryption mutator RPCs, while buildEncryptionWriteWiring deliberately keeps writes cleartext until both BootstrapEncryption and EnableStorageEnvelope have applied; nothing in setup! invokes either operation. Consequently the Redis and DynamoDB acceptance runs can report PASS while all workload data was stored unencrypted. After cluster membership is established, the harness must bootstrap the DEKs, perform the storage-envelope cutover, and wait for it to apply before beginning the workload.
Useful? React with 👍 / 👎.
| [nil "--encryption" "Run against a cluster with data-at-rest encryption enabled." | ||
| :default false] |
There was a problem hiding this comment.
Do not advertise encryption to workloads that drop the option
Because this option is added to common-cli-opts, the S3, SQS, DynamoDB-types, DynamoDB-multi-table, and Redis-zset entrypoints all accept --encryption; however, their test constructors omit :encryption when building ekdb/db (a repo-wide search shows only redis_workload.clj and dynamodb_workload.clj propagate it). Those commands therefore silently launch an unencrypted cluster despite the CLI promise. Either propagate the option through every workload using the common options or reject/remove it from unsupported entrypoints.
Useful? React with 👍 / 👎.
What §8.4 actually asks for
I went looking for an "encrypted Jepsen workload" and found the design says the opposite:
So the missing piece was never a workload — it was the ability to stand the cluster up encrypted.
--encryptiondoes that: it provisions the §5.1 KEK file on each node with owner-only permissions and starts the server with--encryption-enabled,--kekFileand--encryptionSidecarPath.Decisions worth reviewing
All three flags travel together. A sidecar path alone only enables read-only capability probing; the mutating RPCs a bootstrap needs require
--encryption-enabledand a KEK source. Two of the three yields either a refusal to start or, worse, a cluster that starts unencrypted.The KEK is a fixed test value, not generated. Every node must unwrap the same sidecar — a per-node random KEK fails startup with
ErrKEKMismatch, which is a much more confusing failure to debug than a hardcoded test key is to notice.The flag lives in
common-cli-optsso a future workload inherits the gate without opting in, and defaults off so existing runs measure exactly what they did before.The test that wasn't testing anything
My first version of these tests passed with
db.cljdropping the encryption flags entirely — i.e. it would have green-lit a--encryptionrun that produced an unencrypted cluster, reported PASS, and been recorded as evidence for this acceptance gate. That is strictly worse than having no gate.Fixed by extracting
server-argsfromstart-node!as a pure function, purely so the flag set is assertable without SSH. Dropping the flags now fails 3 tests.Test evidence
db.cljdrops the flags → 3 failures (this is the one that initially passed)9 tests: flag availability and default, propagation through both workloads named in §8.4,
ekdb/dbcarrying the option, the three-flag emission, absence when unrequested, and that enabling it disturbs no other argv entry.Behavior change / risk
Test-harness only; no production code. Default off, so every existing run is byte-identical.
Not included: wiring
--encryptionthrough the remaining workloads (S3, SQS, multi-table). §8.4 names Redis and DynamoDB as the gate, and the flag is incommon-cli-opts, so extending is a one-line change per workload if you want it.Self-review (five passes)
start-node!, since startup guards refuse a missing KEK before anything a workload could observe.https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE