-
Notifications
You must be signed in to change notification settings - Fork 5k
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
fix: Fail gracefully when unable to unmarshal cluster secret #6427
Conversation
Signed-off-by: jannfis <jann@mistrust.net>
util/db/cluster.go
Outdated
clusterList.Items[i] = cluster | ||
cluster, err := secretToCluster(clusterSecret) | ||
if err != nil { | ||
log.Errorf("could not unmarshal cluster secret: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure (here as well as below) whether we should pass the value of err
to the Logger, since it may contain some confidential information?
Is the error content required for users to troubelshoot, or shall we just omit the details?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. The error might have token. Even if it is just part of a token still not good to log it. May be just print secret name e.g.:
log.Errorf("could not unmarshal cluster secret: %s", clusterSecret.Name)
util/db/cluster.go
Outdated
clusterList.Items[i] = cluster | ||
cluster, err := secretToCluster(clusterSecret) | ||
if err != nil { | ||
log.Errorf("could not unmarshal cluster secret: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. The error might have token. Even if it is just part of a token still not good to log it. May be just print secret name e.g.:
log.Errorf("could not unmarshal cluster secret: %s", clusterSecret.Name)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please resolve comment about error before merging
Signed-off-by: jannfis <jann@mistrust.net>
Codecov Report
@@ Coverage Diff @@
## master #6427 +/- ##
==========================================
+ Coverage 41.01% 41.02% +0.01%
==========================================
Files 152 152
Lines 20088 20131 +43
==========================================
+ Hits 8239 8259 +20
- Misses 10716 10733 +17
- Partials 1133 1139 +6
Continue to review full report at Codecov.
|
Signed-off-by: jannfis <jann@mistrust.net>
Signed-off-by: jannfis <jann@mistrust.net>
Correctly handle errors when failing to parse cluster secrets instead of emitting a
panic()
Fixes #6423
Signed-off-by: jannfis jann@mistrust.net
Note on DCO:
If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.
Checklist: