diff --git a/internal/controller/helmrepository_controller.go b/internal/controller/helmrepository_controller.go index dd75ff915..1f9d26168 100644 --- a/internal/controller/helmrepository_controller.go +++ b/internal/controller/helmrepository_controller.go @@ -17,6 +17,7 @@ limitations under the License. package controller import ( + "bytes" "context" "errors" "fmt" @@ -449,11 +450,11 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc // Early comparison to current Artifact. if curArtifact := obj.GetArtifact(); curArtifact != nil { - curDig := digest.Digest(curArtifact.Digest) - if curDig.Validate() == nil { + curRev := digest.Digest(curArtifact.Revision) + if curRev.Validate() == nil { // Short-circuit based on the fetched index being an exact match to the // stored Artifact. - if newDig := chartRepo.Digest(curDig.Algorithm()); newDig.Validate() == nil && (newDig == curDig) { + if newRev := chartRepo.Digest(curRev.Algorithm()); newRev.Validate() == nil && (newRev == curRev) { *artifact = *curArtifact conditions.Delete(obj, sourcev1.FetchFailedCondition) return sreconcile.ResultSuccess, nil @@ -473,13 +474,6 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc // Delete any stale failure observation conditions.Delete(obj, sourcev1.FetchFailedCondition) - // Check if index has changed compared to current Artifact revision. - var changed bool - if artifact := obj.Status.Artifact; artifact != nil { - curRev := digest.Digest(artifact.Revision) - changed = curRev.Validate() != nil || curRev != chartRepo.Digest(curRev.Algorithm()) - } - // Calculate revision. revision := chartRepo.Digest(intdigest.Canonical) if revision.Validate() != nil { @@ -492,16 +486,14 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc } // Mark observations about the revision on the object. - if obj.Status.Artifact == nil || changed { - message := fmt.Sprintf("new index revision '%s'", revision) - if obj.GetArtifact() != nil { - conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message) - } - rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, "building artifact: %s", message) - if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { - ctrl.LoggerFrom(ctx).Error(err, "failed to patch") - return sreconcile.ResultEmpty, err - } + message := fmt.Sprintf("new index revision '%s'", revision) + if obj.GetArtifact() != nil { + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message) + } + rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, "building artifact: %s", message) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + ctrl.LoggerFrom(ctx).Error(err, "failed to patch") + return sreconcile.ResultEmpty, err } // Create potential new artifact. @@ -566,8 +558,17 @@ func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *pa } defer unlock() - // Save artifact to storage. - if err = r.Storage.CopyFromPath(artifact, chartRepo.Path); err != nil { + // Save artifact to storage in JSON format. + b, err := chartRepo.ToJSON() + if err != nil { + e := &serror.Event{ + Err: fmt.Errorf("unable to get JSON index from chart repo: %w", err), + Reason: sourcev1.ArchiveOperationFailedReason, + } + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e + } + if err = r.Storage.Copy(artifact, bytes.NewBuffer(b)); err != nil { e := &serror.Event{ Err: fmt.Errorf("unable to save artifact to storage: %w", err), Reason: sourcev1.ArchiveOperationFailedReason, diff --git a/internal/controller/helmrepository_controller_test.go b/internal/controller/helmrepository_controller_test.go index 3d3e914c2..370cac0ed 100644 --- a/internal/controller/helmrepository_controller_test.go +++ b/internal/controller/helmrepository_controller_test.go @@ -19,6 +19,7 @@ package controller import ( "context" "crypto/tls" + "encoding/json" "errors" "fmt" "net/http" @@ -417,7 +418,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { server options url string secret *corev1.Secret - beforeFunc func(t *WithT, obj *helmv1.HelmRepository, rev, dig digest.Digest) + beforeFunc func(t *WithT, obj *helmv1.HelmRepository, rev digest.Digest) afterFunc func(t *WithT, obj *helmv1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) want sreconcile.Result wantErr bool @@ -436,7 +437,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { "caFile": tlsCA, }, }, - beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev, dig digest.Digest) { + beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev digest.Digest) { obj.Spec.CertSecretRef = &meta.LocalObjectReference{Name: "ca-file"} }, assertConditions: []metav1.Condition{ @@ -474,7 +475,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { "password": []byte("1234"), }, }, - beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev, dig digest.Digest) { + beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev digest.Digest) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "basic-auth"} }, want: sreconcile.ResultSuccess, @@ -504,7 +505,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { "caFile": []byte("invalid"), }, }, - beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev, dig digest.Digest) { + beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev digest.Digest) { obj.Spec.CertSecretRef = &meta.LocalObjectReference{Name: "invalid-ca"} conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") @@ -525,7 +526,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { { name: "Invalid URL makes FetchFailed=True and returns stalling error", protocol: "http", - beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev, dig digest.Digest) { + beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev digest.Digest) { obj.Spec.URL = strings.ReplaceAll(obj.Spec.URL, "http://", "") conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") @@ -547,7 +548,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { { name: "Unsupported scheme makes FetchFailed=True and returns stalling error", protocol: "http", - beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev, dig digest.Digest) { + beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev digest.Digest) { obj.Spec.URL = strings.ReplaceAll(obj.Spec.URL, "http://", "ftp://") conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") @@ -569,7 +570,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { { name: "Missing secret returns FetchFailed=True and returns error", protocol: "http", - beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev, dig digest.Digest) { + beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev digest.Digest) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "non-existing"} conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") @@ -598,7 +599,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { "username": []byte("git"), }, }, - beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev, dig digest.Digest) { + beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev digest.Digest) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "malformed-basic-auth"} conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") @@ -617,12 +618,11 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { }, }, { - name: "Stored index with same digest and revision", + name: "Stored index with same revision", protocol: "http", - beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev, dig digest.Digest) { + beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev digest.Digest) { obj.Status.Artifact = &sourcev1.Artifact{ Revision: rev.String(), - Digest: dig.String(), } conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") @@ -642,41 +642,15 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { want: sreconcile.ResultSuccess, }, { - name: "Stored index with different digest and same revision", + name: "Stored index with different revision", protocol: "http", - beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev, dig digest.Digest) { - obj.Status.Artifact = &sourcev1.Artifact{ - Revision: rev.String(), - Digest: "sha256:80bb3dd67c63095d985850459834ea727603727a370079de90d221191d375a86", - } - - conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") - conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, "foo", "bar") - }, - assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), - *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), - }, - afterFunc: func(t *WithT, obj *helmv1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) { - t.Expect(chartRepo.Path).ToNot(BeEmpty()) - t.Expect(chartRepo.Index).ToNot(BeNil()) - - t.Expect(artifact.Revision).To(Equal(obj.Status.Artifact.Revision)) - t.Expect(artifact.Digest).ToNot(Equal(obj.Status.Artifact.Digest)) - }, - want: sreconcile.ResultSuccess, - }, - { - name: "Stored index with different revision and digest", - protocol: "http", - beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev, dig digest.Digest) { + beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev digest.Digest) { obj.Status.Artifact = &sourcev1.Artifact{ Revision: "80bb3dd67c63095d985850459834ea727603727a370079de90d221191d375a86", - Digest: "sha256:80bb3dd67c63095d985850459834ea727603727a370079de90d221191d375a86", } conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, "foo", "bar") }, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"), @@ -689,14 +663,13 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { t.Expect(artifact.Path).To(Not(BeEmpty())) t.Expect(artifact.Revision).ToNot(Equal(obj.Status.Artifact.Revision)) - t.Expect(artifact.Digest).ToNot(Equal(obj.Status.Artifact.Digest)) }, want: sreconcile.ResultSuccess, }, { name: "Existing artifact makes ArtifactOutdated=True", protocol: "http", - beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev, dig digest.Digest) { + beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev digest.Digest) { obj.Status.Artifact = &sourcev1.Artifact{ Path: "some-path", Revision: "some-rev", @@ -806,14 +779,9 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { } g.Expect(err).ToNot(HaveOccurred()) - // NOTE: digest will be empty in beforeFunc for invalid repo - // configurations as the client can't get the repo. - var rev, dig digest.Digest + var rev digest.Digest if validSecret { g.Expect(newChartRepo.CacheIndex()).To(Succeed()) - dig = newChartRepo.Digest(intdigest.Canonical) - - g.Expect(newChartRepo.LoadFromPath()).To(Succeed()) rev = newChartRepo.Digest(intdigest.Canonical) } @@ -825,7 +793,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { patchOptions: getPatchOptions(helmRepositoryReadyCondition.Owned, "sc"), } if tt.beforeFunc != nil { - tt.beforeFunc(g, obj, rev, dig) + tt.beforeFunc(g, obj, rev) } g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) @@ -866,11 +834,17 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { assertConditions []metav1.Condition }{ { - name: "Archiving artifact to storage makes ArtifactInStorage=True", + name: "Archiving artifact to storage makes ArtifactInStorage=True and artifact is stored as JSON", beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, artifact sourcev1.Artifact, index *repository.ChartRepository) { obj.Spec.Interval = metav1.Duration{Duration: interval} }, want: sreconcile.ResultSuccess, + afterFunc: func(t *WithT, obj *helmv1.HelmRepository, cache *cache.Cache) { + localPath := testStorage.LocalPath(*obj.GetArtifact()) + b, err := os.ReadFile(localPath) + t.Expect(err).To(Not(HaveOccurred())) + t.Expect(json.Valid(b)).To(BeTrue()) + }, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact: revision 'existing'"), }, @@ -970,17 +944,9 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { }, } - tmpDir := t.TempDir() - - // Create an empty cache file. - cachePath := filepath.Join(tmpDir, "index.yaml") - cacheFile, err := os.Create(cachePath) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(cacheFile.Close()).ToNot(HaveOccurred()) - chartRepo, err := repository.NewChartRepository(obj.Spec.URL, "", testGetters, nil) g.Expect(err).ToNot(HaveOccurred()) - chartRepo.Path = cachePath + chartRepo.Index = &repo.IndexFile{} artifact := testStorage.NewArtifactFor(obj.Kind, obj, "existing", "foo.tar.gz") // Digest of the index file calculated by the ChartRepository. diff --git a/internal/helm/repository/chart_repository.go b/internal/helm/repository/chart_repository.go index 3dcd265d2..4908e8f36 100644 --- a/internal/helm/repository/chart_repository.go +++ b/internal/helm/repository/chart_repository.go @@ -20,6 +20,7 @@ import ( "bytes" "context" "crypto/tls" + "encoding/json" "errors" "fmt" "io" @@ -76,7 +77,7 @@ func IndexFromBytes(b []byte) (*repo.IndexFile, error) { } i := &repo.IndexFile{} - if err := yaml.UnmarshalStrict(b, i); err != nil { + if err := jsonOrYamlUnmarshal(b, i); err != nil { return nil, err } @@ -401,6 +402,15 @@ func (r *ChartRepository) Digest(algorithm digest.Algorithm) digest.Digest { return r.digests[algorithm] } +// ToJSON returns the index formatted as JSON. +func (r *ChartRepository) ToJSON() ([]byte, error) { + if !r.HasIndex() { + return nil, fmt.Errorf("index not loaded yet") + } + + return json.MarshalIndent(r.Index, "", " ") +} + // HasIndex returns true if the Index is not nil. func (r *ChartRepository) HasIndex() bool { r.RLock() @@ -459,3 +469,20 @@ func (r *ChartRepository) VerifyChart(_ context.Context, _ *repo.ChartVersion) e // this is a no-op because this is not implemented yet. return fmt.Errorf("not implemented") } + +// jsonOrYamlUnmarshal unmarshals the given byte slice containing JSON or YAML +// into the provided interface. +// +// It automatically detects whether the data is in JSON or YAML format by +// checking its validity as JSON. If the data is valid JSON, it will use the +// `encoding/json` package to unmarshal it. Otherwise, it will use the +// `sigs.k8s.io/yaml` package to unmarshal the YAML data. +// +// Can potentially be replaced when Helm PR for JSON support has been merged. +// xref: https://github.com/helm/helm/pull/12245 +func jsonOrYamlUnmarshal(b []byte, i interface{}) error { + if json.Valid(b) { + return json.Unmarshal(b, i) + } + return yaml.UnmarshalStrict(b, i) +} diff --git a/internal/helm/repository/chart_repository_test.go b/internal/helm/repository/chart_repository_test.go index a961f3e89..269008a21 100644 --- a/internal/helm/repository/chart_repository_test.go +++ b/internal/helm/repository/chart_repository_test.go @@ -39,9 +39,10 @@ import ( var now = time.Now() const ( - testFile = "../testdata/local-index.yaml" - chartmuseumTestFile = "../testdata/chartmuseum-index.yaml" - unorderedTestFile = "../testdata/local-index-unordered.yaml" + testFile = "../testdata/local-index.yaml" + chartmuseumTestFile = "../testdata/chartmuseum-index.yaml" + chartmuseumJSONTestFile = "../testdata/chartmuseum-index.json" + unorderedTestFile = "../testdata/local-index-unordered.yaml" ) // mockGetter is a simple mocking getter.Getter implementation, returning @@ -81,6 +82,10 @@ func TestIndexFromFile(t *testing.T) { name: "chartmuseum index file", filename: chartmuseumTestFile, }, + { + name: "chartmuseum json index file", + filename: chartmuseumJSONTestFile, + }, { name: "error if index size exceeds max size", filename: bigIndexFile, @@ -407,6 +412,25 @@ func TestChartRepository_CacheIndex(t *testing.T) { g.Expect(r.digests).To(BeEmpty()) } +func TestChartRepository_ToJSON(t *testing.T) { + g := NewWithT(t) + + r := newChartRepository() + r.Path = chartmuseumTestFile + + _, err := r.ToJSON() + g.Expect(err).To(HaveOccurred()) + + g.Expect(r.LoadFromPath()).To(Succeed()) + b, err := r.ToJSON() + g.Expect(err).ToNot(HaveOccurred()) + + jsonBytes, err := os.ReadFile(chartmuseumJSONTestFile) + jsonBytes = bytes.TrimRight(jsonBytes, "\n") + g.Expect(err).To(Not(HaveOccurred())) + g.Expect(string(b)).To(Equal(string(jsonBytes))) +} + func TestChartRepository_DownloadIndex(t *testing.T) { g := NewWithT(t) diff --git a/internal/helm/testdata/chartmuseum-index.json b/internal/helm/testdata/chartmuseum-index.json new file mode 100644 index 000000000..745617e30 --- /dev/null +++ b/internal/helm/testdata/chartmuseum-index.json @@ -0,0 +1,82 @@ +{ + "serverInfo": { + "contextPath": "/v1/helm" + }, + "apiVersion": "v1", + "generated": "0001-01-01T00:00:00Z", + "entries": { + "alpine": [ + { + "name": "alpine", + "home": "https://github.com/something", + "version": "1.0.0", + "description": "string", + "keywords": [ + "linux", + "alpine", + "small", + "sumtin" + ], + "apiVersion": "v1", + "urls": [ + "https://kubernetes-charts.storage.googleapis.com/alpine-1.0.0.tgz", + "http://storage2.googleapis.com/kubernetes-charts/alpine-1.0.0.tgz" + ], + "created": "0001-01-01T00:00:00Z", + "digest": "sha256:1234567890abcdef" + } + ], + "chartWithNoURL": [ + { + "name": "chartWithNoURL", + "home": "https://github.com/something", + "version": "1.0.0", + "description": "string", + "keywords": [ + "small", + "sumtin" + ], + "apiVersion": "v1", + "urls": null, + "created": "0001-01-01T00:00:00Z", + "digest": "sha256:1234567890abcdef" + } + ], + "nginx": [ + { + "name": "nginx", + "home": "https://github.com/something/else", + "version": "0.2.0", + "description": "string", + "keywords": [ + "popular", + "web server", + "proxy" + ], + "apiVersion": "v1", + "urls": [ + "https://kubernetes-charts.storage.googleapis.com/nginx-0.2.0.tgz" + ], + "created": "0001-01-01T00:00:00Z", + "digest": "sha256:1234567890abcdef" + }, + { + "name": "nginx", + "home": "https://github.com/something", + "version": "0.1.0", + "description": "string", + "keywords": [ + "popular", + "web server", + "proxy" + ], + "apiVersion": "v1", + "urls": [ + "https://kubernetes-charts.storage.googleapis.com/nginx-0.1.0.tgz" + ], + "created": "0001-01-01T00:00:00Z", + "digest": "sha256:1234567890abcdef" + } + ] + } +}