From b22ae81fc4470d911efc3d99ab3e9cd259e62e44 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Tue, 6 Apr 2021 22:53:11 -0700 Subject: [PATCH 01/18] Add config to skip storing audit payload if exceed limit --- .../org/apache/druid/audit/AuditManager.java | 2 + docs/configuration/index.md | 2 + .../druid/server/audit/SQLAuditManager.java | 14 ++- .../server/audit/SQLAuditManagerConfig.java | 8 ++ .../server/audit/SQLAuditManagerTest.java | 85 +++++++++++++++++++ 5 files changed, 110 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/druid/audit/AuditManager.java b/core/src/main/java/org/apache/druid/audit/AuditManager.java index 6389350fea03..0f11fc957f92 100644 --- a/core/src/main/java/org/apache/druid/audit/AuditManager.java +++ b/core/src/main/java/org/apache/druid/audit/AuditManager.java @@ -28,6 +28,8 @@ public interface AuditManager { + String PAYLOAD_SKIP_MESSAGE = "Payload was not stored as the payload size exceed limit configured by druid.audit.manager.skipStorePayloadExceedSizeByte"; + String X_DRUID_AUTHOR = "X-Druid-Author"; String X_DRUID_COMMENT = "X-Druid-Comment"; diff --git a/docs/configuration/index.md b/docs/configuration/index.md index 5a91a772b861..92fb8e872a6c 100644 --- a/docs/configuration/index.md +++ b/docs/configuration/index.md @@ -338,6 +338,8 @@ Coordinator and Overlord log changes to lookups, segment load/drop rules, dynami |--------|-----------|-------| |`druid.audit.manager.auditHistoryMillis`|Default duration for querying audit history.|1 week| |`druid.audit.manager.includePayloadAsDimensionInMetric`|Boolean flag on whether to add `payload` column in service metric.|false| +|`druid.audit.manager.skipStorePayloadExceedSizeByte`|The maximum size of audit payload, in bytes, to store in Druid's metadata store audit table. If the size of audit payload exceed this value, the audit log would be stored with a messaging indicating that the payload was omitted instead. Setting skipStorePayloadExceedSizeByte to -1 (default value) disables this check, meaning Druid will always store audit payload regardless of it's size.|-1| + ### Enabling Metrics diff --git a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java index 9ea53c670e1a..1312e53e0d91 100644 --- a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java +++ b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java @@ -114,6 +114,18 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException { emitter.emit(getAuditMetricEventBuilder(auditEntry).build("config/audit", 1)); + AuditEntry auditEntryToStore = auditEntry; + int payloadSize = jsonMapper.writeValueAsBytes(auditEntry.getPayload()).length; + if (config.getSkipStorePayloadExceedSizeByte() >=0 && payloadSize > config.getSkipStorePayloadExceedSizeByte()) { + auditEntryToStore = AuditEntry.builder() + .key(auditEntry.getKey()) + .type(auditEntry.getType()) + .auditInfo(auditEntry.getAuditInfo()) + .payload(PAYLOAD_SKIP_MESSAGE) + .auditTime(auditEntry.getAuditTime()) + .build(); + } + handle.createStatement( StringUtils.format( "INSERT INTO %s ( audit_key, type, author, comment, created_date, payload) VALUES (:audit_key, :type, :author, :comment, :created_date, :payload)", @@ -125,7 +137,7 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException .bind("author", auditEntry.getAuditInfo().getAuthor()) .bind("comment", auditEntry.getAuditInfo().getComment()) .bind("created_date", auditEntry.getAuditTime().toString()) - .bind("payload", jsonMapper.writeValueAsBytes(auditEntry)) + .bind("payload", jsonMapper.writeValueAsBytes(auditEntryToStore)) .execute(); } diff --git a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java index 4ef45d1ee925..bbde55d7b40c 100644 --- a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java +++ b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java @@ -31,6 +31,9 @@ public class SQLAuditManagerConfig @JsonProperty private boolean includePayloadAsDimensionInMetric = false; + @JsonProperty + private long skipStorePayloadExceedSizeByte = -1; + public long getAuditHistoryMillis() { return auditHistoryMillis; @@ -40,4 +43,9 @@ public boolean getIncludePayloadAsDimensionInMetric() { return includePayloadAsDimensionInMetric; } + + public long getSkipStorePayloadExceedSizeByte() + { + return skipStorePayloadExceedSizeByte; + } } diff --git a/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java b/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java index 13dc10ab316f..84acc57c4b61 100644 --- a/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java +++ b/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java @@ -270,6 +270,91 @@ public void testFetchAuditHistoryLimitZero() auditManager.fetchAuditHistory("testType", 0); } + @Test(timeout = 60_000L) + public void testCreateAuditEntryWithPayloadOverSkipPayloadLimit() throws IOException + { + SQLAuditManager auditManagerWithSkipStorePayloadExceedSizeByte = new SQLAuditManager( + connector, + derbyConnectorRule.metadataTablesConfigSupplier(), + new NoopServiceEmitter(), + mapper, + new SQLAuditManagerConfig() + { + @Override + public long getSkipStorePayloadExceedSizeByte() + { + return 10; + } + } + ); + + AuditEntry.Builder auditEntryBuilder = AuditEntry.builder() + .key("testKey") + .type("testType") + .auditInfo(new AuditInfo( + "testAuthor", + "testComment", + "127.0.0.1" + )) + .auditTime(DateTimes.of("1994-04-29T00:00:00Z")); + + AuditEntry auditEntryToStore = auditEntryBuilder.payload("payload audit to store").build(); + AuditEntry expectedWithSkipPayload = auditEntryBuilder.payload(AuditManager.PAYLOAD_SKIP_MESSAGE).build(); + + auditManagerWithSkipStorePayloadExceedSizeByte.doAudit(auditEntryToStore); + byte[] payload = connector.lookup( + derbyConnectorRule.metadataTablesConfigSupplier().get().getAuditTable(), + "audit_key", + "payload", + "testKey" + ); + + AuditEntry dbEntry = mapper.readValue(payload, AuditEntry.class); + Assert.assertEquals(expectedWithSkipPayload, dbEntry); + } + + @Test(timeout = 60_000L) + public void testCreateAuditEntryWithPayloadUnderSkipPayloadLimit() throws IOException + { + SQLAuditManager auditManagerWithSkipStorePayloadExceedSizeByte = new SQLAuditManager( + connector, + derbyConnectorRule.metadataTablesConfigSupplier(), + new NoopServiceEmitter(), + mapper, + new SQLAuditManagerConfig() + { + @Override + public long getSkipStorePayloadExceedSizeByte() + { + return 500; + } + } + ); + + AuditEntry.Builder auditEntryBuilder = AuditEntry.builder() + .key("testKey") + .type("testType") + .auditInfo(new AuditInfo( + "testAuthor", + "testComment", + "127.0.0.1" + )) + .auditTime(DateTimes.of("1994-04-29T00:00:00Z")); + + AuditEntry auditEntryToStore = auditEntryBuilder.payload("payload audit to store").build(); + + auditManagerWithSkipStorePayloadExceedSizeByte.doAudit(auditEntryToStore); + byte[] payload = connector.lookup( + derbyConnectorRule.metadataTablesConfigSupplier().get().getAuditTable(), + "audit_key", + "payload", + "testKey" + ); + + AuditEntry dbEntry = mapper.readValue(payload, AuditEntry.class); + Assert.assertEquals(auditEntryToStore, dbEntry); + } + @After public void cleanup() { From fae67e1eafa98be69745c426ff58ac914d5af09f Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Tue, 6 Apr 2021 22:54:27 -0700 Subject: [PATCH 02/18] fix checkstyle --- .../java/org/apache/druid/server/audit/SQLAuditManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java index 1312e53e0d91..3a204417b50e 100644 --- a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java +++ b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java @@ -116,7 +116,7 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException AuditEntry auditEntryToStore = auditEntry; int payloadSize = jsonMapper.writeValueAsBytes(auditEntry.getPayload()).length; - if (config.getSkipStorePayloadExceedSizeByte() >=0 && payloadSize > config.getSkipStorePayloadExceedSizeByte()) { + if (config.getSkipStorePayloadExceedSizeByte() >= 0 && payloadSize > config.getSkipStorePayloadExceedSizeByte()) { auditEntryToStore = AuditEntry.builder() .key(auditEntry.getKey()) .type(auditEntry.getType()) From ff1aab5ce6b2ca7700c353a1c778ac462ce624a6 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Wed, 7 Apr 2021 10:54:49 -0700 Subject: [PATCH 03/18] change config name --- .../java/org/apache/druid/audit/AuditManager.java | 2 +- docs/configuration/index.md | 2 +- .../apache/druid/server/audit/SQLAuditManager.java | 2 +- .../druid/server/audit/SQLAuditManagerConfig.java | 6 +++--- .../druid/server/audit/SQLAuditManagerTest.java | 12 ++++++------ website/.spelling | 1 + 6 files changed, 13 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/org/apache/druid/audit/AuditManager.java b/core/src/main/java/org/apache/druid/audit/AuditManager.java index 0f11fc957f92..3f682ec66649 100644 --- a/core/src/main/java/org/apache/druid/audit/AuditManager.java +++ b/core/src/main/java/org/apache/druid/audit/AuditManager.java @@ -28,7 +28,7 @@ public interface AuditManager { - String PAYLOAD_SKIP_MESSAGE = "Payload was not stored as the payload size exceed limit configured by druid.audit.manager.skipStorePayloadExceedSizeByte"; + String PAYLOAD_SKIP_MESSAGE = "Payload was not stored as the payload size exceed limit configured by druid.audit.manager.maxPayloadSizeBytes"; String X_DRUID_AUTHOR = "X-Druid-Author"; diff --git a/docs/configuration/index.md b/docs/configuration/index.md index 92fb8e872a6c..b1e149380165 100644 --- a/docs/configuration/index.md +++ b/docs/configuration/index.md @@ -338,7 +338,7 @@ Coordinator and Overlord log changes to lookups, segment load/drop rules, dynami |--------|-----------|-------| |`druid.audit.manager.auditHistoryMillis`|Default duration for querying audit history.|1 week| |`druid.audit.manager.includePayloadAsDimensionInMetric`|Boolean flag on whether to add `payload` column in service metric.|false| -|`druid.audit.manager.skipStorePayloadExceedSizeByte`|The maximum size of audit payload, in bytes, to store in Druid's metadata store audit table. If the size of audit payload exceed this value, the audit log would be stored with a messaging indicating that the payload was omitted instead. Setting skipStorePayloadExceedSizeByte to -1 (default value) disables this check, meaning Druid will always store audit payload regardless of it's size.|-1| +|`druid.audit.manager.maxPayloadSizeBytes`|The maximum size of audit payload, in bytes, to store in Druid's metadata store audit table. If the size of audit payload exceed this value, the audit log would be stored with a messaging indicating that the payload was omitted instead. Setting maxPayloadSizeBytes to -1 (default value) disables this check, meaning Druid will always store audit payload regardless of it's size.|-1| ### Enabling Metrics diff --git a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java index 3a204417b50e..053acdcae836 100644 --- a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java +++ b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java @@ -116,7 +116,7 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException AuditEntry auditEntryToStore = auditEntry; int payloadSize = jsonMapper.writeValueAsBytes(auditEntry.getPayload()).length; - if (config.getSkipStorePayloadExceedSizeByte() >= 0 && payloadSize > config.getSkipStorePayloadExceedSizeByte()) { + if (config.getMaxPayloadSizeBytes() >= 0 && payloadSize > config.getMaxPayloadSizeBytes()) { auditEntryToStore = AuditEntry.builder() .key(auditEntry.getKey()) .type(auditEntry.getType()) diff --git a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java index bbde55d7b40c..59ce6bceee20 100644 --- a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java +++ b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java @@ -32,7 +32,7 @@ public class SQLAuditManagerConfig private boolean includePayloadAsDimensionInMetric = false; @JsonProperty - private long skipStorePayloadExceedSizeByte = -1; + private long maxPayloadSizeBytes = -1; public long getAuditHistoryMillis() { @@ -44,8 +44,8 @@ public boolean getIncludePayloadAsDimensionInMetric() return includePayloadAsDimensionInMetric; } - public long getSkipStorePayloadExceedSizeByte() + public long getMaxPayloadSizeBytes() { - return skipStorePayloadExceedSizeByte; + return maxPayloadSizeBytes; } } diff --git a/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java b/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java index 84acc57c4b61..e6707e04a973 100644 --- a/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java +++ b/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java @@ -273,7 +273,7 @@ public void testFetchAuditHistoryLimitZero() @Test(timeout = 60_000L) public void testCreateAuditEntryWithPayloadOverSkipPayloadLimit() throws IOException { - SQLAuditManager auditManagerWithSkipStorePayloadExceedSizeByte = new SQLAuditManager( + SQLAuditManager auditManagerWithMaxPayloadSizeBytes = new SQLAuditManager( connector, derbyConnectorRule.metadataTablesConfigSupplier(), new NoopServiceEmitter(), @@ -281,7 +281,7 @@ public void testCreateAuditEntryWithPayloadOverSkipPayloadLimit() throws IOExcep new SQLAuditManagerConfig() { @Override - public long getSkipStorePayloadExceedSizeByte() + public long getMaxPayloadSizeBytes() { return 10; } @@ -301,7 +301,7 @@ public long getSkipStorePayloadExceedSizeByte() AuditEntry auditEntryToStore = auditEntryBuilder.payload("payload audit to store").build(); AuditEntry expectedWithSkipPayload = auditEntryBuilder.payload(AuditManager.PAYLOAD_SKIP_MESSAGE).build(); - auditManagerWithSkipStorePayloadExceedSizeByte.doAudit(auditEntryToStore); + auditManagerWithMaxPayloadSizeBytes.doAudit(auditEntryToStore); byte[] payload = connector.lookup( derbyConnectorRule.metadataTablesConfigSupplier().get().getAuditTable(), "audit_key", @@ -316,7 +316,7 @@ public long getSkipStorePayloadExceedSizeByte() @Test(timeout = 60_000L) public void testCreateAuditEntryWithPayloadUnderSkipPayloadLimit() throws IOException { - SQLAuditManager auditManagerWithSkipStorePayloadExceedSizeByte = new SQLAuditManager( + SQLAuditManager auditManagerWithMaxPayloadSizeBytes = new SQLAuditManager( connector, derbyConnectorRule.metadataTablesConfigSupplier(), new NoopServiceEmitter(), @@ -324,7 +324,7 @@ public void testCreateAuditEntryWithPayloadUnderSkipPayloadLimit() throws IOExce new SQLAuditManagerConfig() { @Override - public long getSkipStorePayloadExceedSizeByte() + public long getMaxPayloadSizeBytes() { return 500; } @@ -343,7 +343,7 @@ public long getSkipStorePayloadExceedSizeByte() AuditEntry auditEntryToStore = auditEntryBuilder.payload("payload audit to store").build(); - auditManagerWithSkipStorePayloadExceedSizeByte.doAudit(auditEntryToStore); + auditManagerWithMaxPayloadSizeBytes.doAudit(auditEntryToStore); byte[] payload = connector.lookup( derbyConnectorRule.metadataTablesConfigSupplier().get().getAuditTable(), "audit_key", diff --git a/website/.spelling b/website/.spelling index 017a1a91c125..c9cbe2fdb4b2 100644 --- a/website/.spelling +++ b/website/.spelling @@ -1752,6 +1752,7 @@ loadqueuepeon loadspec localStorage maxHeaderSize +maxPayloadSizeBytes maxQueuedBytes maxSize middlemanager From adc3731c9aa3cd89fadf18cb786126dbff90f3e0 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Wed, 7 Apr 2021 11:32:26 -0700 Subject: [PATCH 04/18] skip null fields for audit payload --- .../druid/common/config/ConfigSerde.java | 1 + .../common/config/JacksonConfigManager.java | 27 ++- .../config/JacksonConfigManagerTest.java | 157 ++++++++++++++++++ 3 files changed, 184 insertions(+), 1 deletion(-) create mode 100644 core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java diff --git a/core/src/main/java/org/apache/druid/common/config/ConfigSerde.java b/core/src/main/java/org/apache/druid/common/config/ConfigSerde.java index 119c0e5ad328..3302ac610054 100644 --- a/core/src/main/java/org/apache/druid/common/config/ConfigSerde.java +++ b/core/src/main/java/org/apache/druid/common/config/ConfigSerde.java @@ -25,5 +25,6 @@ public interface ConfigSerde { byte[] serialize(T obj); String serializeToString(T obj); + String serializeSkipNullToString(T obj); T deserialize(byte[] bytes); } diff --git a/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java b/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java index 1075e4c1aba6..4ca35566c77b 100644 --- a/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java +++ b/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java @@ -19,6 +19,7 @@ package org.apache.druid.common.config; +import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.ObjectMapper; @@ -38,6 +39,7 @@ public class JacksonConfigManager { private final ConfigManager configManager; private final ObjectMapper jsonMapper; + private final ObjectMapper jsonMapperSkipNull; private final AuditManager auditManager; @Inject @@ -50,6 +52,7 @@ public JacksonConfigManager( this.configManager = configManager; this.jsonMapper = jsonMapper; this.auditManager = auditManager; + this.jsonMapperSkipNull = jsonMapper.copy().setSerializationInclusion(JsonInclude.Include.NON_NULL); } public AtomicReference watch(String key, Class clazz) @@ -77,7 +80,7 @@ public SetResult set(String key, T val, AuditInfo auditInfo) .key(key) .type(key) .auditInfo(auditInfo) - .payload(configSerde.serializeToString(val)) + .payload(configSerde.serializeSkipNullToString(val)) .build() ); return configManager.set(key, configSerde, val); @@ -109,6 +112,17 @@ public String serializeToString(T obj) } } + @Override + public String serializeSkipNullToString(T obj) + { + try { + return jsonMapperSkipNull.writeValueAsString(obj); + } + catch (JsonProcessingException e) { + throw new RuntimeException(e); + } + } + @Override public T deserialize(byte[] bytes) { @@ -147,6 +161,17 @@ public String serializeToString(T obj) } } + @Override + public String serializeSkipNullToString(T obj) + { + try { + return jsonMapperSkipNull.writeValueAsString(obj); + } + catch (JsonProcessingException e) { + throw new RuntimeException(e); + } + } + @Override public T deserialize(byte[] bytes) { diff --git a/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java b/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java new file mode 100644 index 000000000000..915872c7064e --- /dev/null +++ b/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java @@ -0,0 +1,157 @@ +/* + * 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.common.config; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.druid.audit.AuditEntry; +import org.apache.druid.audit.AuditInfo; +import org.apache.druid.audit.AuditManager; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.MockitoJUnitRunner; + +import java.util.Objects; + +@RunWith(MockitoJUnitRunner.class) +public class JacksonConfigManagerTest +{ + @Mock + private ConfigManager mockConfigManager; + + @Mock + private AuditManager mockAuditManager; + + private JacksonConfigManager jacksonConfigManager; + @Before + public void setUp() + { + jacksonConfigManager = new JacksonConfigManager(mockConfigManager, new ObjectMapper(), mockAuditManager); + } + + @Test + public void testSetConfigWithNull() + { + TestConfig config = new TestConfig("version", null, 3); + AuditInfo auditInfo = new AuditInfo("maytas", "hello world", "111"); + String auditKey = "key"; + jacksonConfigManager.set(auditKey, config, auditInfo); + ArgumentCaptor auditEntryCapture = ArgumentCaptor.forClass( + AuditEntry.class); + Mockito.verify(mockAuditManager).doAudit( + auditEntryCapture.capture() + ); + AuditEntry actual = auditEntryCapture.getValue(); + Assert.assertEquals(auditKey, actual.getKey()); + Assert.assertEquals(auditKey, actual.getType()); + Assert.assertEquals(auditInfo, actual.getAuditInfo()); + Assert.assertEquals("{\"version\":\"version\",\"settingInt\":3}", actual.getPayload()); + } + + @Test + public void testSetConfigWithoutNull() + { + TestConfig config = new TestConfig("version", "string", 3); + AuditInfo auditInfo = new AuditInfo("maytas", "hello world", "111"); + String auditKey = "key"; + jacksonConfigManager.set(auditKey, config, auditInfo); + ArgumentCaptor auditEntryCapture = ArgumentCaptor.forClass( + AuditEntry.class); + Mockito.verify(mockAuditManager).doAudit( + auditEntryCapture.capture() + ); + AuditEntry actual = auditEntryCapture.getValue(); + Assert.assertEquals(auditKey, actual.getKey()); + Assert.assertEquals(auditKey, actual.getType()); + Assert.assertEquals(auditInfo, actual.getAuditInfo()); + Assert.assertEquals("{\"version\":\"version\",\"settingString\":\"string\",\"settingInt\":3}", actual.getPayload()); + } + + static class TestConfig + { + private final String version; + private final String settingString; + private final int settingInt; + + @JsonCreator + public TestConfig( + @JsonProperty("version") String version, + @JsonProperty("settingString") String settingString, + @JsonProperty("settingInt") int settingInt + ) + { + this.version = version; + this.settingString = settingString; + this.settingInt = settingInt; + } + + public String getVersion() + { + return version; + } + + public String getSettingString() + { + return settingString; + } + + public int getSettingInt() + { + return settingInt; + } + + @Override + public String toString() + { + return "TestConfig{" + + "version='" + version + '\'' + + ", settingString='" + settingString + '\'' + + ", settingInt=" + settingInt + + '}'; + } + + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + TestConfig that = (TestConfig) o; + return settingInt == that.settingInt && + Objects.equals(version, that.version) && + Objects.equals(settingString, that.settingString); + } + + @Override + public int hashCode() + { + return Objects.hash(version, settingString, settingInt); + } + } +} From a3aec87a3c19c35a62a17269d4dbf12154c47473 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Wed, 7 Apr 2021 11:35:17 -0700 Subject: [PATCH 05/18] fix checkstyle --- .../org/apache/druid/common/config/JacksonConfigManagerTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java b/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java index 915872c7064e..36029ef8ab88 100644 --- a/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java +++ b/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java @@ -46,6 +46,7 @@ public class JacksonConfigManagerTest private AuditManager mockAuditManager; private JacksonConfigManager jacksonConfigManager; + @Before public void setUp() { From 9fd23857adcfc32d8c73e1d0b234e4ce1284823f Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Wed, 7 Apr 2021 18:05:08 -0700 Subject: [PATCH 06/18] address comments --- .../org/apache/druid/audit/AuditManager.java | 4 ++ .../druid/common/config/ConfigSerde.java | 12 ++++++ .../common/config/JacksonConfigManager.java | 8 ++-- .../JsonOnlyNonNullValueSerialization.java | 37 +++++++++++++++++++ .../config/JacksonConfigManagerTest.java | 30 ++++++++++++++- docs/configuration/index.md | 2 +- .../apache/druid/jackson/JacksonModule.java | 11 ++++++ website/.spelling | 1 - 8 files changed, 98 insertions(+), 7 deletions(-) create mode 100644 core/src/main/java/org/apache/druid/guice/annotations/JsonOnlyNonNullValueSerialization.java diff --git a/core/src/main/java/org/apache/druid/audit/AuditManager.java b/core/src/main/java/org/apache/druid/audit/AuditManager.java index 3f682ec66649..1dace3237462 100644 --- a/core/src/main/java/org/apache/druid/audit/AuditManager.java +++ b/core/src/main/java/org/apache/druid/audit/AuditManager.java @@ -28,6 +28,10 @@ public interface AuditManager { + /** + * This String is the default message stored instead of the actual audit payload if the audit payload size + * exceeded the maximum size limit configuration + */ String PAYLOAD_SKIP_MESSAGE = "Payload was not stored as the payload size exceed limit configured by druid.audit.manager.maxPayloadSizeBytes"; String X_DRUID_AUTHOR = "X-Druid-Author"; diff --git a/core/src/main/java/org/apache/druid/common/config/ConfigSerde.java b/core/src/main/java/org/apache/druid/common/config/ConfigSerde.java index 3302ac610054..13d9e997c388 100644 --- a/core/src/main/java/org/apache/druid/common/config/ConfigSerde.java +++ b/core/src/main/java/org/apache/druid/common/config/ConfigSerde.java @@ -24,7 +24,19 @@ public interface ConfigSerde { byte[] serialize(T obj); + /** + * Serialize the input object to String + * @param obj to be serialize + * @return String serialization of the input + */ String serializeToString(T obj); + /** + * Serialize the input object to String, skipping serialization of any field with null value. This method can be + * used instead of {@link ConfigSerde#serializeToString(Object)} to reduce the size of the resulting String. + * + * @param obj to be serialize + * @return String serialization of the input without fields with null values + */ String serializeSkipNullToString(T obj); T deserialize(byte[] bytes); } diff --git a/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java b/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java index 4ca35566c77b..378c07e3c684 100644 --- a/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java +++ b/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java @@ -19,7 +19,6 @@ package org.apache.druid.common.config; -import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.ObjectMapper; @@ -28,6 +27,8 @@ import org.apache.druid.audit.AuditInfo; import org.apache.druid.audit.AuditManager; import org.apache.druid.common.config.ConfigManager.SetResult; +import org.apache.druid.guice.annotations.Json; +import org.apache.druid.guice.annotations.JsonOnlyNonNullValueSerialization; import org.apache.druid.java.util.common.jackson.JacksonUtils; import java.io.IOException; @@ -45,14 +46,15 @@ public class JacksonConfigManager @Inject public JacksonConfigManager( ConfigManager configManager, - ObjectMapper jsonMapper, + @Json ObjectMapper jsonMapper, + @JsonOnlyNonNullValueSerialization ObjectMapper jsonMapperOnlyNonNullValue, AuditManager auditManager ) { this.configManager = configManager; this.jsonMapper = jsonMapper; this.auditManager = auditManager; - this.jsonMapperSkipNull = jsonMapper.copy().setSerializationInclusion(JsonInclude.Include.NON_NULL); + this.jsonMapperSkipNull = jsonMapperOnlyNonNullValue; } public AtomicReference watch(String key, Class clazz) diff --git a/core/src/main/java/org/apache/druid/guice/annotations/JsonOnlyNonNullValueSerialization.java b/core/src/main/java/org/apache/druid/guice/annotations/JsonOnlyNonNullValueSerialization.java new file mode 100644 index 000000000000..9ec8f364836c --- /dev/null +++ b/core/src/main/java/org/apache/druid/guice/annotations/JsonOnlyNonNullValueSerialization.java @@ -0,0 +1,37 @@ +/* + * 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.guice.annotations; + +import com.google.inject.BindingAnnotation; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + */ +@Target({ElementType.FIELD, ElementType.PARAMETER, ElementType.METHOD}) +@Retention(RetentionPolicy.RUNTIME) +@BindingAnnotation +@PublicApi +public @interface JsonOnlyNonNullValueSerialization +{ +} diff --git a/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java b/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java index 36029ef8ab88..4f05cb025821 100644 --- a/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java +++ b/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java @@ -20,6 +20,7 @@ package org.apache.druid.common.config; import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.databind.ObjectMapper; import org.apache.druid.audit.AuditEntry; @@ -27,7 +28,9 @@ import org.apache.druid.audit.AuditManager; import org.junit.Assert; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; import org.mockito.Mock; @@ -47,10 +50,18 @@ public class JacksonConfigManagerTest private JacksonConfigManager jacksonConfigManager; + @Rule + public ExpectedException exception = ExpectedException.none(); + @Before - public void setUp() + public void setUp() throws Exception { - jacksonConfigManager = new JacksonConfigManager(mockConfigManager, new ObjectMapper(), mockAuditManager); + jacksonConfigManager = new JacksonConfigManager( + mockConfigManager, + new ObjectMapper(), + new ObjectMapper().setSerializationInclusion(JsonInclude.Include.NON_NULL), + mockAuditManager + ); } @Test @@ -91,6 +102,16 @@ public void testSetConfigWithoutNull() Assert.assertEquals("{\"version\":\"version\",\"settingString\":\"string\",\"settingInt\":3}", actual.getPayload()); } + @Test + public void testSetWithInvalidConfig() + { + AuditInfo auditInfo = new AuditInfo("maytas", "hello world", "111"); + String auditKey = "key"; + exception.expect(RuntimeException.class); + exception.expectMessage("InvalidDefinitionException"); + jacksonConfigManager.set(auditKey, new ClassThatJacksonCannotSerialize(), auditInfo); + } + static class TestConfig { private final String version; @@ -155,4 +176,9 @@ public int hashCode() return Objects.hash(version, settingString, settingInt); } } + + static class ClassThatJacksonCannotSerialize + { + + } } diff --git a/docs/configuration/index.md b/docs/configuration/index.md index b1e149380165..c0d2a54b1d59 100644 --- a/docs/configuration/index.md +++ b/docs/configuration/index.md @@ -338,7 +338,7 @@ Coordinator and Overlord log changes to lookups, segment load/drop rules, dynami |--------|-----------|-------| |`druid.audit.manager.auditHistoryMillis`|Default duration for querying audit history.|1 week| |`druid.audit.manager.includePayloadAsDimensionInMetric`|Boolean flag on whether to add `payload` column in service metric.|false| -|`druid.audit.manager.maxPayloadSizeBytes`|The maximum size of audit payload, in bytes, to store in Druid's metadata store audit table. If the size of audit payload exceed this value, the audit log would be stored with a messaging indicating that the payload was omitted instead. Setting maxPayloadSizeBytes to -1 (default value) disables this check, meaning Druid will always store audit payload regardless of it's size.|-1| +|`druid.audit.manager.maxPayloadSizeBytes`|The maximum size of audit payload, in bytes, to store in Druid's metadata store audit table. If the size of audit payload exceed this value, the audit log would be stored with a messaging indicating that the payload was omitted instead. Setting `maxPayloadSizeBytes` to -1 (default value) disables this check, meaning Druid will always store audit payload regardless of it's size.|-1| ### Enabling Metrics diff --git a/processing/src/main/java/org/apache/druid/jackson/JacksonModule.java b/processing/src/main/java/org/apache/druid/jackson/JacksonModule.java index 853a088dfbaa..f72121a2fcaa 100644 --- a/processing/src/main/java/org/apache/druid/jackson/JacksonModule.java +++ b/processing/src/main/java/org/apache/druid/jackson/JacksonModule.java @@ -19,6 +19,7 @@ package org.apache.druid.jackson; +import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.dataformat.smile.SmileFactory; import com.fasterxml.jackson.dataformat.smile.SmileGenerator; @@ -28,6 +29,7 @@ import com.google.inject.Provides; import org.apache.druid.guice.LazySingleton; import org.apache.druid.guice.annotations.Json; +import org.apache.druid.guice.annotations.JsonOnlyNonNullValueSerialization; import org.apache.druid.guice.annotations.Smile; /** @@ -46,6 +48,15 @@ public ObjectMapper jsonMapper() return new DefaultObjectMapper(); } + /** + * Provides ObjectMapper that suppress serializing properties with null values + */ + @Provides @LazySingleton @JsonOnlyNonNullValueSerialization + public ObjectMapper jsonMapperOnlyNonNullValue() + { + return new DefaultObjectMapper().setSerializationInclusion(JsonInclude.Include.NON_NULL); + } + @Provides @LazySingleton @Smile public ObjectMapper smileMapper() { diff --git a/website/.spelling b/website/.spelling index c9cbe2fdb4b2..017a1a91c125 100644 --- a/website/.spelling +++ b/website/.spelling @@ -1752,7 +1752,6 @@ loadqueuepeon loadspec localStorage maxHeaderSize -maxPayloadSizeBytes maxQueuedBytes maxSize middlemanager From 4317700415f86973124a029541b699f5f1c032fc Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Wed, 7 Apr 2021 19:01:51 -0700 Subject: [PATCH 07/18] fix guice --- .../org/apache/druid/guice/DruidSecondaryModule.java | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/core/src/main/java/org/apache/druid/guice/DruidSecondaryModule.java b/core/src/main/java/org/apache/druid/guice/DruidSecondaryModule.java index 5e4db78ac823..b8a3d9f2c39f 100644 --- a/core/src/main/java/org/apache/druid/guice/DruidSecondaryModule.java +++ b/core/src/main/java/org/apache/druid/guice/DruidSecondaryModule.java @@ -30,6 +30,7 @@ import com.google.inject.Module; import com.google.inject.Provides; import org.apache.druid.guice.annotations.Json; +import org.apache.druid.guice.annotations.JsonOnlyNonNullValueSerialization; import org.apache.druid.guice.annotations.Smile; import org.skife.config.ConfigurationObjectFactory; @@ -41,6 +42,7 @@ public class DruidSecondaryModule implements Module private final Properties properties; private final ConfigurationObjectFactory factory; private final ObjectMapper jsonMapper; + private final ObjectMapper jsonMapperOnlyNonNullValueSerialization; private final ObjectMapper smileMapper; private final Validator validator; @@ -49,6 +51,7 @@ public DruidSecondaryModule( Properties properties, ConfigurationObjectFactory factory, @Json ObjectMapper jsonMapper, + @JsonOnlyNonNullValueSerialization ObjectMapper jsonMapperOnlyNonNullValueSerialization, @Smile ObjectMapper smileMapper, Validator validator ) @@ -56,6 +59,7 @@ public DruidSecondaryModule( this.properties = properties; this.factory = factory; this.jsonMapper = jsonMapper; + this.jsonMapperOnlyNonNullValueSerialization = jsonMapperOnlyNonNullValueSerialization; this.smileMapper = smileMapper; this.validator = validator; } @@ -78,6 +82,13 @@ public ObjectMapper getJsonMapper(final Injector injector) return jsonMapper; } + @Provides @LazySingleton @JsonOnlyNonNullValueSerialization + public ObjectMapper getJsonMapperOnlyNonNullValueSerialization(final Injector injector) + { + setupJackson(injector, jsonMapperOnlyNonNullValueSerialization); + return jsonMapperOnlyNonNullValueSerialization; + } + @Provides @LazySingleton @Smile public ObjectMapper getSmileMapper(Injector injector) { From cdc45b7d3359447930f7c995429c130467614c95 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Wed, 7 Apr 2021 22:29:18 -0700 Subject: [PATCH 08/18] fix test --- .../druid/common/config/ConfigSerde.java | 13 +++----- .../common/config/JacksonConfigManager.java | 32 +++---------------- .../config/JacksonConfigManagerTest.java | 2 +- 3 files changed, 10 insertions(+), 37 deletions(-) diff --git a/core/src/main/java/org/apache/druid/common/config/ConfigSerde.java b/core/src/main/java/org/apache/druid/common/config/ConfigSerde.java index 13d9e997c388..0c350da09d50 100644 --- a/core/src/main/java/org/apache/druid/common/config/ConfigSerde.java +++ b/core/src/main/java/org/apache/druid/common/config/ConfigSerde.java @@ -25,18 +25,13 @@ public interface ConfigSerde { byte[] serialize(T obj); /** - * Serialize the input object to String - * @param obj to be serialize - * @return String serialization of the input - */ - String serializeToString(T obj); - /** - * Serialize the input object to String, skipping serialization of any field with null value. This method can be - * used instead of {@link ConfigSerde#serializeToString(Object)} to reduce the size of the resulting String. + * Serialize object to String * * @param obj to be serialize + * @param skipNull if true, then skip serialization of any field with null value. + * This can be used to reduce the size of the resulting String. * @return String serialization of the input without fields with null values */ - String serializeSkipNullToString(T obj); + String serializeToString(T obj, boolean skipNull); T deserialize(byte[] bytes); } diff --git a/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java b/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java index 378c07e3c684..fafaf2c9eff1 100644 --- a/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java +++ b/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java @@ -82,7 +82,7 @@ public SetResult set(String key, T val, AuditInfo auditInfo) .key(key) .type(key) .auditInfo(auditInfo) - .payload(configSerde.serializeSkipNullToString(val)) + .payload(configSerde.serializeToString(val, true)) .build() ); return configManager.set(key, configSerde, val); @@ -104,21 +104,10 @@ public byte[] serialize(T obj) } @Override - public String serializeToString(T obj) + public String serializeToString(T obj, boolean skipNull) { try { - return jsonMapper.writeValueAsString(obj); - } - catch (JsonProcessingException e) { - throw new RuntimeException(e); - } - } - - @Override - public String serializeSkipNullToString(T obj) - { - try { - return jsonMapperSkipNull.writeValueAsString(obj); + return skipNull ? jsonMapperSkipNull.writeValueAsString(obj) : jsonMapper.writeValueAsString(obj); } catch (JsonProcessingException e) { throw new RuntimeException(e); @@ -153,21 +142,10 @@ public byte[] serialize(T obj) } @Override - public String serializeToString(T obj) - { - try { - return jsonMapper.writeValueAsString(obj); - } - catch (JsonProcessingException e) { - throw new RuntimeException(e); - } - } - - @Override - public String serializeSkipNullToString(T obj) + public String serializeToString(T obj, boolean skipNull) { try { - return jsonMapperSkipNull.writeValueAsString(obj); + return skipNull ? jsonMapperSkipNull.writeValueAsString(obj) : jsonMapper.writeValueAsString(obj); } catch (JsonProcessingException e) { throw new RuntimeException(e); diff --git a/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java b/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java index 4f05cb025821..432c9c0a3b20 100644 --- a/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java +++ b/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java @@ -54,7 +54,7 @@ public class JacksonConfigManagerTest public ExpectedException exception = ExpectedException.none(); @Before - public void setUp() throws Exception + public void setUp() { jacksonConfigManager = new JacksonConfigManager( mockConfigManager, From 563b1d06bb9a8e3ae6e50222d7eb8c74c3b2523e Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Thu, 8 Apr 2021 12:21:53 -0700 Subject: [PATCH 09/18] add tests --- .../common/config/JacksonConfigManager.java | 7 +++-- .../config/JacksonConfigManagerTest.java | 27 +++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java b/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java index fafaf2c9eff1..01d16fd8856a 100644 --- a/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java +++ b/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java @@ -22,6 +22,7 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.common.annotations.VisibleForTesting; import com.google.inject.Inject; import org.apache.druid.audit.AuditEntry; import org.apache.druid.audit.AuditInfo; @@ -88,7 +89,8 @@ public SetResult set(String key, T val, AuditInfo auditInfo) return configManager.set(key, configSerde, val); } - private ConfigSerde create(final Class clazz, final T defaultVal) + @VisibleForTesting + ConfigSerde create(final Class clazz, final T defaultVal) { return new ConfigSerde() { @@ -126,7 +128,8 @@ public T deserialize(byte[] bytes) }; } - private ConfigSerde create(final TypeReference clazz, final T defaultVal) + @VisibleForTesting + ConfigSerde create(final TypeReference clazz, final T defaultVal) { return new ConfigSerde() { diff --git a/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java b/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java index 432c9c0a3b20..4cdbe97d980f 100644 --- a/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java +++ b/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java @@ -22,6 +22,7 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.ObjectMapper; import org.apache.druid.audit.AuditEntry; import org.apache.druid.audit.AuditInfo; @@ -49,6 +50,8 @@ public class JacksonConfigManagerTest private AuditManager mockAuditManager; private JacksonConfigManager jacksonConfigManager; + private ConfigSerde configConfigSerdeFromTypeReference; + private ConfigSerde configConfigSerdeFromClass; @Rule public ExpectedException exception = ExpectedException.none(); @@ -62,6 +65,10 @@ public void setUp() new ObjectMapper().setSerializationInclusion(JsonInclude.Include.NON_NULL), mockAuditManager ); + configConfigSerdeFromTypeReference = jacksonConfigManager.create(new TypeReference() + { + }, null); + configConfigSerdeFromClass = jacksonConfigManager.create(TestConfig.class, null); } @Test @@ -83,6 +90,26 @@ public void testSetConfigWithNull() Assert.assertEquals("{\"version\":\"version\",\"settingInt\":3}", actual.getPayload()); } + @Test + public void testSerializeToStringWithSkipNullTrue() + { + TestConfig config = new TestConfig("version", null, 3); + String actual = configConfigSerdeFromTypeReference.serializeToString(config, true); + Assert.assertEquals("{\"version\":\"version\",\"settingInt\":3}", actual); + actual = configConfigSerdeFromClass.serializeToString(config, true); + Assert.assertEquals("{\"version\":\"version\",\"settingInt\":3}", actual); + } + + @Test + public void testSerializeToStringWithSkipNullFalse() + { + TestConfig config = new TestConfig("version", null, 3); + String actual = configConfigSerdeFromTypeReference.serializeToString(config, false); + Assert.assertEquals("{\"version\":\"version\",\"settingString\":null,\"settingInt\":3}", actual); + actual = configConfigSerdeFromClass.serializeToString(config, false); + Assert.assertEquals("{\"version\":\"version\",\"settingString\":null,\"settingInt\":3}", actual); + } + @Test public void testSetConfigWithoutNull() { From 73da5da305d3aac8aa8b1f360c88b1dfd274822d Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Thu, 8 Apr 2021 19:37:16 -0700 Subject: [PATCH 10/18] address comments --- .../main/java/org/apache/druid/audit/AuditManager.java | 2 +- .../java/org/apache/druid/common/config/ConfigSerde.java | 2 +- .../apache/druid/common/config/JacksonConfigManager.java | 8 +++++--- .../java/org/apache/druid/guice/DruidSecondaryModule.java | 6 +++--- ...nlyNonNullValueSerialization.java => JsonNonNull.java} | 3 ++- docs/configuration/index.md | 2 +- .../main/java/org/apache/druid/jackson/JacksonModule.java | 4 ++-- .../org/apache/druid/server/audit/SQLAuditManager.java | 2 +- .../apache/druid/server/audit/SQLAuditManagerConfig.java | 5 +++-- .../apache/druid/server/audit/SQLAuditManagerTest.java | 5 +++-- 10 files changed, 22 insertions(+), 17 deletions(-) rename core/src/main/java/org/apache/druid/guice/annotations/{JsonOnlyNonNullValueSerialization.java => JsonNonNull.java} (90%) diff --git a/core/src/main/java/org/apache/druid/audit/AuditManager.java b/core/src/main/java/org/apache/druid/audit/AuditManager.java index 1dace3237462..9479476c9d1c 100644 --- a/core/src/main/java/org/apache/druid/audit/AuditManager.java +++ b/core/src/main/java/org/apache/druid/audit/AuditManager.java @@ -32,7 +32,7 @@ public interface AuditManager * This String is the default message stored instead of the actual audit payload if the audit payload size * exceeded the maximum size limit configuration */ - String PAYLOAD_SKIP_MESSAGE = "Payload was not stored as the payload size exceed limit configured by druid.audit.manager.maxPayloadSizeBytes"; + String PAYLOAD_SKIP_MESSAGE = "Payload was not stored as its size exceeds the limit [%d] configured by druid.audit.manager.maxPayloadSizeBytes"; String X_DRUID_AUTHOR = "X-Druid-Author"; diff --git a/core/src/main/java/org/apache/druid/common/config/ConfigSerde.java b/core/src/main/java/org/apache/druid/common/config/ConfigSerde.java index 0c350da09d50..708d16d8b190 100644 --- a/core/src/main/java/org/apache/druid/common/config/ConfigSerde.java +++ b/core/src/main/java/org/apache/druid/common/config/ConfigSerde.java @@ -30,7 +30,7 @@ public interface ConfigSerde * @param obj to be serialize * @param skipNull if true, then skip serialization of any field with null value. * This can be used to reduce the size of the resulting String. - * @return String serialization of the input without fields with null values + * @return String serialization of the input */ String serializeToString(T obj, boolean skipNull); T deserialize(byte[] bytes); diff --git a/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java b/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java index 01d16fd8856a..83b850ad0af7 100644 --- a/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java +++ b/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java @@ -29,7 +29,7 @@ import org.apache.druid.audit.AuditManager; import org.apache.druid.common.config.ConfigManager.SetResult; import org.apache.druid.guice.annotations.Json; -import org.apache.druid.guice.annotations.JsonOnlyNonNullValueSerialization; +import org.apache.druid.guice.annotations.JsonNonNull; import org.apache.druid.java.util.common.jackson.JacksonUtils; import java.io.IOException; @@ -48,7 +48,7 @@ public class JacksonConfigManager public JacksonConfigManager( ConfigManager configManager, @Json ObjectMapper jsonMapper, - @JsonOnlyNonNullValueSerialization ObjectMapper jsonMapperOnlyNonNullValue, + @JsonNonNull ObjectMapper jsonMapperOnlyNonNullValue, AuditManager auditManager ) { @@ -78,12 +78,14 @@ public SetResult set(String key, T val, AuditInfo auditInfo) ConfigSerde configSerde = create(val.getClass(), null); // Audit and actual config change are done in separate transactions // there can be phantom audits and reOrdering in audit changes as well. + if (con) + String serializedPayload = configSerde.serializeToString(val, true); auditManager.doAudit( AuditEntry.builder() .key(key) .type(key) .auditInfo(auditInfo) - .payload(configSerde.serializeToString(val, true)) + .payload(serializedPayload) .build() ); return configManager.set(key, configSerde, val); diff --git a/core/src/main/java/org/apache/druid/guice/DruidSecondaryModule.java b/core/src/main/java/org/apache/druid/guice/DruidSecondaryModule.java index b8a3d9f2c39f..bb03146a05bc 100644 --- a/core/src/main/java/org/apache/druid/guice/DruidSecondaryModule.java +++ b/core/src/main/java/org/apache/druid/guice/DruidSecondaryModule.java @@ -30,7 +30,7 @@ import com.google.inject.Module; import com.google.inject.Provides; import org.apache.druid.guice.annotations.Json; -import org.apache.druid.guice.annotations.JsonOnlyNonNullValueSerialization; +import org.apache.druid.guice.annotations.JsonNonNull; import org.apache.druid.guice.annotations.Smile; import org.skife.config.ConfigurationObjectFactory; @@ -51,7 +51,7 @@ public DruidSecondaryModule( Properties properties, ConfigurationObjectFactory factory, @Json ObjectMapper jsonMapper, - @JsonOnlyNonNullValueSerialization ObjectMapper jsonMapperOnlyNonNullValueSerialization, + @JsonNonNull ObjectMapper jsonMapperOnlyNonNullValueSerialization, @Smile ObjectMapper smileMapper, Validator validator ) @@ -82,7 +82,7 @@ public ObjectMapper getJsonMapper(final Injector injector) return jsonMapper; } - @Provides @LazySingleton @JsonOnlyNonNullValueSerialization + @Provides @LazySingleton @JsonNonNull public ObjectMapper getJsonMapperOnlyNonNullValueSerialization(final Injector injector) { setupJackson(injector, jsonMapperOnlyNonNullValueSerialization); diff --git a/core/src/main/java/org/apache/druid/guice/annotations/JsonOnlyNonNullValueSerialization.java b/core/src/main/java/org/apache/druid/guice/annotations/JsonNonNull.java similarity index 90% rename from core/src/main/java/org/apache/druid/guice/annotations/JsonOnlyNonNullValueSerialization.java rename to core/src/main/java/org/apache/druid/guice/annotations/JsonNonNull.java index 9ec8f364836c..ae4672f01f46 100644 --- a/core/src/main/java/org/apache/druid/guice/annotations/JsonOnlyNonNullValueSerialization.java +++ b/core/src/main/java/org/apache/druid/guice/annotations/JsonNonNull.java @@ -27,11 +27,12 @@ import java.lang.annotation.Target; /** + * The ObjectMapper of this annotation will skip serialization of any field with null value. */ @Target({ElementType.FIELD, ElementType.PARAMETER, ElementType.METHOD}) @Retention(RetentionPolicy.RUNTIME) @BindingAnnotation @PublicApi -public @interface JsonOnlyNonNullValueSerialization +public @interface JsonNonNull { } diff --git a/docs/configuration/index.md b/docs/configuration/index.md index c0d2a54b1d59..00d7a281a411 100644 --- a/docs/configuration/index.md +++ b/docs/configuration/index.md @@ -338,7 +338,7 @@ Coordinator and Overlord log changes to lookups, segment load/drop rules, dynami |--------|-----------|-------| |`druid.audit.manager.auditHistoryMillis`|Default duration for querying audit history.|1 week| |`druid.audit.manager.includePayloadAsDimensionInMetric`|Boolean flag on whether to add `payload` column in service metric.|false| -|`druid.audit.manager.maxPayloadSizeBytes`|The maximum size of audit payload, in bytes, to store in Druid's metadata store audit table. If the size of audit payload exceed this value, the audit log would be stored with a messaging indicating that the payload was omitted instead. Setting `maxPayloadSizeBytes` to -1 (default value) disables this check, meaning Druid will always store audit payload regardless of it's size.|-1| +|`druid.audit.manager.maxPayloadSizeBytes`|The maximum size of audit payload, in bytes, to store in Druid's metadata store audit table. If the size of audit payload exceed this value, the audit log would be stored with a messaging indicating that the payload was omitted instead. Setting `maxPayloadSizeBytes` to -1 (default value) disables this check, meaning Druid will always store audit payload regardless of it's size. Human-readable format is supported, see [here](human-readable-byte.md). |-1| ### Enabling Metrics diff --git a/processing/src/main/java/org/apache/druid/jackson/JacksonModule.java b/processing/src/main/java/org/apache/druid/jackson/JacksonModule.java index f72121a2fcaa..a7c947d365e6 100644 --- a/processing/src/main/java/org/apache/druid/jackson/JacksonModule.java +++ b/processing/src/main/java/org/apache/druid/jackson/JacksonModule.java @@ -29,7 +29,7 @@ import com.google.inject.Provides; import org.apache.druid.guice.LazySingleton; import org.apache.druid.guice.annotations.Json; -import org.apache.druid.guice.annotations.JsonOnlyNonNullValueSerialization; +import org.apache.druid.guice.annotations.JsonNonNull; import org.apache.druid.guice.annotations.Smile; /** @@ -51,7 +51,7 @@ public ObjectMapper jsonMapper() /** * Provides ObjectMapper that suppress serializing properties with null values */ - @Provides @LazySingleton @JsonOnlyNonNullValueSerialization + @Provides @LazySingleton @JsonNonNull public ObjectMapper jsonMapperOnlyNonNullValue() { return new DefaultObjectMapper().setSerializationInclusion(JsonInclude.Include.NON_NULL); diff --git a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java index 053acdcae836..340d5c3e478c 100644 --- a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java +++ b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java @@ -121,7 +121,7 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException .key(auditEntry.getKey()) .type(auditEntry.getType()) .auditInfo(auditEntry.getAuditInfo()) - .payload(PAYLOAD_SKIP_MESSAGE) + .payload(StringUtils.format(PAYLOAD_SKIP_MESSAGE, config.getMaxPayloadSizeBytes())) .auditTime(auditEntry.getAuditTime()) .build(); } diff --git a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java index 59ce6bceee20..20c3fac00346 100644 --- a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java +++ b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java @@ -20,6 +20,7 @@ package org.apache.druid.server.audit; import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.druid.java.util.common.HumanReadableBytes; /** */ @@ -32,7 +33,7 @@ public class SQLAuditManagerConfig private boolean includePayloadAsDimensionInMetric = false; @JsonProperty - private long maxPayloadSizeBytes = -1; + private HumanReadableBytes maxPayloadSizeBytes = HumanReadableBytes.valueOf(-1); public long getAuditHistoryMillis() { @@ -46,6 +47,6 @@ public boolean getIncludePayloadAsDimensionInMetric() public long getMaxPayloadSizeBytes() { - return maxPayloadSizeBytes; + return maxPayloadSizeBytes.getBytes(); } } diff --git a/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java b/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java index e6707e04a973..538fe6e40df6 100644 --- a/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java +++ b/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java @@ -273,6 +273,7 @@ public void testFetchAuditHistoryLimitZero() @Test(timeout = 60_000L) public void testCreateAuditEntryWithPayloadOverSkipPayloadLimit() throws IOException { + int maxPayloadSize = 10; SQLAuditManager auditManagerWithMaxPayloadSizeBytes = new SQLAuditManager( connector, derbyConnectorRule.metadataTablesConfigSupplier(), @@ -283,7 +284,7 @@ public void testCreateAuditEntryWithPayloadOverSkipPayloadLimit() throws IOExcep @Override public long getMaxPayloadSizeBytes() { - return 10; + return maxPayloadSize; } } ); @@ -299,7 +300,7 @@ public long getMaxPayloadSizeBytes() .auditTime(DateTimes.of("1994-04-29T00:00:00Z")); AuditEntry auditEntryToStore = auditEntryBuilder.payload("payload audit to store").build(); - AuditEntry expectedWithSkipPayload = auditEntryBuilder.payload(AuditManager.PAYLOAD_SKIP_MESSAGE).build(); + AuditEntry expectedWithSkipPayload = auditEntryBuilder.payload(StringUtils.format(AuditManager.PAYLOAD_SKIP_MESSAGE, maxPayloadSize)).build(); auditManagerWithMaxPayloadSizeBytes.doAudit(auditEntryToStore); byte[] payload = connector.lookup( From ba77a167e65151a23328d6601be1448f071a2842 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Thu, 8 Apr 2021 20:17:36 -0700 Subject: [PATCH 11/18] address comments --- .../org/apache/druid/audit/AuditManager.java | 7 +- .../common/config/JacksonConfigManager.java | 11 +- .../config/JacksonConfigManagerTest.java | 63 ++-- .../druid/server/audit/SQLAuditManager.java | 11 +- .../server/audit/SQLAuditManagerConfig.java | 9 + .../server/audit/SQLAuditManagerTest.java | 273 +++++++++++------- 6 files changed, 204 insertions(+), 170 deletions(-) diff --git a/core/src/main/java/org/apache/druid/audit/AuditManager.java b/core/src/main/java/org/apache/druid/audit/AuditManager.java index 9479476c9d1c..204978771f15 100644 --- a/core/src/main/java/org/apache/druid/audit/AuditManager.java +++ b/core/src/main/java/org/apache/druid/audit/AuditManager.java @@ -20,6 +20,7 @@ package org.apache.druid.audit; +import org.apache.druid.common.config.ConfigSerde; import org.joda.time.Interval; import org.skife.jdbi.v2.Handle; @@ -39,10 +40,10 @@ public interface AuditManager String X_DRUID_COMMENT = "X-Druid-Comment"; /** - * inserts an audit Entry in the Audit Table - * @param auditEntry + * inserts an audit entry in the Audit Table + * @param key */ - void doAudit(AuditEntry auditEntry); + void doAudit(String key, String type, AuditInfo auditInfo, T payload, ConfigSerde configSerde); /** * inserts an audit Entry in audit table using the handler provided diff --git a/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java b/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java index 83b850ad0af7..1c588ff4d0b1 100644 --- a/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java +++ b/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java @@ -78,16 +78,7 @@ public SetResult set(String key, T val, AuditInfo auditInfo) ConfigSerde configSerde = create(val.getClass(), null); // Audit and actual config change are done in separate transactions // there can be phantom audits and reOrdering in audit changes as well. - if (con) - String serializedPayload = configSerde.serializeToString(val, true); - auditManager.doAudit( - AuditEntry.builder() - .key(key) - .type(key) - .auditInfo(auditInfo) - .payload(serializedPayload) - .build() - ); + auditManager.doAudit(key, key, auditInfo, val, configSerde); return configManager.set(key, configSerde, val); } diff --git a/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java b/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java index 4cdbe97d980f..a5f6f7dc85be 100644 --- a/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java +++ b/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java @@ -50,8 +50,6 @@ public class JacksonConfigManagerTest private AuditManager mockAuditManager; private JacksonConfigManager jacksonConfigManager; - private ConfigSerde configConfigSerdeFromTypeReference; - private ConfigSerde configConfigSerdeFromClass; @Rule public ExpectedException exception = ExpectedException.none(); @@ -65,34 +63,15 @@ public void setUp() new ObjectMapper().setSerializationInclusion(JsonInclude.Include.NON_NULL), mockAuditManager ); - configConfigSerdeFromTypeReference = jacksonConfigManager.create(new TypeReference() - { - }, null); - configConfigSerdeFromClass = jacksonConfigManager.create(TestConfig.class, null); - } - - @Test - public void testSetConfigWithNull() - { - TestConfig config = new TestConfig("version", null, 3); - AuditInfo auditInfo = new AuditInfo("maytas", "hello world", "111"); - String auditKey = "key"; - jacksonConfigManager.set(auditKey, config, auditInfo); - ArgumentCaptor auditEntryCapture = ArgumentCaptor.forClass( - AuditEntry.class); - Mockito.verify(mockAuditManager).doAudit( - auditEntryCapture.capture() - ); - AuditEntry actual = auditEntryCapture.getValue(); - Assert.assertEquals(auditKey, actual.getKey()); - Assert.assertEquals(auditKey, actual.getType()); - Assert.assertEquals(auditInfo, actual.getAuditInfo()); - Assert.assertEquals("{\"version\":\"version\",\"settingInt\":3}", actual.getPayload()); } @Test public void testSerializeToStringWithSkipNullTrue() { + ConfigSerde configConfigSerdeFromTypeReference = jacksonConfigManager.create(new TypeReference() + { + }, null); + ConfigSerde configConfigSerdeFromClass = jacksonConfigManager.create(TestConfig.class, null); TestConfig config = new TestConfig("version", null, 3); String actual = configConfigSerdeFromTypeReference.serializeToString(config, true); Assert.assertEquals("{\"version\":\"version\",\"settingInt\":3}", actual); @@ -103,6 +82,10 @@ public void testSerializeToStringWithSkipNullTrue() @Test public void testSerializeToStringWithSkipNullFalse() { + ConfigSerde configConfigSerdeFromTypeReference = jacksonConfigManager.create(new TypeReference() + { + }, null); + ConfigSerde configConfigSerdeFromClass = jacksonConfigManager.create(TestConfig.class, null); TestConfig config = new TestConfig("version", null, 3); String actual = configConfigSerdeFromTypeReference.serializeToString(config, false); Assert.assertEquals("{\"version\":\"version\",\"settingString\":null,\"settingInt\":3}", actual); @@ -111,34 +94,26 @@ public void testSerializeToStringWithSkipNullFalse() } @Test - public void testSetConfigWithoutNull() + public void testSetWithInvalidConfigForConfigSerdeFromTypeReference() { - TestConfig config = new TestConfig("version", "string", 3); - AuditInfo auditInfo = new AuditInfo("maytas", "hello world", "111"); - String auditKey = "key"; - jacksonConfigManager.set(auditKey, config, auditInfo); - ArgumentCaptor auditEntryCapture = ArgumentCaptor.forClass( - AuditEntry.class); - Mockito.verify(mockAuditManager).doAudit( - auditEntryCapture.capture() - ); - AuditEntry actual = auditEntryCapture.getValue(); - Assert.assertEquals(auditKey, actual.getKey()); - Assert.assertEquals(auditKey, actual.getType()); - Assert.assertEquals(auditInfo, actual.getAuditInfo()); - Assert.assertEquals("{\"version\":\"version\",\"settingString\":\"string\",\"settingInt\":3}", actual.getPayload()); + ConfigSerde configConfigSerdeFromTypeReference = jacksonConfigManager.create(new TypeReference() + { + }, null); + exception.expect(RuntimeException.class); + exception.expectMessage("InvalidDefinitionException"); + configConfigSerdeFromTypeReference.serializeToString(new ClassThatJacksonCannotSerialize(), false); } @Test - public void testSetWithInvalidConfig() + public void testSetWithInvalidConfigForConfigSerdeFromClass() { - AuditInfo auditInfo = new AuditInfo("maytas", "hello world", "111"); - String auditKey = "key"; + ConfigSerde configConfigSerdeFromClass = jacksonConfigManager.create(ClassThatJacksonCannotSerialize.class, null); exception.expect(RuntimeException.class); exception.expectMessage("InvalidDefinitionException"); - jacksonConfigManager.set(auditKey, new ClassThatJacksonCannotSerialize(), auditInfo); + configConfigSerdeFromClass.serializeToString(new ClassThatJacksonCannotSerialize(), false); } + static class TestConfig { private final String version; diff --git a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java index 340d5c3e478c..a58d1fb77243 100644 --- a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java +++ b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java @@ -24,7 +24,9 @@ import com.google.common.base.Supplier; import com.google.inject.Inject; import org.apache.druid.audit.AuditEntry; +import org.apache.druid.audit.AuditInfo; import org.apache.druid.audit.AuditManager; +import org.apache.druid.common.config.ConfigSerde; import org.apache.druid.guice.ManageLifecycle; import org.apache.druid.guice.annotations.Json; import org.apache.druid.java.util.common.DateTimes; @@ -76,8 +78,15 @@ public String getAuditTable() } @Override - public void doAudit(final AuditEntry auditEntry) + public void doAudit(String key, String type, AuditInfo auditInfo, T payload, ConfigSerde configSerde) { + AuditEntry auditEntry = AuditEntry.builder() + .key(key) + .type(key) + .auditInfo(auditInfo) + .payload(configSerde.serializeToString(payload, config.isSkipNullField())) + .build(); + dbi.withHandle( new HandleCallback() { diff --git a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java index 20c3fac00346..34cffa88a688 100644 --- a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java +++ b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java @@ -35,6 +35,9 @@ public class SQLAuditManagerConfig @JsonProperty private HumanReadableBytes maxPayloadSizeBytes = HumanReadableBytes.valueOf(-1); + @JsonProperty + private boolean skipNullField = false; + public long getAuditHistoryMillis() { return auditHistoryMillis; @@ -49,4 +52,10 @@ public long getMaxPayloadSizeBytes() { return maxPayloadSizeBytes.getBytes(); } + + public boolean isSkipNullField() + { + return skipNullField; + } + } diff --git a/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java b/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java index 538fe6e40df6..fef4ed1e6b31 100644 --- a/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java +++ b/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java @@ -19,14 +19,18 @@ package org.apache.druid.server.audit; +import com.fasterxml.jackson.annotation.JsonInclude; +import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import org.apache.druid.audit.AuditEntry; import org.apache.druid.audit.AuditInfo; import org.apache.druid.audit.AuditManager; +import org.apache.druid.common.config.ConfigSerde; import org.apache.druid.jackson.DefaultObjectMapper; import org.apache.druid.java.util.common.DateTimes; import org.apache.druid.java.util.common.Intervals; import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.java.util.common.jackson.JacksonUtils; import org.apache.druid.java.util.emitter.service.ServiceMetricEvent; import org.apache.druid.metadata.TestDerbyConnector; import org.apache.druid.server.metrics.NoopServiceEmitter; @@ -49,6 +53,7 @@ public class SQLAuditManagerTest private TestDerbyConnector connector; private AuditManager auditManager; private final String PAYLOAD_DIMENSION_KEY = "payload"; + private ConfigSerde configSerde; private final ObjectMapper mapper = new DefaultObjectMapper(); @@ -64,6 +69,40 @@ public void setUp() mapper, new SQLAuditManagerConfig() ); + ObjectMapper jsonMapperSkipNull = mapper.copy().setSerializationInclusion(JsonInclude.Include.NON_NULL); + configSerde = new ConfigSerde() + { + @Override + public byte[] serialize(String obj) + { + try { + return mapper.writeValueAsBytes(obj); + } + catch (JsonProcessingException e) { + throw new RuntimeException(e); + } + } + + @Override + public String serializeToString(String obj, boolean skipNull) + { + try { + return skipNull ? jsonMapperSkipNull.writeValueAsString(obj) : mapper.writeValueAsString(obj); + } + catch (JsonProcessingException e) { + throw new RuntimeException(e); + } + } + + @Override + public String deserialize(byte[] bytes) + { + if (bytes == null) { + return ""; + } + return JacksonUtils.readValue(mapper, bytes, String.class); + } + }; } @Test(timeout = 60_000L) @@ -125,18 +164,17 @@ public boolean getIncludePayloadAsDimensionInMetric() @Test(timeout = 60_000L) public void testCreateAuditEntry() throws IOException { - AuditEntry entry = new AuditEntry( - "testKey", - "testType", - new AuditInfo( - "testAuthor", - "testComment", - "127.0.0.1" - ), - "testPayload", - DateTimes.of("2013-01-01T00:00:00Z") + String entry1Key = "testKey"; + String entry1Type = "testType"; + AuditInfo entry1AuditInfo = new AuditInfo( + "testAuthor", + "testComment", + "127.0.0.1" ); - auditManager.doAudit(entry); + String entry1Payload = "testPayload"; + + auditManager.doAudit(entry1Key, entry1Type, entry1AuditInfo, entry1Payload, configSerde); + byte[] payload = connector.lookup( derbyConnectorRule.metadataTablesConfigSupplier().get().getAuditTable(), "audit_key", @@ -144,118 +182,128 @@ public void testCreateAuditEntry() throws IOException "testKey" ); AuditEntry dbEntry = mapper.readValue(payload, AuditEntry.class); - Assert.assertEquals(entry, dbEntry); - + Assert.assertEquals(entry1Key, dbEntry.getKey()); + Assert.assertEquals(entry1Payload, dbEntry.getPayload()); + Assert.assertEquals(entry1Type, dbEntry.getType()); + Assert.assertEquals(entry1AuditInfo, dbEntry.getAuditInfo()); } @Test(timeout = 60_000L) public void testFetchAuditHistory() { - AuditEntry entry = new AuditEntry( - "testKey", - "testType", - new AuditInfo( - "testAuthor", - "testComment", - "127.0.0.1" - ), - "testPayload", - DateTimes.of("2013-01-01T00:00:00Z") + String entry1Key = "testKey1"; + String entry1Type = "testType"; + AuditInfo entry1AuditInfo = new AuditInfo( + "testAuthor", + "testComment", + "127.0.0.1" ); - auditManager.doAudit(entry); - auditManager.doAudit(entry); + String entry1Payload = "testPayload"; + + auditManager.doAudit(entry1Key, entry1Type, entry1AuditInfo, entry1Payload, configSerde); + auditManager.doAudit(entry1Key, entry1Type, entry1AuditInfo, entry1Payload, configSerde); + List auditEntries = auditManager.fetchAuditHistory( "testKey", "testType", - Intervals.of("2012-01-01T00:00:00Z/2013-01-03T00:00:00Z") + Intervals.ETERNITY ); Assert.assertEquals(2, auditEntries.size()); - Assert.assertEquals(entry, auditEntries.get(0)); - Assert.assertEquals(entry, auditEntries.get(1)); + + Assert.assertEquals(entry1Key, auditEntries.get(0).getKey()); + Assert.assertEquals(entry1Payload, auditEntries.get(0).getPayload()); + Assert.assertEquals(entry1Type, auditEntries.get(0).getType()); + Assert.assertEquals(entry1AuditInfo, auditEntries.get(0).getAuditInfo()); + + Assert.assertEquals(entry1Key, auditEntries.get(1).getKey()); + Assert.assertEquals(entry1Payload, auditEntries.get(1).getPayload()); + Assert.assertEquals(entry1Type, auditEntries.get(1).getType()); + Assert.assertEquals(entry1AuditInfo, auditEntries.get(1).getAuditInfo()); } @Test(timeout = 60_000L) public void testFetchAuditHistoryByKeyAndTypeWithLimit() { - AuditEntry entry1 = new AuditEntry( - "testKey1", - "testType", - new AuditInfo( - "testAuthor", - "testComment", - "127.0.0.1" - ), - "testPayload", - DateTimes.of("2013-01-01T00:00:00Z") + String entry1Key = "testKey1"; + String entry1Type = "testType"; + AuditInfo entry1AuditInfo = new AuditInfo( + "testAuthor", + "testComment", + "127.0.0.1" ); - AuditEntry entry2 = new AuditEntry( - "testKey2", - "testType", - new AuditInfo( - "testAuthor", - "testComment", - "127.0.0.1" - ), - "testPayload", - DateTimes.of("2013-01-02T00:00:00Z") + String entry1Payload = "testPayload"; + + String entry2Key = "testKey2"; + String entry2Type = "testType"; + AuditInfo entry2AuditInfo = new AuditInfo( + "testAuthor", + "testComment", + "127.0.0.1" ); - auditManager.doAudit(entry1); - auditManager.doAudit(entry2); + String entry2Payload = "testPayload"; + + auditManager.doAudit(entry1Key, entry1Type, entry1AuditInfo, entry1Payload, configSerde); + auditManager.doAudit(entry2Key, entry2Type, entry2AuditInfo, entry2Payload, configSerde); List auditEntries = auditManager.fetchAuditHistory( "testKey1", "testType", 1 ); Assert.assertEquals(1, auditEntries.size()); - Assert.assertEquals(entry1, auditEntries.get(0)); + Assert.assertEquals(entry1Key, auditEntries.get(0).getKey()); + Assert.assertEquals(entry1Payload, auditEntries.get(0).getPayload()); + Assert.assertEquals(entry1Type, auditEntries.get(0).getType()); + Assert.assertEquals(entry1AuditInfo, auditEntries.get(0).getAuditInfo()); } @Test(timeout = 60_000L) public void testFetchAuditHistoryByTypeWithLimit() { - AuditEntry entry1 = new AuditEntry( - "testKey", - "testType", - new AuditInfo( - "testAuthor", - "testComment", - "127.0.0.1" - ), - "testPayload", - DateTimes.of("2013-01-01T00:00:00Z") + String entry1Key = "testKey"; + String entry1Type = "testType"; + AuditInfo entry1AuditInfo = new AuditInfo( + "testAuthor", + "testComment", + "127.0.0.1" ); - AuditEntry entry2 = new AuditEntry( - "testKey", - "testType", - new AuditInfo( - "testAuthor", - "testComment", - "127.0.0.1" - ), - "testPayload", - DateTimes.of("2013-01-02T00:00:00Z") + String entry1Payload = "testPayload1"; + + String entry2Key = "testKey"; + String entry2Type = "testType"; + AuditInfo entry2AuditInfo = new AuditInfo( + "testAuthor", + "testComment", + "127.0.0.1" ); - AuditEntry entry3 = new AuditEntry( - "testKey", - "testType", - new AuditInfo( - "testAuthor", - "testComment", - "127.0.0.1" - ), - "testPayload", - DateTimes.of("2013-01-03T00:00:00Z") + String entry2Payload = "testPayload2"; + + String entry3Key = "testKey"; + String entry3Type = "testType"; + AuditInfo entry3AuditInfo = new AuditInfo( + "testAuthor", + "testComment", + "127.0.0.1" ); - auditManager.doAudit(entry1); - auditManager.doAudit(entry2); - auditManager.doAudit(entry3); + String entry3Payload = "testPayload3"; + + auditManager.doAudit(entry1Key, entry1Type, entry1AuditInfo, entry1Payload, configSerde); + auditManager.doAudit(entry2Key, entry2Type, entry2AuditInfo, entry2Payload, configSerde); + auditManager.doAudit(entry3Key, entry3Type, entry3AuditInfo, entry3Payload, configSerde); + List auditEntries = auditManager.fetchAuditHistory( "testType", 2 ); Assert.assertEquals(2, auditEntries.size()); - Assert.assertEquals(entry3, auditEntries.get(0)); - Assert.assertEquals(entry2, auditEntries.get(1)); + Assert.assertEquals(entry3Key, auditEntries.get(0).getKey()); + Assert.assertEquals(entry3Payload, auditEntries.get(0).getPayload()); + Assert.assertEquals(entry3Type, auditEntries.get(0).getType()); + Assert.assertEquals(entry3AuditInfo, auditEntries.get(0).getAuditInfo()); + + Assert.assertEquals(entry2Key, auditEntries.get(1).getKey()); + Assert.assertEquals(entry2Payload, auditEntries.get(1).getPayload()); + Assert.assertEquals(entry2Type, auditEntries.get(1).getType()); + Assert.assertEquals(entry2AuditInfo, auditEntries.get(1).getAuditInfo()); } @Test(expected = IllegalArgumentException.class, timeout = 10_000L) @@ -289,20 +337,17 @@ public long getMaxPayloadSizeBytes() } ); - AuditEntry.Builder auditEntryBuilder = AuditEntry.builder() - .key("testKey") - .type("testType") - .auditInfo(new AuditInfo( - "testAuthor", - "testComment", - "127.0.0.1" - )) - .auditTime(DateTimes.of("1994-04-29T00:00:00Z")); + String entry1Key = "testKey"; + String entry1Type = "testType"; + AuditInfo entry1AuditInfo = new AuditInfo( + "testAuthor", + "testComment", + "127.0.0.1" + ); + String entry1Payload = "payload audit to store"; - AuditEntry auditEntryToStore = auditEntryBuilder.payload("payload audit to store").build(); - AuditEntry expectedWithSkipPayload = auditEntryBuilder.payload(StringUtils.format(AuditManager.PAYLOAD_SKIP_MESSAGE, maxPayloadSize)).build(); + auditManagerWithMaxPayloadSizeBytes.doAudit(entry1Key, entry1Type, entry1AuditInfo, entry1Payload, configSerde); - auditManagerWithMaxPayloadSizeBytes.doAudit(auditEntryToStore); byte[] payload = connector.lookup( derbyConnectorRule.metadataTablesConfigSupplier().get().getAuditTable(), "audit_key", @@ -311,7 +356,11 @@ public long getMaxPayloadSizeBytes() ); AuditEntry dbEntry = mapper.readValue(payload, AuditEntry.class); - Assert.assertEquals(expectedWithSkipPayload, dbEntry); + Assert.assertEquals(entry1Key, dbEntry.getKey()); + Assert.assertNotEquals(entry1Payload, dbEntry.getPayload()); + Assert.assertEquals(StringUtils.format(AuditManager.PAYLOAD_SKIP_MESSAGE, maxPayloadSize), dbEntry.getPayload()); + Assert.assertEquals(entry1Type, dbEntry.getType()); + Assert.assertEquals(entry1AuditInfo, dbEntry.getAuditInfo()); } @Test(timeout = 60_000L) @@ -331,20 +380,17 @@ public long getMaxPayloadSizeBytes() } } ); + String entry1Key = "testKey"; + String entry1Type = "testType"; + AuditInfo entry1AuditInfo = new AuditInfo( + "testAuthor", + "testComment", + "127.0.0.1" + ); + String entry1Payload = "payload audit to store"; - AuditEntry.Builder auditEntryBuilder = AuditEntry.builder() - .key("testKey") - .type("testType") - .auditInfo(new AuditInfo( - "testAuthor", - "testComment", - "127.0.0.1" - )) - .auditTime(DateTimes.of("1994-04-29T00:00:00Z")); - - AuditEntry auditEntryToStore = auditEntryBuilder.payload("payload audit to store").build(); + auditManagerWithMaxPayloadSizeBytes.doAudit(entry1Key, entry1Type, entry1AuditInfo, entry1Payload, configSerde); - auditManagerWithMaxPayloadSizeBytes.doAudit(auditEntryToStore); byte[] payload = connector.lookup( derbyConnectorRule.metadataTablesConfigSupplier().get().getAuditTable(), "audit_key", @@ -353,7 +399,10 @@ public long getMaxPayloadSizeBytes() ); AuditEntry dbEntry = mapper.readValue(payload, AuditEntry.class); - Assert.assertEquals(auditEntryToStore, dbEntry); + Assert.assertEquals(entry1Key, dbEntry.getKey()); + Assert.assertEquals(entry1Payload, dbEntry.getPayload()); + Assert.assertEquals(entry1Type, dbEntry.getType()); + Assert.assertEquals(entry1AuditInfo, dbEntry.getAuditInfo()); } @After From f09beb0b94d55c5d634ba106a913475256a6ae54 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Thu, 8 Apr 2021 21:12:02 -0700 Subject: [PATCH 12/18] address comments --- .../config/JacksonConfigManagerTest.java | 31 ------- docs/configuration/index.md | 2 +- .../druid/server/audit/SQLAuditManager.java | 22 +++-- .../server/audit/SQLAuditManagerTest.java | 91 +++++++++++++------ 4 files changed, 78 insertions(+), 68 deletions(-) diff --git a/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java b/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java index a5f6f7dc85be..d8beb6820383 100644 --- a/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java +++ b/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java @@ -146,37 +146,6 @@ public int getSettingInt() { return settingInt; } - - @Override - public String toString() - { - return "TestConfig{" + - "version='" + version + '\'' + - ", settingString='" + settingString + '\'' + - ", settingInt=" + settingInt + - '}'; - } - - @Override - public boolean equals(Object o) - { - if (this == o) { - return true; - } - if (o == null || getClass() != o.getClass()) { - return false; - } - TestConfig that = (TestConfig) o; - return settingInt == that.settingInt && - Objects.equals(version, that.version) && - Objects.equals(settingString, that.settingString); - } - - @Override - public int hashCode() - { - return Objects.hash(version, settingString, settingInt); - } } static class ClassThatJacksonCannotSerialize diff --git a/docs/configuration/index.md b/docs/configuration/index.md index 00d7a281a411..c6e9b796924f 100644 --- a/docs/configuration/index.md +++ b/docs/configuration/index.md @@ -339,7 +339,7 @@ Coordinator and Overlord log changes to lookups, segment load/drop rules, dynami |`druid.audit.manager.auditHistoryMillis`|Default duration for querying audit history.|1 week| |`druid.audit.manager.includePayloadAsDimensionInMetric`|Boolean flag on whether to add `payload` column in service metric.|false| |`druid.audit.manager.maxPayloadSizeBytes`|The maximum size of audit payload, in bytes, to store in Druid's metadata store audit table. If the size of audit payload exceed this value, the audit log would be stored with a messaging indicating that the payload was omitted instead. Setting `maxPayloadSizeBytes` to -1 (default value) disables this check, meaning Druid will always store audit payload regardless of it's size. Human-readable format is supported, see [here](human-readable-byte.md). |-1| - +|`druid.audit.manager.skipNullField`|If true, the audit payload stored in metadata store will be exclude any field with null value. |false| ### Enabling Metrics diff --git a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java index a58d1fb77243..c3bdd03b7065 100644 --- a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java +++ b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java @@ -82,7 +82,7 @@ public void doAudit(String key, String type, AuditInfo auditInfo, T payload, { AuditEntry auditEntry = AuditEntry.builder() .key(key) - .type(key) + .type(type) .auditInfo(auditInfo) .payload(configSerde.serializeToString(payload, config.isSkipNullField())) .build(); @@ -124,15 +124,17 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException emitter.emit(getAuditMetricEventBuilder(auditEntry).build("config/audit", 1)); AuditEntry auditEntryToStore = auditEntry; - int payloadSize = jsonMapper.writeValueAsBytes(auditEntry.getPayload()).length; - if (config.getMaxPayloadSizeBytes() >= 0 && payloadSize > config.getMaxPayloadSizeBytes()) { - auditEntryToStore = AuditEntry.builder() - .key(auditEntry.getKey()) - .type(auditEntry.getType()) - .auditInfo(auditEntry.getAuditInfo()) - .payload(StringUtils.format(PAYLOAD_SKIP_MESSAGE, config.getMaxPayloadSizeBytes())) - .auditTime(auditEntry.getAuditTime()) - .build(); + if (config.getMaxPayloadSizeBytes() >= 0) { + int payloadSize = jsonMapper.writeValueAsBytes(auditEntry.getPayload()).length; + if (payloadSize > config.getMaxPayloadSizeBytes()) { + auditEntryToStore = AuditEntry.builder() + .key(auditEntry.getKey()) + .type(auditEntry.getType()) + .auditInfo(auditEntry.getAuditInfo()) + .payload(StringUtils.format(PAYLOAD_SKIP_MESSAGE, config.getMaxPayloadSizeBytes())) + .auditTime(auditEntry.getAuditTime()) + .build(); + } } handle.createStatement( diff --git a/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java b/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java index fef4ed1e6b31..646a0c338244 100644 --- a/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java +++ b/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java @@ -19,7 +19,6 @@ package org.apache.druid.server.audit; -import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import org.apache.druid.audit.AuditEntry; @@ -39,12 +38,19 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.ArgumentMatchers; +import org.mockito.Mockito; +import org.mockito.junit.MockitoJUnitRunner; import org.skife.jdbi.v2.Handle; import org.skife.jdbi.v2.tweak.HandleCallback; import java.io.IOException; +import java.util.HashMap; import java.util.List; +import java.util.Map; +@RunWith(MockitoJUnitRunner.class) public class SQLAuditManagerTest { @Rule @@ -53,12 +59,13 @@ public class SQLAuditManagerTest private TestDerbyConnector connector; private AuditManager auditManager; private final String PAYLOAD_DIMENSION_KEY = "payload"; - private ConfigSerde configSerde; + private ConfigSerde stringConfigSerde; + private final ObjectMapper mapper = new DefaultObjectMapper(); @Before - public void setUp() + public void setUp() throws Exception { connector = derbyConnectorRule.getConnector(); connector.createAuditTable(); @@ -69,8 +76,7 @@ public void setUp() mapper, new SQLAuditManagerConfig() ); - ObjectMapper jsonMapperSkipNull = mapper.copy().setSerializationInclusion(JsonInclude.Include.NON_NULL); - configSerde = new ConfigSerde() + stringConfigSerde = new ConfigSerde() { @Override public byte[] serialize(String obj) @@ -86,20 +92,14 @@ public byte[] serialize(String obj) @Override public String serializeToString(String obj, boolean skipNull) { - try { - return skipNull ? jsonMapperSkipNull.writeValueAsString(obj) : mapper.writeValueAsString(obj); - } - catch (JsonProcessingException e) { - throw new RuntimeException(e); - } + // In our test, payload Object is already a String + // So to serialize to String, we just return a String + return obj; } @Override public String deserialize(byte[] bytes) { - if (bytes == null) { - return ""; - } return JacksonUtils.readValue(mapper, bytes, String.class); } }; @@ -173,7 +173,7 @@ public void testCreateAuditEntry() throws IOException ); String entry1Payload = "testPayload"; - auditManager.doAudit(entry1Key, entry1Type, entry1AuditInfo, entry1Payload, configSerde); + auditManager.doAudit(entry1Key, entry1Type, entry1AuditInfo, entry1Payload, stringConfigSerde); byte[] payload = connector.lookup( derbyConnectorRule.metadataTablesConfigSupplier().get().getAuditTable(), @@ -191,7 +191,7 @@ public void testCreateAuditEntry() throws IOException @Test(timeout = 60_000L) public void testFetchAuditHistory() { - String entry1Key = "testKey1"; + String entry1Key = "testKey"; String entry1Type = "testType"; AuditInfo entry1AuditInfo = new AuditInfo( "testAuthor", @@ -200,13 +200,13 @@ public void testFetchAuditHistory() ); String entry1Payload = "testPayload"; - auditManager.doAudit(entry1Key, entry1Type, entry1AuditInfo, entry1Payload, configSerde); - auditManager.doAudit(entry1Key, entry1Type, entry1AuditInfo, entry1Payload, configSerde); + auditManager.doAudit(entry1Key, entry1Type, entry1AuditInfo, entry1Payload, stringConfigSerde); + auditManager.doAudit(entry1Key, entry1Type, entry1AuditInfo, entry1Payload, stringConfigSerde); List auditEntries = auditManager.fetchAuditHistory( "testKey", "testType", - Intervals.ETERNITY + Intervals.of("2000-01-01T00:00:00Z/2100-01-03T00:00:00Z") ); Assert.assertEquals(2, auditEntries.size()); @@ -242,8 +242,8 @@ public void testFetchAuditHistoryByKeyAndTypeWithLimit() ); String entry2Payload = "testPayload"; - auditManager.doAudit(entry1Key, entry1Type, entry1AuditInfo, entry1Payload, configSerde); - auditManager.doAudit(entry2Key, entry2Type, entry2AuditInfo, entry2Payload, configSerde); + auditManager.doAudit(entry1Key, entry1Type, entry1AuditInfo, entry1Payload, stringConfigSerde); + auditManager.doAudit(entry2Key, entry2Type, entry2AuditInfo, entry2Payload, stringConfigSerde); List auditEntries = auditManager.fetchAuditHistory( "testKey1", "testType", @@ -286,9 +286,9 @@ public void testFetchAuditHistoryByTypeWithLimit() ); String entry3Payload = "testPayload3"; - auditManager.doAudit(entry1Key, entry1Type, entry1AuditInfo, entry1Payload, configSerde); - auditManager.doAudit(entry2Key, entry2Type, entry2AuditInfo, entry2Payload, configSerde); - auditManager.doAudit(entry3Key, entry3Type, entry3AuditInfo, entry3Payload, configSerde); + auditManager.doAudit(entry1Key, entry1Type, entry1AuditInfo, entry1Payload, stringConfigSerde); + auditManager.doAudit(entry2Key, entry2Type, entry2AuditInfo, entry2Payload, stringConfigSerde); + auditManager.doAudit(entry3Key, entry3Type, entry3AuditInfo, entry3Payload, stringConfigSerde); List auditEntries = auditManager.fetchAuditHistory( "testType", @@ -346,7 +346,9 @@ public long getMaxPayloadSizeBytes() ); String entry1Payload = "payload audit to store"; - auditManagerWithMaxPayloadSizeBytes.doAudit(entry1Key, entry1Type, entry1AuditInfo, entry1Payload, configSerde); + auditManagerWithMaxPayloadSizeBytes.doAudit(entry1Key, entry1Type, entry1AuditInfo, entry1Payload, + stringConfigSerde + ); byte[] payload = connector.lookup( derbyConnectorRule.metadataTablesConfigSupplier().get().getAuditTable(), @@ -389,7 +391,9 @@ public long getMaxPayloadSizeBytes() ); String entry1Payload = "payload audit to store"; - auditManagerWithMaxPayloadSizeBytes.doAudit(entry1Key, entry1Type, entry1AuditInfo, entry1Payload, configSerde); + auditManagerWithMaxPayloadSizeBytes.doAudit(entry1Key, entry1Type, entry1AuditInfo, entry1Payload, + stringConfigSerde + ); byte[] payload = connector.lookup( derbyConnectorRule.metadataTablesConfigSupplier().get().getAuditTable(), @@ -405,6 +409,41 @@ public long getMaxPayloadSizeBytes() Assert.assertEquals(entry1AuditInfo, dbEntry.getAuditInfo()); } + @Test(timeout = 60_000L) + public void testCreateAuditEntryWithSkipNullConfigTrue() throws IOException + { + ConfigSerde> mockConfigSerde = Mockito.mock(ConfigSerde.class); + SQLAuditManager auditManagerWithSkipNull = new SQLAuditManager( + connector, + derbyConnectorRule.metadataTablesConfigSupplier(), + new NoopServiceEmitter(), + mapper, + new SQLAuditManagerConfig() + { + @Override + public boolean isSkipNullField() + { + return true; + } + } + ); + + String entry1Key = "test1Key"; + String entry1Type = "test1Type"; + AuditInfo entry1AuditInfo = new AuditInfo( + "testAuthor", + "testComment", + "127.0.0.1" + ); + // Entry 1 payload has a null field for one of the property + Map entryPayload1WithNull = new HashMap<>(); + entryPayload1WithNull.put("version", "x"); + entryPayload1WithNull.put("something", null); + + auditManagerWithSkipNull.doAudit(entry1Key, entry1Type, entry1AuditInfo, entryPayload1WithNull, mockConfigSerde); + Mockito.verify(mockConfigSerde).serializeToString(ArgumentMatchers.eq(entryPayload1WithNull), ArgumentMatchers.eq(true)); + } + @After public void cleanup() { From a4ec89885e7f9e754f070a413a4561dbffd04671 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Thu, 8 Apr 2021 21:12:52 -0700 Subject: [PATCH 13/18] fix checkstyle --- .../apache/druid/common/config/JacksonConfigManager.java | 1 - .../druid/common/config/JacksonConfigManagerTest.java | 6 ------ 2 files changed, 7 deletions(-) diff --git a/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java b/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java index 1c588ff4d0b1..c62a8b72cabf 100644 --- a/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java +++ b/core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java @@ -24,7 +24,6 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.annotations.VisibleForTesting; import com.google.inject.Inject; -import org.apache.druid.audit.AuditEntry; import org.apache.druid.audit.AuditInfo; import org.apache.druid.audit.AuditManager; import org.apache.druid.common.config.ConfigManager.SetResult; diff --git a/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java b/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java index d8beb6820383..841ab284c532 100644 --- a/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java +++ b/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java @@ -24,8 +24,6 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.ObjectMapper; -import org.apache.druid.audit.AuditEntry; -import org.apache.druid.audit.AuditInfo; import org.apache.druid.audit.AuditManager; import org.junit.Assert; import org.junit.Before; @@ -33,13 +31,9 @@ import org.junit.Test; import org.junit.rules.ExpectedException; import org.junit.runner.RunWith; -import org.mockito.ArgumentCaptor; import org.mockito.Mock; -import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; -import java.util.Objects; - @RunWith(MockitoJUnitRunner.class) public class JacksonConfigManagerTest { From 48ebcb3635f23d3cd4f72bf29caadd73659a85ec Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Thu, 8 Apr 2021 21:15:50 -0700 Subject: [PATCH 14/18] address comments --- core/src/main/java/org/apache/druid/audit/AuditManager.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/druid/audit/AuditManager.java b/core/src/main/java/org/apache/druid/audit/AuditManager.java index 204978771f15..2327973ad87d 100644 --- a/core/src/main/java/org/apache/druid/audit/AuditManager.java +++ b/core/src/main/java/org/apache/druid/audit/AuditManager.java @@ -41,7 +41,11 @@ public interface AuditManager /** * inserts an audit entry in the Audit Table - * @param key + * @param key of the audit entry + * @param type of the audit entry + * @param auditInfo of the audit entry + * @param payload of the audit entry + * @param configSerde of the payload of the audit entry */ void doAudit(String key, String type, AuditInfo auditInfo, T payload, ConfigSerde configSerde); From 604ab8cd9cc61b89df961225f6b4466e2a54f2ab Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Fri, 9 Apr 2021 00:22:05 -0700 Subject: [PATCH 15/18] fix test --- .../config/JacksonConfigManagerTest.java | 34 +++++++++++++++++-- .../server/audit/SQLAuditManagerTest.java | 4 +-- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java b/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java index 841ab284c532..0b8442322386 100644 --- a/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java +++ b/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java @@ -24,6 +24,8 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.druid.audit.AuditEntry; +import org.apache.druid.audit.AuditInfo; import org.apache.druid.audit.AuditManager; import org.junit.Assert; import org.junit.Before; @@ -31,7 +33,10 @@ import org.junit.Test; import org.junit.rules.ExpectedException; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.ArgumentMatchers; import org.mockito.Mock; +import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; @RunWith(MockitoJUnitRunner.class) @@ -88,7 +93,7 @@ public void testSerializeToStringWithSkipNullFalse() } @Test - public void testSetWithInvalidConfigForConfigSerdeFromTypeReference() + public void testSerializeToStringWithInvalidConfigForConfigSerdeFromTypeReference() { ConfigSerde configConfigSerdeFromTypeReference = jacksonConfigManager.create(new TypeReference() { @@ -99,7 +104,7 @@ public void testSetWithInvalidConfigForConfigSerdeFromTypeReference() } @Test - public void testSetWithInvalidConfigForConfigSerdeFromClass() + public void testSerializeToStringWithInvalidConfigForConfigSerdeFromClass() { ConfigSerde configConfigSerdeFromClass = jacksonConfigManager.create(ClassThatJacksonCannotSerialize.class, null); exception.expect(RuntimeException.class); @@ -107,6 +112,31 @@ public void testSetWithInvalidConfigForConfigSerdeFromClass() configConfigSerdeFromClass.serializeToString(new ClassThatJacksonCannotSerialize(), false); } + @Test + public void testSet() + { + String key = "key"; + TestConfig val = new TestConfig("version", "string", 3); + AuditInfo auditInfo = new AuditInfo( + "testAuthor", + "testComment", + "127.0.0.1" + ); + + jacksonConfigManager.set(key, val, auditInfo); + + ArgumentCaptor configSerdeCapture = ArgumentCaptor.forClass( + ConfigSerde.class); + Mockito.verify(mockAuditManager).doAudit( + ArgumentMatchers.eq(key), + ArgumentMatchers.eq(key), + ArgumentMatchers.eq(auditInfo), + ArgumentMatchers.eq(val), + configSerdeCapture.capture() + ); + Assert.assertNotNull(configSerdeCapture.getValue()); + } + static class TestConfig { diff --git a/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java b/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java index 646a0c338244..2ce652464315 100644 --- a/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java +++ b/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java @@ -65,7 +65,7 @@ public class SQLAuditManagerTest private final ObjectMapper mapper = new DefaultObjectMapper(); @Before - public void setUp() throws Exception + public void setUp() { connector = derbyConnectorRule.getConnector(); connector.createAuditTable(); @@ -410,7 +410,7 @@ public long getMaxPayloadSizeBytes() } @Test(timeout = 60_000L) - public void testCreateAuditEntryWithSkipNullConfigTrue() throws IOException + public void testCreateAuditEntryWithSkipNullConfigTrue() { ConfigSerde> mockConfigSerde = Mockito.mock(ConfigSerde.class); SQLAuditManager auditManagerWithSkipNull = new SQLAuditManager( From d6b53f8500b9cf532591b3e5e231cf9238753216 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Fri, 9 Apr 2021 00:23:26 -0700 Subject: [PATCH 16/18] fix test --- .../org/apache/druid/common/config/JacksonConfigManagerTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java b/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java index 0b8442322386..25220ebb8921 100644 --- a/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java +++ b/core/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java @@ -24,7 +24,6 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.ObjectMapper; -import org.apache.druid.audit.AuditEntry; import org.apache.druid.audit.AuditInfo; import org.apache.druid.audit.AuditManager; import org.junit.Assert; From acaa6a429a4e8576e03208bde14740eb3b246c95 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Sat, 10 Apr 2021 15:31:03 -0700 Subject: [PATCH 17/18] address comments --- core/src/main/java/org/apache/druid/audit/AuditManager.java | 2 +- docs/configuration/index.md | 4 ++-- .../java/org/apache/druid/server/audit/SQLAuditManager.java | 2 +- .../org/apache/druid/server/audit/SQLAuditManagerConfig.java | 5 +++++ .../org/apache/druid/server/audit/SQLAuditManagerTest.java | 2 +- 5 files changed, 10 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/apache/druid/audit/AuditManager.java b/core/src/main/java/org/apache/druid/audit/AuditManager.java index 2327973ad87d..73804d7da351 100644 --- a/core/src/main/java/org/apache/druid/audit/AuditManager.java +++ b/core/src/main/java/org/apache/druid/audit/AuditManager.java @@ -33,7 +33,7 @@ public interface AuditManager * This String is the default message stored instead of the actual audit payload if the audit payload size * exceeded the maximum size limit configuration */ - String PAYLOAD_SKIP_MESSAGE = "Payload was not stored as its size exceeds the limit [%d] configured by druid.audit.manager.maxPayloadSizeBytes"; + String PAYLOAD_SKIP_MSG_FORMAT = "Payload was not stored as its size exceeds the limit [%d] configured by druid.audit.manager.maxPayloadSizeBytes"; String X_DRUID_AUTHOR = "X-Druid-Author"; diff --git a/docs/configuration/index.md b/docs/configuration/index.md index c6e9b796924f..ac133c0440c7 100644 --- a/docs/configuration/index.md +++ b/docs/configuration/index.md @@ -338,8 +338,8 @@ Coordinator and Overlord log changes to lookups, segment load/drop rules, dynami |--------|-----------|-------| |`druid.audit.manager.auditHistoryMillis`|Default duration for querying audit history.|1 week| |`druid.audit.manager.includePayloadAsDimensionInMetric`|Boolean flag on whether to add `payload` column in service metric.|false| -|`druid.audit.manager.maxPayloadSizeBytes`|The maximum size of audit payload, in bytes, to store in Druid's metadata store audit table. If the size of audit payload exceed this value, the audit log would be stored with a messaging indicating that the payload was omitted instead. Setting `maxPayloadSizeBytes` to -1 (default value) disables this check, meaning Druid will always store audit payload regardless of it's size. Human-readable format is supported, see [here](human-readable-byte.md). |-1| -|`druid.audit.manager.skipNullField`|If true, the audit payload stored in metadata store will be exclude any field with null value. |false| +|`druid.audit.manager.maxPayloadSizeBytes`|The maximum size of audit payload, in bytes, to store in Druid's metadata store audit table. If the size of audit payload exceed this value, the audit log would be stored with a messaging indicating that the payload was omitted instead. Setting `maxPayloadSizeBytes` to -1 (default value) disables this check, meaning Druid will always store audit payload regardless of it's size. Setting to any negative number other than `-1` is invalid. Human-readable format is supported, see [here](human-readable-byte.md). |-1| +|`druid.audit.manager.skipNullField`|If true, the audit payload stored in metadata store will exclude any field with null value. |false| ### Enabling Metrics diff --git a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java index c3bdd03b7065..130204361743 100644 --- a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java +++ b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java @@ -131,7 +131,7 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException .key(auditEntry.getKey()) .type(auditEntry.getType()) .auditInfo(auditEntry.getAuditInfo()) - .payload(StringUtils.format(PAYLOAD_SKIP_MESSAGE, config.getMaxPayloadSizeBytes())) + .payload(StringUtils.format(PAYLOAD_SKIP_MSG_FORMAT, config.getMaxPayloadSizeBytes())) .auditTime(auditEntry.getAuditTime()) .build(); } diff --git a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java index 34cffa88a688..8509e06c1029 100644 --- a/server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java +++ b/server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java @@ -21,6 +21,7 @@ import com.fasterxml.jackson.annotation.JsonProperty; import org.apache.druid.java.util.common.HumanReadableBytes; +import org.apache.druid.java.util.common.HumanReadableBytesRange; /** */ @@ -33,6 +34,10 @@ public class SQLAuditManagerConfig private boolean includePayloadAsDimensionInMetric = false; @JsonProperty + @HumanReadableBytesRange( + min = -1, + message = "maxPayloadSizeBytes must either be -1 (for disabling the check) or a non negative number" + ) private HumanReadableBytes maxPayloadSizeBytes = HumanReadableBytes.valueOf(-1); @JsonProperty diff --git a/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java b/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java index 2ce652464315..429402e9e9e5 100644 --- a/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java +++ b/server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java @@ -360,7 +360,7 @@ public long getMaxPayloadSizeBytes() AuditEntry dbEntry = mapper.readValue(payload, AuditEntry.class); Assert.assertEquals(entry1Key, dbEntry.getKey()); Assert.assertNotEquals(entry1Payload, dbEntry.getPayload()); - Assert.assertEquals(StringUtils.format(AuditManager.PAYLOAD_SKIP_MESSAGE, maxPayloadSize), dbEntry.getPayload()); + Assert.assertEquals(StringUtils.format(AuditManager.PAYLOAD_SKIP_MSG_FORMAT, maxPayloadSize), dbEntry.getPayload()); Assert.assertEquals(entry1Type, dbEntry.getType()); Assert.assertEquals(entry1AuditInfo, dbEntry.getAuditInfo()); } From 34c6005a1d00c90ecb63ffbe174582aa6f276059 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Tue, 13 Apr 2021 13:55:24 -0700 Subject: [PATCH 18/18] Address comments Co-authored-by: Jihoon Son --- docs/configuration/index.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/configuration/index.md b/docs/configuration/index.md index ac133c0440c7..368e1acd502b 100644 --- a/docs/configuration/index.md +++ b/docs/configuration/index.md @@ -338,7 +338,7 @@ Coordinator and Overlord log changes to lookups, segment load/drop rules, dynami |--------|-----------|-------| |`druid.audit.manager.auditHistoryMillis`|Default duration for querying audit history.|1 week| |`druid.audit.manager.includePayloadAsDimensionInMetric`|Boolean flag on whether to add `payload` column in service metric.|false| -|`druid.audit.manager.maxPayloadSizeBytes`|The maximum size of audit payload, in bytes, to store in Druid's metadata store audit table. If the size of audit payload exceed this value, the audit log would be stored with a messaging indicating that the payload was omitted instead. Setting `maxPayloadSizeBytes` to -1 (default value) disables this check, meaning Druid will always store audit payload regardless of it's size. Setting to any negative number other than `-1` is invalid. Human-readable format is supported, see [here](human-readable-byte.md). |-1| +|`druid.audit.manager.maxPayloadSizeBytes`|The maximum size of audit payload to store in Druid's metadata store audit table. If the size of audit payload exceeds this value, the audit log would be stored with a message indicating that the payload was omitted instead. Setting `maxPayloadSizeBytes` to -1 (default value) disables this check, meaning Druid will always store audit payload regardless of it's size. Setting to any negative number other than `-1` is invalid. Human-readable format is supported, see [here](human-readable-byte.md). |-1| |`druid.audit.manager.skipNullField`|If true, the audit payload stored in metadata store will exclude any field with null value. |false| ### Enabling Metrics