refactor TLSconfig funcs

Signed-off-by: Soule BA <[email protected]>

Author: souleb

api/v1beta2/zz_generated.deepcopy.go

@@ -459,16 +459,8 @@ a deprecation warning will be logged.
 
 ### Cert secret reference
 
-<<<<<<< HEAD
-**Note:** TLS authentication is not yet supported by OCI Helm repositories.
-
 `.spec.certSecretRef.name` is an optional field to specify a secret containing TLS
 certificate data. The secret can contain the following keys:
-=======
-To provide TLS credentials to use while connecting with the Helm repository,
-the referenced Secret is expected to contain `.data.certFile` and
-`.data.keyFile`, and/or `.data.caFile` values.
->>>>>>> 3df4c49 (refactoring and fix tests)
 
 * `certFile` and `keyFile`, to specify the client certificate and private key used for
 TLS client authentication. These must be used in conjunction, i.e. specifying one without
@@ -515,28 +507,6 @@ data:
   caFile: <BASE64>
 ```
 
-#### Provide TLS credentials in a secret of type kubernetes.io/dockerconfigjson
-
-For OCI Helm repositories, Kubernetes secrets of type [kubernetes.io/dockerconfigjson](https://kubernetes.io/docs/concepts/configuration/secret/#secret-types)
-are also supported. It is possible to append TLS credentials to the secret data.
-
-For example:
-
-```yaml
-apiVersion: v1
-kind: Secret
-metadata:
-  name: example-tls
-  namespace: default
-type: kubernetes.io/dockerconfigjson
-data:
-  .dockerconfigjson: <BASE64>
-  certFile: <BASE64>
-  keyFile: <BASE64>
-  # NOTE: Can be supplied without the above values
-  caFile: <BASE64>
-```
-
 ### Pass credentials
 
 `.spec.passCredentials` is an optional field to allow the credentials from the

docs/spec/v1beta2/helmrepositories.md

@@ -512,7 +512,8 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 	if err != nil {
 		return chartRepoConfigErrorReturn(err, obj)
 	}
-	clientOpts, err := getter.GetClientOpts(ctxTimeout, r.Client, repo, normalizedURL)
+
+	clientOpts, certsTmpDir, err := getter.GetClientOpts(ctxTimeout, r.Client, repo, normalizedURL)
 	if err != nil && !errors.Is(err, getter.ErrDeprecatedTLSConfig) {
 		e := &serror.Event{
 			Err:    err,
@@ -521,6 +522,10 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
 		return sreconcile.ResultEmpty, e
 	}
+	if certsTmpDir != "" {
+		// this happens when the tls certs are stored in a temporary directory
+		defer os.RemoveAll(certsTmpDir)
+	}
 	getterOpts := clientOpts.GetterOpts
 
 	// Initialize the chart repository
@@ -536,7 +541,8 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 		// this is needed because otherwise the credentials are stored in ~/.docker/config.json.
 		// TODO@souleb: remove this once the registry move to Oras v2
 		// or rework to enable reusing credentials to avoid the unneccessary handshake operations
-		registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.RegLoginOpt != nil)
+		shouldLogin := len(clientOpts.RegLoginOpts) > 0 && clientOpts.RegLoginOpts[0] != nil
+		registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, shouldLogin)
 		if err != nil {
 			e := &serror.Event{
 				Err:    fmt.Errorf("failed to construct Helm client: %w", err),
@@ -585,8 +591,8 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 
 		// If login options are configured, use them to login to the registry
 		// The OCIGetter will later retrieve the stored credentials to pull the chart
-		if clientOpts.RegLoginOpt != nil {
-			err = ociChartRepo.Login(clientOpts.RegLoginOpt)
+		if len(clientOpts.RegLoginOpts) > 0 && clientOpts.RegLoginOpts[0] != nil {
+			err = ociChartRepo.Login(clientOpts.RegLoginOpts...)
 			if err != nil {
 				e := &serror.Event{
 					Err:    fmt.Errorf("failed to login to OCI registry: %w", err),
@@ -983,15 +989,20 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
 		ctxTimeout, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration)
 		defer cancel()
 
-		clientOpts, err := getter.GetClientOpts(ctxTimeout, r.Client, obj, normalizedURL)
+		clientOpts, certsTmpDir, err := getter.GetClientOpts(ctxTimeout, r.Client, obj, normalizedURL)
 		if err != nil && !errors.Is(err, getter.ErrDeprecatedTLSConfig) {
 			return nil, err
 		}
+		if certsTmpDir != "" {
+			// this happens when the tls certs are stored in a temporary directory
+			defer os.RemoveAll(certsTmpDir)
+		}
 		getterOpts := clientOpts.GetterOpts
 
 		var chartRepo repository.Downloader
 		if helmreg.IsOCI(normalizedURL) {
-			registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.RegLoginOpt != nil)
+			shouldLogin := len(clientOpts.RegLoginOpts) > 0 && clientOpts.RegLoginOpts[0] != nil
+			registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, shouldLogin)
 			if err != nil {
 				return nil, fmt.Errorf("failed to create registry client: %w", err)
 			}
@@ -1016,8 +1027,8 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
 
 			// If login options are configured, use them to login to the registry
 			// The OCIGetter will later retrieve the stored credentials to pull the chart
-			if clientOpts.RegLoginOpt != nil {
-				err = ociChartRepo.Login(clientOpts.RegLoginOpt)
+			if len(clientOpts.RegLoginOpts) > 0 && clientOpts.RegLoginOpts[0] != nil {
+				err = ociChartRepo.Login(clientOpts.RegLoginOpts...)
 				if err != nil {
 					errs = append(errs, fmt.Errorf("failed to login to OCI chart repository: %w", err))
 					// clean up the credentialsFile

internal/controller/helmchart_controller.go

@@ -2245,24 +2245,27 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 		registryOpts     registryOptions
 		secretOpts       secretOptions
 		secret           *corev1.Secret
-		withTLS          bool
+		certsecret       *corev1.Secret
+		insecure         bool
 		provider         string
 		providerImg      string
 		want             sreconcile.Result
 		wantErr          bool
 		assertConditions []metav1.Condition
 	}{
 		{
-			name: "HTTP without basic auth",
-			want: sreconcile.ResultSuccess,
+			name:     "HTTP without basic auth",
+			want:     sreconcile.ResultSuccess,
+			insecure: true,
 			assertConditions: []metav1.Condition{
 				*conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"),
 				*conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"),
 			},
 		},
 		{
-			name: "HTTP with basic auth secret",
-			want: sreconcile.ResultSuccess,
+			name:     "HTTP with basic auth secret",
+			want:     sreconcile.ResultSuccess,
+			insecure: true,
 			registryOpts: registryOptions{
 				withBasicAuth: true,
 			},
@@ -2283,9 +2286,10 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 			},
 		},
 		{
-			name:    "HTTP registry - basic auth with invalid secret",
-			want:    sreconcile.ResultEmpty,
-			wantErr: true,
+			name:     "HTTP registry - basic auth with invalid secret",
+			want:     sreconcile.ResultEmpty,
+			wantErr:  true,
+			insecure: true,
 			registryOpts: registryOptions{
 				withBasicAuth: true,
 			},
@@ -2307,6 +2311,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 		{
 			name:        "with contextual login provider",
 			wantErr:     true,
+			insecure:    true,
 			provider:    "aws",
 			providerImg: "oci://123456789000.dkr.ecr.us-east-2.amazonaws.com/test",
 			assertConditions: []metav1.Condition{
@@ -2319,6 +2324,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 			registryOpts: registryOptions{
 				withBasicAuth: true,
 			},
+			insecure: true,
 			secretOpts: secretOptions{
 				username: testRegistryUsername,
 				password: testRegistryPassword,
@@ -2340,9 +2346,9 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 			name:    "HTTPS With invalid CA cert",
 			wantErr: true,
 			registryOpts: registryOptions{
-				withTLS: true,
+				withTLS:            true,
+				withClientCertAuth: true,
 			},
-			withTLS: true,
 			secretOpts: secretOptions{
 				username: testRegistryUsername,
 				password: testRegistryPassword,
@@ -2352,21 +2358,27 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 					Name: "auth-secretref",
 				},
 				Type: corev1.SecretTypeDockerConfigJson,
+				Data: map[string][]byte{},
+			},
+			certsecret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "certs-secretref",
+				},
 				Data: map[string][]byte{
 					"caFile": []byte("invalid caFile"),
 				},
 			},
 			assertConditions: []metav1.Condition{
-				*conditions.TrueCondition(sourcev1.FetchFailedCondition, "Unknown", "unknown build error: failed to create TLS client config with secret data: cannot append certificate into certificate pool: invalid caFile"),
+				*conditions.TrueCondition(sourcev1.FetchFailedCondition, "Unknown", "unknown build error: failed to construct Helm client's TLS config: cannot append certificate into certificate pool: invalid caFile"),
 			},
 		},
 		{
 			name: "HTTPS With CA cert",
 			want: sreconcile.ResultSuccess,
 			registryOpts: registryOptions{
-				withTLS: true,
+				withTLS:            true,
+				withClientCertAuth: true,
 			},
-			withTLS: true,
 			secretOpts: secretOptions{
 				username: testRegistryUsername,
 				password: testRegistryPassword,
@@ -2376,10 +2388,16 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 					Name: "auth-secretref",
 				},
 				Type: corev1.SecretTypeDockerConfigJson,
+				Data: map[string][]byte{},
+			},
+			certsecret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "certs-secretref",
+				},
 				Data: map[string][]byte{
 					"caFile":   tlsCA,
-					"certFile": tlsPublicKey,
-					"keyFile":  tlsPrivateKey,
+					"certFile": clientPublicKey,
+					"keyFile":  clientPrivateKey,
 				},
 			},
 			assertConditions: []metav1.Condition{
@@ -2399,7 +2417,9 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 
 			workspaceDir := t.TempDir()
 
-			tt.registryOpts.disableDNSMocking = true
+			if tt.insecure {
+				tt.registryOpts.disableDNSMocking = true
+			}
 			server, err := setupRegistryServer(ctx, workspaceDir, tt.registryOpts)
 			g.Expect(err).NotTo(HaveOccurred())
 			t.Cleanup(func() {
@@ -2411,7 +2431,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 			g.Expect(err).ToNot(HaveOccurred())
 
 			// Upload the test chart
-			metadata, err := loadTestChartToOCI(chartData, server, "testdata/certs/server.pem", "testdata/certs/server-key.pem", "testdata/certs/ca.pem")
+			metadata, err := loadTestChartToOCI(chartData, server, "testdata/certs/client.pem", "testdata/certs/client-key.pem", "testdata/certs/ca.pem")
 			g.Expect(err).ToNot(HaveOccurred())
 
 			repo := &helmv1.HelmRepository{
@@ -2446,11 +2466,18 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 				repo.Spec.SecretRef = &meta.LocalObjectReference{
 					Name: tt.secret.Name,
 				}
-				clientBuilder.WithObjects(tt.secret, repo)
-			} else {
-				clientBuilder.WithObjects(repo)
+				clientBuilder.WithObjects(tt.secret)
+			}
+
+			if tt.certsecret != nil {
+				repo.Spec.CertSecretRef = &meta.LocalObjectReference{
+					Name: tt.certsecret.Name,
+				}
+				clientBuilder.WithObjects(tt.certsecret)
 			}
 
+			clientBuilder.WithObjects(repo)
+
 			obj := &helmv1.HelmChart{
 				ObjectMeta: metav1.ObjectMeta{
 					GenerateName: "auth-strategy-",
@@ -2761,18 +2788,18 @@ func loadTestChartToOCI(chartData []byte, server *registryClientTestServer, cert
 		helmreg.LoginOptBasicAuth(testRegistryUsername, testRegistryPassword),
 		helmreg.LoginOptTLSClientConfig(certFile, keyFile, cafile))
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to login to OCI registry: %w", err)
 	}
 	metadata, err := extractChartMeta(chartData)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to extract chart metadata: %w", err)
 	}
 
 	// Upload the test chart
 	ref := fmt.Sprintf("%s/testrepo/%s:%s", server.registryHost, metadata.Name, metadata.Version)
 	_, err = server.registryClient.Push(chartData, ref)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to push chart: %w", err)
 	}
 
 	return metadata, nil

internal/controller/helmchart_controller_test.go

@@ -399,7 +399,7 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc
 		return sreconcile.ResultEmpty, e
 	}
 
-	clientOpts, err := getter.GetClientOpts(ctx, r.Client, obj, normalizedURL)
+	clientOpts, _, err := getter.GetClientOpts(ctx, r.Client, obj, normalizedURL)
 	if err != nil {
 		if errors.Is(err, getter.ErrDeprecatedTLSConfig) {
 			ctrl.LoggerFrom(ctx).