Skip to content
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.

Sign up for GitHub

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

Jump to bottom

log: the 21.2 refactor removed auto-redaction for log tags #72905

Closed
knz opened this issue Nov 18, 2021 · 6 comments · Fixed by #72992
Closed

log: the 21.2 refactor removed auto-redaction for log tags #72905

knz opened this issue Nov 18, 2021 · 6 comments · Fixed by #72992
Assignees
@dhartunian
Labels
A-logging In and around the logging infrastructure. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. regression Regression from a release.

Comments

@knz
Copy link
Contributor

knz commented Nov 18, 2021

Describe the problem

In 21.1 and prior versions, the "tags" part of a log entry envelope were redacted if the sink flag redact was set to true.

In 21.2, this redaction is not taking place any more. This is a regression.

To Reproduce

cockroach start-single-node ... --log='sinks: {stderr: {filter: INFO, redact: true}}'

Expected behavior

redactable value are redacted
@knz knz added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. regression Regression from a release. A-logging In and around the logging infrastructure. labels Nov 18, 2021
@blathers-crl blathers-crl bot added the T-server-and-security DB Server & Security label Nov 18, 2021
@knz
Copy link
Contributor Author

knz commented Nov 18, 2021

There's another issue: if redactable: false is set, any redactable tags do not get their redaction markers stripped.

@knz
Copy link
Contributor Author

knz commented Nov 18, 2021

cc @thtruo @dhartunian @jtsiros for triage

@knz
Copy link
Contributor Author

knz commented Nov 18, 2021

NB: this currently impacts the CC infrastructure and the 21.2 telemetry pipeline

@thtruo
Copy link
Contributor

thtruo commented Nov 18, 2021

Thanks @knz just wanted to clarify if you see work that will be split between DB Server and Obs Infra, or if Obs Infra will take over now. If the latter, I'll update the T-* labels so that it appears in our backlog for triaging

cc @nkodali to put on our radar

@knz knz mentioned this issue Nov 19, 2021
Merged
@knz
Copy link
Contributor Author

knz commented Nov 19, 2021

I have implemented a unit test that demonstrates the problem in #72992.

@dhartunian dhartunian added T-observability-inf and removed T-server-and-security DB Server & Security labels Nov 22, 2021
@knz
Copy link
Contributor Author

knz commented Dec 20, 2021

changed #72992 to include a fix too.

@blathers-crl blathers-crl bot mentioned this issue Dec 21, 2021
Merged
@craig craig bot closed this as completed in 779f08b Dec 21, 2021
@craig craig bot closed this as completed in #72992 Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dhartunian dhartunian

Labels
A-logging In and around the logging infrastructure. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. regression Regression from a release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

util/log: fix redactability of logging tags knz/cockroach
3 participants
@knz @dhartunian @thtruo