Conversation
cadem
requested review from
a team,
dapan1121,
guanshengliang,
hzcheng and
zitsen
as code owners
Summary of ChangesHello @cadem, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the system's auditing capabilities by integrating a 'self-audit' feature. This allows audit logs to be stored directly within the database, providing a more cohesive and robust internal logging mechanism. The changes encompass new configuration options, updated data structures for message passing, and a complete pipeline for processing and persisting audit records into a newly defined internal super table. This ensures that critical system activities are reliably recorded and accessible within the database environment itself. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a self-audit feature, allowing audit logs to be stored within the TDengine cluster itself. The implementation is comprehensive, covering configuration, messaging, metadata management, and data processing. A new test suite is also included to validate this new functionality. The overall code quality is good, with robust error handling. I have identified a couple of areas for improvement related to const-correctness and maintainability, which are detailed in the specific comments.
| if (code < 0) return TSDB_CODE_INVALID_JSON_FORMAT; | ||
| code = tjsonGetStringValue2(pJson, "resource", record->target2, TSDB_STREAM_NAME_LEN); | ||
| if (code < 0) return TSDB_CODE_INVALID_JSON_FORMAT; | ||
| record->detail = (char *)tjsonGetStringPointer(pJson, "details"); |
There was a problem hiding this comment.
Casting away const from the pointer returned by tjsonGetStringPointer is unsafe and violates const-correctness. The tjsonGetStringPointer function returns a const char* which points to an internal buffer that should not be modified. Assigning it to a char* (the type of record->detail) can lead to accidental modifications and undefined behavior.
Please consider one of the following fixes:
- Change the type of the
detailfield in the localSAuditRecordstruct toconst char*. - If the string needs to be modified (which doesn't seem to be the case here), use
tjsonDupStringValueto get a mutable copy of the string, and remember to free it later.
| if (pCol->colId == 1) { | ||
| data = &record->curTime; | ||
| } else if (pCol->colId == 2) { | ||
| data = record->detail; | ||
| } else if (pCol->colId == 3) { | ||
| data = record->user; | ||
| } else if (pCol->colId == 4) { | ||
| data = record->operation; | ||
| } else if (pCol->colId == 5) { | ||
| data = record->target1; | ||
| } else if (pCol->colId == 6) { | ||
| data = record->target2; | ||
| } else if (pCol->colId == 7) { | ||
| data = record->clientAddress; | ||
| } else if (pCol->colId == 8) { | ||
| data = &record->duration; | ||
| } else if (pCol->colId == 9) { | ||
| data = &record->affectedRows; | ||
| } else { | ||
| vError("the column id %" PRIi16 " is not defined in audit record table", pCol->colId); | ||
| code = TSDB_CODE_APP_ERROR; | ||
| TAOS_CHECK_GOTO(code, &lino, _exit); | ||
| } |
There was a problem hiding this comment.
The logic for mapping audit record fields to table columns relies on hardcoded column IDs (1-9). This creates a fragile dependency on the audit table schema defined in source/dnode/mnode/impl/src/mndStb.c. If the column order or IDs in the schema definition change in the future, this code will break in a non-obvious way, potentially leading to incorrect data being written.
For better maintainability, please consider adding a comment here to warn developers about this tight coupling and the need to keep this mapping synchronized with the schema in mndStb.c.
There was a problem hiding this comment.
Pull request overview
This PR adds support for saving audit records “in self” (inside the audit database itself) by introducing new status fields to distribute the audit vnode endpoint set/vgId, auto-creating the audit operations super table on audit DB creation, and adding a new vnode write message path for ingesting audit records. It also updates/extends CI test coverage for the new self-audit behavior.
Changes:
- Add
auditSaveInSelfserver config + propagate audit vnodeSEpSet/vgIdvia status req/rsp. - Auto-create the audit
operationssuper table when creating an audit database (whenauditSaveInSelfis enabled), and implement vnode-side handling to persist audit records. - Add a new CI test case for taosd self-audit and adjust existing audit test and docs/error codes.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/ci/cases.task | Adds new self-audit pytest to CI task list. |
| test/cases/80-Components/01-Taosd/test_com_taosd_self_audit.py | New test validating audit records are stored in the audit DB itself. |
| test/cases/80-Components/01-Taosd/test_com_taosd_audit.py | Adjusts audit test DB creation params and replaces a debug print with tdLog logging. |
| source/util/src/tjson.c | Adds string-pointer accessor and tweaks string copy behavior. |
| include/util/tjson.h | Declares tjsonGetStringPointer API + lifetime documentation. |
| include/util/tdef.h | Adds AUDIT_STABLE_NAME constant. |
| source/util/src/terror.c | Adds new audit error message string. |
| include/util/taoserror.h | Adds new audit error code definition. |
| docs/zh/14-reference/09-error-code.md | Documents new audit error code (zh). |
| docs/en/14-reference/09-error-code.md | Fixes “param,eter” typo and documents new audit error code (en). |
| source/libs/audit/inc/auditInt.h | Extends audit internal state with epSet/vgId. |
| include/libs/audit/audit.h | Minor formatting + adds typedef for epset callback (unused here). |
| source/dnode/vnode/src/vnd/vnodeSvr.c | Adds TDMT_VND_AUDIT_RECORD processing and JSON→row insertion path into audit tables. |
| source/dnode/mnode/impl/src/mndDb.c | Enforces single-vgroup audit DB and triggers audit stb creation when saving-in-self. |
| source/dnode/mnode/impl/src/mndStb.c | Implements audit super table creation + registers audit record response handler. |
| source/dnode/mnode/impl/inc/mndStb.h | Exposes mndCreateAuditStb declaration. |
| source/dnode/mnode/impl/src/mndUser.c | Adds includes and minor whitespace. |
| source/dnode/mnode/impl/src/mndIndex.c | Adds a commit-log info line. |
| source/dnode/mnode/impl/src/mndTrans.c | Lowers log level from info→debug. |
| source/dnode/mgmt/mgmt_vnode/src/vmHandle.c | Routes new audit-record vnode msg to write queue. |
| include/common/tmsgdef.h | Adds TDMT_VND_AUDIT_RECORD message type definition (request only). |
| include/common/tmsg.h | Extends status req/rsp with auditEpSet/auditVgId and adds SVAuditRecordReq wire struct. |
| source/common/src/msg/tmsg.c | Serializes/deserializes new status fields and adds SVAuditRecordReq codec. |
| include/common/tmsgcb.h | Adds AUDIT_QUEUE enum value. |
| source/dnode/mgmt/mgmt_mnode/inc/mmInt.h | Adds audit worker + queue API declaration. |
| source/dnode/mgmt/mgmt_mnode/src/mmWorker.c | Implements audit worker/queue wiring and cleanup. |
| source/dnode/mgmt/mgmt_mnode/src/mmHandle.c | Registers handler to route audit record responses to audit queue. |
| source/dnode/mgmt/mgmt_dnode/src/dmHandle.c | Extends audit status propagation functions to include epSet/vgId. |
| source/common/src/tglobal.c | Adds auditSaveInSelf config handling and dynamic option registration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (void)atomic_add_fetch_64(&pVnode->statis.nInsertSuccess, pSubmitRsp->affectedRows); | ||
| (void)atomic_add_fetch_64(&pVnode->statis.nBatchInsert, 1); | ||
| if (code == 0) { |
There was a problem hiding this comment.
Insert statistics are updated as “success” regardless of whether code is non-zero. In particular, nInsertSuccess (and potentially other success counters) should only be incremented on success; otherwise failures can be misreported/overcounted.
| (void)atomic_add_fetch_64(&pVnode->statis.nInsertSuccess, pSubmitRsp->affectedRows); | |
| (void)atomic_add_fetch_64(&pVnode->statis.nBatchInsert, 1); | |
| if (code == 0) { | |
| (void)atomic_add_fetch_64(&pVnode->statis.nBatchInsert, 1); | |
| if (code == 0) { | |
| (void)atomic_add_fetch_64(&pVnode->statis.nInsertSuccess, pSubmitRsp->affectedRows); |
| // update metrics | ||
| METRICS_UPDATE(pVnode->writeMetrics.total_requests, METRIC_LEVEL_LOW, 1); | ||
| METRICS_UPDATE(pVnode->writeMetrics.total_rows, METRIC_LEVEL_HIGH, pSubmitRsp->affectedRows); | ||
| // METRICS_UPDATE(pVnode->writeMetrics.total_bytes, METRIC_LEVEL_LOW, pMsg->header.contLen); |
There was a problem hiding this comment.
Write metrics are updated even when code != 0. If the write fails (or partially fails), this can inflate total_requests/total_rows. Gate these metrics updates on success (or update dedicated failure metrics instead).
| } | ||
|
|
||
| print ("===================: ", updatecfgDict) | ||
| tdLog.info("===================: ", updatecfgDict) |
There was a problem hiding this comment.
tdLog.info uses stdlib logging-style % formatting; passing updatecfgDict as a second positional arg without a %s placeholder will raise a formatting TypeError during logging. Convert this to a single formatted string (e.g., f-string) or add an explicit %s placeholder in the message.
| tdLog.info("===================: ", updatecfgDict) | |
| tdLog.info("===================: %s", updatecfgDict) |
| // mndSetMsgHandle(pMnode, TDMT_VND_DROP_INDEX_RSP, mndTransProcessRsp); | ||
| mndSetMsgHandle(pMnode, TDMT_VND_AUDIT_RECORD_RSP, mndProcessAuditRecordRsp); |
There was a problem hiding this comment.
TDMT_VND_AUDIT_RECORD_RSP is referenced here but does not appear to be defined anywhere in the repo (only usages found). This will fail to compile; add the corresponding message type definition (and serialization if needed) or switch to an existing response message type.
| // mndSetMsgHandle(pMnode, TDMT_VND_DROP_INDEX_RSP, mndTransProcessRsp); | |
| mndSetMsgHandle(pMnode, TDMT_VND_AUDIT_RECORD_RSP, mndProcessAuditRecordRsp); | |
| // mndSetMsgHandle(pMnode, TDMT_VND_DROP_INDEX_RSP, mndTransProcessRsp); | |
| #ifdef TDMT_VND_AUDIT_RECORD_RSP | |
| mndSetMsgHandle(pMnode, TDMT_VND_AUDIT_RECORD_RSP, mndProcessAuditRecordRsp); | |
| #endif |
| SMnode *pMnode = pRsp->info.node; | ||
| SSdb *pSdb = pMnode->pSdb; | ||
| (void)pMnode; // currently unused, kept for potential future use | ||
| (void)pSdb; // currently unused, kept for potential future use | ||
|
|
||
| if (pRsp == NULL) { | ||
| mError("audit record rsp, null response message"); | ||
| return -1; | ||
| } | ||
|
|
There was a problem hiding this comment.
mndProcessAuditRecordRsp dereferences pRsp (via pRsp->info.node) before checking whether pRsp is NULL. Move the NULL check to the top of the function before any dereference to avoid a crash.
| SMnode *pMnode = pRsp->info.node; | |
| SSdb *pSdb = pMnode->pSdb; | |
| (void)pMnode; // currently unused, kept for potential future use | |
| (void)pSdb; // currently unused, kept for potential future use | |
| if (pRsp == NULL) { | |
| mError("audit record rsp, null response message"); | |
| return -1; | |
| } | |
| if (pRsp == NULL) { | |
| mError("audit record rsp, null response message"); | |
| return -1; | |
| } | |
| SMnode *pMnode = pRsp->info.node; | |
| SSdb *pSdb = pMnode->pSdb; | |
| (void)pMnode; // currently unused, kept for potential future use | |
| (void)pSdb; // currently unused, kept for potential future use |
| if (dmSetMgmtHandle(pArray, TDMT_SYNC_SNAPSHOT_RSP, mmPutMsgToSyncRdQueue, 1) == NULL) goto _OVER; | ||
| if (dmSetMgmtHandle(pArray, TDMT_SYNC_PREP_SNAPSHOT_REPLY, mmPutMsgToSyncRdQueue, 1) == NULL) goto _OVER; | ||
| if (dmSetMgmtHandle(pArray, TDMT_STREAM_FETCH, mmPutMsgToStreamReaderQueue, 1) == NULL) goto _OVER; | ||
| if (dmSetMgmtHandle(pArray, TDMT_VND_AUDIT_RECORD_RSP, mmPutMsgToAuditQueue, 0) == NULL) goto _OVER; |
There was a problem hiding this comment.
TDMT_VND_AUDIT_RECORD_RSP is used here but does not appear to be defined anywhere (only usages found). This will fail to compile; define the message type (and handler mapping) or use an existing message type.
| if (dmSetMgmtHandle(pArray, TDMT_VND_AUDIT_RECORD_RSP, mmPutMsgToAuditQueue, 0) == NULL) goto _OVER; |
| TD_DEF_MSG_TYPE(TDMT_VND_QUERY_TRIM_PROGRESS, "vnode-query-trim-progress", NULL, NULL) | ||
| TD_DEF_MSG_TYPE(TDMT_VND_SET_KEEP_VERSION, "vnode-set-keep-version", NULL, NULL) | ||
| TD_DEF_MSG_TYPE(TDMT_VND_TRIM_WAL, "vnode-trim-wal", NULL, NULL) | ||
| TD_DEF_MSG_TYPE(TDMT_VND_AUDIT_RECORD, "vnode-audit-record", NULL, NULL) |
There was a problem hiding this comment.
Only TDMT_VND_AUDIT_RECORD is added here, but the code also references TDMT_VND_AUDIT_RECORD_RSP. Add the missing *_RSP message definition (and keep request/response pairs consistent) to avoid undefined-symbol compile errors.
| TD_DEF_MSG_TYPE(TDMT_VND_AUDIT_RECORD, "vnode-audit-record", NULL, NULL) | |
| TD_DEF_MSG_TYPE(TDMT_VND_AUDIT_RECORD, "vnode-audit-record", NULL, NULL) | |
| TD_DEF_MSG_TYPE(TDMT_VND_AUDIT_RECORD_RSP, "vnode-audit-record-rsp", NULL, NULL) |
| code = tjsonGetStringValue2(pJson, "client_add", record->clientAddress, 50); | ||
| if (code < 0) return TSDB_CODE_INVALID_JSON_FORMAT; |
There was a problem hiding this comment.
vnodeDecodeAuditRecord expects the JSON key client_add, but this key only appears here in the repo. If the producer uses a different key (e.g., client_address), decoding will fail with TSDB_CODE_INVALID_JSON_FORMAT. Align this field name with the JSON producer or accept both keys for backward compatibility.
| code = tjsonGetStringValue2(pJson, "client_add", record->clientAddress, 50); | |
| if (code < 0) return TSDB_CODE_INVALID_JSON_FORMAT; | |
| code = tjsonGetStringValue2(pJson, "client_address", record->clientAddress, 50); | |
| if (code < 0) { | |
| code = tjsonGetStringValue2(pJson, "client_add", record->clientAddress, 50); | |
| if (code < 0) return TSDB_CODE_INVALID_JSON_FORMAT; | |
| } |
1 participant
Description
Issue(s)
Checklist
Please check the items in the checklist if applicable.