Skip to content

Commit

Permalink
Merge pull request #691 from fluxcd/cached-helmrepo-diff-checksum
Browse files Browse the repository at this point in the history 
helmrepo: same revision different checksum scenario
  • Loading branch information
@darkowlzz
darkowlzz authored Apr 27, 2022
2 parents dd40748 + eeaa958 commit 745d6ee
Show file tree
Hide file tree
Showing 2 changed files with 147 additions and 16 deletions.
7 changes: 5 additions & 2 deletions controllers/helmrepository_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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,
Expand Down
156 changes: 142 additions & 14 deletions controllers/helmrepository_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package controllers

import (
"context"
"crypto/tls"
"errors"
"fmt"
"net/http"
Expand All @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -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",
Expand All @@ -319,14 +328,20 @@ 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,
assertConditions: []metav1.Condition{
*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",
Expand All @@ -344,14 +359,20 @@ 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,
assertConditions: []metav1.Condition{
*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",
Expand All @@ -369,48 +390,76 @@ 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,
wantErr: true,
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,
wantErr: true,
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",
Expand All @@ -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,
},
}

Expand Down Expand Up @@ -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(),
Expand All @@ -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)
}
})
}
Expand Down

0 comments on commit 745d6ee

Please sign in to comment.