Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use upstream's MergeValues with assets #3010

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 2 additions & 11 deletions provider/pkg/helm/tool.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (
"helm.sh/helm/v3/pkg/chart/loader"
"helm.sh/helm/v3/pkg/chartutil"
"helm.sh/helm/v3/pkg/cli"
"helm.sh/helm/v3/pkg/cli/values"
"helm.sh/helm/v3/pkg/downloader"
"helm.sh/helm/v3/pkg/getter"
"helm.sh/helm/v3/pkg/registry"
Expand Down Expand Up @@ -98,7 +99,7 @@ type TemplateOrInstallCommand struct {
Chart string

// Values to be applied to the chart.
Values ValueOpts
Values values.Options

tool *Tool
actionConfig *action.Configuration
Expand Down Expand Up @@ -130,17 +131,9 @@ func (cmd *TemplateOrInstallCommand) addInstallFlags() {
client.SubNotes = false
client.Labels = nil
client.EnableDNS = false
cmd.addValueOptionsFlags()
cmd.addChartPathOptionsFlags()
}

func (cmd *TemplateOrInstallCommand) addValueOptionsFlags() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still keep addValueOptionsFlags to set defaults on Values and to further resemble:
https://github.com/helm/helm/blob/14d0c13e9eefff5b4a1b511cf50643529692ec94/cmd/helm/flags.go#L45-L51

// https://github.com/helm/helm/blob/14d0c13e9eefff5b4a1b511cf50643529692ec94/cmd/helm/flags.go#L45-L51
v := cmd.Values
v.Values = map[string]any{}
v.ValuesFiles = []pulumi.Asset{}
}

func (cmd *TemplateOrInstallCommand) addChartPathOptionsFlags() {
// https://github.com/helm/helm/blob/14d0c13e9eefff5b4a1b511cf50643529692ec94/cmd/helm/flags.go#L54-L66
c := &cmd.Install.ChartPathOptions
Expand Down Expand Up @@ -180,7 +173,6 @@ func (t *Tool) Template() *TemplateCommand {
tool: t,
actionConfig: actionConfig,
Install: action.NewInstall(actionConfig),
Values: ValueOpts{},
},
}

Expand Down Expand Up @@ -472,7 +464,6 @@ type cleanupF func() error

// downloadAsset downloads an asset to the local filesystem.
func downloadAsset(p getter.Providers, asset pulumi.AssetOrArchive) (string, cleanupF, error) {

a, isAsset := asset.(pulumi.Asset)
if !isAsset {
return "", nil, errors.New("expected an asset")
Expand Down
15 changes: 10 additions & 5 deletions provider/pkg/provider/helm/v4/chart.go
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,12 @@ func (r *ChartProvider) Construct(ctx *pulumi.Context, typ, name string, inputs
cmd.Keyring = keyring
}

cmd.Values.Values = chartArgs.Values
cmd.Values.ValuesFiles = chartArgs.ValuesFiles
valueOpts, cleanup, err := readValues(p, chartArgs.Values, chartArgs.ValuesFiles)
defer cleanup()
if err != nil {
return nil, err
}
cmd.Values = valueOpts
cmd.IncludeCRDs = !chartArgs.SkipCrds
cmd.DisableHooks = true
cmd.ReleaseName = chartArgs.Name
Expand Down Expand Up @@ -247,7 +251,8 @@ func (r *ChartProvider) Construct(ctx *pulumi.Context, typ, name string, inputs
SkipAwait: chartArgs.SkipAwait,
ResourceOptions: []pulumi.ResourceOption{pulumi.Parent(comp)},
PreRegisterF: func(ctx *pulumi.Context, apiVersion, kind, resourceName string, obj *unstructured.Unstructured,
resourceOpts []pulumi.ResourceOption) (*unstructured.Unstructured, []pulumi.ResourceOption) {
resourceOpts []pulumi.ResourceOption,
) (*unstructured.Unstructured, []pulumi.ResourceOption) {
return preregister(ctx, comp, obj, resourceOpts)
},
}
Expand All @@ -261,8 +266,8 @@ func (r *ChartProvider) Construct(ctx *pulumi.Context, typ, name string, inputs
}

func preregister(ctx *pulumi.Context, comp *ChartState, obj *unstructured.Unstructured,
resourceOpts []pulumi.ResourceOption) (*unstructured.Unstructured, []pulumi.ResourceOption) {

resourceOpts []pulumi.ResourceOption,
) (*unstructured.Unstructured, []pulumi.ResourceOption) {
// Implement support for Helm resource policies.
// https://helm.sh/docs/howto/charts_tips_and_tricks/#tell-helm-not-to-uninstall-a-resource
policy, hasPolicy, err := unstructured.NestedString(obj.Object, "metadata", "annotations", helmkube.ResourcePolicyAnno)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2016-2022, Pulumi Corporation.
// Copyright 2024, Pulumi Corporation.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand All @@ -12,55 +12,75 @@
// See the License for the specific language governing permissions and
// limitations under the License.

package helm
package v4

import (
"fmt"
"net/url"
"os"
"path/filepath"

"github.com/pkg/errors"
"github.com/pulumi/pulumi/sdk/v3/go/pulumi"
"gopkg.in/yaml.v3"
"helm.sh/helm/v3/pkg/cli/values"
"helm.sh/helm/v3/pkg/getter"
"sigs.k8s.io/yaml"
)

// ValueOpts handles merging of chart values from various sources.
type ValueOpts struct {
// ValuesFiles is a list of Helm values files encapsulated as Pulumi assets.
ValuesFiles []pulumi.Asset
// Values is a map of Pulumi values.
Values map[string]any
}

// MergeValues merges the values in Helm's priority order.
func (opts *ValueOpts) MergeValues(p getter.Providers) (map[string]interface{}, error) {
base := map[string]interface{}{}
// readValues hydrates Assets and persists values on-disk in order to provide
// them to upstream's MergeValues logic.
//
// The returned function cleans up the on-disk values and should always be
// called, even on error.
func readValues(p getter.Providers, v map[string]any, files []pulumi.Asset) (values.Options, func(), error) {
opts := values.Options{}
tmp, err := os.MkdirTemp(os.TempDir(), "pulumi-kubernetes")
if err != nil {
return opts, func() {}, err
}
cleanup := func() {
_ = os.RemoveAll(tmp)
}

// User specified a values files via -f/--values
for _, asset := range opts.ValuesFiles {
currentMap := map[string]interface{}{}
valuesFiles := make([]string, 0, len(files)+1)

bytes, err := readAsset(p, asset)
persist := func(out []byte) error {
fname := filepath.Join(tmp, fmt.Sprintf("values-%d.yaml", len(valuesFiles)))
err := os.WriteFile(fname, out, 0o600)
if err != nil {
return nil, err
return err
}
valuesFiles = append(valuesFiles, fname)
return nil
}

if err := yaml.Unmarshal(bytes, &currentMap); err != nil {
return nil, err
for _, f := range files {
out, err := readAsset(p, f)
if err != nil {
return opts, cleanup, err
}
err = persist(out)
if err != nil {
return opts, cleanup, err
}
// Merge with the previous map
base = MergeMaps(base, currentMap)
}

// User specified a literal value map (possibly containing assets)
values, err := marshalValues(p, opts.Values)
values, err := readAssets(p, v)
if err != nil {
return opts, cleanup, err
}
out, err := yaml.Marshal(values)
if err != nil {
return opts, cleanup, err
}
err = persist(out)
if err != nil {
return nil, err
return opts, cleanup, err
}
base = MergeMaps(base, values)

return base, nil
opts.ValueFiles = valuesFiles

return opts, cleanup, nil
}

// readAsset reads the content of a Pulumi asset.
Expand Down Expand Up @@ -93,14 +113,14 @@ func readAsset(p getter.Providers, asset pulumi.Asset) ([]byte, error) {
}
}

// marshalValues converts Pulumi values to Helm values.
// - Expands assets to their content (to support --set-file).
func marshalValues(p getter.Providers, a map[string]interface{}) (map[string]interface{}, error) {
// readAssets converts Pulumi values to Helm values, hydrating Asset values
// along the way.
func readAssets(p getter.Providers, a map[string]interface{}) (map[string]interface{}, error) {
var err error
Comment on lines +116 to +118
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like marshalValues is a better name because it isn't just dealing with assets; imagine that resource references were marshaled in some way.

out := make(map[string]interface{}, len(a))
for k, v := range a {
if v, ok := v.(map[string]interface{}); ok {
out[k], err = marshalValues(p, v)
out[k], err = readAssets(p, v)
if err != nil {
return nil, err
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,18 @@
// See the License for the specific language governing permissions and
// limitations under the License.

package helm
package v4

import (
"bytes"
"os"
"path/filepath"
"testing"

"github.com/pulumi/pulumi/sdk/v3/go/pulumi"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"helm.sh/helm/v3/pkg/cli"
"helm.sh/helm/v3/pkg/getter"
)

Expand All @@ -37,7 +39,6 @@ func (m *mockGetter) Get(url string, options ...getter.Option) (*bytes.Buffer, e
}

func TestMergeValues(t *testing.T) {

bitnamiImage := `
image:
repository: bitnami/nginx
Expand Down Expand Up @@ -121,7 +122,7 @@ image:
},
want: map[string]interface{}{
"configuration": map[string]any{
"backupStorageLocation": []map[string]any{},
"backupStorageLocation": []any{},
Copy link
Contributor Author

@blampe blampe May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A subtle (probably inconsequential) difference between the previous MergeValues and upstream's.

},
},
},
Expand All @@ -143,30 +144,29 @@ image:
for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
merger := &ValueOpts{
ValuesFiles: tt.valuesFiles,
Values: tt.values,
}
p := getter.All(cli.New())
opts, cleanup, err := readValues(p, tt.values, tt.valuesFiles)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use getter.Providers{} for p?

defer cleanup()
require.NoError(t, err)

actual, err := merger.MergeValues(getter.Providers{})
actual, err := opts.MergeValues(p)
require.NoError(t, err)
assert.Equal(t, tt.want, actual)
})
}
}

func TestReadAsset(t *testing.T) {

bitnamiImage := `
image:
repository: bitnami/nginx
tag: latest
`
bitnamiImageFile, err := os.CreateTemp("", "pulumi-TestReadAsset-*.yaml")

tmp := t.TempDir()
fname := filepath.Join(tmp, "bitnami.yaml")
err := os.WriteFile(fname, []byte(bitnamiImage), 0o600)
require.NoError(t, err)
_, _ = bitnamiImageFile.WriteString(bitnamiImage)
_ = bitnamiImageFile.Close()
defer os.Remove(bitnamiImageFile.Name())

tests := []struct {
name string
Expand All @@ -182,7 +182,7 @@ image:
},
{
name: "file asset",
asset: pulumi.NewFileAsset(bitnamiImageFile.Name()),
asset: pulumi.NewFileAsset(fname),
want: []byte(bitnamiImage),
},
{
Expand Down
Loading