Make credentialprovider less verbose about benign errors. #2674
Conversation
|
Can one of the admins verify this patch? |
|
@jbeda FYI |
| glog.V(2).Infof("body of failing http response: %v", resp.Body) | ||
| return nil, err | ||
| return nil, fmt.Errorf("http status code: %d while fetching url %s", resp.StatusCode, url) |
There was a problem hiding this comment.
please don't wrap this error. You are transforming an instrospectable error (e.g. with an accessible status code) into an opaque string.
There was a problem hiding this comment.
This isn't wrapping an error, do you mean the one above?
There was a problem hiding this comment.
I am fixing the two above, LMK if you want me to create something more structured to hold the fmt.Errorf from this line
There was a problem hiding this comment.
Can you at least TODO it, I've found this to be super painful upstream when I'm trying to figure out what went wrong...
There was a problem hiding this comment.
I added a TODO and will follow up with a PR to use a structured error.
There was a problem hiding this comment.
Implementing the error was quick enough, so I just did it.
LMK if you have any other feedback.
|
Small stuff. --brendan |
|
@brendandburns PTAL, I have pushed changes that avoid wrapping the errors. |
In particular, a few of the utilities used within the credentialprovider had the pattern:
glog.Errorf("while blah %s: %v", s, err)
return nil, err
This change propagates those error message and puts the burden of logging on the caller.
In particular, this allows us to squelch all output during kubelet startup when we are detecting whether certain credentialprovider plugins should even be enabled.
Fixes: #2673
|
LGTM, thanks |
Make credentialprovider less verbose about benign errors.
In particular, a few of the utilities used within the credentialprovider had the pattern:
This change composes the error message that is propagated and puts the burden of logging on the caller, which gives us the new pattern:
In particular, this allows us to squelch all output during kubelet startup when we are detecting whether certain credentialprovider plugins should even be enabled.
Fixes: #2673