Skip to content

Commit

Permalink
helmrepo: same revision different checksum condn
Browse files Browse the repository at this point in the history 
This change prevents Reconciling and ArtifactOutdated conditions to be
set on HelmRepo when the checksum of a cached repo index changes.

Adds some tests to ensure that when the repo index is cached, the
revision and checksum of the returned artifact are the same as on the
existing object status.
Also adds checks for the returned artifact and chartRepo from
reconcileSource, to ensure that chartRepo is populated and the checksum
of a new potential artifact is always empty, as it's populated when the
artifact is written in the storage.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
  • Loading branch information
@darkowlzz
darkowlzz committed Apr 27, 2022
1 parent dd40748 commit eeaa958
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 eeaa958

Please sign in to comment.