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.

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

sql,ast: better redact support for statements #55862

Open
ajwerner opened this issue Oct 22, 2020 · 2 comments
Open

sql,ast: better redact support for statements #55862

ajwerner opened this issue Oct 22, 2020 · 2 comments
Labels
A-observability-inf C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-observability

Comments

@ajwerner
Copy link
Contributor

ajwerner commented Oct 22, 2020

Is your feature request related to a problem? Please describe.

Currently the cockroach code generates an anonymized string for statements by formatting the AST to hide all constants. This ends up preserving relation names but losing information about columns making it hard to unpack the structure of a statement.

updatedPrivileges = sql.CreateInheritedPrivilegesFromDBDesc(parentDB, user)

Another issue is that we pass this anonymized statement around as a string. That means that if we log it or include it in an error without remembering to wrap it in redact.Safe or log.Safe then it will be redacted. Ideally we'd be able to get nice redacted statement structure that might even be able to preserve more of the structure than we do today.

Describe the solution you'd like

I see several options with some tradeoffs. We may want to do all of them. The details are intentionally vague.

  1. Implement redact.SafeMessage or something like it on all of the ast statements (or even add it to the interface). This would eliminate the need to call log.Safe but doesn't make the story around constants any better.
  2. Implement some middleware SafePrinter that can remap constants to hashes as appropriate or something like that
  3. Implement a more stateful SafePrinter that might have access to column identifiers and table identifiers. This might be better served as an AST transformation pre-processing step rather than as something that happens during formatting.

Epic: CRDB-6668

Jira issue: CRDB-3621

@blathers-crl
Copy link

blathers-crl bot commented Oct 22, 2020

Hi @ajwerner, I've guessed the C-ategory of your issue and suitably labeled it. Please re-label if inaccurate.

While you're here, please consider adding an A- label to help keep our repository tidy.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Oct 22, 2020
@github-actions
Copy link

github-actions bot commented Sep 6, 2023

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
10 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-observability-inf C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-observability
Projects
None yet
Development

No branches or pull requests

1 participant