Repository navigation
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/release-note-label-needed
|
Hi @Tema. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
Bot
added
the
needs-ok-to-test
|
|
ti-chi-bot
Bot
added
size/XXL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @Tema, 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!
I've implemented a robust log redaction system across all TiFlow components to enhance security and control over sensitive data in logs. This involved creating a new unified redaction package, integrating it into the existing logging infrastructure, and updating configuration mechanisms to support flexible redaction modes (OFF, ON, MARKER). I've applied these changes broadly to ensure sensitive information like database keys, queries, and URLs are properly masked, while maintaining backward compatibility for existing deployments. This work significantly improves data privacy and compliance without hindering debugging capabilities.
Highlights
- Unified Log Redaction Framework: I've introduced a new unified log redaction package (
pkg/logutil/redact) that provides core functionalities for redacting strings, keys, and values, supporting three distinct modes: OFF (no redaction), ON (replaces sensitive data with '?'), and MARKER (encloses sensitive data with '‹›' for later de-redaction). - Enhanced Logging Utilities: I've integrated the new redaction capabilities into the main logging utility (
pkg/logutil), offering convenient functions likeRedactString,RedactKey, andZapRedactStringto easily apply redaction when logging sensitive information. - Unified Configuration for Redaction Modes: I've updated the configuration systems for both TiCDC server (
pkg/config/server_config.go) and DM worker (dm/worker/config.go) to use a newRedactInfoLogType. This allows users to configure log redaction via boolean values (true/false) or the string "MARKER" in TOML/JSON files and command-line flags, providing consistent behavior across components. - Broad Application of Redaction: I've systematically applied the new redaction functions across various TiFlow components (CDC, DM, Engine) to protect sensitive data such as database keys, DDL queries, API query parameters, Etcd keys, HTTP URLs, and SQL statements from being exposed in logs.
- Backward Compatibility: I've ensured backward compatibility for existing DM configurations and logging calls, allowing a smooth transition to the new unified redaction system without breaking current deployments.
- New Documentation and Testing: I've added comprehensive documentation (
docs/log-redaction.md) detailing how to configure and use the new log redaction feature, including examples for each mode and guidance on de-redaction for debugging purposes.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
ti-chi-bot
added
ok-to-test
|
Now you can start all CI jobs with |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive and well-designed log redaction feature across the TiFlow project, which is a great improvement for security and data privacy. The implementation is thorough, covering various components like CDC, DM, and the engine, and correctly redacts sensitive information such as keys, queries, and URLs. The addition of different redaction modes (ON, OFF, MARKER) provides excellent flexibility for different environments. The new logutil packages are well-structured, and the inclusion of extensive unit and integration tests is commendable. I have a couple of minor suggestions to improve error handling and documentation style.
| // Initialize log redaction using the unified RedactInfoLogType | ||
| err2 := logutil.InitLogRedactionFromType(cfg.RedactInfoLog) | ||
| if err2 != nil { | ||
| common.PrintLinesf("init log redaction error %s", terror.Message(err2)) | ||
| os.Exit(2) | ||
| } | ||
|
|
||
| // For backward compatibility, also set DM's legacy redaction | ||
| log.SetRedactLog(cfg.RedactInfoLog != logutil.RedactInfoLogOFF) | ||
| if err != nil { | ||
| common.PrintLinesf("init logger error %s", terror.Message(err)) | ||
| os.Exit(2) |
There was a problem hiding this comment.
The error handling here can be improved for correctness and clarity:
- The error from
log.InitLogger(line 51) should be checked immediately after the call, not after initializing redaction. This prevents running setup logic on a faulty state. - A new variable
err2is introduced. It's cleaner to reuse theerrvariable.
A suggested refactoring would be to check err right after line 55, and then reuse err for the logutil.InitLogRedactionFromType call.
| Convenience functions: | ||
| - `InitLogRedactionFromType(redactType RedactInfoLogType) error`: Initialize redaction from unified type | ||
| - `RedactString(s string) string`: Redact string with current settings | ||
| - `ZapRedactString(key, value string) zap.Field`: Create redacted zap field No newline at end of file |
There was a problem hiding this comment.
This file is missing a final newline character. It's a common convention and good practice to end all text files with a newline to prevent issues with some tools and scripts.
Add opt-in OFF, ON, and MARKER modes for annotated row values, keys, DML arguments, scheduler messages, and etcd values. Share the mode with TiDB errors and TiKV client logs while preserving default log behavior. Defer marker formatting until field encoding, escape embedded markers, and keep parameterized SQL templates readable. Preserve CLI precedence and boolean/marker serialization, and protect inputs embedded in Avro parse errors. Add real-path log regressions, a MySQL replication test, and configuration documentation. Related implementation: pingcap/ticdc#2919.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 31.58% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 38 functions across 50 files. (19 skipped… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly and concisely describes the main change: configurable sensitive-data redaction for TiCDC logs. Description check The description covers the required problem statement, issue reference, implementation details, tests, compatibility considerations, documentation updates, and release note.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 31.58% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 38 functions across 50 files. (19 skipped: 3 unsupported, 16 over the file limit.)
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks the logs at night
And wraps each key from sight
OFF keeps values as they came
ON masks them all the same
MARKER leaves its sign in view
Then hops along with secrets few
Comment @coderabbitai help to get the list of available commands.
Tema
changed the title
ti-chi-bot
Bot
added
release-note
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔇 Additional comments: 33
pkg/orchestrator/etcd_worker.go: 1
28-28: LGTM!Also applies to: 224-224, 366-366
pkg/orchestrator/reactor_state.go: 1
26-26: LGTM!Also applies to: 160-160, 432-432
pkg/orchestrator/reactor_state_test.go: 1
24-24: LGTM!Also applies to: 28-28, 32-34, 723-762
pkg/logutil/redact/redact.go: 1
1-205: LGTM!pkg/logutil/redact_type.go: 1
1-218: LGTM!pkg/logutil/redact_type_test.go: 1
1-324: LGTM!pkg/logutil/log.go: 1
19-19: LGTM!Also applies to: 30-30, 354-373
pkg/logutil/log_redact_test.go: 1
1-261: LGTM!pkg/logutil/redact/redact_test.go: 1
1-303: LGTM!pkg/logutil/integration_test.go: 1
1-84: LGTM!pkg/logutil/log_redact_dependencies_test.go: 1
1-61: LGTM!pkg/security/credential.go: 1
26-26: LGTM!Also applies to: 50-51
pkg/cmd/server/server.go: 1
73-74: LGTM!Also applies to: 110-116, 207-208, 267-268
pkg/cmd/server/server_test.go: 1
26-26: LGTM!Also applies to: 110-184
pkg/cmd/util/ticdc.toml: 1
37-38: LGTM!pkg/config/config_test_data.go: 1
146-147: LGTM!docs/log-redaction.md: 1
1-65: LGTM!cdc/sink/dmlsink/blackhole/black_hole_dml_sink.go: 1
22-22: LGTM!Also applies to: 43-43
cdc/sink/dmlsink/blackhole/log_redaction_test.go: 1
33-73: LGTM!cdc/sink/dmlsink/mq/worker.go: 1
30-30: LGTM!Also applies to: 142-142, 256-256
cdc/sink/dmlsink/txn/mysql/mysql.go: 1
230-230: LGTM!Also applies to: 640-640, 667-667, 714-714
cdc/sink/dmlsink/txn/mysql/mysql_test.go: 1
106-211: LGTM!cdc/sink/tablesink/progress_tracker.go: 1
312-312: LGTM!cdc/sink/tablesink/table_sink_impl.go: 1
243-243: LGTM!Also applies to: 261-261
pkg/sink/codec/avro/avro.go: 1
928-936: LGTM!pkg/sink/codec/avro/confluent_schema_registry_test.go: 1
28-31: LGTM!pkg/sink/codec/avro/log_redaction_test.go: 1
39-213: LGTM!pkg/sqlmodel/multirow.go: 1
187-187: LGTM!pkg/sqlmodel/multirow_redaction_test.go: 1
31-64: LGTM!tests/integration_tests/log_redaction/conf/changefeed.toml: 1
1-2: LGTM!tests/integration_tests/log_redaction/run.sh: 1
1-91: LGTM!tests/integration_tests/run_group.sh: 1
14-14: LGTM!engine/pkg/orm/log.go: 1
94-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Redacting the whole SQL string removes the ORM query template from logs.
logutil.ZapRedactString("sql", sql)replaces the complete SQL text with?in ON mode. GORM'sTracecallback supplies interpolated SQL, so the string can contain literal values. Redaction can be correct for this reason. The cost is that the slow-log and error-log entries no longer show which query ran, even when that query contains no sensitive literals. This behavior differs from the MySQL sink, which keeps templates readable. If you want the same troubleshooting value, log the statement type or a hash next to the redacted SQL. Otherwise, document that ORM SQL is fully redacted.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
b051cd3e-f637-490e-8712-459521025e81
📒 Files selected for processing: 69
cdc/api/middleware/middleware.gocdc/entry/mounter.gocdc/kv/matcher.gocdc/kv/matcher_redaction_test.gocdc/kv/regionlock/region_range_lock.gocdc/kv/shared_client.gocdc/kv/shared_region_worker.gocdc/model/sink.gocdc/model/sink_redaction_test.gocdc/processor/memquota/mem_quota.gocdc/processor/processor.gocdc/processor/sinkmanager/manager.gocdc/processor/sinkmanager/redo_log_advancer.gocdc/processor/sinkmanager/redo_log_worker.gocdc/processor/sinkmanager/table_sink_advancer.gocdc/processor/sinkmanager/table_sink_worker.gocdc/processor/sinkmanager/table_sink_wrapper.gocdc/processor/sinkmanager/tasks.gocdc/processor/sourcemanager/sorter/memory/event_sorter.gocdc/processor/sourcemanager/sorter/pebble/event_sorter.gocdc/puller/multiplexing_puller.gocdc/redo/manager.gocdc/scheduler/internal/v3/agent/agent.gocdc/scheduler/internal/v3/agent/agent_test.gocdc/scheduler/internal/v3/agent/table.gocdc/scheduler/internal/v3/agent/table_test.gocdc/scheduler/internal/v3/coordinator.gocdc/scheduler/internal/v3/keyspan/reconciler.gocdc/scheduler/internal/v3/keyspan/splitter_region_count.gocdc/scheduler/internal/v3/keyspan/splitter_write.gocdc/scheduler/internal/v3/replication/replication_manager.gocdc/scheduler/internal/v3/replication/replication_set.gocdc/scheduler/internal/v3/replication/replication_set_test.gocdc/scheduler/internal/v3/scheduler/scheduler_drain_capture.gocdc/scheduler/internal/v3/scheduler/scheduler_manager.gocdc/scheduler/internal/v3/scheduler/scheduler_move_table.gocdc/sink/dmlsink/blackhole/black_hole_dml_sink.gocdc/sink/dmlsink/blackhole/log_redaction_test.gocdc/sink/dmlsink/mq/worker.gocdc/sink/dmlsink/txn/mysql/mysql.gocdc/sink/dmlsink/txn/mysql/mysql_test.gocdc/sink/tablesink/progress_tracker.gocdc/sink/tablesink/table_sink_impl.godocs/log-redaction.mdengine/pkg/orm/log.gopkg/cmd/server/server.gopkg/cmd/server/server_test.gopkg/cmd/util/ticdc.tomlpkg/config/config_test_data.gopkg/logutil/integration_test.gopkg/logutil/log.gopkg/logutil/log_redact_dependencies_test.gopkg/logutil/log_redact_test.gopkg/logutil/redact/redact.gopkg/logutil/redact/redact_test.gopkg/logutil/redact_type.gopkg/logutil/redact_type_test.gopkg/orchestrator/etcd_worker.gopkg/orchestrator/reactor_state.gopkg/orchestrator/reactor_state_test.gopkg/security/credential.gopkg/sink/codec/avro/avro.gopkg/sink/codec/avro/confluent_schema_registry_test.gopkg/sink/codec/avro/log_redaction_test.gopkg/sqlmodel/multirow.gopkg/sqlmodel/multirow_redaction_test.gotests/integration_tests/log_redaction/conf/changefeed.tomltests/integration_tests/log_redaction/run.shtests/integration_tests/run_group.sh
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.
|
@Tema: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
What problem does this PR solve?
TiCDC diagnostics can expose row values, keys, DML arguments, and etcd values
containing changefeed credentials. Operators need an opt-in way to hide these
values or mark them for removal before sharing logs.
Issue Number: ref #859
What is changed and how it works?
cdc server --redact-info-logand[security].redact-info-logwith OFF(
false, default), ON (true, replace annotated content with?), and MARKER(
"MARKER", enclose content in‹...›) modes. Explicit CLI flags override theconfiguration file. Preserve boolean/marker JSON and TOML representations.
Annotate row values, keys/spans, scheduler messages and status maps, MySQL
arguments, API query strings, and etcd values/watch events. Cover debug,
warning, error, and panic paths, including input embedded in Avro year errors.
formatting. Escape literal marker characters and retain the mode selected
when each field was created. Avoid evaluating sensitive Stringers in ON mode.
and etcd-credential regressions plus a MySQL replication integration case.
Document configuration, precedence, marker escaping, and coverage limits.
Parameterized DML templates and schema/table/column names remain readable;
arguments are protected separately. DDL and region error objects also remain
readable, so DDL DEFAULT literals can contain sensitive data. MARKER retains
sensitive values until its marked content is removed. This protects annotated
fields and does not guarantee that every third-party error or log is redacted.
TiKV's key helpers suppress keys in both ON and MARKER. DM retains its existing
setting. Restart TiCDC to change the configured mode.
The shared state and diagnostic choices align with
TiCDC's log-redaction implementation.
Check List
Tests
Passed locally with Go 1.26.9 on darwin/amd64, CGO enabled:
KV, processor/sink-manager, and ORM suites also passed with their required
failpoints enabled. Temporary failpoint instrumentation was restored after the
suites.
The new MySQL replication case waits for unchanged downstream values, requires
real DML log entries, checks ON-mode absence across the log, and verifies escaped
MARKER arguments. Cluster binaries are unavailable locally, so it has not run.
Run it in a prepared integration environment with:
Questions
Will it cause performance regression or break compatibility?
OFF is the default and preserves existing annotated field types and values. ON
and MARKER intentionally change annotated field content. MARKER defers work for
filtered entries; enabled logging still incurs formatting/escaping overhead.
The server option is opt-in and requires a restart to change modes. DM's existing
configuration is retained.
Do you need to update user documentation, design documentation or monitoring documentation?
Yes. Add
docs/log-redaction.mdand update the example server configuration.The guide explains the modes, CLI precedence, shared dependency behavior,
readable SQL/DDL, marker handling, and coverage limits.
Release note
Summary by CodeRabbit
--redact-info-logoption; the command-line setting takes precedence.