Redact credentials from endpoint when logging#434
Conversation
gardener-prow
bot
added
kind/enhancement
|
|
|
Welcome @relusc! |
gardener-prow
bot
added
size/S
pkg/config/config_test.go
Outdated
| Expect(cfg).ToNot(BeNil()) | ||
|
|
||
| // Verify credentials were redacted | ||
| Expect(cfg.OTLPConfig.Endpoint).To(Equal("https://xxxxx@otel-collector.example.com:4317")) |
There was a problem hiding this comment.
Here the test wrongly checks the configuration value, where the intent is to return an obscured value from GetEndPoint() functions of the clients, which per se shall not be the case.
| } | ||
|
|
||
| // GetEndPoint returns the configured endpoint | ||
| func (c *OTLPHTTPClient) GetEndPoint() string { |
There was a problem hiding this comment.
If the intent is to print obscured logs, the correct place is where the logs are printed and not to modify the value at the GetEndPoint function.
There was a problem hiding this comment.
you are totally right :) Updated
pkg/client/noop_client.go
Outdated
| // GetEndPoint returns the configured endpoint | ||
| func (c *NoopClient) GetEndPoint() string { | ||
| return c.endpoint | ||
| // Redact possible credentials in endpoint URL |
There was a problem hiding this comment.
Same comment as otlp_httpclient.go
pkg/client/otlp_grpcclient.go
Outdated
| // GetEndPoint returns the configured endpoint | ||
| func (c *OTLPGRPCClient) GetEndPoint() string { | ||
| return c.endpoint | ||
| // Redact possible credentials in endpoint URL |
There was a problem hiding this comment.
Same comment as otlp_httpclient.go
pkg/client/stdout_client.go
Outdated
| // GetEndPoint returns the configured endpoint | ||
| func (c *StdoutClient) GetEndPoint() string { | ||
| return c.endpoint | ||
| // Redact possible credentials in endpoint URL |
There was a problem hiding this comment.
Same comment as otlp_httpclient.go
|
It is not correct to change the GetXXX method to return different than actual value. If the logs are required to be obscured it is better to change where the logs are printed. |
|
LGTM label has been added. DetailsGit tree hash: 462139814b79952c0740d545995fa5747c07aab6 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nickytd The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
gardener-prow
bot
added
the
approved
3 participants
How to categorize this PR?
/kind enhancement
/area logging
What this PR does / why we need it:
Which issue(s) this PR fixes:
No related issue, small change
Special notes for your reviewer:
After plugin creation and stopping, a success message is logged. This also prints out the configured endpoint. When having credentials in the endpoint URL, they are not redacted. This change replaces possible
...user:password...credentials in the endpoint URL when logging.Release note: