From ce083d969dd131a17e4167a6ccd151ef577fd5e0 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Sat, 13 Nov 2021 00:16:59 +0100 Subject: [PATCH] Further refactor `ChartBuilder` into interface Signed-off-by: Hidde Beydals --- internal/helm/chart.go | 51 +- internal/helm/chart_builder.go | 416 ++++------------ internal/helm/chart_builder_local.go | 190 +++++++ internal/helm/chart_builder_local_test.go | 137 ++++++ internal/helm/chart_builder_remote.go | 199 ++++++++ internal/helm/chart_builder_remote_test.go | 118 +++++ internal/helm/chart_builder_test.go | 543 +-------------------- internal/helm/chart_test.go | 20 +- internal/helm/dependency_manager.go | 136 +++--- internal/helm/dependency_manager_test.go | 38 +- internal/helm/getter.go | 82 ++-- internal/helm/getter_test.go | 21 +- internal/helm/helm.go | 29 ++ internal/helm/repository.go | 12 +- internal/helm/repository_test.go | 2 +- 15 files changed, 993 insertions(+), 1001 deletions(-) create mode 100644 internal/helm/chart_builder_local.go create mode 100644 internal/helm/chart_builder_local_test.go create mode 100644 internal/helm/chart_builder_remote.go create mode 100644 internal/helm/chart_builder_remote_test.go create mode 100644 internal/helm/helm.go diff --git a/internal/helm/chart.go b/internal/helm/chart.go index dcc868c1d..4f89cab61 100644 --- a/internal/helm/chart.go +++ b/internal/helm/chart.go @@ -19,6 +19,7 @@ package helm import ( "archive/tar" "bufio" + "bytes" "compress/gzip" "errors" "fmt" @@ -35,30 +36,35 @@ import ( ) // OverwriteChartDefaultValues overwrites the chart default values file with the given data. -func OverwriteChartDefaultValues(chart *helmchart.Chart, data []byte) (bool, error) { - // Read override values file data - values, err := chartutil.ReadValues(data) - if err != nil { - return false, fmt.Errorf("failed to parse provided override values file data") +func OverwriteChartDefaultValues(chart *helmchart.Chart, vals chartutil.Values) (bool, error) { + if vals == nil { + return false, nil + } + + var bVals bytes.Buffer + if len(vals) > 0 { + if err := vals.Encode(&bVals); err != nil { + return false, err + } } // Replace current values file in Raw field for _, f := range chart.Raw { if f.Name == chartutil.ValuesfileName { // Do nothing if contents are equal - if reflect.DeepEqual(f.Data, data) { + if reflect.DeepEqual(f.Data, bVals.Bytes()) { return false, nil } // Replace in Files field for _, f := range chart.Files { if f.Name == chartutil.ValuesfileName { - f.Data = data + f.Data = bVals.Bytes() } } - f.Data = data - chart.Values = values + f.Data = bVals.Bytes() + chart.Values = vals.AsMap() return true, nil } } @@ -100,7 +106,21 @@ func LoadChartMetadataFromDir(dir string) (*helmchart.Metadata, error) { m.APIVersion = helmchart.APIVersionV1 } - b, err = os.ReadFile(filepath.Join(dir, "requirements.yaml")) + fp := filepath.Join(dir, "requirements.yaml") + stat, err := os.Stat(fp) + if (err != nil && !errors.Is(err, os.ErrNotExist)) || stat != nil { + if err != nil { + return nil, err + } + if stat.IsDir() { + return nil, fmt.Errorf("'%s' is a directory", stat.Name()) + } + if stat.Size() > MaxChartFileSize { + return nil, fmt.Errorf("size of '%s' exceeds '%d' limit", stat.Name(), MaxChartFileSize) + } + } + + b, err = os.ReadFile(fp) if err != nil && !errors.Is(err, os.ErrNotExist) { return nil, err } @@ -115,6 +135,17 @@ func LoadChartMetadataFromDir(dir string) (*helmchart.Metadata, error) { // LoadChartMetadataFromArchive loads the chart.Metadata from the "Chart.yaml" file in the archive at the given path. // It takes "requirements.yaml" files into account, and is therefore compatible with the chart.APIVersionV1 format. func LoadChartMetadataFromArchive(archive string) (*helmchart.Metadata, error) { + stat, err := os.Stat(archive) + if err != nil || stat.IsDir() { + if err == nil { + err = fmt.Errorf("'%s' is a directory", stat.Name()) + } + return nil, err + } + if stat.Size() > MaxChartSize { + return nil, fmt.Errorf("size of chart '%s' exceeds '%d' limit", stat.Name(), MaxChartSize) + } + f, err := os.Open(archive) if err != nil { return nil, err diff --git a/internal/helm/chart_builder.go b/internal/helm/chart_builder.go index 7b90cba81..4177983c6 100644 --- a/internal/helm/chart_builder.go +++ b/internal/helm/chart_builder.go @@ -22,338 +22,145 @@ import ( "os" "path/filepath" "strings" - "sync" - securejoin "github.com/cyphar/filepath-securejoin" "github.com/fluxcd/source-controller/internal/fs" 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" ) -// ChartBuilder aims to efficiently build a Helm chart from a directory or packaged chart. -// It avoids or delays loading the chart into memory in full, working with chart.Metadata -// as much as it can, and returns early (by copying over the already packaged source chart) -// if no modifications were made during the build process. -type ChartBuilder struct { - // baseDir is the chroot for the chart builder when path isDir. - // It must be (a higher) relative to path. File references (during e.g. - // value file merge operations) are not allowed to traverse out of it. - baseDir string - - // path is the file or directory path to a chart source. - path string - - // chart holds a (partly) loaded chart.Chart, it contains at least the - // chart.Metadata, which may expand to the full chart.Chart if required - // for Build operations. - chart *helmchart.Chart - - // valueFiles holds a list of path references of valueFiles that should be - // merged and packaged as a single "values.yaml" during Build. - valueFiles []string - - // repositories holds an index of repository URLs and their ChartRepository. - // They are used to configure a DependencyManager for missing chart dependencies - // if isDir is true. - repositories map[string]*ChartRepository - - // getChartRepositoryCallback is used to configure a DependencyManager for - // missing chart dependencies if isDir is true. - getChartRepositoryCallback GetChartRepositoryCallback - - mu sync.Mutex -} - -// NewChartBuilder constructs a new ChartBuilder for the given chart path. -// It returns an error if no chart.Metadata can be loaded from the path. -func NewChartBuilder(path string) (*ChartBuilder, error) { - metadata, err := LoadChartMetadata(path) - if err != nil { - return nil, fmt.Errorf("could not create new chart builder: %w", err) - } - return &ChartBuilder{ - path: path, - chart: &helmchart.Chart{ - Metadata: metadata, - }, - }, nil +// ChartReference holds information to locate a chart. +type ChartReference interface { + // Validate returns an error if the ChartReference is not valid according + // to the spec of the interface implementation. + Validate() error } -// WithBaseDir configures the base dir on the ChartBuilder. -func (b *ChartBuilder) WithBaseDir(p string) *ChartBuilder { - b.mu.Lock() - b.baseDir = p - b.mu.Unlock() - return b -} - -// WithValueFiles appends the given paths to the ChartBuilder's valueFiles. -func (b *ChartBuilder) WithValueFiles(path ...string) *ChartBuilder { - b.mu.Lock() - b.valueFiles = append(b.valueFiles, path...) - b.mu.Unlock() - return b -} - -// WithChartRepository indexes the given ChartRepository by the NormalizeChartRepositoryURL, -// used to configure the DependencyManager if the chart is not packaged. -func (b *ChartBuilder) WithChartRepository(url string, index *ChartRepository) *ChartBuilder { - b.mu.Lock() - b.repositories[NormalizeChartRepositoryURL(url)] = index - b.mu.Unlock() - return b -} - -// WithChartRepositoryCallback configures the GetChartRepositoryCallback used by the -// DependencyManager if the chart is not packaged. -func (b *ChartBuilder) WithChartRepositoryCallback(c GetChartRepositoryCallback) *ChartBuilder { - b.mu.Lock() - b.getChartRepositoryCallback = c - b.mu.Unlock() - return b -} - -// ChartBuildResult contains the ChartBuilder result, including build specific -// information about the chart. -type ChartBuildResult struct { - // SourceIsDir indicates if the chart was build from a directory. - SourceIsDir bool - // Path contains the absolute path to the packaged chart. +// LocalChartReference contains sufficient information to locate a chart on the +// local filesystem. +type LocalChartReference struct { + // BaseDir used as chroot during build operations. + // File references are not allowed to traverse outside it. + BaseDir string + // Path of the chart on the local filesystem. Path string - // ValuesOverwrite holds a structured map with the merged values used - // to overwrite chart default "values.yaml". - ValuesOverwrite map[string]interface{} - // CollectedDependencies contains the number of missing local and remote - // dependencies that were collected by the DependencyManager before building - // the chart. - CollectedDependencies int - // Packaged indicates if the ChartBuilder has packaged the chart. - // This can for example be false if SourceIsDir is false and ValuesOverwrite - // is nil, which makes the ChartBuilder copy the chart source to Path without - // making any modifications. - Packaged bool } -// String returns the Path of the ChartBuildResult. -func (b *ChartBuildResult) String() string { - if b != nil { - return b.Path +// Validate returns an error if the LocalChartReference does not have +// a Path set. +func (r LocalChartReference) Validate() error { + if r.Path == "" { + return fmt.Errorf("no path set for local chart reference") } - return "" + return nil } -// Build attempts to build a new chart using ChartBuilder configuration, -// writing it to the provided path. -// It returns a ChartBuildResult containing all information about the resulting chart, -// or an error. -func (b *ChartBuilder) Build(ctx context.Context, p string) (_ *ChartBuildResult, err error) { - b.mu.Lock() - defer b.mu.Unlock() - - if b.chart == nil { - err = fmt.Errorf("chart build failed: no initial chart (metadata) loaded") - return - } - if b.path == "" { - err = fmt.Errorf("chart build failed: no path set") - return - } - - result := &ChartBuildResult{} - result.SourceIsDir = pathIsDir(b.path) - result.Path = p - - // Merge chart values - if err = b.mergeValues(result); err != nil { - err = fmt.Errorf("chart build failed: %w", err) - return - } - - // Ensure chart has all dependencies - if err = b.buildDependencies(ctx, result); err != nil { - err = fmt.Errorf("chart build failed: %w", err) - return - } - - // Package (or copy) chart - if err = b.packageChart(result); err != nil { - err = fmt.Errorf("chart package failed: %w", err) - return - } - return result, nil +// RemoteChartReference contains sufficient information to look up a chart in +// a ChartRepository. +type RemoteChartReference struct { + // Name of the chart. + Name string + // Version of the chart. + // Can be a Semver range, or empty for latest. + Version string } -// load lazy-loads chart.Chart into chart from the set path, it replaces any previously set -// chart.Metadata shim. -func (b *ChartBuilder) load() (err error) { - if b.chart == nil || len(b.chart.Files) <= 0 { - if b.path == "" { - return fmt.Errorf("failed to load chart: path not set") - } - chart, err := loader.Load(b.path) - if err != nil { - return fmt.Errorf("failed to load chart: %w", err) - } - b.chart = chart +// Validate returns an error if the RemoteChartReference does not have +// a Name set. +func (r RemoteChartReference) Validate() error { + if r.Name == "" { + return fmt.Errorf("no name set for remote chart reference") } - return + return nil } -// buildDependencies builds the missing dependencies for a chart from a directory. -// Using the chart using a NewDependencyManager and the configured repositories -// and getChartRepositoryCallback -// It returns the number of dependencies it collected, or an error. -func (b *ChartBuilder) buildDependencies(ctx context.Context, result *ChartBuildResult) (err error) { - if !result.SourceIsDir { - return - } - - if err = b.load(); err != nil { - err = fmt.Errorf("failed to ensure chart has no missing dependencies: %w", err) - return +// ChartBuilder is capable of building a (specific) ChartReference. +type ChartBuilder interface { + // Build builds and packages a Helm chart with the given ChartReference + // and BuildOptions and writes it to p. It returns the ChartBuild result, + // or an error. It may return an error for unsupported ChartReference + // implementations. + Build(ctx context.Context, ref ChartReference, p string, opts BuildOptions) (*ChartBuild, error) +} + +// BuildOptions provides a list of options for ChartBuilder.Build. +type BuildOptions struct { + // VersionMetadata can be set to SemVer build metadata as defined in + // the spec, and is included during packaging. + // Ref: https://semver.org/#spec-item-10 + VersionMetadata string + // ValueFiles can be set to a list of relative paths, used to compose + // and overwrite an alternative default "values.yaml" for the chart. + ValueFiles []string + // CachedChart can be set to the absolute path of a chart stored on + // the local filesystem, and is used for simple validation by metadata + // comparisons. + CachedChart string + // Force can be set to force the build of the chart, for example + // because the list of ValueFiles has changed. + Force bool +} + +// GetValueFiles returns BuildOptions.ValueFiles, except if it equals +// "values.yaml", which returns nil. +func (o BuildOptions) GetValueFiles() []string { + if len(o.ValueFiles) == 1 && filepath.Clean(o.ValueFiles[0]) == filepath.Clean(chartutil.ValuesfileName) { + return nil } - - dm := NewDependencyManager(b.chart, b.baseDir, strings.TrimLeft(b.path, b.baseDir)). - WithRepositories(b.repositories). - WithChartRepositoryCallback(b.getChartRepositoryCallback) - - result.CollectedDependencies, err = dm.Build(ctx) - return + return o.ValueFiles } -// mergeValues strategically merges the valueFiles, it merges using mergeFileValues -// or mergeChartValues depending on if the chart is sourced from a package or directory. -// Ir only calls load to propagate the chart if required by the strategy. -// It returns the merged values, or an error. -func (b *ChartBuilder) mergeValues(result *ChartBuildResult) (err error) { - if len(b.valueFiles) == 0 { - return - } +// ChartBuild contains the ChartBuilder.Build result, including specific +// information about the built chart like ResolvedDependencies. +type ChartBuild struct { + // Path is the absolute path to the packaged chart. + Path string + // Name of the packaged chart. + Name string + // Version of the packaged chart. + Version string + // ValueFiles is the list of files used to compose the chart's + // default "values.yaml". + ValueFiles []string + // ResolvedDependencies is the number of local and remote dependencies + // collected by the DependencyManager before building the chart. + ResolvedDependencies int + // Packaged indicates if the ChartBuilder has packaged the chart. + // This can for example be false if ValueFiles is empty and the chart + // source was already packaged. + Packaged bool +} - if result.SourceIsDir { - result.ValuesOverwrite, err = mergeFileValues(b.baseDir, b.valueFiles) - if err != nil { - err = fmt.Errorf("failed to merge value files: %w", err) - } - return +// Summary returns a human-readable summary of the ChartBuild. +func (b *ChartBuild) Summary() string { + if b == nil { + return "no chart build" } - // Values equal to default - if len(b.valueFiles) == 1 && b.valueFiles[0] == chartutil.ValuesfileName { - return - } + var s strings.Builder - if err = b.load(); err != nil { - err = fmt.Errorf("failed to merge chart values: %w", err) - return + action := "Fetched" + if b.Packaged { + action = "Packaged" } + s.WriteString(fmt.Sprintf("%s '%s' chart with version '%s'.", action, b.Name, b.Version)) - if result.ValuesOverwrite, err = mergeChartValues(b.chart, b.valueFiles); err != nil { - err = fmt.Errorf("failed to merge chart values: %w", err) - return + if b.Packaged && b.ResolvedDependencies > 0 { + s.WriteString(fmt.Sprintf(" Resolved %d dependencies before packaging.", b.ResolvedDependencies)) } - return nil -} -// packageChart determines if it should copyFileToPath or packageToPath -// based on the provided result. It sets Packaged on ChartBuildResult to -// true if packageToPath is successful. -func (b *ChartBuilder) packageChart(result *ChartBuildResult) error { - // If we are not building from a directory, and we do not have any - // replacement values, we can copy over the already packaged source - // chart without making any modifications - if !result.SourceIsDir && len(result.ValuesOverwrite) == 0 { - if err := copyFileToPath(b.path, result.Path); err != nil { - return fmt.Errorf("chart build failed: %w", err) - } - return nil + if len(b.ValueFiles) > 0 { + s.WriteString(fmt.Sprintf(" Merged %v value files into default chart values.", b.ValueFiles)) } - // Package chart to a new temporary directory - if err := packageToPath(b.chart, result.Path); err != nil { - return fmt.Errorf("chart build failed: %w", err) - } - result.Packaged = true - return nil + return s.String() } -// mergeChartValues merges the given chart.Chart Files paths into a single "values.yaml" map. -// It returns the merge result, or an error. -func mergeChartValues(chart *helmchart.Chart, paths []string) (map[string]interface{}, error) { - mergedValues := make(map[string]interface{}) - for _, p := range paths { - cfn := filepath.Clean(p) - if cfn == chartutil.ValuesfileName { - mergedValues = transform.MergeMaps(mergedValues, chart.Values) - continue - } - var b []byte - for _, f := range chart.Files { - if f.Name == cfn { - b = f.Data - break - } - } - if b == nil { - return nil, fmt.Errorf("no values file found at path '%s'", p) - } - values := make(map[string]interface{}) - if err := yaml.Unmarshal(b, &values); err != nil { - return nil, fmt.Errorf("unmarshaling values from '%s' failed: %w", p, err) - } - mergedValues = transform.MergeMaps(mergedValues, values) - } - return mergedValues, nil -} - -// mergeFileValues merges the given value file paths into a single "values.yaml" map. -// The provided (relative) paths may not traverse outside baseDir. It returns the merge -// result, or an error. -func mergeFileValues(baseDir string, paths []string) (map[string]interface{}, error) { - mergedValues := make(map[string]interface{}) - for _, p := range paths { - secureP, err := securejoin.SecureJoin(baseDir, p) - if err != nil { - return nil, err - } - if f, err := os.Stat(secureP); os.IsNotExist(err) || !f.Mode().IsRegular() { - return nil, fmt.Errorf("no values file found at path '%s' (reference '%s')", - strings.TrimPrefix(secureP, baseDir), p) - } - b, err := os.ReadFile(secureP) - if err != nil { - return nil, fmt.Errorf("could not read values from file '%s': %w", p, err) - } - values := make(map[string]interface{}) - err = yaml.Unmarshal(b, &values) - if err != nil { - return nil, fmt.Errorf("unmarshaling values from '%s' failed: %w", p, err) - } - mergedValues = transform.MergeMaps(mergedValues, values) - } - return mergedValues, nil -} - -// copyFileToPath attempts to copy in to out. It returns an error if out already exists. -func copyFileToPath(in, out string) error { - o, err := os.Create(out) - if err != nil { - return fmt.Errorf("failed to create copy target: %w", err) - } - defer o.Close() - i, err := os.Open(in) - if err != nil { - return fmt.Errorf("failed to open file to copy from: %w", err) - } - defer i.Close() - if _, err := o.ReadFrom(i); err != nil { - return fmt.Errorf("failed to read from source during copy: %w", err) +// String returns the Path of the ChartBuild. +func (b *ChartBuild) String() string { + if b != nil { + return b.Path } - return nil + return "" } // packageToPath attempts to package the given chart.Chart to the out filepath. @@ -368,17 +175,8 @@ func packageToPath(chart *helmchart.Chart, out string) error { if err != nil { return fmt.Errorf("failed to package chart: %w", err) } - return fs.RenameWithFallback(p, out) -} - -// pathIsDir returns a boolean indicating if the given path points to a directory. -// In case os.Stat on the given path returns an error it returns false as well. -func pathIsDir(p string) bool { - if p == "" { - return false + if err = fs.RenameWithFallback(p, out); err != nil { + return fmt.Errorf("failed to write chart to file: %w", err) } - if i, err := os.Stat(p); err != nil || !i.IsDir() { - return false - } - return true + return nil } diff --git a/internal/helm/chart_builder_local.go b/internal/helm/chart_builder_local.go new file mode 100644 index 000000000..7f78c1a19 --- /dev/null +++ b/internal/helm/chart_builder_local.go @@ -0,0 +1,190 @@ +/* +Copyright 2021 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package helm + +import ( + "context" + "fmt" + "os" + "strings" + + "github.com/Masterminds/semver/v3" + securejoin "github.com/cyphar/filepath-securejoin" + "github.com/fluxcd/pkg/runtime/transform" + "helm.sh/helm/v3/pkg/chart/loader" + "sigs.k8s.io/yaml" +) + +type localChartBuilder struct { + dm *DependencyManager +} + +// NewLocalChartBuilder returns a ChartBuilder capable of building a Helm +// chart with a LocalChartReference. For chart references pointing to a +// directory, the DependencyManager is used to resolve missing local and +// remote dependencies. +func NewLocalChartBuilder(dm *DependencyManager) ChartBuilder { + return &localChartBuilder{ + dm: dm, + } +} + +func (b *localChartBuilder) Build(ctx context.Context, ref ChartReference, p string, opts BuildOptions) (*ChartBuild, error) { + localRef, ok := ref.(LocalChartReference) + if !ok { + return nil, fmt.Errorf("expected local chart reference") + } + + if err := ref.Validate(); err != nil { + return nil, err + } + + // Load the chart metadata from the LocalChartReference to ensure it points + // to a chart + meta, err := LoadChartMetadata(localRef.Path) + if err != nil { + return nil, err + } + + result := &ChartBuild{} + result.Name = meta.Name + + // Set build specific metadata if instructed + result.Version = meta.Version + if opts.VersionMetadata != "" { + ver, err := semver.NewVersion(meta.Version) + if err != nil { + return nil, fmt.Errorf("failed to parse chart version from metadata as SemVer: %w", err) + } + if *ver, err = ver.SetMetadata(opts.VersionMetadata); err != nil { + return nil, fmt.Errorf("failed to set metadata on chart version: %w", err) + } + result.Version = ver.String() + } + + // If all the following is true, we do not need to package the chart: + // Chart version from metadata matches chart version for ref + // BuildOptions.Force is False + if opts.CachedChart != "" && !opts.Force { + if meta, err = LoadChartMetadataFromArchive(opts.CachedChart); err == nil && result.Version == meta.Version { + result.Path = opts.CachedChart + result.ValueFiles = opts.ValueFiles + return result, nil + } + } + + // If the chart at the path is already packaged and no custom value files + // options are set, we can copy the chart without making modifications + isChartDir := pathIsDir(localRef.Path) + if !isChartDir && len(opts.GetValueFiles()) == 0 { + if err := copyFileToPath(localRef.Path, p); err != nil { + return nil, err + } + result.Path = p + return result, nil + } + + // Merge chart values, if instructed + var mergedValues map[string]interface{} + if len(opts.GetValueFiles()) > 0 { + if mergedValues, err = mergeFileValues(localRef.BaseDir, opts.ValueFiles); err != nil { + return nil, fmt.Errorf("failed to merge value files: %w", err) + } + } + + // 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) + if err != nil { + return nil, err + } + // Set earlier resolved version (with metadata) + chart.Metadata.Version = result.Version + + // Overwrite default values with merged values, if any + if ok, err = OverwriteChartDefaultValues(chart, mergedValues); ok || err != nil { + if err != nil { + return nil, err + } + result.ValueFiles = opts.GetValueFiles() + } + + // Ensure dependencies are fetched if building from a directory + if isChartDir { + if b.dm == nil { + return nil, fmt.Errorf("local chart builder requires dependency manager for unpackaged charts") + } + if result.ResolvedDependencies, err = b.dm.Build(ctx, ref, chart); err != nil { + return nil, err + } + } + + // Package the chart + if err = packageToPath(chart, p); err != nil { + return nil, err + } + result.Path = p + result.Packaged = true + return result, nil +} + +// mergeFileValues merges the given value file paths into a single "values.yaml" map. +// The provided (relative) paths may not traverse outside baseDir. It returns the merge +// result, or an error. +func mergeFileValues(baseDir string, paths []string) (map[string]interface{}, error) { + mergedValues := make(map[string]interface{}) + for _, p := range paths { + secureP, err := securejoin.SecureJoin(baseDir, p) + if err != nil { + return nil, err + } + if f, err := os.Stat(secureP); os.IsNotExist(err) || !f.Mode().IsRegular() { + return nil, fmt.Errorf("no values file found at path '%s' (reference '%s')", + strings.TrimPrefix(secureP, baseDir), p) + } + b, err := os.ReadFile(secureP) + if err != nil { + return nil, fmt.Errorf("could not read values from file '%s': %w", p, err) + } + values := make(map[string]interface{}) + err = yaml.Unmarshal(b, &values) + if err != nil { + return nil, fmt.Errorf("unmarshaling values from '%s' failed: %w", p, err) + } + mergedValues = transform.MergeMaps(mergedValues, values) + } + return mergedValues, nil +} + +// copyFileToPath attempts to copy in to out. It returns an error if out already exists. +func copyFileToPath(in, out string) error { + o, err := os.Create(out) + if err != nil { + return fmt.Errorf("failed to create copy target: %w", err) + } + defer o.Close() + i, err := os.Open(in) + if err != nil { + return fmt.Errorf("failed to open file to copy from: %w", err) + } + defer i.Close() + if _, err := o.ReadFrom(i); err != nil { + return fmt.Errorf("failed to read from source during copy: %w", err) + } + return nil +} diff --git a/internal/helm/chart_builder_local_test.go b/internal/helm/chart_builder_local_test.go new file mode 100644 index 000000000..c2f16d694 --- /dev/null +++ b/internal/helm/chart_builder_local_test.go @@ -0,0 +1,137 @@ +/* +Copyright 2021 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package helm + +import ( + "os" + "path/filepath" + "testing" + + . "github.com/onsi/gomega" + helmchart "helm.sh/helm/v3/pkg/chart" +) + +func Test_mergeFileValues(t *testing.T) { + tests := []struct { + name string + files []*helmchart.File + paths []string + want map[string]interface{} + wantErr string + }{ + { + name: "merges values from files", + files: []*helmchart.File{ + {Name: "a.yaml", Data: []byte("a: b")}, + {Name: "b.yaml", Data: []byte("b: c")}, + {Name: "c.yaml", Data: []byte("b: d")}, + }, + paths: []string{"a.yaml", "b.yaml", "c.yaml"}, + want: map[string]interface{}{ + "a": "b", + "b": "d", + }, + }, + { + name: "illegal traverse", + paths: []string{"../../../traversing/illegally/a/p/a/b"}, + wantErr: "no values file found at path '/traversing/illegally/a/p/a/b'", + }, + { + name: "unmarshal error", + files: []*helmchart.File{ + {Name: "invalid", Data: []byte("abcd")}, + }, + paths: []string{"invalid"}, + wantErr: "unmarshaling values from 'invalid' failed", + }, + { + name: "error on invalid path", + paths: []string{"a.yaml"}, + wantErr: "no values file found at path '/a.yaml'", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + baseDir, err := os.MkdirTemp("", "merge-file-values-*") + g.Expect(err).ToNot(HaveOccurred()) + defer os.RemoveAll(baseDir) + + for _, f := range tt.files { + g.Expect(os.WriteFile(filepath.Join(baseDir, f.Name), f.Data, 0644)).To(Succeed()) + } + + got, err := mergeFileValues(baseDir, tt.paths) + if tt.wantErr != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) + g.Expect(got).To(BeNil()) + return + } + + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(got).To(Equal(tt.want)) + }) + } +} + +func Test_copyFileToPath(t *testing.T) { + tests := []struct { + name string + in string + wantErr string + }{ + { + name: "copies input file", + in: "testdata/local-index.yaml", + }, + { + name: "invalid input file", + in: "testdata/invalid.tgz", + wantErr: "failed to open file to copy from", + }, + { + name: "invalid input directory", + in: "testdata/charts", + wantErr: "failed to read from source during copy", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + out := tmpFile("copy-0.1.0", ".tgz") + defer os.RemoveAll(out) + err := copyFileToPath(tt.in, out) + if tt.wantErr != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) + return + } + + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(out).To(BeARegularFile()) + f1, err := os.ReadFile(tt.in) + g.Expect(err).ToNot(HaveOccurred()) + f2, err := os.ReadFile(out) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(f2).To(Equal(f1)) + }) + } +} diff --git a/internal/helm/chart_builder_remote.go b/internal/helm/chart_builder_remote.go new file mode 100644 index 000000000..eaa77a044 --- /dev/null +++ b/internal/helm/chart_builder_remote.go @@ -0,0 +1,199 @@ +/* +Copyright 2021 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package helm + +import ( + "context" + "fmt" + "io" + "os" + "path/filepath" + + "github.com/Masterminds/semver/v3" + "github.com/fluxcd/pkg/runtime/transform" + "github.com/fluxcd/source-controller/internal/fs" + helmchart "helm.sh/helm/v3/pkg/chart" + "helm.sh/helm/v3/pkg/chart/loader" + "helm.sh/helm/v3/pkg/chartutil" + "sigs.k8s.io/yaml" +) + +type remoteChartBuilder struct { + remote *ChartRepository +} + +// NewRemoteChartBuilder returns a ChartBuilder capable of building a Helm +// chart with a RemoteChartReference from the given ChartRepository. +func NewRemoteChartBuilder(repository *ChartRepository) ChartBuilder { + return &remoteChartBuilder{ + remote: repository, + } +} + +func (b *remoteChartBuilder) Build(_ context.Context, ref ChartReference, p string, opts BuildOptions) (*ChartBuild, error) { + remoteRef, ok := ref.(RemoteChartReference) + if !ok { + return nil, fmt.Errorf("expected remote chart reference") + } + + if err := ref.Validate(); err != nil { + return nil, err + } + + if err := b.remote.LoadFromCache(); err != nil { + return nil, fmt.Errorf("could not load repository index for remote chart reference: %w", err) + } + defer b.remote.Unload() + + // Get the current version for the RemoteChartReference + cv, err := b.remote.Get(remoteRef.Name, remoteRef.Version) + if err != nil { + return nil, fmt.Errorf("failed to get chart version for remote reference: %w", err) + } + + result := &ChartBuild{} + result.Name = cv.Name + result.Version = cv.Version + // Set build specific metadata if instructed + if opts.VersionMetadata != "" { + ver, err := semver.NewVersion(result.Version) + if err != nil { + return nil, err + } + if *ver, err = ver.SetMetadata(opts.VersionMetadata); err != nil { + return nil, err + } + result.Version = ver.String() + } + + // If all the following is true, we do not need to download and/or build the chart: + // Chart version from metadata matches chart version for ref + // BuildOptions.Force is False + if opts.CachedChart != "" && !opts.Force { + if meta, err := LoadChartMetadataFromArchive(opts.CachedChart); err == nil && meta.Version == cv.Version { + result.Path = opts.CachedChart + result.ValueFiles = opts.GetValueFiles() + return result, nil + } + } + + // Download the package for the resolved version + res, err := b.remote.DownloadChart(cv) + if err != nil { + return nil, fmt.Errorf("failed to download chart for remote reference: %w", err) + } + + // Use literal chart copy from remote if no custom value files options are set + if len(opts.GetValueFiles()) == 0 { + if err = validatePackageAndWriteToPath(res, p); err != nil { + return nil, err + } + result.Path = p + return result, nil + } + + // Load the chart and merge chart values + var chart *helmchart.Chart + if chart, err = loader.LoadArchive(res); err != nil { + return nil, fmt.Errorf("failed to load downloaded chart: %w", err) + } + + mergedValues, err := mergeChartValues(chart, opts.ValueFiles) + if err != nil { + return nil, fmt.Errorf("failed to merge chart values: %w", err) + } + // Overwrite default values with merged values, if any + if ok, err = OverwriteChartDefaultValues(chart, mergedValues); ok || err != nil { + if err != nil { + return nil, err + } + result.ValueFiles = opts.GetValueFiles() + } + + // Package the chart with the custom values + if err = packageToPath(chart, p); err != nil { + return nil, err + } + result.Path = p + result.Packaged = true + return result, nil +} + +// mergeChartValues merges the given chart.Chart Files paths into a single "values.yaml" map. +// It returns the merge result, or an error. +func mergeChartValues(chart *helmchart.Chart, paths []string) (map[string]interface{}, error) { + mergedValues := make(map[string]interface{}) + for _, p := range paths { + cfn := filepath.Clean(p) + if cfn == chartutil.ValuesfileName { + mergedValues = transform.MergeMaps(mergedValues, chart.Values) + continue + } + var b []byte + for _, f := range chart.Files { + if f.Name == cfn { + b = f.Data + break + } + } + if b == nil { + return nil, fmt.Errorf("no values file found at path '%s'", p) + } + values := make(map[string]interface{}) + if err := yaml.Unmarshal(b, &values); err != nil { + return nil, fmt.Errorf("unmarshaling values from '%s' failed: %w", p, err) + } + mergedValues = transform.MergeMaps(mergedValues, values) + } + return mergedValues, nil +} + +// validatePackageAndWriteToPath atomically writes the packaged chart from reader +// to out while validating it by loading the chart metadata from the archive. +func validatePackageAndWriteToPath(reader io.Reader, out string) error { + tmpFile, err := os.CreateTemp("", filepath.Base(out)) + if err != nil { + return fmt.Errorf("failed to create temporary file for chart: %w", err) + } + defer os.Remove(tmpFile.Name()) + if _, err = tmpFile.ReadFrom(reader); err != nil { + _ = tmpFile.Close() + return fmt.Errorf("failed to write chart to file: %w", err) + } + if err = tmpFile.Close(); err != nil { + return err + } + if _, err = LoadChartMetadataFromArchive(tmpFile.Name()); err != nil { + return fmt.Errorf("failed to load chart metadata from written chart: %w", err) + } + if err = fs.RenameWithFallback(tmpFile.Name(), out); err != nil { + return fmt.Errorf("failed to write chart to file: %w", err) + } + return nil +} + +// pathIsDir returns a boolean indicating if the given path points to a directory. +// In case os.Stat on the given path returns an error it returns false as well. +func pathIsDir(p string) bool { + if p == "" { + return false + } + if i, err := os.Stat(p); err != nil || !i.IsDir() { + return false + } + return true +} diff --git a/internal/helm/chart_builder_remote_test.go b/internal/helm/chart_builder_remote_test.go new file mode 100644 index 000000000..260bcbce1 --- /dev/null +++ b/internal/helm/chart_builder_remote_test.go @@ -0,0 +1,118 @@ +/* +Copyright 2021 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package helm + +import ( + "testing" + + . "github.com/onsi/gomega" + helmchart "helm.sh/helm/v3/pkg/chart" + "helm.sh/helm/v3/pkg/chartutil" +) + +func Test_mergeChartValues(t *testing.T) { + tests := []struct { + name string + chart *helmchart.Chart + paths []string + want map[string]interface{} + wantErr string + }{ + { + name: "merges values", + chart: &helmchart.Chart{ + Files: []*helmchart.File{ + {Name: "a.yaml", Data: []byte("a: b")}, + {Name: "b.yaml", Data: []byte("b: c")}, + {Name: "c.yaml", Data: []byte("b: d")}, + }, + }, + paths: []string{"a.yaml", "b.yaml", "c.yaml"}, + want: map[string]interface{}{ + "a": "b", + "b": "d", + }, + }, + { + name: "uses chart values", + chart: &helmchart.Chart{ + Files: []*helmchart.File{ + {Name: "c.yaml", Data: []byte("b: d")}, + }, + Values: map[string]interface{}{ + "a": "b", + }, + }, + paths: []string{chartutil.ValuesfileName, "c.yaml"}, + want: map[string]interface{}{ + "a": "b", + "b": "d", + }, + }, + { + name: "unmarshal error", + chart: &helmchart.Chart{ + Files: []*helmchart.File{ + {Name: "invalid", Data: []byte("abcd")}, + }, + }, + paths: []string{"invalid"}, + wantErr: "unmarshaling values from 'invalid' failed", + }, + { + name: "error on invalid path", + chart: &helmchart.Chart{}, + paths: []string{"a.yaml"}, + wantErr: "no values file found at path 'a.yaml'", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + got, err := mergeChartValues(tt.chart, tt.paths) + if tt.wantErr != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) + g.Expect(got).To(BeNil()) + return + } + + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(got).To(Equal(tt.want)) + }) + } +} + +func Test_pathIsDir(t *testing.T) { + tests := []struct { + name string + p string + want bool + }{ + {name: "directory", p: "testdata/", want: true}, + {name: "file", p: "testdata/local-index.yaml", want: false}, + {name: "not found error", p: "testdata/does-not-exist.yaml", want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + g.Expect(pathIsDir(tt.p)).To(Equal(tt.want)) + }) + } +} diff --git a/internal/helm/chart_builder_test.go b/internal/helm/chart_builder_test.go index afc0107ce..a4252be8f 100644 --- a/internal/helm/chart_builder_test.go +++ b/internal/helm/chart_builder_test.go @@ -17,545 +17,27 @@ limitations under the License. package helm import ( - "context" "encoding/hex" - "fmt" "math/rand" "os" "path/filepath" - "sync" "testing" . "github.com/onsi/gomega" - 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" ) func TestChartBuildResult_String(t *testing.T) { g := NewWithT(t) - var result *ChartBuildResult + var result *ChartBuild g.Expect(result.String()).To(Equal("")) - result = &ChartBuildResult{} + result = &ChartBuild{} g.Expect(result.String()).To(Equal("")) - result = &ChartBuildResult{Path: "/foo/"} + result = &ChartBuild{Path: "/foo/"} g.Expect(result.String()).To(Equal("/foo/")) } -func TestChartBuilder_Build(t *testing.T) { - tests := []struct { - name string - baseDir string - path string - valueFiles []string - repositories map[string]*ChartRepository - getChartRepositoryCallback GetChartRepositoryCallback - wantErr string - }{ - { - name: "builds chart from directory", - path: "testdata/charts/helmchart", - }, - { - name: "builds chart from package", - path: "testdata/charts/helmchart-0.1.0.tgz", - }, - { - // TODO(hidde): add more diverse tests - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - b, err := NewChartBuilder(tt.path) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(b).ToNot(BeNil()) - - b.WithBaseDir(tt.baseDir) - b.WithValueFiles(tt.valueFiles...) - b.WithChartRepositoryCallback(b.getChartRepositoryCallback) - for k, v := range tt.repositories { - b.WithChartRepository(k, v) - } - - out := tmpFile("build-0.1.0", ".tgz") - defer os.RemoveAll(out) - got, err := b.Build(context.TODO(), out) - if tt.wantErr != "" { - g.Expect(err).To(HaveOccurred()) - g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) - g.Expect(got).To(BeNil()) - return - } - - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(got).ToNot(BeNil()) - - g.Expect(got.Path).ToNot(BeEmpty()) - g.Expect(got.Path).To(Equal(out)) - g.Expect(got.Path).To(BeARegularFile()) - _, err = loader.Load(got.Path) - g.Expect(err).ToNot(HaveOccurred()) - }) - } -} - -func TestChartBuilder_load(t *testing.T) { - tests := []struct { - name string - path string - chart *helmchart.Chart - wantFunc func(g *WithT, c *helmchart.Chart) - wantErr string - }{ - { - name: "loads chart", - chart: nil, - path: "testdata/charts/helmchart-0.1.0.tgz", - wantFunc: func(g *WithT, c *helmchart.Chart) { - g.Expect(c.Metadata.Name).To(Equal("helmchart")) - g.Expect(c.Files).ToNot(BeZero()) - }, - }, - { - name: "overwrites chart without any files (metadata shim)", - chart: &helmchart.Chart{ - Metadata: &helmchart.Metadata{Name: "dummy"}, - }, - path: "testdata/charts/helmchart-0.1.0.tgz", - wantFunc: func(g *WithT, c *helmchart.Chart) { - g.Expect(c.Metadata.Name).To(Equal("helmchart")) - g.Expect(c.Files).ToNot(BeZero()) - }, - }, - { - name: "does not overwrite loaded chart", - chart: &helmchart.Chart{ - Metadata: &helmchart.Metadata{Name: "dummy"}, - Files: []*helmchart.File{ - {Name: "mock.yaml", Data: []byte("loaded chart")}, - }, - }, - path: "testdata/charts/helmchart-0.1.0.tgz", - wantFunc: func(g *WithT, c *helmchart.Chart) { - g.Expect(c.Metadata.Name).To(Equal("dummy")) - g.Expect(c.Files).To(HaveLen(1)) - }, - }, - { - name: "no path", - wantErr: "failed to load chart: path not set", - }, - { - name: "invalid chart", - path: "testdata/charts/empty.tgz", - wantErr: "failed to load chart: no files in chart archive", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - b := &ChartBuilder{ - path: tt.path, - chart: tt.chart, - } - err := b.load() - if tt.wantErr != "" { - g.Expect(err).To(HaveOccurred()) - g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) - return - } - - g.Expect(err).ToNot(HaveOccurred()) - if tt.wantFunc != nil { - tt.wantFunc(g, b.chart) - } - }) - } -} - -func TestChartBuilder_buildDependencies(t *testing.T) { - g := NewWithT(t) - - chartB, err := os.ReadFile("testdata/charts/helmchart-0.1.0.tgz") - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(chartB).ToNot(BeEmpty()) - - mockRepo := func() *ChartRepository { - return &ChartRepository{ - Client: &mockGetter{ - response: chartB, - }, - Index: &repo.IndexFile{ - Entries: map[string]repo.ChartVersions{ - "grafana": { - &repo.ChartVersion{ - Metadata: &helmchart.Metadata{ - Name: "grafana", - Version: "6.17.4", - }, - URLs: []string{"https://example.com/chart.tgz"}, - }, - }, - }, - }, - RWMutex: &sync.RWMutex{}, - } - } - - var mockCallback GetChartRepositoryCallback = func(url string) (*ChartRepository, error) { - if url == "https://grafana.github.io/helm-charts/" { - return mockRepo(), nil - } - return nil, fmt.Errorf("no repository for URL") - } - - tests := []struct { - name string - baseDir string - path string - chart *helmchart.Chart - fromDir bool - repositories map[string]*ChartRepository - getChartRepositoryCallback GetChartRepositoryCallback - wantCollectedDependencies int - wantErr string - }{ - { - name: "builds dependencies using callback", - fromDir: true, - baseDir: "testdata/charts", - path: "testdata/charts/helmchartwithdeps", - getChartRepositoryCallback: mockCallback, - wantCollectedDependencies: 2, - }, - { - name: "builds dependencies using repositories", - fromDir: true, - baseDir: "testdata/charts", - path: "testdata/charts/helmchartwithdeps", - repositories: map[string]*ChartRepository{ - "https://grafana.github.io/helm-charts/": mockRepo(), - }, - wantCollectedDependencies: 2, - }, - { - name: "skips dependency build for packaged chart", - path: "testdata/charts/helmchart-0.1.0.tgz", - }, - { - name: "attempts to load chart", - fromDir: true, - path: "testdata", - wantErr: "failed to ensure chart has no missing dependencies", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - b := &ChartBuilder{ - baseDir: tt.baseDir, - path: tt.path, - chart: tt.chart, - repositories: tt.repositories, - getChartRepositoryCallback: tt.getChartRepositoryCallback, - } - - result := &ChartBuildResult{SourceIsDir: tt.fromDir} - err := b.buildDependencies(context.TODO(), result) - if tt.wantErr != "" { - g.Expect(err).To(HaveOccurred()) - g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) - g.Expect(result.CollectedDependencies).To(BeZero()) - g.Expect(b.chart).To(Equal(tt.chart)) - return - } - - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(result).ToNot(BeNil()) - g.Expect(result.CollectedDependencies).To(Equal(tt.wantCollectedDependencies)) - if tt.wantCollectedDependencies > 0 { - g.Expect(b.chart).ToNot(Equal(tt.chart)) - } - }) - } -} - -func TestChartBuilder_mergeValues(t *testing.T) { - tests := []struct { - name string - baseDir string - path string - isDir bool - chart *helmchart.Chart - valueFiles []string - want map[string]interface{} - wantErr string - }{ - { - name: "merges chart values", - chart: &helmchart.Chart{ - Files: []*helmchart.File{ - {Name: "a.yaml", Data: []byte("a: b")}, - {Name: "b.yaml", Data: []byte("a: c")}, - }, - }, - valueFiles: []string{"a.yaml", "b.yaml"}, - want: map[string]interface{}{ - "a": "c", - }, - }, - { - name: "chart values merge error", - chart: &helmchart.Chart{ - Files: []*helmchart.File{ - {Name: "b.yaml", Data: []byte("a: c")}, - }, - }, - valueFiles: []string{"a.yaml"}, - wantErr: "failed to merge chart values", - }, - { - name: "merges file values", - isDir: true, - baseDir: "testdata/charts", - path: "helmchart", - valueFiles: []string{"helmchart/values-prod.yaml"}, - want: map[string]interface{}{ - "replicaCount": float64(2), - }, - }, - { - name: "file values merge error", - isDir: true, - baseDir: "testdata/charts", - path: "helmchart", - valueFiles: []string{"invalid.yaml"}, - wantErr: "failed to merge value files", - }, - { - name: "error on chart load failure", - baseDir: "testdata/charts", - path: "invalid", - wantErr: "failed to load chart", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - b := &ChartBuilder{ - baseDir: tt.baseDir, - path: tt.path, - chart: tt.chart, - valueFiles: tt.valueFiles, - } - - result := &ChartBuildResult{SourceIsDir: tt.isDir} - err := b.mergeValues(result) - if tt.wantErr != "" { - g.Expect(err).To(HaveOccurred()) - g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) - g.Expect(result.ValuesOverwrite).To(BeNil()) - return - } - - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(result.ValuesOverwrite).To(Equal(tt.want)) - }) - } -} - -func Test_mergeChartValues(t *testing.T) { - tests := []struct { - name string - chart *helmchart.Chart - paths []string - want map[string]interface{} - wantErr string - }{ - { - name: "merges values", - chart: &helmchart.Chart{ - Files: []*helmchart.File{ - {Name: "a.yaml", Data: []byte("a: b")}, - {Name: "b.yaml", Data: []byte("b: c")}, - {Name: "c.yaml", Data: []byte("b: d")}, - }, - }, - paths: []string{"a.yaml", "b.yaml", "c.yaml"}, - want: map[string]interface{}{ - "a": "b", - "b": "d", - }, - }, - { - name: "uses chart values", - chart: &helmchart.Chart{ - Files: []*helmchart.File{ - {Name: "c.yaml", Data: []byte("b: d")}, - }, - Values: map[string]interface{}{ - "a": "b", - }, - }, - paths: []string{chartutil.ValuesfileName, "c.yaml"}, - want: map[string]interface{}{ - "a": "b", - "b": "d", - }, - }, - { - name: "unmarshal error", - chart: &helmchart.Chart{ - Files: []*helmchart.File{ - {Name: "invalid", Data: []byte("abcd")}, - }, - }, - paths: []string{"invalid"}, - wantErr: "unmarshaling values from 'invalid' failed", - }, - { - name: "error on invalid path", - chart: &helmchart.Chart{}, - paths: []string{"a.yaml"}, - wantErr: "no values file found at path 'a.yaml'", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - got, err := mergeChartValues(tt.chart, tt.paths) - if tt.wantErr != "" { - g.Expect(err).To(HaveOccurred()) - g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) - g.Expect(got).To(BeNil()) - return - } - - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(got).To(Equal(tt.want)) - }) - } -} - -func Test_mergeFileValues(t *testing.T) { - tests := []struct { - name string - files []*helmchart.File - paths []string - want map[string]interface{} - wantErr string - }{ - { - name: "merges values from files", - files: []*helmchart.File{ - {Name: "a.yaml", Data: []byte("a: b")}, - {Name: "b.yaml", Data: []byte("b: c")}, - {Name: "c.yaml", Data: []byte("b: d")}, - }, - paths: []string{"a.yaml", "b.yaml", "c.yaml"}, - want: map[string]interface{}{ - "a": "b", - "b": "d", - }, - }, - { - name: "illegal traverse", - paths: []string{"../../../traversing/illegally/a/p/a/b"}, - wantErr: "no values file found at path '/traversing/illegally/a/p/a/b'", - }, - { - name: "unmarshal error", - files: []*helmchart.File{ - {Name: "invalid", Data: []byte("abcd")}, - }, - paths: []string{"invalid"}, - wantErr: "unmarshaling values from 'invalid' failed", - }, - { - name: "error on invalid path", - paths: []string{"a.yaml"}, - wantErr: "no values file found at path '/a.yaml'", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - baseDir, err := os.MkdirTemp("", "merge-file-values-*") - g.Expect(err).ToNot(HaveOccurred()) - defer os.RemoveAll(baseDir) - - for _, f := range tt.files { - g.Expect(os.WriteFile(filepath.Join(baseDir, f.Name), f.Data, 0644)).To(Succeed()) - } - - got, err := mergeFileValues(baseDir, tt.paths) - if tt.wantErr != "" { - g.Expect(err).To(HaveOccurred()) - g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) - g.Expect(got).To(BeNil()) - return - } - - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(got).To(Equal(tt.want)) - }) - } -} - -func Test_copyFileToPath(t *testing.T) { - tests := []struct { - name string - in string - wantErr string - }{ - { - name: "copies input file", - in: "testdata/local-index.yaml", - }, - { - name: "invalid input file", - in: "testdata/invalid.tgz", - wantErr: "failed to open file to copy from", - }, - { - name: "invalid input directory", - in: "testdata/charts", - wantErr: "failed to read from source during copy", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - out := tmpFile("copy-0.1.0", ".tgz") - defer os.RemoveAll(out) - err := copyFileToPath(tt.in, out) - if tt.wantErr != "" { - g.Expect(err).To(HaveOccurred()) - g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) - return - } - - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(out).To(BeARegularFile()) - f1, err := os.ReadFile(tt.in) - g.Expect(err).ToNot(HaveOccurred()) - f2, err := os.ReadFile(out) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(f2).To(Equal(f1)) - }) - } -} - func Test_packageToPath(t *testing.T) { g := NewWithT(t) @@ -572,25 +54,6 @@ func Test_packageToPath(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) } -func Test_pathIsDir(t *testing.T) { - tests := []struct { - name string - p string - want bool - }{ - {name: "directory", p: "testdata/", want: true}, - {name: "file", p: "testdata/local-index.yaml", want: false}, - {name: "not found error", p: "testdata/does-not-exist.yaml", want: false}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - g.Expect(pathIsDir(tt.p)).To(Equal(tt.want)) - }) - } -} - func tmpFile(prefix, suffix string) string { randBytes := make([]byte, 16) rand.Read(randBytes) diff --git a/internal/helm/chart_test.go b/internal/helm/chart_test.go index 23d50b96b..ac7114e87 100644 --- a/internal/helm/chart_test.go +++ b/internal/helm/chart_test.go @@ -25,8 +25,9 @@ import ( ) var ( - originalValuesFixture = []byte("override: original") - chartFilesFixture = []*helmchart.File{ + originalValuesFixture = []byte(`override: original +`) + chartFilesFixture = []*helmchart.File{ { Name: "values.yaml", Data: originalValuesFixture, @@ -69,19 +70,14 @@ func TestOverwriteChartDefaultValues(t *testing.T) { desc: "valid override", chart: chartFixture, ok: true, - data: []byte("override: test"), + data: []byte(`override: test +`), }, { desc: "empty override", chart: chartFixture, ok: true, - data: []byte(""), - }, - { - desc: "invalid", - chart: chartFixture, - data: []byte("!fail:"), - expectErr: true, + data: []byte(``), }, } for _, tt := range testCases { @@ -89,7 +85,9 @@ func TestOverwriteChartDefaultValues(t *testing.T) { g := NewWithT(t) fixture := tt.chart - ok, err := OverwriteChartDefaultValues(&fixture, tt.data) + vals, err := chartutil.ReadValues(tt.data) + g.Expect(err).ToNot(HaveOccurred()) + ok, err := OverwriteChartDefaultValues(&fixture, vals) g.Expect(ok).To(Equal(tt.ok)) if tt.expectErr { diff --git a/internal/helm/dependency_manager.go b/internal/helm/dependency_manager.go index 043b0e7e3..0331a46b4 100644 --- a/internal/helm/dependency_manager.go +++ b/internal/helm/dependency_manager.go @@ -37,22 +37,8 @@ import ( // or an error describing why it could not be returned. type GetChartRepositoryCallback func(url string) (*ChartRepository, error) -// DependencyManager manages dependencies for a Helm chart, downloading -// only those that are missing from the chart it holds. +// DependencyManager manages dependencies for a Helm chart. type DependencyManager struct { - // chart contains the chart.Chart from the path. - chart *helmchart.Chart - - // baseDir is the chroot path for dependency manager operations, - // Dependencies that hold a local (relative) path reference are not - // allowed to traverse outside this directory. - baseDir string - - // path is the path of the chart relative to the baseDir, - // the combination of the baseDir and path is used to - // determine the absolute path of a local dependency. - path string - // repositories contains a map of ChartRepository indexed by their // normalized URL. It is used as a lookup table for missing // dependencies. @@ -62,20 +48,22 @@ type DependencyManager struct { // callback which returned result is cached to repositories. getChartRepositoryCallback GetChartRepositoryCallback - // workers is the number of concurrent chart-add operations during + // concurrent is the number of concurrent chart-add operations during // Build. Defaults to 1 (non-concurrent). - workers int64 + concurrent int64 // mu contains the lock for chart writes. mu sync.Mutex } -func NewDependencyManager(chart *helmchart.Chart, baseDir, path string) *DependencyManager { - return &DependencyManager{ - chart: chart, - baseDir: baseDir, - path: path, - } +// chartWithLock holds a chart.Chart with a sync.Mutex to lock for writes. +type chartWithLock struct { + *helmchart.Chart + mu sync.Mutex +} + +func NewDependencyManager() *DependencyManager { + return &DependencyManager{} } func (dm *DependencyManager) WithRepositories(r map[string]*ChartRepository) *DependencyManager { @@ -89,20 +77,30 @@ func (dm *DependencyManager) WithChartRepositoryCallback(c GetChartRepositoryCal } func (dm *DependencyManager) WithWorkers(w int64) *DependencyManager { - dm.workers = w + dm.concurrent = w return dm } -// Build compiles and builds the dependencies of the chart with the -// configured number of workers. -func (dm *DependencyManager) Build(ctx context.Context) (int, error) { +func (dm *DependencyManager) Clear() []error { + var errs []error + for _, v := range dm.repositories { + v.Unload() + errs = append(errs, v.RemoveCache()) + } + return errs +} + +// Build compiles a set of missing dependencies from chart.Chart, and attempts to +// resolve and build them using the information from ChartReference. +// It returns the number of resolved local and remote dependencies, or an error. +func (dm *DependencyManager) Build(ctx context.Context, ref ChartReference, chart *helmchart.Chart) (int, error) { // Collect dependency metadata var ( - deps = dm.chart.Dependencies() - reqs = dm.chart.Metadata.Dependencies + deps = chart.Dependencies() + reqs = chart.Metadata.Dependencies ) // Lock file takes precedence - if lock := dm.chart.Lock; lock != nil { + if lock := chart.Lock; lock != nil { reqs = lock.Dependencies } @@ -113,31 +111,26 @@ func (dm *DependencyManager) Build(ctx context.Context) (int, error) { } // Run the build for the missing dependencies - if err := dm.build(ctx, missing); err != nil { + if err := dm.build(ctx, ref, chart, missing); err != nil { return 0, err } return len(missing), nil } -// build (concurrently) adds the given list of deps to the chart with the configured -// number of workers. It returns the first error, cancelling all other workers. -func (dm *DependencyManager) build(ctx context.Context, deps map[string]*helmchart.Dependency) error { - workers := dm.workers - if workers <= 0 { - workers = 1 +// build adds the given list of deps to the chart with the configured number of +// concurrent workers. If the chart.Chart references a local dependency but no +// LocalChartReference is given, or any dependency could not be added, an error +// is returned. The first error it encounters cancels all other workers. +func (dm *DependencyManager) build(ctx context.Context, ref ChartReference, chart *helmchart.Chart, deps map[string]*helmchart.Dependency) error { + current := dm.concurrent + if current <= 0 { + current = 1 } - // Garbage collect temporary cached ChartRepository indexes - defer func() { - for _, v := range dm.repositories { - v.Unload() - _ = v.RemoveCache() - } - }() - group, groupCtx := errgroup.WithContext(ctx) group.Go(func() error { - sem := semaphore.NewWeighted(workers) + sem := semaphore.NewWeighted(current) + chart := &chartWithLock{Chart: chart} for name, dep := range deps { name, dep := name, dep if err := sem.Acquire(groupCtx, 1); err != nil { @@ -146,12 +139,17 @@ func (dm *DependencyManager) build(ctx context.Context, deps map[string]*helmcha group.Go(func() (err error) { defer sem.Release(1) if isLocalDep(dep) { - if err = dm.addLocalDependency(dep); err != nil { + localRef, ok := ref.(LocalChartReference) + if !ok { + err = fmt.Errorf("failed to add local dependency '%s': no local chart reference", name) + return + } + if err = dm.addLocalDependency(localRef, chart, dep); err != nil { err = fmt.Errorf("failed to add local dependency '%s': %w", name, err) } return } - if err = dm.addRemoteDependency(dep); err != nil { + if err = dm.addRemoteDependency(chart, dep); err != nil { err = fmt.Errorf("failed to add remote dependency '%s': %w", name, err) } return @@ -162,17 +160,17 @@ func (dm *DependencyManager) build(ctx context.Context, deps map[string]*helmcha return group.Wait() } -// addLocalDependency attempts to resolve and add the given local chart.Dependency to the chart. -func (dm *DependencyManager) addLocalDependency(dep *helmchart.Dependency) error { - sLocalChartPath, err := dm.secureLocalChartPath(dep) +// addLocalDependency attempts to resolve and add the given local chart.Dependency +// to the chart. +func (dm *DependencyManager) addLocalDependency(ref LocalChartReference, chart *chartWithLock, dep *helmchart.Dependency) error { + sLocalChartPath, err := dm.secureLocalChartPath(ref, dep) if err != nil { return err } if _, err := os.Stat(sLocalChartPath); err != nil { if os.IsNotExist(err) { - return fmt.Errorf("no chart found at '%s' (reference '%s')", - strings.TrimPrefix(sLocalChartPath, dm.baseDir), dep.Repository) + return fmt.Errorf("no chart found at '%s' (reference '%s')", sLocalChartPath, dep.Repository) } return err } @@ -186,7 +184,7 @@ func (dm *DependencyManager) addLocalDependency(dep *helmchart.Dependency) error ch, err := loader.Load(sLocalChartPath) if err != nil { return fmt.Errorf("failed to load chart from '%s' (reference '%s'): %w", - strings.TrimPrefix(sLocalChartPath, dm.baseDir), dep.Repository, err) + strings.TrimPrefix(sLocalChartPath, ref.BaseDir), dep.Repository, err) } ver, err := semver.NewVersion(ch.Metadata.Version) @@ -199,14 +197,16 @@ func (dm *DependencyManager) addLocalDependency(dep *helmchart.Dependency) error return err } - dm.mu.Lock() - dm.chart.AddDependency(ch) - dm.mu.Unlock() + chart.mu.Lock() + chart.AddDependency(ch) + chart.mu.Unlock() return nil } -// addRemoteDependency attempts to resolve and add the given remote chart.Dependency to the chart. -func (dm *DependencyManager) addRemoteDependency(dep *helmchart.Dependency) error { +// addRemoteDependency attempts to resolve and add the given remote chart.Dependency +// to the chart. It locks the chartWithLock before the downloaded dependency is +// added to the chart. +func (dm *DependencyManager) addRemoteDependency(chart *chartWithLock, dep *helmchart.Dependency) error { repo, err := dm.resolveRepository(dep.Repository) if err != nil { return err @@ -216,7 +216,6 @@ func (dm *DependencyManager) addRemoteDependency(dep *helmchart.Dependency) erro return fmt.Errorf("failed to load index for '%s': %w", dep.Name, err) } - ver, err := repo.Get(dep.Name, dep.Version) if err != nil { return err @@ -230,9 +229,9 @@ func (dm *DependencyManager) addRemoteDependency(dep *helmchart.Dependency) erro return fmt.Errorf("failed to load downloaded archive of version '%s': %w", ver.Version, err) } - dm.mu.Lock() - dm.chart.AddDependency(ch) - dm.mu.Unlock() + chart.mu.Lock() + chart.AddDependency(ch) + chart.mu.Unlock() return nil } @@ -260,8 +259,9 @@ func (dm *DependencyManager) resolveRepository(url string) (_ *ChartRepository, } // secureLocalChartPath returns the secure absolute path of a local dependency. -// It does not allow the dependency's path to be outside the scope of baseDir. -func (dm *DependencyManager) secureLocalChartPath(dep *helmchart.Dependency) (string, error) { +// It does not allow the dependency's path to be outside the scope of +// LocalChartReference.BaseDir. +func (dm *DependencyManager) secureLocalChartPath(ref LocalChartReference, dep *helmchart.Dependency) (string, error) { localUrl, err := url.Parse(dep.Repository) if err != nil { return "", fmt.Errorf("failed to parse alleged local chart reference: %w", err) @@ -269,7 +269,11 @@ func (dm *DependencyManager) secureLocalChartPath(dep *helmchart.Dependency) (st if localUrl.Scheme != "" && localUrl.Scheme != "file" { return "", fmt.Errorf("'%s' is not a local chart reference", dep.Repository) } - return securejoin.SecureJoin(dm.baseDir, filepath.Join(dm.path, localUrl.Host, localUrl.Path)) + relPath, err := filepath.Rel(ref.BaseDir, ref.Path) + if err != nil { + return "", err + } + return securejoin.SecureJoin(ref.BaseDir, filepath.Join(relPath, localUrl.Host, localUrl.Path)) } // collectMissing returns a map with reqs that are missing from current, diff --git a/internal/helm/dependency_manager_test.go b/internal/helm/dependency_manager_test.go index e51e6b768..255b1685d 100644 --- a/internal/helm/dependency_manager_test.go +++ b/internal/helm/dependency_manager_test.go @@ -88,10 +88,10 @@ func TestDependencyManager_Build(t *testing.T) { chart, err := loader.Load(filepath.Join(tt.baseDir, tt.path)) g.Expect(err).ToNot(HaveOccurred()) - got, err := NewDependencyManager(chart, tt.baseDir, tt.path). + got, err := NewDependencyManager(). WithRepositories(tt.repositories). WithChartRepositoryCallback(tt.getChartRepositoryCallback). - Build(context.TODO()) + Build(context.TODO(), LocalChartReference{BaseDir: tt.baseDir, Path: tt.path}, chart) if tt.wantErr != "" { g.Expect(err).To(HaveOccurred()) @@ -134,10 +134,8 @@ func TestDependencyManager_build(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - dm := &DependencyManager{ - baseDir: "testdata/charts", - } - err := dm.build(context.TODO(), tt.deps) + dm := NewDependencyManager() + err := dm.build(context.TODO(), LocalChartReference{}, &helmchart.Chart{}, tt.deps) if tt.wantErr != "" { g.Expect(err).To(HaveOccurred()) return @@ -182,7 +180,7 @@ func TestDependencyManager_addLocalDependency(t *testing.T) { Version: chartVersion, Repository: "file://../../../absolutely/invalid", }, - wantErr: "no chart found at 'absolutely/invalid'", + wantErr: "no chart found at 'testdata/charts/absolutely/invalid'", }, { name: "invalid chart archive", @@ -191,7 +189,7 @@ func TestDependencyManager_addLocalDependency(t *testing.T) { Version: chartVersion, Repository: "file://../empty.tgz", }, - wantErr: "failed to load chart from 'empty.tgz'", + wantErr: "failed to load chart from '/empty.tgz'", }, { name: "invalid constraint", @@ -207,13 +205,10 @@ func TestDependencyManager_addLocalDependency(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - dm := &DependencyManager{ - chart: &helmchart.Chart{}, - baseDir: "testdata/charts/", - path: "helmchartwithdeps", - } - - err := dm.addLocalDependency(tt.dep) + dm := NewDependencyManager() + chart := &helmchart.Chart{} + err := dm.addLocalDependency(LocalChartReference{BaseDir: "testdata/charts", Path: "helmchartwithdeps"}, + &chartWithLock{Chart: chart}, tt.dep) if tt.wantErr != "" { g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) @@ -389,10 +384,10 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { g := NewWithT(t) dm := &DependencyManager{ - chart: &helmchart.Chart{}, repositories: tt.repositories, } - err := dm.addRemoteDependency(tt.dep) + chart := &helmchart.Chart{} + err := dm.addRemoteDependency(&chartWithLock{Chart: chart}, tt.dep) if tt.wantErr != "" { g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) @@ -400,7 +395,7 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { } g.Expect(err).ToNot(HaveOccurred()) if tt.wantFunc != nil { - tt.wantFunc(g, dm.chart) + tt.wantFunc(g, chart) } }) } @@ -522,11 +517,8 @@ func TestDependencyManager_secureLocalChartPath(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - dm := &DependencyManager{ - baseDir: tt.baseDir, - path: tt.path, - } - got, err := dm.secureLocalChartPath(tt.dep) + dm := NewDependencyManager() + got, err := dm.secureLocalChartPath(LocalChartReference{BaseDir: tt.baseDir, Path: tt.path}, tt.dep) if tt.wantErr != "" { g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) diff --git a/internal/helm/getter.go b/internal/helm/getter.go index b0f07e96b..1ca8b0e9b 100644 --- a/internal/helm/getter.go +++ b/internal/helm/getter.go @@ -19,31 +19,30 @@ package helm import ( "fmt" "os" - "path/filepath" "helm.sh/helm/v3/pkg/getter" corev1 "k8s.io/api/core/v1" ) // ClientOptionsFromSecret constructs a getter.Option slice for the given secret. -// It returns the slice, and a callback to remove temporary files. -func ClientOptionsFromSecret(secret corev1.Secret) ([]getter.Option, func(), error) { +// It returns the slice, or an error. +func ClientOptionsFromSecret(dir string, secret corev1.Secret) ([]getter.Option, error) { var opts []getter.Option basicAuth, err := BasicAuthFromSecret(secret) if err != nil { - return opts, nil, err + return opts, err } if basicAuth != nil { opts = append(opts, basicAuth) } - tlsClientConfig, cleanup, err := TLSClientConfigFromSecret(secret) + tlsClientConfig, err := TLSClientConfigFromSecret(dir, secret) if err != nil { - return opts, nil, err + return opts, err } if tlsClientConfig != nil { opts = append(opts, tlsClientConfig) } - return opts, cleanup, nil + return opts, nil } // BasicAuthFromSecret attempts to construct a basic auth getter.Option for the @@ -63,50 +62,65 @@ func BasicAuthFromSecret(secret corev1.Secret) (getter.Option, error) { } // TLSClientConfigFromSecret attempts to construct a TLS client config -// getter.Option for the given v1.Secret. It returns the getter.Option and a -// callback to remove the temporary TLS files. +// getter.Option for the given v1.Secret, placing the required TLS config +// related files in the given directory. It returns the getter.Option, or +// an error. // // Secrets with no certFile, keyFile, AND caFile are ignored, if only a // certBytes OR keyBytes is defined it returns an error. -func TLSClientConfigFromSecret(secret corev1.Secret) (getter.Option, func(), error) { +func TLSClientConfigFromSecret(dir string, secret corev1.Secret) (getter.Option, error) { certBytes, keyBytes, caBytes := secret.Data["certFile"], secret.Data["keyFile"], secret.Data["caFile"] switch { case len(certBytes)+len(keyBytes)+len(caBytes) == 0: - return nil, func() {}, nil + return nil, nil case (len(certBytes) > 0 && len(keyBytes) == 0) || (len(keyBytes) > 0 && len(certBytes) == 0): - return nil, nil, fmt.Errorf("invalid '%s' secret data: fields 'certFile' and 'keyFile' require each other's presence", + return nil, fmt.Errorf("invalid '%s' secret data: fields 'certFile' and 'keyFile' require each other's presence", secret.Name) } - // create tmp dir for TLS files - tmp, err := os.MkdirTemp("", "helm-tls-"+secret.Name) - if err != nil { - return nil, nil, err - } - cleanup := func() { os.RemoveAll(tmp) } - - var certFile, keyFile, caFile string - + var certPath, keyPath, caPath string if len(certBytes) > 0 && len(keyBytes) > 0 { - certFile = filepath.Join(tmp, "cert.crt") - if err := os.WriteFile(certFile, certBytes, 0644); err != nil { - cleanup() - return nil, nil, err + certFile, err := os.CreateTemp(dir, "cert-*.crt") + if err != nil { + return nil, err + } + if _, err = certFile.Write(certBytes); err != nil { + _ = certFile.Close() + return nil, err } - keyFile = filepath.Join(tmp, "key.crt") - if err := os.WriteFile(keyFile, keyBytes, 0644); err != nil { - cleanup() - return nil, nil, err + if err = certFile.Close(); err != nil { + return nil, err } + certPath = certFile.Name() + + keyFile, err := os.CreateTemp(dir, "key-*.crt") + if err != nil { + return nil, err + } + if _, err = keyFile.Write(keyBytes); err != nil { + _ = keyFile.Close() + return nil, err + } + if err = keyFile.Close(); err != nil { + return nil, err + } + keyPath = keyFile.Name() } if len(caBytes) > 0 { - caFile = filepath.Join(tmp, "ca.pem") - if err := os.WriteFile(caFile, caBytes, 0644); err != nil { - cleanup() - return nil, nil, err + caFile, err := os.CreateTemp(dir, "ca-*.pem") + if err != nil { + return nil, err + } + if _, err = caFile.Write(caBytes); err != nil { + _ = caFile.Close() + return nil, err + } + if err = caFile.Close(); err != nil { + return nil, err } + caPath = caFile.Name() } - return getter.WithTLSClientConfig(certFile, keyFile, caFile), cleanup, nil + return getter.WithTLSClientConfig(certPath, keyPath, caPath), nil } diff --git a/internal/helm/getter_test.go b/internal/helm/getter_test.go index bd4e1058c..2c55e7cbb 100644 --- a/internal/helm/getter_test.go +++ b/internal/helm/getter_test.go @@ -17,6 +17,7 @@ limitations under the License. package helm import ( + "os" "testing" corev1 "k8s.io/api/core/v1" @@ -56,10 +57,14 @@ func TestClientOptionsFromSecret(t *testing.T) { secret.Data[k] = v } } - got, cleanup, err := ClientOptionsFromSecret(secret) - if cleanup != nil { - defer cleanup() + + tmpDir, err := os.MkdirTemp("", "client-opts-secret-") + if err != nil { + t.Fatal(err) } + defer os.RemoveAll(tmpDir) + + got, err := ClientOptionsFromSecret(tmpDir, secret) if err != nil { t.Errorf("ClientOptionsFromSecret() error = %v", err) return @@ -123,10 +128,14 @@ func TestTLSClientConfigFromSecret(t *testing.T) { if tt.modify != nil { tt.modify(secret) } - got, cleanup, err := TLSClientConfigFromSecret(*secret) - if cleanup != nil { - defer cleanup() + + tmpDir, err := os.MkdirTemp("", "client-opts-secret-") + if err != nil { + t.Fatal(err) } + defer os.RemoveAll(tmpDir) + + got, err := TLSClientConfigFromSecret(tmpDir, *secret) if (err != nil) != tt.wantErr { t.Errorf("TLSClientConfigFromSecret() error = %v, wantErr %v", err, tt.wantErr) return diff --git a/internal/helm/helm.go b/internal/helm/helm.go new file mode 100644 index 000000000..4bb65f842 --- /dev/null +++ b/internal/helm/helm.go @@ -0,0 +1,29 @@ +/* +Copyright 2021 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package helm + +// This list defines a set of global variables used to ensure the upper bound +// of Helm files loaded into memory during runtime does not exceed limits. +var ( + // MaxIndexSize is the max allowed file size in bytes of a ChartRepository. + MaxIndexSize int64 = 50 << 20 + // MaxChartSize is the max allowed file size in bytes of a Helm Chart. + MaxChartSize int64 = 2 << 20 + // MaxChartFileSize is the max allowed file size in bytes of any arbitrary + // file originating from a chart. + MaxChartFileSize int64 = 2 << 10 +) diff --git a/internal/helm/repository.go b/internal/helm/repository.go index e2446f944..eb9e668a1 100644 --- a/internal/helm/repository.go +++ b/internal/helm/repository.go @@ -234,6 +234,16 @@ func (r *ChartRepository) LoadIndexFromBytes(b []byte) error { // LoadFromFile reads the file at the given path and loads it into Index. func (r *ChartRepository) LoadFromFile(path string) error { + stat, err := os.Stat(path) + if err != nil || stat.IsDir() { + if err == nil { + err = fmt.Errorf("'%s' is a directory", path) + } + return err + } + if stat.Size() > MaxIndexSize { + return fmt.Errorf("size of index '%s' exceeds '%d' limit", stat.Name(), MaxIndexSize) + } b, err := os.ReadFile(path) if err != nil { return err @@ -342,7 +352,7 @@ func (r *ChartRepository) HasCacheFile() bool { // Unload can be used to signal the Go garbage collector the Index can // be freed from memory if the ChartRepository object is expected to // continue to exist in the stack for some time. -func (r *ChartRepository) Unload() { +func (r *ChartRepository) Unload() { if r == nil { return } diff --git a/internal/helm/repository_test.go b/internal/helm/repository_test.go index 0d2077dfd..9c124b791 100644 --- a/internal/helm/repository_test.go +++ b/internal/helm/repository_test.go @@ -416,7 +416,7 @@ func TestChartRepository_LoadFromCache(t *testing.T) { { name: "invalid cache path", cachePath: "invalid", - wantErr: "open invalid: no such file", + wantErr: "stat invalid: no such file", }, { name: "no cache path",