-
Notifications
You must be signed in to change notification settings - Fork 12
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
Redacting password while logging connection params in import data to target #2097
Conversation
@@ -431,7 +431,7 @@ func GetRedactedURLs(urlList []string) []string { | |||
for _, u := range urlList { | |||
obj, err := url.Parse(u) | |||
if err != nil { | |||
ErrExit("invalid URL: %q", u) |
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.
previously we were printing u
but can we now print err
?
can the error message also have the url in 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.
I think yes, an error can include the URL in some cases, as I see url.Parse()
is including the rawURL
in the error msg.
func Parse(rawURL string) (*URL, error) {
...
if err = url.setFragment(frag); err != nil {
return nil, &Error{"parse", rawURL, 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.
ok, what we can do here is instead of using ErrExit() -> log + console
We print different messages for log vs console.
console can have error message(containing url)
log should have a generic msg like you added
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 with a nit comment
@@ -431,7 +431,7 @@ func GetRedactedURLs(urlList []string) []string { | |||
for _, u := range urlList { | |||
obj, err := url.Parse(u) | |||
if err != nil { | |||
ErrExit("invalid URL: %q", u) |
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.
ok, what we can do here is instead of using ErrExit() -> log + console
We print different messages for log vs console.
console can have error message(containing url)
log should have a generic msg like you added
Describe the changes in this pull request
Fixes #2095
Describe if there are any user-facing changes
N/A
How was this pull request tested?
Manually, password is no more coming up in log
Does your PR have changes that can cause upgrade issues?