-
Notifications
You must be signed in to change notification settings - Fork 4.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DSIP-26][Audit log] Audit log improvement design #15554
Conversation
qingwli
commented
Feb 2, 2024
- close [DSIP-26][Audit log] Audit log improvement design #15423
# Conflicts: # dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/ClusterController.java # dolphinscheduler-dao/src/test/java/org/apache/dolphinscheduler/dao/mapper/AuditLogMapperTest.java
update audit log ui
update object name
# Conflicts: # dolphinscheduler-dao/src/main/resources/sql/upgrade/3.3.0_schema/mysql/dolphinscheduler_ddl.sql # dolphinscheduler-dao/src/main/resources/sql/upgrade/3.3.0_schema/postgresql/dolphinscheduler_ddl.sql
`object_id` bigint(20) DEFAULT NULL COMMENT 'object id', | ||
`object_name` varchar(100) DEFAULT NULL COMMENT 'object id', | ||
`object_type` varchar(100) NOT NULL COMMENT 'object type', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`object_id` bigint(20) DEFAULT NULL COMMENT 'object id', | |
`object_name` varchar(100) DEFAULT NULL COMMENT 'object id', | |
`object_type` varchar(100) NOT NULL COMMENT 'object type', | |
`object_id` bigint(20) DEFAULT NULL COMMENT 'object id', | |
`object_name` varchar(100) DEFAULT NULL COMMENT 'object id', | |
`object_type` varchar(100) NOT NULL COMMENT 'object type', |
Use domain_id
or module_id
is better than object_id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this part, I defined object id means the object I modified, task instance
it's not a module. File
is not a domain, I think object
is more suit for this, can cover all the stuff we record, WDYT
|
||
PROJECT("Project", null), // 1 | ||
PROCESS("Process", PROJECT), | ||
PROCESS_INSTANCE("Process instance", PROCESS), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PROCESS_INSTANCE("Process instance", PROCESS), | |
PROCESS_INSTANCE("ProcessInstance", PROCESS), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// Api don't need record log | ||
if (operatorLog == null) { | ||
return point.proceed(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logPointCut
should make sure the method exist @OperatorLog
.
return point.proceed(); | ||
} | ||
|
||
Operation operation = method.getAnnotation(Operation.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find the Operation
class, is this exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Operation(summary = "queryAuditObjectTypeList", description = "QUERY_AUDIT_OBJECT_TYPE_LIST")
@GetMapping(value = "/audit-log-object-type")
@ResponseStatus(HttpStatus.OK)
@ApiException(QUERY_AUDIT_LOG_LIST_PAGING)
public Result<List<AuditObjectTypeDto>> queryAuditObjectTypeList() {
return Result.success(AuditObjectTypeDto.getObjectTypeDtoList());
}
void execute(AuditMessage message); | ||
import org.aspectj.lang.ProceedingJoinPoint; | ||
|
||
public interface Operator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public interface Operator { | |
public interface AuditOperator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
long beginTime = System.currentTimeMillis(); | ||
|
||
MethodSignature signature = (MethodSignature) point.getSignature(); | ||
Map<String, Object> paramsMap = OperatorUtils.getParamsMap(point, signature); | ||
|
||
User user = OperatorUtils.getUser(paramsMap); | ||
|
||
if (user == null) { | ||
log.error("user is null"); | ||
return point.proceed(); | ||
} | ||
|
||
List<AuditLog> auditLogList = OperatorUtils.buildAuditLogList(describe, auditType, user); | ||
setRequestParam(auditType, auditLogList, paramsMap); | ||
|
||
Result result = (Result) point.proceed(); | ||
if (OperatorUtils.resultFail(result)) { | ||
log.error("request fail, code {}", result.getCode()); | ||
return result; | ||
} | ||
|
||
setObjectIdentityFromReturnObject(auditType, result, auditLogList); | ||
|
||
modifyAuditOperationType(auditType, paramsMap, auditLogList); | ||
modifyAuditObjectType(auditType, paramsMap, auditLogList); | ||
|
||
long latency = System.currentTimeMillis() - beginTime; | ||
auditService.addAudit(auditLogList, latency); | ||
|
||
return result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may need to audit the case when proceed
throw exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo #15788
return; | ||
} | ||
|
||
Long objId = checkNum(returnObjectMap.get(params[0]).toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long objId = checkNum(returnObjectMap.get(params[0]).toString()); | |
Long objId = NumberUtils.toLong(returnObjectMap.get(params[0]).toString(), -1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -293,7 +293,7 @@ private void checkMasterExists() { | |||
|
|||
// no master | |||
if (masterServers.isEmpty()) { | |||
throw new ServiceException(Status.MASTER_NOT_EXISTS); | |||
// throw new ServiceException(Status.MASTER_NOT_EXISTS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rollback this kind of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
auditDto.setUserName(auditLog.getUserName()); | ||
auditDto.setResourceName(auditLogMapper.queryResourceNameByType(resourceType, auditLog.getResourceId())); | ||
auditDto.setLatency(String.format("%.2f", (double) auditLog.getLatency() / 1000)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auditDto.setLatency(String.format("%.2f", (double) auditLog.getLatency() / 1000)); | |
auditDto.setLatency(String.format("%.2f", (double) auditLog.getLatency() / 1000)); |
It's better to use ms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Please take care of the new issues. |
...uler-api/src/main/java/org/apache/dolphinscheduler/api/audit/operator/BaseAuditOperator.java
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please move the ddl to 3.2.2
PTAL @ruanwenjun |
dolphinscheduler-dao/src/main/resources/sql/dolphinscheduler_postgresql.sql
Show resolved
Hide resolved
Quality Gate passedIssues Measures |