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

Store Helm indexes in JSON format #1178

Merged
merged 1 commit into from
Aug 7, 2023
Merged

Store Helm indexes in JSON format #1178

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
45 changes: 23 additions & 22 deletions internal/controller/helmrepository_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package controller

import (
"bytes"
"context"
"errors"
"fmt"
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand All @@ -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.
Expand Down Expand Up @@ -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,
Expand Down
84 changes: 25 additions & 59 deletions internal/controller/helmrepository_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package controller
import (
"context"
"crypto/tls"
"encoding/json"
"errors"
"fmt"
"net/http"
Expand Down Expand Up @@ -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
Expand All @@ -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{
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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")
Expand All @@ -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")
Expand All @@ -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")
Expand All @@ -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")
Expand Down Expand Up @@ -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")
Expand All @@ -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")
Expand All @@ -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"),
Expand All @@ -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",
Expand Down Expand Up @@ -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)
}

Expand All @@ -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())
Expand Down Expand Up @@ -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'"),
},
Expand Down Expand Up @@ -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.
Expand Down
29 changes: 28 additions & 1 deletion internal/helm/repository/chart_repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"bytes"
"context"
"crypto/tls"
"encoding/json"
"errors"
"fmt"
"io"
Expand Down Expand Up @@ -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
}

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