fluxcd
/
source-controller
mirror of https://github.com/fluxcd/source-controller.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

helm: switch to our own chart loader package 

This includes some rewiring of tests, and slight changes in how we work
with the local chart reference. `Path` is expected to be relative to
`WorkDir`, and both fields are now mandatory.

Signed-off-by: Hidde Beydals <hello@hidde.co>
This commit is contained in:
Hidde Beydals 2022-04-08 11:34:56 +02:00
parent b9063d7362
commit e85ea781e2
9 changed files with 93 additions and 62 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
controllers/helmchart_controller.go
View File

@ -28,7 +28,6 @@ import (
"strings"
"time"
securejoin "github.com/cyphar/filepath-securejoin"
helmgetter "helm.sh/helm/v3/pkg/getter"
helmrepo "helm.sh/helm/v3/pkg/repo"
corev1 "k8s.io/api/core/v1"
@ -609,18 +608,6 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj
}
}
// Calculate (secure) absolute chart path
chartPath, err := securejoin.SecureJoin(sourceDir, obj.Spec.Chart)
if err != nil {
e := &serror.Stalling{
Err: fmt.Errorf("path calculation for chart '%s' failed: %w", obj.Spec.Chart, err),
Reason: "IllegalPath",
}
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
// We are unable to recover from this change without a change in generation
return sreconcile.ResultEmpty, e
}
// Setup dependency manager
dm := chart.NewDependencyManager(
chart.WithRepositoryCallback(r.namespacedChartRepositoryCallback(ctx, obj.GetNamespace())),
@ -673,7 +660,7 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj
cb := chart.NewLocalBuilder(dm)
build, err := cb.Build(ctx, chart.LocalReference{
WorkDir: sourceDir,
Path: chartPath,
Path: obj.Spec.Chart,
}, util.TempPathForObj("", ".tgz", obj), opts)
if err != nil {
return sreconcile.ResultEmpty, err

11
internal/helm/chart/builder.go
View File

@ -43,16 +43,25 @@ type LocalReference struct {
// WorkDir used as chroot during build operations.
// File references are not allowed to traverse outside it.
WorkDir string
// Path of the chart on the local filesystem.
// Path of the chart on the local filesystem relative to WorkDir.
Path string
}
// Validate returns an error if the LocalReference does not have
// a Path set.
func (r LocalReference) Validate() error {
if r.WorkDir == "" {
return fmt.Errorf("no work dir set for local chart reference")
}
if r.Path == "" {
return fmt.Errorf("no path set for local chart reference")
}
if !filepath.IsAbs(r.WorkDir) {
return fmt.Errorf("local chart reference work dir is expected to be absolute")
}
if filepath.IsAbs(r.Path) {
return fmt.Errorf("local chart reference path is expected to be relative")
}
return nil
}

24
internal/helm/chart/builder_local.go
View File

@ -24,10 +24,11 @@ import (
"github.com/Masterminds/semver/v3"
securejoin "github.com/cyphar/filepath-securejoin"
"helm.sh/helm/v3/pkg/chart/loader"
"sigs.k8s.io/yaml"
"github.com/fluxcd/pkg/runtime/transform"
"github.com/fluxcd/source-controller/internal/helm/chart/secureloader"
)
type localChartBuilder struct {
@ -75,7 +76,11 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string,
// Load the chart metadata from the LocalReference to ensure it points
// to a chart
curMeta, err := LoadChartMetadata(localRef.Path)
securePath, err := securejoin.SecureJoin(localRef.WorkDir, localRef.Path)
if err != nil {
return nil, &BuildError{Reason: ErrChartReference, Err: err}
}
curMeta, err := LoadChartMetadata(securePath)
if err != nil {
return nil, &BuildError{Reason: ErrChartReference, Err: err}
}
@ -101,7 +106,7 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string,
result.Version = ver.String()
}
isChartDir := pathIsDir(localRef.Path)
isChartDir := pathIsDir(securePath)
requiresPackaging := isChartDir || opts.VersionMetadata != "" || len(opts.GetValuesFiles()) != 0
// If all the following is true, we do not need to package the chart:
@ -127,7 +132,7 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string,
// If the chart at the path is already packaged and no custom values files
// options are set, we can copy the chart without making modifications
if !requiresPackaging {
if err = copyFileToPath(localRef.Path, p); err != nil {
if err = copyFileToPath(securePath, p); err != nil {
return result, &BuildError{Reason: ErrChartPull, Err: err}
}
result.Path = p
@ -145,15 +150,16 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string,
// At this point we are certain we need to load the chart;
// either to package it because it originates from a directory,
// or because we have merged values and need to repackage
chart, err := loader.Load(localRef.Path)
loadedChart, err := secureloader.Load(localRef.WorkDir, localRef.Path)
if err != nil {
return result, &BuildError{Reason: ErrChartPackage, Err: err}
}
// Set earlier resolved version (with metadata)
chart.Metadata.Version = result.Version
loadedChart.Metadata.Version = result.Version
// Overwrite default values with merged values, if any
if ok, err = OverwriteChartDefaultValues(chart, mergedValues); ok || err != nil {
if ok, err = OverwriteChartDefaultValues(loadedChart, mergedValues); ok || err != nil {
if err != nil {
return result, &BuildError{Reason: ErrValuesFilesMerge, Err: err}
}
@ -166,13 +172,13 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string,
err = fmt.Errorf("local chart builder requires dependency manager for unpackaged charts")
return result, &BuildError{Reason: ErrDependencyBuild, Err: err}
}
if result.ResolvedDependencies, err = b.dm.Build(ctx, ref, chart); err != nil {
if result.ResolvedDependencies, err = b.dm.Build(ctx, ref, loadedChart); err != nil {
return result, &BuildError{Reason: ErrDependencyBuild, Err: err}
}
}
// Package the chart
if err = packageToPath(chart, p); err != nil {
if err = packageToPath(loadedChart, p); err != nil {
return result, &BuildError{Reason: ErrChartPackage, Err: err}
}
result.Path = p

41
internal/helm/chart/builder_local_test.go
View File

@ -26,10 +26,10 @@ import (
. "github.com/onsi/gomega"
"github.com/otiai10/copy"
helmchart "helm.sh/helm/v3/pkg/chart"
"helm.sh/helm/v3/pkg/chart/loader"
"helm.sh/helm/v3/pkg/chartutil"
"helm.sh/helm/v3/pkg/repo"
"github.com/fluxcd/source-controller/internal/helm/chart/secureloader"
"github.com/fluxcd/source-controller/internal/helm/repository"
)
@ -86,31 +86,31 @@ func TestLocalBuilder_Build(t *testing.T) {
},
{
name: "invalid local reference - no file",
reference: LocalReference{Path: "/tmp/non-existent-path.xyz"},
reference: LocalReference{WorkDir: "/tmp", Path: "non-existent-path.xyz"},
wantErr: "no such file or directory",
},
{
name: "invalid version metadata",
reference: LocalReference{Path: "./../testdata/charts/helmchart"},
reference: LocalReference{Path: "../testdata/charts/helmchart"},
buildOpts: BuildOptions{VersionMetadata: "^"},
wantErr: "Invalid Metadata string",
},
{
name: "with version metadata",
reference: LocalReference{Path: "./../testdata/charts/helmchart"},
reference: LocalReference{Path: "../testdata/charts/helmchart"},
buildOpts: BuildOptions{VersionMetadata: "foo"},
wantVersion: "0.1.0+foo",
wantPackaged: true,
},
{
name: "already packaged chart",
reference: LocalReference{Path: "./../testdata/charts/helmchart-0.1.0.tgz"},
reference: LocalReference{Path: "../testdata/charts/helmchart-0.1.0.tgz"},
wantVersion: "0.1.0",
wantPackaged: false,
},
{
name: "default values",
reference: LocalReference{Path: "./../testdata/charts/helmchart"},
reference: LocalReference{Path: "../testdata/charts/helmchart"},
wantValues: chartutil.Values{
"replicaCount": float64(1),
},
@ -119,7 +119,7 @@ func TestLocalBuilder_Build(t *testing.T) {
},
{
name: "with values files",
reference: LocalReference{Path: "./../testdata/charts/helmchart"},
reference: LocalReference{Path: "../testdata/charts/helmchart"},
buildOpts: BuildOptions{
ValuesFiles: []string{"custom-values1.yaml", "custom-values2.yaml"},
},
@ -145,7 +145,7 @@ fullnameOverride: "full-foo-name-override"`),
},
{
name: "chart with dependencies",
reference: LocalReference{Path: "./../testdata/charts/helmchartwithdeps"},
reference: LocalReference{Path: "../testdata/charts/helmchartwithdeps"},
repositories: map[string]*repository.ChartRepository{
"https://grafana.github.io/helm-charts/": mockRepo(),
},
@ -164,11 +164,11 @@ fullnameOverride: "full-foo-name-override"`),
},
{
name: "v1 chart with dependencies",
reference: LocalReference{Path: "./../testdata/charts/helmchartwithdeps-v1"},
reference: LocalReference{Path: "../testdata/charts/helmchartwithdeps-v1"},
repositories: map[string]*repository.ChartRepository{
"https://grafana.github.io/helm-charts/": mockRepo(),
},
dependentChartPaths: []string{"./../testdata/charts/helmchart-v1"},
dependentChartPaths: []string{"../testdata/charts/helmchart-v1"},
wantVersion: "0.3.0",
wantPackaged: true,
},
@ -184,13 +184,23 @@ fullnameOverride: "full-foo-name-override"`),
// Only if the reference is a LocalReference, set the WorkDir.
localRef, ok := tt.reference.(LocalReference)
if ok {
// If the source chart path is valid, copy it into the workdir
// and update the localRef.Path with the copied local chart
// path.
if localRef.Path != "" {
_, err := os.Lstat(localRef.Path)
if err == nil {
helmchartDir := filepath.Join(workDir, "testdata", "charts", filepath.Base(localRef.Path))
g.Expect(copy.Copy(localRef.Path, helmchartDir)).ToNot(HaveOccurred())
}
}
localRef.WorkDir = workDir
tt.reference = localRef
}
// Write value file in the base dir.
for _, f := range tt.valuesFiles {
vPath := filepath.Join(workDir, f.Name)
vPath := filepath.Join(localRef.WorkDir, f.Name)
g.Expect(os.WriteFile(vPath, f.Data, 0644)).ToNot(HaveOccurred())
}
@ -223,7 +233,7 @@ fullnameOverride: "full-foo-name-override"`),
g.Expect(cb.Path).ToNot(BeEmpty(), "empty Build.Path")
// Load the resulting chart and verify the values.
resultChart, err := loader.Load(cb.Path)
resultChart, err := secureloader.LoadFile(cb.Path)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(resultChart.Metadata.Version).To(Equal(tt.wantVersion))
@ -241,7 +251,7 @@ func TestLocalBuilder_Build_CachedChart(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())
defer os.RemoveAll(workDir)
reference := LocalReference{Path: "./../testdata/charts/helmchart"}
testChartPath := "./../testdata/charts/helmchart"
dm := NewDependencyManager()
b := NewLocalBuilder(dm)
@ -250,6 +260,11 @@ func TestLocalBuilder_Build_CachedChart(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())
defer os.RemoveAll(tmpDir)
// Copy the source chart into the workdir.
g.Expect(copy.Copy(testChartPath, filepath.Join(workDir, "testdata", "charts", filepath.Base("helmchart")))).ToNot(HaveOccurred())
reference := LocalReference{WorkDir: workDir, Path: testChartPath}
// Build first time.
targetPath := filepath.Join(tmpDir, "chart1.tgz")
buildOpts := BuildOptions{}

4
internal/helm/chart/builder_remote.go
View File

@ -25,13 +25,13 @@ import (
"github.com/Masterminds/semver/v3"
helmchart "helm.sh/helm/v3/pkg/chart"
"helm.sh/helm/v3/pkg/chart/loader"
"helm.sh/helm/v3/pkg/chartutil"
"sigs.k8s.io/yaml"
"github.com/fluxcd/pkg/runtime/transform"
"github.com/fluxcd/source-controller/internal/fs"
"github.com/fluxcd/source-controller/internal/helm/chart/secureloader"
"github.com/fluxcd/source-controller/internal/helm/repository"
)
@ -145,7 +145,7 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o
// Load the chart and merge chart values
var chart *helmchart.Chart
if chart, err = loader.LoadArchive(res); err != nil {
if chart, err = secureloader.LoadArchive(res); err != nil {
err = fmt.Errorf("failed to load downloaded chart: %w", err)
return result, &BuildError{Reason: ErrChartPackage, Err: err}
}

4
internal/helm/chart/builder_remote_test.go
View File

@ -27,10 +27,10 @@ import (
. "github.com/onsi/gomega"
helmchart "helm.sh/helm/v3/pkg/chart"
"helm.sh/helm/v3/pkg/chart/loader"
"helm.sh/helm/v3/pkg/chartutil"
helmgetter "helm.sh/helm/v3/pkg/getter"
"github.com/fluxcd/source-controller/internal/helm/chart/secureloader"
"github.com/fluxcd/source-controller/internal/helm/repository"
)
@ -186,7 +186,7 @@ entries:
g.Expect(cb.Path).ToNot(BeEmpty(), "empty Build.Path")
// Load the resulting chart and verify the values.
resultChart, err := loader.Load(cb.Path)
resultChart, err := secureloader.LoadFile(cb.Path)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(resultChart.Metadata.Version).To(Equal(tt.wantVersion))

26
internal/helm/chart/builder_test.go
View File

@ -24,8 +24,9 @@ import (
"testing"
. "github.com/onsi/gomega"
"helm.sh/helm/v3/pkg/chart/loader"
"helm.sh/helm/v3/pkg/chartutil"
"github.com/fluxcd/source-controller/internal/helm/chart/secureloader"
)
func TestLocalReference_Validate(t *testing.T) {
@ -35,18 +36,29 @@ func TestLocalReference_Validate(t *testing.T) {
wantErr string
}{
{
name: "ref with path",
ref: LocalReference{Path: "/a/path"},
name: "ref with path and work dir",
ref: LocalReference{WorkDir: "/workdir/", Path: "./a/path"},
},
{
name: "ref with path and work dir",
ref: LocalReference{Path: "/a/path", WorkDir: "/with/a/workdir"},
name: "ref without work dir",
ref: LocalReference{Path: "/a/path"},
wantErr: "no work dir set for local chart reference",
},
{
name: "ref with relative work dir",
ref: LocalReference{WorkDir: "../a/path", Path: "foo"},
wantErr: "local chart reference work dir is expected to be absolute",
},
{
name: "ref without path",
ref: LocalReference{WorkDir: "/just/a/workdir"},
wantErr: "no path set for local chart reference",
},
{
name: "ref with an absolute path",
ref: LocalReference{WorkDir: "/a/path", Path: "/foo"},
wantErr: "local chart reference path is expected to be relative",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
@ -210,7 +222,7 @@ func TestChartBuildResult_String(t *testing.T) {
func Test_packageToPath(t *testing.T) {
g := NewWithT(t)
chart, err := loader.Load("../testdata/charts/helmchart-0.1.0.tgz")
chart, err := secureloader.LoadFile("../testdata/charts/helmchart-0.1.0.tgz")
g.Expect(err).ToNot(HaveOccurred())
g.Expect(chart).ToNot(BeNil())
@ -219,7 +231,7 @@ func Test_packageToPath(t *testing.T) {
err = packageToPath(chart, out)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(out).To(BeARegularFile())
_, err = loader.Load(out)
_, err = secureloader.LoadFile(out)
g.Expect(err).ToNot(HaveOccurred())
}

14
internal/helm/chart/dependency_manager.go
View File

@ -30,8 +30,8 @@ import (
"golang.org/x/sync/errgroup"
"golang.org/x/sync/semaphore"
helmchart "helm.sh/helm/v3/pkg/chart"
"helm.sh/helm/v3/pkg/chart/loader"
"github.com/fluxcd/source-controller/internal/helm/chart/secureloader"
"github.com/fluxcd/source-controller/internal/helm/repository"
)
@ -191,7 +191,7 @@ func (dm *DependencyManager) addLocalDependency(ref LocalReference, c *chartWith
if _, err := os.Stat(sLocalChartPath); err != nil {
if os.IsNotExist(err) {
return fmt.Errorf("no chart found at '%s' (reference '%s')", sLocalChartPath, dep.Repository)
return fmt.Errorf("no chart found at '%s' (reference '%s')", strings.TrimPrefix(sLocalChartPath, ref.WorkDir), dep.Repository)
}
return err
}
@ -202,7 +202,7 @@ func (dm *DependencyManager) addLocalDependency(ref LocalReference, c *chartWith
return err
}
ch, err := loader.Load(sLocalChartPath)
ch, err := secureloader.Load(ref.WorkDir, sLocalChartPath)
if err != nil {
return fmt.Errorf("failed to load chart from '%s' (reference '%s'): %w",
strings.TrimPrefix(sLocalChartPath, ref.WorkDir), dep.Repository, err)
@ -245,7 +245,7 @@ func (dm *DependencyManager) addRemoteDependency(chart *chartWithLock, dep *helm
if err != nil {
return fmt.Errorf("chart download of version '%s' failed: %w", ver.Version, err)
}
ch, err := loader.LoadArchive(res)
ch, err := secureloader.LoadArchive(res)
if err != nil {
return fmt.Errorf("failed to load downloaded archive of version '%s': %w", ver.Version, err)
}
@ -290,11 +290,7 @@ func (dm *DependencyManager) secureLocalChartPath(ref LocalReference, dep *helmc
if localUrl.Scheme != "" && localUrl.Scheme != "file" {
return "", fmt.Errorf("'%s' is not a local chart reference", dep.Repository)
}
relPath, err := filepath.Rel(ref.WorkDir, ref.Path)
if err != nil {
relPath = ref.Path
}
return securejoin.SecureJoin(ref.WorkDir, filepath.Join(relPath, localUrl.Host, localUrl.Path))
return securejoin.SecureJoin(ref.WorkDir, filepath.Join(ref.Path, localUrl.Host, localUrl.Path))
}
// collectMissing returns a map with dependencies from reqs that are missing

16
internal/helm/chart/dependency_manager_test.go
View File

@ -28,10 +28,10 @@ import (
. "github.com/onsi/gomega"
helmchart "helm.sh/helm/v3/pkg/chart"
"helm.sh/helm/v3/pkg/chart/loader"
helmgetter "helm.sh/helm/v3/pkg/getter"
"helm.sh/helm/v3/pkg/repo"
"github.com/fluxcd/source-controller/internal/helm/chart/secureloader"
"github.com/fluxcd/source-controller/internal/helm/repository"
)
@ -166,14 +166,16 @@ func TestDependencyManager_Build(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)
chart, err := loader.Load(filepath.Join(tt.baseDir, tt.path))
chart, err := secureloader.Load(tt.baseDir, tt.path)
g.Expect(err).ToNot(HaveOccurred())
dm := NewDependencyManager(
WithRepositories(tt.repositories),
WithRepositoryCallback(tt.getChartRepositoryCallback),
)
got, err := dm.Build(context.TODO(), LocalReference{WorkDir: tt.baseDir, Path: tt.path}, chart)
absBaseDir, err := filepath.Abs(tt.baseDir)
g.Expect(err).ToNot(HaveOccurred())
got, err := dm.Build(context.TODO(), LocalReference{WorkDir: absBaseDir, Path: tt.path}, chart)
if tt.wantErr != "" {
g.Expect(err).To(HaveOccurred())
@ -262,7 +264,7 @@ func TestDependencyManager_addLocalDependency(t *testing.T) {
Version: chartVersion,
Repository: "file://../../../absolutely/invalid",
},
wantErr: "no chart found at '../testdata/charts/absolutely/invalid'",
wantErr: "no chart found at '/absolutely/invalid'",
},
{
name: "invalid chart archive",
@ -289,7 +291,11 @@ func TestDependencyManager_addLocalDependency(t *testing.T) {
dm := NewDependencyManager()
chart := &helmchart.Chart{}
err := dm.addLocalDependency(LocalReference{WorkDir: "../testdata/charts", Path: "helmchartwithdeps"},
absWorkDir, err := filepath.Abs("../testdata/charts")
g.Expect(err).ToNot(HaveOccurred())
err = dm.addLocalDependency(LocalReference{WorkDir: absWorkDir, Path: "helmchartwithdeps"},
&chartWithLock{Chart: chart}, tt.dep)
if tt.wantErr != "" {
g.Expect(err).To(HaveOccurred())