From 0d66b815696130108c0d8b8d94a8a7aa0ba9ccbb Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Tue, 22 Mar 2022 16:28:26 +0530 Subject: [PATCH] improve conditions and fix temp file storage Signed-off-by: Sanskar Jaiswal --- api/v1beta2/helmchart_types.go | 4 +-- controllers/helmchart_controller.go | 7 +++-- controllers/helmchart_controller_test.go | 2 +- internal/helm/chart/builder_remote.go | 7 ++--- internal/helm/repository/chart_repository.go | 2 +- internal/util/file.go | 29 ++++++++++++-------- 6 files changed, 29 insertions(+), 22 deletions(-) diff --git a/api/v1beta2/helmchart_types.go b/api/v1beta2/helmchart_types.go index afaa5c1b4..8c634b138 100644 --- a/api/v1beta2/helmchart_types.go +++ b/api/v1beta2/helmchart_types.go @@ -172,9 +172,9 @@ const ( // chart succeeded. ChartPackageSucceededReason string = "ChartPackageSucceeded" - // ChartVerifiedSucceededReason signals that the Helm chart's signature + // ChartVerificationSucceededReason signals that the Helm chart's signature // has been verified using it's provenance file. - ChartVerifiedSucceededReason string = "ChartVerifiedSucceeded" + ChartVerificationSucceededReason string = "ChartVerificationSucceeded" ) // GetConditions returns the status conditions of the object. diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index b2f40bc3e..06aa0b68c 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -809,11 +809,14 @@ func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *sourcev1. } return false } - if _, err := r.Storage.RemoveConditionally(dir, callback); err != nil { + if deleted, err := r.Storage.RemoveConditionally(dir, callback); err != nil { return &serror.Event{ Err: fmt.Errorf("garbage collection of old artifacts failed: %w", err), Reason: "GarbageCollectionFailed", } + } else if len(deleted) > 0 { + r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", + "garbage collected old artifacts") } } return nil @@ -1039,7 +1042,7 @@ func observeChartBuild(obj *sourcev1.HelmChart, build *chart.Build, err error) { sigVerMsg.WriteString(fmt.Sprintf(" using key with fingeprint: '%X'", build.VerificationSignature.KeyFingerprint)) sigVerMsg.WriteString(fmt.Sprintf(" and hash verified: '%s'", build.VerificationSignature.FileHash)) - conditions.MarkTrue(obj, sourcev1.SourceVerifiedCondition, sourcev1.ChartVerifiedSucceededReason, sigVerMsg.String()) + conditions.MarkTrue(obj, sourcev1.SourceVerifiedCondition, sourcev1.ChartVerificationSucceededReason, sigVerMsg.String()) } else { conditions.Delete(obj, sourcev1.SourceVerifiedCondition) } diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go index 51a701431..255e42b70 100644 --- a/controllers/helmchart_controller_test.go +++ b/controllers/helmchart_controller_test.go @@ -551,7 +551,7 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { g.Expect(obj.Status.ObservedSourceArtifactRevision).To(Equal(gitArtifact.Revision)) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled 'helmchart' chart with version '0.1.0'"), - *conditions.TrueCondition(sourcev1.SourceVerifiedCondition, sourcev1.ChartVerifiedSucceededReason, "chart signed by: 'TestUser' using key with fingeprint: '943CB5929ECDA2B5B5EC88BC7035BA97D32A87C1' and hash verified: 'sha256:007c7b7446eebcb18caeffe9898a3356ba1795f54df40ad39cfcc7382874a10a'"), + *conditions.TrueCondition(sourcev1.SourceVerifiedCondition, sourcev1.ChartVerificationSucceededReason, "chart signed by: 'TestUser' using key with fingeprint: '943CB5929ECDA2B5B5EC88BC7035BA97D32A87C1' and hash verified: 'sha256:007c7b7446eebcb18caeffe9898a3356ba1795f54df40ad39cfcc7382874a10a'"), })) }, cleanFunc: func(g *WithT, build *chart.Build) { diff --git a/internal/helm/chart/builder_remote.go b/internal/helm/chart/builder_remote.go index 9c34a5f36..c16e56e02 100644 --- a/internal/helm/chart/builder_remote.go +++ b/internal/helm/chart/builder_remote.go @@ -158,13 +158,12 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o // This is needed, since the verification will work only if the .tgz file is untampered. // But we write the packaged chart to disk under a different name, so the provenance file // will not be valid for this _new_ packaged chart. - chart, err := util.WriteToFile(chartBuf, fmt.Sprintf("%s-%s.tgz", cv.Name, cv.Version)) - defer os.Remove(chart.Name()) + chart, err := util.WriteToTempFile(chartBuf, fmt.Sprintf("%s-%s.tgz", cv.Name, cv.Version), true) if err != nil { return nil, err } + defer os.Remove(chart.Name()) ver, err := verifyChartWithProvFile(bytes.NewReader(opts.Keyring), chart.Name(), provFilePath) - if err != nil { return nil, err } @@ -245,7 +244,7 @@ func mergeChartValues(chart *helmchart.Chart, paths []string) (map[string]interf // validatePackageAndWriteToPath atomically writes the packaged chart from reader // to out while validating it by loading the chart metadata from the archive. func validatePackageAndWriteToPath(b []byte, out string) error { - tmpFile, err := util.WriteToTempFile(b, out) + tmpFile, err := util.WriteToTempFile(b, out, false) defer os.Remove(tmpFile.Name()) if err != nil { diff --git a/internal/helm/repository/chart_repository.go b/internal/helm/repository/chart_repository.go index b9b6482f2..b5965e51a 100644 --- a/internal/helm/repository/chart_repository.go +++ b/internal/helm/repository/chart_repository.go @@ -216,7 +216,7 @@ func (r *ChartRepository) DownloadProvenanceFile(chart *repo.ChartVersion, path if err != nil { return err } - tmpFile, err := util.WriteToTempFile(res.Bytes(), path) + tmpFile, err := util.WriteToTempFile(res.Bytes(), path, false) defer os.Remove(tmpFile.Name()) if err != nil { diff --git a/internal/util/file.go b/internal/util/file.go index 403799b60..28636ae7d 100644 --- a/internal/util/file.go +++ b/internal/util/file.go @@ -20,6 +20,8 @@ import ( "fmt" "os" "path/filepath" + + "github.com/fluxcd/source-controller/internal/fs" ) func writeBytesToFile(bytes []byte, file *os.File) error { @@ -33,21 +35,24 @@ func writeBytesToFile(bytes []byte, file *os.File) error { return nil } -// Writes the provided bytes to a file at the given path and returns the file handle. -func WriteToFile(bytes []byte, path string) (*os.File, error) { - file, err := os.Create(path) - if err != nil { - return nil, fmt.Errorf("failed to create temporary file for chart %s: %w", path, err) - } - return file, writeBytesToFile(bytes, file) -} - // Writes the provided bytes to a temp file with the name provided in the path and -// returns the file handle. -func WriteToTempFile(bytes []byte, out string) (*os.File, error) { +// returns the file handle. If renameToOriginal is true, it renames the temp file to +// the intended file name (since temp file names have random bytes as suffix). +func WriteToTempFile(bytes []byte, out string, renameToOriginal bool) (*os.File, error) { file, err := os.CreateTemp("", filepath.Base(out)) if err != nil { return nil, fmt.Errorf("failed to create temporary file %s: %w", filepath.Base(out), err) } - return file, writeBytesToFile(bytes, file) + err = writeBytesToFile(bytes, file) + if err != nil { + return nil, err + } + if renameToOriginal { + err = fs.RenameWithFallback(file.Name(), filepath.Join("/tmp", filepath.Base(out))) + file, err = os.Open(filepath.Join("/tmp", filepath.Base(out))) + if err != nil { + return nil, fmt.Errorf("failed to rename temporary file %s: %w", filepath.Base(out), err) + } + } + return file, nil }