abhishekagarwal87 commented on code in PR #15480:
URL: https://github.com/apache/druid/pull/15480#discussion_r1428758903
##########
server/src/main/java/org/apache/druid/rpc/indexing/OverlordClientImpl.java:
##########
@@ -96,7 +97,8 @@ public ListenableFuture<Void> runTask(final String taskId,
final Object taskObje
return FutureUtils.transform(
client.asyncRequest(
new RequestBuilder(HttpMethod.POST, "/druid/indexer/v1/task")
- .jsonContent(jsonMapper, taskObject),
+ .jsonContent(jsonMapper, taskObject)
+ .header(AuditManager.X_DRUID_AUTHOR,
AuditManager.AUTHOR_DRUID_SYSTEM),
Review Comment:
is this needed? can we not rely on authenticated identity instead?
##########
server/src/main/java/org/apache/druid/guice/SQLMetadataStorageDruidModule.java:
##########
@@ -130,21 +131,33 @@ public void configure(Binder binder)
.to(SQLMetadataStorageUpdaterJobHandler.class)
.in(LazySingleton.class);
- JsonConfigProvider.bind(binder, "druid.audit.manager",
SQLAuditManagerConfig.class);
-
- PolyBind.optionBinder(binder, Key.get(AuditManager.class))
- .addBinding(type)
- .to(SQLAuditManager.class)
- .in(LazySingleton.class);
-
- PolyBind.optionBinder(binder, Key.get(AuditManagerProvider.class))
- .addBinding(type)
- .to(SQLAuditManagerProvider.class)
- .in(LazySingleton.class);
-
PolyBind.optionBinder(binder, Key.get(MetadataSupervisorManager.class))
.addBinding(type)
.to(SQLMetadataSupervisorManager.class)
.in(LazySingleton.class);
}
+
+ private void configureAuditManager(Binder binder)
+ {
+ JsonConfigProvider.bind(binder, "druid.audit.manager",
AuditManagerConfig.class);
+
+ PolyBind.createChoice(
+ binder,
+ "druid.audit.manager.type",
+ Key.get(AuditManager.class),
+ Key.get(SQLAuditManager.class)
+ );
+ final MapBinder<String, AuditManager> auditManagerBinder
+ = PolyBind.optionBinder(binder, Key.get(AuditManager.class));
+ auditManagerBinder
+ .addBinding("log")
+ .to(LoggingAuditManager.class)
+ .in(LazySingleton.class);
Review Comment:
seems off that the log choice is added in this module. should this be moved
to some other module?
##########
server/src/main/java/org/apache/druid/server/coordinator/duty/KillCompactionConfig.java:
##########
@@ -133,6 +133,7 @@ private int tryDeleteCompactionConfigs() throws
RetryableException
return updated;
},
new AuditInfo(
+ "KillCompactionConfig",
"KillCompactionConfig",
Review Comment:
identity and author are KillCompactionConfig?
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordResource.java:
##########
@@ -225,18 +222,29 @@ public Response taskPost(final Task task, @Context final
HttpServletRequest req)
taskQueue -> {
try {
taskQueue.add(task);
+
+ auditManager.doAudit(
Review Comment:
can we filter task submissions that were initiated by the system itself?
E.g. compaction duty, MSQ controller task, etc?
##########
processing/src/main/java/org/apache/druid/audit/AuditManager.java:
##########
@@ -29,74 +28,76 @@
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_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";
-
String X_DRUID_COMMENT = "X-Druid-Comment";
/**
- * inserts an audit entry in the Audit Table
- * @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
+ * Value of header {@link #X_DRUID_AUTHOR} used by Druid services so that
they
+ * can be distinguished from external requests.
+ */
+ String AUTHOR_DRUID_SYSTEM = "druid_system";
+
+ /**
+ * @return true if the audited event was initiated by the Druid system
itself.
*/
- <T> void doAudit(String key, String type, AuditInfo auditInfo, T payload,
ConfigSerde<T> configSerde);
+ default boolean isSystemRequest(AuditInfo auditInfo)
+ {
+ return AUTHOR_DRUID_SYSTEM.equals(auditInfo.getAuthor());
Review Comment:
would this be valid for any auth extension?
##########
server/src/main/java/org/apache/druid/server/audit/AuditManagerConfig.java:
##########
@@ -19,11 +19,20 @@
package org.apache.druid.server.audit;
-import com.google.inject.Provider;
-import org.apache.druid.audit.AuditManager;
+import com.fasterxml.jackson.annotation.JsonSubTypes;
+import com.fasterxml.jackson.annotation.JsonSubTypes.Type;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
-public interface AuditManagerProvider extends Provider<AuditManager>
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type", defaultImpl =
SQLAuditManagerConfig.class)
Review Comment:
default should be no-op?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
