From 74242e5d8a46888760a613c1fb24883da7c5e563 Mon Sep 17 00:00:00 2001 From: cappyzawa Date: Sat, 19 Jul 2025 00:34:00 +0900 Subject: [PATCH 1/2] Migrate OCIRepository controller to runtime/secrets Migrates the OCIRepository controller's authentication handling from internal implementations to the unified runtime/secrets API package. The migration moves TLS configuration from internal/tls to runtime/secrets.TLSConfigFromSecretRef and ServiceAccount processing to secrets.PullSecretsFromServiceAccountRef, providing consistent authentication handling across all source-controller components. This change eliminates duplicate secret fetching logic and aligns the OCIRepository controller with the standardized authentication patterns used by other controllers in the GitOps Toolkit. Signed-off-by: cappyzawa --- go.mod | 2 +- go.sum | 4 +- .../helmrepository_controller_test.go | 6 +- .../controller/ocirepository_controller.go | 120 +++-------- .../ocirepository_controller_test.go | 189 +----------------- 5 files changed, 40 insertions(+), 281 deletions(-) diff --git a/go.mod b/go.mod index 12a42d0b9..a713b6b8b 100644 --- a/go.mod +++ b/go.mod @@ -38,7 +38,7 @@ require ( github.com/fluxcd/pkg/lockedfile v0.6.0 github.com/fluxcd/pkg/masktoken v0.7.0 github.com/fluxcd/pkg/oci v0.51.0 - github.com/fluxcd/pkg/runtime v0.73.0 + github.com/fluxcd/pkg/runtime v0.75.0 github.com/fluxcd/pkg/sourceignore v0.13.0 github.com/fluxcd/pkg/ssh v0.20.0 github.com/fluxcd/pkg/tar v0.13.0 diff --git a/go.sum b/go.sum index 848fb49fa..e5204a362 100644 --- a/go.sum +++ b/go.sum @@ -398,8 +398,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.51.0 h1:9oYnm+T4SCVSBif9gn80ALJkMGSERabVMDJiaMIdr7Y= github.com/fluxcd/pkg/oci v0.51.0/go.mod h1:5J6IhHoDVYCVeBEC+4E3nPeKh7d0kjJ8IEL6NVCiTx4= -github.com/fluxcd/pkg/runtime v0.73.0 h1:BV3qEwMT3lfHA2lterT3Es62z6EkJr2ST/jkyBmmskQ= -github.com/fluxcd/pkg/runtime v0.73.0/go.mod h1:iGhdaEq+lMJQTJNAFEPOU4gUJ7kt3yeDcJPZy7O9IUw= +github.com/fluxcd/pkg/runtime v0.75.0 h1:wIaODmU5D54nyrehTqA9oQDFoi6BbBj/24adLStXc0I= +github.com/fluxcd/pkg/runtime v0.75.0/go.mod h1:iGhdaEq+lMJQTJNAFEPOU4gUJ7kt3yeDcJPZy7O9IUw= github.com/fluxcd/pkg/sourceignore v0.13.0 h1:ZvkzX2WsmyZK9cjlqOFFW1onHVzhPZIqDbCh96rPqbU= github.com/fluxcd/pkg/sourceignore v0.13.0/go.mod h1:Z9H1GoBx0ljOhptnzoV0PL6Nd/UzwKcSphP27lqb4xI= github.com/fluxcd/pkg/ssh v0.20.0 h1:Ak0laIYIc/L8lEfqls/LDWRW8wYPESGaravQsCRGLb8= diff --git a/internal/controller/helmrepository_controller_test.go b/internal/controller/helmrepository_controller_test.go index 8beb0850f..c0a5b3357 100644 --- a/internal/controller/helmrepository_controller_test.go +++ b/internal/controller/helmrepository_controller_test.go @@ -482,7 +482,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { repoURL, err := repository.NormalizeURL(serverURL) t.Expect(err).ToNot(HaveOccurred()) - tlsConfig, err := secrets.TLSConfigFromSecret(context.TODO(), secret) + tlsConfig, err := secrets.TLSConfigFromSecret(context.TODO(), secret, serverURL, false) t.Expect(err).ToNot(HaveOccurred()) getterOpts := []helmgetter.Option{ @@ -534,7 +534,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { repoURL, err := repository.NormalizeURL(serverURL) t.Expect(err).ToNot(HaveOccurred()) - tlsConfig, err := secrets.TLSConfigFromSecret(context.TODO(), secret) + tlsConfig, err := secrets.TLSConfigFromSecret(context.TODO(), secret, serverURL, false) t.Expect(err).ToNot(HaveOccurred()) getterOpts := []helmgetter.Option{ @@ -588,7 +588,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { repoURL, err := repository.NormalizeURL(serverURL) t.Expect(err).ToNot(HaveOccurred()) - tlsConfig, err := secrets.TLSConfigFromSecret(context.TODO(), secret) + tlsConfig, err := secrets.TLSConfigFromSecret(context.TODO(), secret, serverURL, false) t.Expect(err).ToNot(HaveOccurred()) getterOpts := []helmgetter.Option{ diff --git a/internal/controller/ocirepository_controller.go b/internal/controller/ocirepository_controller.go index ed407c201..9d9ca18d6 100644 --- a/internal/controller/ocirepository_controller.go +++ b/internal/controller/ocirepository_controller.go @@ -43,7 +43,6 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/sets" kuberecorder "k8s.io/client-go/tools/record" "k8s.io/client-go/util/workqueue" "k8s.io/utils/ptr" @@ -60,6 +59,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 +77,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 +354,21 @@ 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.ProxyURLFromSecretRef(ctx, r.Client, types.NamespacedName{ + Name: obj.Spec.ProxySecretRef.Name, + Namespace: 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 { @@ -920,44 +926,36 @@ func (r *OCIRepositoryReconciler) getTagBySemver(repo name.Repository, exp strin // configuration. If no auth is specified a default keychain with // anonymous access is returned func (r *OCIRepositoryReconciler) keychain(ctx context.Context, obj *sourcev1.OCIRepository) (authn.Keychain, error) { - pullSecretNames := sets.NewString() + var imagePullSecrets []corev1.Secret // lookup auth secret if obj.Spec.SecretRef != nil { - pullSecretNames.Insert(obj.Spec.SecretRef.Name) + var imagePullSecret corev1.Secret + secretRef := types.NamespacedName{Namespace: obj.Namespace, Name: obj.Spec.SecretRef.Name} + err := r.Get(ctx, secretRef, &imagePullSecret) + if err != nil { + r.eventLogf(ctx, obj, eventv1.EventTypeTrace, sourcev1.AuthenticationFailedReason, + "auth secret '%s' not found", obj.Spec.SecretRef.Name) + return nil, err + } + imagePullSecrets = append(imagePullSecrets, imagePullSecret) } // lookup service account if obj.Spec.ServiceAccountName != "" { - serviceAccountName := obj.Spec.ServiceAccountName - serviceAccount := corev1.ServiceAccount{} - err := r.Get(ctx, types.NamespacedName{Namespace: obj.Namespace, Name: serviceAccountName}, &serviceAccount) + saRef := types.NamespacedName{Namespace: obj.Namespace, Name: obj.Spec.ServiceAccountName} + saSecrets, err := secrets.PullSecretsFromServiceAccountRef(ctx, r.Client, saRef) if err != nil { return nil, err } - for _, ips := range serviceAccount.ImagePullSecrets { - pullSecretNames.Insert(ips.Name) - } + imagePullSecrets = append(imagePullSecrets, saSecrets...) } // if no pullsecrets available return an AnonymousKeychain - if len(pullSecretNames) == 0 { + if len(imagePullSecrets) == 0 { return soci.Anonymous{}, nil } - // lookup image pull secrets - imagePullSecrets := make([]corev1.Secret, len(pullSecretNames)) - for i, imagePullSecretName := range pullSecretNames.List() { - imagePullSecret := corev1.Secret{} - err := r.Get(ctx, types.NamespacedName{Namespace: obj.Namespace, Name: imagePullSecretName}, &imagePullSecret) - if err != nil { - r.eventLogf(ctx, obj, eventv1.EventTypeTrace, sourcev1.AuthenticationFailedReason, - "auth secret '%s' not found", imagePullSecretName) - return nil, err - } - imagePullSecrets[i] = imagePullSecret - } - return k8schain.NewFromPullSecrets(ctx, imagePullSecrets) } @@ -995,65 +993,11 @@ func (r *OCIRepositoryReconciler) getTLSConfig(ctx context.Context, obj *sourcev return nil, nil } - certSecretName := types.NamespacedName{ + secretName := 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, "") - 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") - } - } - - 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 + return secrets.TLSConfigFromSecretRef(ctx, r.Client, secretName, obj.Spec.URL, obj.Spec.Insecure) } // reconcileStorage ensures the current state of the storage matches the 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)) - } - }) - } -} From b2993a76bceffefe888bb549771830d80563b10e Mon Sep 17 00:00:00 2001 From: cappyzawa Date: Sat, 19 Jul 2025 21:25:09 +0900 Subject: [PATCH 2/2] Fix missing TLS ServerName in HelmRepository Add ServerName configuration to TLS config in HelmRepository client options to ensure proper SNI (Server Name Indication) support for virtual hosting environments. This addresses the regression introduced when migrating from internal/tls to runtime/secrets, where ServerName was not being set automatically. Without ServerName, TLS handshakes fail with certificate mismatch errors when connecting to Helm repositories using virtual hosting where multiple repositories are hosted on the same IP address. Signed-off-by: cappyzawa --- internal/helm/getter/client_opts.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/helm/getter/client_opts.go b/internal/helm/getter/client_opts.go index 0c5eaf0cb..cbcd09d9d 100644 --- a/internal/helm/getter/client_opts.go +++ b/internal/helm/getter/client_opts.go @@ -122,7 +122,7 @@ func configureAuthentication(ctx context.Context, c client.Client, obj *sourcev1 } certSecret = secret - tlsConfig, err := secrets.TLSConfigFromSecret(ctx, secret) + tlsConfig, err := secrets.TLSConfigFromSecret(ctx, secret, obj.Spec.URL, obj.Spec.Insecure) if err != nil { return false, nil, nil, fmt.Errorf("failed to construct Helm client's TLS config: %w", err) } @@ -138,7 +138,7 @@ func configureAuthentication(ctx context.Context, c client.Client, obj *sourcev1 } authSecret = secret - methods, err := secrets.AuthMethodsFromSecret(ctx, secret) + methods, err := secrets.AuthMethodsFromSecret(ctx, secret, secrets.WithTLS(obj.Spec.URL, obj.Spec.Insecure)) if err != nil { return false, nil, nil, fmt.Errorf("failed to detect authentication methods: %w", err) }