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 install and upgrade applying subchart CRDs when condition is false #1123

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

matheuscscp
Copy link
Member

Fixes #938

Supersedes #941

@matheuscscp matheuscscp marked this pull request as ready for review December 11, 2024 16:22
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @matheuscscp 🎖️

@matheuscscp
Copy link
Member Author

Will add a test for upgrade tomorrow.

@matheuscscp matheuscscp marked this pull request as draft December 13, 2024 14:58
@matheuscscp matheuscscp marked this pull request as ready for review December 13, 2024 14:58
@@ -55,7 +55,7 @@ func Upgrade(ctx context.Context, config *helmaction.Configuration, obj *v2.Helm
if err != nil {
return nil, err
}
if err := applyCRDs(config, policy, chrt, setOriginVisitor(v2.GroupVersion.Group, obj.Namespace, obj.Name)); err != nil {
if err := applyCRDs(config, policy, chrt, vals, setOriginVisitor(v2.GroupVersion.Group, obj.Namespace, obj.Name)); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems I have encountered a related bug for the upgrade in our code. But it could be debatable considering how we pass values in helm.

While reading the upstream upgrade code that calls ProcessDependenciesWithMerge(), I noticed that unlike the install code, it reuses the value from the previous release, refer https://github.com/helm/helm/blob/v3.16.3/pkg/action/upgrade.go#L240-L246. This made me wonder about the scenario where initially, the dependency condition evaluates to false, no subchart CRD gets installed. But in the next upgrade, if the same values are not explicitly set, would it result in a different result? I tried this scenario with helm CLI and helm-controller and saw different results.

I created a chart in which the default values.yaml doesn't have the value that determines the chart dependency condition. The dependencies in Chart.yaml has:

dependencies:
  - name: mysubchart
    condition: mysubchart.enabled

But mysubchart.enabled is not set in the default values.yaml file.

While installing, I passed the value explicitly to disable the subchart:

$ helm install full-coral ./mychart/ --set 'mysubchart.enabled=false'

After installing the chart, when I query the values of the release, I get:

$ helm get values full-coral
USER-SUPPLIED VALUES:
mysubchart:
  enabled: false

Then I run upgrade:

$ helm upgrade full-coral ./mychart

And see the values from the first release version still present:

$ helm get values full-coral
USER-SUPPLIED VALUES:
mysubchart:
  enabled: false

And the subchart CRD has not been installed.

Trying the same scenario with HC.
Setting the values in HR:

  values:
    mysubchart:
      enabled: false

After first install, querying the values results in:

$ helm get values mychart-release 
USER-SUPPLIED VALUES:
mysubchart:
  enabled: false

Subchart CRD is not installed.

Removing the mysubchart value from HR and upgrading results in the subchart CRD getting installed and querying the values:

$ helm get values mychart-release
USER-SUPPLIED VALUES:
null

Even if I pass -a to see all the values, there are other default values but there's no mysubchart value present.

The way values are used in HR, removing the values may not be something users are likely to do, but this shows a difference in behavior. Helm-controller doesn't reuse the values from the previous release if no values are explicitly passed during upgrade. This may be a separate issue as a whole, but applying the same theory of value reuse in upgrade to what this PR is trying to address, it still seems like a bug. The ProcessDependenciesWithMerge() call for apply CRD should receive the reused values, like the upstream upgrade does, to prevent subchart CRDs from getting installed.

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.

HelmRelease: CRDs of disabled subcharts get installed anyway
3 participants