Add OCI Helm Repositories section to the doc

This make sure to provide a new registryClient for every reconciliation.

Various other comments addressed.

Signed-off-by: Soule BA <soule@weave.works>

Author: souleb

api/v1beta1/helmrepository_types.go

@@ -30,11 +30,6 @@ const (
 	// HelmRepositoryURLIndexKey is the key to use for indexing HelmRepository
 	// resources by their HelmRepositorySpec.URL.
 	HelmRepositoryURLIndexKey = ".metadata.helmRepositoryURL"
-	// HelmRepositoryTypeDefault is the default HelmRepository type.
-	// It is used when no type is specified and corresponds to a Helm repository.
-	HelmRepositoryTypeDefault = "default"
-	// HelmRepositoryTypeOCI is the type for an OCI repository.
-	HelmRepositoryTypeOCI = "oci"
 )
 
 // HelmRepositorySpec defines the reference to a Helm repository.
@@ -77,12 +72,6 @@ type HelmRepositorySpec struct {
 	// AccessFrom defines an Access Control List for allowing cross-namespace references to this object.
 	// +optional
 	AccessFrom *acl.AccessFrom `json:"accessFrom,omitempty"`
-
-	// Type of the HelmRepository.
-	// When this field is set to  "oci", the URL field value must be prefixed with "oci://".
-	// +kubebuilder:validation:Enum=default;oci
-	// +optional
-	Type string `json:"type,omitempty"`
 }
 
 // HelmRepositoryStatus defines the observed state of the HelmRepository.

api/v1beta2/condition_types.go

@@ -39,10 +39,6 @@ const (
 	// is enabled.
 	SourceVerifiedCondition string = "SourceVerified"
 
-	//SourceValidCondition indicates the validity of the Source.
-	// If True, the Source is valid. If False, it is not valid.
-	SourceValidCondition string = "SourceValid"
-
 	// FetchFailedCondition indicates a transient or persistent fetch failure
 	// of an upstream Source.
 	// If True, observations on the upstream Source revision may be impossible,

config/crd/bases/source.toolkit.fluxcd.io_helmrepositories.yaml

@@ -109,13 +109,6 @@ spec:
                 default: 60s
                 description: The timeout of index downloading, defaults to 60s.
                 type: string
-              type:
-                description: Type of the HelmRepository. When this field is set to  "oci",
-                  the URL field value must be prefixed with "oci://".
-                enum:
-                - default
-                - oci
-                type: string
               url:
                 description: The Helm repository URL, a valid URL contains at least
                   a protocol and host.

controllers/helmchart_controller.go

@@ -117,10 +117,10 @@ type HelmChartReconciler struct {
 	kuberecorder.EventRecorder
 	helper.Metrics
 
-	RegistryClient *registry.Client
-	Storage        *Storage
-	Getters        helmgetter.Providers
-	ControllerName string
+	RegistryClientGenerator RegistryClientGeneratorFunc
+	Storage                 *Storage
+	Getters                 helmgetter.Providers
+	ControllerName          string
 
 	Cache *cache.Cache
 	TTL   time.Duration
@@ -508,14 +508,31 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 
 	// Initialize the chart repository
 	var chartRepo chart.Remote
-	if repo.Spec.Type == sourcev1.HelmRepositoryTypeOCI {
+	switch repo.Spec.Type {
+	case sourcev1.HelmRepositoryTypeOCI:
 		if !registry.IsOCI(repo.Spec.URL) {
 			err := fmt.Errorf("invalid OCI registry URL: %s", repo.Spec.URL)
 			return chartRepoErrorReturn(err, obj)
 		}
+
+		// with this function call, we create a temporary file to store the credentials if needed.
+		// this is needed because otherwise the credentials are stored in ~/.docker/config.json.
+		// TODO@souleb: remove this once the registry move to Oras v2
+		// or rework to enable reusing credentials to avoid the unneccessary handshake operations
+		registryClient, file, err := r.RegistryClientGenerator(logOpts != nil)
+		if err != nil {
+			return chartRepoErrorReturn(err, obj)
+		}
+
+		defer func() {
+			if file != "" {
+				os.Remove(file)
+			}
+		}()
+
 		// Tell the chart repository to use the OCI client with the configured getter
-		clientOpts = append(clientOpts, helmgetter.WithRegistryClient(r.RegistryClient))
-		ociChartRepo, err := repository.NewOCIChartRepository(repo.Spec.URL, repository.WithOCIGetter(r.Getters), repository.WithOCIGetterOptions(clientOpts), repository.WithOCIRegistryClient(r.RegistryClient))
+		clientOpts = append(clientOpts, helmgetter.WithRegistryClient(registryClient))
+		ociChartRepo, err := repository.NewOCIChartRepository(repo.Spec.URL, repository.WithOCIGetter(r.Getters), repository.WithOCIGetterOptions(clientOpts), repository.WithOCIRegistryClient(registryClient))
 		if err != nil {
 			return chartRepoErrorReturn(err, obj)
 		}
@@ -524,24 +541,12 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 		// If login options are configured, use them to login to the registry
 		// The OCIGetter will later retrieve the stored credentials to pull the chart
 		if logOpts != nil {
-			// create a temporary file to store the credentials
-			// this is needed because otherwise the credentials are stored in ~/.docker/config.json.
-			// TODO@souleb: remove this once the registry move to Oras v2
-			// or rework to enable reusing credentials to avoid the unneccessary handshake operations
-			credentialFile, err := os.CreateTemp("", "credentials")
-			if err != nil {
-				return chartRepoErrorReturn(err, obj)
-			}
-			defer os.Remove(credentialFile.Name())
-
-			// set the credentials file to the registry client
-			registry.ClientOptCredentialsFile(credentialFile.Name())(r.RegistryClient)
 			err = ociChartRepo.Login(logOpts...)
 			if err != nil {
 				return chartRepoErrorReturn(err, obj)
 			}
 		}
-	} else {
+	default:
 		var httpChartRepo *repository.ChartRepository
 		httpChartRepo, err := repository.NewChartRepository(repo.Spec.URL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, tlsConfig, clientOpts,
 			repository.WithMemoryCache(r.Storage.LocalPath(*repo.GetArtifact()), r.Cache, r.TTL, func(event string) {

controllers/helmchart_controller_test.go

@@ -942,12 +942,16 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
 				clientBuilder.WithObjects(tt.secret.DeepCopy())
 			}
 
+			registryClientGenerator := func(_ bool) (*registry.Client, string, error) {
+				return testRegistryClient, "", nil
+			}
+
 			r := &HelmChartReconciler{
-				Client:         clientBuilder.Build(),
-				EventRecorder:  record.NewFakeRecorder(32),
-				Getters:        testGetters,
-				Storage:        storage,
-				RegistryClient: testRegistryClient,
+				Client:                  clientBuilder.Build(),
+				EventRecorder:           record.NewFakeRecorder(32),
+				Getters:                 testGetters,
+				Storage:                 storage,
+				RegistryClientGenerator: registryClientGenerator,
 			}
 
 			repository := &sourcev1.HelmRepository{

controllers/helmrepository_controller_oci.go

@@ -49,14 +49,12 @@ var helmRepositoryOCIReadyCondition = summarize.Conditions{
 	Target: meta.ReadyCondition,
 	Owned: []string{
 		sourcev1.FetchFailedCondition,
-		sourcev1.SourceValidCondition,
 		meta.ReadyCondition,
 		meta.ReconcilingCondition,
 		meta.StalledCondition,
 	},
 	Summarize: []string{
 		sourcev1.FetchFailedCondition,
-		sourcev1.SourceValidCondition,
 		meta.StalledCondition,
 		meta.ReconcilingCondition,
 	},
@@ -83,11 +81,17 @@ type HelmRepositoryOCIReconciler struct {
 	client.Client
 	kuberecorder.EventRecorder
 	helper.Metrics
-	Getters        helmgetter.Providers
-	ControllerName string
-	RegistryClient *registry.Client
+	Getters                 helmgetter.Providers
+	ControllerName          string
+	RegistryClientGenerator RegistryClientGeneratorFunc
 }
 
+// RegistryClientGeneratorFunc is a function that returns a registry client
+// and an optional file name.
+// The file is used to store the registry client credentials.
+// The caller is responsible for deleting the file.
+type RegistryClientGeneratorFunc func(isLogin bool) (*registry.Client, string, error)
+
 // helmRepositoryOCIReconcileFunc is the function type for all the
 // v1beta2.HelmRepository (sub)reconcile functions for OCI type. The type implementations
 // are grouped and executed serially to perform the complete reconcile of the
@@ -283,55 +287,56 @@ func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj *
 
 // validateSource the HelmRepository object by checking the url and connecting to the underlying registry
 // with he provided credentials.
-func (r *HelmRepositoryOCIReconciler) validateSource(ctx context.Context, obj *sourcev1.HelmRepository, loginOpts ...registry.LoginOption) (sreconcile.Result, error) {
-	chartRepo, err := repository.NewOCIChartRepository(obj.Spec.URL, repository.WithOCIRegistryClient(r.RegistryClient))
+func (r *HelmRepositoryOCIReconciler) validateSource(ctx context.Context, obj *sourcev1.HelmRepository, logOpts ...registry.LoginOption) (sreconcile.Result, error) {
+	registryClient, file, err := r.RegistryClientGenerator(logOpts != nil)
+	if err != nil {
+		e := &serror.Stalling{
+			Err:    fmt.Errorf("failed to create registry client:: %w", err),
+			Reason: meta.FailedReason,
+		}
+		conditions.MarkFalse(obj, meta.ReadyCondition, e.Reason, e.Err.Error())
+		return sreconcile.ResultEmpty, e
+	}
+
+	defer func() {
+		if file != "" {
+			os.Remove(file)
+		}
+	}()
+
+	chartRepo, err := repository.NewOCIChartRepository(obj.Spec.URL, repository.WithOCIRegistryClient(registryClient))
 	if err != nil {
 		if strings.Contains(err.Error(), "parse") {
-			e := &serror.Event{
+			e := &serror.Stalling{
 				Err:    fmt.Errorf("failed to parse URL '%s': %w", obj.Spec.URL, err),
-				Reason: "ValidationError",
+				Reason: sourcev1.URLInvalidReason,
 			}
-			conditions.MarkFalse(obj, sourcev1.SourceValidCondition, e.Reason, e.Err.Error())
+			conditions.MarkFalse(obj, meta.ReadyCondition, e.Reason, e.Err.Error())
 			return sreconcile.ResultEmpty, e
 		} else if strings.Contains(err.Error(), "the url scheme is not supported") {
 			e := &serror.Event{
 				Err:    err,
-				Reason: "ValidationError",
+				Reason: sourcev1.URLInvalidReason,
 			}
-			conditions.MarkFalse(obj, sourcev1.SourceValidCondition, e.Reason, e.Err.Error())
+			conditions.MarkFalse(obj, meta.ReadyCondition, e.Reason, e.Err.Error())
 			return sreconcile.ResultEmpty, e
 		}
 	}
 
 	// Attempt to login to the registry if credentials are provided.
-	if loginOpts != nil {
-		// create a temporary file to store the credentials
-		// this is needed because otherwise the credentials are stored in ~/.docker/config.json.
-		credentialFile, err := os.CreateTemp("", "credentials")
+	if logOpts != nil {
+		err = chartRepo.Login(logOpts...)
 		if err != nil {
 			e := &serror.Event{
 				Err:    fmt.Errorf("failed to create temporary file: %w", err),
-				Reason: "ValidationError",
-			}
-			conditions.MarkFalse(obj, sourcev1.SourceValidCondition, e.Reason, e.Err.Error())
-			return sreconcile.ResultEmpty, e
-		}
-		defer os.Remove(credentialFile.Name())
-
-		// set the credentials file to the registry client
-		registry.ClientOptCredentialsFile(credentialFile.Name())(r.RegistryClient)
-		err = chartRepo.Login(loginOpts...)
-		if err != nil {
-			e := &serror.Event{
-				Err:    fmt.Errorf("failed to login to registry: %w", err),
-				Reason: "ValidationError",
+				Reason: meta.FailedReason,
 			}
-			conditions.MarkFalse(obj, sourcev1.SourceValidCondition, e.Reason, e.Err.Error())
+			conditions.MarkFalse(obj, meta.ReadyCondition, e.Reason, e.Err.Error())
 			return sreconcile.ResultEmpty, e
 		}
 	}
 
-	conditions.MarkTrue(obj, sourcev1.SourceValidCondition, meta.SucceededReason, "Helm repository %q is valid", obj.Name)
+	conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "Helm repository %q is valid", obj.Name)
 
 	return sreconcile.ResultSuccess, nil
 }

controllers/suite_test.go

@@ -233,12 +233,15 @@ func TestMain(m *testing.M) {
 		panic(fmt.Sprintf("Failed to start HelmRepositoryReconciler: %v", err))
 	}
 
+	registryClientGenerator := func(_ bool) (*registry.Client, string, error) {
+		return testRegistryClient, "", nil
+	}
 	if err = (&HelmRepositoryOCIReconciler{
-		Client:         testEnv,
-		EventRecorder:  record.NewFakeRecorder(32),
-		Metrics:        testMetricsH,
-		Getters:        testGetters,
-		RegistryClient: testRegistryClient,
+		Client:                  testEnv,
+		EventRecorder:           record.NewFakeRecorder(32),
+		Metrics:                 testMetricsH,
+		Getters:                 testGetters,
+		RegistryClientGenerator: registryClientGenerator,
 	}).SetupWithManager(testEnv); err != nil {
 		panic(fmt.Sprintf("Failed to start HelmRepositoryOCIReconciler: %v", err))
 	}