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

Merged
merged 1 commit into from
Jan 10, 2025
Merged

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
1 commit
Select commit
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 {
stefanprodan marked this conversation as resolved.
Show resolved Hide resolved
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