-
Notifications
You must be signed in to change notification settings - Fork 760
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
Redact protocol error log when hide-user-data-from-log enabled #1889
base: unstable
Are you sure you want to change the base?
Conversation
36f37c0
to
9813901
Compare
When exactly is this triggered? Only when the client sends some incorrectly encoded RESP? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #1889 +/- ##
============================================
- Coverage 71.00% 71.00% -0.01%
============================================
Files 123 123
Lines 65671 65675 +4
============================================
+ Hits 46631 46632 +1
- Misses 19040 19043 +3
🚀 New features to boost your workflow:
|
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.
Could you add a test case and assert *redacted*
on it ?.
74ff2b0
to
addd428
Compare
Looks good to me |
@zuiderkwast Basically any sort of protocol parsing error from the primary to the replica can lead to this condition to get triggered and the query buffer containing potential user data/commands would get logged. This can happen for various reasons including data corruption over the wire etc. i.e. we saw this in a couple of our redis clusters in the past.
The chances of this happening is pretty low, but still non zero. |
addd428
to
e703ac9
Compare
ffde5c4
to
6d8099b
Compare
Signed-off-by: VanessaTang <yuetan@amazon.com>
6d8099b
to
42b0bb9
Compare
1. print the query buffer info in case hide-user-data-from-log is disabled 2. fix tests to wait for the information in the log Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
In this code logic: https://github.com/valkey-io/valkey/blob/unstable/src/networking.c#L2767-L2773,
c->querybuf + c->qb_pos
may also include user data.Update the log message when config
hide-user-data-from-log
is enabled.