Skip to content

jepsen: run the existing suites against an encrypted cluster (§8.4) - #1232

Open
bootjp wants to merge 1 commit into
mainfrom
design/jepsen-encrypted-cluster
Open

jepsen: run the existing suites against an encrypted cluster (§8.4)#1232
bootjp wants to merge 1 commit into
mainfrom
design/jepsen-encrypted-cluster

Conversation

@bootjp

@bootjp bootjp commented Sep 10, 2026

Copy link
Copy Markdown
Owner

What §8.4 actually asks for

I went looking for an "encrypted Jepsen workload" and found the design says the opposite:

Encryption is consistency-transparent (same input bytes, different output bytes; FSM apply still deterministic), so no new Jepsen workload is required. A pass under the existing suite is the acceptance gate.

So the missing piece was never a workload — it was the ability to stand the cluster up encrypted. --encryption does that: it provisions the §5.1 KEK file on each node with owner-only permissions and starts the server with --encryption-enabled, --kekFile and --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-enabled and 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-opts so 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.clj dropping the encryption flags entirely — i.e. it would have green-lit a --encryption run 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-args from start-node! as a pure function, purely so the flag set is assertable without SSH. Dropping the flags now fails 3 tests.

Test evidence

  • Full Jepsen suite: 157 tests, 353 assertions, 0 failures
  • Revert-checked, restores byte-exact:
    • workload drops the option → 1 failure
    • flag defaults on → 1 failure (it would silently change what every existing run measures)
    • db.clj drops 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/db carrying 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 --encryption through the remaining workloads (S3, SQS, multi-table). §8.4 names Redis and DynamoDB as the gate, and the flag is in common-cli-opts, so extending is a one-line change per workload if you want it.

Self-review (five passes)

  1. Data loss — none; harness code. The gate exists to detect it.
  2. Concurrency / distributed failures — the KEK is provisioned before start-node!, since startup guards refuse a missing KEK before anything a workload could observe.
  3. Performance — one small file write per node at setup.
  4. Data consistency — the point of the gate: the existing linearizability checkers now run against an encrypted cluster unchanged.
  5. Test coverage — 9 tests, 3 revert-checks. The load-bearing one is that the flags reach the server, since a silently-unencrypted PASS is the failure mode that matters.

https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE

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
@bootjp

bootjp commented Sep 10, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@bootjp

bootjp commented Sep 10, 2026

Copy link
Copy Markdown
Owner Author

@claude review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-10T15:09:22.189751Z 395f7d6 Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Sep 10, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 50 minutes.

Check out review usage here.

View limit details

Limit 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: b0ea7adf-2dfd-4ba4-8aef-aae2e863a830

📥 Commits

Reviewing files that changed from the base of the PR and between e3ac4cc and 395f7d6.

📒 Files selected for processing (6)
  • docs/design/2026_04_29_partial_data_at_rest_encryption.md
  • jepsen/src/elastickv/cli.clj
  • jepsen/src/elastickv/db.clj
  • jepsen/src/elastickv/dynamodb_workload.clj
  • jepsen/src/elastickv/redis_workload.clj
  • jepsen/test/elastickv/encrypted_cluster_test.clj

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown

Claude finished @bootjp's task in 3m 39s —— View job


Review: jepsen: run the existing suites against an encrypted cluster (§8.4)

  • Gather context (PR description, diff, CLAUDE.md conventions)
  • Read changed files (cli.clj, db.clj, workloads, design doc, new test)
  • Review --encryption flag wiring and server-args extraction
  • Review new encrypted_cluster_test.clj
  • Cross-check against main.go/design doc for what the three flags actually turn on
  • Post final review

The cluster this stands up is never actually encrypted

--encryption provisions the KEK file and starts every node with --encryption-enabled --kekFile --encryptionSidecarPath (jepsen/src/elastickv/db.clj:129-166), but per the design doc's own milestone table, those three flags only make a node capable of accepting the encryption-mutating admin RPCs — they don't turn encryption on. --encryption-enabled's own flag text says it opts into "encryption-mutating EncryptionAdmin RPCs" (main.go:233), and Stage 6's changelog is explicit that reaching an actually-encrypted state requires three further operator actions that this PR never performs:

  1. encryption bootstrap (Stage 5C/5E) — proposes BootstrapEncryption, creates the initial DEK/writer registry.
  2. enable-storage-envelope (Stage 6D) — cuts storage over from cleartext to ciphertext.
  3. enable-raft-envelope (Stage 6E) — cuts the raft log over.

None of these are invoked anywhere in this PR. upload-binaries! (jepsen/src/elastickv/db.clj:70-79) only builds/ships the elastickv and raftadmin binaries — elastickv-admin, the only binary with an encryption bootstrap/enable-storage-envelope/enable-raft-envelope subcommand, is never built, uploaded, or invoked anywhere in jepsen/. There's also no auto-bootstrap-on-startup path in main.go (--encryption-rotate-on-startup rotates an existing DEK; it doesn't create one).

Net effect: a --encryption Jepsen run boots nodes that are encryption-capable but never cut over, so every key/value written during the run is stored in cleartext exactly as it would be without the flag. This is materially different from what §8.4 asks for ("Run the existing ... workloads against an encrypted cluster") and from what the design doc's own "Implemented" note now claims (docs/design/2026_04_29_partial_data_at_rest_encryption.md:2448-2455). It's also the same failure mode the PR is explicitly written to guard against — a --encryption run that silently produces an unencrypted cluster, reports PASS, and gets recorded as evidence for the gate — just one layer above where encrypted_cluster_test.clj checks (it pins that the flags reach argv, not that the storage/raft envelope is ever cut over).

Suggested fix: after start-node! brings the bootstrap node up (and after the join loop in setup!), shell out to elastickv-admin encryption bootstrap (build/upload that binary alongside elastickv/raftadmin), then enable-storage-envelope and enable-raft-envelope, before the workload begins issuing ops. Until that lands, --encryption and the "Implemented" note should be described as "starts an encryption-capable cluster" rather than "runs against an encrypted cluster."

Fix this →

Secondary: --encryption is silently inert on the workloads that don't wire it

The flag lives in cli/common-cli-opts (jepsen/src/elastickv/cli.clj:19-20), so s3_workload.clj, sqs_htfifo_workload.clj, dynamodb_multi_table_workload.clj, dynamodb_types_workload.clj, and redis_zset_safety_workload.clj all parse it into their CLI opts (via (into cli/common-cli-opts ...)) but never thread it into their ekdb/db call. Running any of those with --encryption parses cleanly and silently produces an unencrypted cluster — no error, no warning. The PR body calls this out as known/deferred scope, which is reasonable, but it's worth a one-line assert/warn in those workloads' option validation (or in cli.clj) given how much of this PR's own design is about not letting --encryption silently do nothing.

What looks solid

  • server-args extraction (jepsen/src/elastickv/db.clj:135-166) is a faithful pure-function refactor of the old inline args cond-> in start-node! — I diffed the two forms line-by-line and the non-encryption behavior is unchanged.
  • kek-test-bytes (32 raw k bytes) matches --kekFile's documented "32 raw bytes" requirement (main.go:248), and using a fixed value across all nodes is correct — a per-node random KEK would indeed hit ErrKEKMismatch since they all unwrap the same sidecar.
  • provision-kek! runs only in setup!, not in db/Kill's start!; since kill! doesn't wipe data-dir (only teardown! does), the KEK file survives nemesis-driven restarts, so it doesn't need re-provisioning there.
  • The three-flags-travel-together design (sidecar-alone = read-only probing) matches main.go:218-233's documented gating.
  • encrypted_cluster_test.clj's revert-checks are good practice, and the flag names/opts plumbing genuinely match what main.go expects.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +251 to +252
(when (:encryption opts)
(provision-kek! node))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Comment on lines +20 to +21
[nil "--encryption" "Run against a cluster with data-at-rest encryption enabled."
:default false]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant