🐛 fix: add registry client TLS connection idle timeout #389

mhrabovcin · 2025-04-17T16:00:33Z

What this PR does / why we need it:
Currently the controller runtime creates a new registry client for interaction with OCI registries when HelmReleaseProxy or HelmChartProxy is reconciled. The client is not being reused and is discarded after the reconciliation loop for a single item exits. When a TLS configuration is provided in k8s resources spec the controller uses helm library NewRegistryClientWithTLS function, that creates a http.Client and http.Transport on for each registry client instance.

This causes a problem with registry servers that won't close the http connection. The http.Transport implementation will keep the connection open and ready for re-use. This can result in many leaked goroutines with TCP connections opened in memory with no chance of reusing those.

In our tests when the controller was handling 100s HelmReleaseProxy and HelmChartProxy objects the memory consumption was high, and the controller hit a limit of maximum number of ephemeral TCP ports for outgoing TCP connections.

$ ss -s
Total: 28236
TCP:   29500 (estab 28233, closed 1264, orphaned 0, timewait 9)

Transport Total     IP        IPv6
RAW	  0         0         0
UDP	  0         0         0
TCP	  28236     28233     3
INET	  28236     28233     3
FRAG	  0         0         0

By adding the IdleConnTimeout the TCP connections are correctly closed by the client and goroutines are cleaned up. Example:

$ ss -s
Total: 7
TCP:   1277 (estab 3, closed 1270, orphaned 0, timewait 8)

Transport Total     IP        IPv6
RAW       0         0         0
UDP       0         0         0
TCP       7         2         5
INET      7         2         5
FRAG      0         0         0

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 #

k8s-ci-robot · 2025-04-17T16:00:48Z

Welcome @mhrabovcin!

It looks like this is your first PR to kubernetes-sigs/cluster-api-addon-provider-helm 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-addon-provider-helm has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-04-17T16:00:49Z

Hi @mhrabovcin. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

tuxtof · 2025-04-17T16:04:09Z

/ok-to-test

Jont828

Changes seem reasonable! Do you think it would be feasible to add some unit tests given that we have quite a few more functions and if/else branches?

internal/helm_client.go

mhrabovcin · 2025-04-19T13:49:24Z

Thanks for the review @Jont828 . I've added unit tests for the refactored function:

sigs.k8s.io/cluster-api-addon-provider-helm/internal/helm_client.go:261:                newDefaultRegistryClient                88.9%
sigs.k8s.io/cluster-api-addon-provider-helm/internal/helm_client.go:293:                newClientTLS                            100.0%
sigs.k8s.io/cluster-api-addon-provider-helm/internal/helm_client.go:309:                certPoolFromFile                        100.0%

Jont828

Just a few minor comments. After that, please squash commits and we will be good to go!

Jont828 · 2025-04-28T23:07:11Z

internal/helm_client.go

+	return registry.NewClient(opts...)
+}
+
+func newClientTLS(caFile string, insecureSkipTLSverify bool) (*tls.Config, error) {


Could you add a doc comment for this function?

Jont828 · 2025-04-28T23:07:28Z

internal/helm_client.go

+	return &config, nil
+}
+
+func certPoolFromFile(filename string) (*x509.CertPool, error) {


Could you also add a doc comment for this as well?

Jont828 · 2025-04-28T23:08:36Z

internal/helm_client.go

+func certPoolFromFile(filename string) (*x509.CertPool, error) {
+	b, err := os.ReadFile(filename)
+	if err != nil {
+		return nil, errors.Errorf("can't read CA file: %v", filename)


Suggested change

return nil, errors.Errorf("can't read CA file: %v", filename)

return nil, errors.Wrapf(err, "...")

Just so we don't lose the original error message

Jont828 · 2025-04-29T19:32:23Z

Thanks for working on this!

/lgtm
/approve

k8s-ci-robot · 2025-04-29T19:32:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jont828, mhrabovcin

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:

~~OWNERS~~ [Jont828]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from jackfrancis and Jont828 April 17, 2025 16:00

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 17, 2025

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 17, 2025

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 17, 2025

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 17, 2025

mhrabovcin changed the title ~~fix: add registry client TLS connection idle timeout 🐛~~ 🐛 fix: add registry client TLS connection idle timeout Apr 17, 2025

Jont828 reviewed Apr 18, 2025

View reviewed changes

internal/helm_client.go Show resolved Hide resolved

mhrabovcin force-pushed the registry-client-idle-timeout branch from 18fc46c to 83da56f Compare April 19, 2025 13:42

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 19, 2025

mhrabovcin force-pushed the registry-client-idle-timeout branch from 34852b7 to 85b8c39 Compare April 19, 2025 13:44

mhrabovcin requested a review from Jont828 April 23, 2025 09:58

Jont828 reviewed Apr 28, 2025

View reviewed changes

mhrabovcin force-pushed the registry-client-idle-timeout branch from e7a2929 to 4efe7c4 Compare April 29, 2025 09:07

fix: add registry client TLS connection idle timeout

6432b3f

mhrabovcin force-pushed the registry-client-idle-timeout branch from 4efe7c4 to 6432b3f Compare April 29, 2025 11:40

k8s-ci-robot assigned Jont828 Apr 29, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 29, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 29, 2025

k8s-ci-robot merged commit 7fa1379 into kubernetes-sigs:main Apr 29, 2025
11 checks passed

mhrabovcin deleted the registry-client-idle-timeout branch April 30, 2025 08:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 fix: add registry client TLS connection idle timeout #389

🐛 fix: add registry client TLS connection idle timeout #389

mhrabovcin commented Apr 17, 2025

k8s-ci-robot commented Apr 17, 2025

k8s-ci-robot commented Apr 17, 2025

tuxtof commented Apr 17, 2025

Jont828 left a comment

mhrabovcin commented Apr 19, 2025

Jont828 left a comment

Jont828 Apr 28, 2025

Jont828 Apr 28, 2025

Jont828 Apr 28, 2025

Jont828 commented Apr 29, 2025

k8s-ci-robot commented Apr 29, 2025

	return nil, errors.Errorf("can't read CA file: %v", filename)
	return nil, errors.Wrapf(err, "...")

🐛 fix: add registry client TLS connection idle timeout #389

🐛 fix: add registry client TLS connection idle timeout #389

Conversation

mhrabovcin commented Apr 17, 2025

k8s-ci-robot commented Apr 17, 2025

k8s-ci-robot commented Apr 17, 2025

tuxtof commented Apr 17, 2025

Jont828 left a comment

Choose a reason for hiding this comment

mhrabovcin commented Apr 19, 2025

Jont828 left a comment

Choose a reason for hiding this comment

Jont828 Apr 28, 2025

Choose a reason for hiding this comment

Jont828 Apr 28, 2025

Choose a reason for hiding this comment

Jont828 Apr 28, 2025

Choose a reason for hiding this comment

Jont828 commented Apr 29, 2025

k8s-ci-robot commented Apr 29, 2025