FrankChen021 commented on code in PR #19501:
URL: https://github.com/apache/druid/pull/19501#discussion_r3288573649
##########
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:
[P1] Conditional writes can overwrite newer configs when CAS is disabled
`setIfMatch` validates `If-Match` against `currentBytes`, but then relies on
`ConfigManager.set(..., currentBytes, ...)` to enforce that value at commit
time. `ConfigManager.set` ignores `oldValue` whenever
`druid.manager.config.enableCompareAndSwap` is false, so in that supported mode
a writer with a stale but locally matching ETag can still `insertOrUpdate` over
a concurrent change and receive 200. That violates the advertised `If-Match`
contract and can silently lose dynamic-config updates; conditional requests
should fail when CAS is disabled or use a storage-level compare-and-swap before
returning success.
##########
server/src/main/java/org/apache/druid/server/http/DynamicConfigEtagHelper.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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.druid.server.http;
+
+import org.apache.druid.common.config.ConfigEtag;
+import org.apache.druid.common.config.ConfigManager.SetResult;
+import org.apache.druid.error.InvalidInput;
+
+import javax.annotation.Nullable;
+import javax.servlet.http.HttpServletRequest;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.Response;
+
+/**
+ * HTTP-layer helpers for {@code ETag} / {@code If-Match} on dynamic-config
endpoints.
+ */
+public final class DynamicConfigEtagHelper
+{
+ private DynamicConfigEtagHelper()
+ {
+ }
+
+ /**
+ * Read the {@code If-Match} header. Returns {@code null} when absent. Throws
+ * 400 if present but blank (RFC 7232 ยง3.1 requires a non-empty value).
+ */
+ @Nullable
+ public static String getIfMatch(HttpServletRequest req)
+ {
+ if (req == null) {
+ return null;
+ }
+ final String header = req.getHeader(HttpHeaders.IF_MATCH);
+ if (header == null) {
+ return null;
+ }
+ if (header.trim().isEmpty()) {
+ throw InvalidInput.exception("If-Match header must not be blank");
Review Comment:
[P2] Blank If-Match escapes as an unhandled DruidException
`getIfMatch` documents a 400 for a blank `If-Match` header, but it throws
`InvalidInput.exception` directly. Several new callers invoke it outside
`ServletResourceUtils.buildReadResponse` and only catch
`IllegalArgumentException` or do not catch at all, so a request with a
present-but-blank `If-Match` escapes the resource method instead of producing
the documented 400 response. Catch `DruidException` at the call sites or make
the helper return an error response/result that callers handle explicitly.
--
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]
