Enable contextual login in OCI HelmRepository

[souleb]

If implemented, this pr will enable user to use the auto login feature in order to automatically login to their provider of choice's container registry (i.e. aws, gcr, acr).

This must be tested with the following providers

• aws. This has been tested on an eks cluster (with IRSA) that was previously using a cronjob to rotate tokens.
• gcr. This has been tested on gke with a chart in artifact registry.
• acr. Tested by @darkowlzz successfully.

If people have existing running gke or aks clusters, that would be nice to help test this.

Fix: #867
Fix: #870
Implements: fluxcd/flux2#3025

[darkowlzz]

I tested it on azure using the test infrastructure from fluxcd/pkg/oci/tests/integration. It works.

[stefanprodan]

We need to document the contextual login usage. I think the docs can be adapted from https://github.com/fluxcd/source-controller/blob/main/docs/spec/v1beta2/ocirepositories.md#provider

[stefanprodan]

LGTM

Thanks @souleb 🥇

[pjbgf]

Tiny nit

[darkowlzz]

While testing events and notifications, I found an issue which I believe didn't get much attention earlier because most of the focus was on getting the status conditions correct.
The line

source-controller/controllers/helmrepository_controller_oci.go

Lines 249 to 250 in 6afb263

	// Became not ready from ready.
	if conditions.IsReady(oldObj) && !conditions.IsReady(obj) {

checks if the object was ready before and became not ready after the reconciliation, and emits a notification based on that. This works for that exact situation, when the object was ready before. But it doesn't work when the object is created for the first time.
Let's change this to notify always when it's not ready but notify once on recovery.
Something around:

-               // Emit events when object's state changes.
+               // Emit event when reconciliation fails or recovers from failure.
                ready := conditions.Get(obj, meta.ReadyCondition)
                // Became ready from not ready.
                if !conditions.IsReady(oldObj) && conditions.IsReady(obj) {
                        r.eventLogf(ctx, obj, corev1.EventTypeNormal, ready.Reason, ready.Message)
                }
-               // Became not ready from ready.
-               if conditions.IsReady(oldObj) && !conditions.IsReady(obj) {
+               // Not ready, failed.
+               if !conditions.IsReady(obj) {

[souleb]

While testing events and notifications, I found an issue which I believe didn't get much attention earlier because most of the focus was on getting the status conditions correct. The line

source-controller/controllers/helmrepository_controller_oci.go

Lines 249 to 250 in 6afb263

	// Became not ready from ready.
	if conditions.IsReady(oldObj) && !conditions.IsReady(obj) {

checks if the object was ready before and became not ready after the reconciliation, and emits a notification based on that. This works for that exact situation, when the object was ready before. But it doesn't work when the object is created for the first time.
Let's change this to notify always when it's not ready but notify once on recovery.
Something around:

-               // Emit events when object's state changes.
+               // Emit event when reconciliation fails or recovers from failure.
                ready := conditions.Get(obj, meta.ReadyCondition)
                // Became ready from not ready.
                if !conditions.IsReady(oldObj) && conditions.IsReady(obj) {
                        r.eventLogf(ctx, obj, corev1.EventTypeNormal, ready.Reason, ready.Message)
                }
-               // Became not ready from ready.
-               if conditions.IsReady(oldObj) && !conditions.IsReady(obj) {
+               // Not ready, failed.
+               if !conditions.IsReady(obj) {

can we do this in another PR? I will make the update.

[darkowlzz]

can we do this in another PR? I will make the update.

@souleb okay.

[pjbgf]

LGTM

great job @souleb! 🙇

[darkowlzz]

The code in this function combined with loginWithManager() below, it seems like registry.OIDCAdaptHelper() can just take the result of OCIRepositoryReconciler.oidcAuth(). The arguments of OCIRepositoryReconciler.oidcAuth() can just be the URL and the provider name to not make it source type specific. It should be okay to convert OCIRepositoryReconciler.oidcAuth() into an independent function oidcAuth().

In the future, I think we can make a private package with a lot of the OCI related code here and in OCIRepo reconciler.

[souleb]

true, it would be better. But as you said OCIRepositoryReconciler.oidcAuth() is a receiver function, so I thought that this would need a specific PR to refactor it.

[darkowlzz]

okay to change it separately.

[darkowlzz]

LGTM!

[darkowlzz]

Just remembered that when testing with both spec.secretRef and spec.provider set, the provider login option overwrites the secretRef login option. For example, if I create a secret with incorrect credentials and set the provider correctly, the result is a Ready HelmRepository. As per the RFC, https://github.com/fluxcd/flux2/blob/80669d71ef679b2a3018a958e1ac1a30c8a628f5/rfcs/0002-helm-oci/README.md?plain=1#L92-L93

If both spec.secretRef and a non-generic provider are present in the definition,
the controller will use the static credentials from the referenced secret.

it seems to be a different behavior.
But if the referred secret doesn't exist, then the reconciliation fails with secret not found error.

Looking at the code in OCIRepo reconciler, I believe it does the same.

[stefanprodan]

if I create a secret with incorrect credentials and set the provider correctly, the result is a Ready

Let's fix this for both OCI implementations in a separate PR.