Stricter UpdateSettingsRequest parsing on the REST layer (elastic#79228)

ywangd · Oct 19, 2021 · 4e89777 · 4e89777
1 parent 73f2d60
commit 4e89777
Show file tree

Hide file tree

Showing 2 changed files with 123 additions and 3 deletions.
diff --git a/.../main/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequest.java b/.../main/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequest.java
@@ -8,6 +8,8 @@
 
 package org.elasticsearch.action.admin.indices.settings.put;
 
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.elasticsearch.Version;
 import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.action.IndicesRequest;
@@ -27,6 +29,7 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
+import java.util.stream.Collectors;
 
 import static org.elasticsearch.action.ValidateActions.addValidationError;
 import static org.elasticsearch.common.settings.Settings.readSettingsFromStream;
@@ -39,6 +42,7 @@ public class UpdateSettingsRequest extends AcknowledgedRequest<UpdateSettingsReq
         implements IndicesRequest.Replaceable, ToXContentObject {
 
     public static final IndicesOptions DEFAULT_INDICES_OPTIONS = IndicesOptions.fromOptions(false, false, true, true);
+    private static final Logger LOGGER = LogManager.getLogger(UpdateSettingsRequest.class);
 
     private String[] indices;
     private IndicesOptions indicesOptions = DEFAULT_INDICES_OPTIONS;
@@ -204,12 +208,29 @@ public UpdateSettingsRequest fromXContent(XContentParser parser) throws IOExcept
             @SuppressWarnings("unchecked")
             Map<String, Object> innerBodySettingsMap = (Map<String, Object>) innerBodySettings;
             settings.putAll(innerBodySettingsMap);
+            checkMixedRequest(bodySettings);
         } else {
             settings.putAll(bodySettings);
         }
         return this.settings(settings);
     }
 
+    /**
+     * Checks if the request is a "mixed request". A mixed request contains both a
+     * "settings" map and other top-level properties. Detection of a mixed request
+     * will result in a warning message being logged.
+     */
+    private static void checkMixedRequest(Map<String, Object> bodySettings) {
+        assert bodySettings.containsKey("settings");
+        if (bodySettings.size() > 1) {
+            Map<String, Object> map = bodySettings.entrySet().stream()
+                .filter(k -> k.getKey().equals("settings") == false)
+                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
+            Settings ignoredSettings = Settings.builder().loadFromMap(map).build();
+            LOGGER.warn("Ignoring top-level request settings: " + ignoredSettings);
+        }
+    }
+
     @Override
     public String toString() {
         return "indices : " + Arrays.toString(indices) + "," + Strings.toString(this);

diff --git a/.../java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequestTests.java b/.../java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequestTests.java
@@ -8,19 +8,62 @@
 
 package org.elasticsearch.action.admin.indices.settings.put;
 
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.elasticsearch.common.bytes.BytesReference;
+import org.elasticsearch.common.logging.Loggers;
+import org.elasticsearch.common.xcontent.XContentHelper;
+import org.elasticsearch.test.MockLogAppender;
+import org.elasticsearch.test.XContentTestUtils;
+import org.elasticsearch.xcontent.ToXContent;
 import org.elasticsearch.xcontent.XContentBuilder;
+import org.elasticsearch.xcontent.XContentFactory;
 import org.elasticsearch.xcontent.XContentParser;
 import org.elasticsearch.test.AbstractXContentTestCase;
+import org.elasticsearch.xcontent.XContentType;
 
 import java.io.IOException;
 import java.util.function.Predicate;
 
 public class UpdateSettingsRequestTests extends AbstractXContentTestCase<UpdateSettingsRequest> {
 
-    private final boolean enclosedSettings = randomBoolean();
+    /** True if the setting should be enclosed in a settings map. */
+    private final boolean enclosedSettings;
+    /** True if the request should contain unknown top-level properties. */
+    private final boolean unknownFields;
+
+    public UpdateSettingsRequestTests() {
+        this(randomBoolean(), randomBoolean());
+    }
+
+    private UpdateSettingsRequestTests(boolean enclosedSettings, boolean unknownFields) {
+        this.enclosedSettings = enclosedSettings;
+        this.unknownFields = unknownFields;
+    }
+
+    final UpdateSettingsRequestTests withEnclosedSettings() {
+        return new UpdateSettingsRequestTests(true, unknownFields);
+    }
+
+    final UpdateSettingsRequestTests withoutEnclosedSettings() {
+        return new UpdateSettingsRequestTests(false, unknownFields);
+    }
+
+    final UpdateSettingsRequestTests withUnknownFields() {
+        return new UpdateSettingsRequestTests(enclosedSettings, true);
+    }
+
+    final UpdateSettingsRequestTests withoutUnknownFields() {
+        return new UpdateSettingsRequestTests(enclosedSettings, false);
+    }
 
     @Override
     protected UpdateSettingsRequest createTestInstance() {
+        return createTestInstance(enclosedSettings);
+    }
+
+    private static UpdateSettingsRequest createTestInstance(boolean enclosedSettings) {
         UpdateSettingsRequest testRequest = UpdateSettingsRequestSerializationTests.createTestItem();
         if (enclosedSettings) {
             UpdateSettingsRequest requestWithEnclosingSettings = new UpdateSettingsRequest(testRequest.settings()) {
@@ -45,9 +88,9 @@ protected UpdateSettingsRequest doParseInstance(XContentParser parser) throws IO
 
     @Override
     protected boolean supportsUnknownFields() {
-        // if the settings are enclose as a "settings" object
+        // if the settings are enclosed as a "settings" object
         // then all other top-level elements will be ignored during the parsing
-        return enclosedSettings;
+        return enclosedSettings && unknownFields;
     }
 
     @Override
@@ -73,4 +116,60 @@ protected boolean assertToXContentEquivalence() {
         return enclosedSettings == false;
     }
 
+    public void testWithEnclosedSettingsWithUnknownFields() throws IOException {
+        testFromXContent((new UpdateSettingsRequestTests()).withEnclosedSettings().withUnknownFields());
+    }
+
+    public void testWithEnclosedSettingsWithoutUnknownFields() throws IOException {
+        testFromXContent((new UpdateSettingsRequestTests()).withEnclosedSettings().withoutUnknownFields());
+    }
+
+    public void testWithoutEnclosedSettingsWithoutUnknownFields() throws IOException {
+        testFromXContent((new UpdateSettingsRequestTests()).withoutEnclosedSettings().withoutUnknownFields());
+    }
+
+    private static void testFromXContent(UpdateSettingsRequestTests test) throws IOException {
+        AbstractXContentTestCase.testFromXContent(NUMBER_OF_TEST_RUNS / 2,
+            test::createTestInstance,
+            test.supportsUnknownFields(),
+            test.getShuffleFieldsExceptions(),
+            test.getRandomFieldsExcludeFilter(),
+            test::createParser,
+            test::doParseInstance,
+            test::assertEqualInstances,
+            test.assertToXContentEquivalence(),
+            test.getToXContentParams());
+    }
+
+    /** Tests that mixed requests, containing both an enclosed settings and top-level fields, generate a log warning message. */
+    public void testLogIgnoredFields() throws Exception {
+        final Logger settingsLogger = LogManager.getLogger(UpdateSettingsRequest.class);
+        final MockLogAppender appender = new MockLogAppender();
+        Loggers.addAppender(settingsLogger, appender);
+        appender.start();
+        appender.addExpectation(
+            new MockLogAppender.SeenEventExpectation(
+                "Ignored settings warning",
+                settingsLogger.getName(),
+                Level.WARN,
+                "Ignoring top-level request settings"
+            )
+        );
+        try {
+            UpdateSettingsRequestTests test = (new UpdateSettingsRequestTests()).withEnclosedSettings().withUnknownFields();
+            UpdateSettingsRequest updateSettingsRequest = test.createTestInstance();
+            XContentType xContentType = randomFrom(XContentType.values());
+            BytesReference originalXContent = XContentHelper.toXContent(updateSettingsRequest, xContentType,
+                ToXContent.EMPTY_PARAMS, false);
+            BytesReference updatedXContent = XContentTestUtils.insertRandomFields(xContentType, originalXContent,
+                test.getRandomFieldsExcludeFilter(), random());
+            XContentParser parser = test.createParser(XContentFactory.xContent(xContentType), updatedXContent);
+            UpdateSettingsRequest parsedRequest = (new UpdateSettingsRequest()).fromXContent(parser);
+            test.assertEqualInstances(updateSettingsRequest, parsedRequest);
+            appender.assertAllExpectationsMatched();
+        } finally {
+            appender.stop();
+            Loggers.removeAppender(settingsLogger, appender);
+        }
+    }
 }