Adding helmChart reconciler test as well as e2e test

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

config/testdata/helmchart-from-oci/source.yaml

@@ -0,0 +1,21 @@
+---
+apiVersion: source.toolkit.fluxcd.io/v1beta2
+kind: HelmRepository
+metadata:
+  name: podinfo
+spec:
+  url: oci://ghcr.io/stefanprodan/charts
+  type: "oci"
+  interval: 1m
+---
+apiVersion: source.toolkit.fluxcd.io/v1beta2
+kind: HelmChart
+metadata:
+  name: podinfo
+spec:
+  chart: podinfo
+  sourceRef:
+    kind: HelmRepository
+    name: podinfo
+  version: '6.1.*'
+  interval: 1m

controllers/helmchart_controller.go

@@ -453,7 +453,8 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 		helmgetter.WithTimeout(repo.Spec.Timeout.Duration),
 		helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
 	}
-	if secret, err := r.getHelmRepositorySecret(ctx, repo); secret != nil || err != nil {
+	secret, err := r.getHelmRepositorySecret(ctx, repo)
+	if secret != nil || err != nil {
 		if err != nil {
 			e := &serror.Event{
 				Err:    fmt.Errorf("failed to get secret '%s': %w", repo.Spec.SecretRef.Name, err),
@@ -491,15 +492,43 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 
 	// Initialize the chart repository
 	var chartRepo chart.Remote
-	var err error
-	if registry.IsOCI(repo.Spec.URL) {
-		chartRepo, err = repository.NewOCIChartRepository(repo.Spec.URL, repository.WithOCIGetter(r.Getters), repository.WithOCIRegistryClient(r.RegistryClient))
+	if repo.Spec.Type == sourcev1.HelmRepositoryTypeOCI {
+		if !registry.IsOCI(repo.Spec.URL) {
+			err = fmt.Errorf("invalid OCI registry URL: %s", repo.Spec.URL)
+			return chartRepoErrorReturn(err, obj)
+		}
+		// 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))
+		if err != nil {
+			return chartRepoErrorReturn(err, obj)
+		}
+		chartRepo = ociChartRepo
+
+		// 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 secret != nil {
+			// Construct actual options
+			logOpt, err := loginOptionFromSecret(*secret)
+			if err != nil {
+				return chartRepoErrorReturn(err, obj)
+			}
+
+			logOpts := append([]registry.LoginOption{}, logOpt)
+			err = ociChartRepo.Login(logOpts...)
+			if err != nil {
+				return chartRepoErrorReturn(err, obj)
+			}
+		}
 	} else {
 		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) {
 				r.IncCacheEvents(event, obj.Name, obj.Namespace)
 			}))
+		if err != nil {
+			return chartRepoErrorReturn(err, obj)
+		}
 		chartRepo = httpChartRepo
 		defer func() {
 			if httpChartRepo == nil {
@@ -523,26 +552,6 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 			}
 		}()
 	}
-	if err != nil {
-		// Any error requires a change in generation,
-		// which we should be informed about by the watcher
-		switch err.(type) {
-		case *url.Error:
-			e := &serror.Stalling{
-				Err:    fmt.Errorf("invalid Helm repository URL: %w", err),
-				Reason: sourcev1.URLInvalidReason,
-			}
-			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
-			return sreconcile.ResultEmpty, e
-		default:
-			e := &serror.Stalling{
-				Err:    fmt.Errorf("failed to construct Helm client: %w", err),
-				Reason: meta.FailedReason,
-			}
-			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
-			return sreconcile.ResultEmpty, e
-		}
-	}
 
 	// Construct the chart builder with scoped configuration
 	cb := chart.NewRemoteBuilder(chartRepo)
@@ -1106,3 +1115,22 @@ func reasonForBuild(build *chart.Build) string {
 	}
 	return sourcev1.ChartPullSucceededReason
 }
+
+func chartRepoErrorReturn(err error, obj *sourcev1.HelmChart) (sreconcile.Result, error) {
+	switch err.(type) {
+	case *url.Error:
+		e := &serror.Stalling{
+			Err:    fmt.Errorf("invalid Helm repository URL: %w", err),
+			Reason: sourcev1.URLInvalidReason,
+		}
+		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
+		return sreconcile.ResultEmpty, e
+	default:
+		e := &serror.Stalling{
+			Err:    fmt.Errorf("failed to construct Helm client: %w", err),
+			Reason: meta.FailedReason,
+		}
+		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
+		return sreconcile.ResultEmpty, e
+	}
+}

controllers/helmchart_controller_test.go

@@ -17,10 +17,12 @@ limitations under the License.
 package controllers
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
 	"io"
+	"io/ioutil"
 	"net/http"
 	"os"
 	"path/filepath"
@@ -31,6 +33,8 @@ import (
 
 	"github.com/darkowlzz/controller-check/status"
 	. "github.com/onsi/gomega"
+	"helm.sh/helm/v3/pkg/chart/loader"
+	"helm.sh/helm/v3/pkg/registry"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -45,12 +49,12 @@ import (
 	"github.com/fluxcd/pkg/runtime/conditions"
 	"github.com/fluxcd/pkg/runtime/patch"
 	"github.com/fluxcd/pkg/testserver"
-
 	sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
 	serror "github.com/fluxcd/source-controller/internal/error"
 	"github.com/fluxcd/source-controller/internal/helm/chart"
 	sreconcile "github.com/fluxcd/source-controller/internal/reconcile"
 	"github.com/fluxcd/source-controller/internal/reconcile/summarize"
+	hchart "helm.sh/helm/v3/pkg/chart"
 )
 
 func TestHelmChartReconciler_Reconcile(t *testing.T) {
@@ -776,6 +780,217 @@ func TestHelmChartReconciler_buildFromHelmRepository(t *testing.T) {
 	}
 }
 
+func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
+	g := NewWithT(t)
+
+	tmpDir := t.TempDir()
+
+	const (
+		chartPath = "testdata/charts/helmchart-0.1.0.tgz"
+	)
+
+	// Login to the registry
+	err := testRegistryserver.RegistryClient.Login(testRegistryserver.DockerRegistryHost,
+		registry.LoginOptBasicAuth(testUsername, testPassword),
+		registry.LoginOptInsecure(true))
+	g.Expect(err).NotTo(HaveOccurred())
+
+	// Load a test chart
+	chartData, err := ioutil.ReadFile(chartPath)
+	g.Expect(err).NotTo(HaveOccurred())
+	metadata, err := extractChartMeta(chartData)
+	g.Expect(err).NotTo(HaveOccurred())
+
+	// Upload the test chart
+	ref := fmt.Sprintf("%s/testrepo/%s:%s", testRegistryserver.DockerRegistryHost, metadata.Name, metadata.Version)
+	_, err = testRegistryserver.RegistryClient.Push(chartData, ref)
+	g.Expect(err).NotTo(HaveOccurred())
+
+	storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords)
+	g.Expect(err).ToNot(HaveOccurred())
+
+	cachedArtifact := &sourcev1.Artifact{
+		Revision: "0.1.0",
+		Path:     metadata.Name + "-" + metadata.Version + ".tgz",
+	}
+	g.Expect(storage.CopyFromPath(cachedArtifact, "testdata/charts/helmchart-0.1.0.tgz")).To(Succeed())
+
+	tests := []struct {
+		name       string
+		secret     *corev1.Secret
+		beforeFunc func(obj *sourcev1.HelmChart, repository *sourcev1.HelmRepository)
+		want       sreconcile.Result
+		wantErr    error
+		assertFunc func(g *WithT, obj *sourcev1.HelmChart, build chart.Build)
+		cleanFunc  func(g *WithT, build *chart.Build)
+	}{
+		{
+			name: "Reconciles chart build with repository credentials",
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "auth",
+				},
+				Data: map[string][]byte{
+					"username": []byte(testUsername),
+					"password": []byte(testPassword),
+				},
+			},
+			beforeFunc: func(obj *sourcev1.HelmChart, repository *sourcev1.HelmRepository) {
+				obj.Spec.Chart = metadata.Name
+				obj.Spec.Version = metadata.Version
+				repository.Spec.SecretRef = &meta.LocalObjectReference{Name: "auth"}
+			},
+			want: sreconcile.ResultSuccess,
+			assertFunc: func(g *WithT, _ *sourcev1.HelmChart, build chart.Build) {
+				g.Expect(build.Name).To(Equal(metadata.Name))
+				g.Expect(build.Version).To(Equal(metadata.Version))
+				g.Expect(build.Path).ToNot(BeEmpty())
+				g.Expect(build.Path).To(BeARegularFile())
+			},
+			cleanFunc: func(g *WithT, build *chart.Build) {
+				g.Expect(os.Remove(build.Path)).To(Succeed())
+			},
+		},
+		{
+			name: "Uses artifact as build cache",
+			beforeFunc: func(obj *sourcev1.HelmChart, repository *sourcev1.HelmRepository) {
+				obj.Spec.Chart = metadata.Name
+				obj.Spec.Version = metadata.Version
+				obj.Status.Artifact = &sourcev1.Artifact{Path: metadata.Name + "-" + metadata.Version + ".tgz"}
+			},
+			want: sreconcile.ResultSuccess,
+			assertFunc: func(g *WithT, obj *sourcev1.HelmChart, build chart.Build) {
+				g.Expect(build.Name).To(Equal(metadata.Name))
+				g.Expect(build.Version).To(Equal(metadata.Version))
+				g.Expect(build.Path).To(Equal(storage.LocalPath(*cachedArtifact.DeepCopy())))
+				g.Expect(build.Path).To(BeARegularFile())
+			},
+		},
+		{
+			name: "Forces build on generation change",
+			beforeFunc: func(obj *sourcev1.HelmChart, repository *sourcev1.HelmRepository) {
+				obj.Generation = 3
+				obj.Spec.Chart = metadata.Name
+				obj.Spec.Version = metadata.Version
+
+				obj.Status.ObservedGeneration = 2
+				obj.Status.Artifact = &sourcev1.Artifact{Path: metadata.Name + "-" + metadata.Version + ".tgz"}
+			},
+			want: sreconcile.ResultSuccess,
+			assertFunc: func(g *WithT, obj *sourcev1.HelmChart, build chart.Build) {
+				g.Expect(build.Name).To(Equal(metadata.Name))
+				g.Expect(build.Version).To(Equal(metadata.Version))
+				fmt.Println("buildpath", build.Path)
+				fmt.Println("storage Path", storage.LocalPath(*cachedArtifact.DeepCopy()))
+				g.Expect(build.Path).ToNot(Equal(storage.LocalPath(*cachedArtifact.DeepCopy())))
+				g.Expect(build.Path).To(BeARegularFile())
+			},
+			cleanFunc: func(g *WithT, build *chart.Build) {
+				g.Expect(os.Remove(build.Path)).To(Succeed())
+			},
+		},
+		{
+			name: "Event on unsuccessful secret retrieval",
+			beforeFunc: func(_ *sourcev1.HelmChart, repository *sourcev1.HelmRepository) {
+				repository.Spec.SecretRef = &meta.LocalObjectReference{
+					Name: "invalid",
+				}
+			},
+			want:    sreconcile.ResultEmpty,
+			wantErr: &serror.Event{Err: errors.New("failed to get secret 'invalid'")},
+			assertFunc: func(g *WithT, obj *sourcev1.HelmChart, build chart.Build) {
+				g.Expect(build.Complete()).To(BeFalse())
+
+				g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{
+					*conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret 'invalid'"),
+				}))
+			},
+		},
+		{
+			name: "Stalling on invalid client options",
+			beforeFunc: func(obj *sourcev1.HelmChart, repository *sourcev1.HelmRepository) {
+				repository.Spec.URL = "https://unsupported" // Unsupported protocol
+			},
+			want:    sreconcile.ResultEmpty,
+			wantErr: &serror.Stalling{Err: errors.New("failed to construct Helm client: invalid OCI registry URL: https://unsupported")},
+			assertFunc: func(g *WithT, obj *sourcev1.HelmChart, build chart.Build) {
+				g.Expect(build.Complete()).To(BeFalse())
+
+				g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{
+					*conditions.TrueCondition(sourcev1.FetchFailedCondition, meta.FailedReason, "failed to construct Helm client"),
+				}))
+			},
+		},
+		{
+			name: "BuildError on temporary build error",
+			beforeFunc: func(obj *sourcev1.HelmChart, _ *sourcev1.HelmRepository) {
+				obj.Spec.Chart = "invalid"
+			},
+			want:    sreconcile.ResultEmpty,
+			wantErr: &chart.BuildError{Err: errors.New("failed to get chart version for remote reference")},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			testRegistryClient, err = registry.NewClient(registry.ClientOptWriter(os.Stdout))
+			g.Expect(err).To(BeNil())
+
+			clientBuilder := fake.NewClientBuilder()
+			if tt.secret != nil {
+				clientBuilder.WithObjects(tt.secret.DeepCopy())
+			}
+
+			r := &HelmChartReconciler{
+				Client:         clientBuilder.Build(),
+				EventRecorder:  record.NewFakeRecorder(32),
+				Getters:        testGetters,
+				Storage:        storage,
+				RegistryClient: testRegistryClient,
+			}
+
+			repository := &sourcev1.HelmRepository{
+				ObjectMeta: metav1.ObjectMeta{
+					GenerateName: "helmrepository-",
+				},
+				Spec: sourcev1.HelmRepositorySpec{
+					URL:     fmt.Sprintf("oci://%s/testrepo", testRegistryserver.DockerRegistryHost),
+					Timeout: &metav1.Duration{Duration: timeout},
+					Type:    sourcev1.HelmRepositoryTypeOCI,
+				},
+			}
+			obj := &sourcev1.HelmChart{
+				ObjectMeta: metav1.ObjectMeta{
+					GenerateName: "helmrepository-",
+				},
+				Spec: sourcev1.HelmChartSpec{},
+			}
+
+			if tt.beforeFunc != nil {
+				tt.beforeFunc(obj, repository)
+			}
+
+			var b chart.Build
+			if tt.cleanFunc != nil {
+				defer tt.cleanFunc(g, &b)
+			}
+			got, err := r.buildFromHelmRepository(context.TODO(), obj, repository, &b)
+
+			g.Expect(err != nil).To(Equal(tt.wantErr != nil))
+			if tt.wantErr != nil {
+				g.Expect(reflect.TypeOf(err).String()).To(Equal(reflect.TypeOf(tt.wantErr).String()))
+				g.Expect(err.Error()).To(ContainSubstring(tt.wantErr.Error()))
+			}
+			g.Expect(got).To(Equal(tt.want))
+
+			if tt.assertFunc != nil {
+				tt.assertFunc(g, obj, b)
+			}
+		})
+	}
+}
+
 func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) {
 	g := NewWithT(t)
 
@@ -1690,3 +1905,12 @@ func TestHelmChartReconciler_notify(t *testing.T) {
 		})
 	}
 }
+
+// extractChartMeta is used to extract a chart metadata from a byte array
+func extractChartMeta(chartData []byte) (*hchart.Metadata, error) {
+	ch, err := loader.LoadArchive(bytes.NewReader(chartData))
+	if err != nil {
+		return nil, err
+	}
+	return ch.Metadata, nil
+}