FrankChen021 commented on code in PR #19501:
URL: https://github.com/apache/druid/pull/19501#discussion_r3294101777
##########
processing/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java:
##########
@@ -127,6 +127,54 @@ public <T> SetResult set(
return configManager.set(key, configSerde, oldValue, newValue);
}
+ @Nullable
+ public byte[] getCurrentBytes(String key)
+ {
+ return configManager.getCurrentBytes(key);
+ }
+
+ /**
+ * Set the config, optionally guarded by an {@code If-Match}-style
+ * precondition. When {@code ifMatchEtag} is {@code null}, behaves like
+ * {@link #set(String, Object, AuditInfo)}. Otherwise the write only commits
+ * if the currently stored bytes hash to {@code ifMatchEtag}; on mismatch the
+ * result reports {@link SetResult#isPreconditionFailed()
preconditionFailed}.
+ *
+ * <p>The precondition is enforced via metadata-store CAS, so it only holds
+ * when {@code druid.manager.config.enableCompareAndSwap} is true (the
+ * default). With CAS disabled, the {@code If-Match} check degrades to a
+ * best-effort TOCTOU read and concurrent writers may still race past it.
+ */
+ public <T> SetResult setIfMatch(
+ String key,
+ @Nullable String ifMatchEtag,
+ T newValue,
+ AuditInfo auditInfo
+ )
+ {
+ if (newValue == null) {
+ return SetResult.failure(new IllegalArgumentException("input obj is
null"));
+ }
+ if (ifMatchEtag == null) {
+ return set(key, newValue, auditInfo);
+ }
+ final byte[] currentBytes = configManager.getCurrentBytes(key);
+ if (!ConfigEtag.matches(ifMatchEtag, currentBytes)) {
+ return SetResult.preconditionFailed(
+ new IllegalStateException("If-Match precondition failed for key[" +
key + "]")
+ );
+ }
+ final SetResult result = set(key, currentBytes, newValue, auditInfo);
Review Comment:
Thanks, the new guard fixes the `JacksonConfigManager.setIfMatch` callers,
but this still looks open for compaction config writes.
`CoordinatorConfigManager.getAndUpdateCompactionConfig` does its own `If-Match`
check and then calls `jacksonConfigManager.set(..., currentBytes, updated,
...)`; when `enableCompareAndSwap=false`, `ConfigManager.set` still ignores
that `oldValue` and performs `insertOrUpdate`, so the compaction endpoints that
advertise `If-Match` can still overwrite a concurrent update and return
success. Can we apply the same disabled-CAS precondition failure to that path
as well? Reviewed 23 of 23 changed files.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
