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

[v21.11.x] cluster: clean up config error logging #5014

Merged
merged 1 commit into from
Jun 3, 2022
Merged

[v21.11.x] cluster: clean up config error logging #5014

merged 1 commit into from
Jun 3, 2022

Conversation

ajfabbri
Copy link
Contributor

@ajfabbri ajfabbri commented Jun 2, 2022

Backport from pull request #4942.

This one required manual editing.

@ajfabbri ajfabbri requested review from andrewhsu and removed request for a team June 2, 2022 23:25
@ajfabbri ajfabbri changed the base branch from dev to v21.11.x June 2, 2022 23:25
@ajfabbri
Copy link
Contributor Author

ajfabbri commented Jun 2, 2022 •
edited

Accidentally had dev as pr base initially. Doesn't seem like buildkite can get past this even with a rebuild. May need to close this PR and try again. Edit: Though the Rebuild button doesn't work, you can force-push a no-op commit change (i.e. git commit --amend and just save, then force push) and it will start over with the new base branch.

@ajfabbri ajfabbri closed this Jun 2, 2022
@ajfabbri ajfabbri reopened this Jun 2, 2022
cluster: clean up config error logging
2b70d9f 
There are a bunch of places we log config values on
error, which are currently safe for secrets (because
secrets are all freeform strings that cannot fail
validation), but future-proof these paths by using
format_raw to ensure that if we added validated+secret
fields in future they'd be redacted properly.

(cherry picked from commit b74e04a)

Conflicts:
    src/v/cluster/config_manager.cc
    src/v/redpanda/admin_server.cc
@ajfabbri ajfabbri added this to the v21.11.17 milestone Jun 2, 2022
@andrwng
Copy link
Contributor

andrwng commented Jun 3, 2022

For posterity, the conflict here is that in the original PR b74e04a, one of the things we do in admin_server.cc is validate that the changes we're going to apply are valid, and we log a message in case of errors. In this case, since we're not doing that validation, seems we're in the clear.

andrwng
andrwng approved these changes Jun 3, 2022
View reviewed changes
Copy link
Contributor

@andrwng andrwng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Might be good to get another look from Noah if he's around since it looks like he was a reviewer of some of the config management work.

dotnwat
dotnwat approved these changes Jun 3, 2022
View reviewed changes
Copy link
Member

@dotnwat dotnwat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks ok with respect to the conflicts that occurred.

my only concern would be that whatever search was performed to find the log points which needed an update for redaction purposes would need to be repeated in 21.11.x etc to take into account code churn etc...

@ajfabbri ajfabbri merged commit 9c3e63c into redpanda-data:v21.11.x Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dotnwat dotnwat dotnwat approved these changes

@andrwng andrwng andrwng approved these changes

@graphcareful graphcareful graphcareful approved these changes

@NyaliaLui NyaliaLui Awaiting requested review from NyaliaLui

@jcsp jcsp Awaiting requested review from jcsp

@andrewhsu andrewhsu Awaiting requested review from andrewhsu andrewhsu was automatically assigned from redpanda-data/devprod

Assignees
No one assigned
Labels
area/redpanda
Projects
None yet
Milestone
v21.11.17
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ajfabbri @andrwng @dotnwat @graphcareful