Merge pull request #389 from mhrabovcin/registry-client-idle-timeout

🐛 fix: add registry client TLS connection idle timeout

Author: k8s-ci-robot

internal/helm_client.go

@@ -18,10 +18,14 @@ package internal
 
 import (
 	"context"
+	"crypto/tls"
+	"crypto/x509"
 	"fmt"
+	"net/http"
 	"net/url"
 	"os"
 	"path"
+	"time"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/pkg/errors"
@@ -255,21 +259,68 @@ func (c *HelmClient) InstallHelmRelease(ctx context.Context, restConfig *rest.Co
 
 // newDefaultRegistryClient creates registry client object with default config which can be used to install/upgrade helm charts.
 func newDefaultRegistryClient(credentialsPath string, enableCache bool, caFilePath string, insecureSkipTLSVerify bool) (*registry.Client, error) {
-	if caFilePath == "" && !insecureSkipTLSVerify {
-		opts := []registry.ClientOption{
-			registry.ClientOptDebug(true),
-			registry.ClientOptEnableCache(enableCache),
-			registry.ClientOptWriter(os.Stderr),
+	opts := []registry.ClientOption{
+		registry.ClientOptDebug(true),
+		registry.ClientOptEnableCache(enableCache),
+		registry.ClientOptWriter(os.Stderr),
+	}
+	if credentialsPath != "" {
+		// Create a new registry client with credentials
+		opts = append(opts, registry.ClientOptCredentialsFile(credentialsPath))
+	}
+
+	if caFilePath != "" || insecureSkipTLSVerify {
+		tlsConf, err := newClientTLS(caFilePath, insecureSkipTLSVerify)
+		if err != nil {
+			return nil, fmt.Errorf("can't create TLS config for client: %w", err)
 		}
-		if credentialsPath != "" {
-			// Create a new registry client with credentials
-			opts = append(opts, registry.ClientOptCredentialsFile(credentialsPath))
+		opts = append(opts, registry.ClientOptHTTPClient(&http.Client{
+			Transport: &http.Transport{
+				TLSClientConfig: tlsConf,
+				Proxy:           http.ProxyFromEnvironment,
+				// This registry client is not reused and is discarded after a single reconciliation
+				// loop. Limit how long can be the idle connection open. Otherwise its possible that
+				// a registry server that keeps the connection open for a long time could result in
+				// connections being openend in the controller for a long time.
+				IdleConnTimeout: 1 * time.Second,
+			},
+		}),
+		)
+	}
+
+	return registry.NewClient(opts...)
+}
+
+// newClientTLS creates a new TLS config for the registry client based on the provided
+// CA file and insecureSkipTLSverify flag.
+func newClientTLS(caFile string, insecureSkipTLSverify bool) (*tls.Config, error) {
+	config := tls.Config{
+		InsecureSkipVerify: insecureSkipTLSverify,
+	}
+
+	if caFile != "" {
+		cp, err := certPoolFromFile(caFile)
+		if err != nil {
+			return nil, err
 		}
+		config.RootCAs = cp
+	}
 
-		return registry.NewClient(opts...)
+	return &config, nil
+}
+
+// certPoolFromFile creates a new CertPool and appends the certificates from the given file.
+func certPoolFromFile(filename string) (*x509.CertPool, error) {
+	b, err := os.ReadFile(filename)
+	if err != nil {
+		return nil, errors.Wrapf(err, "can't read CA file: %s", filename)
+	}
+	cp := x509.NewCertPool()
+	if !cp.AppendCertsFromPEM(b) {
+		return nil, errors.Errorf("failed to append certificates from file: %s", filename)
 	}
 
-	return registry.NewRegistryClientWithTLS(os.Stderr, "", "", caFilePath, insecureSkipTLSVerify, credentialsPath, true)
+	return cp, nil
 }
 
 // getHelmChartAndRepoName returns chartName, repoURL as per the format requirred in helm install/upgrade config.

internal/helm_client_test.go

@@ -0,0 +1,104 @@
+/*
+Copyright 2025 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package internal
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	. "github.com/onsi/gomega"
+	"github.com/onsi/gomega/types"
+)
+
+func TestNewDefaultRegistryClient(t *testing.T) {
+	t.Parallel()
+
+	emptyFilepath := filepath.Join(t.TempDir(), "empty-file")
+	NewWithT(t).Expect(os.Create(emptyFilepath)).Error().ShouldNot(HaveOccurred())
+
+	testCases := []struct {
+		name                  string
+		credentialsPath       string
+		enableCache           bool
+		caFilePath            string
+		insecureSkipTLSVerify bool
+		assertErr             types.GomegaMatcher
+		assertClient          types.GomegaMatcher
+	}{
+		{
+			name:                  "default config",
+			credentialsPath:       "",
+			enableCache:           true,
+			caFilePath:            "",
+			insecureSkipTLSVerify: false,
+			assertErr:             Not(HaveOccurred()),
+			assertClient:          Not(BeNil()),
+		},
+		{
+			name:                  "skip tls verification",
+			credentialsPath:       "",
+			enableCache:           true,
+			caFilePath:            "",
+			insecureSkipTLSVerify: true,
+			assertErr:             Not(HaveOccurred()),
+			assertClient:          Not(BeNil()),
+		},
+		{
+			name:                  "invalid ca bundle path",
+			credentialsPath:       "",
+			enableCache:           true,
+			caFilePath:            filepath.Join(t.TempDir(), "non-existing"),
+			insecureSkipTLSVerify: false,
+			assertErr:             MatchError(ContainSubstring("can't create TLS config")),
+			assertClient:          BeNil(),
+		},
+		{
+			name:                  "empty ca bundle file",
+			credentialsPath:       "",
+			enableCache:           true,
+			caFilePath:            emptyFilepath,
+			insecureSkipTLSVerify: false,
+			assertErr:             MatchError(ContainSubstring("failed to append certificates from file")),
+			assertClient:          BeNil(),
+		},
+		{
+			name:                  "valid ca bundle",
+			credentialsPath:       "",
+			enableCache:           true,
+			caFilePath:            "testdata/ca-bundle.pem",
+			insecureSkipTLSVerify: false,
+			assertErr:             Not(HaveOccurred()),
+			assertClient:          Not(BeNil()),
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			g := NewWithT(t)
+			client, err := newDefaultRegistryClient(
+				tc.credentialsPath,
+				tc.enableCache,
+				tc.caFilePath,
+				tc.insecureSkipTLSVerify,
+			)
+			g.Expect(err).To(tc.assertErr)
+			g.Expect(client).To(tc.assertClient)
+		})
+	}
+}

internal/testdata/ca-bundle.pem

@@ -0,0 +1,31 @@
+-----BEGIN CERTIFICATE-----
+MIIFazCCA1OgAwIBAgIRAIIQz7DSQONZRGPgu2OCiwAwDQYJKoZIhvcNAQELBQAw
+TzELMAkGA1UEBhMCVVMxKTAnBgNVBAoTIEludGVybmV0IFNlY3VyaXR5IFJlc2Vh
+cmNoIEdyb3VwMRUwEwYDVQQDEwxJU1JHIFJvb3QgWDEwHhcNMTUwNjA0MTEwNDM4
+WhcNMzUwNjA0MTEwNDM4WjBPMQswCQYDVQQGEwJVUzEpMCcGA1UEChMgSW50ZXJu
+ZXQgU2VjdXJpdHkgUmVzZWFyY2ggR3JvdXAxFTATBgNVBAMTDElTUkcgUm9vdCBY
+MTCCAiIwDQYJKoZIhvcNAQEBBQADggIPADCCAgoCggIBAK3oJHP0FDfzm54rVygc
+h77ct984kIxuPOZXoHj3dcKi/vVqbvYATyjb3miGbESTtrFj/RQSa78f0uoxmyF+
+0TM8ukj13Xnfs7j/EvEhmkvBioZxaUpmZmyPfjxwv60pIgbz5MDmgK7iS4+3mX6U
+A5/TR5d8mUgjU+g4rk8Kb4Mu0UlXjIB0ttov0DiNewNwIRt18jA8+o+u3dpjq+sW
+T8KOEUt+zwvo/7V3LvSye0rgTBIlDHCNAymg4VMk7BPZ7hm/ELNKjD+Jo2FR3qyH
+B5T0Y3HsLuJvW5iB4YlcNHlsdu87kGJ55tukmi8mxdAQ4Q7e2RCOFvu396j3x+UC
+B5iPNgiV5+I3lg02dZ77DnKxHZu8A/lJBdiB3QW0KtZB6awBdpUKD9jf1b0SHzUv
+KBds0pjBqAlkd25HN7rOrFleaJ1/ctaJxQZBKT5ZPt0m9STJEadao0xAH0ahmbWn
+OlFuhjuefXKnEgV4We0+UXgVCwOPjdAvBbI+e0ocS3MFEvzG6uBQE3xDk3SzynTn
+jh8BCNAw1FtxNrQHusEwMFxIt4I7mKZ9YIqioymCzLq9gwQbooMDQaHWBfEbwrbw
+qHyGO0aoSCqI3Haadr8faqU9GY/rOPNk3sgrDQoo//fb4hVC1CLQJ13hef4Y53CI
+rU7m2Ys6xt0nUW7/vGT1M0NPAgMBAAGjQjBAMA4GA1UdDwEB/wQEAwIBBjAPBgNV
+HRMBAf8EBTADAQH/MB0GA1UdDgQWBBR5tFnme7bl5AFzgAiIyBpY9umbbjANBgkq
+hkiG9w0BAQsFAAOCAgEAVR9YqbyyqFDQDLHYGmkgJykIrGF1XIpu+ILlaS/V9lZL
+ubhzEFnTIZd+50xx+7LSYK05qAvqFyFWhfFQDlnrzuBZ6brJFe+GnY+EgPbk6ZGQ
+3BebYhtF8GaV0nxvwuo77x/Py9auJ/GpsMiu/X1+mvoiBOv/2X/qkSsisRcOj/KK
+NFtY2PwByVS5uCbMiogziUwthDyC3+6WVwW6LLv3xLfHTjuCvjHIInNzktHCgKQ5
+ORAzI4JMPJ+GslWYHb4phowim57iaztXOoJwTdwJx4nLCgdNbOhdjsnvzqvHu7Ur
+TkXWStAmzOVyyghqpZXjFaH3pO3JLF+l+/+sKAIuvtd7u+Nxe5AW0wdeRlN8NwdC
+jNPElpzVmbUq4JUagEiuTDkHzsxHpFKVK7q4+63SM1N95R1NbdWhscdCb+ZAJzVc
+oyi3B43njTOQ5yOf+1CceWxG1bQVs5ZufpsMljq4Ui0/1lvh+wjChP4kqKOJ2qxq
+4RgqsahDYVvTH9w7jXbyLeiNdd8XM2w9U/t7y0Ff/9yi0GE44Za4rF2LN9d11TPA
+mRGunUHBcnWEvgJBQl9nJEiU0Zsnvgc/ubhPgXRR4Xq37Z0j4r7g1SgEEzwxA57d
+emyPxgcYxn/eR44/KJ4EBs+lVDR3veyJm+kXQ99b21/+jh5Xos1AnX5iItreGCc=
+-----END CERTIFICATE-----