From 1f18625d12eaf48cfd8f835e3203cb642e8fc7d0 Mon Sep 17 00:00:00 2001 From: Paulo Motta Date: Wed, 17 Jun 2026 12:15:24 -0400 Subject: [PATCH 1/7] CASSSIDECAR-429: Add RFC 6902-inspired JSON Patch support to ConfigurationManager 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 --- .../CassandraConfigurationOverlay.java | 69 +-- .../ConfigurationConflictException.java | 54 +++ .../ConfigurationManager.java | 92 ++++ .../ConfigurationPatchApplier.java | 451 ++++++++++++++++++ .../ConfigurationPatchException.java | 45 ++ .../ConfigurationPatchOperation.java | 117 +++++ .../ConfigurationPatchValidator.java | 283 +++++++++++ .../ConfigurationProvider.java | 6 +- .../CassandraConfigurationOverlayTest.java | 116 +---- .../ConfigurationManagerTest.java | 408 ++++++++++++++++ .../ConfigurationPatchApplierTest.java | 434 +++++++++++++++++ .../ConfigurationPatchValidatorTest.java | 381 +++++++++++++++ 12 files changed, 2289 insertions(+), 167 deletions(-) create mode 100644 server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationConflictException.java create mode 100644 server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplier.java create mode 100644 server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchException.java create mode 100644 server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchOperation.java create mode 100644 server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidator.java create mode 100644 server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplierTest.java create mode 100644 server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidatorTest.java diff --git a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/CassandraConfigurationOverlay.java b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/CassandraConfigurationOverlay.java index ad518b905..0734babbb 100644 --- a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/CassandraConfigurationOverlay.java +++ b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/CassandraConfigurationOverlay.java @@ -39,6 +39,9 @@ *

The {@code extraJvmOpts} field contains JVM options that are appended to the Cassandra JVM startup * command. Each entry maps the full option flag (e.g. {@code -Dcassandra.jmx.local.port}) to its value * (e.g. {@code 7199}). These are opaque strings not subject to schema validation. + * + *

This class is immutable. Mutations are performed by {@link ConfigurationPatchApplier} which + * constructs a new instance with the desired changes. */ public class CassandraConfigurationOverlay { @@ -114,59 +117,6 @@ public static CassandraConfigurationOverlay fromJson(@NotNull JsonObject json) return new CassandraConfigurationOverlay(cassandraYaml, extraJvmOpts); } - /** - * Returns a new overlay with the given updates applied. The current instance is not modified. - * - *

Both parameters follow the same semantics: a {@code null} value for a key removes that entry, - * a non-null value upserts it. - * - * @param cassandraYamlUpdates field-level changes to cassandra.yaml: key = field name, value = new value. - * A {@code null} value removes the field. Pass {@code null} for no yaml changes. - * @param extraJvmOptsUpdates JVM option changes: key = option name, value = new option value. - * A {@code null} value removes the option. Pass {@code null} for no changes. - * @return a new overlay with the updates applied - */ - @NotNull - public CassandraConfigurationOverlay updated(@Nullable Map cassandraYamlUpdates, - @Nullable Map extraJvmOptsUpdates) - { - JsonObject mergedYaml = cassandraYaml.copy(); - if (cassandraYamlUpdates != null) - { - for (Map.Entry entry : cassandraYamlUpdates.entrySet()) - { - if (entry.getValue() == null) - { - mergedYaml.remove(entry.getKey()); - } - else - { - mergedYaml.put(entry.getKey(), entry.getValue()); - } - } - } - - LinkedHashMap mergedOpts = new LinkedHashMap<>(extraJvmOpts); - if (extraJvmOptsUpdates != null) - { - for (Map.Entry entry : extraJvmOptsUpdates.entrySet()) - { - if (entry.getValue() == null) - { - mergedOpts.remove(entry.getKey()); - } - else - { - mergedOpts.put(entry.getKey(), entry.getValue()); - } - } - } - - validateNoConflictingBooleanOpts(mergedOpts); - - return new CassandraConfigurationOverlay(mergedYaml, mergedOpts); - } - static boolean hasConflictingBooleanOpt(Map existing, String key) { String conflicting = conflictingBooleanOpt(key); @@ -186,19 +136,6 @@ static String conflictingBooleanOpt(String key) return null; } - private static void validateNoConflictingBooleanOpts(Map opts) - { - for (String key : opts.keySet()) - { - if (hasConflictingBooleanOpt(opts, key)) - { - String option = key.substring(5); - throw new IllegalArgumentException( - "Conflicting boolean JVM options: -XX:+" + option + " and -XX:-" + option); - } - } - } - @Override public boolean equals(Object o) { diff --git a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationConflictException.java b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationConflictException.java new file mode 100644 index 000000000..beeb701ab --- /dev/null +++ b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationConflictException.java @@ -0,0 +1,54 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.sidecar.configmanagement; + +import org.jetbrains.annotations.NotNull; + +/** + * Thrown when a configuration patch fails due to a hash mismatch, + * indicating a concurrent modification to the effective configuration. + */ +public class ConfigurationConflictException extends ConfigurationManagerException +{ + @NotNull + private final String expectedHash; + + @NotNull + private final String actualHash; + + public ConfigurationConflictException(@NotNull String expectedHash, @NotNull String actualHash) + { + super("Configuration conflict: expected hash [" + expectedHash + + "] but found [" + actualHash + "]", null); + this.expectedHash = expectedHash; + this.actualHash = actualHash; + } + + @NotNull + public String expectedHash() + { + return expectedHash; + } + + @NotNull + public String actualHash() + { + return actualHash; + } +} diff --git a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationManager.java b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationManager.java index 0ae2b6ce8..15600aea0 100644 --- a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationManager.java +++ b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationManager.java @@ -19,6 +19,8 @@ package org.apache.cassandra.sidecar.configmanagement; import java.nio.file.Path; +import java.time.Instant; +import java.util.List; import java.util.Objects; import org.apache.cassandra.sidecar.cluster.instance.InstanceMetadata; @@ -84,4 +86,94 @@ private ConfigurationOverlaySnapshot getBaseSnapshot() cachedBaseSnapshot = snapshot; return snapshot; } + + /** + * Patches the effective configuration for the given instance using RFC 6902-inspired JSON Patch + * operations. Validates the caller's expected hash against the current effective configuration, + * validates and applies the patch operations to the overlay, persists via the + * {@link ConfigurationProvider}, and returns the new effective configuration. + * + *

Paths reference the effective configuration structure, but mutations target the overlay. + * For nested paths within a top-level {@code cassandraYaml} key, + * the entire top-level key value is copied from the effective config into the overlay before + * applying the leaf change (copy-siblings strategy). This ensures the overlay is always + * self-contained at the top-level key granularity. + * + *

All operations are validated atomically: either all succeed or none are applied. + * + * @param instance the Cassandra instance metadata + * @param expectedHash the hash of the effective configuration as last seen by the caller + * @param operations the patch operations to apply + * @return a snapshot of the new effective configuration with its SHA-256 hash and last modified timestamp + * @throws ConfigurationConflictException if the expectedHash does not match the current effective hash + * @throws ConfigurationPatchException if any patch operation fails validation or application + * @throws ConfigurationManagerException if the provider fails during the operation + */ + @NotNull + public ConfigurationOverlaySnapshot patchConfiguration(@NotNull InstanceMetadata instance, + @NotNull String expectedHash, + @NotNull List operations) + { + Objects.requireNonNull(instance, "instance must not be null"); + Objects.requireNonNull(expectedHash, "expectedHash must not be null"); + Objects.requireNonNull(operations, "operations must not be null"); + + try + { + // 1. Validate operations structure (path format, value presence, duplicates) + List parsedOps = + ConfigurationPatchValidator.validate(operations); + + // 2. Read current state and validate the caller's hash matches + ConfigurationOverlaySnapshot baseSnapshot = getBaseSnapshot(); + ConfigurationOverlaySnapshot currentOverlay = provider.getOverlay(instance); + + ConfigurationOverlaySnapshot effectiveConfig = currentOverlay != null + ? baseSnapshot.overlay(currentOverlay, instance.id()) + : baseSnapshot; + + if (!expectedHash.equals(effectiveConfig.hash())) + { + throw new ConfigurationConflictException(expectedHash, effectiveConfig.hash()); + } + + // 3. Apply patch operations to the overlay (precondition checks + mutations) + CassandraConfigurationOverlay currentOverlayConfig = currentOverlay != null + ? currentOverlay.configuration() + : new CassandraConfigurationOverlay(null, null); + CassandraConfigurationOverlay updatedOverlay = + ConfigurationPatchApplier.apply(parsedOps, effectiveConfig.configuration(), currentOverlayConfig); + ConfigurationOverlaySnapshot newOverlaySnapshot = new ConfigurationOverlaySnapshot(Instant.now(), + updatedOverlay); + + // 4. CAS via provider — concurrent patches are resolved by the provider's own atomicity + String currentOverlayHash = currentOverlay != null ? currentOverlay.hash() : null; + boolean stored = provider.storeOverlay(instance, currentOverlayHash, newOverlaySnapshot); + + // 5. Store rejected — re-read to determine if this is a true conflict or an unexpected failure + if (!stored) + { + ConfigurationOverlaySnapshot updatedCurrent = provider.getOverlay(instance); + ConfigurationOverlaySnapshot newEffective = updatedCurrent != null + ? baseSnapshot.overlay(updatedCurrent, instance.id()) + : baseSnapshot; + if (!expectedHash.equals(newEffective.hash())) + { + throw new ConfigurationConflictException(expectedHash, newEffective.hash()); + } + throw new ConfigurationManagerException( + "Provider rejected the overlay store unexpectedly", null); + } + + return baseSnapshot.overlay(newOverlaySnapshot, instance.id()); + } + catch (ConfigurationManagerException e) + { + throw e; + } + catch (Exception e) + { + throw new ConfigurationManagerException("Failed to patch configuration", e); + } + } } diff --git a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplier.java b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplier.java new file mode 100644 index 000000000..db57ae9c8 --- /dev/null +++ b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplier.java @@ -0,0 +1,451 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.sidecar.configmanagement; + +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; + +import io.vertx.core.json.JsonObject; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +import static org.apache.cassandra.sidecar.configmanagement.ConfigurationPatchValidator.CASSANDRA_YAML_SECTION; +import static org.apache.cassandra.sidecar.configmanagement.ConfigurationPatchValidator.EXTRA_JVM_OPTS_SECTION; + +/** + * Applies validated patch operations to the configuration overlay. + * + *

Paths reference the effective configuration (base + overlay merged), + * but mutations target the overlay only. For nested paths within a top-level {@code cassandraYaml} key, + * the entire top-level key value is copied from the effective config into the overlay before applying + * the leaf change (copy-siblings strategy). + * + *

All precondition checks are performed before any mutations, ensuring atomicity: + * either all operations succeed or none are applied. + */ +public final class ConfigurationPatchApplier +{ + private ConfigurationPatchApplier() + { + throw new UnsupportedOperationException(); + } + + /** + * Applies a list of validated patch operations and returns the updated overlay. + * + * @param parsedOps the validated and parsed operations + * @param effectiveConfig the current effective configuration (base + overlay merged) + * @param currentOverlay the current overlay (may be empty) + * @return the updated overlay with all operations applied + * @throws ConfigurationPatchException if any precondition check fails + */ + @NotNull + public static CassandraConfigurationOverlay apply( + @NotNull List parsedOps, + @NotNull CassandraConfigurationOverlay effectiveConfig, + @NotNull CassandraConfigurationOverlay currentOverlay) + { + // Phase 1: Precondition checks (no mutations) + for (ConfigurationPatchValidator.ParsedPatchOperation parsed : parsedOps) + { + checkPreconditions(parsed, effectiveConfig, currentOverlay); + } + + // Phase 2: Apply mutations + JsonObject updatedYaml = currentOverlay.cassandraYaml().copy(); + Map updatedOpts = new LinkedHashMap<>(currentOverlay.extraJvmOpts()); + + for (ConfigurationPatchValidator.ParsedPatchOperation parsed : parsedOps) + { + applyMutation(parsed, effectiveConfig, updatedYaml, updatedOpts); + } + + return new CassandraConfigurationOverlay(updatedYaml, updatedOpts); + } + + private static void checkPreconditions(ConfigurationPatchValidator.ParsedPatchOperation parsed, + CassandraConfigurationOverlay effectiveConfig, + CassandraConfigurationOverlay currentOverlay) + { + if (parsed.section().equals(CASSANDRA_YAML_SECTION)) + { + checkYamlPreconditions(parsed, effectiveConfig.cassandraYaml(), currentOverlay.cassandraYaml()); + } + else if (parsed.section().equals(EXTRA_JVM_OPTS_SECTION)) + { + checkJvmOptsPreconditions(parsed, effectiveConfig.extraJvmOpts(), currentOverlay.extraJvmOpts()); + } + } + + private static void checkYamlPreconditions(ConfigurationPatchValidator.ParsedPatchOperation parsed, + JsonObject effectiveYaml, + JsonObject overlayYaml) + { + ConfigurationPatchOperation op = parsed.operation(); + + switch (op.op()) + { + case TEST: + checkNestedPathTraversable(effectiveYaml, parsed); + Object actualValue = resolveValue(effectiveYaml, parsed.topLevelKey(), parsed.nestedSegments()); + if (actualValue == null) + { + throw new ConfigurationPatchException( + "Test failed: path does not exist in effective config: '" + op.path() + "'", op); + } + if (!valueEquals(actualValue, op.value())) + { + throw new ConfigurationPatchException( + "Test failed: expected " + op.value() + " but found " + actualValue + + " at path '" + op.path() + "'", op); + } + break; + + case REPLACE: + checkNestedPathTraversable(effectiveYaml, parsed); + Object existingValue = resolveValue(effectiveYaml, parsed.topLevelKey(), parsed.nestedSegments()); + if (existingValue == null) + { + throw new ConfigurationPatchException( + "Replace failed: path does not exist in effective config: '" + op.path() + "'", op); + } + break; + + case REMOVE: + checkNestedPathTraversable(overlayYaml, parsed); + Object overlayValue = resolveValue(overlayYaml, parsed.topLevelKey(), parsed.nestedSegments()); + if (overlayValue == null) + { + throw new ConfigurationPatchException( + "Remove failed: path does not exist in overlay (may only exist in base template): '" + + op.path() + "'", op); + } + break; + + case ADD: + if (parsed.isNested()) + { + // Parent must exist in effective config (RFC 6902) + List parentSegments = parsed.nestedSegments() + .subList(0, parsed.nestedSegments().size() - 1); + checkNestedPathTraversable(effectiveYaml, parsed); + Object parent = resolveValue(effectiveYaml, parsed.topLevelKey(), parentSegments); + if (parent == null && !parentSegments.isEmpty()) + { + throw new ConfigurationPatchException( + "Add failed: parent path does not exist in effective config: '" + op.path() + "'", op); + } + if (parent != null && !(parent instanceof JsonObject) && !(parent instanceof Map)) + { + throw new ConfigurationPatchException( + "Add failed: parent is not an object at path: '" + op.path() + "'", op); + } + } + break; + } + } + + /** + * Validates that the nested path can be traversed without hitting a non-object (e.g., an array) + * at an intermediate segment. Throws a clear error if a segment resolves to a value that + * cannot be traversed further. + */ + private static void checkNestedPathTraversable(JsonObject yaml, + ConfigurationPatchValidator.ParsedPatchOperation parsed) + { + if (!parsed.isNested()) + { + return; + } + ConfigurationPatchOperation op = parsed.operation(); + Object current = yaml.getValue(parsed.topLevelKey()); + if (current == null) + { + return; // will be caught by subsequent resolveValue null check + } + + for (int i = 0; i < parsed.nestedSegments().size() - 1; i++) + { + JsonObject currentObj = asJsonObject(current); + if (currentObj == null) + { + throw new ConfigurationPatchException( + "Path traversal failed: intermediate value is not an object at path: '" + + op.path() + "'", op); + } + current = currentObj.getValue(parsed.nestedSegments().get(i)); + if (current == null) + { + return; // will be caught by subsequent resolveValue null check + } + } + } + + private static void checkJvmOptsPreconditions(ConfigurationPatchValidator.ParsedPatchOperation parsed, + Map effectiveOpts, + Map overlayOpts) + { + ConfigurationPatchOperation op = parsed.operation(); + String key = parsed.topLevelKey(); + + switch (op.op()) + { + case TEST: + if (!effectiveOpts.containsKey(key)) + { + throw new ConfigurationPatchException( + "Test failed: key does not exist in effective extraJvmOpts: '" + op.path() + "'", op); + } + String actual = effectiveOpts.get(key); + if (!Objects.equals(actual, op.value())) + { + throw new ConfigurationPatchException( + "Test failed: expected '" + op.value() + "' but found '" + actual + + "' at path '" + op.path() + "'", op); + } + break; + + case REPLACE: + if (!effectiveOpts.containsKey(key)) + { + throw new ConfigurationPatchException( + "Replace failed: key does not exist in effective extraJvmOpts: '" + op.path() + "'", op); + } + break; + + case REMOVE: + if (!overlayOpts.containsKey(key)) + { + throw new ConfigurationPatchException( + "Remove failed: key does not exist in overlay extraJvmOpts " + + "(may only exist in base template): '" + op.path() + "'", op); + } + break; + + case ADD: + // No precondition — extraJvmOpts is flat, parent always exists + break; + } + } + + private static void applyMutation(ConfigurationPatchValidator.ParsedPatchOperation parsed, + CassandraConfigurationOverlay effectiveConfig, + JsonObject updatedYaml, + Map updatedOpts) + { + ConfigurationPatchOperation op = parsed.operation(); + + if (op.op() == ConfigurationPatchOperation.Op.TEST) + { + return; // test is read-only + } + + if (parsed.section().equals(CASSANDRA_YAML_SECTION)) + { + applyYamlMutation(parsed, effectiveConfig.cassandraYaml(), updatedYaml); + } + else if (parsed.section().equals(EXTRA_JVM_OPTS_SECTION)) + { + applyJvmOptsMutation(parsed, updatedOpts); + } + } + + private static void applyYamlMutation(ConfigurationPatchValidator.ParsedPatchOperation parsed, + JsonObject effectiveYaml, + JsonObject updatedYaml) + { + ConfigurationPatchOperation op = parsed.operation(); + + if (!parsed.isNested()) + { + // Top-level key: direct put/remove on overlay yaml + if (op.op() == ConfigurationPatchOperation.Op.REMOVE) + { + updatedYaml.remove(parsed.topLevelKey()); + } + else + { + updatedYaml.put(parsed.topLevelKey(), op.value()); + } + } + else if (op.op() == ConfigurationPatchOperation.Op.REMOVE) + { + // Nested remove: modify the existing overlay subtree in-place. + // The precondition check already verified the leaf exists in the overlay. + // updatedYaml is already a deep copy so in-place mutation is safe. + JsonObject topLevel = asJsonObject(updatedYaml.getValue(parsed.topLevelKey())); + if (topLevel != null) + { + removeAtPath(topLevel, parsed.nestedSegments()); + } + } + else + { + // Nested add/replace: copy-siblings strategy. + // If a prior op in this batch already copied the top-level key into the overlay, + // operate on that copy. Otherwise, copy from effective config to ensure the overlay + // is self-contained at the top-level key granularity. + JsonObject topLevel; + if (updatedYaml.containsKey(parsed.topLevelKey())) + { + topLevel = asJsonObject(updatedYaml.getValue(parsed.topLevelKey())); + } + else + { + topLevel = deepCopyValue(effectiveYaml.getValue(parsed.topLevelKey())); + } + if (topLevel == null) + { + topLevel = new JsonObject(); + } + setAtPath(topLevel, parsed.nestedSegments(), op.value()); + updatedYaml.put(parsed.topLevelKey(), topLevel); + } + } + + private static void applyJvmOptsMutation(ConfigurationPatchValidator.ParsedPatchOperation parsed, + Map updatedOpts) + { + ConfigurationPatchOperation op = parsed.operation(); + String key = parsed.topLevelKey(); + + if (op.op() == ConfigurationPatchOperation.Op.REMOVE) + { + updatedOpts.remove(key); + } + else + { + if (CassandraConfigurationOverlay.hasConflictingBooleanOpt(updatedOpts, key)) + { + String conflicting = CassandraConfigurationOverlay.conflictingBooleanOpt(key); + throw new ConfigurationPatchException( + "Conflicting boolean JVM option: '" + key + "' conflicts with existing '" + + conflicting + "'", op); + } + updatedOpts.put(key, String.valueOf(op.value())); + } + } + + /** + * Resolves a value at the given path within a JsonObject. + * + * @param root the root object to traverse + * @param topLevelKey the first key to look up + * @param nestedSegments remaining path segments after the top-level key + * @return the value at the path, or {@code null} if any segment is missing + */ + @Nullable + @SuppressWarnings("unchecked") + static Object resolveValue(JsonObject root, String topLevelKey, List nestedSegments) + { + Object current = root.getValue(topLevelKey); + if (current == null) + { + return null; + } + + for (String segment : nestedSegments) + { + JsonObject obj = asJsonObject(current); + if (obj == null) + { + return null; + } + current = obj.getValue(segment); + if (current == null) + { + return null; + } + } + return current; + } + + private static void setAtPath(JsonObject root, List segments, Object value) + { + JsonObject parent = root; + for (int i = 0; i < segments.size() - 1; i++) + { + Object child = parent.getValue(segments.get(i)); + JsonObject childObj = asJsonObject(child); + if (childObj == null) + { + childObj = new JsonObject(); + parent.put(segments.get(i), childObj); + } + parent = childObj; + } + parent.put(segments.get(segments.size() - 1), value); + } + + private static void removeAtPath(JsonObject root, List segments) + { + JsonObject parent = root; + for (int i = 0; i < segments.size() - 1; i++) + { + Object child = parent.getValue(segments.get(i)); + JsonObject childObj = asJsonObject(child); + if (childObj == null) + { + return; + } + parent = childObj; + } + parent.remove(segments.get(segments.size() - 1)); + } + + @SuppressWarnings("unchecked") + @Nullable + private static JsonObject asJsonObject(@Nullable Object value) + { + if (value instanceof JsonObject) + { + return (JsonObject) value; + } + if (value instanceof Map) + { + return new JsonObject((Map) value); + } + return null; + } + + @SuppressWarnings("unchecked") + private static JsonObject deepCopyValue(@Nullable Object value) + { + if (value instanceof JsonObject) + { + return ((JsonObject) value).copy(); + } + if (value instanceof Map) + { + return new JsonObject((Map) value).copy(); + } + return new JsonObject(); + } + + private static boolean valueEquals(@NotNull Object actual, @Nullable Object expected) + { + if (actual instanceof JsonObject && expected instanceof Map) + { + return actual.equals(new JsonObject((Map) expected)); + } + return Objects.equals(actual, expected); + } +} diff --git a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchException.java b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchException.java new file mode 100644 index 000000000..6a234ba9e --- /dev/null +++ b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchException.java @@ -0,0 +1,45 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.sidecar.configmanagement; + +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +/** + * Thrown when a configuration patch operation fails validation or application. + * Contains the failing operation for error reporting. + */ +public class ConfigurationPatchException extends ConfigurationManagerException +{ + @Nullable + private final ConfigurationPatchOperation failedOperation; + + public ConfigurationPatchException(@NotNull String message, + @Nullable ConfigurationPatchOperation failedOperation) + { + super(message, null); + this.failedOperation = failedOperation; + } + + @Nullable + public ConfigurationPatchOperation failedOperation() + { + return failedOperation; + } +} diff --git a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchOperation.java b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchOperation.java new file mode 100644 index 000000000..581abb600 --- /dev/null +++ b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchOperation.java @@ -0,0 +1,117 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.sidecar.configmanagement; + +import java.util.Objects; + +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +/** + * Represents a single JSON Patch operation (RFC 6902-inspired) for configuration updates. + * Supported operations: {@code add}, {@code remove}, {@code replace}, {@code test}. + * + *

Paths use JSON Pointer syntax (RFC 6901) and reference the effective configuration structure. + * Operations are interpreted as overlay mutations: + *

+ */ +public class ConfigurationPatchOperation +{ + /** + * The supported patch operations. + */ + public enum Op + { + ADD, + REMOVE, + REPLACE, + TEST + } + + @NotNull + private final Op op; + + @NotNull + private final String path; + + @Nullable + private final Object value; + + /** + * @param op the operation type + * @param path the JSON Pointer path (e.g. "/configuration/cassandraYaml/concurrent_reads") + * @param value the value for add/replace/test operations; must be {@code null} for remove + */ + public ConfigurationPatchOperation(@NotNull Op op, @NotNull String path, @Nullable Object value) + { + this.op = Objects.requireNonNull(op, "op must not be null"); + this.path = Objects.requireNonNull(path, "path must not be null"); + this.value = value; + } + + @NotNull + public Op op() + { + return op; + } + + @NotNull + public String path() + { + return path; + } + + @Nullable + public Object value() + { + return value; + } + + @Override + public String toString() + { + return "ConfigurationPatchOperation{op=" + op + ", path='" + path + "', value=" + value + '}'; + } + + @Override + public boolean equals(Object o) + { + if (this == o) + { + return true; + } + if (o == null || getClass() != o.getClass()) + { + return false; + } + ConfigurationPatchOperation that = (ConfigurationPatchOperation) o; + return op == that.op && Objects.equals(path, that.path) && Objects.equals(value, that.value); + } + + @Override + public int hashCode() + { + return Objects.hash(op, path, value); + } +} diff --git a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidator.java b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidator.java new file mode 100644 index 000000000..d6802f2ab --- /dev/null +++ b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidator.java @@ -0,0 +1,283 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.sidecar.configmanagement; + +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.regex.Pattern; + +import org.jetbrains.annotations.NotNull; + +/** + * Validates a list of {@link ConfigurationPatchOperation} instances before application. + * + *

Validation includes: + *

+ */ +public final class ConfigurationPatchValidator +{ + static final String PATH_PREFIX = "/configuration/"; + static final String CASSANDRA_YAML_SECTION = "cassandraYaml"; + static final String EXTRA_JVM_OPTS_SECTION = "extraJvmOpts"; + static final String CASSANDRA_YAML_PREFIX = PATH_PREFIX + CASSANDRA_YAML_SECTION + "/"; + static final String EXTRA_JVM_OPTS_PREFIX = PATH_PREFIX + EXTRA_JVM_OPTS_SECTION + "/"; + + // Allows: -Dproperty.name, -Xmx, -Xss, -XX:+Flag, -XX:-Flag, -XX:Flag + // Rejects: -javaagent, -agentpath, -agentlib, keys with = or shell metacharacters + static final Pattern JVM_OPT_KEY_PATTERN = Pattern.compile( + "^-(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 BLOCKED_XX_FLAGS = Set.of( + "-XX:OnOutOfMemoryError", + "-XX:OnError"); + + // 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}$"); + + // Matches /../ path traversal sequences (start, middle, or end of path) + static final Pattern PATH_TRAVERSAL_PATTERN = Pattern.compile("(?:^|/)\\.\\.(?:/|$)"); + + private ConfigurationPatchValidator() + { + throw new UnsupportedOperationException(); + } + + /** + * Validates a list of patch operations and returns their parsed representations. + * + * @param operations the raw patch operations to validate + * @return the list of parsed operations with extracted section, top-level key, and nested segments + * @throws ConfigurationPatchException if any validation check fails + */ + @NotNull + public static List validate(@NotNull List operations) + { + if (operations.isEmpty()) + { + throw new ConfigurationPatchException("Patch operations list must not be empty", null); + } + + Set seenMutationPaths = new HashSet<>(); + List parsed = new ArrayList<>(operations.size()); + + for (ConfigurationPatchOperation op : operations) + { + validateValuePresence(op); + ParsedPatchOperation parsedOp = parsePath(op); + // Only mutation ops (add/remove/replace) are checked for duplicates. + // test is read-only and may target the same path as a subsequent mutation. + if (op.op() != ConfigurationPatchOperation.Op.TEST && !seenMutationPaths.add(op.path())) + { + throw new ConfigurationPatchException( + "Duplicate path in patch: '" + op.path() + "'", op); + } + parsed.add(parsedOp); + } + return parsed; + } + + private static void validateValuePresence(ConfigurationPatchOperation op) + { + switch (op.op()) + { + case ADD: + case REPLACE: + case TEST: + if (op.value() == null) + { + throw new ConfigurationPatchException( + "Operation '" + op.op() + "' requires a value", op); + } + break; + case REMOVE: + if (op.value() != null) + { + throw new ConfigurationPatchException( + "Operation 'REMOVE' must not have a value", op); + } + break; + } + } + + private static ParsedPatchOperation parsePath(ConfigurationPatchOperation op) + { + String path = op.path(); + + if (path.startsWith(CASSANDRA_YAML_PREFIX)) + { + String remaining = path.substring(CASSANDRA_YAML_PREFIX.length()); + List segments = parsePointerSegments(remaining, op); + if (segments.isEmpty()) + { + throw new ConfigurationPatchException( + "Path must specify at least one key after section: '" + path + "'", op); + } + String topLevelKey = segments.get(0); + List nestedSegments = segments.size() > 1 ? segments.subList(1, segments.size()) + : List.of(); + return new ParsedPatchOperation(op, CASSANDRA_YAML_SECTION, topLevelKey, nestedSegments); + } + else if (path.startsWith(EXTRA_JVM_OPTS_PREFIX)) + { + String remaining = path.substring(EXTRA_JVM_OPTS_PREFIX.length()); + List segments = parsePointerSegments(remaining, op); + if (segments.isEmpty()) + { + throw new ConfigurationPatchException( + "Path must specify at least one key after section: '" + path + "'", op); + } + if (segments.size() > 1) + { + throw new ConfigurationPatchException( + "extraJvmOpts paths must be flat (no nested segments): '" + path + "'", op); + } + String jvmOptKey = segments.get(0); + validateJvmOptKey(jvmOptKey, op); + if (op.value() != null) + { + validateJvmOptValue(String.valueOf(op.value()), op); + } + return new ParsedPatchOperation(op, EXTRA_JVM_OPTS_SECTION, jvmOptKey, List.of()); + } + else + { + throw new ConfigurationPatchException( + "Path must start with '/configuration/cassandraYaml/' or " + + "'/configuration/extraJvmOpts/': '" + path + "'", op); + } + } + + private static void validateJvmOptKey(String key, ConfigurationPatchOperation op) + { + if (!JVM_OPT_KEY_PATTERN.matcher(key).matches()) + { + throw new ConfigurationPatchException( + "Invalid JVM option key '" + key + "': must be a valid JVM option " + + "(-Dproperty.name, -Xflag, or -XX:[+-]Flag)", op); + } + if (BLOCKED_XX_FLAGS.contains(key)) + { + throw new ConfigurationPatchException( + "Blocked extraJvmOpts key '" + key + "': this JVM option is not allowed", op); + } + } + + private static void validateJvmOptValue(String value, ConfigurationPatchOperation op) + { + if (!JVM_OPT_VALUE_PATTERN.matcher(value).matches()) + { + throw new ConfigurationPatchException( + "Invalid extraJvmOpts value: contains disallowed characters or exceeds 512 characters", op); + } + if (PATH_TRAVERSAL_PATTERN.matcher(value).find()) + { + throw new ConfigurationPatchException( + "Invalid extraJvmOpts value: path traversal ('..') is not allowed", op); + } + } + + /** + * Splits a path remainder into segments by {@code /}. + */ + static List parsePointerSegments(String pointer, ConfigurationPatchOperation op) + { + if (pointer.isEmpty()) + { + return List.of(); + } + + String[] parts = pointer.split("/", -1); + List segments = new ArrayList<>(parts.length); + for (String part : parts) + { + if (part.isEmpty()) + { + throw new ConfigurationPatchException( + "Path contains empty segment: '" + op.path() + "'", op); + } + segments.add(part); + } + return segments; + } + + /** + * A validated and parsed patch operation with its path decomposed into section, top-level key, + * and optional nested segments. + */ + public static class ParsedPatchOperation + { + private final ConfigurationPatchOperation operation; + private final String section; + private final String topLevelKey; + private final List nestedSegments; + + ParsedPatchOperation(@NotNull ConfigurationPatchOperation operation, + @NotNull String section, + @NotNull String topLevelKey, + @NotNull List nestedSegments) + { + this.operation = operation; + this.section = section; + this.topLevelKey = topLevelKey; + this.nestedSegments = nestedSegments; + } + + @NotNull + public ConfigurationPatchOperation operation() + { + return operation; + } + + /** Either "cassandraYaml" or "extraJvmOpts" */ + @NotNull + public String section() + { + return section; + } + + /** The first path segment after the section (e.g. "memtable", "concurrent_reads") */ + @NotNull + public String topLevelKey() + { + return topLevelKey; + } + + /** Segments after the top-level key; empty for flat paths */ + @NotNull + public List nestedSegments() + { + return nestedSegments; + } + + public boolean isNested() + { + return !nestedSegments.isEmpty(); + } + } +} diff --git a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationProvider.java b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationProvider.java index 86fcb3b81..e639eef7c 100644 --- a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationProvider.java +++ b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationProvider.java @@ -31,7 +31,7 @@ * *

The provider stores version-agnostic overlays and does not perform version-specific * validation or merge logic. Validation against a version-aware schema and computing updated - * overlays (via {@link CassandraConfigurationOverlay#updated}) are the responsibility of the + * overlays (via {@link ConfigurationPatchApplier}) are the responsibility of the * Configuration Manager. */ public interface ConfigurationProvider @@ -50,8 +50,8 @@ public interface ConfigurationProvider * subject to hash-based optimistic concurrency control. * *

The caller is responsible for computing the new snapshot (via - * {@link CassandraConfigurationOverlay#updated}). The provider only validates - * the original hash against the currently stored version and persists the result. + * {@link ConfigurationPatchApplier}). The provider only validates the original + * hash against the currently stored version and persists the result. * * @param instance the Cassandra instance metadata * @param originalHash the overlay hash from the previously read snapshot, diff --git a/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/CassandraConfigurationOverlayTest.java b/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/CassandraConfigurationOverlayTest.java index 26f9f886d..450f5b5a0 100644 --- a/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/CassandraConfigurationOverlayTest.java +++ b/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/CassandraConfigurationOverlayTest.java @@ -18,8 +18,6 @@ package org.apache.cassandra.sidecar.configmanagement; -import java.util.Collections; -import java.util.LinkedHashMap; import java.util.Map; import org.junit.jupiter.api.Test; @@ -27,7 +25,6 @@ import io.vertx.core.json.JsonObject; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; /** * Tests for {@link CassandraConfigurationOverlay} @@ -35,117 +32,40 @@ class CassandraConfigurationOverlayTest { @Test - void testUpdatedAppliesCassandraYamlChanges() + void testConstructorDeepCopiesCassandraYaml() { - JsonObject yaml = new JsonObject() - .put("concurrent_reads", 32) - .put("memtable_flush_writers", 4) - .put("storage_compatibility_mode", "CASSANDRA_4"); + JsonObject yaml = new JsonObject().put("concurrent_reads", 32); CassandraConfigurationOverlay overlay = new CassandraConfigurationOverlay(yaml, null); - Map updates = new LinkedHashMap<>(); - updates.put("concurrent_reads", 64); - updates.put("storage_compatibility_mode", null); - - CassandraConfigurationOverlay updated = overlay.updated(updates, null); - - // concurrent_reads updated - assertThat(updated.cassandraYaml().getInteger("concurrent_reads")).isEqualTo(64); - // storage_compatibility_mode removed - assertThat(updated.cassandraYaml().containsKey("storage_compatibility_mode")).isFalse(); - // memtable_flush_writers preserved - assertThat(updated.cassandraYaml().getInteger("memtable_flush_writers")).isEqualTo(4); - } - - @Test - void testUpdatedUpsertsAndRemovesJvmOpts() - { - Map jvmOpts = new LinkedHashMap<>(); - jvmOpts.put("-Dcassandra.jmx.local.port", "7199"); - jvmOpts.put("-Xmx", "4G"); - CassandraConfigurationOverlay overlay = new CassandraConfigurationOverlay(null, jvmOpts); - - Map updates = new LinkedHashMap<>(); - updates.put("-Xmx", "8G"); - - CassandraConfigurationOverlay updated = overlay.updated(null, updates); - - Map expected = new LinkedHashMap<>(); - expected.put("-Dcassandra.jmx.local.port", "7199"); - expected.put("-Xmx", "8G"); - assertThat(updated.extraJvmOpts()).containsExactlyEntriesOf(expected); - } - - @Test - void testUpdatedRemovesJvmOptWithNullValue() - { - Map jvmOpts = new LinkedHashMap<>(); - jvmOpts.put("-Dcassandra.jmx.local.port", "7199"); - jvmOpts.put("-Xmx", "4G"); - CassandraConfigurationOverlay overlay = new CassandraConfigurationOverlay(null, jvmOpts); - - Map updates = new LinkedHashMap<>(); - updates.put("-Xmx", null); - - CassandraConfigurationOverlay updated = overlay.updated(null, updates); - - assertThat(updated.extraJvmOpts()).containsExactlyEntriesOf(Map.of( - "-Dcassandra.jmx.local.port", "7199")); - } - - @Test - void testUpdatedRejectsConflictingBooleanOpts() - { - CassandraConfigurationOverlay overlay = new CassandraConfigurationOverlay(null, Map.of("-XX:+UseG1GC", "")); - - Map updates = new LinkedHashMap<>(); - updates.put("-XX:-UseG1GC", ""); - - assertThatThrownBy(() -> overlay.updated(null, updates)) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("-XX:+UseG1GC") - .hasMessageContaining("-XX:-UseG1GC"); - } - - @Test - void testUpdatedAllowsReplacingBooleanOpt() - { - CassandraConfigurationOverlay overlay = new CassandraConfigurationOverlay(null, Map.of("-XX:+UseG1GC", "")); - - Map updates = new LinkedHashMap<>(); - updates.put("-XX:+UseG1GC", null); - updates.put("-XX:-UseG1GC", ""); - - CassandraConfigurationOverlay updated = overlay.updated(null, updates); + yaml.put("concurrent_reads", 64); - assertThat(updated.extraJvmOpts()).containsExactlyEntriesOf(Map.of("-XX:-UseG1GC", "")); + assertThat(overlay.cassandraYaml().getInteger("concurrent_reads")).isEqualTo(32); } @Test - void testConstructorDeepCopiesCassandraYaml() + void testFromJsonRoundTrip() { - JsonObject yaml = new JsonObject().put("concurrent_reads", 32); - CassandraConfigurationOverlay overlay = new CassandraConfigurationOverlay(yaml, null); + JsonObject yaml = new JsonObject() + .put("concurrent_reads", 32) + .put("commitlog_sync", "periodic"); + CassandraConfigurationOverlay overlay = new CassandraConfigurationOverlay(yaml, Map.of("-Xmx", "4G")); - yaml.put("concurrent_reads", 64); + CassandraConfigurationOverlay fromJson = CassandraConfigurationOverlay.fromJson(overlay.toJson()); - assertThat(overlay.cassandraYaml().getInteger("concurrent_reads")).isEqualTo(32); + assertThat(fromJson.cassandraYaml().getInteger("concurrent_reads")).isEqualTo(32); + assertThat(fromJson.cassandraYaml().getString("commitlog_sync")).isEqualTo("periodic"); + assertThat(fromJson.extraJvmOpts()).containsEntry("-Xmx", "4G"); } @Test - void testUpdatedReturnsNewInstance() + void testEqualsAndHashCode() { JsonObject yaml = new JsonObject().put("concurrent_reads", 32); - CassandraConfigurationOverlay overlay = new CassandraConfigurationOverlay(yaml, Map.of("-Xmx", "4G")); + CassandraConfigurationOverlay a = new CassandraConfigurationOverlay(yaml, Map.of("-Xmx", "4G")); + CassandraConfigurationOverlay b = new CassandraConfigurationOverlay(yaml.copy(), Map.of("-Xmx", "4G")); - CassandraConfigurationOverlay updated = overlay.updated( - Collections.singletonMap("concurrent_reads", 64), - null); - - assertThat(updated).isNotSameAs(overlay); - assertThat(updated.cassandraYaml().getInteger("concurrent_reads")).isEqualTo(64); - // Original is not modified by updated() — deep copy used internally - assertThat(overlay.cassandraYaml().getInteger("concurrent_reads")).isEqualTo(32); + assertThat(a).isEqualTo(b); + assertThat(a.hashCode()).isEqualTo(b.hashCode()); } @Test diff --git a/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationManagerTest.java b/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationManagerTest.java index 49080915a..96137f0fa 100644 --- a/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationManagerTest.java +++ b/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationManagerTest.java @@ -23,15 +23,23 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.time.Instant; +import java.util.ArrayList; import java.util.Collections; import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.atomic.AtomicInteger; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import io.vertx.core.json.JsonObject; import org.apache.cassandra.sidecar.cluster.instance.InstanceMetadata; +import org.jetbrains.annotations.NotNull; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -173,6 +181,406 @@ void testGetEffectiveConfigurationCachesBaseSnapshot() assertThat(second).isSameAs(first); } + @Test + void testPatchAddTopLevelKey() + { + ConfigurationManager manager = new ConfigurationManager(provider, BASE_TEMPLATE); + InstanceMetadata instance = mockInstance(1); + + ConfigurationOverlaySnapshot baseEffective = manager.getEffectiveConfiguration(instance); + String baseHash = baseEffective.hash(); + + List ops = List.of( + new ConfigurationPatchOperation(ConfigurationPatchOperation.Op.ADD, + "/configuration/cassandraYaml/concurrent_reads", 64)); + + ConfigurationOverlaySnapshot result = manager.patchConfiguration(instance, baseHash, ops); + + assertThat(result.configuration().cassandraYaml().getInteger("concurrent_reads")).isEqualTo(64); + assertThat(result.configuration().cassandraYaml().getString("cluster_name")).isEqualTo("Test Cluster"); + assertThat(result.hash()).startsWith("sha256:"); + assertThat(result.hash()).isNotEqualTo(baseHash); + } + + @Test + void testPatchAddJvmOpt() + { + ConfigurationManager manager = new ConfigurationManager(provider, BASE_TEMPLATE); + InstanceMetadata instance = mockInstance(1); + String baseHash = manager.getEffectiveConfiguration(instance).hash(); + + List ops = List.of( + new ConfigurationPatchOperation(ConfigurationPatchOperation.Op.ADD, + "/configuration/extraJvmOpts/-Xmx", "8g")); + + ConfigurationOverlaySnapshot result = manager.patchConfiguration(instance, baseHash, ops); + + assertThat(result.configuration().extraJvmOpts()).containsEntry("-Xmx", "8g"); + } + + @Test + void testPatchRemoveTopLevelKeyFromOverlay() + { + InstanceMetadata instance = mockInstance(1); + + JsonObject initialYaml = new JsonObject().put("concurrent_reads", 128); + CassandraConfigurationOverlay initial = new CassandraConfigurationOverlay(initialYaml, null); + provider.storeOverlay(instance, null, new ConfigurationOverlaySnapshot(Instant.now(), initial)); + + ConfigurationManager manager = new ConfigurationManager(provider, BASE_TEMPLATE); + String effectiveHash = manager.getEffectiveConfiguration(instance).hash(); + + List ops = List.of( + new ConfigurationPatchOperation(ConfigurationPatchOperation.Op.REMOVE, + "/configuration/cassandraYaml/concurrent_reads", null)); + + ConfigurationOverlaySnapshot result = manager.patchConfiguration(instance, effectiveHash, ops); + + // Falls back to base template value + assertThat(result.configuration().cassandraYaml().getInteger("concurrent_reads")).isEqualTo(32); + } + + @Test + void testPatchRemoveTemplateOnlyKeyFails() + { + ConfigurationManager manager = new ConfigurationManager(provider, BASE_TEMPLATE); + InstanceMetadata instance = mockInstance(1); + String baseHash = manager.getEffectiveConfiguration(instance).hash(); + + List ops = List.of( + new ConfigurationPatchOperation(ConfigurationPatchOperation.Op.REMOVE, + "/configuration/cassandraYaml/cluster_name", null)); + + assertThatThrownBy(() -> manager.patchConfiguration(instance, baseHash, ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("does not exist in overlay"); + } + + @Test + void testPatchReplaceExistingKey() + { + ConfigurationManager manager = new ConfigurationManager(provider, BASE_TEMPLATE); + InstanceMetadata instance = mockInstance(1); + String baseHash = manager.getEffectiveConfiguration(instance).hash(); + + // concurrent_reads=32 exists in base template + List ops = List.of( + new ConfigurationPatchOperation(ConfigurationPatchOperation.Op.REPLACE, + "/configuration/cassandraYaml/concurrent_reads", 256)); + + ConfigurationOverlaySnapshot result = manager.patchConfiguration(instance, baseHash, ops); + + assertThat(result.configuration().cassandraYaml().getInteger("concurrent_reads")).isEqualTo(256); + } + + @Test + void testPatchReplaceAbsentKeyFails() + { + ConfigurationManager manager = new ConfigurationManager(provider, BASE_TEMPLATE); + InstanceMetadata instance = mockInstance(1); + String baseHash = manager.getEffectiveConfiguration(instance).hash(); + + List ops = List.of( + new ConfigurationPatchOperation(ConfigurationPatchOperation.Op.REPLACE, + "/configuration/cassandraYaml/nonexistent_key", 42)); + + assertThatThrownBy(() -> manager.patchConfiguration(instance, baseHash, ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("does not exist in effective config"); + } + + @Test + void testPatchTestMatchingValue() + { + ConfigurationManager manager = new ConfigurationManager(provider, BASE_TEMPLATE); + InstanceMetadata instance = mockInstance(1); + String baseHash = manager.getEffectiveConfiguration(instance).hash(); + + List ops = List.of( + new ConfigurationPatchOperation(ConfigurationPatchOperation.Op.TEST, + "/configuration/cassandraYaml/concurrent_reads", 32), + new ConfigurationPatchOperation(ConfigurationPatchOperation.Op.ADD, + "/configuration/cassandraYaml/concurrent_reads", 128)); + + ConfigurationOverlaySnapshot result = manager.patchConfiguration(instance, baseHash, ops); + + assertThat(result.configuration().cassandraYaml().getInteger("concurrent_reads")).isEqualTo(128); + } + + @Test + void testPatchTestMismatchRejectsEntirePatch() + { + InstanceMetadata instance = mockInstance(1); + + JsonObject initialYaml = new JsonObject().put("concurrent_reads", 64); + CassandraConfigurationOverlay initial = new CassandraConfigurationOverlay(initialYaml, null); + provider.storeOverlay(instance, null, new ConfigurationOverlaySnapshot(Instant.now(), initial)); + + ConfigurationManager manager = new ConfigurationManager(provider, BASE_TEMPLATE); + String effectiveHash = manager.getEffectiveConfiguration(instance).hash(); + + List ops = List.of( + new ConfigurationPatchOperation(ConfigurationPatchOperation.Op.TEST, + "/configuration/cassandraYaml/concurrent_reads", 999), + new ConfigurationPatchOperation(ConfigurationPatchOperation.Op.ADD, + "/configuration/cassandraYaml/memtable_flush_writers", 16)); + + assertThatThrownBy(() -> manager.patchConfiguration(instance, effectiveHash, ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("Test failed"); + + // Verify no mutation occurred + ConfigurationOverlaySnapshot current = manager.getEffectiveConfiguration(instance); + assertThat(current.configuration().cassandraYaml().getInteger("concurrent_reads")).isEqualTo(64); + assertThat(current.configuration().cassandraYaml().containsKey("memtable_flush_writers")).isFalse(); + } + + @Test + void testPatchConflictStaleHash() + { + InstanceMetadata instance = mockInstance(1); + + JsonObject initialYaml = new JsonObject().put("concurrent_reads", 64); + CassandraConfigurationOverlay initial = new CassandraConfigurationOverlay(initialYaml, null); + provider.storeOverlay(instance, null, new ConfigurationOverlaySnapshot(Instant.now(), initial)); + + ConfigurationManager manager = new ConfigurationManager(provider, BASE_TEMPLATE); + String actualHash = manager.getEffectiveConfiguration(instance).hash(); + String staleHash = "sha256:0000000000000000000000000000000000000000000000000000000000000000"; + + List ops = List.of( + new ConfigurationPatchOperation(ConfigurationPatchOperation.Op.ADD, + "/configuration/cassandraYaml/concurrent_reads", 128)); + + assertThatThrownBy(() -> manager.patchConfiguration(instance, staleHash, ops)) + .isInstanceOf(ConfigurationConflictException.class) + .satisfies(e -> { + ConfigurationConflictException conflict = (ConfigurationConflictException) e; + assertThat(conflict.expectedHash()).isEqualTo(staleHash); + assertThat(conflict.actualHash()).isEqualTo(actualHash); + }); + + // Overlay unchanged + assertThat(provider.getOverlay(instance).configuration().cassandraYaml().getInteger("concurrent_reads")) + .isEqualTo(64); + } + + @Test + void testPatchStoreOverlayReturnsFalse() + { + InstanceMetadata instance = mockInstance(1); + + JsonObject initialYaml = new JsonObject().put("concurrent_reads", 64); + CassandraConfigurationOverlay initial = new CassandraConfigurationOverlay(initialYaml, null); + ConfigurationOverlaySnapshot initialSnapshot = new ConfigurationOverlaySnapshot(Instant.now(), initial); + + ConfigurationProvider rejectingProvider = new ConfigurationProvider() + { + private ConfigurationOverlaySnapshot stored = initialSnapshot; + + @Override + public ConfigurationOverlaySnapshot getOverlay(InstanceMetadata inst) + { + return stored; + } + + @Override + public boolean storeOverlay(InstanceMetadata inst, String originalHash, + ConfigurationOverlaySnapshot newSnapshot) + { + return false; + } + }; + + ConfigurationManager manager = new ConfigurationManager(rejectingProvider, BASE_TEMPLATE); + String effectiveHash = manager.getEffectiveConfiguration(instance).hash(); + + List ops = List.of( + new ConfigurationPatchOperation(ConfigurationPatchOperation.Op.ADD, + "/configuration/cassandraYaml/concurrent_reads", 128)); + + assertThatThrownBy(() -> manager.patchConfiguration(instance, effectiveHash, ops)) + .isInstanceOf(ConfigurationManagerException.class) + .isNotInstanceOf(ConfigurationConflictException.class) + .hasMessageContaining("Provider rejected the overlay store unexpectedly"); + } + + @Test + void testPatchStoreOverlayReturnsFalseWithConflict() + { + InstanceMetadata instance = mockInstance(1); + + JsonObject initialYaml = new JsonObject().put("concurrent_reads", 64); + CassandraConfigurationOverlay initial = new CassandraConfigurationOverlay(initialYaml, null); + ConfigurationOverlaySnapshot initialSnapshot = new ConfigurationOverlaySnapshot(Instant.now(), initial); + + // Overlay changes between storeOverlay rejection and re-read + JsonObject changedYaml = new JsonObject().put("concurrent_reads", 256); + CassandraConfigurationOverlay changed = new CassandraConfigurationOverlay(changedYaml, null); + ConfigurationOverlaySnapshot changedSnapshot = new ConfigurationOverlaySnapshot(Instant.now(), changed); + + AtomicInteger getOverlayCallCount = new AtomicInteger(0); + ConfigurationProvider conflictingProvider = new ConfigurationProvider() + { + @Override + public ConfigurationOverlaySnapshot getOverlay(InstanceMetadata inst) + { + // First call returns initial, second call (after store rejection) returns changed + return getOverlayCallCount.incrementAndGet() <= 1 ? initialSnapshot : changedSnapshot; + } + + @Override + public boolean storeOverlay(InstanceMetadata inst, String originalHash, + ConfigurationOverlaySnapshot newSnapshot) + { + return false; + } + }; + + ConfigurationManager manager = new ConfigurationManager(conflictingProvider, BASE_TEMPLATE); + String effectiveHash = manager.getEffectiveConfiguration(instance).hash(); + + List ops = List.of( + new ConfigurationPatchOperation(ConfigurationPatchOperation.Op.ADD, + "/configuration/cassandraYaml/concurrent_reads", 128)); + + assertThatThrownBy(() -> manager.patchConfiguration(instance, effectiveHash, ops)) + .isInstanceOf(ConfigurationConflictException.class) + .satisfies(e -> { + ConfigurationConflictException conflict = (ConfigurationConflictException) e; + assertThat(conflict.expectedHash()).isEqualTo(effectiveHash); + assertThat(conflict.actualHash()).isNotEqualTo(effectiveHash); + }); + } + + @Test + void testPatchProviderFailure() + { + ConfigurationProvider failingProvider = new ConfigurationProvider() + { + @Override + public ConfigurationOverlaySnapshot getOverlay(InstanceMetadata instance) + { + throw new UncheckedIOException(new IOException("provider unavailable")); + } + + @Override + public boolean storeOverlay(InstanceMetadata instance, String originalHash, + @NotNull ConfigurationOverlaySnapshot newSnapshot) + { + throw new UnsupportedOperationException(); + } + }; + + ConfigurationManager manager = new ConfigurationManager(failingProvider, BASE_TEMPLATE); + InstanceMetadata instance = mockInstance(1); + + List ops = List.of( + new ConfigurationPatchOperation(ConfigurationPatchOperation.Op.ADD, + "/configuration/cassandraYaml/concurrent_reads", 128)); + + assertThatThrownBy(() -> manager.patchConfiguration(instance, "sha256:abc", ops)) + .isInstanceOf(ConfigurationManagerException.class) + .isNotInstanceOf(ConfigurationConflictException.class) + .hasMessageContaining("Failed to patch configuration") + .hasCauseInstanceOf(UncheckedIOException.class); + } + + @Test + void testPatchConcurrentSameInstance() throws Exception + { + InstanceMetadata instance = mockInstance(1); + ConfigurationManager manager = new ConfigurationManager(provider, BASE_TEMPLATE); + String baseHash = manager.getEffectiveConfiguration(instance).hash(); + + int threadCount = 10; + ExecutorService executor = Executors.newFixedThreadPool(threadCount); + CountDownLatch startLatch = new CountDownLatch(1); + AtomicInteger successCount = new AtomicInteger(0); + AtomicInteger conflictCount = new AtomicInteger(0); + + List> futures = new ArrayList<>(); + for (int i = 0; i < threadCount; i++) + { + int value = i; + futures.add(executor.submit(() -> { + try + { + startLatch.await(); + List ops = List.of( + new ConfigurationPatchOperation(ConfigurationPatchOperation.Op.ADD, + "/configuration/cassandraYaml/concurrent_reads", value)); + manager.patchConfiguration(instance, baseHash, ops); + successCount.incrementAndGet(); + } + catch (ConfigurationConflictException e) + { + conflictCount.incrementAndGet(); + } + catch (InterruptedException e) + { + Thread.currentThread().interrupt(); + } + })); + } + + startLatch.countDown(); + for (Future future : futures) + { + future.get(); + } + executor.shutdown(); + + assertThat(successCount.get()).isEqualTo(1); + assertThat(conflictCount.get()).isEqualTo(threadCount - 1); + } + + @Test + void testPatchDuplicatePathsRejected() + { + ConfigurationManager manager = new ConfigurationManager(provider, BASE_TEMPLATE); + InstanceMetadata instance = mockInstance(1); + String baseHash = manager.getEffectiveConfiguration(instance).hash(); + + List ops = List.of( + new ConfigurationPatchOperation(ConfigurationPatchOperation.Op.ADD, + "/configuration/cassandraYaml/concurrent_reads", 64), + new ConfigurationPatchOperation(ConfigurationPatchOperation.Op.ADD, + "/configuration/cassandraYaml/concurrent_reads", 128)); + + assertThatThrownBy(() -> manager.patchConfiguration(instance, baseHash, ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("Duplicate path"); + } + + @Test + void testPatchInvalidPathFormat() + { + ConfigurationManager manager = new ConfigurationManager(provider, BASE_TEMPLATE); + InstanceMetadata instance = mockInstance(1); + String baseHash = manager.getEffectiveConfiguration(instance).hash(); + + List ops = List.of( + new ConfigurationPatchOperation(ConfigurationPatchOperation.Op.ADD, + "/invalid/path", 42)); + + assertThatThrownBy(() -> manager.patchConfiguration(instance, baseHash, ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("Path must start with"); + } + + @Test + void testPatchEmptyOperationsRejected() + { + ConfigurationManager manager = new ConfigurationManager(provider, BASE_TEMPLATE); + InstanceMetadata instance = mockInstance(1); + String baseHash = manager.getEffectiveConfiguration(instance).hash(); + + assertThatThrownBy(() -> manager.patchConfiguration(instance, baseHash, List.of())) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("must not be empty"); + } + private static InstanceMetadata mockInstance(int id) { InstanceMetadata instance = mock(InstanceMetadata.class); diff --git a/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplierTest.java b/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplierTest.java new file mode 100644 index 000000000..0a45ace8a --- /dev/null +++ b/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplierTest.java @@ -0,0 +1,434 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.sidecar.configmanagement; + +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import io.vertx.core.json.JsonObject; + +import static org.apache.cassandra.sidecar.configmanagement.ConfigurationPatchOperation.Op.ADD; +import static org.apache.cassandra.sidecar.configmanagement.ConfigurationPatchOperation.Op.REMOVE; +import static org.apache.cassandra.sidecar.configmanagement.ConfigurationPatchOperation.Op.REPLACE; +import static org.apache.cassandra.sidecar.configmanagement.ConfigurationPatchOperation.Op.TEST; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * Tests for {@link ConfigurationPatchApplier} + */ +class ConfigurationPatchApplierTest +{ + private static final CassandraConfigurationOverlay EMPTY_OVERLAY = new CassandraConfigurationOverlay(null, null); + + private CassandraConfigurationOverlay effectiveConfig; + + @BeforeEach + void setUp() + { + JsonObject effectiveYaml = new JsonObject() + .put("cluster_name", "TestCluster") + .put("concurrent_reads", 32) + .put("memtable", new JsonObject() + .put("configurations", new JsonObject() + .put("trie", new JsonObject() + .put("class_name", "TrieMemtable") + .put("max_shard_count", 4)) + .put("skiplist", new JsonObject() + .put("class_name", "SkipListMemtable")))); + + Map effectiveOpts = new LinkedHashMap<>(); + effectiveOpts.put("-Xmx", "4g"); + effectiveOpts.put("-Dcassandra.ring_delay_ms", "60000"); + + effectiveConfig = new CassandraConfigurationOverlay(effectiveYaml, effectiveOpts); + } + + // --- Top-level cassandraYaml operations --- + + @Test + void testAddTopLevelKey() + { + CassandraConfigurationOverlay newOverlay = applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(ADD, "/configuration/cassandraYaml/new_key", "new_value")); + + assertThat(newOverlay.cassandraYaml().getString("new_key")).isEqualTo("new_value"); + } + + @Test + void testAddOverwritesExistingOverlayValue() + { + CassandraConfigurationOverlay overlay = new CassandraConfigurationOverlay( + new JsonObject().put("concurrent_reads", 64), null); + + CassandraConfigurationOverlay newOverlay = applyOps(effectiveConfig, overlay, + new ConfigurationPatchOperation(ADD, "/configuration/cassandraYaml/concurrent_reads", 256)); + + assertThat(newOverlay.cassandraYaml().getInteger("concurrent_reads")).isEqualTo(256); + } + + @Test + void testRemoveTopLevelKey() + { + CassandraConfigurationOverlay overlay = new CassandraConfigurationOverlay( + new JsonObject().put("concurrent_reads", 64), null); + + CassandraConfigurationOverlay newOverlay = applyOps(effectiveConfig, overlay, + new ConfigurationPatchOperation(REMOVE, "/configuration/cassandraYaml/concurrent_reads", null)); + + assertThat(newOverlay.cassandraYaml().containsKey("concurrent_reads")).isFalse(); + } + + @Test + void testRemoveTemplateOnlyKeyFails() + { + assertThatThrownBy(() -> applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(REMOVE, "/configuration/cassandraYaml/cluster_name", null))) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("does not exist in overlay"); + } + + @Test + void testReplaceExistingKey() + { + CassandraConfigurationOverlay newOverlay = applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(REPLACE, "/configuration/cassandraYaml/concurrent_reads", 128)); + + assertThat(newOverlay.cassandraYaml().getInteger("concurrent_reads")).isEqualTo(128); + } + + @Test + void testReplaceAbsentKeyFails() + { + assertThatThrownBy(() -> applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(REPLACE, "/configuration/cassandraYaml/nonexistent", 42))) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("does not exist in effective config"); + } + + @Test + void testTestMatchingValue() + { + CassandraConfigurationOverlay newOverlay = applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(TEST, "/configuration/cassandraYaml/concurrent_reads", 32)); + + assertThat(newOverlay.cassandraYaml()).isEmpty(); + } + + @Test + void testTestMismatchFails() + { + assertThatThrownBy(() -> applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(TEST, "/configuration/cassandraYaml/concurrent_reads", 999))) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("Test failed: expected 999 but found 32"); + } + + @Test + void testTestAbsentPathFails() + { + assertThatThrownBy(() -> applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(TEST, "/configuration/cassandraYaml/nonexistent", "x"))) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("path does not exist in effective config"); + } + + // --- Nested cassandraYaml operations (copy-siblings) --- + + @Test + void testAddNestedKeyCopiesSiblings() + { + CassandraConfigurationOverlay newOverlay = applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(ADD, + "/configuration/cassandraYaml/memtable/configurations/trie/compression", "lz4")); + + JsonObject memtable = newOverlay.cassandraYaml().getJsonObject("memtable"); + JsonObject trie = memtable.getJsonObject("configurations").getJsonObject("trie"); + // New key added + assertThat(trie.getString("compression")).isEqualTo("lz4"); + // Siblings copied from effective config + assertThat(trie.getString("class_name")).isEqualTo("TrieMemtable"); + assertThat(trie.getInteger("max_shard_count")).isEqualTo(4); + + JsonObject skiplist = memtable.getJsonObject("configurations").getJsonObject("skiplist"); + assertThat(skiplist.getString("class_name")).isEqualTo("SkipListMemtable"); + } + + @Test + void testReplaceNestedKeyCopiesSiblings() + { + CassandraConfigurationOverlay newOverlay = applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(REPLACE, + "/configuration/cassandraYaml/memtable/configurations/trie/class_name", "ShardedMemtable")); + + JsonObject trie = newOverlay.cassandraYaml().getJsonObject("memtable") + .getJsonObject("configurations").getJsonObject("trie"); + assertThat(trie.getString("class_name")).isEqualTo("ShardedMemtable"); + assertThat(trie.getInteger("max_shard_count")).isEqualTo(4); + } + + @Test + void testMultipleNestedOpsOnSameTopLevelKey() + { + CassandraConfigurationOverlay newOverlay = applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(REPLACE, + "/configuration/cassandraYaml/memtable/configurations/trie/class_name", "ShardedMemtable"), + new ConfigurationPatchOperation(REPLACE, + "/configuration/cassandraYaml/memtable/configurations/trie/max_shard_count", 8)); + + JsonObject trie = newOverlay.cassandraYaml().getJsonObject("memtable") + .getJsonObject("configurations").getJsonObject("trie"); + assertThat(trie.getString("class_name")).isEqualTo("ShardedMemtable"); + assertThat(trie.getInteger("max_shard_count")).isEqualTo(8); + + JsonObject skiplist = newOverlay.cassandraYaml().getJsonObject("memtable") + .getJsonObject("configurations").getJsonObject("skiplist"); + assertThat(skiplist.getString("class_name")).isEqualTo("SkipListMemtable"); + } + + @Test + void testRemoveNestedKeyFromOverlay() + { + CassandraConfigurationOverlay overlay = new CassandraConfigurationOverlay(new JsonObject() + .put("memtable", new JsonObject() + .put("configurations", new JsonObject() + .put("trie", new JsonObject() + .put("class_name", "ShardedMemtable") + .put("max_shard_count", 8)) + .put("skiplist", new JsonObject() + .put("class_name", "SkipListMemtable")))), null); + + CassandraConfigurationOverlay newOverlay = applyOps(effectiveConfig, overlay, + new ConfigurationPatchOperation(REMOVE, + "/configuration/cassandraYaml/memtable/configurations/trie/max_shard_count", null)); + + JsonObject trie = newOverlay.cassandraYaml().getJsonObject("memtable") + .getJsonObject("configurations").getJsonObject("trie"); + assertThat(trie.containsKey("max_shard_count")).isFalse(); + assertThat(trie.getString("class_name")).isEqualTo("ShardedMemtable"); + } + + @Test + void testRemoveNestedKeyNotInOverlayFails() + { + assertThatThrownBy(() -> applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(REMOVE, + "/configuration/cassandraYaml/memtable/configurations/trie/class_name", null))) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("does not exist in overlay"); + } + + @Test + void testAddNestedKeyParentAbsentFails() + { + assertThatThrownBy(() -> applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(ADD, + "/configuration/cassandraYaml/nonexistent_parent/child/leaf", "value"))) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("parent path does not exist"); + } + + @Test + void testTestNestedValue() + { + applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(TEST, + "/configuration/cassandraYaml/memtable/configurations/trie/class_name", "TrieMemtable")); + } + + @Test + void testTestNestedValueMismatchFails() + { + assertThatThrownBy(() -> applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(TEST, + "/configuration/cassandraYaml/memtable/configurations/trie/class_name", "WrongValue"))) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("Test failed"); + } + + // --- Array-valued top-level keys --- + + @Test + void testAddTopLevelArrayValue() + { + List directories = List.of("/data1", "/data2"); + CassandraConfigurationOverlay newOverlay = applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(ADD, "/configuration/cassandraYaml/data_file_directories", directories)); + + assertThat(newOverlay.cassandraYaml().getJsonArray("data_file_directories")) + .containsExactly("/data1", "/data2"); + } + + @Test + void testReplaceTopLevelArrayValue() + { + JsonObject effectiveYaml = effectiveConfig.cassandraYaml().copy() + .put("data_file_directories", List.of("/old_data")); + CassandraConfigurationOverlay effective = new CassandraConfigurationOverlay( + effectiveYaml, effectiveConfig.extraJvmOpts()); + + CassandraConfigurationOverlay newOverlay = applyOps(effective, EMPTY_OVERLAY, + new ConfigurationPatchOperation(REPLACE, "/configuration/cassandraYaml/data_file_directories", + List.of("/new_data1", "/new_data2"))); + + assertThat(newOverlay.cassandraYaml().getJsonArray("data_file_directories")) + .containsExactly("/new_data1", "/new_data2"); + } + + @Test + void testNestedPathIntoArrayValueFails() + { + JsonObject effectiveYaml = effectiveConfig.cassandraYaml().copy() + .put("seed_provider", List.of(Map.of("class_name", "SimpleSeedProvider"))); + CassandraConfigurationOverlay effective = new CassandraConfigurationOverlay( + effectiveYaml, effectiveConfig.extraJvmOpts()); + + assertThatThrownBy(() -> applyOps(effective, EMPTY_OVERLAY, + new ConfigurationPatchOperation(REPLACE, + "/configuration/cassandraYaml/seed_provider/0/class_name", "OtherProvider"))) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("intermediate value is not an object"); + } + + // --- extraJvmOpts operations --- + + @Test + void testAddJvmOpt() + { + CassandraConfigurationOverlay newOverlay = applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-Xms", "2g")); + + assertThat(newOverlay.extraJvmOpts()).containsEntry("-Xms", "2g"); + } + + @Test + void testRemoveJvmOptFromOverlay() + { + CassandraConfigurationOverlay overlay = new CassandraConfigurationOverlay(null, + new LinkedHashMap<>(Map.of("-Xmx", "8g"))); + + CassandraConfigurationOverlay newOverlay = applyOps(effectiveConfig, overlay, + new ConfigurationPatchOperation(REMOVE, "/configuration/extraJvmOpts/-Xmx", null)); + + assertThat(newOverlay.extraJvmOpts()).doesNotContainKey("-Xmx"); + } + + @Test + void testReplaceJvmOptAbsentFails() + { + assertThatThrownBy(() -> applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(REPLACE, "/configuration/extraJvmOpts/-Xms", "2g"))) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("does not exist in effective extraJvmOpts"); + } + + @Test + void testTestJvmOptValue() + { + applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(TEST, "/configuration/extraJvmOpts/-Xmx", "4g")); + } + + @Test + void testTestJvmOptMismatchFails() + { + assertThatThrownBy(() -> applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(TEST, "/configuration/extraJvmOpts/-Xmx", "16g"))) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("Test failed"); + } + + @Test + void testAddInvalidJvmOptKeyFails() + { + assertThatThrownBy(() -> applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/invalidKey", "value"))) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("Invalid JVM option key 'invalidKey'"); + } + + @Test + void testAddConflictingBooleanJvmOptFails() + { + CassandraConfigurationOverlay overlay = new CassandraConfigurationOverlay(null, + new LinkedHashMap<>(Map.of("-XX:+UseG1GC", ""))); + + assertThatThrownBy(() -> applyOps(effectiveConfig, overlay, + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-XX:-UseG1GC", ""))) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("Conflicting boolean JVM option"); + } + + @Test + void testReplaceBooleanJvmOptByRemovingThenAdding() + { + CassandraConfigurationOverlay overlay = new CassandraConfigurationOverlay(null, + new LinkedHashMap<>(Map.of("-XX:+UseG1GC", ""))); + + Map effectiveOpts = new LinkedHashMap<>(); + effectiveOpts.put("-Xmx", "4g"); + effectiveOpts.put("-XX:+UseG1GC", ""); + CassandraConfigurationOverlay effective = new CassandraConfigurationOverlay( + effectiveConfig.cassandraYaml(), effectiveOpts); + + CassandraConfigurationOverlay newOverlay = applyOps(effective, overlay, + new ConfigurationPatchOperation(REMOVE, "/configuration/extraJvmOpts/-XX:+UseG1GC", null), + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-XX:-UseG1GC", "")); + + assertThat(newOverlay.extraJvmOpts()).doesNotContainKey("-XX:+UseG1GC"); + assertThat(newOverlay.extraJvmOpts()).containsEntry("-XX:-UseG1GC", ""); + } + + // --- Atomicity --- + + @Test + void testTestFailurePreventsAllMutations() + { + assertThatThrownBy(() -> applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(ADD, "/configuration/cassandraYaml/new_key", "value"), + new ConfigurationPatchOperation(TEST, "/configuration/cassandraYaml/concurrent_reads", 999))) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("Test failed"); + } + + @Test + void testMultipleOpsAppliedAtomically() + { + CassandraConfigurationOverlay newOverlay = applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(ADD, "/configuration/cassandraYaml/concurrent_writes", 64), + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-Xms", "2g"), + new ConfigurationPatchOperation(TEST, "/configuration/cassandraYaml/concurrent_reads", 32)); + + assertThat(newOverlay.cassandraYaml().getInteger("concurrent_writes")).isEqualTo(64); + assertThat(newOverlay.extraJvmOpts()).containsEntry("-Xms", "2g"); + } + + private static CassandraConfigurationOverlay applyOps(CassandraConfigurationOverlay effective, + CassandraConfigurationOverlay overlay, + ConfigurationPatchOperation... ops) + { + List parsed = + ConfigurationPatchValidator.validate(List.of(ops)); + return ConfigurationPatchApplier.apply(parsed, effective, overlay); + } +} diff --git a/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidatorTest.java b/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidatorTest.java new file mode 100644 index 000000000..425a2f807 --- /dev/null +++ b/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidatorTest.java @@ -0,0 +1,381 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.sidecar.configmanagement; + +import java.util.List; + +import org.junit.jupiter.api.Test; + +import static org.apache.cassandra.sidecar.configmanagement.ConfigurationPatchOperation.Op.ADD; +import static org.apache.cassandra.sidecar.configmanagement.ConfigurationPatchOperation.Op.REMOVE; +import static org.apache.cassandra.sidecar.configmanagement.ConfigurationPatchOperation.Op.REPLACE; +import static org.apache.cassandra.sidecar.configmanagement.ConfigurationPatchOperation.Op.TEST; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * Tests for {@link ConfigurationPatchValidator} + */ +class ConfigurationPatchValidatorTest +{ + @Test + void testRejectsEmptyOperationsList() + { + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(List.of())) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("must not be empty"); + } + + @Test + void testRejectsRemoveWithValue() + { + List ops = List.of( + new ConfigurationPatchOperation(REMOVE, "/configuration/cassandraYaml/key", "value")); + + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("REMOVE' must not have a value"); + } + + @Test + void testRejectsAddWithoutValue() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/cassandraYaml/key", null)); + + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("requires a value"); + } + + @Test + void testRejectsReplaceWithoutValue() + { + List ops = List.of( + new ConfigurationPatchOperation(REPLACE, "/configuration/cassandraYaml/key", null)); + + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("requires a value"); + } + + @Test + void testRejectsTestWithoutValue() + { + List ops = List.of( + new ConfigurationPatchOperation(TEST, "/configuration/cassandraYaml/key", null)); + + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("requires a value"); + } + + @Test + void testRejectsInvalidPathPrefix() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/invalid/path", 42)); + + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("Path must start with"); + } + + @Test + void testRejectsPathWithNoKeyAfterSection() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/cassandraYaml/", 42)); + + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("Path must specify at least one key after section"); + } + + @Test + void testRejectsNestedExtraJvmOpts() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-Dfoo/nested", "bar")); + + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("extraJvmOpts paths must be flat"); + } + + @Test + void testRejectsEmptySegmentInPath() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/cassandraYaml//key", "value")); + + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("empty segment"); + } + + @Test + void testRejectsDuplicateMutationPaths() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/cassandraYaml/key", "a"), + new ConfigurationPatchOperation(ADD, "/configuration/cassandraYaml/key", "b")); + + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("Duplicate path"); + } + + @Test + void testAllowsTestAndMutationOnSamePath() + { + List ops = List.of( + new ConfigurationPatchOperation(TEST, "/configuration/cassandraYaml/key", "old"), + new ConfigurationPatchOperation(REPLACE, "/configuration/cassandraYaml/key", "new")); + + List parsed = ConfigurationPatchValidator.validate(ops); + + assertThat(parsed).hasSize(2); + } + + @Test + void testParsesTopLevelCassandraYamlPath() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/cassandraYaml/concurrent_reads", 64)); + + List parsed = ConfigurationPatchValidator.validate(ops); + + assertThat(parsed).hasSize(1); + assertThat(parsed.get(0).section()).isEqualTo("cassandraYaml"); + assertThat(parsed.get(0).topLevelKey()).isEqualTo("concurrent_reads"); + assertThat(parsed.get(0).nestedSegments()).isEmpty(); + assertThat(parsed.get(0).isNested()).isFalse(); + } + + @Test + void testParsesNestedCassandraYamlPath() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, + "/configuration/cassandraYaml/memtable/configurations/trie/class_name", "value")); + + List parsed = ConfigurationPatchValidator.validate(ops); + + assertThat(parsed.get(0).section()).isEqualTo("cassandraYaml"); + assertThat(parsed.get(0).topLevelKey()).isEqualTo("memtable"); + assertThat(parsed.get(0).nestedSegments()).containsExactly("configurations", "trie", "class_name"); + assertThat(parsed.get(0).isNested()).isTrue(); + } + + @Test + void testParsesExtraJvmOptsPath() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-Xmx", "8g")); + + List parsed = ConfigurationPatchValidator.validate(ops); + + assertThat(parsed.get(0).section()).isEqualTo("extraJvmOpts"); + assertThat(parsed.get(0).topLevelKey()).isEqualTo("-Xmx"); + assertThat(parsed.get(0).nestedSegments()).isEmpty(); + } + + // --- extraJvmOpts key allowlist tests --- + + @Test + void testAcceptsValidSystemPropertyKey() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-Dcassandra.jmx.local.port", "7199")); + + List parsed = ConfigurationPatchValidator.validate(ops); + + assertThat(parsed).hasSize(1); + assertThat(parsed.get(0).topLevelKey()).isEqualTo("-Dcassandra.jmx.local.port"); + } + + @Test + void testAcceptsValidXFlag() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-Xms", "2g")); + + assertThat(ConfigurationPatchValidator.validate(ops)).hasSize(1); + } + + @Test + void testAcceptsValidXXBooleanFlag() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-XX:+UseG1GC", "")); + + assertThat(ConfigurationPatchValidator.validate(ops)).hasSize(1); + } + + @Test + void testAcceptsValidXXValueFlag() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-XX:MaxGCPauseMillis", "200")); + + assertThat(ConfigurationPatchValidator.validate(ops)).hasSize(1); + } + + @Test + void testRejectsJavaAgentKey() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-javaagent", "/tmp/malicious.jar")); + + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("Invalid JVM option key '-javaagent'"); + } + + @Test + void testRejectsAgentPathKey() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-agentpath", "/tmp/agent.so")); + + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("Invalid JVM option key '-agentpath'"); + } + + @Test + void testRejectsBlockedOnOutOfMemoryError() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-XX:OnOutOfMemoryError", "kill -9 %p")); + + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessage("Blocked extraJvmOpts key '-XX:OnOutOfMemoryError': this JVM option is not allowed"); + } + + @Test + void testRejectsBlockedOnError() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-XX:OnError", "rm -rf /")); + + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessage("Blocked extraJvmOpts key '-XX:OnError': this JVM option is not allowed"); + } + + @Test + void testRejectsKeyWithEqualsSign() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-Dfoo=bar", "value")); + + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("Invalid JVM option key '-Dfoo=bar'"); + } + + @Test + void testRejectsKeyWithShellMetacharacters() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-Dfoo;echo pwned", "value")); + + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("Invalid JVM option key '-Dfoo;echo pwned'"); + } + + // --- extraJvmOpts value validation tests --- + + @Test + void testAcceptsValidJvmOptValue() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-Xmx", "4g")); + + assertThat(ConfigurationPatchValidator.validate(ops)).hasSize(1); + } + + @Test + void testAcceptsJvmOptValueWithAbsolutePath() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-XX:HeapDumpPath", "/var/log/cassandra")); + + assertThat(ConfigurationPatchValidator.validate(ops)).hasSize(1); + } + + @Test + void testRejectsJvmOptValueWithPathTraversal() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-XX:HeapDumpPath", "/tmp/../../etc/cron.d")); + + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("path traversal"); + } + + @Test + void testRejectsJvmOptValueWithShellMetacharacters() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-Dfoo", "val;rm -rf /")); + + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("disallowed characters"); + } + + @Test + void testRejectsJvmOptValueWithBacktick() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-Dfoo", "`whoami`")); + + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("disallowed characters"); + } + + @Test + void testRejectsJvmOptValueExceedingMaxLength() + { + String longValue = "a".repeat(513); + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-Xmx", longValue)); + + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("disallowed characters or exceeds 512"); + } + + @Test + void testAllowsRemoveWithoutValueValidation() + { + List ops = List.of( + new ConfigurationPatchOperation(REMOVE, "/configuration/extraJvmOpts/-Xmx", null)); + + assertThat(ConfigurationPatchValidator.validate(ops)).hasSize(1); + } + +} From 17fb8896cef0465c68b417fe63af788862d1f7c2 Mon Sep 17 00:00:00 2001 From: Paulo Motta Date: Fri, 10 Jul 2026 12:54:02 -0400 Subject: [PATCH 2/7] CASSSIDECAR-429: Detect conflicting boolean JVM options against the effective 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. --- .../ConfigurationPatchApplier.java | 57 ++++++++++++++++-- .../ConfigurationManagerTest.java | 58 +++++++++++++++++++ .../ConfigurationPatchApplierTest.java | 23 +++++++- 3 files changed, 131 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplier.java b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplier.java index db57ae9c8..85c3b80aa 100644 --- a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplier.java +++ b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplier.java @@ -69,6 +69,12 @@ public static CassandraConfigurationOverlay apply( checkPreconditions(parsed, effectiveConfig, currentOverlay); } + // Phase 1b: Reject patches that would leave conflicting boolean JVM options in the + // effective configuration (e.g. adding -XX:-UseG1GC while the base already sets -XX:+UseG1GC). + // Checking against the effective opts, rather than the overlay alone, ensures the stored + // overlay and the returned effective configuration stay consistent. + checkResultingJvmOptConflicts(parsedOps, effectiveConfig.extraJvmOpts()); + // Phase 2: Apply mutations JsonObject updatedYaml = currentOverlay.cassandraYaml().copy(); Map updatedOpts = new LinkedHashMap<>(currentOverlay.extraJvmOpts()); @@ -333,14 +339,53 @@ private static void applyJvmOptsMutation(ConfigurationPatchValidator.ParsedPatch } else { - if (CassandraConfigurationOverlay.hasConflictingBooleanOpt(updatedOpts, key)) + // Boolean-opt conflicts are validated up-front against the effective configuration + // in checkResultingJvmOptConflicts, so the mutation here is unconditional. + updatedOpts.put(key, String.valueOf(op.value())); + } + } + + /** + * Rejects patches that would leave conflicting boolean JVM options (e.g. both {@code -XX:+UseG1GC} + * and {@code -XX:-UseG1GC}) in the effective configuration once applied. + * + *

The check is performed against a projection of the effective options after the patch's + * add/remove/replace operations are applied in order. This catches conflicts against options that + * originate in the base template (not just the overlay), keeping the stored overlay and the + * returned effective configuration consistent. + */ + private static void checkResultingJvmOptConflicts( + List parsedOps, + Map effectiveOpts) + { + Map projected = new LinkedHashMap<>(effectiveOpts); + for (ConfigurationPatchValidator.ParsedPatchOperation parsed : parsedOps) + { + if (!parsed.section().equals(EXTRA_JVM_OPTS_SECTION)) { - String conflicting = CassandraConfigurationOverlay.conflictingBooleanOpt(key); - throw new ConfigurationPatchException( - "Conflicting boolean JVM option: '" + key + "' conflicts with existing '" - + conflicting + "'", op); + continue; + } + ConfigurationPatchOperation op = parsed.operation(); + String key = parsed.topLevelKey(); + switch (op.op()) + { + case REMOVE: + projected.remove(key); + break; + case ADD: + case REPLACE: + if (CassandraConfigurationOverlay.hasConflictingBooleanOpt(projected, key)) + { + String conflicting = CassandraConfigurationOverlay.conflictingBooleanOpt(key); + throw new ConfigurationPatchException( + "Conflicting boolean JVM option: '" + key + "' conflicts with existing '" + + conflicting + "' in the effective configuration", op); + } + projected.put(key, String.valueOf(op.value())); + break; + case TEST: + break; } - updatedOpts.put(key, String.valueOf(op.value())); } } diff --git a/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationManagerTest.java b/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationManagerTest.java index 96137f0fa..719f6b0f4 100644 --- a/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationManagerTest.java +++ b/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationManagerTest.java @@ -218,6 +218,64 @@ void testPatchAddJvmOpt() assertThat(result.configuration().extraJvmOpts()).containsEntry("-Xmx", "8g"); } + @Test + void testPatchConflictingBooleanJvmOptRejected() + { + InstanceMetadata instance = mockInstance(1); + + // Existing overlay enables G1GC; a patch that adds the opposite flag must be rejected outright + // rather than being stored and silently dropped when merged with the base. + Map jvmOpts = new LinkedHashMap<>(); + jvmOpts.put("-XX:+UseG1GC", ""); + CassandraConfigurationOverlay initial = new CassandraConfigurationOverlay(null, jvmOpts); + provider.storeOverlay(instance, null, new ConfigurationOverlaySnapshot(Instant.now(), initial)); + + ConfigurationManager manager = new ConfigurationManager(provider, BASE_TEMPLATE); + String effectiveHash = manager.getEffectiveConfiguration(instance).hash(); + + List ops = List.of( + new ConfigurationPatchOperation(ConfigurationPatchOperation.Op.ADD, + "/configuration/extraJvmOpts/-XX:-UseG1GC", "")); + + assertThatThrownBy(() -> manager.patchConfiguration(instance, effectiveHash, ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("Conflicting boolean JVM option"); + + // Nothing was stored: the overlay still enables G1GC and does not contain the conflicting flag + CassandraConfigurationOverlay storedOverlay = provider.getOverlay(instance).configuration(); + assertThat(storedOverlay.extraJvmOpts()).containsEntry("-XX:+UseG1GC", ""); + assertThat(storedOverlay.extraJvmOpts()).doesNotContainKey("-XX:-UseG1GC"); + } + + @Test + void testPatchReturnedConfigMatchesStoredConfig() + { + InstanceMetadata instance = mockInstance(1); + + Map jvmOpts = new LinkedHashMap<>(); + jvmOpts.put("-XX:+UseG1GC", ""); + CassandraConfigurationOverlay initial = new CassandraConfigurationOverlay(null, jvmOpts); + provider.storeOverlay(instance, null, new ConfigurationOverlaySnapshot(Instant.now(), initial)); + + ConfigurationManager manager = new ConfigurationManager(provider, BASE_TEMPLATE); + String effectiveHash = manager.getEffectiveConfiguration(instance).hash(); + + List ops = List.of( + new ConfigurationPatchOperation(ConfigurationPatchOperation.Op.ADD, + "/configuration/extraJvmOpts/-Xmx", "8g")); + + ConfigurationOverlaySnapshot returned = manager.patchConfiguration(instance, effectiveHash, ops); + + // The effective config returned to the caller must equal the effective config computed from the + // persisted overlay on a subsequent read (no silent divergence between stored and returned state). + ConfigurationOverlaySnapshot reread = manager.getEffectiveConfiguration(instance); + assertThat(returned.hash()).isEqualTo(reread.hash()); + assertThat(returned.configuration()).isEqualTo(reread.configuration()); + assertThat(returned.configuration().extraJvmOpts()) + .containsEntry("-XX:+UseG1GC", "") + .containsEntry("-Xmx", "8g"); + } + @Test void testPatchRemoveTopLevelKeyFromOverlay() { diff --git a/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplierTest.java b/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplierTest.java index 0a45ace8a..c30ba8b2b 100644 --- a/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplierTest.java +++ b/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplierTest.java @@ -370,10 +370,31 @@ void testAddInvalidJvmOptKeyFails() @Test void testAddConflictingBooleanJvmOptFails() { + Map effectiveOpts = new LinkedHashMap<>(); + effectiveOpts.put("-Xmx", "4g"); + effectiveOpts.put("-XX:+UseG1GC", ""); + CassandraConfigurationOverlay effective = new CassandraConfigurationOverlay( + effectiveConfig.cassandraYaml(), effectiveOpts); CassandraConfigurationOverlay overlay = new CassandraConfigurationOverlay(null, new LinkedHashMap<>(Map.of("-XX:+UseG1GC", ""))); - assertThatThrownBy(() -> applyOps(effectiveConfig, overlay, + assertThatThrownBy(() -> applyOps(effective, overlay, + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-XX:-UseG1GC", ""))) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("Conflicting boolean JVM option"); + } + + @Test + void testAddConflictingBooleanJvmOptAgainstBaseFails() + { + // Conflicting option exists only in the effective config (i.e. base template), not the overlay. + // The overlay-only check would miss this; the effective-config check must catch it. + Map effectiveOpts = new LinkedHashMap<>(); + effectiveOpts.put("-XX:+UseG1GC", ""); + CassandraConfigurationOverlay effective = new CassandraConfigurationOverlay( + effectiveConfig.cassandraYaml(), effectiveOpts); + + assertThatThrownBy(() -> applyOps(effective, EMPTY_OVERLAY, new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-XX:-UseG1GC", ""))) .isInstanceOf(ConfigurationPatchException.class) .hasMessageContaining("Conflicting boolean JVM option"); From 5c676938a6c26377e2c08bfc9090825f1aecece4 Mon Sep 17 00:00:00 2001 From: Paulo Motta Date: Fri, 10 Jul 2026 12:55:43 -0400 Subject: [PATCH 3/7] CASSSIDECAR-429: Block path-bearing and file-writing JVM options in extraJvmOpts 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. --- .../ConfigurationPatchValidator.java | 20 ++++++++++++--- .../ConfigurationPatchValidatorTest.java | 25 +++++++++++++++++-- 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidator.java b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidator.java index d6802f2ab..976b8f4b7 100644 --- a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidator.java +++ b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidator.java @@ -51,10 +51,22 @@ public final class ConfigurationPatchValidator static final Pattern JVM_OPT_KEY_PATTERN = Pattern.compile( "^-(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 BLOCKED_XX_FLAGS = Set.of( + // JVM options that are rejected because they execute arbitrary commands or write to + // arbitrary filesystem paths. The value pattern permits absolute paths (Cassandra system + // properties legitimately need them), so path-bearing flags must be blocked by key instead. + static final Set BLOCKED_JVM_OPTS = Set.of( + // Execute arbitrary commands "-XX:OnOutOfMemoryError", - "-XX:OnError"); + "-XX:OnError", + // Write to arbitrary filesystem paths + "-XX:ErrorFile", + "-XX:HeapDumpPath", + "-XX:LogFile", + "-XX:FlightRecorderOptions", + "-XX:StartFlightRecording", + "-Xloggc", + "-Xlog", + "-Xbootclasspath"); // Allows: alphanumeric, dots, colons, slashes, @, +, commas, spaces, hyphens (max 512 chars) // Rejects: shell metacharacters (;|&$`), quotes, newlines, and other control characters @@ -181,7 +193,7 @@ private static void validateJvmOptKey(String key, ConfigurationPatchOperation op "Invalid JVM option key '" + key + "': must be a valid JVM option " + "(-Dproperty.name, -Xflag, or -XX:[+-]Flag)", op); } - if (BLOCKED_XX_FLAGS.contains(key)) + if (BLOCKED_JVM_OPTS.contains(key)) { throw new ConfigurationPatchException( "Blocked extraJvmOpts key '" + key + "': this JVM option is not allowed", op); diff --git a/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidatorTest.java b/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidatorTest.java index 425a2f807..42ee74a26 100644 --- a/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidatorTest.java +++ b/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidatorTest.java @@ -282,6 +282,27 @@ void testRejectsBlockedOnError() .hasMessage("Blocked extraJvmOpts key '-XX:OnError': this JVM option is not allowed"); } + @Test + void testRejectsPathBearingJvmOpts() + { + // Options that write to arbitrary filesystem paths must be blocked by key, since the value + // pattern legitimately permits absolute paths for Cassandra system properties. + List blockedPathOpts = List.of( + "-XX:ErrorFile", "-XX:HeapDumpPath", "-XX:LogFile", + "-XX:FlightRecorderOptions", "-XX:StartFlightRecording", + "-Xloggc", "-Xlog", "-Xbootclasspath"); + + for (String key : blockedPathOpts) + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/" + key, "/tmp/evil")); + + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessage("Blocked extraJvmOpts key '" + key + "': this JVM option is not allowed"); + } + } + @Test void testRejectsKeyWithEqualsSign() { @@ -319,7 +340,7 @@ void testAcceptsValidJvmOptValue() void testAcceptsJvmOptValueWithAbsolutePath() { List ops = List.of( - new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-XX:HeapDumpPath", "/var/log/cassandra")); + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-Dcassandra.logdir", "/var/log/cassandra")); assertThat(ConfigurationPatchValidator.validate(ops)).hasSize(1); } @@ -328,7 +349,7 @@ void testAcceptsJvmOptValueWithAbsolutePath() void testRejectsJvmOptValueWithPathTraversal() { List ops = List.of( - new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-XX:HeapDumpPath", "/tmp/../../etc/cron.d")); + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-Dcassandra.logdir", "/tmp/../../etc/cron.d")); assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) .isInstanceOf(ConfigurationPatchException.class) From d973ec408dba723698623ff398f6ed7cf365ba2f Mon Sep 17 00:00:00 2001 From: Paulo Motta Date: Fri, 10 Jul 2026 12:57:32 -0400 Subject: [PATCH 4/7] CASSSIDECAR-429: Simplify TEST value comparison in ConfigurationPatchApplier 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. --- .../ConfigurationPatchApplier.java | 13 ++--- .../ConfigurationPatchApplierTest.java | 55 +++++++++++++++++++ 2 files changed, 59 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplier.java b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplier.java index 85c3b80aa..1e9531706 100644 --- a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplier.java +++ b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplier.java @@ -117,7 +117,10 @@ private static void checkYamlPreconditions(ConfigurationPatchValidator.ParsedPat throw new ConfigurationPatchException( "Test failed: path does not exist in effective config: '" + op.path() + "'", op); } - if (!valueEquals(actualValue, op.value())) + // 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. + if (!Objects.equals(actualValue, op.value())) { throw new ConfigurationPatchException( "Test failed: expected " + op.value() + " but found " + actualValue @@ -485,12 +488,4 @@ private static JsonObject deepCopyValue(@Nullable Object value) return new JsonObject(); } - private static boolean valueEquals(@NotNull Object actual, @Nullable Object expected) - { - if (actual instanceof JsonObject && expected instanceof Map) - { - return actual.equals(new JsonObject((Map) expected)); - } - return Objects.equals(actual, expected); - } } diff --git a/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplierTest.java b/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplierTest.java index c30ba8b2b..9b0616c97 100644 --- a/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplierTest.java +++ b/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplierTest.java @@ -25,6 +25,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import io.vertx.core.json.JsonArray; import io.vertx.core.json.JsonObject; import static org.apache.cassandra.sidecar.configmanagement.ConfigurationPatchOperation.Op.ADD; @@ -256,6 +257,60 @@ void testTestNestedValue() "/configuration/cassandraYaml/memtable/configurations/trie/class_name", "TrieMemtable")); } + @Test + void testTestObjectValue() + { + // TEST against an object-valued path. The request handler reads the value via JsonObject.getValue, + // so the expected value is a JsonObject - the same type resolveValue returns. + applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(TEST, + "/configuration/cassandraYaml/memtable/configurations/trie", + new JsonObject().put("class_name", "TrieMemtable").put("max_shard_count", 4))); + } + + @Test + void testTestObjectValueMismatchFails() + { + assertThatThrownBy(() -> applyOps(effectiveConfig, EMPTY_OVERLAY, + new ConfigurationPatchOperation(TEST, + "/configuration/cassandraYaml/memtable/configurations/trie", + new JsonObject().put("class_name", "TrieMemtable").put("max_shard_count", 99)))) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("Test failed"); + } + + @Test + void testTestArrayValue() + { + // TEST against an array-valued path. The expected value is a JsonArray, matching what + // resolveValue returns for array values. + JsonObject effectiveYaml = effectiveConfig.cassandraYaml().copy() + .put("data_file_directories", new JsonArray().add("/data1").add("/data2")); + CassandraConfigurationOverlay effective = new CassandraConfigurationOverlay( + effectiveYaml, effectiveConfig.extraJvmOpts()); + + applyOps(effective, EMPTY_OVERLAY, + new ConfigurationPatchOperation(TEST, + "/configuration/cassandraYaml/data_file_directories", + new JsonArray().add("/data1").add("/data2"))); + } + + @Test + void testTestArrayValueMismatchFails() + { + JsonObject effectiveYaml = effectiveConfig.cassandraYaml().copy() + .put("data_file_directories", new JsonArray().add("/data1").add("/data2")); + CassandraConfigurationOverlay effective = new CassandraConfigurationOverlay( + effectiveYaml, effectiveConfig.extraJvmOpts()); + + assertThatThrownBy(() -> applyOps(effective, EMPTY_OVERLAY, + new ConfigurationPatchOperation(TEST, + "/configuration/cassandraYaml/data_file_directories", + new JsonArray().add("/data1").add("/other")))) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("Test failed"); + } + @Test void testTestNestedValueMismatchFails() { From 62c0ebc52bbdb90a1af5d1c722cd14f9132067c8 Mon Sep 17 00:00:00 2001 From: Paulo Motta Date: Mon, 13 Jul 2026 17:00:43 -0400 Subject: [PATCH 5/7] CASSSIDECAR-429: Fix stale javadoc reference in CassandraConfigurationOverlay --- .../sidecar/configmanagement/CassandraConfigurationOverlay.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/CassandraConfigurationOverlay.java b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/CassandraConfigurationOverlay.java index 0734babbb..57a061370 100644 --- a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/CassandraConfigurationOverlay.java +++ b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/CassandraConfigurationOverlay.java @@ -62,7 +62,7 @@ public CassandraConfigurationOverlay(@Nullable JsonObject cassandraYaml, /** * Returns the cassandra.yaml overlay as a version-agnostic JSON object. Callers must not mutate the - * returned object; use {@link #updated} to produce a new overlay with changes applied. + * returned object; use {@link ConfigurationPatchApplier} to produce a new overlay with changes applied. * * @return the cassandra.yaml overlay as a version-agnostic JSON object */ From d818e0764e54b91771523a0e4d6219f9c6aa8bff Mon Sep 17 00:00:00 2001 From: Paulo Motta Date: Mon, 13 Jul 2026 16:13:49 -0400 Subject: [PATCH 6/7] CASSSIDECAR-429: Allow JSON values in extra JVM option validation 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. --- .../ConfigurationPatchValidator.java | 7 ++-- .../ConfigurationPatchValidatorTest.java | 35 +++++++++++++++++++ 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidator.java b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidator.java index 976b8f4b7..ea996e2e5 100644 --- a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidator.java +++ b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidator.java @@ -68,9 +68,10 @@ public final class ConfigurationPatchValidator "-Xlog", "-Xbootclasspath"); - // 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}$"); + // Allows: alphanumeric, dots, colons, slashes, @, +, commas, hyphens, braces, brackets, quotes (max 512 chars). + // Quotes/braces/brackets permit JSON values. Whitespace is rejected because the Cassandra launcher + // word-splits it; shell metacharacters (;|&$`), newlines and other control characters are also rejected. + static final Pattern JVM_OPT_VALUE_PATTERN = Pattern.compile("^[a-zA-Z0-9._:/@+,\"{}\\[\\]-]{0,512}$"); // Matches /../ path traversal sequences (start, middle, or end of path) static final Pattern PATH_TRAVERSAL_PATTERN = Pattern.compile("(?:^|/)\\.\\.(?:/|$)"); diff --git a/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidatorTest.java b/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidatorTest.java index 42ee74a26..391b51e81 100644 --- a/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidatorTest.java +++ b/server/src/test/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchValidatorTest.java @@ -345,6 +345,41 @@ void testAcceptsJvmOptValueWithAbsolutePath() assertThat(ConfigurationPatchValidator.validate(ops)).hasSize(1); } + @Test + void testAcceptsJvmOptValueWithJson() + { + String jsonValue = "{\"class_name\":\"SizeTieredCompactionStrategy\"," + + "\"parameters\":{\"min_threshold\":\"4\",\"max_threshold\":\"32\"}}"; + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-Dcassandra.settings.default_compaction", + jsonValue)); + + assertThat(ConfigurationPatchValidator.validate(ops)).hasSize(1); + } + + @Test + void testAcceptsJvmOptValueWithCommas() + { + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-Dcassandra.seed_provider", + "org.apache.cassandra.locator.SimpleSeedProvider:127.0.0.1,127.0.0.2")); + + assertThat(ConfigurationPatchValidator.validate(ops)).hasSize(1); + } + + @Test + void testRejectsJvmOptValueWithWhitespace() + { + // Whitespace is rejected because bin/cassandra word-splits unquoted values during `eval`, + // silently truncating a value like "hello world" to "hello". + List ops = List.of( + new ConfigurationPatchOperation(ADD, "/configuration/extraJvmOpts/-Dfoo", "hello world")); + + assertThatThrownBy(() -> ConfigurationPatchValidator.validate(ops)) + .isInstanceOf(ConfigurationPatchException.class) + .hasMessageContaining("disallowed characters"); + } + @Test void testRejectsJvmOptValueWithPathTraversal() { From 38b8780ea4a09eaa7f77d42975fda7d0b2f3ccff Mon Sep 17 00:00:00 2001 From: Paulo Motta Date: Mon, 13 Jul 2026 17:46:01 -0400 Subject: [PATCH 7/7] CASSSIDECAR-429: Drop reference to not-yet-existing ConfigurationPatchHandler in comment --- .../sidecar/configmanagement/ConfigurationPatchApplier.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplier.java b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplier.java index 1e9531706..88e08976f 100644 --- a/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplier.java +++ b/server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplier.java @@ -118,8 +118,8 @@ private static void checkYamlPreconditions(ConfigurationPatchValidator.ParsedPat "Test failed: path does not exist in effective config: '" + op.path() + "'", op); } // 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. + // and op.value() is expected to be read from the request via JsonObject.getValue, + // which wraps objects/arrays the same way. if (!Objects.equals(actualValue, op.value())) { throw new ConfigurationPatchException(