diff --git a/controllers/helmrepository_controller.go b/controllers/helmrepository_controller.go index fb0ced3a8..9b9db4968 100644 --- a/controllers/helmrepository_controller.go +++ b/controllers/helmrepository_controller.go @@ -421,7 +421,8 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou return sreconcile.ResultSuccess, nil } - // Load the cached repository index to ensure it passes validation. + // Load the cached repository index to ensure it passes validation. This + // also populates chartRepo.Checksum. if err := chartRepo.LoadFromCache(); err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to load Helm repository from cache: %w", err), @@ -433,13 +434,15 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou chartRepo.Unload() // Mark observations about the revision on the object. - if !obj.GetArtifact().HasRevision(newChartRepo.Checksum) { + if !obj.GetArtifact().HasRevision(chartRepo.Checksum) { message := fmt.Sprintf("new index revision '%s'", checksum) conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message) conditions.MarkReconciling(obj, "NewRevision", message) } // Create potential new artifact. + // Note: Since this is a potential artifact, artifact.Checksum is empty at + // this stage. It's populated when the artifact is written in storage. *artifact = r.Storage.NewArtifactFor(obj.Kind, obj.ObjectMeta.GetObjectMeta(), chartRepo.Checksum, diff --git a/controllers/helmrepository_controller_test.go b/controllers/helmrepository_controller_test.go index 97934d509..4d713d9ee 100644 --- a/controllers/helmrepository_controller_test.go +++ b/controllers/helmrepository_controller_test.go @@ -18,6 +18,7 @@ package controllers import ( "context" + "crypto/tls" "errors" "fmt" "net/http" @@ -33,6 +34,7 @@ import ( "github.com/fluxcd/pkg/runtime/conditions" "github.com/fluxcd/pkg/runtime/patch" . "github.com/onsi/gomega" + helmgetter "helm.sh/helm/v3/pkg/getter" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -43,6 +45,7 @@ import ( fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" + "github.com/fluxcd/source-controller/internal/helm/getter" "github.com/fluxcd/source-controller/internal/helm/repository" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" "github.com/fluxcd/source-controller/internal/reconcile/summarize" @@ -288,8 +291,8 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { protocol string server options secret *corev1.Secret - beforeFunc func(t *WithT, obj *sourcev1.HelmRepository) - afterFunc func(t *WithT, obj *sourcev1.HelmRepository) + beforeFunc func(t *WithT, obj *sourcev1.HelmRepository, checksum string) + afterFunc func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) want sreconcile.Result wantErr bool assertConditions []metav1.Condition @@ -302,6 +305,12 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"), *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision"), }, + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { + t.Expect(chartRepo.Checksum).ToNot(BeEmpty()) + t.Expect(chartRepo.CachePath).ToNot(BeEmpty()) + t.Expect(artifact.Checksum).To(BeEmpty()) + t.Expect(artifact.Revision).ToNot(BeEmpty()) + }, }, { name: "HTTP with Basic Auth secret makes ArtifactOutdated=True", @@ -319,7 +328,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { "password": []byte("1234"), }, }, - beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository) { + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "basic-auth"} }, want: sreconcile.ResultSuccess, @@ -327,6 +336,12 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"), *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision"), }, + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { + t.Expect(chartRepo.Checksum).ToNot(BeEmpty()) + t.Expect(chartRepo.CachePath).ToNot(BeEmpty()) + t.Expect(artifact.Checksum).To(BeEmpty()) + t.Expect(artifact.Revision).ToNot(BeEmpty()) + }, }, { name: "HTTPS with CAFile secret makes ArtifactOutdated=True", @@ -344,7 +359,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { "caFile": tlsCA, }, }, - beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository) { + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "ca-file"} }, want: sreconcile.ResultSuccess, @@ -352,6 +367,12 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"), *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision"), }, + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { + t.Expect(chartRepo.Checksum).ToNot(BeEmpty()) + t.Expect(chartRepo.CachePath).ToNot(BeEmpty()) + t.Expect(artifact.Checksum).To(BeEmpty()) + t.Expect(artifact.Revision).ToNot(BeEmpty()) + }, }, { name: "HTTPS with invalid CAFile secret makes FetchFailed=True and returns error", @@ -369,18 +390,25 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { "caFile": []byte("invalid"), }, }, - beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository) { + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "invalid-ca"} }, wantErr: true, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to create TLS client config with secret data: cannot append certificate into certificate pool: invalid caFile"), }, + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { + // No repo index due to fetch fail. + t.Expect(chartRepo.Checksum).To(BeEmpty()) + t.Expect(chartRepo.CachePath).To(BeEmpty()) + t.Expect(artifact.Checksum).To(BeEmpty()) + t.Expect(artifact.Revision).To(BeEmpty()) + }, }, { name: "Invalid URL makes FetchFailed=True and returns stalling error", protocol: "http", - beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository) { + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { obj.Spec.URL = strings.ReplaceAll(obj.Spec.URL, "http://", "") }, want: sreconcile.ResultEmpty, @@ -388,11 +416,18 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.URLInvalidReason, "first path segment in URL cannot contain colon"), }, + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { + // No repo index due to fetch fail. + t.Expect(chartRepo.Checksum).To(BeEmpty()) + t.Expect(chartRepo.CachePath).To(BeEmpty()) + t.Expect(artifact.Checksum).To(BeEmpty()) + t.Expect(artifact.Revision).To(BeEmpty()) + }, }, { name: "Unsupported scheme makes FetchFailed=True and returns stalling error", protocol: "http", - beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository) { + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { obj.Spec.URL = strings.ReplaceAll(obj.Spec.URL, "http://", "ftp://") }, want: sreconcile.ResultEmpty, @@ -400,17 +435,31 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, meta.FailedReason, "scheme \"ftp\" not supported"), }, + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { + // No repo index due to fetch fail. + t.Expect(chartRepo.Checksum).To(BeEmpty()) + t.Expect(chartRepo.CachePath).To(BeEmpty()) + t.Expect(artifact.Checksum).To(BeEmpty()) + t.Expect(artifact.Revision).To(BeEmpty()) + }, }, { name: "Missing secret returns FetchFailed=True and returns error", protocol: "http", - beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository) { + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "non-existing"} }, wantErr: true, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "secrets \"non-existing\" not found"), }, + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { + // No repo index due to fetch fail. + t.Expect(chartRepo.Checksum).To(BeEmpty()) + t.Expect(chartRepo.CachePath).To(BeEmpty()) + t.Expect(artifact.Checksum).To(BeEmpty()) + t.Expect(artifact.Revision).To(BeEmpty()) + }, }, { name: "Malformed secret returns FetchFailed=True and returns error", @@ -423,13 +472,56 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { "username": []byte("git"), }, }, - beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository) { + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "malformed-basic-auth"} }, wantErr: true, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "required fields 'username' and 'password"), }, + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { + // No repo index due to fetch fail. + t.Expect(chartRepo.Checksum).To(BeEmpty()) + t.Expect(chartRepo.CachePath).To(BeEmpty()) + t.Expect(artifact.Checksum).To(BeEmpty()) + t.Expect(artifact.Revision).To(BeEmpty()) + }, + }, + { + name: "cached index with same checksum", + protocol: "http", + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { + obj.Status.Artifact = &sourcev1.Artifact{ + Revision: checksum, + Checksum: checksum, + } + }, + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { + // chartRepo.Checksum isn't populated, artifact.Checksum is + // populated from the cached repo index data. + t.Expect(chartRepo.Checksum).To(BeEmpty()) + t.Expect(chartRepo.CachePath).ToNot(BeEmpty()) + t.Expect(artifact.Checksum).To(Equal(obj.Status.Artifact.Checksum)) + t.Expect(artifact.Revision).To(Equal(obj.Status.Artifact.Revision)) + }, + want: sreconcile.ResultSuccess, + }, + { + name: "cached index with different checksum", + protocol: "http", + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { + obj.Status.Artifact = &sourcev1.Artifact{ + Revision: checksum, + Checksum: "foo", + } + }, + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { + t.Expect(chartRepo.Checksum).ToNot(BeEmpty()) + t.Expect(chartRepo.CachePath).ToNot(BeEmpty()) + t.Expect(artifact.Checksum).To(BeEmpty()) + t.Expect(artifact.Revision).To(Equal(obj.Status.Artifact.Revision)) + }, + want: sreconcile.ResultSuccess, }, } @@ -481,15 +573,51 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { t.Fatalf("unsupported protocol %q", tt.protocol) } - if tt.beforeFunc != nil { - tt.beforeFunc(g, obj) - } - builder := fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()) if secret != nil { builder.WithObjects(secret.DeepCopy()) } + // Calculate the artifact checksum for valid repos configurations. + clientOpts := []helmgetter.Option{ + helmgetter.WithURL(server.URL()), + } + var newChartRepo *repository.ChartRepository + var tOpts *tls.Config + validSecret := true + if secret != nil { + // Extract the client options from secret, ignoring any invalid + // value. validSecret is used to determine if the indexChecksum + // should be calculated below. + var cOpts []helmgetter.Option + var serr error + cOpts, serr = getter.ClientOptionsFromSecret(*secret) + if serr != nil { + validSecret = false + } + clientOpts = append(clientOpts, cOpts...) + tOpts, serr = getter.TLSClientConfigFromSecret(*secret, server.URL()) + if serr != nil { + validSecret = false + } + newChartRepo, err = repository.NewChartRepository(obj.Spec.URL, "", testGetters, tOpts, clientOpts) + } else { + newChartRepo, err = repository.NewChartRepository(obj.Spec.URL, "", testGetters, nil, nil) + } + g.Expect(err).ToNot(HaveOccurred()) + + // NOTE: checksum will be empty in beforeFunc for invalid repo + // configurations as the client can't get the repo. + var indexChecksum string + if validSecret { + indexChecksum, err = newChartRepo.CacheIndex() + g.Expect(err).ToNot(HaveOccurred()) + } + + if tt.beforeFunc != nil { + tt.beforeFunc(g, obj, indexChecksum) + } + r := &HelmRepositoryReconciler{ EventRecorder: record.NewFakeRecorder(32), Client: builder.Build(), @@ -507,7 +635,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { g.Expect(got).To(Equal(tt.want)) if tt.afterFunc != nil { - tt.afterFunc(g, obj) + tt.afterFunc(g, obj, artifact, chartRepo) } }) }