Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

chore: fix generic map conversion #1838

Merged
merged 4 commits into from
Feb 10, 2025
Merged

chore: fix generic map conversion #1838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4890139
chore: fix generic map conversion
emosbaugh Feb 6, 2025
0e761a5
f
emosbaugh Feb 7, 2025
7e18a4e
f
emosbaugh Feb 8, 2025
dfa3fa3
f
emosbaugh Feb 8, 2025
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: 6 additions & 7 deletions pkg/extensions/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
ecv1beta1 "github.com/replicatedhq/embedded-cluster/kinds/apis/v1beta1"
"github.com/replicatedhq/embedded-cluster/pkg/helm"
"github.com/sirupsen/logrus"
"gopkg.in/yaml.v3"
helmrepo "helm.sh/helm/v3/pkg/repo"
)

Expand Down Expand Up @@ -40,12 +39,12 @@ func addRepos(hcli helm.Client, repos []k0sv1beta1.Repository) error {
}

func install(ctx context.Context, hcli helm.Client, ext ecv1beta1.Chart) error {
var values map[string]interface{}
if err := yaml.Unmarshal([]byte(ext.Values), &values); err != nil {
values, err := helm.UnmarshalValues(ext.Values)
if err != nil {
return errors.Wrap(err, "unmarshal values")
}

_, err := hcli.Install(ctx, helm.InstallOptions{
_, err = hcli.Install(ctx, helm.InstallOptions{
ReleaseName: ext.Name,
ChartPath: ext.ChartName,
ChartVersion: ext.Version,
Expand All @@ -61,8 +60,8 @@ func install(ctx context.Context, hcli helm.Client, ext ecv1beta1.Chart) error {
}

func upgrade(ctx context.Context, hcli helm.Client, ext ecv1beta1.Chart) error {
var values map[string]interface{}
if err := yaml.Unmarshal([]byte(ext.Values), &values); err != nil {
values, err := helm.UnmarshalValues(ext.Values)
if err != nil {
return errors.Wrap(err, "unmarshal values")
}

Expand All @@ -78,7 +77,7 @@ func upgrade(ctx context.Context, hcli helm.Client, ext ecv1beta1.Chart) error {
if ext.ForceUpgrade != nil {
opts.Force = *ext.ForceUpgrade
}
_, err := hcli.Upgrade(ctx, opts)
_, err = hcli.Upgrade(ctx, opts)
if err != nil {
return errors.Wrap(err, "helm upgrade")
}
Expand Down
103 changes: 26 additions & 77 deletions pkg/helm/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (

"github.com/Masterminds/semver/v3"
"github.com/sirupsen/logrus"
"gopkg.in/yaml.v2"
"gopkg.in/yaml.v3"
"helm.sh/helm/v3/pkg/action"
"helm.sh/helm/v3/pkg/chart"
"helm.sh/helm/v3/pkg/chart/loader"
Expand All @@ -28,6 +28,7 @@ import (
"helm.sh/helm/v3/pkg/storage/driver"
"helm.sh/helm/v3/pkg/uploader"
"k8s.io/cli-runtime/pkg/genericclioptions"
k8syaml "sigs.k8s.io/yaml"
)

type RESTClientGetterFactory func(namespace string) genericclioptions.RESTClientGetter
Expand Down Expand Up @@ -145,7 +146,7 @@ func (h *HelmClient) prepare() error {
return nil
}

data, err := yaml.Marshal(repo.File{Repositories: h.repos})
data, err := k8syaml.Marshal(repo.File{Repositories: h.repos})
if err != nil {
return fmt.Errorf("marshal repositories: %w", err)
}
Expand Down Expand Up @@ -356,7 +357,10 @@ func (h *HelmClient) Install(ctx context.Context, opts InstallOptions) (*release
}
}

cleanVals := cleanUpGenericMap(opts.Values)
cleanVals, err := cleanUpGenericMap(opts.Values)
if err != nil {
return nil, fmt.Errorf("clean up generic map: %w", err)
}

release, err := client.RunWithContext(ctx, chartRequested, cleanVals)
if err != nil {
Expand Down Expand Up @@ -410,7 +414,10 @@ func (h *HelmClient) Upgrade(ctx context.Context, opts UpgradeOptions) (*release
}
}

cleanVals := cleanUpGenericMap(opts.Values)
cleanVals, err := cleanUpGenericMap(opts.Values)
if err != nil {
return nil, fmt.Errorf("clean up generic map: %w", err)
}

release, err := client.RunWithContext(ctx, opts.ReleaseName, chartRequested, cleanVals)
if err != nil {
Expand Down Expand Up @@ -473,7 +480,10 @@ func (h *HelmClient) Render(releaseName string, chartPath string, values map[str
}
}

cleanVals := cleanUpGenericMap(values)
cleanVals, err := cleanUpGenericMap(values)
if err != nil {
return nil, fmt.Errorf("clean up generic map: %w", err)
}

release, err := client.Run(chartRequested, cleanVals)
if err != nil {
Expand Down Expand Up @@ -526,80 +536,19 @@ func (h *HelmClient) getRESTClientGetter(namespace string) genericclioptions.RES
return cfgFlags
}

// cleanUpGenericMap is a helper to "cleanup" generic yaml parsing where nested maps
// are unmarshalled with type map[interface{}]interface{}
func cleanUpGenericMap(in map[string]interface{}) map[string]interface{} {
result := make(map[string]interface{})
for k, v := range in {
result[fmt.Sprintf("%v", k)] = cleanUpMapValue(v)
}
return result
}

// Cleans up the value in the map, recurses in case of arrays and maps
func cleanUpMapValue(v interface{}) interface{} {
// Keep null values as nil to avoid type mismatches
if v == nil {
return nil
}
switch v := v.(type) {
case []interface{}:
return cleanUpInterfaceArray(v)
case []map[string]interface{}:
return cleanUpGenericMapArray(v)
case []map[interface{}]interface{}:
return cleanUpInterfaceMapArray(v)
case map[string]interface{}:
return cleanUpGenericMap(v)
case map[interface{}]interface{}:
return cleanUpInterfaceMap(v)
case string:
return v
case int:
return v
case bool:
return v
case float64:
return v
default:
return fmt.Sprintf("%v", v)
}
}

// Cleans up a slice of interfaces into slice of actual values
func cleanUpInterfaceArray(in []interface{}) []interface{} {
result := make([]interface{}, len(in))
for i, v := range in {
result[i] = cleanUpMapValue(v)
}
return result
}

// Cleans up a slice of map to interface into slice of actual values
func cleanUpGenericMapArray(in []map[string]interface{}) []map[string]interface{} {
result := make([]map[string]interface{}, len(in))
for i, v := range in {
result[i] = cleanUpGenericMap(v)
}
return result
}

// Cleans up a slice of map to interface into slice of actual values
func cleanUpInterfaceMapArray(in []map[interface{}]interface{}) []map[string]interface{} {
result := make([]map[string]interface{}, len(in))
for i, v := range in {
result[i] = cleanUpInterfaceMap(v)
func cleanUpGenericMap(m map[string]interface{}) (map[string]interface{}, error) {
// we must first use yaml marshal to convert the map[interface{}]interface{} to a []byte
// otherwise we will get an error "unsupported type: map[interface {}]interface {}"
b, err := yaml.Marshal(m)
Copy link
Member

Choose a reason for hiding this comment

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

why not k8syaml?

Copy link
Member Author

Choose a reason for hiding this comment

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

i think because k8syaml cannot marshal a map[interface{}]interface{}. i will test and add a comment

Copy link
Member Author

Choose a reason for hiding this comment

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

i added comments to clarify

if err != nil {
return nil, fmt.Errorf("yaml marshal: %w", err)
}
return result
}

// Cleans up the map keys to be strings
func cleanUpInterfaceMap(in map[interface{}]interface{}) map[string]interface{} {
result := make(map[string]interface{})
for k, v := range in {
result[fmt.Sprintf("%v", k)] = cleanUpMapValue(v)
next := map[string]interface{}{}
err = k8syaml.Unmarshal(b, &next)
if err != nil {
return nil, fmt.Errorf("yaml unmarshal: %w", err)
}
return result
return next, nil
}

func _logFn(format string, args ...interface{}) {
Expand Down
28 changes: 18 additions & 10 deletions pkg/helm/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"testing"

"github.com/stretchr/testify/require"
k8syaml "sigs.k8s.io/yaml"
)

func Test_cleanUpGenericMap(t *testing.T) {
Expand All @@ -25,7 +26,7 @@ func Test_cleanUpGenericMap(t *testing.T) {
},
want: map[string]interface{}{
"abc": "xyz",
"number": 5,
"number": float64(5),
"float": 1.5,
"bool": true,
"array": []interface{}{
Expand All @@ -49,7 +50,7 @@ func Test_cleanUpGenericMap(t *testing.T) {
want: map[string]interface{}{
"nest": map[string]interface{}{
"abc": "xyz",
"number": 5,
"number": float64(5),
"float": 1.5,
"bool": true,
"array": []interface{}{
Expand All @@ -74,7 +75,7 @@ func Test_cleanUpGenericMap(t *testing.T) {
want: map[string]interface{}{
"nest": map[string]interface{}{
"abc": "xyz",
"number": 5,
"number": float64(5),
"float": 1.5,
"bool": true,
"array": []interface{}{
Expand Down Expand Up @@ -102,11 +103,11 @@ func Test_cleanUpGenericMap(t *testing.T) {
want: map[string]interface{}{
"nest": map[string]interface{}{
"abc": "xyz",
"number": 5,
"number": float64(5),
"float": 1.5,
"bool": true,
"array": []map[string]interface{}{
{
"array": []interface{}{
Copy link
Member

Choose a reason for hiding this comment

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

think helm will accept an []interface{}?

Copy link
Member Author

Choose a reason for hiding this comment

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

i assume since its passing tests...

Copy link
Member

Choose a reason for hiding this comment

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

our e2e tests never caught this bug, which is why these unit tests were introduced. so i wouldn't be so confident...

Copy link
Member

Choose a reason for hiding this comment

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

if want did not change at all in these unit tests, i would be more confident in the change.

Copy link
Member Author

@emosbaugh emosbaugh Feb 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

i added this to the test

https://github.com/replicatedhq/embedded-cluster/pull/1838/files#diff-3b840e94038dbcf173c67aaaaaf40cb03afb791f1322baad32026a1979521613R157-R158

map[string]interface{}{
"name": "example",
"value": "true",
},
Expand All @@ -133,11 +134,11 @@ func Test_cleanUpGenericMap(t *testing.T) {
want: map[string]interface{}{
"nest": map[string]interface{}{
"abc": "xyz",
"number": 5,
"number": float64(5),
"float": 1.5,
"bool": true,
"array": []map[string]interface{}{
{
"array": []interface{}{
map[string]interface{}{
"name": "example",
"value": "true",
},
Expand All @@ -149,7 +150,14 @@ func Test_cleanUpGenericMap(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
req := require.New(t)
req.Equal(tt.want, cleanUpGenericMap(tt.in))
out, err := cleanUpGenericMap(tt.in)
req.NoError(err, "cleanUpGenericMap failed")
req.Equal(tt.want, out)

// ultimately helm calls k8syaml.Marshal so we must make sure that the output is compatible
Copy link
Member

Choose a reason for hiding this comment

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

did you verify (even if manually) that this would've caught the issue fixed by #1834?

Copy link
Member Author

Choose a reason for hiding this comment

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

How can I verify? Can you provide reproduction steps? They are not in the original fix.

// https://github.com/helm/helm/blob/v3.17.0/pkg/chartutil/values.go#L39
_, err = k8syaml.Marshal(out)
req.NoError(err, "yaml marshal failed")
})
}
}
4 changes: 2 additions & 2 deletions pkg/helm/images.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import (

"github.com/distribution/reference"
"github.com/replicatedhq/embedded-cluster/pkg/helpers"
"gopkg.in/yaml.v2"
"helm.sh/helm/v3/pkg/chart"
k8syaml "sigs.k8s.io/yaml"
)

type reducedResource struct {
Expand Down Expand Up @@ -101,7 +101,7 @@ func extractImagesFromK8sManifest(resource []byte) ([]string, error) {
images := []string{}

r := reducedResource{}
if err := yaml.Unmarshal([]byte(resource), &r); err != nil {
if err := k8syaml.Unmarshal([]byte(resource), &r); err != nil {
// Not a k8s resource, ignore
return nil, nil
}
Expand Down
5 changes: 2 additions & 3 deletions pkg/helm/values.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,22 @@ import (

jsonpatch "github.com/evanphx/json-patch"
"github.com/ohler55/ojg/jp"
"gopkg.in/yaml.v3"
"helm.sh/helm/v3/pkg/strvals"
k8syaml "sigs.k8s.io/yaml"
)

// UnmarshalValues unmarshals the given JSON compatible YAML string into a map[string]interface{}.
func UnmarshalValues(valuesYaml string) (map[string]interface{}, error) {
newValuesMap := map[string]interface{}{}
if err := yaml.Unmarshal([]byte(valuesYaml), &newValuesMap); err != nil {
if err := k8syaml.Unmarshal([]byte(valuesYaml), &newValuesMap); err != nil {
return nil, fmt.Errorf("yaml unmarshal: %w", err)
}
return newValuesMap, nil
}

// MarshalValues marshals the given map[string]interface{} into a JSON compatible YAML string.
func MarshalValues(values map[string]interface{}) (string, error) {
newValuesYaml, err := yaml.Marshal(values)
newValuesYaml, err := k8syaml.Marshal(values)
if err != nil {
return "", fmt.Errorf("yaml marshal: %w", err)
}
Expand Down
Loading