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

Fix unmarshalling of Helm values yaml file #2815

Merged
merged 3 commits into from
Feb 9, 2024

Conversation

rquitales
Copy link
Member

@rquitales rquitales commented Feb 8, 2024

Proposed changes

Previously, we used gopkg.in/yaml.v3 as our yaml library to unmarshal helm values. This library allows unmarshalling complex mapping key types, which would be unmarshalled into map[any]any types instead of map[string]any.

Upstream Kubernetes has mostly switched to using their yaml fork (sigs.k8s.io/yaml), which upstream Helm is also using. sigs.k8s.io/yaml does not handle complex mapping key types, and instead, will try to marshal these keys as strings instead. In short, they convert yaml to json before marshaling/unmarshaling into a struct. To maintain compatibility with upstream Helm, this PR switches our yaml library to the Kubernetes fork to ensure the unmarshalled result is comparable.

Example yaml:

key1: value1
2: value2

With gopkg.in/yaml.v3 unmarshaling:

objType: map[any]any

With sigs.k8s.io/yaml:

objType: map[string]any

Changes made:

  • Switched yaml package to sigs.k8s.io/yaml
  • Expanded integration test case to incorporate complex mapping types in the values file, ensuring resolution of panics

Related issues (optional)

Fixes: #2814
Fixes #2684

@rquitales rquitales requested review from EronWright and a team February 8, 2024 01:39
@rquitales rquitales self-assigned this Feb 8, 2024
Copy link

github-actions bot commented Feb 8, 2024

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Copy link

codecov bot commented Feb 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (22663e6) 24.64% compared to head (d3700d0) 24.64%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2815   +/-   ##
=======================================
  Coverage   24.64%   24.64%           
=======================================
  Files          48       48           
  Lines        9710     9710           
=======================================
  Hits         2393     2393           
  Misses       7160     7160           
  Partials      157      157           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rquitales rquitales force-pushed the rquitales/fix-yaml-helm-merge branch from 72f72bb to d3700d0 Compare February 8, 2024 17:20
@t0yv0
Copy link
Member

t0yv0 commented Feb 9, 2024

I'm surprised of this behavior of "gopkg.in/yaml.v3", so it would result in map[any]any in the uncommon case that keys have non-strings in them? Or also in the common case? Is it possible for the change to introduce errors that were not there before?

LGTM of course looks very reasonable and surgical. But as you know I do not have a good handle on this codebase to have a sense of what could break.

@rquitales
Copy link
Member Author

I'm surprised of this behavior of "gopkg.in/yaml.v3", so it would result in map[any]any in the uncommon case that keys have non-strings in them?

The YAML specification is indeed complex and nuanced. The original author of sigs.k8s.io/yaml elaborates on the early challenges faced in the Kubernetes project in this blog post: The Right Way to Handle YAML in Golang.

Essentially, the logic go-yaml uses to unmarshal to Go structs differs from the standard library JSON package, creating an impedance mismatch. This discrepancy arises because upstream Kubernetes and Helm libraries internally utilize JSON unmarshaling to handle YAML files.

Or also in the common case? Is it possible for the change to introduce errors that were not there before?

As for the potential introduction of errors, I believe this change wouldn't introduce new errors. This PR specifically addresses the unmarshalling of custom helm values in the Helm Release resource, and is essentially scoped to 1 line of usage (ref: provider/helm_release.go#L298). Prior to this adjustment, the provider would have encountered a panic if a mapping key was anything other than a string. However, using normal Kubernetes or Helm wouldn't pose an issue, as the underlying sigs.k8s.io/yaml package attempts to convert non-string keys to strings. This PR essentially aligns the values file parsing with the approach used upstream by Helm.

@rquitales rquitales merged commit 6370a3b into master Feb 9, 2024
20 checks passed
@rquitales rquitales deleted the rquitales/fix-yaml-helm-merge branch February 9, 2024 21:05
lumiere-bot bot referenced this pull request in coolguy1771/home-ops Feb 24, 2024
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@pulumi/kubernetes](https://pulumi.com)
([source](https://togithub.com/pulumi/pulumi-kubernetes)) | dependencies
| minor | [`4.7.1` ->
`4.8.0`](https://renovatebot.com/diffs/npm/@pulumi%2fkubernetes/4.7.1/4.8.0)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>pulumi/pulumi-kubernetes (@&#8203;pulumi/kubernetes)</summary>

###
[`v4.8.0`](https://togithub.com/pulumi/pulumi-kubernetes/blob/HEAD/CHANGELOG.md#480-February-22-2024)

[Compare
Source](https://togithub.com/pulumi/pulumi-kubernetes/compare/v4.7.1...v4.8.0)

- Fix DiffConfig issue when when provider's kubeconfig is set to file
path
([https://github.com/pulumi/pulumi-kubernetes/pull/2771](https://togithub.com/pulumi/pulumi-kubernetes/pull/2771))
- Fix for replacement having incorrect status messages
([https://github.com/pulumi/pulumi-kubernetes/pull/2810](https://togithub.com/pulumi/pulumi-kubernetes/pull/2810))
- Use output properties for await logic
([https://github.com/pulumi/pulumi-kubernetes/pull/2790](https://togithub.com/pulumi/pulumi-kubernetes/pull/2790))
- Support for metadata.generateName (CSA)
([https://github.com/pulumi/pulumi-kubernetes/pull/2808](https://togithub.com/pulumi/pulumi-kubernetes/pull/2808))
- Fix unmarshalling of Helm values yaml file
([https://github.com/pulumi/pulumi-kubernetes/issues/2815](https://togithub.com/pulumi/pulumi-kubernetes/issues/2815))
- Handle unknowns in Helm Release resource
([https://github.com/pulumi/pulumi-kubernetes/pull/2822](https://togithub.com/pulumi/pulumi-kubernetes/pull/2822))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://togithub.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yMTAuMCIsInVwZGF0ZWRJblZlciI6IjM3LjIxMC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: lumiere-bot[bot] <98047013+lumiere-bot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nginx-Ingress install panic with helm.v3.Release Nginx Ingress no longer installs in v3.Release
3 participants