feat: support sql server - #11214
Conversation
73b49a3 to
ab24cd2
Compare
5c78337 to
0cc7a56
Compare
09d3c60 to
c329c66
Compare
052f29e to
767dad0
Compare
|
@claude review 一下代码 |
|
@claude review一下当前PR所提交的代码修改 |
|
@claude 你好,请 review 一下代码 |
|
Claude finished @SongZhen0704's task in 6m 36s —— View job Code Review: SQL Server (TDS) Protocol Support
Overall, the PR adds the necessary wiring for SQL Server (TDS protocol) support across the full stack — eBPF inference, Rust L7 parser, server-side config, and querier enum. The structure follows existing patterns well (similar to how Oracle, MySQL, etc. are integrated). However, I found several issues, including one critical bug in the eBPF header parsing. Critical Issues1. Per the MS-TDS packet header specification, the 8-byte TDS header layout is:
The code defines // Current (incorrect):
#define LENGTH_OFFSET 1
// Should be:
#define LENGTH_OFFSET 22. pub fn new(_: &[u8]) -> Self {
TdsParser::default() // ignores input payload entirely
}
pub fn parse(&mut self) -> Result<(), ParserError> {
Err(ParserError::InvalidData) // always fails
}The
The eBPF layer will detect SQL Server traffic, but the userspace parser will never successfully parse any TDS packets. Is this intentional as a first-phase PR with the actual parser coming later? If so, it should be documented. If not, the actual TDS parsing logic is missing. Moderate Issues3. Documentation incorrectly marks SqlServer as Enterprise-only Multiple places in the docs state:
However, in the code (
4. Generic #define HEADER_SIZE 8
#define MESSAGE_TYPE_OFFSET 0
#define LENGTH_OFFSET 1
#define WINDOWS_OFFSET 7These are defined inside the function body but C macros have file scope — they are not scoped to the function. Names like #define TDS_HEADER_SIZE 8
#define TDS_MESSAGE_TYPE_OFFSET 0
#define TDS_LENGTH_OFFSET 2
#define TDS_WINDOWS_OFFSET 75. Missing reference link for TDS protocol inference ( As also noted by @yinjiping — other protocol inference functions include reference links (e.g., Oracle references Wireshark dissector code, ISO8583 references Wikipedia). The // ref: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-tds/Minor Issues6. Mixed indentation ( Two lines use spaces instead of tabs, inconsistent with the surrounding code: char pkt_type = buf[MESSAGE_TYPE_OFFSET];
__u16 pkt_length = __bpf_ntohs(*(__u16 *) & buf[LENGTH_OFFSET]); // ← spaces
char windows = buf[WINDOWS_OFFSET]; // ← spaces7. Dead code: The 8. Response status only distinguishes if let Some(error_message) = tds.error_message {
info.error_message = error_message;
info.resp_status = L7ResponseStatus::ClientError;TDS errors can also be server errors (severity-based). Currently all errors are mapped to 9. No unit tests The PR checklist notes "Added unit test" is unchecked. Other protocol implementations in this codebase include test modules. Tests should at minimum cover:
What looks good
| |
|
@yuanchaoa 可在agent/src/ebpf 目录下 在 10.50.100.64 中测试下,5.2, 4.19, 4.14 内核是否正常运行(运行下socket_tracer程序) |
767dad0 to
cd24437
Compare
130d1ad to
85e64a2
Compare
|
@claude review一下当前PR所提交的代码修改 |
|
Claude finished @yuanchaoa's task in 9m 5s —— View job Review complete
I reviewed the SQL Server support changes and found four concrete issues worth fixing before merge.
I did not re-list the lower-priority style/doc comments that are already present in the PR discussion. |
|
@claude review一下当前PR所提交的代码修改, 用中文回复 |
|
Claude finished @yuanchaoa's task in 17m 33s —— View job Todo
Review 结论我重点看了 eBPF 协议推断、Rust SQL Server parser,以及 CE / EE 两条接线。当前有 5 个需要在合并前处理的问题;我没有重复 PR 讨论里已经提过的纯文档/格式类意见。
总结建议至少先修掉前 4 个问题再合并;第 5 个如果暂时还保留占位实现,也需要一起处理掉,不然 |
85e64a2 to
ab126d0
Compare
|
@claude review一下当前PR所提交的代码修改, 用中文回复 |
|
Claude finished @yuanchaoa's task in 8m 42s —— View job Todo
Review 结论我重新按
补充说明
总体上看,这个 PR 现在的主要问题不是“接线没接上”,而是TDS parser 还没有真正实现;建议至少把第 1 项解决后再合并,第 2 项也最好顺手收干净,避免 CE/EE 行为继续分叉。 |
ab126d0 to
2f3ca0f
Compare
This PR is for:
feat: support sql server
Checklist
Backport to branches