helm: add support for specifying TLS auth via .spec.certSecretRef

Add support for specifying TLS auth data via `.spec.certSecretRef` in HelmRepository and emit a deprecation event if TLS is configured via `.spec.secretRef`. Introduce (and refactor) Helm client builder and auth helpers to reduce duplicated code and increase uniformity and testability. Signed-off-by: Sanskar Jaiswal <[email protected]>
fluxcd · Jul 20, 2023 · 87aa433 · 87aa433
1 parent b2da5c4
commit 87aa433
Show file tree

Hide file tree

Showing 9 changed files with 383 additions and 227 deletions.
diff --git a/internal/controller/helmchart_controller.go b/internal/controller/helmchart_controller.go
@@ -54,7 +54,6 @@ import (
 	eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1"
 	"github.com/fluxcd/pkg/apis/meta"
 	"github.com/fluxcd/pkg/git"
-	"github.com/fluxcd/pkg/oci"
 	"github.com/fluxcd/pkg/runtime/conditions"
 	helper "github.com/fluxcd/pkg/runtime/controller"
 	"github.com/fluxcd/pkg/runtime/patch"
@@ -68,7 +67,6 @@ import (
 	serror "github.com/fluxcd/source-controller/internal/error"
 	"github.com/fluxcd/source-controller/internal/helm/chart"
 	"github.com/fluxcd/source-controller/internal/helm/getter"
-	"github.com/fluxcd/source-controller/internal/helm/registry"
 	"github.com/fluxcd/source-controller/internal/helm/repository"
 	soci "github.com/fluxcd/source-controller/internal/oci"
 	sreconcile "github.com/fluxcd/source-controller/internal/reconcile"
@@ -506,11 +504,6 @@ func (r *HelmChartReconciler) reconcileSource(ctx context.Context, sp *patch.Ser
 // object, and returns early.
 func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *helmv1.HelmChart,
 	repo *helmv1.HelmRepository, b *chart.Build) (sreconcile.Result, error) {
-	var (
-		tlsConfig     *tls.Config
-		authenticator authn.Authenticator
-		keychain      authn.Keychain
-	)
 	// Used to login with the repository declared provider
 	ctxTimeout, cancel := context.WithTimeout(ctx, repo.Spec.Timeout.Duration)
 	defer cancel()
@@ -519,64 +512,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 	if err != nil {
 		return chartRepoConfigErrorReturn(err, obj)
 	}
-	// Construct the Getter options from the HelmRepository data
-	clientOpts := []helmgetter.Option{
-		helmgetter.WithURL(normalizedURL),
-		helmgetter.WithTimeout(repo.Spec.Timeout.Duration),
-		helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
-	}
-	if secret, err := r.getHelmRepositorySecret(ctx, repo); secret != nil || err != nil {
-		if err != nil {
-			e := &serror.Event{
-				Err:    fmt.Errorf("failed to get secret '%s': %w", repo.Spec.SecretRef.Name, err),
-				Reason: sourcev1.AuthenticationFailedReason,
-			}
-			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
-			// Return error as the world as observed may change
-			return sreconcile.ResultEmpty, e
-		}
-
-		// Build client options from secret
-		opts, tlsCfg, err := r.clientOptionsFromSecret(secret, normalizedURL)
-		if err != nil {
-			e := &serror.Event{
-				Err:    err,
-				Reason: sourcev1.AuthenticationFailedReason,
-			}
-			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
-			// Requeue as content of secret might change
-			return sreconcile.ResultEmpty, e
-		}
-		clientOpts = append(clientOpts, opts...)
-		tlsConfig = tlsCfg
-
-		// Build registryClient options from secret
-		keychain, err = registry.LoginOptionFromSecret(normalizedURL, *secret)
-		if err != nil {
-			e := &serror.Event{
-				Err:    fmt.Errorf("failed to configure Helm client with secret data: %w", err),
-				Reason: sourcev1.AuthenticationFailedReason,
-			}
-			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
-			// Requeue as content of secret might change
-			return sreconcile.ResultEmpty, e
-		}
-	} else if repo.Spec.Provider != helmv1.GenericOCIProvider && repo.Spec.Type == helmv1.HelmRepositoryTypeOCI {
-		auth, authErr := oidcAuth(ctxTimeout, repo.Spec.URL, repo.Spec.Provider)
-		if authErr != nil && !errors.Is(authErr, oci.ErrUnconfiguredProvider) {
-			e := &serror.Event{
-				Err:    fmt.Errorf("failed to get credential from %s: %w", repo.Spec.Provider, authErr),
-				Reason: sourcev1.AuthenticationFailedReason,
-			}
-			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
-			return sreconcile.ResultEmpty, e
-		}
-		if auth != nil {
-			authenticator = auth
-		}
-	}
-
-	loginOpt, err := makeLoginOption(authenticator, keychain, normalizedURL)
+	hcOpts, err := repository.GetHelmClientOpts(ctxTimeout, r.Client, repo, normalizedURL, true)
 	if err != nil {
 		e := &serror.Event{
 			Err:    err,
@@ -585,6 +521,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
 		return sreconcile.ResultEmpty, e
 	}
+	clientOpts := hcOpts.GetterOpts
 
 	// Initialize the chart repository
 	var chartRepo repository.Downloader
@@ -599,7 +536,7 @@ 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(loginOpt != nil)
+		registryClient, credentialsFile, err := r.RegistryClientGenerator(hcOpts.LoginOpt != nil)
 		if err != nil {
 			e := &serror.Event{
 				Err:    fmt.Errorf("failed to construct Helm client: %w", err),
@@ -621,7 +558,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 		var verifiers []soci.Verifier
 		if obj.Spec.Verify != nil {
 			provider := obj.Spec.Verify.Provider
-			verifiers, err = r.makeVerifiers(ctx, obj, authenticator, keychain)
+			verifiers, err = r.makeVerifiers(ctx, obj, hcOpts.Authenticator, hcOpts.Keychain)
 			if err != nil {
 				if obj.Spec.Verify.SecretRef == nil {
 					provider = fmt.Sprintf("%s keyless", provider)
@@ -645,12 +582,11 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 		if err != nil {
 			return chartRepoConfigErrorReturn(err, obj)
 		}
-		chartRepo = ociChartRepo
 
 		// 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 loginOpt != nil {
-			err = ociChartRepo.Login(loginOpt)
+		if hcOpts.LoginOpt != nil {
+			err = ociChartRepo.Login(hcOpts.LoginOpt)
 			if err != nil {
 				e := &serror.Event{
 					Err:    fmt.Errorf("failed to login to OCI registry: %w", err),
@@ -660,8 +596,9 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 				return sreconcile.ResultEmpty, e
 			}
 		}
+		chartRepo = ociChartRepo
 	default:
-		httpChartRepo, err := repository.NewChartRepository(normalizedURL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, tlsConfig, clientOpts...)
+		httpChartRepo, err := repository.NewChartRepository(normalizedURL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, hcOpts.TlsConfig, clientOpts...)
 		if err != nil {
 			return chartRepoConfigErrorReturn(err, obj)
 		}
@@ -1024,12 +961,6 @@ func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *helmv1.He
 // The callback returns an object with a state, so the caller has to do the necessary cleanup.
 func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Context, name, namespace string) chart.GetChartDownloaderCallback {
 	return func(url string) (repository.Downloader, error) {
-		var (
-			tlsConfig     *tls.Config
-			authenticator authn.Authenticator
-			keychain      authn.Keychain
-		)
-
 		normalizedURL, err := repository.NormalizeURL(url)
 		if err != nil {
 			return nil, err
@@ -1047,55 +978,26 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
 				},
 			}
 		}
+		objKey := types.NamespacedName{
+			Name:      obj.Name,
+			Namespace: obj.Namespace,
+		}
 
 		// Used to login with the repository declared provider
 		ctxTimeout, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration)
 		defer cancel()
 
-		clientOpts := []helmgetter.Option{
-			helmgetter.WithURL(normalizedURL),
-			helmgetter.WithTimeout(obj.Spec.Timeout.Duration),
-			helmgetter.WithPassCredentialsAll(obj.Spec.PassCredentials),
-		}
-		if secret, err := r.getHelmRepositorySecret(ctx, obj); secret != nil || err != nil {
-			if err != nil {
-				return nil, err
-			}
-
-			// Build client options from secret
-			opts, tlsCfg, err := r.clientOptionsFromSecret(secret, normalizedURL)
-			if err != nil {
-				return nil, err
-			}
-			clientOpts = append(clientOpts, opts...)
-			tlsConfig = tlsCfg
-
-			// Build registryClient options from secret
-			keychain, err = registry.LoginOptionFromSecret(normalizedURL, *secret)
-			if err != nil {
-				return nil, fmt.Errorf("failed to create login options for HelmRepository '%s': %w", obj.Name, err)
-			}
-
-		} else if obj.Spec.Provider != helmv1.GenericOCIProvider && obj.Spec.Type == helmv1.HelmRepositoryTypeOCI {
-			auth, authErr := oidcAuth(ctxTimeout, obj.Spec.URL, obj.Spec.Provider)
-			if authErr != nil && !errors.Is(authErr, oci.ErrUnconfiguredProvider) {
-				return nil, fmt.Errorf("failed to get credential from %s: %w", obj.Spec.Provider, authErr)
-			}
-			if auth != nil {
-				authenticator = auth
-			}
-		}
-
-		loginOpt, err := makeLoginOption(authenticator, keychain, normalizedURL)
+		hcOpts, err := repository.GetHelmClientOpts(ctxTimeout, r.Client, obj, normalizedURL, true)
 		if err != nil {
 			return nil, err
 		}
+		clientOpts := hcOpts.GetterOpts
 
 		var chartRepo repository.Downloader
 		if helmreg.IsOCI(normalizedURL) {
-			registryClient, credentialsFile, err := r.RegistryClientGenerator(loginOpt != nil)
+			registryClient, credentialsFile, err := r.RegistryClientGenerator(hcOpts.LoginOpt != nil)
 			if err != nil {
-				return nil, fmt.Errorf("failed to create registry client for HelmRepository '%s': %w", obj.Name, err)
+				return nil, fmt.Errorf("failed to create registry client for HelmRepository '%s': %w", objKey.String(), err)
 			}
 
 			var errs []error
@@ -1106,7 +1008,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
 				repository.WithOCIRegistryClient(registryClient),
 				repository.WithCredentialsFile(credentialsFile))
 			if err != nil {
-				errs = append(errs, fmt.Errorf("failed to create OCI chart repository for HelmRepository '%s': %w", obj.Name, err))
+				errs = append(errs, fmt.Errorf("failed to create OCI chart repository for HelmRepository '%s': %w", objKey.String(), err))
 				// clean up the credentialsFile
 				if credentialsFile != "" {
 					if err := os.Remove(credentialsFile); err != nil {
@@ -1118,10 +1020,10 @@ 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 loginOpt != nil {
-				err = ociChartRepo.Login(loginOpt)
+			if hcOpts.LoginOpt != nil {
+				err = ociChartRepo.Login(hcOpts.LoginOpt)
 				if err != nil {
-					errs = append(errs, fmt.Errorf("failed to login to OCI chart repository for HelmRepository '%s': %w", obj.Name, err))
+					errs = append(errs, fmt.Errorf("failed to login to OCI chart repository for HelmRepository '%s': %w", objKey.String(), err))
 					// clean up the credentialsFile
 					errs = append(errs, ociChartRepo.Clear())
 					return nil, kerrors.NewAggregate(errs)
@@ -1130,7 +1032,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
 
 			chartRepo = ociChartRepo
 		} else {
-			httpChartRepo, err := repository.NewChartRepository(normalizedURL, "", r.Getters, tlsConfig, clientOpts...)
+			httpChartRepo, err := repository.NewChartRepository(normalizedURL, "", r.Getters, hcOpts.TlsConfig, clientOpts...)
 			if err != nil {
 				return nil, err
 			}

diff --git a/internal/controller/helmchart_controller_test.go b/internal/controller/helmchart_controller_test.go
@@ -922,12 +922,12 @@ func TestHelmChartReconciler_buildFromHelmRepository(t *testing.T) {
 				}
 			},
 			want:    sreconcile.ResultEmpty,
-			wantErr: &serror.Event{Err: errors.New("failed to get secret 'invalid'")},
+			wantErr: &serror.Event{Err: errors.New("failed to get authentication secret '/invalid'")},
 			assertFunc: func(g *WithT, obj *helmv1.HelmChart, build chart.Build) {
 				g.Expect(build.Complete()).To(BeFalse())
 
 				g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{
-					*conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret 'invalid'"),
+					*conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get authentication secret '/invalid'"),
 				}))
 			},
 		},
@@ -1190,12 +1190,12 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
 				}
 			},
 			want:    sreconcile.ResultEmpty,
-			wantErr: &serror.Event{Err: errors.New("failed to get secret 'invalid'")},
+			wantErr: &serror.Event{Err: errors.New("failed to get authentication secret '/invalid'")},
 			assertFunc: func(g *WithT, obj *helmv1.HelmChart, build chart.Build) {
 				g.Expect(build.Complete()).To(BeFalse())
 
 				g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{
-					*conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret 'invalid'"),
+					*conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get authentication secret '/invalid'"),
 				}))
 			},
 		},

diff --git a/internal/controller/helmrepository_controller.go b/internal/controller/helmrepository_controller.go
@@ -18,7 +18,6 @@ package controller
 
 import (
 	"context"
-	"crypto/tls"
 	"errors"
 	"fmt"
 	"net/url"
@@ -29,7 +28,6 @@ import (
 	helmgetter "helm.sh/helm/v3/pkg/getter"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/types"
 	kuberecorder "k8s.io/client-go/tools/record"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -51,7 +49,6 @@ import (
 	"github.com/fluxcd/source-controller/internal/cache"
 	intdigest "github.com/fluxcd/source-controller/internal/digest"
 	serror "github.com/fluxcd/source-controller/internal/error"
-	"github.com/fluxcd/source-controller/internal/helm/getter"
 	"github.com/fluxcd/source-controller/internal/helm/repository"
 	intpredicates "github.com/fluxcd/source-controller/internal/predicates"
 	sreconcile "github.com/fluxcd/source-controller/internal/reconcile"
@@ -390,59 +387,22 @@ func (r *HelmRepositoryReconciler) reconcileStorage(ctx context.Context, sp *pat
 // pointer is set to the newly fetched index.
 func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch.SerialPatcher,
 	obj *helmv1.HelmRepository, artifact *sourcev1.Artifact, chartRepo *repository.ChartRepository) (sreconcile.Result, error) {
-	var tlsConfig *tls.Config
-
-	// Configure Helm client to access repository
-	clientOpts := []helmgetter.Option{
-		helmgetter.WithTimeout(obj.Spec.Timeout.Duration),
-		helmgetter.WithURL(obj.Spec.URL),
-		helmgetter.WithPassCredentialsAll(obj.Spec.PassCredentials),
+	hcOpts, err := repository.GetHelmClientOpts(ctx, r.Client, obj, obj.Spec.URL, false)
+	if err != nil {
+		e := serror.NewGeneric(
+			err,
+			sourcev1.AuthenticationFailedReason,
+		)
+		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
+		return sreconcile.ResultEmpty, e
 	}
-
-	// Configure any authentication related options
-	if obj.Spec.SecretRef != nil {
-		// Attempt to retrieve secret
-		name := types.NamespacedName{
-			Namespace: obj.GetNamespace(),
-			Name:      obj.Spec.SecretRef.Name,
-		}
-		var secret corev1.Secret
-		if err := r.Client.Get(ctx, name, &secret); err != nil {
-			e := &serror.Event{
-				Err:    fmt.Errorf("failed to get secret '%s': %w", name.String(), err),
-				Reason: sourcev1.AuthenticationFailedReason,
-			}
-			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
-			return sreconcile.ResultEmpty, e
-		}
-
-		// Construct actual options
-		opts, err := getter.ClientOptionsFromSecret(secret)
-		if err != nil {
-			e := &serror.Event{
-				Err:    fmt.Errorf("failed to configure Helm client with secret data: %w", err),
-				Reason: sourcev1.AuthenticationFailedReason,
-			}
-			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
-			// Return err as the content of the secret may change.
-			return sreconcile.ResultEmpty, e
-		}
-		clientOpts = append(clientOpts, opts...)
-
-		tlsConfig, err = getter.TLSClientConfigFromSecret(secret, obj.Spec.URL)
-		if err != nil {
-			e := &serror.Event{
-				Err:    fmt.Errorf("failed to create TLS client config with secret data: %w", err),
-				Reason: sourcev1.AuthenticationFailedReason,
-			}
-			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
-			// Requeue as content of secret might change
-			return sreconcile.ResultEmpty, e
-		}
+	if hcOpts.EmitDeprecationWarning {
+		r.Event(obj, "Warning", "DeprecationWarning",
+			"specifying TLS authentication data via `.spec.secretRef` is deprecated, please use `.spec.certSecretRef` instead")
 	}
 
 	// Construct Helm chart repository with options and download index
-	newChartRepo, err := repository.NewChartRepository(obj.Spec.URL, "", r.Getters, tlsConfig, clientOpts...)
+	newChartRepo, err := repository.NewChartRepository(obj.Spec.URL, "", r.Getters, hcOpts.TlsConfig, hcOpts.GetterOpts...)
 	if err != nil {
 		switch err.(type) {
 		case *url.Error:

diff --git a/internal/controller/helmrepository_controller_oci.go b/internal/controller/helmrepository_controller_oci.go
@@ -318,7 +318,7 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, sp *patch.S
 			return
 		}
 	} else if obj.Spec.Provider != helmv1.GenericOCIProvider && obj.Spec.Type == helmv1.HelmRepositoryTypeOCI {
-		auth, authErr := oidcAuth(ctxTimeout, obj.Spec.URL, obj.Spec.Provider)
+		auth, authErr := registry.OIDCAuth(ctxTimeout, obj.Spec.URL, obj.Spec.Provider)
 		if authErr != nil && !errors.Is(authErr, oci.ErrUnconfiguredProvider) {
 			e := fmt.Errorf("failed to get credential from %s: %w", obj.Spec.Provider, authErr)
 			conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, e.Error())