Skip to content

Commit

Permalink
Merge pull request #1123 from fluxcd/remove-subchart-crd
Browse files Browse the repository at this point in the history 
Fix install and upgrade applying subchart CRDs when condition is false
  • Loading branch information
@matheuscscp
matheuscscp authored Jan 10, 2025
2 parents 9b78c2e + dd3b66a commit 58d5812
Show file tree
Hide file tree
Showing 6 changed files with 433 additions and 3 deletions.
9 changes: 8 additions & 1 deletion internal/action/crds.go
View file
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
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
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 {
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
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
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

0 comments on commit 58d5812

Please sign in to comment.