From a1bab65757f2601d745250e2fc36ad5c50fd9d21 Mon Sep 17 00:00:00 2001 From: cappyzawa Date: Fri, 4 Jul 2025 22:25:58 +0900 Subject: [PATCH] Migrate to runtime/secrets v0.63.0 and refactor Update pkg/runtime from v0.60.0 to v0.63.0 and migrate all secret handling to use runtime/secrets APIs directly. Key changes: - Remove wrapper functions and use runtime/secrets directly - Refactor bucket controller reconcileSource into focused functions - Update error messages to runtime/secrets v0.63.0 format - Add comprehensive tests for LoginOptionFromSecretRef - Clean up 600+ lines of redundant code and tests This establishes unified secret handling across source-controller. Signed-off-by: cappyzawa --- go.mod | 2 +- go.sum | 4 +- internal/controller/bucket_controller.go | 378 ++++++++---------- internal/controller/bucket_controller_test.go | 242 ++--------- internal/controller/helmchart_controller.go | 20 +- .../helmrepository_controller_test.go | 32 +- .../controller/ocirepository_controller.go | 79 +--- .../ocirepository_controller_test.go | 189 +-------- internal/helm/getter/client_opts.go | 239 +++++++---- internal/helm/getter/getter.go | 54 --- internal/helm/getter/getter_test.go | 93 ----- internal/helm/registry/auth.go | 36 ++ internal/helm/registry/auth_test.go | 152 +++++++ pkg/gcp/gcp.go | 3 +- pkg/gcp/gcp_test.go | 2 +- pkg/minio/minio.go | 15 +- pkg/minio/minio_test.go | 18 +- 17 files changed, 615 insertions(+), 943 deletions(-) delete mode 100644 internal/helm/getter/getter.go delete mode 100644 internal/helm/getter/getter_test.go diff --git a/go.mod b/go.mod index 6bf20c478..324bbc9ec 100644 --- a/go.mod +++ b/go.mod @@ -37,7 +37,7 @@ require ( github.com/fluxcd/pkg/lockedfile v0.6.0 github.com/fluxcd/pkg/masktoken v0.7.0 github.com/fluxcd/pkg/oci v0.49.0 - github.com/fluxcd/pkg/runtime v0.60.0 + github.com/fluxcd/pkg/runtime v0.63.0 github.com/fluxcd/pkg/sourceignore v0.12.0 github.com/fluxcd/pkg/ssh v0.19.0 github.com/fluxcd/pkg/tar v0.12.0 diff --git a/go.sum b/go.sum index 6fe367061..521f3fb4f 100644 --- a/go.sum +++ b/go.sum @@ -391,8 +391,8 @@ github.com/fluxcd/pkg/masktoken v0.7.0 h1:pitmyOg2pUVdW+nn2Lk/xqm2TaA08uxvOC0ns3 github.com/fluxcd/pkg/masktoken v0.7.0/go.mod h1:Lc1uoDjO1GY6+YdkK+ZqqBIBWquyV58nlSJ5S1N1IYU= github.com/fluxcd/pkg/oci v0.49.0 h1:L8/dmNSIzqu6X8vzIkPLrW8NAF7Et/SnOuI8WJkXeq8= github.com/fluxcd/pkg/oci v0.49.0/go.mod h1:iZkF4bQTpc6YOU5IJWMBp0Q8voGm7bkMYiAarJ9407U= -github.com/fluxcd/pkg/runtime v0.60.0 h1:d++EkV3FlycB+bzakB5NumwY4J8xts8i7lbvD6jBLeU= -github.com/fluxcd/pkg/runtime v0.60.0/go.mod h1:UeU0/eZLErYC/1bTmgzBfNXhiHy9fuQzjfLK0HxRgxY= +github.com/fluxcd/pkg/runtime v0.63.0 h1:55J7ascGmXyTXWGwhD21N9fU7jC1l5rhdzjgNXs6aZg= +github.com/fluxcd/pkg/runtime v0.63.0/go.mod h1:7pxGvaU0Yy1cDIUhiHAHhCx2yCLnkcVsplbYZG6j4JY= github.com/fluxcd/pkg/sourceignore v0.12.0 h1:jCIe6d50rQ3wdXPF0+PhhqN0XrTRIq3upMomPelI8Mw= github.com/fluxcd/pkg/sourceignore v0.12.0/go.mod h1:dc0zvkuXM5OgL/b3IkrVuwvPjj1zJn4NBUMH45uJ4Y0= github.com/fluxcd/pkg/ssh v0.19.0 h1:njSwNJQZ+3TGhBXshU/2TbqvooMbf6lQzFn7w6vuaKI= diff --git a/internal/controller/bucket_controller.go b/internal/controller/bucket_controller.go index d67c10f9b..3564b594b 100644 --- a/internal/controller/bucket_controller.go +++ b/internal/controller/bucket_controller.go @@ -31,6 +31,7 @@ import ( "golang.org/x/sync/errgroup" "golang.org/x/sync/semaphore" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" kuberecorder "k8s.io/client-go/tools/record" @@ -50,6 +51,7 @@ import ( "github.com/fluxcd/pkg/runtime/patch" "github.com/fluxcd/pkg/runtime/predicates" rreconcile "github.com/fluxcd/pkg/runtime/reconcile" + "github.com/fluxcd/pkg/runtime/secrets" "github.com/fluxcd/pkg/sourceignore" sourcev1 "github.com/fluxcd/source-controller/api/v1" @@ -58,7 +60,6 @@ import ( "github.com/fluxcd/source-controller/internal/index" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" "github.com/fluxcd/source-controller/internal/reconcile/summarize" - "github.com/fluxcd/source-controller/internal/tls" "github.com/fluxcd/source-controller/pkg/azure" "github.com/fluxcd/source-controller/pkg/gcp" "github.com/fluxcd/source-controller/pkg/minio" @@ -421,165 +422,37 @@ func (r *BucketReconciler) reconcileStorage(ctx context.Context, sp *patch.Seria // the provider. If this fails, it records v1.FetchFailedCondition=True on // the object and returns early. func (r *BucketReconciler) reconcileSource(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.Bucket, index *index.Digester, dir string) (sreconcile.Result, error) { - secret, err := r.getSecret(ctx, obj.Spec.SecretRef, obj.GetNamespace()) - if err != nil { - e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - // Return error as the world as observed may change - return sreconcile.ResultEmpty, e - } - proxyURL, err := r.getProxyURL(ctx, obj) + secret, proxyURL, err := r.setupCredentials(ctx, obj) if err != nil { e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) return sreconcile.ResultEmpty, e } - // Construct provider client - var provider BucketProvider - switch obj.Spec.Provider { - case sourcev1.BucketProviderGoogle: - if err = gcp.ValidateSecret(secret); err != nil { - e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } - var opts []gcp.Option - if secret != nil { - opts = append(opts, gcp.WithSecret(secret)) - } - if proxyURL != nil { - opts = append(opts, gcp.WithProxyURL(proxyURL)) - } - if provider, err = gcp.NewClient(ctx, opts...); err != nil { - e := serror.NewGeneric(err, "ClientError") - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } - case sourcev1.BucketProviderAzure: - if err = azure.ValidateSecret(secret); err != nil { - e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } - var opts []azure.Option - if secret != nil { - opts = append(opts, azure.WithSecret(secret)) - } - if proxyURL != nil { - opts = append(opts, azure.WithProxyURL(proxyURL)) - } - if provider, err = azure.NewClient(obj, opts...); err != nil { - e := serror.NewGeneric(err, "ClientError") - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } - default: - if err = minio.ValidateSecret(secret); err != nil { - e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } - tlsConfig, err := r.getTLSConfig(ctx, obj.Spec.CertSecretRef, obj.GetNamespace(), obj.Spec.Endpoint) - if err != nil { - e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } - stsSecret, err := r.getSTSSecret(ctx, obj) - if err != nil { - e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } - stsTLSConfig, err := r.getSTSTLSConfig(ctx, obj) - if err != nil { - err := fmt.Errorf("failed to get STS TLS config: %w", err) + provider, err := r.createBucketProvider(ctx, obj, secret, proxyURL) + if err != nil { + var stallingErr *serror.Stalling + var genericErr *serror.Generic + if errors.As(err, &stallingErr) { + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, stallingErr.Reason, "%s", stallingErr) + return sreconcile.ResultEmpty, stallingErr + } else if errors.As(err, &genericErr) { + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, genericErr.Reason, "%s", genericErr) + return sreconcile.ResultEmpty, genericErr + } else { e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) return sreconcile.ResultEmpty, e } - if sts := obj.Spec.STS; sts != nil { - if err := minio.ValidateSTSProvider(obj.Spec.Provider, sts); err != nil { - e := serror.NewStalling(err, sourcev1.InvalidSTSConfigurationReason) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } - if _, err := url.Parse(sts.Endpoint); err != nil { - err := fmt.Errorf("failed to parse STS endpoint '%s': %w", sts.Endpoint, err) - e := serror.NewStalling(err, sourcev1.URLInvalidReason) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } - if err := minio.ValidateSTSSecret(sts.Provider, stsSecret); err != nil { - e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } - } - var opts []minio.Option - if secret != nil { - opts = append(opts, minio.WithSecret(secret)) - } - if tlsConfig != nil { - opts = append(opts, minio.WithTLSConfig(tlsConfig)) - } - if proxyURL != nil { - opts = append(opts, minio.WithProxyURL(proxyURL)) - } - if stsSecret != nil { - opts = append(opts, minio.WithSTSSecret(stsSecret)) - } - if stsTLSConfig != nil { - opts = append(opts, minio.WithSTSTLSConfig(stsTLSConfig)) - } - if provider, err = minio.NewClient(obj, opts...); err != nil { - e := serror.NewGeneric(err, "ClientError") - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } } - // Fetch etag index - if err = fetchEtagIndex(ctx, provider, obj, index, dir); err != nil { + err = r.processArtifacts(ctx, sp, provider, obj, index, dir) + if err != nil { e := serror.NewGeneric(err, sourcev1.BucketOperationFailedReason) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) return sreconcile.ResultEmpty, e } - // Check if index has changed compared to current Artifact revision. - var changed bool - if artifact := obj.Status.Artifact; artifact != nil && artifact.Revision != "" { - curRev := digest.Digest(artifact.Revision) - changed = curRev.Validate() != nil || curRev != index.Digest(curRev.Algorithm()) - } - - // Fetch the bucket objects if required to. - if artifact := obj.GetArtifact(); artifact == nil || changed { - // Mark observations about the revision on the object - defer func() { - // As fetchIndexFiles can make last-minute modifications to the etag - // index, we need to re-calculate the revision at the end - revision := index.Digest(intdigest.Canonical) - - message := fmt.Sprintf("new upstream revision '%s'", revision) - if obj.GetArtifact() != nil { - conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "%s", message) - } - rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, "building artifact: %s", message) - if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { - ctrl.LoggerFrom(ctx).Error(err, "failed to patch") - return - } - }() - - if err = fetchIndexFiles(ctx, provider, obj, index, dir); err != nil { - e := serror.NewGeneric(err, sourcev1.BucketOperationFailedReason) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } - } - conditions.Delete(obj, sourcev1.FetchFailedCondition) return sreconcile.ResultSuccess, nil } @@ -748,73 +621,14 @@ func (r *BucketReconciler) getSecret(ctx context.Context, secretRef *meta.LocalO } secret := &corev1.Secret{} if err := r.Get(ctx, secretName, secret); err != nil { + if apierrors.IsNotFound(err) { + return nil, fmt.Errorf("secret '%s' not found", secretName.String()) + } return nil, fmt.Errorf("failed to get secret '%s': %w", secretName.String(), err) } return secret, nil } -// getTLSConfig attempts to fetch a TLS configuration from the given -// Secret reference, namespace and endpoint. -func (r *BucketReconciler) getTLSConfig(ctx context.Context, - secretRef *meta.LocalObjectReference, namespace, endpoint string) (*stdtls.Config, error) { - certSecret, err := r.getSecret(ctx, secretRef, namespace) - if err != nil || certSecret == nil { - return nil, err - } - tlsConfig, _, err := tls.KubeTLSClientConfigFromSecret(*certSecret, endpoint) - if err != nil { - return nil, fmt.Errorf("failed to create TLS config: %w", err) - } - if tlsConfig == nil { - return nil, fmt.Errorf("certificate secret does not contain any TLS configuration") - } - return tlsConfig, nil -} - -// getProxyURL attempts to fetch a proxy URL from the object's proxy secret -// reference. -func (r *BucketReconciler) getProxyURL(ctx context.Context, obj *sourcev1.Bucket) (*url.URL, error) { - namespace := obj.GetNamespace() - proxySecret, err := r.getSecret(ctx, obj.Spec.ProxySecretRef, namespace) - if err != nil || proxySecret == nil { - return nil, err - } - proxyData := proxySecret.Data - address, ok := proxyData["address"] - if !ok { - return nil, fmt.Errorf("invalid proxy secret '%s/%s': key 'address' is missing", - namespace, obj.Spec.ProxySecretRef.Name) - } - proxyURL, err := url.Parse(string(address)) - if err != nil { - return nil, fmt.Errorf("failed to parse proxy address '%s': %w", address, err) - } - user, hasUser := proxyData["username"] - password, hasPassword := proxyData["password"] - if hasUser || hasPassword { - proxyURL.User = url.UserPassword(string(user), string(password)) - } - return proxyURL, nil -} - -// getSTSSecret attempts to fetch the secret from the object's STS secret -// reference. -func (r *BucketReconciler) getSTSSecret(ctx context.Context, obj *sourcev1.Bucket) (*corev1.Secret, error) { - if obj.Spec.STS == nil { - return nil, nil - } - return r.getSecret(ctx, obj.Spec.STS.SecretRef, obj.GetNamespace()) -} - -// getSTSTLSConfig attempts to fetch the certificate secret from the object's -// STS configuration. -func (r *BucketReconciler) getSTSTLSConfig(ctx context.Context, obj *sourcev1.Bucket) (*stdtls.Config, error) { - if obj.Spec.STS == nil { - return nil, nil - } - return r.getTLSConfig(ctx, obj.Spec.STS.CertSecretRef, obj.GetNamespace(), obj.Spec.STS.Endpoint) -} - // eventLogf records events, and logs at the same time. // // This log is different from the debug log in the EventRecorder, in the sense @@ -943,3 +757,157 @@ func fetchIndexFiles(ctx context.Context, provider BucketProvider, obj *sourcev1 return nil } + +// setupCredentials retrieves the secret and proxy URL configuration for bucket access. +// It returns the secret for authentication, proxy URL if configured, and any error encountered. +func (r *BucketReconciler) setupCredentials(ctx context.Context, obj *sourcev1.Bucket) (*corev1.Secret, *url.URL, error) { + secret, err := r.getSecret(ctx, obj.Spec.SecretRef, obj.GetNamespace()) + if err != nil { + return nil, nil, err + } + + var proxyURL *url.URL + if obj.Spec.ProxySecretRef != nil { + proxyURL, err = secrets.ProxyURLFromSecret(ctx, r.Client, obj.Spec.ProxySecretRef.Name, obj.GetNamespace()) + } + if err != nil { + return nil, nil, err + } + + return secret, proxyURL, nil +} + +// createBucketProvider creates a provider-specific bucket client using the given credentials and configuration. +// It handles different bucket providers (AWS, GCP, Azure, generic) and returns the appropriate client. +func (r *BucketReconciler) createBucketProvider(ctx context.Context, obj *sourcev1.Bucket, secret *corev1.Secret, proxyURL *url.URL) (BucketProvider, error) { + switch obj.Spec.Provider { + case sourcev1.BucketProviderGoogle: + if err := gcp.ValidateSecret(secret); err != nil { + return nil, err + } + var opts []gcp.Option + if secret != nil { + opts = append(opts, gcp.WithSecret(secret)) + } + if proxyURL != nil { + opts = append(opts, gcp.WithProxyURL(proxyURL)) + } + return gcp.NewClient(ctx, opts...) + + case sourcev1.BucketProviderAzure: + if err := azure.ValidateSecret(secret); err != nil { + return nil, err + } + var opts []azure.Option + if secret != nil { + opts = append(opts, azure.WithSecret(secret)) + } + if proxyURL != nil { + opts = append(opts, azure.WithProxyURL(proxyURL)) + } + return azure.NewClient(obj, opts...) + + default: + if err := minio.ValidateSecret(secret); err != nil { + return nil, err + } + var tlsConfig *stdtls.Config + var err error + if obj.Spec.CertSecretRef != nil { + tlsConfig, err = secrets.TLSConfigFromSecret(ctx, r.Client, obj.Spec.CertSecretRef.Name, obj.GetNamespace()) + if err != nil { + return nil, fmt.Errorf("failed to create TLS config: %w", err) + } + if tlsConfig == nil { + return nil, fmt.Errorf("certificate secret does not contain any TLS configuration") + } + u, err := url.Parse(obj.Spec.Endpoint) + if err != nil { + return nil, fmt.Errorf("cannot parse endpoint URL: %w", err) + } + tlsConfig.ServerName = u.Hostname() + tlsConfig.MinVersion = stdtls.VersionTLS12 + } + var stsSecret *corev1.Secret + if obj.Spec.STS != nil { + stsSecret, err = r.getSecret(ctx, obj.Spec.STS.SecretRef, obj.GetNamespace()) + if err != nil { + return nil, err + } + } + var stsTLSConfig *stdtls.Config + if obj.Spec.STS != nil && obj.Spec.STS.CertSecretRef != nil { + stsTLSConfig, err = secrets.TLSConfigFromSecret(ctx, r.Client, obj.Spec.STS.CertSecretRef.Name, obj.GetNamespace()) + if err != nil { + return nil, fmt.Errorf("failed to get STS TLS config: %w", err) + } + } + if sts := obj.Spec.STS; sts != nil { + if err := minio.ValidateSTSProvider(obj.Spec.Provider, sts); err != nil { + return nil, serror.NewStalling(err, sourcev1.InvalidSTSConfigurationReason) + } + if _, err := url.Parse(sts.Endpoint); err != nil { + return nil, serror.NewStalling(fmt.Errorf("failed to parse STS endpoint '%s': %w", sts.Endpoint, err), sourcev1.URLInvalidReason) + } + if err := minio.ValidateSTSSecret(sts.Provider, stsSecret); err != nil { + return nil, serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) + } + } + var opts []minio.Option + if secret != nil { + opts = append(opts, minio.WithSecret(secret)) + } + if tlsConfig != nil { + opts = append(opts, minio.WithTLSConfig(tlsConfig)) + } + if proxyURL != nil { + opts = append(opts, minio.WithProxyURL(proxyURL)) + } + if stsSecret != nil { + opts = append(opts, minio.WithSTSSecret(stsSecret)) + } + if stsTLSConfig != nil { + opts = append(opts, minio.WithSTSTLSConfig(stsTLSConfig)) + } + return minio.NewClient(obj, opts...) + } +} + +// processArtifacts handles etag index retrieval and bucket object fetching. +// It fetches the etag index from the provider and downloads objects to the specified directory. +func (r *BucketReconciler) processArtifacts(ctx context.Context, sp *patch.SerialPatcher, provider BucketProvider, obj *sourcev1.Bucket, index *index.Digester, dir string) error { + if err := fetchEtagIndex(ctx, provider, obj, index, dir); err != nil { + return err + } + var changed bool + if artifact := obj.Status.Artifact; artifact != nil && artifact.Revision != "" { + curRev := digest.Digest(artifact.Revision) + changed = curRev.Validate() != nil || curRev != index.Digest(curRev.Algorithm()) + } + + // Fetch the bucket objects if required to. + if artifact := obj.GetArtifact(); artifact == nil || changed { + // Mark observations about the revision on the object + defer func() { + // As fetchIndexFiles can make last-minute modifications to the etag + // index, we need to re-calculate the revision at the end + revision := index.Digest(intdigest.Canonical) + + message := fmt.Sprintf("new upstream revision '%s'", revision) + if obj.GetArtifact() != nil { + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "%s", message) + } + rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, "building artifact: %s", message) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + ctrl.LoggerFrom(ctx).Error(err, "failed to patch") + return + } + }() + + if err := fetchIndexFiles(ctx, provider, obj, index, dir); err != nil { + return err + } + } + + return nil +} diff --git a/internal/controller/bucket_controller_test.go b/internal/controller/bucket_controller_test.go index 7563d6e99..eedc44bd4 100644 --- a/internal/controller/bucket_controller_test.go +++ b/internal/controller/bucket_controller_test.go @@ -481,7 +481,7 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { wantErr: true, assertIndex: index.NewDigester(), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret '/dummy': secrets \"dummy\" not found"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "secret 'default/dummy' not found"), *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, @@ -491,8 +491,10 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { bucketName: "dummy", secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", + Name: "dummy", + Namespace: "default", }, + Data: map[string][]byte{}, }, beforeFunc: func(obj *sourcev1.Bucket) { obj.Spec.SecretRef = &meta.LocalObjectReference{ @@ -504,7 +506,7 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { wantErr: true, assertIndex: index.NewDigester(), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "invalid 'dummy' secret data: required fields 'accesskey' and 'secretkey'"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "secret 'default/dummy': key 'accesskey' not found"), *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, @@ -522,7 +524,7 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { wantErr: true, assertIndex: index.NewDigester(), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret '/dummy': secrets \"dummy\" not found"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to create TLS config: secret 'default/dummy' not found"), *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, @@ -532,7 +534,8 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { bucketName: "dummy", secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", + Name: "dummy", + Namespace: "default", }, }, beforeFunc: func(obj *sourcev1.Bucket) { @@ -547,7 +550,7 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "certificate secret does not contain any TLS configuration"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to create TLS config: secret 'default/dummy' must contain either 'ca.crt' or both 'tls.crt' and 'tls.key'"), }, }, { @@ -563,7 +566,7 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { wantErr: true, assertIndex: index.NewDigester(), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret '/dummy': secrets \"dummy\" not found"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "secret 'default/dummy' not found"), *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, @@ -573,7 +576,8 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { bucketName: "dummy", secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", + Name: "dummy", + Namespace: "default", }, }, beforeFunc: func(obj *sourcev1.Bucket) { @@ -588,7 +592,7 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "invalid proxy secret '/dummy': key 'address' is missing"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "secret 'default/dummy': key 'address' not found"), }, }, { @@ -604,7 +608,7 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { wantErr: true, assertIndex: index.NewDigester(), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret '/dummy': secrets \"dummy\" not found"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "secret 'default/dummy' not found"), *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, @@ -614,8 +618,10 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { bucketName: "dummy", secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", + Name: "dummy", + Namespace: "default", }, + Data: map[string][]byte{}, }, beforeFunc: func(obj *sourcev1.Bucket) { obj.Spec.Provider = "generic" @@ -632,7 +638,7 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "invalid 'dummy' secret data for 'ldap' STS provider: required fields username, password"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "secret 'default/dummy': key 'username' not found"), }, }, { @@ -648,7 +654,7 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { wantErr: true, assertIndex: index.NewDigester(), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret '/dummy': secrets \"dummy\" not found"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get STS TLS config: secret 'default/dummy' not found"), *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, @@ -658,7 +664,8 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { bucketName: "dummy", secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", + Name: "dummy", + Namespace: "default", }, }, beforeFunc: func(obj *sourcev1.Bucket) { @@ -676,7 +683,7 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get STS TLS config: certificate secret does not contain any TLS configuration"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get STS TLS config: secret 'default/dummy' must contain either 'ca.crt' or both 'tls.crt' and 'tls.key'"), }, }, { @@ -921,6 +928,7 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-bucket-", Generation: 1, + Namespace: "default", }, Spec: sourcev1.BucketSpec{ Timeout: &metav1.Duration{Duration: timeout}, @@ -994,7 +1002,8 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { }, secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", + Name: "dummy", + Namespace: "default", }, Data: map[string][]byte{ "accesskey": []byte("key"), @@ -1030,7 +1039,7 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { wantErr: true, assertIndex: index.NewDigester(), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret '/dummy': secrets \"dummy\" not found"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "secret 'default/dummy' not found"), *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, @@ -1040,8 +1049,10 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { bucketName: "dummy", secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", + Name: "dummy", + Namespace: "default", }, + Data: map[string][]byte{}, }, beforeFunc: func(obj *sourcev1.Bucket) { obj.Spec.SecretRef = &meta.LocalObjectReference{ @@ -1054,7 +1065,7 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { wantErr: true, assertIndex: index.NewDigester(), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "invalid 'dummy' secret data: required fields"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "secret 'default/dummy': key 'serviceaccount' not found"), *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, @@ -1073,7 +1084,7 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { wantErr: true, assertIndex: index.NewDigester(), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret '/dummy': secrets \"dummy\" not found"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "secret 'default/dummy' not found"), *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, @@ -1083,7 +1094,8 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { bucketName: "dummy", secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", + Name: "dummy", + Namespace: "default", }, }, beforeFunc: func(obj *sourcev1.Bucket) { @@ -1097,7 +1109,7 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { wantErr: true, assertIndex: index.NewDigester(), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "invalid proxy secret '/dummy': key 'address' is missing"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "secret 'default/dummy': key 'address' not found"), *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, @@ -1309,6 +1321,7 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-bucket-", Generation: 1, + Namespace: "default", }, Spec: sourcev1.BucketSpec{ BucketName: tt.bucketName, @@ -1751,191 +1764,6 @@ func TestBucketReconciler_notify(t *testing.T) { } } -func TestBucketReconciler_getProxyURL(t *testing.T) { - tests := []struct { - name string - bucket *sourcev1.Bucket - objects []client.Object - expectedURL string - expectedErr string - }{ - { - name: "empty proxySecretRef", - bucket: &sourcev1.Bucket{ - Spec: sourcev1.BucketSpec{ - ProxySecretRef: nil, - }, - }, - }, - { - name: "non-existing proxySecretRef", - bucket: &sourcev1.Bucket{ - Spec: sourcev1.BucketSpec{ - ProxySecretRef: &meta.LocalObjectReference{ - Name: "non-existing", - }, - }, - }, - expectedErr: "failed to get secret '/non-existing': secrets \"non-existing\" not found", - }, - { - name: "missing address in proxySecretRef", - bucket: &sourcev1.Bucket{ - Spec: sourcev1.BucketSpec{ - ProxySecretRef: &meta.LocalObjectReference{ - Name: "dummy", - }, - }, - }, - objects: []client.Object{ - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", - }, - Data: map[string][]byte{}, - }, - }, - expectedErr: "invalid proxy secret '/dummy': key 'address' is missing", - }, - { - name: "invalid address in proxySecretRef", - bucket: &sourcev1.Bucket{ - Spec: sourcev1.BucketSpec{ - ProxySecretRef: &meta.LocalObjectReference{ - Name: "dummy", - }, - }, - }, - objects: []client.Object{ - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", - }, - Data: map[string][]byte{ - "address": {0x7f}, - }, - }, - }, - expectedErr: "failed to parse proxy address '\x7f': parse \"\\x7f\": net/url: invalid control character in URL", - }, - { - name: "no user, no password", - bucket: &sourcev1.Bucket{ - Spec: sourcev1.BucketSpec{ - ProxySecretRef: &meta.LocalObjectReference{ - Name: "dummy", - }, - }, - }, - objects: []client.Object{ - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", - }, - Data: map[string][]byte{ - "address": []byte("http://proxy.example.com"), - }, - }, - }, - expectedURL: "http://proxy.example.com", - }, - { - name: "user, no password", - bucket: &sourcev1.Bucket{ - Spec: sourcev1.BucketSpec{ - ProxySecretRef: &meta.LocalObjectReference{ - Name: "dummy", - }, - }, - }, - objects: []client.Object{ - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", - }, - Data: map[string][]byte{ - "address": []byte("http://proxy.example.com"), - "username": []byte("user"), - }, - }, - }, - expectedURL: "http://user:@proxy.example.com", - }, - { - name: "no user, password", - bucket: &sourcev1.Bucket{ - Spec: sourcev1.BucketSpec{ - ProxySecretRef: &meta.LocalObjectReference{ - Name: "dummy", - }, - }, - }, - objects: []client.Object{ - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", - }, - Data: map[string][]byte{ - "address": []byte("http://proxy.example.com"), - "password": []byte("password"), - }, - }, - }, - expectedURL: "http://:password@proxy.example.com", - }, - { - name: "user, password", - bucket: &sourcev1.Bucket{ - Spec: sourcev1.BucketSpec{ - ProxySecretRef: &meta.LocalObjectReference{ - Name: "dummy", - }, - }, - }, - objects: []client.Object{ - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", - }, - Data: map[string][]byte{ - "address": []byte("http://proxy.example.com"), - "username": []byte("user"), - "password": []byte("password"), - }, - }, - }, - expectedURL: "http://user:password@proxy.example.com", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - c := fakeclient.NewClientBuilder(). - WithScheme(testEnv.Scheme()). - WithObjects(tt.objects...). - Build() - - r := &BucketReconciler{ - Client: c, - } - - u, err := r.getProxyURL(ctx, tt.bucket) - if tt.expectedErr == "" { - g.Expect(err).To(BeNil()) - } else { - g.Expect(err.Error()).To(ContainSubstring(tt.expectedErr)) - } - if tt.expectedURL == "" { - g.Expect(u).To(BeNil()) - } else { - g.Expect(u.String()).To(Equal(tt.expectedURL)) - } - }) - } -} - func TestBucketReconciler_APIServerValidation_STS(t *testing.T) { tests := []struct { name string diff --git a/internal/controller/helmchart_controller.go b/internal/controller/helmchart_controller.go index 19d320ecf..e86940a6e 100644 --- a/internal/controller/helmchart_controller.go +++ b/internal/controller/helmchart_controller.go @@ -1342,8 +1342,8 @@ func (r *HelmChartReconciler) makeVerifiers(ctx context.Context, obj *sourcev1.H Name: secretRef.Name, } - pubSecret, err := r.retrieveSecret(ctx, verifySecret) - if err != nil { + var pubSecret corev1.Secret + if err := r.Get(ctx, verifySecret, &pubSecret); err != nil { return nil, err } @@ -1393,8 +1393,8 @@ func (r *HelmChartReconciler) makeVerifiers(ctx context.Context, obj *sourcev1.H Name: secretRef.Name, } - pubSecret, err := r.retrieveSecret(ctx, verifySecret) - if err != nil { + var pubSecret corev1.Secret + if err := r.Get(ctx, verifySecret, &pubSecret); err != nil { return nil, err } @@ -1442,15 +1442,3 @@ func (r *HelmChartReconciler) makeVerifiers(ctx context.Context, obj *sourcev1.H return nil, fmt.Errorf("unsupported verification provider: %s", obj.Spec.Verify.Provider) } } - -// retrieveSecret retrieves a secret from the specified namespace with the given secret name. -// It returns the retrieved secret and any error encountered during the retrieval process. -func (r *HelmChartReconciler) retrieveSecret(ctx context.Context, verifySecret types.NamespacedName) (corev1.Secret, error) { - - var pubSecret corev1.Secret - - if err := r.Get(ctx, verifySecret, &pubSecret); err != nil { - return corev1.Secret{}, err - } - return pubSecret, nil -} diff --git a/internal/controller/helmrepository_controller_test.go b/internal/controller/helmrepository_controller_test.go index 9724baf65..21c54341e 100644 --- a/internal/controller/helmrepository_controller_test.go +++ b/internal/controller/helmrepository_controller_test.go @@ -48,11 +48,11 @@ import ( "github.com/fluxcd/pkg/runtime/conditions" conditionscheck "github.com/fluxcd/pkg/runtime/conditions/check" "github.com/fluxcd/pkg/runtime/patch" + "github.com/fluxcd/pkg/runtime/secrets" sourcev1 "github.com/fluxcd/source-controller/api/v1" "github.com/fluxcd/source-controller/internal/cache" intdigest "github.com/fluxcd/source-controller/internal/digest" - "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" @@ -487,13 +487,15 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { }, secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "ca-file", + Name: "ca-file", + Namespace: "default", }, Data: map[string][]byte{ "caFile": tlsCA, }, }, beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, rev digest.Digest) { + obj.Namespace = "default" obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "ca-file"} }, want: sreconcile.ResultSuccess, @@ -518,7 +520,8 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { }, secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "ca-file", + Name: "ca-file", + Namespace: "default", }, Data: map[string][]byte{ "caFile": tlsCA, @@ -526,6 +529,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { Type: corev1.SecretTypeDockerConfigJson, }, beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, rev digest.Digest) { + obj.Namespace = "default" obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "ca-file"} }, want: sreconcile.ResultSuccess, @@ -719,20 +723,22 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { protocol: "http", secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "malformed-basic-auth", + Name: "malformed-basic-auth", + Namespace: "default", }, Data: map[string][]byte{ "username": []byte("git"), }, }, beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, rev digest.Digest) { + obj.Namespace = "default" obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "malformed-basic-auth"} conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, wantErr: true, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "required fields 'username' and 'password"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "secret 'default/malformed-basic-auth': key 'password' not found"), *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, @@ -873,6 +879,8 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { clientBuilder.WithObjects(secret.DeepCopy()) } + fakeClient := clientBuilder.Build() + // Calculate the artifact digest for valid repos configurations. getterOpts := []helmgetter.Option{ helmgetter.WithURL(server.URL()), @@ -881,16 +889,14 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { var tlsConf *tls.Config validSecret := true if secret != nil { - // Extract the client options from secret, ignoring any invalid - // value. validSecret is used to determine if the index digest - // should be calculated below. - var gOpts []helmgetter.Option - var serr error - gOpts, serr = getter.GetterOptionsFromSecret(*secret) + // Extract the client option from secret. validSecret is used to + // determine if the index digest should be calculated below. + username, password, serr := secrets.BasicAuthFromSecret(ctx, fakeClient, secret.Name, secret.Namespace) if serr != nil { validSecret = false + } else { + getterOpts = append(getterOpts, helmgetter.WithBasicAuth(username, password)) } - getterOpts = append(getterOpts, gOpts...) repoURL := server.URL() if tt.url != "" { repoURL = tt.url @@ -919,7 +925,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { r := &HelmRepositoryReconciler{ EventRecorder: record.NewFakeRecorder(32), - Client: clientBuilder.Build(), + Client: fakeClient, Storage: testStorage, Getters: testGetters, patchOptions: getPatchOptions(helmRepositoryReadyCondition.Owned, "sc"), diff --git a/internal/controller/ocirepository_controller.go b/internal/controller/ocirepository_controller.go index ed407c201..b1e2df46f 100644 --- a/internal/controller/ocirepository_controller.go +++ b/internal/controller/ocirepository_controller.go @@ -60,6 +60,7 @@ import ( "github.com/fluxcd/pkg/runtime/patch" "github.com/fluxcd/pkg/runtime/predicates" rreconcile "github.com/fluxcd/pkg/runtime/reconcile" + "github.com/fluxcd/pkg/runtime/secrets" "github.com/fluxcd/pkg/sourceignore" "github.com/fluxcd/pkg/tar" "github.com/fluxcd/pkg/version" @@ -77,7 +78,6 @@ import ( "github.com/fluxcd/source-controller/internal/oci/notation" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" "github.com/fluxcd/source-controller/internal/reconcile/summarize" - "github.com/fluxcd/source-controller/internal/tls" "github.com/fluxcd/source-controller/internal/util" ) @@ -355,14 +355,18 @@ func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch return sreconcile.ResultEmpty, e } - proxyURL, err := r.getProxyURL(ctx, obj) - if err != nil { - e := serror.NewGeneric( - fmt.Errorf("failed to get proxy address: %w", err), - sourcev1.AuthenticationFailedReason, - ) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e + var proxyURL *url.URL + if obj.Spec.ProxySecretRef != nil { + var err error + proxyURL, err = secrets.ProxyURLFromSecret(ctx, r.Client, obj.Spec.ProxySecretRef.Name, obj.GetNamespace()) + if err != nil { + e := serror.NewGeneric( + fmt.Errorf("failed to get proxy address: %w", err), + sourcev1.AuthenticationFailedReason, + ) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) + return sreconcile.ResultEmpty, e + } } if _, ok := keychain.(soci.Anonymous); obj.Spec.Provider != "" && obj.Spec.Provider != sourcev1.GenericOCIProvider && ok { @@ -995,67 +999,14 @@ func (r *OCIRepositoryReconciler) getTLSConfig(ctx context.Context, obj *sourcev return nil, nil } - certSecretName := types.NamespacedName{ - Namespace: obj.Namespace, - Name: obj.Spec.CertSecretRef.Name, - } - var certSecret corev1.Secret - if err := r.Get(ctx, certSecretName, &certSecret); err != nil { - return nil, err - } - - tlsConfig, _, err := tls.KubeTLSClientConfigFromSecret(certSecret, "") + tlsConfig, err := secrets.TLSConfigFromSecret(ctx, r.Client, obj.Spec.CertSecretRef.Name, obj.Namespace) if err != nil { return nil, err } - if tlsConfig == nil { - tlsConfig, _, err = tls.TLSClientConfigFromSecret(certSecret, "") - if err != nil { - return nil, err - } - if tlsConfig != nil { - ctrl.LoggerFrom(ctx). - Info("warning: specifying TLS auth data via `certFile`/`keyFile`/`caFile` is deprecated, please use `tls.crt`/`tls.key`/`ca.crt` instead") - } - } - + tlsConfig.MinVersion = cryptotls.VersionTLS12 return tlsConfig, nil } -// getProxyURL gets the proxy configuration for the transport based on the -// specified proxy secret reference in the OCIRepository object. -func (r *OCIRepositoryReconciler) getProxyURL(ctx context.Context, obj *sourcev1.OCIRepository) (*url.URL, error) { - if obj.Spec.ProxySecretRef == nil || obj.Spec.ProxySecretRef.Name == "" { - return nil, nil - } - - proxySecretName := types.NamespacedName{ - Namespace: obj.Namespace, - Name: obj.Spec.ProxySecretRef.Name, - } - var proxySecret corev1.Secret - if err := r.Get(ctx, proxySecretName, &proxySecret); err != nil { - return nil, err - } - - proxyData := proxySecret.Data - address, ok := proxyData["address"] - if !ok { - return nil, fmt.Errorf("invalid proxy secret '%s/%s': key 'address' is missing", - obj.Namespace, obj.Spec.ProxySecretRef.Name) - } - proxyURL, err := url.Parse(string(address)) - if err != nil { - return nil, fmt.Errorf("failed to parse proxy address '%s': %w", address, err) - } - user, hasUser := proxyData["username"] - password, hasPassword := proxyData["password"] - if hasUser || hasPassword { - proxyURL.User = url.UserPassword(string(user), string(password)) - } - return proxyURL, nil -} - // reconcileStorage ensures the current state of the storage matches the // desired and previously observed state. // diff --git a/internal/controller/ocirepository_controller_test.go b/internal/controller/ocirepository_controller_test.go index aa024082f..fe026cad9 100644 --- a/internal/controller/ocirepository_controller_test.go +++ b/internal/controller/ocirepository_controller_test.go @@ -644,7 +644,7 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { }, }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "%s", "cannot append certificate into certificate pool: invalid CA certificate"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "%s", "failed to parse CA certificate"), }, }, { @@ -913,7 +913,7 @@ func TestOCIRepository_CertSecret(t *testing.T) { }, }, expectreadyconition: false, - expectedstatusmessage: "failed to generate transport for '': tls: failed to find any PEM data in key input", + expectedstatusmessage: "failed to generate transport for '': failed to parse TLS certificate and key: tls: failed to find any PEM data in key input", }, } @@ -3705,188 +3705,3 @@ func TestOCIContentConfigChanged(t *testing.T) { }) } } - -func TestOCIRepositoryReconciler_getProxyURL(t *testing.T) { - tests := []struct { - name string - ociRepo *sourcev1.OCIRepository - objects []client.Object - expectedURL string - expectedErr string - }{ - { - name: "empty proxySecretRef", - ociRepo: &sourcev1.OCIRepository{ - Spec: sourcev1.OCIRepositorySpec{ - ProxySecretRef: nil, - }, - }, - }, - { - name: "non-existing proxySecretRef", - ociRepo: &sourcev1.OCIRepository{ - Spec: sourcev1.OCIRepositorySpec{ - ProxySecretRef: &meta.LocalObjectReference{ - Name: "non-existing", - }, - }, - }, - expectedErr: "secrets \"non-existing\" not found", - }, - { - name: "missing address in proxySecretRef", - ociRepo: &sourcev1.OCIRepository{ - Spec: sourcev1.OCIRepositorySpec{ - ProxySecretRef: &meta.LocalObjectReference{ - Name: "dummy", - }, - }, - }, - objects: []client.Object{ - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", - }, - Data: map[string][]byte{}, - }, - }, - expectedErr: "invalid proxy secret '/dummy': key 'address' is missing", - }, - { - name: "invalid address in proxySecretRef", - ociRepo: &sourcev1.OCIRepository{ - Spec: sourcev1.OCIRepositorySpec{ - ProxySecretRef: &meta.LocalObjectReference{ - Name: "dummy", - }, - }, - }, - objects: []client.Object{ - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", - }, - Data: map[string][]byte{ - "address": {0x7f}, - }, - }, - }, - expectedErr: "failed to parse proxy address '\x7f': parse \"\\x7f\": net/url: invalid control character in URL", - }, - { - name: "no user, no password", - ociRepo: &sourcev1.OCIRepository{ - Spec: sourcev1.OCIRepositorySpec{ - ProxySecretRef: &meta.LocalObjectReference{ - Name: "dummy", - }, - }, - }, - objects: []client.Object{ - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", - }, - Data: map[string][]byte{ - "address": []byte("http://proxy.example.com"), - }, - }, - }, - expectedURL: "http://proxy.example.com", - }, - { - name: "user, no password", - ociRepo: &sourcev1.OCIRepository{ - Spec: sourcev1.OCIRepositorySpec{ - ProxySecretRef: &meta.LocalObjectReference{ - Name: "dummy", - }, - }, - }, - objects: []client.Object{ - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", - }, - Data: map[string][]byte{ - "address": []byte("http://proxy.example.com"), - "username": []byte("user"), - }, - }, - }, - expectedURL: "http://user:@proxy.example.com", - }, - { - name: "no user, password", - ociRepo: &sourcev1.OCIRepository{ - Spec: sourcev1.OCIRepositorySpec{ - ProxySecretRef: &meta.LocalObjectReference{ - Name: "dummy", - }, - }, - }, - objects: []client.Object{ - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", - }, - Data: map[string][]byte{ - "address": []byte("http://proxy.example.com"), - "password": []byte("password"), - }, - }, - }, - expectedURL: "http://:password@proxy.example.com", - }, - { - name: "user, password", - ociRepo: &sourcev1.OCIRepository{ - Spec: sourcev1.OCIRepositorySpec{ - ProxySecretRef: &meta.LocalObjectReference{ - Name: "dummy", - }, - }, - }, - objects: []client.Object{ - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", - }, - Data: map[string][]byte{ - "address": []byte("http://proxy.example.com"), - "username": []byte("user"), - "password": []byte("password"), - }, - }, - }, - expectedURL: "http://user:password@proxy.example.com", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - c := fakeclient.NewClientBuilder(). - WithScheme(testEnv.Scheme()). - WithObjects(tt.objects...). - Build() - - r := &OCIRepositoryReconciler{ - Client: c, - } - - u, err := r.getProxyURL(ctx, tt.ociRepo) - if tt.expectedErr == "" { - g.Expect(err).To(BeNil()) - } else { - g.Expect(err.Error()).To(ContainSubstring(tt.expectedErr)) - } - if tt.expectedURL == "" { - g.Expect(u).To(BeNil()) - } else { - g.Expect(u.String()).To(Equal(tt.expectedURL)) - } - }) - } -} diff --git a/internal/helm/getter/client_opts.go b/internal/helm/getter/client_opts.go index 7fd472b1b..5ed966495 100644 --- a/internal/helm/getter/client_opts.go +++ b/internal/helm/getter/client_opts.go @@ -24,6 +24,7 @@ import ( "os" "path" + "github.com/fluxcd/pkg/runtime/secrets" "github.com/google/go-containerregistry/pkg/authn" helmgetter "helm.sh/helm/v3/pkg/getter" helmreg "helm.sh/helm/v3/pkg/registry" @@ -78,103 +79,40 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *sourcev1.HelmRepos } ociRepo := obj.Spec.Type == sourcev1.HelmRepositoryTypeOCI - var ( - certSecret *corev1.Secret - tlsBytes *stls.TLSBytes - certFile string - keyFile string - caFile string - dir string - err error - ) - // Check `.spec.certSecretRef` first for any TLS auth data. - if obj.Spec.CertSecretRef != nil { - certSecret, err = fetchSecret(ctx, c, obj.Spec.CertSecretRef.Name, obj.GetNamespace()) - if err != nil { - return nil, "", fmt.Errorf("failed to get TLS authentication secret '%s/%s': %w", obj.GetNamespace(), obj.Spec.CertSecretRef.Name, err) - } - - hrOpts.TlsConfig, tlsBytes, err = stls.KubeTLSClientConfigFromSecret(*certSecret, url) - if err != nil { - return nil, "", fmt.Errorf("failed to construct Helm client's TLS config: %w", err) - } + // Setup TLS from dedicated cert secret + tlsBytes, err := setupCertSecret(ctx, c, obj, url, hrOpts) + if err != nil { + return nil, "", err } - var authSecret *corev1.Secret - var deprecatedTLSConfig bool - if obj.Spec.SecretRef != nil { - authSecret, err = fetchSecret(ctx, c, obj.Spec.SecretRef.Name, obj.GetNamespace()) - if err != nil { - return nil, "", fmt.Errorf("failed to get authentication secret '%s/%s': %w", obj.GetNamespace(), obj.Spec.SecretRef.Name, err) - } - - // Construct actual Helm client options. - opts, err := GetterOptionsFromSecret(*authSecret) - if err != nil { - return nil, "", fmt.Errorf("failed to configure Helm client: %w", err) - } - hrOpts.GetterOpts = append(hrOpts.GetterOpts, opts...) - - // If the TLS config is nil, i.e. one couldn't be constructed using - // `.spec.certSecretRef`, then try to use `.spec.secretRef`. - if hrOpts.TlsConfig == nil && !ociRepo { - hrOpts.TlsConfig, tlsBytes, err = stls.LegacyTLSClientConfigFromSecret(*authSecret, url) - if err != nil { - return nil, "", fmt.Errorf("failed to construct Helm client's TLS config: %w", err) - } - // Constructing a TLS config using the auth secret is deprecated behavior. - if hrOpts.TlsConfig != nil { - deprecatedTLSConfig = true - } - } + // Setup authentication and optional legacy TLS + deprecatedTLSConfig, err := setupAuthSecret(ctx, c, obj, url, hrOpts, &tlsBytes, ociRepo) + if err != nil { + return nil, "", err + } - if ociRepo { - hrOpts.Keychain, err = registry.LoginOptionFromSecret(url, *authSecret) - if err != nil { - return nil, "", fmt.Errorf("failed to configure login options: %w", err) - } - } - } else if p := obj.Spec.Provider; p != "" && p != sourcev1.GenericOCIProvider && obj.Spec.Type == sourcev1.HelmRepositoryTypeOCI && ociRepo { - authenticator, authErr := soci.OIDCAuth(ctx, obj.Spec.URL, obj.Spec.Provider) - if authErr != nil { - return nil, "", fmt.Errorf("failed to get credential from '%s': %w", obj.Spec.Provider, authErr) - } - hrOpts.Authenticator = authenticator + // Setup OCI provider authentication + err = setupOCIAuth(ctx, obj, hrOpts, ociRepo) + if err != nil { + return nil, "", err } - if ociRepo { - // Persist the certs files to the path if needed. - if tlsBytes != nil { - dir, err = os.MkdirTemp("", "helm-repo-oci-certs") - if err != nil { - return nil, "", fmt.Errorf("cannot create temporary directory: %w", err) - } - certFile, keyFile, caFile, err = storeTLSCertificateFiles(tlsBytes, dir) - if err != nil { - return nil, "", fmt.Errorf("cannot write certs files to path: %w", err) - } - } - loginOpt, err := registry.NewLoginOption(hrOpts.Authenticator, hrOpts.Keychain, url) - if err != nil { - return nil, "", err - } - if loginOpt != nil { - hrOpts.RegLoginOpts = []helmreg.LoginOption{loginOpt, helmreg.LoginOptInsecure(obj.Spec.Insecure)} - tlsLoginOpt := registry.TLSLoginOption(certFile, keyFile, caFile) - if tlsLoginOpt != nil { - hrOpts.RegLoginOpts = append(hrOpts.RegLoginOpts, tlsLoginOpt) - } - } + // Setup OCI registry configuration + dir, err := setupOCIRegistry(hrOpts, tlsBytes, url, obj, ociRepo) + if err != nil { + return nil, "", err } + if deprecatedTLSConfig { err = ErrDeprecatedTLSConfig } hrOpts.Insecure = obj.Spec.Insecure - return hrOpts, dir, err } +// TODO: Remove fetchSecret once runtime/secrets migration is complete. +// This helper function will be replaced by runtime/secrets package functionality. func fetchSecret(ctx context.Context, c client.Client, name, namespace string) (*corev1.Secret, error) { key := types.NamespacedName{ Namespace: namespace, @@ -222,3 +160,138 @@ func writeToFile(data []byte, filename, tmpDir string) (string, error) { } return file, nil } + +// setupCertSecret configures TLS from the dedicated cert secret (.spec.certSecretRef) +func setupCertSecret(ctx context.Context, c client.Client, obj *sourcev1.HelmRepository, url string, hrOpts *ClientOpts) (*stls.TLSBytes, error) { + if obj.Spec.CertSecretRef == nil { + return nil, nil + } + + // TODO: Replace with runtime/secrets package functionality once migration is complete + certSecret, err := fetchSecret(ctx, c, obj.Spec.CertSecretRef.Name, obj.GetNamespace()) + if err != nil { + return nil, fmt.Errorf("failed to get TLS authentication secret '%s/%s': %w", obj.GetNamespace(), obj.Spec.CertSecretRef.Name, err) + } + + // TODO: Replace with runtime/secrets.TLSConfigFromSecret once migration is complete + tlsConfig, tlsBytes, err := stls.KubeTLSClientConfigFromSecret(*certSecret, url) + if err != nil { + return nil, fmt.Errorf("failed to construct Helm client's TLS config: %w", err) + } + + hrOpts.TlsConfig = tlsConfig + return tlsBytes, nil +} + +// setupAuthSecret configures authentication from .spec.secretRef (Basic Auth, Docker config, or legacy TLS) +func setupAuthSecret(ctx context.Context, c client.Client, obj *sourcev1.HelmRepository, url string, hrOpts *ClientOpts, tlsBytes **stls.TLSBytes, ociRepo bool) (bool, error) { + if obj.Spec.SecretRef == nil { + return false, nil + } + + // TODO: Replace with runtime/secrets package functionality once migration is complete + authSecret, err := fetchSecret(ctx, c, obj.Spec.SecretRef.Name, obj.GetNamespace()) + if err != nil { + return false, fmt.Errorf("failed to get authentication secret '%s/%s': %w", obj.GetNamespace(), obj.Spec.SecretRef.Name, err) + } + + // Try Basic Auth first (highest priority), then fall back to Docker config or legacy TLS + username, password, err := secrets.BasicAuthFromSecret(ctx, c, obj.Spec.SecretRef.Name, obj.GetNamespace()) + if err != nil { + // Basic Auth failed - check if it's a Docker config secret or legacy TLS-only secret + _, hasDockerConfig := authSecret.Data[".dockerconfigjson"] + _, hasCAFile := authSecret.Data["caFile"] + _, hasCertFile := authSecret.Data["certFile"] + _, hasKeyFile := authSecret.Data["keyFile"] + hasLegacyTLS := hasCAFile || hasCertFile || hasKeyFile + if !hasDockerConfig && !hasLegacyTLS { + // Not a Docker config or legacy TLS-only secret, so Basic Auth failure is an error + return false, err + } + // Docker config or legacy TLS-only secret - Basic Auth failure is expected, continue + } else { + hrOpts.GetterOpts = append(hrOpts.GetterOpts, helmgetter.WithBasicAuth(username, password)) + } + + // Setup legacy TLS if no dedicated cert secret was configured + deprecatedTLSConfig := false + if hrOpts.TlsConfig == nil && !ociRepo { + // TODO: Replace with runtime/secrets.TLSConfigFromSecret once migration is complete + tlsConfig, legacyTLSBytes, err := stls.LegacyTLSClientConfigFromSecret(*authSecret, url) + if err != nil { + return false, fmt.Errorf("failed to construct Helm client's TLS config: %w", err) + } + hrOpts.TlsConfig = tlsConfig + if tlsConfig != nil { + deprecatedTLSConfig = true + *tlsBytes = legacyTLSBytes + } + } + + // Setup OCI authentication if this is an OCI repository + if ociRepo { + keychain, err := registry.LoginOptionFromSecretRef(ctx, c, url, obj.Spec.SecretRef.Name, obj.GetNamespace()) + if err != nil { + return false, fmt.Errorf("failed to configure login options: %w", err) + } + hrOpts.Keychain = keychain + } + + return deprecatedTLSConfig, nil +} + +// setupOCIAuth configures OCI provider authentication when no secret is configured +func setupOCIAuth(ctx context.Context, obj *sourcev1.HelmRepository, hrOpts *ClientOpts, ociRepo bool) error { + // Only setup provider auth if no secret is configured and it's an OCI repo with a non-generic provider + if obj.Spec.SecretRef != nil || !ociRepo { + return nil + } + + provider := obj.Spec.Provider + if provider == "" || provider == sourcev1.GenericOCIProvider { + return nil + } + + authenticator, err := soci.OIDCAuth(ctx, obj.Spec.URL, provider) + if err != nil { + return fmt.Errorf("failed to get credential from '%s': %w", provider, err) + } + + hrOpts.Authenticator = authenticator + return nil +} + +// setupOCIRegistry configures OCI registry login options and certificate files +func setupOCIRegistry(hrOpts *ClientOpts, tlsBytes *stls.TLSBytes, url string, obj *sourcev1.HelmRepository, ociRepo bool) (string, error) { + + var dir string + var certFile, keyFile, caFile string + var err error + + // Persist the certs files to the path if needed + if tlsBytes != nil { + dir, err = os.MkdirTemp("", "helm-repo-oci-certs") + if err != nil { + return "", fmt.Errorf("cannot create temporary directory: %w", err) + } + certFile, keyFile, caFile, err = storeTLSCertificateFiles(tlsBytes, dir) + if err != nil { + return "", fmt.Errorf("cannot write certs files to path: %w", err) + } + } + + loginOpt, err := registry.NewLoginOption(hrOpts.Authenticator, hrOpts.Keychain, url) + if err != nil { + return "", err + } + + if loginOpt != nil { + hrOpts.RegLoginOpts = []helmreg.LoginOption{loginOpt, helmreg.LoginOptInsecure(obj.Spec.Insecure)} + tlsLoginOpt := registry.TLSLoginOption(certFile, keyFile, caFile) + if tlsLoginOpt != nil { + hrOpts.RegLoginOpts = append(hrOpts.RegLoginOpts, tlsLoginOpt) + } + } + + return dir, nil +} diff --git a/internal/helm/getter/getter.go b/internal/helm/getter/getter.go deleted file mode 100644 index 18661da16..000000000 --- a/internal/helm/getter/getter.go +++ /dev/null @@ -1,54 +0,0 @@ -/* -Copyright 2020 The Flux 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 getter - -import ( - "fmt" - - "helm.sh/helm/v3/pkg/getter" - corev1 "k8s.io/api/core/v1" -) - -// GetterOptionsFromSecret constructs a getter.Option slice for the given secret. -// It returns the slice, or an error. -func GetterOptionsFromSecret(secret corev1.Secret) ([]getter.Option, error) { - var opts []getter.Option - basicAuth, err := basicAuthFromSecret(secret) - if err != nil { - return opts, err - } - if basicAuth != nil { - opts = append(opts, basicAuth) - } - return opts, nil -} - -// basicAuthFromSecret attempts to construct a basic auth getter.Option for the -// given v1.Secret and returns the result. -// -// Secrets with no username AND password are ignored, if only one is defined it -// returns an error. -func basicAuthFromSecret(secret corev1.Secret) (getter.Option, error) { - username, password := string(secret.Data["username"]), string(secret.Data["password"]) - switch { - case username == "" && password == "": - return nil, nil - case username == "" || password == "": - return nil, fmt.Errorf("invalid '%s' secret data: required fields 'username' and 'password'", secret.Name) - } - return getter.WithBasicAuth(username, password), nil -} diff --git a/internal/helm/getter/getter_test.go b/internal/helm/getter/getter_test.go deleted file mode 100644 index cffe0064f..000000000 --- a/internal/helm/getter/getter_test.go +++ /dev/null @@ -1,93 +0,0 @@ -/* -Copyright 2020 The Flux 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 getter - -import ( - "testing" - - corev1 "k8s.io/api/core/v1" -) - -var ( - basicAuthSecretFixture = corev1.Secret{ - Data: map[string][]byte{ - "username": []byte("user"), - "password": []byte("password"), - }, - } -) - -func TestGetterOptionsFromSecret(t *testing.T) { - tests := []struct { - name string - secrets []corev1.Secret - }{ - {"basic auth", []corev1.Secret{basicAuthSecretFixture}}, - {"empty", []corev1.Secret{}}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - secret := corev1.Secret{Data: map[string][]byte{}} - for _, s := range tt.secrets { - for k, v := range s.Data { - secret.Data[k] = v - } - } - - got, err := GetterOptionsFromSecret(secret) - if err != nil { - t.Errorf("ClientOptionsFromSecret() error = %v", err) - return - } - if len(got) != len(tt.secrets) { - t.Errorf("ClientOptionsFromSecret() options = %v, expected = %v", got, len(tt.secrets)) - } - }) - } -} - -func Test_basicAuthFromSecret(t *testing.T) { - tests := []struct { - name string - secret corev1.Secret - modify func(secret *corev1.Secret) - wantErr bool - wantNil bool - }{ - {"username and password", basicAuthSecretFixture, nil, false, false}, - {"without username", basicAuthSecretFixture, func(s *corev1.Secret) { delete(s.Data, "username") }, true, true}, - {"without password", basicAuthSecretFixture, func(s *corev1.Secret) { delete(s.Data, "password") }, true, true}, - {"empty", corev1.Secret{}, nil, false, true}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - secret := tt.secret.DeepCopy() - if tt.modify != nil { - tt.modify(secret) - } - got, err := basicAuthFromSecret(*secret) - if (err != nil) != tt.wantErr { - t.Errorf("BasicAuthFromSecret() error = %v, wantErr %v", err, tt.wantErr) - return - } - if tt.wantNil && got != nil { - t.Error("BasicAuthFromSecret() != nil") - return - } - }) - } -} diff --git a/internal/helm/registry/auth.go b/internal/helm/registry/auth.go index c8b3ca6ae..49c79ac26 100644 --- a/internal/helm/registry/auth.go +++ b/internal/helm/registry/auth.go @@ -18,16 +18,20 @@ package registry import ( "bytes" + "context" + "errors" "fmt" "net/url" "github.com/docker/cli/cli/config" "github.com/docker/cli/cli/config/credentials" + "github.com/fluxcd/pkg/runtime/secrets" "github.com/fluxcd/source-controller/internal/helm/common" "github.com/fluxcd/source-controller/internal/oci" "github.com/google/go-containerregistry/pkg/authn" "helm.sh/helm/v3/pkg/registry" corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client" ) // helper is a subset of the Docker credential helper credentials.Helper interface used by NewKeychainFromHelper. @@ -48,6 +52,9 @@ func (h helper) Get(serverURL string) (string, string, error) { // may either hold "username" and "password" fields or be of the corev1.SecretTypeDockerConfigJson type and hold // a corev1.DockerConfigJsonKey field with a complete Docker configuration. If both, "username" and "password" are // empty, a nil LoginOption and a nil error will be returned. +// +// TODO: Consider consolidating this function into LoginOptionFromSecretRef to eliminate code duplication +// during a future refactoring. Currently kept separate for Docker config JSON handling reuse. func LoginOptionFromSecret(registryURL string, secret corev1.Secret) (authn.Keychain, error) { var username, password string parsedURL, err := url.Parse(registryURL) @@ -86,6 +93,35 @@ func LoginOptionFromSecret(registryURL string, secret corev1.Secret) (authn.Keyc return authn.NewKeychainFromHelper(helper{registry: parsedURL.Host, username: username, password: password}), nil } +// LoginOptionFromSecretRef derives authentication data using runtime/secrets. +func LoginOptionFromSecretRef(ctx context.Context, c client.Client, registryURL, secretName, namespace string) (authn.Keychain, error) { + parsedURL, err := url.Parse(registryURL) + if err != nil { + return nil, fmt.Errorf("unable to parse registry URL '%s' while reconciling Secret '%s/%s': %w", + registryURL, namespace, secretName, err) + } + + secretKey := client.ObjectKey{Name: secretName, Namespace: namespace} + var secret corev1.Secret + if err := c.Get(ctx, secretKey, &secret); err != nil { + return nil, fmt.Errorf("failed to get secret '%s/%s': %w", namespace, secretName, err) + } + + if secret.Type == corev1.SecretTypeDockerConfigJson { + return LoginOptionFromSecret(registryURL, secret) + } + + username, password, err := secrets.BasicAuthFromSecret(ctx, c, secretName, namespace) + if err != nil { + if errors.Is(err, secrets.ErrKeyNotFound) { + return oci.Anonymous{}, nil + } + return nil, err + } + + return authn.NewKeychainFromHelper(helper{registry: parsedURL.Host, username: username, password: password}), nil +} + // KeyChainAdaptHelper returns an ORAS credentials callback configured with the authorization data // from the given authn keychain. This allows for example to make use of credential helpers from // cloud providers. diff --git a/internal/helm/registry/auth_test.go b/internal/helm/registry/auth_test.go index 14942a5bb..517a24d69 100644 --- a/internal/helm/registry/auth_test.go +++ b/internal/helm/registry/auth_test.go @@ -17,16 +17,22 @@ limitations under the License. package registry import ( + "context" "net/url" "testing" "github.com/google/go-containerregistry/pkg/authn" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" ) const repoURL = "https://example.com" +// TODO: Consider consolidating this test with TestLoginOptionFromSecretRef to eliminate code duplication +// during a future refactoring. Currently kept separate to match the function separation. func TestLoginOptionFromSecret(t *testing.T) { testURL := "oci://registry.example.com/foo/bar" testUser := "flux" @@ -134,6 +140,93 @@ func TestLoginOptionFromSecret(t *testing.T) { } } +func TestLoginOptionFromSecretRef(t *testing.T) { + testURL := "oci://registry.example.com/foo/bar" + testUser := "flux" + testPassword := "somepassword" + testDockerconfigjson := `{"auths":{"registry.example.com":{"username":"flux","password":"somepassword","auth":"Zmx1eDpzb21lcGFzc3dvcmQ="}}}` + testDockerconfigjsonHTTPS := `{"auths":{"https://registry.example.com":{"username":"flux","password":"somepassword","auth":"Zmx1eDpzb21lcGFzc3dvcmQ="}}}` + + tests := []struct { + name string + url string + secret *corev1.Secret + wantErr bool + }{ + { + name: "generic secret", + url: testURL, + secret: newSecret(withGenericSecret(testUser, testPassword)), + }, + { + name: "generic secret without username", + url: testURL, + secret: newSecret(withPasswordOnly(testPassword)), + }, + { + name: "generic secret without password", + url: testURL, + secret: newSecret(withUsernameOnly(testUser)), + }, + { + name: "generic secret without username and password", + url: testURL, + secret: newSecret(withEmptyData()), + }, + { + name: "docker-registry secret", + url: testURL, + secret: newSecret(withDockerConfigSecret(testDockerconfigjson)), + }, + { + name: "docker-registry secret host mismatch", + url: "oci://registry.gitlab.com", + secret: newSecret(withDockerConfigSecret(testDockerconfigjson)), + wantErr: true, + }, + { + name: "docker-registry secret invalid host", + url: "oci://registry .gitlab.com", + secret: newSecret(withDockerConfigSecret(testDockerconfigjson)), + wantErr: true, + }, + { + name: "docker-registry secret invalid docker config", + url: testURL, + secret: newSecret(withDockerConfigSecret("foo")), + wantErr: true, + }, + { + name: "docker-registry secret with URL scheme", + url: testURL, + secret: newSecret(withDockerConfigSecret(testDockerconfigjsonHTTPS)), + }, + { + name: "secret not found", + url: testURL, + secret: nil, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + ctx := context.TODO() + + var objs []client.Object + if tt.secret != nil { + objs = append(objs, tt.secret) + } + + c := fakeclient.NewClientBuilder().WithObjects(objs...).Build() + + _, err := LoginOptionFromSecretRef(ctx, c, tt.url, "test-secret", "default") + g.Expect(err != nil).To(Equal(tt.wantErr)) + }) + } +} + func TestKeychainAdaptHelper(t *testing.T) { g := NewWithT(t) reg, err := url.Parse(repoURL) @@ -191,3 +284,62 @@ func TestKeychainAdaptHelper(t *testing.T) { }) } } + +type secretOption func(*corev1.Secret) + +func withGenericSecret(username, password string) secretOption { + return func(s *corev1.Secret) { + s.Type = corev1.SecretTypeOpaque + s.Data = map[string][]byte{ + "username": []byte(username), + "password": []byte(password), + } + } +} + +func withUsernameOnly(username string) secretOption { + return func(s *corev1.Secret) { + s.Type = corev1.SecretTypeOpaque + s.Data = map[string][]byte{ + "username": []byte(username), + } + } +} + +func withPasswordOnly(password string) secretOption { + return func(s *corev1.Secret) { + s.Type = corev1.SecretTypeOpaque + s.Data = map[string][]byte{ + "password": []byte(password), + } + } +} + +func withDockerConfigSecret(dockerConfig string) secretOption { + return func(s *corev1.Secret) { + s.Type = corev1.SecretTypeDockerConfigJson + s.Data = map[string][]byte{ + ".dockerconfigjson": []byte(dockerConfig), + } + } +} + +func withEmptyData() secretOption { + return func(s *corev1.Secret) { + s.Type = corev1.SecretTypeOpaque + s.Data = map[string][]byte{} + } +} + +func newSecret(opts ...secretOption) *corev1.Secret { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-secret", + Namespace: "default", + }, + } + for _, opt := range opts { + opt(secret) + } + return secret +} diff --git a/pkg/gcp/gcp.go b/pkg/gcp/gcp.go index 936c7587a..68ba1270d 100644 --- a/pkg/gcp/gcp.go +++ b/pkg/gcp/gcp.go @@ -27,6 +27,7 @@ import ( "path/filepath" gcpstorage "cloud.google.com/go/storage" + "github.com/fluxcd/pkg/runtime/secrets" "github.com/go-logr/logr" "golang.org/x/oauth2/google" "google.golang.org/api/iterator" @@ -151,7 +152,7 @@ func ValidateSecret(secret *corev1.Secret) error { return nil } if _, exists := secret.Data["serviceaccount"]; !exists { - return fmt.Errorf("invalid '%s' secret data: required fields 'serviceaccount'", secret.Name) + return &secrets.KeyNotFoundError{Key: "serviceaccount", Secret: secret} } return nil } diff --git a/pkg/gcp/gcp_test.go b/pkg/gcp/gcp_test.go index aa252324c..e012c656b 100644 --- a/pkg/gcp/gcp_test.go +++ b/pkg/gcp/gcp_test.go @@ -356,7 +356,7 @@ func TestValidateSecret(t *testing.T) { t.Parallel() err := ValidateSecret(tt.secret) if tt.error { - assert.Error(t, err, fmt.Sprintf("invalid '%v' secret data: required fields 'serviceaccount'", tt.secret.Name)) + assert.Error(t, err, fmt.Sprintf("secret '%s/%s': key 'serviceaccount' not found", tt.secret.Namespace, tt.secret.Name)) } else { assert.NilError(t, err) } diff --git a/pkg/minio/minio.go b/pkg/minio/minio.go index 6c7da9727..eb63fc7e0 100644 --- a/pkg/minio/minio.go +++ b/pkg/minio/minio.go @@ -23,8 +23,8 @@ import ( "fmt" "net/http" "net/url" - "strings" + "github.com/fluxcd/pkg/runtime/secrets" "github.com/minio/minio-go/v7" "github.com/minio/minio-go/v7/pkg/credentials" "github.com/minio/minio-go/v7/pkg/s3utils" @@ -221,12 +221,11 @@ func ValidateSecret(secret *corev1.Secret) error { if secret == nil { return nil } - err := fmt.Errorf("invalid '%s' secret data: required fields 'accesskey' and 'secretkey'", secret.Name) if _, ok := secret.Data["accesskey"]; !ok { - return err + return &secrets.KeyNotFoundError{Key: "accesskey", Secret: secret} } if _, ok := secret.Data["secretkey"]; !ok { - return err + return &secrets.KeyNotFoundError{Key: "secretkey", Secret: secret} } return nil } @@ -282,15 +281,13 @@ func validateSTSSecretForProvider(stsProvider string, secret *corev1.Secret, key if secret == nil { return nil } - err := fmt.Errorf("invalid '%s' secret data for '%s' STS provider: required fields %s", - secret.Name, stsProvider, strings.Join(keys, ", ")) - if len(secret.Data) == 0 { - return err + if len(secret.Data) == 0 && len(keys) > 0 { + return &secrets.KeyNotFoundError{Key: keys[0], Secret: secret} } for _, key := range keys { value, ok := secret.Data[key] if !ok || len(value) == 0 { - return err + return &secrets.KeyNotFoundError{Key: key, Secret: secret} } } return nil diff --git a/pkg/minio/minio_test.go b/pkg/minio/minio_test.go index 596e61810..7251a0b7d 100644 --- a/pkg/minio/minio_test.go +++ b/pkg/minio/minio_test.go @@ -573,7 +573,7 @@ func TestValidateSecret(t *testing.T) { t.Parallel() err := ValidateSecret(tt.secret) if tt.error { - assert.Error(t, err, fmt.Sprintf("invalid '%v' secret data: required fields 'accesskey' and 'secretkey'", tt.secret.Name)) + assert.Error(t, err, "secret 'default/minio-secret': key 'accesskey' not found") } else { assert.NilError(t, err) } @@ -696,50 +696,54 @@ func TestValidateSTSSecret(t *testing.T) { { name: "empty ldap secret", provider: "ldap", - secret: &corev1.Secret{ObjectMeta: v1.ObjectMeta{Name: "ldap-secret"}}, - err: "invalid 'ldap-secret' secret data for 'ldap' STS provider: required fields username, password", + secret: &corev1.Secret{ObjectMeta: v1.ObjectMeta{Name: "ldap-secret", Namespace: "default"}}, + err: "secret 'default/ldap-secret': key 'username' not found", }, { name: "ldap secret missing password", provider: "ldap", secret: &corev1.Secret{ + ObjectMeta: v1.ObjectMeta{Name: "ldap-secret", Namespace: "default"}, Data: map[string][]byte{ "username": []byte("user"), }, }, - err: "invalid '' secret data for 'ldap' STS provider: required fields username, password", + err: "secret 'default/ldap-secret': key 'password' not found", }, { name: "ldap secret missing username", provider: "ldap", secret: &corev1.Secret{ + ObjectMeta: v1.ObjectMeta{Name: "ldap-secret", Namespace: "default"}, Data: map[string][]byte{ "password": []byte("pass"), }, }, - err: "invalid '' secret data for 'ldap' STS provider: required fields username, password", + err: "secret 'default/ldap-secret': key 'username' not found", }, { name: "ldap secret with empty username", provider: "ldap", secret: &corev1.Secret{ + ObjectMeta: v1.ObjectMeta{Name: "ldap-secret", Namespace: "default"}, Data: map[string][]byte{ "username": []byte(""), "password": []byte("pass"), }, }, - err: "invalid '' secret data for 'ldap' STS provider: required fields username, password", + err: "secret 'default/ldap-secret': key 'username' not found", }, { name: "ldap secret with empty password", provider: "ldap", secret: &corev1.Secret{ + ObjectMeta: v1.ObjectMeta{Name: "ldap-secret", Namespace: "default"}, Data: map[string][]byte{ "username": []byte("user"), "password": []byte(""), }, }, - err: "invalid '' secret data for 'ldap' STS provider: required fields username, password", + err: "secret 'default/ldap-secret': key 'password' not found", }, }