kubeadm: Utilize transport defaults from API machinery for http calls inside kubeadm #53895
Conversation
k8s-ci-robot
added
release-note
|
/retest |
| @@ -29,7 +30,8 @@ import ( | |||
| // securely to the API Server using the provided CA cert and | |||
| // optionally refreshes the cluster-info information from the cluster-info ConfigMap | |||
| func RetrieveValidatedClusterInfo(httpsURL string) (*clientcmdapi.Cluster, error) { | |||
| response, err := http.Get(httpsURL) | |||
| client := &http.Client{Transport: netutil.SetOldTransportDefaults(&http.Transport{})} | |||
There was a problem hiding this comment.
SetTransportDefaults does http2 initialization, which in most cases here is not expected.
Even SetOldTransportDefaults does a bit more than expected, but setting defaults for Dialer and TLSHandshakeTimeout is safe enough to not be worried about.
|
could have a kubeadm default client constructed using the cidr-supporting no-proxy rather than constructing one-off clients all over the place also, is there a reason not to use lgtm otherwise |
|
@liggitt regarding sharing one client: we have in places like etcd connection different settings for client applied (tls, timeouts). In all other places, I think it is better for now keep as it is, rather than introduce global variable for kubeadm that would keep once constructed client. |
k8s-github-robot
added
the
needs-rebase
Default Go HTTP transport does not allow to use CIDR notations in NO_PROXY variables, thus for certain HTTP calls that is done inside kubeadm user needs to put explicitly multiple IP addresses. For most of calls done via API machinery it is get solved by setting different Proxy resolver. This patch allows to use CIDR notations in NO_PROXY variables for currently all other HTTP calls that is made inside kubeadm.
kad
force-pushed
the
kubeadm-proxy-transports
branch
from
348d9b4 to
4bd692a
Compare
k8s-github-robot
removed
the
needs-rebase
|
@liggitt deferring this to your assessment |
|
@liggitt ping |
|
I think construction of ad-hoc clients throughout kubeadm, and use of SetOldTransportDefaults should be revisited, but this is a strict improvement over what currently exists /lgtm |
k8s-ci-robot
added
the
lgtm
|
/test all Tests are more than 96 hours old. Re-running tests. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kad, liggitt, timothysc Associated issue: #324 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-ci-robot
added
the
approved
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
Default Go HTTP transport does not allow to use CIDR notations in
NO_PROXY variables, thus for certain HTTP calls that is done inside
kubeadm user needs to put explicitly multiple IP addresses. For most of
calls done via API machinery it is get solved by setting different Proxy
resolver. This patch allows to use CIDR notations in NO_PROXY variables
for currently all other HTTP calls that is made inside kubeadm.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes kubernetes/kubeadm#324Special notes for your reviewer:
Based on discussion in #52788, replacing this patch replacing all calls inside kubeadm that are done via DefaultTransport to explicitly defined and initialized with API machinery defaults Transport and http client.
Release note: