Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

helmrepo: same revision different checksum scenario #691

Merged
merged 1 commit into from
Apr 27, 2022
Merged

helmrepo: same revision different checksum scenario #691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions controllers/helmrepository_controller.go
View file
Open in desktop
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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newChartRepo.Checksum is always empty, resulting in this condition to be true always.

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
Open in desktop
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