Migrate OCIRepository controller to runtime/secrets

[cappyzawa]

Summary

This PR migrates OCIRepository authentication to use runtime/secrets API as part of fluxcd/flux2#5433.

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.

[matheuscscp]

Proxy is missing 😁

[matheuscscp]

diff --git a/internal/controller/ocirepository_controller.go b/internal/controller/ocirepository_controller.go
index 97ae1b1..af54e33 100644
--- a/internal/controller/ocirepository_controller.go
+++ b/internal/controller/ocirepository_controller.go
@@ -354,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 {

Then func (r *OCIRepositoryReconciler) getProxyURL() can be removed

[cappyzawa]

@matheuscscp Good catch! I missed that. I'll take care of it!

[cappyzawa]

@matheuscscp Done! The codebase is much smaller now—lots of lines removed!

[stefanprodan]

What about

source-controller/internal/tls/config.go

Lines 124 to 126 in 173a1cc

	tlsConf := &tls.Config{
	MinVersion: tls.VersionTLS12,
	}

And

source-controller/internal/tls/config.go

Lines 147 to 154 in 173a1cc

	if url != "" {
	u, err := neturl.Parse(url)
	if err != nil {
	return nil, nil, fmt.Errorf("cannot parse repository URL: %w", err)
	}
	tlsConf.ServerName = u.Hostname()
	}

[matheuscscp]

@stefanprodan Would it make sense to add those to runtime/secrets and have them everywhere?

Not sure how to go about the url, since the inputs for the library TLS config are just the Secret object or reference...

[stefanprodan]

We should get rid of internal/tls/config.go and looks like 1.2 is now the min default https://go-review.googlesource.com/c/go/+/541516

[stefanprodan]

Would it make sense to add those to runtime/secrets and have them everywhere?

Yes for the min TLS version

Not sure how to go about the url, since the inputs for the library TLS config are just the Secret object or reference

I think the URL validation happen for OCIRepo and HelmRepo OCI no matter if TLS is set or not. For sure it doesn't belong in runtime/secrets

[matheuscscp]

ImageRepository did not have this tlsConf.ServerName = u.Hostname() before the migration to runtime/secrets. What's the effect of this assignment? If we remove this are we breaking any setups?

	// ServerName is used to verify the hostname on the returned
	// certificates unless InsecureSkipVerify is given. It is also included
	// in the client's handshake to support virtual hosting unless it is
	// an IP address.
	ServerName string

This tells me we would breaking a feature:

It is also included in the client's handshake to support virtual hosting unless it is an IP address.

Edit: Is this a feature missing in ImageRepository?

[cappyzawa]

@stefanprodan Thanks for pointing out those missing parts!

Regarding TLS MinVersion: Go 1.20+ defaults to TLS 1.2 as you mentioned, so I believe the explicit setting is no longer necessary. I didn't add it to maintain consistency with the Go defaults.

Regarding ServerName: You're right - this was a critical regression in the migration from internal/tls to runtime/secrets. Without ServerName, TLS handshakes fail with certificate mismatch errors in virtual hosting environments. I've fixed this in 26abfa9 for OCIRepository.

Additionally, I discovered that #1849 had the same ServerName issue, which I've also addressed in 06b3d72.

[cappyzawa]

Regarding removing internal/tls/config.go: I agree! Once we migrate the Bucket controller to runtime/secrets in a follow-up PR, there should be no more dependencies on internal/tls/config.go, and we can remove it completely at that time.

[matheuscscp]

For consistency we should probably also implement ServerName for ImageRepository since both OCI APIs in source-controller have this feature

[matheuscscp]

LGTM! 🚀

[stefanprodan]

LGTM

Thanks @cappyzawa 🏅

[cappyzawa]

@matheuscscp @stefanprodan Thanks for your approval!

Since we’re already discussing changes to the pkg in fluxcd/flux2#5433, I’d like to wait and incorporate those updates before merging this PR, if that’s okay 🙏

[matheuscscp]

LGTM! 🚀

[cappyzawa]

pkg/runtime v0.75.0 is released: fluxcd/pkg#991

This release resolves the missing ServerName in tls.Config.
I addressed the OCI Repository in e0bd19c, and handled the Helm Repository in 23729ce.

[cappyzawa]

Got the approvals, so I'll go ahead and squash!

[stefanprodan]

LGTM