🐛 Do not overwrite global http.DefaultClient TLSConfig #13058
Conversation
The current TLS configuration was overriding the TLSConfig for the global `http.DefaultClient`. This call is being used by controllers such as the `ExtensionConfig` controller which calls this function from multiple concurrent workers. This leads to a race where the TLS `ServerName` is configured differently to that of the URL it is trying to call and X509 validation fails. An example can be seen from the CAPI logs below: ``` E1126 12:43:22.449064 1 controller.go:347] "Reconciler error" err="failed to discover ExtensionConfig extension-config-a: failed to discover extension \"extension-config-a\": http call failed: Post \"https://extension-config-a-runtimehooks.extension-config-a-system.svc:443/hooks.runtime.cluster.x-k8s.io/v1alpha1/discovery?timeout=10s\": tls: failed to verify certificate: x509: certificate is valid for extension-config-a-runtimehooks.extension-config-a-system.svc, extension-config-a-runtimehooks.extension-config-a-system.svc.cluster.local, not extension-config-b.extension-config-b-system.svc" controller="extensionconfig" controllerGroup="runtime.cluster.x-k8s.io" controllerKind="ExtensionConfig" ExtensionConfig="extension-config-a" namespace="" name="extension-config-a" reconcileID="dfd00b69-3666-4818-b4a0-52eb1c391848" E1126 12:53:42.919995 1 controller.go:347] "Reconciler error" err="failed to discover ExtensionConfig extension-config-b: failed to discover extension \"extension-config-b\": http call failed: Post \"https://extension-config-b.extension-config-b-system.svc:443/hooks.runtime.cluster.x-k8s.io/v1alpha1/discovery?timeout=10s\": tls: failed to verify certificate: x509: certificate is valid for extension-config-b.extension-config-b-system.svc, extension-config-b.extension-config-b-system.svc.cluster.local, not extension-config-a-runtimehooks.extension-config-a-system.svc" controller="extensionconfig" controllerGroup="runtime.cluster.x-k8s.io" controllerKind="ExtensionConfig" ExtensionConfig="extension-config-b" namespace="" name="extension-config-b" reconcileID="4cc93a96-cfcf-49f8-8276-b3725fc8e1b8" ``` Notice how the URL and the expected hostname are swapped in each log indicating a race (TLSConfig being reconfigured in the middle of the call by different worker threads.
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
size/XS
|
/area clusterclass |
k8s-ci-robot
added
area/clusterclass
neolit123
left a comment
neolit123
left a comment
There was a problem hiding this comment.
LGTM, reusing the DefaultClient seems like a bug here.
/lgtm
/hold
/cc @chrischdi
PTAL for second review if possible.
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 6ce10d71fe63f6473fb69822fd1a88e49da8bc44
|
|
Thx! /lgtm |
|
/approve if there are follow-up concerns, let's revert. |
k8s-ci-robot
removed
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neolit123 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
|
@neolit123 Are you OK for me to backport this to release-1.12, release-1.11 and release-1.10? Are you still cutting upstream releases for those minors? |
|
/cherry-pick release-1.12 |
|
@jimmidyson: new pull request created: #13061 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@jimmidyson: new pull request created: #13062 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@jimmidyson: new pull request created: #13063 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
yes, releases are being cut for these. |
@neolit123 please give maintainers time to look into PRs |
|
@fabriziopandini Do you have concerns with this PR? Let me know so I can fix anything up 🙏 |
|
I will take a look at the change, but It would have been better if a mantainers can do this before PR are merged and in this case also backported. |
|
💯 , that would be great! We are close to the release and should be careful on the changes we do. |
Let me know once you've had a look and if it needs a different approach.
If that's the case, perhaps the approvers group needs to be trimmed accordingly? |
|
Note: this PR merged without running any E2E test that exercise the most of the runtime extension machinery, we should check periodic tomorrow. |
| client := &http.Client{} | ||
| defer client.CloseIdleConnections() |
There was a problem hiding this comment.
@jimmidyson with this change we are moving away from a single, long lived http.Client and starting using many short lived httpClients, which as far as I understand, it means to give up on all the optimisations that exists inside golang http client (and defer client.CloseIdleConnections() makes this very explicit)
I need some time to dig more into details, but I was wondering what is your take on this?
There was a problem hiding this comment.
Honestly I'm not too worried about it because from what I can see this client is not that heavily used.
However I can take a look and try to provide a custom transport instead that dynamically configures the TLSConfig instead of creating a new client each time.
There was a problem hiding this comment.
@fabriziopandini Pushed #13064 as an alternative implementation that fixes the same issue without creating a new http.Client every call, while avoiding reconfiguring the global http.DefaultClient.
There was a problem hiding this comment.
from what I can see this client is not that heavily used
This client is used by all the runtime extension calls; it is true that we did a few optimisations to reduce the number of calls, but it is definitely used in few controllers now.
However I can take a look and try to provide a custom transport instead that dynamically configures the TLSConfig instead of creating a new client each time.
Thanks. I will try to discuss with other maintainers what is the best solution here.
However, I also think that we should split the discussion in two parts:
- what to do with the change that was already backported (we are past rc.0 and we should not take any risk)
- what to do on main
There was a problem hiding this comment.
if #13064 is applied on top of all branches it will be exactly the same as it was before perf wise, but actually not having the bug of TLS config race..
and yes, i'd find it highly unlikely that a new client for each call would kick any bee hives.
There was a problem hiding this comment.
We need to fix the race one way or another. I can understand being rush averse at this stage of an RC, but we need to fix it.
There was a problem hiding this comment.
@jimmidyson BTW, i think something that is missing in the OP is the explanation of the issue at extension discovery.
i.e. what happens due to the TLS config race at scale.
There was a problem hiding this comment.
When this occurs the ExtensionConfigs become unreconciled and therefore ClusterClasses that reference them also become unreconcilable. Sorry I don't have resource histories showing this but the log I shared in the OP would trigger this.
We're hitting this with just 2 ExtensionConfigs and luckily for us so far this has recovered pretty quickly on requeue and thanks to jitter etc this does not materially affect cluster deployments. However, at scale with more ExtensionConfigs this race will be hit more often and could lead to worse outcomes.
I'll update the new PR with this detail as well as the original description from this PR.
7 participants
What this PR does / why we need it:
The current TLS configuration was overriding the TLSConfig for the global
http.DefaultClient. This call is being used by controllers such as theExtensionConfigcontroller which calls this function from multiple concurrent workers. This leads to a race where the TLSServerNameis configured differently to that of the URL it is trying to call and X509 validation fails. An example can be seen from the CAPI logs below:Notice how the URL and the expected hostname are swapped in each log indicating a race (TLSConfig being reconfigured in the middle of the call by different worker threads).
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #