-
Notifications
You must be signed in to change notification settings - Fork 706
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
hide sensitive data for GetPackageRepoDetail and UpdatePackageRepository with kubeapps managed secrets #4652
hide sensitive data for GetPackageRepoDetail and UpdatePackageRepository with kubeapps managed secrets #4652
Conversation
…datePackageRepository/
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.
Thanks Greg
@@ -45,6 +45,7 @@ const ( | |||
var ( | |||
// default poll interval is 10 min | |||
defaultPollInterval = metav1.Duration{Duration: 10 * time.Minute} | |||
redactedString = "REDACTED" |
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.
This should be a const
rather than a package-global variable shouldn't it?
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.
👍
}), | ||
} | ||
if _, err := secretsInterface.Update(ctx, secret, metav1.UpdateOptions{}); err != nil { | ||
return statuserror.FromK8sError("update", "secrets", secret.Name, 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.
Nice - so the main change to this file is just the return type of validateUserManagedRepoSecret
and so that the arg to setOwnerReferencesForRepoSecret
can include the secret (and avoid an extra lookup?)
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.
Nice - so the main change to this file is just the return type of
validateUserManagedRepoSecret
and so that the arg tosetOwnerReferencesForRepoSecret
can include the secret (and avoid an extra lookup?)
Well, I would say the 'redacted' string handling, is the main change to this file. What you mention is just a small unrelated performance improvement, an icing on the cake, so to speak.
Thanks for the review
Greg
Documented behavior in the spec (under GetPackageRepositoryDetail
https://docs.google.com/document/d/1z03msZRGsRvcRaom-yrvEwIWcEfNy6fSA5Zg28gjYvA/edit#heading=h.ou8mya83exsw and UpdatePackageRepository https://docs.google.com/document/d/1z03msZRGsRvcRaom-yrvEwIWcEfNy6fSA5Zg28gjYvA/edit#heading=h.gd8t5sfz32li) as per agreement with team
Implemented according to spec in the flux plugin