diff --git a/internal/action/crds.go b/internal/action/crds.go index 883b9ded7..a2dc25825 100644 --- a/internal/action/crds.go +++ b/internal/action/crds.go @@ -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" @@ -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 } @@ -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() { diff --git a/internal/action/install.go b/internal/action/install.go index da093c688..842c7ac09 100644 --- a/internal/action/install.go +++ b/internal/action/install.go @@ -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) } diff --git a/internal/action/upgrade.go b/internal/action/upgrade.go index 079a1336a..db7f792fa 100644 --- a/internal/action/upgrade.go +++ b/internal/action/upgrade.go @@ -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 { return nil, fmt.Errorf("failed to apply CustomResourceDefinitions: %w", err) } diff --git a/internal/reconcile/install_test.go b/internal/reconcile/install_test.go index c1c605986..a2bca19ad 100644 --- a/internal/reconcile/install_test.go +++ b/internal/reconcile/install_test.go @@ -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" @@ -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{ diff --git a/internal/reconcile/upgrade_test.go b/internal/reconcile/upgrade_test.go index 641862403..bf45a913e 100644 --- a/internal/reconcile/upgrade_test.go +++ b/internal/reconcile/upgrade_test.go @@ -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" @@ -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{ diff --git a/internal/testutil/mock_chart.go b/internal/testutil/mock_chart.go index 9b5686b29..29380474c 100644 --- a/internal/testutil/mock_chart.go +++ b/internal/testutil/mock_chart.go @@ -20,6 +20,7 @@ import ( "fmt" helmchart "helm.sh/helm/v3/pkg/chart" + helmchartutil "helm.sh/helm/v3/pkg/chartutil" ) var manifestTmpl = `apiVersion: v1 @@ -31,6 +32,15 @@ data: foo: bar ` +var manifestWithCustomNameTmpl = `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-%[1]s + namespace: %[2]s +data: + foo: bar +` + var manifestWithHookTmpl = `apiVersion: v1 kind: ConfigMap metadata: @@ -82,6 +92,38 @@ spec: restartPolicy: Never ` +var crdManifest = `apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: crontabs.stable.example.com +spec: + group: stable.example.com + versions: + - name: v1 + served: true + storage: true + schema: + openAPIV3Schema: + type: object + properties: + spec: + type: object + properties: + cronSpec: + type: string + image: + type: string + replicas: + type: integer + scope: Namespaced + names: + plural: crontabs + singular: crontab + kind: CronTab + shortNames: + - ct +` + // ChartOptions is a helper to build a Helm chart object. type ChartOptions struct { *helmchart.Chart @@ -166,3 +208,75 @@ func ChartWithFailingTestHook() ChartOption { }) } } + +// ChartWithManifestWithCustomName sets the name of the manifest. +func ChartWithManifestWithCustomName(name string) ChartOption { + return func(opts *ChartOptions) { + opts.Templates = []*helmchart.File{ + { + Name: "templates/manifest", + Data: []byte(fmt.Sprintf(manifestWithCustomNameTmpl, name, "{{ default .Release.Namespace }}")), + }, + } + } +} + +// ChartWithCRD appends a CRD to the chart. +func ChartWithCRD() ChartOption { + return func(opts *ChartOptions) { + opts.Files = []*helmchart.File{ + { + Name: "crds/crd.yaml", + Data: []byte(crdManifest), + }, + } + } +} + +// ChartWithDependency appends a dependency to the chart. +func ChartWithDependency(md *helmchart.Dependency, chrt *helmchart.Chart) ChartOption { + return func(opts *ChartOptions) { + opts.Metadata.Dependencies = append(opts.Metadata.Dependencies, md) + opts.AddDependency(chrt) + } +} + +// ChartWithValues sets the values.yaml file of the chart. +func ChartWithValues(values map[string]any) ChartOption { + return func(opts *ChartOptions) { + opts.Values = values + } +} + +// BuildChartWithSubchartWithCRD returns a Helm chart object with a subchart +// that contains a CRD. Useful for testing helm-controller's staged CRDs-first +// deployment logic. +func BuildChartWithSubchartWithCRD() *helmchart.Chart { + subChart := BuildChart( + ChartWithName("subchart"), + ChartWithManifestWithCustomName("sub-chart"), + ChartWithCRD(), + ChartWithValues(helmchartutil.Values{ + "foo": "bar", + "exports": map[string]any{"data": map[string]any{"myint": 123}}, + "default": map[string]any{"data": map[string]any{"myint": 456}}, + })) + mainChart := BuildChart( + ChartWithManifestWithCustomName("main-chart"), + ChartWithValues(helmchartutil.Values{ + "foo": "baz", + "myimports": map[string]any{"myint": 0}, + }), + ChartWithDependency(&helmchart.Dependency{ + Name: "subchart", + Condition: "subchart.enabled", + ImportValues: []any{ + "data", + map[string]any{ + "child": "default.data", + "parent": "myimports", + }, + }, + }, subChart)) + return mainChart +}