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

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
Open

Fix install and upgrade applying subchart CRDs when condition is false #1123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
232947a
Fix install and upgrade applying subchart CRDs when condition is false
matheuscscp Dec 10, 2024
0f909d6
Add test for CRDs not applied when condition is false
matheuscscp Dec 10, 2024
85c1c7b
Add more tests
matheuscscp Dec 11, 2024
f250d5d
Add test for imported values
matheuscscp Dec 12, 2024
f708e0d
Add comment about helm bug
matheuscscp Dec 12, 2024
0987c72
Add test for upgrade
matheuscscp Dec 13, 2024
da74708
Move common CRDs test chart to testutil package
matheuscscp Dec 13, 2024
667a0f8
Fix upgrade test keeping CRD from previous test
matheuscscp Dec 13, 2024
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
9 changes: 8 additions & 1 deletion internal/action/crds.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (

helmaction "helm.sh/helm/v3/pkg/action"
helmchart "helm.sh/helm/v3/pkg/chart"
helmchartutil "helm.sh/helm/v3/pkg/chartutil"
helmkube "helm.sh/helm/v3/pkg/kube"
apiextension "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
apierrors "k8s.io/apimachinery/pkg/api/errors"
Expand Down Expand Up @@ -64,7 +65,9 @@ func (*rootScoped) Name() apimeta.RESTScopeName {
return apimeta.RESTScopeNameRoot
}

func applyCRDs(cfg *helmaction.Configuration, policy v2.CRDsPolicy, chrt *helmchart.Chart, visitorFunc ...resource.VisitorFunc) error {
func applyCRDs(cfg *helmaction.Configuration, policy v2.CRDsPolicy, chrt *helmchart.Chart,
vals helmchartutil.Values, visitorFunc ...resource.VisitorFunc) error {

if len(chrt.CRDObjects()) == 0 {
return nil
}
Expand All @@ -74,6 +77,10 @@ func applyCRDs(cfg *helmaction.Configuration, policy v2.CRDsPolicy, chrt *helmch
return nil
}

if err := helmchartutil.ProcessDependenciesWithMerge(chrt, vals); err != nil {
return fmt.Errorf("failed to process chart dependencies: %w", err)
}

// Collect all CRDs from all files in `crds` directory.
allCRDs := make(helmkube.ResourceList, 0)
for _, obj := range chrt.CRDObjects() {
Expand Down
2 changes: 1 addition & 1 deletion internal/action/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func Install(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 {
return nil, fmt.Errorf("failed to apply CustomResourceDefinitions: %w", err)
}

Expand Down
2 changes: 1 addition & 1 deletion internal/action/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

return nil, fmt.Errorf("failed to apply CustomResourceDefinitions: %w", err)
}

Expand Down
140 changes: 140 additions & 0 deletions internal/reconcile/install_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,10 @@ import (
helmstorage "helm.sh/helm/v3/pkg/storage"
helmdriver "helm.sh/helm/v3/pkg/storage/driver"
corev1 "k8s.io/api/core/v1"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/tools/record"
"sigs.k8s.io/controller-runtime/pkg/client"

eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1"
"github.com/fluxcd/pkg/apis/meta"
Expand Down Expand Up @@ -300,6 +302,144 @@ func TestInstall_Reconcile(t *testing.T) {
}
}

func TestInstall_Reconcile_withSubchartWithCRDs(t *testing.T) {
getValues := func(subchartValues map[string]any) helmchartutil.Values {
return helmchartutil.Values{"subchart": subchartValues}
}

expectConditions := []metav1.Condition{
*conditions.TrueCondition(meta.ReadyCondition, v2.InstallSucceededReason,
"Helm install succeeded"),
*conditions.TrueCondition(v2.ReleasedCondition, v2.InstallSucceededReason,
"Helm install succeeded"),
}

expectHistory := func(releases []*helmrelease.Release) v2.Snapshots {
return v2.Snapshots{
release.ObservedToSnapshot(release.ObserveRelease(releases[0])),
}
}

for _, tt := range []struct {
name string
subchartValues map[string]any
subchartResourcesPresent bool
expectedMainChartValues map[string]any
}{
{
name: "subchart disabled should not deploy resources, including CRDs",
subchartValues: map[string]any{"enabled": false},
subchartResourcesPresent: false,
expectedMainChartValues: map[string]any{
"foo": "baz",
"myimports": map[string]any{"myint": 0},
},
},
{
name: "subchart enabled should deploy resources, including CRDs",
subchartValues: map[string]any{"enabled": true},
subchartResourcesPresent: true,
expectedMainChartValues: map[string]any{
"foo": "baz",
"myint": 123,
"myimports": map[string]any{"myint": 0}, // should be 456: https://github.com/helm/helm/issues/13223
"subchart": map[string]any{
"foo": "bar",
"global": map[string]any{},
"exports": map[string]any{"data": map[string]any{"myint": 123}},
"default": map[string]any{"data": map[string]any{"myint": 456}},
},
},
},
} {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)

namedNS, err := testEnv.CreateNamespace(context.TODO(), mockReleaseNamespace)
g.Expect(err).NotTo(HaveOccurred())
t.Cleanup(func() {
_ = testEnv.Delete(context.TODO(), namedNS)
})
releaseNamespace := namedNS.Name

obj := &v2.HelmRelease{
Spec: v2.HelmReleaseSpec{
ReleaseName: mockReleaseName,
TargetNamespace: releaseNamespace,
StorageNamespace: releaseNamespace,
Timeout: &metav1.Duration{Duration: 100 * time.Millisecond},
},
}

getter, err := RESTClientGetterFromManager(testEnv.Manager, obj.GetReleaseNamespace())
g.Expect(err).ToNot(HaveOccurred())

cfg, err := action.NewConfigFactory(getter,
action.WithStorage(action.DefaultStorageDriver, obj.GetStorageNamespace()),
)
g.Expect(err).ToNot(HaveOccurred())

store := helmstorage.Init(cfg.Driver)

chart := testutil.BuildChartWithSubchartWithCRD()
recorder := new(record.FakeRecorder)
got := (NewInstall(cfg, recorder)).Reconcile(context.TODO(), &Request{
Object: obj,
Chart: chart,
Values: getValues(tt.subchartValues),
})
g.Expect(got).ToNot(HaveOccurred())

g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(expectConditions))

releases, _ := store.History(mockReleaseName)
releaseutil.SortByRevision(releases)

g.Expect(obj.Status.History).To(testutil.Equal(expectHistory(releases)))

// Assert main chart configmap is present.
mainChartCM := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "cm-main-chart",
Namespace: releaseNamespace,
},
}
err = testEnv.Get(context.TODO(), client.ObjectKeyFromObject(mainChartCM), mainChartCM)
g.Expect(err).NotTo(HaveOccurred())

// Assert subchart configmap is absent or present.
subChartCM := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "cm-sub-chart",
Namespace: releaseNamespace,
},
}
err = testEnv.Get(context.TODO(), client.ObjectKeyFromObject(subChartCM), subChartCM)
if tt.subchartResourcesPresent {
g.Expect(err).NotTo(HaveOccurred())
} else {
g.Expect(err).To(HaveOccurred())
}

// Assert subchart CRD is absent or present.
subChartCRD := &apiextensionsv1.CustomResourceDefinition{
ObjectMeta: metav1.ObjectMeta{
Name: "crontabs.stable.example.com",
},
}
err = testEnv.Get(context.TODO(), client.ObjectKeyFromObject(subChartCRD), subChartCRD)
if tt.subchartResourcesPresent {
g.Expect(err).NotTo(HaveOccurred())
} else {
g.Expect(err).To(HaveOccurred())
}

// Assert main chart values.
g.Expect(chart.Values).To(testutil.Equal(tt.expectedMainChartValues))
})
}
}

func TestInstall_failure(t *testing.T) {
var (
obj = &v2.HelmRelease{
Expand Down
169 changes: 169 additions & 0 deletions internal/reconcile/upgrade_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,10 @@ import (
helmstorage "helm.sh/helm/v3/pkg/storage"
helmdriver "helm.sh/helm/v3/pkg/storage/driver"
corev1 "k8s.io/api/core/v1"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/tools/record"
"sigs.k8s.io/controller-runtime/pkg/client"

eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1"
"github.com/fluxcd/pkg/apis/meta"
Expand Down Expand Up @@ -431,6 +433,173 @@ func TestUpgrade_Reconcile(t *testing.T) {
}
}

func TestUpgrade_Reconcile_withSubchartWithCRDs(t *testing.T) {
getValues := func(subchartValues map[string]any) helmchartutil.Values {
return helmchartutil.Values{"subchart": subchartValues}
}

releases := func(namespace string) []*helmrelease.Release {
return []*helmrelease.Release{
testutil.BuildRelease(&helmrelease.MockReleaseOptions{
Name: mockReleaseName,
Namespace: namespace,
Chart: testutil.BuildChart(testutil.ChartWithTestHook()),
Version: 1,
Status: helmrelease.StatusDeployed,
}),
}
}

status := func(releases []*helmrelease.Release) v2.HelmReleaseStatus {
return v2.HelmReleaseStatus{
History: v2.Snapshots{
release.ObservedToSnapshot(release.ObserveRelease(releases[0])),
},
}
}

expectConditions := []metav1.Condition{
*conditions.TrueCondition(meta.ReadyCondition, v2.UpgradeSucceededReason, "Helm upgrade succeeded"),
*conditions.TrueCondition(v2.ReleasedCondition, v2.UpgradeSucceededReason, "Helm upgrade succeeded"),
}

expectHistory := func(releases []*helmrelease.Release) v2.Snapshots {
return v2.Snapshots{
release.ObservedToSnapshot(release.ObserveRelease(releases[1])),
release.ObservedToSnapshot(release.ObserveRelease(releases[0])),
}
}

for _, tt := range []struct {
name string
subchartValues map[string]any
subchartResourcesPresent bool
expectedMainChartValues map[string]any
}{
{
name: "subchart disabled should not deploy resources, including CRDs",
subchartValues: map[string]any{"enabled": false},
subchartResourcesPresent: false,
expectedMainChartValues: map[string]any{
"foo": "baz",
"myimports": map[string]any{"myint": 0},
},
},
{
name: "subchart enabled should deploy resources, including CRDs",
subchartValues: map[string]any{"enabled": true},
subchartResourcesPresent: true,
expectedMainChartValues: map[string]any{
"foo": "baz",
"myint": 123,
"myimports": map[string]any{"myint": 0}, // should be 456: https://github.com/helm/helm/issues/13223
"subchart": map[string]any{
"foo": "bar",
"global": map[string]any{},
"exports": map[string]any{"data": map[string]any{"myint": 123}},
"default": map[string]any{"data": map[string]any{"myint": 456}},
},
},
},
} {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)

namedNS, err := testEnv.CreateNamespace(context.TODO(), mockReleaseNamespace)
g.Expect(err).NotTo(HaveOccurred())
t.Cleanup(func() {
_ = testEnv.Delete(context.TODO(), namedNS)
})
releaseNamespace := namedNS.Name

releases := releases(releaseNamespace)
helmreleaseutil.SortByRevision(releases)

obj := &v2.HelmRelease{
Spec: v2.HelmReleaseSpec{
ReleaseName: mockReleaseName,
TargetNamespace: releaseNamespace,
StorageNamespace: releaseNamespace,
Timeout: &metav1.Duration{Duration: 100 * time.Millisecond},
},
}
obj.Status = status(releases)

getter, err := RESTClientGetterFromManager(testEnv.Manager, obj.GetReleaseNamespace())
g.Expect(err).ToNot(HaveOccurred())

cfg, err := action.NewConfigFactory(getter,
action.WithStorage(action.DefaultStorageDriver, obj.GetStorageNamespace()),
)
g.Expect(err).ToNot(HaveOccurred())

store := helmstorage.Init(cfg.Driver)
for _, r := range releases {
g.Expect(store.Create(r)).To(Succeed())
}

// Delete any prior CRD.
subChartCRD := &apiextensionsv1.CustomResourceDefinition{
ObjectMeta: metav1.ObjectMeta{
Name: "crontabs.stable.example.com",
},
}
_ = testEnv.Delete(context.TODO(), subChartCRD)

chart := testutil.BuildChartWithSubchartWithCRD()
recorder := new(record.FakeRecorder)
got := NewUpgrade(cfg, recorder).Reconcile(context.TODO(), &Request{
Object: obj,
Chart: chart,
Values: getValues(tt.subchartValues),
})
g.Expect(got).ToNot(HaveOccurred())

g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(expectConditions))

releases, _ = store.History(mockReleaseName)
helmreleaseutil.SortByRevision(releases)

g.Expect(obj.Status.History).To(testutil.Equal(expectHistory(releases)))

// Assert main chart configmap is present.
mainChartCM := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "cm-main-chart",
Namespace: releaseNamespace,
},
}
err = testEnv.Get(context.TODO(), client.ObjectKeyFromObject(mainChartCM), mainChartCM)
g.Expect(err).NotTo(HaveOccurred())

// Assert subchart configmap is absent or present.
subChartCM := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "cm-sub-chart",
Namespace: releaseNamespace,
},
}
err = testEnv.Get(context.TODO(), client.ObjectKeyFromObject(subChartCM), subChartCM)
if tt.subchartResourcesPresent {
g.Expect(err).NotTo(HaveOccurred())
} else {
g.Expect(err).To(HaveOccurred())
}

// Assert subchart CRD is absent or present.
err = testEnv.Get(context.TODO(), client.ObjectKeyFromObject(subChartCRD), subChartCRD)
if tt.subchartResourcesPresent {
g.Expect(err).NotTo(HaveOccurred())
} else {
g.Expect(err).To(HaveOccurred())
}

// Assert main chart values.
g.Expect(chart.Values).To(testutil.Equal(tt.expectedMainChartValues))
})
}
}

func TestUpgrade_failure(t *testing.T) {
var (
obj = &v2.HelmRelease{
Expand Down
Loading
Loading