Redact protocol error log when hide-user-data-from-log enabled #1889
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hpatro
reviewed
Mar 26, 2025
QuChen88
reviewed
Mar 26, 2025
QuChen88
reviewed
Mar 26, 2025
YueTang-Vanessa
force-pushed
the
unstable
branch
from
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:
|
hpatro
reviewed
Mar 27, 2025
zuiderkwast
reviewed
Mar 27, 2025
YueTang-Vanessa
force-pushed
the
unstable
branch
2 times, most recently
from
74ff2b0 to
addd428
Compare
|
Looks good to me |
QuChen88
approved these changes
Mar 28, 2025
|
@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. |
zuiderkwast
reviewed
Mar 28, 2025
YueTang-Vanessa
force-pushed
the
unstable
branch
from
addd428 to
e703ac9
Compare
madolson
reviewed
Mar 28, 2025
YueTang-Vanessa
force-pushed
the
unstable
branch
3 times, most recently
from
ffde5c4 to
6d8099b
Compare
Signed-off-by: VanessaTang <yuetan@amazon.com>
YueTang-Vanessa
force-pushed
the
unstable
branch
from
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>
ranshid
approved these changes
Mar 30, 2025
ranshid
added a commit
to ranshid/valkey
that referenced
this pull request
Mar 31, 2025
…y-io#1889) 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. --------- Signed-off-by: VanessaTang <yuetan@amazon.com> Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: Ran Shidlansik <ranshid@amazon.com>
ranshid
added a commit
that referenced
this pull request
Mar 31, 2025
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. --------- Signed-off-by: VanessaTang <yuetan@amazon.com> Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: Ran Shidlansik <ranshid@amazon.com>
RayaCoo
pushed a commit
to RayaCoo/valkey
that referenced
this pull request
Apr 3, 2025
…y-io#1889) 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. --------- Signed-off-by: VanessaTang <yuetan@amazon.com> Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: Ran Shidlansik <ranshid@amazon.com>
zarkash-aws
pushed a commit
to zarkash-aws/valkey
that referenced
this pull request
Apr 6, 2025
…y-io#1889) 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. --------- Signed-off-by: VanessaTang <yuetan@amazon.com> Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: Ran Shidlansik <ranshid@amazon.com> Signed-off-by: Shai Zarka <zarkash@amazon.com>
murphyjacob4
pushed a commit
to enjoy-binbin/valkey
that referenced
this pull request
Apr 13, 2025
…y-io#1889) 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. --------- Signed-off-by: VanessaTang <yuetan@amazon.com> Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: Ran Shidlansik <ranshid@amazon.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In this code logic: https://github.com/valkey-io/valkey/blob/unstable/src/networking.c#L2767-L2773,
c->querybuf + c->qb_posmay also include user data.Update the log message when config
hide-user-data-from-logis enabled.