Initialize http Request Header before RoundTrip to avoid panic #88064
Conversation
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
area/e2e-test-framework
|
/sig testing This is a Conformance test so would love for it to be fixed. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/assign @krzyzacy Please approve, 1 liner change, TY. |
|
If #88060 is merged, is this still necessary? |
|
/cc @oomichi |
|
@wongma7 Thanks for your explanation, I got the point. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: oomichi, wongma7 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
| @@ -1536,7 +1536,7 @@ func headersForConfig(c *restclient.Config, url *url.URL) (http.Header, error) { | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| if _, err := rt.RoundTrip(&http.Request{URL: url}); err != nil { | |||
| if _, err := rt.RoundTrip(&http.Request{Header: make(http.Header), URL: url}); err != nil { | |||
There was a problem hiding this comment.
Why doesn't this use http.NewRequest? are there other things wrong with this construction?
There was a problem hiding this comment.
I don't know, I guess the original author avoided a constructor to show that this is a "dummy" request whose only purpose is to figure out Headers.
Either way, I did the bare minimum change to fix my problem, but best practice would probably be to do
NewRequest("GET", url, nil).
So let me open another PR.
What type of PR is this?
/kind bug
What this PR does / why we need it: This test panics if authenticated with a credential plugin https://kubernetes.io/docs/reference/access-authn-authz/authentication/#client-go-credential-plugins due to a bug in the credential plugin code (see #88060 for details) where it can try to write to a nil map.
To avoid this, make sure a nil map is never passed to RoundTrip in case it's the exec RoundTripper.
I guess this function is deliberately leaving Headers unset because its purpose is to find out what headers are added to an http request by wrappers and such so that websocket requests can then include the same headers. Anyway it's better to initialize Headers to an empty map instead of leaving it nil.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: