strip proxy credentials from proxy env variables when logging them #13751
Conversation
|
[test] |
|
[testextended][extended:core(builds)] |
|
@openshift/devex ptal |
| newEnv := make([]string, len(env)) | ||
| copy(newEnv, env) | ||
| for i, val := range newEnv { | ||
| newEnv[i] = re.ReplaceAllString(val, "$1$2") |
There was a problem hiding this comment.
Would it be better, and possibly more consistent to use the net/url package to parse the url and pick out the parts that you want to use? https://golang.org/pkg/net/url/
There was a problem hiding this comment.
yeah, that would be better. i'll redo the s2i change and update this pull.
bparees
changed the title
bparees
changed the title
|
turns out we log this information inadvertently in a lot of different places. working on it. |
bparees
force-pushed
the
clear_proxy
branch
10 times, most recently
from
f7e82d4 to
183f9c0
Compare
bparees
changed the title
Seems similar to #13799 - test framework issue? |
|
flake #13814 |
|
@openshift/devex ptal |
|
flake #13067 |
|
flake #13067 [test] |
|
[test] |
openshift-bot
added
the
needs-rebase
|
as half-discussed on IRC, my recommendations would be:
|
openshift-bot
added
needs-rebase
bparees
force-pushed
the
clear_proxy
branch
2 times, most recently
from
81c42b8 to
41bb56e
Compare
openshift-bot
removed
the
needs-rebase
bparees
force-pushed
the
clear_proxy
branch
2 times, most recently
from
0636040 to
3c9db32
Compare
|
@jim-minter ptal. |
|
flake #14122 |
| if err != nil { | ||
| return nil, fmt.Errorf("unable to serialize build: %v", err) | ||
| } | ||
| glog.V(4).Infof("redacted build: %v", string(bytes)) |
There was a problem hiding this comment.
Am I right in thinking the reason you're doing this is that you want to log the (redacted) nested detail beyond what printf/%#v provides on cfg.build?
If so, it makes sense; it's just a shame that %#v doesn't go far enough, because it would be simpler.
There was a problem hiding this comment.
yeah, the problem is printf %#v doesn't dereference nested pointers. I also tried using spew instead, but it also didn't work for me so i punted and did this. The nice part about doing it this way is the behavior should be exactly the same as it was before in terms of the format of the output.
pkg/build/util/util.go
Outdated
| copy(newEnv, env) | ||
| proxyRegex := regexp.MustCompile("(?i).*proxy.*") | ||
| for i, env := range newEnv { | ||
| if proxyRegex.MatchString(env.Name) { |
There was a problem hiding this comment.
(fwiw, or strings.Contains(strings.ToLower(env.Name), "proxy") - throughout patch)
There was a problem hiding this comment.
fair, but this implementation will be easier to extend in the future if we decide there are other keys we want to redact (just expand the regex), so i'd rather not change it at this point.
pkg/build/util/util_test.go
Outdated
| @@ -84,3 +90,219 @@ func TestTrustedMergeEnvWithoutDuplicates(t *testing.T) { | |||
| } | |||
|
|
|||
| } | |||
|
|
|||
| var credsRegex = regexp.MustCompile(".*user:password.*") | |||
| var redactedRegex = regexp.MustCompile(".*redacted*") | |||
There was a problem hiding this comment.
missing '.' - should be (".*redacted.*"), or better strings.Contains(s, "redacted") throughout - leading and trailing .* are superfluous.
|
bar the nit of the '.' in the regex, lgtm |
|
.*s cleaned up. |
|
Evaluated for origin testextended up to fb9a4bd |
|
lgtm |
|
continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/363/) (Base Commit: 31caa0d) (Extended Tests: core(builds)) |
|
Evaluated for origin test up to fb9a4bd |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1316/) (Base Commit: f714687) |
|
[merge] |
|
Evaluated for origin merge up to fb9a4bd |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/604/) (Base Commit: 5b5f19b) (Image: devenv-rhel7_6225) |
fix for https://bugzilla.redhat.com/show_bug.cgi?id=1366795