helmrepo: same revision different checksum condn
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>
This commit is contained in:
Sunny
parent
dd40748e96
commit
eeaa958866
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue