CASSSIDECAR-429: Add RFC 6902-inspired JSON Patch support to ConfigurationManager by pauloricardomg · Pull Request #369 · apache/cassandra-sidecar

pauloricardomg · 2026-06-29T21:33:10Z

Summary

Implements patch operations for the Configuration Manager using RFC 6902-inspired semantics. Paths reference the effective configuration structure (base template + overlay merged) but mutations target the overlay only. Supported operations: add, remove, replace, test. move and copy are intentionally excluded as they introduce ambiguous semantics in the two-tier configuration model.

Configuration Model

The configuration uses a two-tier model:

Base template: the stock cassandra.yaml provided by the operator, read-only via this API.
Overlay: a sparse set of values explicitly set via patch operations, stored by the ConfigurationProvider.
Effective config: the result of deep-merging base template + overlay. Overlay values take precedence. This is what the GET API returns and what Cassandra uses at startup.

Base Template          Overlay              Effective Config
+-----------------+    +-------------+      +-------------------+
| cluster: "prod" |    | reads: 128  |  =>  | cluster: "prod"   |
| reads: 32       | +  | flush: 8    |      | reads: 128        |
| flush: 4        |    +-------------+      | flush: 8          |
+-----------------+                         +-------------------+

Patch operations target paths in the effective config (what the client sees from GET) but only mutate the overlay. The base template is never modified by this API.

Overview

Patch Operations

Operation	Behavior	Precondition
`add`	Upsert value in overlay	Parent must exist in effective config (RFC 6902)
`remove`	Remove key from overlay; effective config falls back to template	Key must exist in overlay (not just template)
`replace`	Set value in overlay	Key must exist in effective config
`test`	Assert effective config value matches expected; no mutation	Path must exist in effective config

Copy-Siblings Strategy (nested paths)

When a patch targets a nested path within a top-level cassandraYaml key, the entire top-level key value is copied from the effective config into the overlay before applying the leaf change. This ensures the overlay is always self-contained at the top-level key granularity, preventing orphan fragments if the base template later removes the parent structure.

Example:

Template:

{"memtable": {"configurations": {"trie": {"class_name": "TrieMemtable", "max_shard_count": 4}, "skiplist": {"class_name": "SkipListMemtable"}}}}

Patch: replace /configuration/cassandraYaml/memtable/configurations/trie/max_shard_count with value 8

Resulting overlay (entire memtable key stored, not just the leaf):

{"memtable": {"configurations": {"trie": {"class_name": "TrieMemtable", "max_shard_count": 8}, "skiplist": {"class_name": "SkipListMemtable"}}}}

The trade-off is that sibling fields are "pinned" in the overlay. If the base template later modifies skiplist, the overlay's copy takes precedence. Operators can remove the sibling path from the overlay to re-sync with the template.

Main Classes

Class	Responsibility
`ConfigurationPatchOperation`	Value object: `Op` enum (ADD, REMOVE, REPLACE, TEST) + path + value
`ConfigurationPatchValidator`	Validates ops before application: path format, value presence, duplicate detection, segment parsing
`ConfigurationPatchApplier`	Applies validated ops: precondition checks (phase 1), then mutations (phase 2) for atomicity
`ConfigurationPatchException`	Thrown on validation/application failures with the failing operation attached
`ConfigurationConflictException`	Thrown on hash mismatch (optimistic concurrency)
`ConfigurationManager.patchConfiguration`	Orchestrates: validate ops, check hash, apply patch, CAS via provider

Validation Flow

Structural validation (ConfigurationPatchValidator): path prefix, segment parsing, value presence, duplicate mutation paths
Hash check (ConfigurationManager): caller's expectedHash vs current effective config hash
Precondition checks (ConfigurationPatchApplier phase 1): per-op checks against effective config and overlay
Mutations (ConfigurationPatchApplier phase 2): only if all preconditions pass

Additional Validations

JVM option key format: must start with -D, -X, or -XX:
Conflicting boolean JVM opts: -XX:+Foo and -XX:-Foo cannot coexist (remove first, then add)
Nested paths into array values: rejected with clear error (array index traversal not supported)
extraJvmOpts paths must be flat: no nested segments allowed

Test Overview

Test Class	Focus
`ConfigurationPatchValidatorTest`	Path parsing, value presence, duplicates, error cases
`ConfigurationPatchApplierTest`	All 4 ops at top-level and nested, copy-siblings, arrays, JVM validation, atomicity
`ConfigurationManagerTest`	End-to-end through the manager: hash conflicts, provider failures, concurrency

skoppu22 · 2026-07-08T14:02:20Z

+                                                                 ? currentOverlay.configuration()
+                                                                 : new CassandraConfigurationOverlay(null, null);
+            CassandraConfigurationOverlay updatedOverlay =
+                    ConfigurationPatchApplier.apply(parsedOps, effectiveConfig.configuration(), currentOverlayConfig);


If incoming patch request has a conflicting jvm option against base yaml, this ConfigurationPatchApplier.apply only checks against overlay and doesn't detect conflict. storeOverlay call below stores it. Then baseSnapshot.overlay call below detects conflict with the base, prints warning and returns config without the conflicting option to the caller. The same thing happens in subsequent calls as well.
Also, returned config should be same as stored config, which is not true in this case.

Good to have test case for the same

Good catch! Addressed in 17fb889.
apply() now checks boolean JVM opt conflicts against the effective config (base + overlay), not just the overlay, and rejects the patch before storeOverlay is called. Stored and returned configs no longer diverge, and the warn-and-drop path in baseSnapshot.overlay() is no longer hit.

Tests: ConfigurationManagerTest.testPatchConflictingBooleanJvmOptRejected (rejected, nothing stored) and testPatchReturnedConfigMatchesStoredConfig (returned == re-read).

skoppu22 · 2026-07-08T14:07:31Z

+            "^-(D[a-zA-Z][a-zA-Z0-9._-]*|X[a-z][a-zA-Z0-9]*|XX:[+-]?[a-zA-Z][a-zA-Z0-9_]*)$");
+
+    // XX flags that accept arbitrary commands as values
+    static final Set<String> BLOCKED_XX_FLAGS = Set.of(


There are more options which can take paths etc e.g. -XX:ErrorFile, -XX:HeapDumpPath, -XX:LogFile, -Xloggc, and -D-style system properties. All absolute paths are allowed. Good to have allowlist instead of blocklist to be specific about which options are allowed and which paths are allowed.

Addressed in 5c67693 by expanding the blocklist to cover path-bearing/file-writing options (-XX:ErrorFile, -XX:HeapDumpPath, -XX:LogFile, -XX:FlightRecorderOptions, -XX:StartFlightRecording, -Xloggc, -Xlog, -Xbootclasspath) alongside the existing command-executing -XX:OnError/-XX:OnOutOfMemoryError.

I honestly don't know the full set of valid JVM configs, and an allowlist would mean enumerating every legitimate Cassandra JVM option (all the -Xms/-Xmx/-Xss/-XX GC and heap tuning flags plus the whole -Dcassandra.* / -Dcom.sun.management.* / -Djava.* system-property surface), which would make the list massive and hard to keep current. This isn't a security issue in practice: the patch endpoint requires admin credentials, so we're just removing the obviously insecure options and can add more as we find them. If you feel strongly about an allowlist, I'm happy to switch to one.

skoppu22 · 2026-07-08T14:28:33Z

+    {
+        if (actual instanceof JsonObject && expected instanceof Map)
+        {
+            return actual.equals(new JsonObject((Map<String, Object>) expected));


As we anyhow converting expected to JsonObject, can we expect the caller to pass both actual and expected as JsonObject, so we don't have to worry if passed params are not exactly matching instanceof expectations?

Addressed in d973ec4. The patch handler builds each operation's value via JsonObject.getValue() (from context.body().asJsonArray()), which returns Vert.x JSON types - JsonObject for objects, JsonArray for arrays, or scalars - the same types resolveValue returns for the effective config. So both sides are already comparable and the TEST precondition is now a plain Objects.equals with no instanceof/conversion. Added coverage for object- and array-valued TEST operations.

skoppu22 · 2026-07-08T14:43:22Z

Well structured code !

skoppu22 · 2026-07-13T12:40:42Z

[nit] stale reference 'updated'

Good catch! addressed on 62c0ebc

skoppu22 · 2026-07-13T12:48:09Z

+
+    // Allows: alphanumeric, dots, colons, slashes, @, +, commas, spaces, hyphens (max 512 chars)
+    // Rejects: shell metacharacters (;|&$`), quotes, newlines, and other control characters
+    static final Pattern JVM_OPT_VALUE_PATTERN = Pattern.compile("^[a-zA-Z0-9._:/@+, -]{0,512}$");


Is allowing space intentional? Just thinking does this allow injection of non-allowed keys or value by any chance?

Good catch - space is no longer allowed on d818e07 since it would break cassandra startup script (the Cassandra launcher word-splits unquoted values during eval).

I also updated the value pattern to permit the JSON characters " { } [ ]. That is needed for JSON-valued options like -Dcassandra.settings.default_compaction={"class_name":"SizeTieredCompactionStrategy","parameters":{...}}.

skoppu22 · 2026-07-13T12:50:13Z

+            }
+            parent = childObj;
+        }
+        parent.remove(segments.get(segments.size() - 1));


[nit] Don't we need to remove parent if it is empty after only child removal?

Per RFC 6902, remove deletes only the targeted member and never prunes a now-empty parent - the client must remove the parent in a separate op. For example, removing the only overridden key under server_encryption_options:

{ "op": "remove", "path": "/cassandraYaml/server_encryption_options/optional" }

leaves server_encryption_options: {} behind; the client has to follow up with { "op": "remove", "path": "/cassandraYaml/server_encryption_options" } to drop it. So this is intentional/spec-compliant, not a bug - the empty parent is only removed if explicitly requested.

skoppu22 · 2026-07-13T12:57:34Z

+                }
+                // Both sides are Vert.x JSON types: resolveValue returns JsonObject/JsonArray/scalars,
+                // and op.value() is read from the request via JsonObject.getValue (see
+                // ConfigurationPatchHandler), which wraps objects/arrays the same way.


Is ConfigurationPatchHandler coming in upcoming PRs, I couldn't locate it ?

This will come in upcoming PR for CASSSIDECAR-436: pauloricardomg#7

I updated the comment to remove this reference on 38b8780

pauloricardomg · 2026-07-13T21:56:25Z

Thanks for the review @skoppu22 !

frankgh

+1

…ationManager Implement patch operations using RFC 6902-inspired semantics: paths reference the effective configuration structure but mutations target the overlay. Supported operations: add, remove, replace, test. Key behaviors: - Paths use JSON Pointer syntax for both top-level and nested cassandraYaml keys - Nested path mutations copy the entire top-level key from effective config into the overlay (copy-siblings strategy), ensuring the overlay is self-contained at top-level key granularity - test operations assert against effective config without mutating - replace fails if path is absent from effective config (typo protection) - remove fails if path only exists in base template (not in overlay) - All operations validated atomically before any mutations are applied - JVM option key format and conflicting boolean option validation - Nested paths into array-valued keys are rejected gracefully

…ffective config Previously ConfigurationPatchApplier only checked boolean JVM option conflicts against the overlay, so a patch adding e.g. -XX:-UseG1GC while the base template already set -XX:+UseG1GC was not rejected. The overlay was stored, then baseSnapshot.overlay() detected the conflict on read, logged a warning, and returned the effective config without the option - leaving the stored overlay and the returned config inconsistent, and repeating on every subsequent call. Conflict detection now runs against a projection of the effective options after the patch is applied, rejecting the patch before anything is stored. Adds coverage in ConfigurationPatchApplierTest and ConfigurationManagerTest, including a stored-equals-returned assertion.

…xtraJvmOpts The extraJvmOpts value pattern permits absolute paths because Cassandra system properties legitimately need them, so JVM options that write to arbitrary filesystem paths must be blocked by key. Expands the blocklist (renamed BLOCKED_XX_FLAGS -> BLOCKED_JVM_OPTS) to cover -XX:ErrorFile, -XX:HeapDumpPath, -XX:LogFile, -XX:FlightRecorderOptions, -XX:StartFlightRecording, -Xloggc, -Xlog and -Xbootclasspath alongside the existing command-executing -XX:OnError / -XX:OnOutOfMemoryError.

…Applier Removes valueEquals(), which used instanceof checks to bridge a JsonObject actual against a Map expected. The patch request handler builds each operation's value via JsonObject.getValue(), which returns Vert.x JSON types (JsonObject for objects, JsonArray for arrays, or scalars) - the same types resolveValue returns for the effective config. Both sides are therefore already comparable, so the TEST precondition is now a plain Objects.equals with no conversion. Adds coverage for object- and array-valued TEST operations.

…nOverlay

Managed extra JVM options may carry JSON values (e.g. a compaction ParameterizedClass). Update JVM_OPT_VALUE_PATTERN to permit braces, brackets and double quotes so such values pass validation, and reject whitespace since the Cassandra launcher word-splits unquoted values. Shell metacharacters and control characters remain rejected.

…hHandler in comment

…Manager class javadoc

JsonObject.getValue wraps nested Maps into JsonObject (via JsonUtil.wrapJsonValue), so values read through it are never raw Maps. Simplify asJsonObject and deepCopyValue to handle only JsonObject, and reduce the add parent-type check accordingly.

…n 5) Previously the applier validated all operation preconditions against the initial effective configuration up front, then applied mutations in a second pass. This meant an operation could not observe the effects of earlier operations in the same patch, deviating from RFC 6902 section 5 (each operation applies to the document produced by the previous one). Concretely: - test-after-replace on the same key was checked against the stale initial value, wrongly rejecting valid change-and-verify patches and wrongly accepting contradictory ones; - creating a nested path and then writing into it in the same patch was rejected because the second op's parent was resolved against the pre-op state; - removing a path and then removing/writing a child of it silently no-opped or resurrected the value from the stale effective snapshot. Collapse the two phases into a single sequential loop that re-derives the effective configuration (base + evolving overlay) before each operation, so preconditions and mutations both see prior operations' effects. Atomicity is unchanged: mutations target a throwaway overlay copy that is only returned on clean completion. - apply() now takes the base configuration and merges the effective view per op; ConfigurationManager passes baseSnapshot.configuration() accordingly. - The separate checkResultingJvmOptConflicts projection pass is removed; the boolean-opt conflict check moves into checkJvmOptsPreconditions against the current effective opts. - Add ConfigUtils.mergeOpts to share overlay opts-merge semantics; it logs when a conflicting overlay boolean option is skipped in favor of the base. - removeAtPath's silent no-op becomes a defensive IllegalStateException, now that the sequential precondition guarantees the path exists. - ConfigurationPatchValidator is unchanged: its checks are state-independent. Add tests covering the sequential cases and restructure the boolean-opt remove-then-add test so the toggled option lives only in the overlay.

…th tests The copy-siblings strategy sits on top of the deep merge (ConfigUtils.mergeConfigurations), which produces two behaviors worth calling out: - Editing a nested leaf snapshots the top-level block's existing leaves into the overlay, pinning those siblings against later base-template drift. Keys added to the base block afterwards are not pinned - the deep merge still surfaces them. - Removing an overlaid leaf reverts it to the current base value, which may differ from the value that was in effect when the overlay was written. Document both in the ConfigurationPatchApplier class javadoc, reference them from ConfigurationManager.patchConfiguration, and add tests asserting each.

QFREEE · 2026-07-24T21:21:46Z

The copy-siblings strategy makes sense to me, and I think the trade-off is reasonable.

One thing that may be worth considering is making this behavior visible to API consumers. A nested patch appears narrow, but it can copy and pin sibling values in the overlay. Could the PATCH response, API documentation, or logs indicate that the containing top-level section was copied into the overlay?

This may fit better in the CASSSIDECAR-436 endpoint PR, but having that visibility would make later base-template drift easier to understand and troubleshoot.

skoppu22 reviewed Jul 8, 2026

View reviewed changes

skoppu22 reviewed Jul 13, 2026

View reviewed changes

skoppu22 approved these changes Jul 13, 2026

View reviewed changes

frankgh approved these changes Jul 22, 2026

View reviewed changes

pauloricardomg added 13 commits July 23, 2026 15:22

CASSSIDECAR-429: Fix stale javadoc reference in CassandraConfiguratio…

887c92d

…nOverlay

CASSSIDECAR-429: Drop reference to not-yet-existing ConfigurationPatc…

a2f5c0d

…hHandler in comment

Update CHANGES.txt

548b195

CASSSIDECAR-429: Define effective configuration once in Configuration…

31899eb

…Manager class javadoc

CASSSIDECAR-429: Add example to nestedSegments javadoc

627b043

CASSSIDECAR-429: Rename topLevel to topLevelObject in applyYamlMutation

2709d07

pauloricardomg force-pushed the CASSSIDECAR-429_v2 branch from 38b8780 to a9644e9 Compare July 23, 2026 21:50

pauloricardomg force-pushed the CASSSIDECAR-429_v2 branch from a9644e9 to e46d50d Compare July 23, 2026 21:55

Uh oh!

Conversation

pauloricardomg commented Jun 29, 2026

Summary

Configuration Model

Overview

Patch Operations

Copy-Siblings Strategy (nested paths)

Main Classes

Validation Flow

Additional Validations

Test Overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pauloricardomg Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skoppu22 commented Jul 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pauloricardomg commented Jul 13, 2026

Uh oh!

frankgh left a comment

Choose a reason for hiding this comment

Uh oh!

QFREEE commented Jul 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pauloricardomg Jul 10, 2026 •

edited

Loading