eventpb,sql: lay some foundation for redactable statement strings#66431
Conversation
|
This change is |
9a9f717 to
499bff9
Compare
499bff9 to
e94ff7b
Compare
maryliag
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @knz, @rauchenstein, and @THardy98)
pkg/sql/event_log.go, line 486 at r1 (raw file):
// In the system.eventlog table, we do not use redaction markers. // (compatibility with previous versions of CockroachDB.) infoBytes = infoBytes.StripMarkers()
I'm not following this part. Why do you need to append the marker if you will remove right after?
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/event_log.go, line 486 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
I'm not following this part. Why do you need to append the marker if you will remove right after?
The problem is that the json.Marshal code that was there prior to this PR does not work with fields that have type redact.RedactableString - it takes the redaction markers as part of the JSON serialization.
The consumers of system.eventlog require all the strings to never contain redaction markers.
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/event_log.go, line 486 at r1 (raw file):
Previously, knz (kena) wrote…
The problem is that the
json.Marshalcode that was there prior to this PR does not work with fields that have typeredact.RedactableString- it takes the redaction markers as part of the JSON serialization.The consumers of
system.eventlogrequire all the strings to never contain redaction markers.
It's also incorrect/unsafe to use StripMarkers (or equivalent logic) on the output of json.Marshal, because that would break events that contain a unicode character as part of a SQL literal, and the unicode character happens to be the same as a redaction marker (e.g. INSERT INTO ... VALUES('›') )
maryliag
left a comment
There was a problem hiding this comment.
Thank you for this!
Reviewable status:
pkg/sql/event_log.go, line 486 at r1 (raw file):
Previously, knz (kena) wrote…
It's also incorrect/unsafe to use StripMarkers (or equivalent logic) on the output of
json.Marshal, because that would break events that contain a unicode character as part of a SQL literal, and the unicode character happens to be the same as a redaction marker (e.g.INSERT INTO ... VALUES('›'))
Got it!
dhartunian
left a comment
There was a problem hiding this comment.
Reviewed 6 of 10 files at r1.
Reviewable status:
Prior to this change, the centralized structured logging logic and the query logging mechanism were both assuming that the entirety of the statement string was unsafe for reporting. We want in a later change to introduce some nuance into this. However, the structured event logging encoding did not know anything else than strings that were either completely-safe or completely-unsafe. This patch introduces the possibility for a structured event to contain a string of go type `redact.RedactableString`, where the caller has already taken responsibility for placing redaction markers. Release note: None
e94ff7b to
8531ddd
Compare
|
TFYRs! bors r=maryliag,dhartunian |
|
Build succeeded: |
Informs #65401.
Needed as prerequisite for #66359.
Prior to this change, the centralized structured logging logic and the
query logging mechanism were both assuming that the entirety of the
statement string was unsafe for reporting.
We want in a later change to introduce some nuance into this. However,
the structured event logging encoding did not know anything else than
strings that were either completely-safe or completely-unsafe.
This patch introduces the possibility for a structured event to
contain a string of go type
redact.RedactableString, where thecaller has already taken responsibility for placing redaction markers.
Release note: None