Skip to content

Commit 9698135

Browse files
author
Per Goncalves da Silva
committed
Refactor applier to use HelmChartLoader
Signed-off-by: Per Goncalves da Silva <[email protected]>
1 parent 365831a commit 9698135

File tree

7 files changed

+426
-163
lines changed

7 files changed

+426
-163
lines changed

‎cmd/operator-controller/main.go‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -439,14 +439,26 @@ func run() error {
439439
isWebhookSupportEnabled = true
440440
}
441441

442+
// create and configure chart loaders
443+
bundleLoaders := map[applier.BundleType]applier.HelmChartLoader{
444+
applier.BundleTypeRegistryV1: &applier.RegistryV1BundleLoader{
445+
BundleToHelmChartConverter: convert.BundleToHelmChartConverter{
446+
BundleRenderer: registryv1.Renderer,
447+
CertificateProvider: certProvider,
448+
IsWebhookSupportEnabled: isWebhookSupportEnabled,
449+
},
450+
},
451+
}
452+
if features.OperatorControllerFeatureGate.Enabled(features.HelmChartSupport) {
453+
bundleLoaders[applier.BundleTypeHelm] = &applier.HelmBundleLoader{}
454+
}
455+
442456
// now initialize the helmApplier, assigning the potentially nil preAuth
443457
helmApplier := &applier.Helm{
444458
ActionClientGetter: acg,
445459
Preflights: preflights,
446-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{
447-
BundleRenderer: registryv1.Renderer,
448-
CertificateProvider: certProvider,
449-
IsWebhookSupportEnabled: isWebhookSupportEnabled,
460+
HelmChartLoader: applier.BundleFSChartLoader{
461+
HelmChartLoaders: bundleLoaders,
450462
},
451463
PreAuthorizer: preAuth,
452464
}

‎internal/operator-controller/applier/helm.go‎

Lines changed: 15 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,8 @@ import (
2626

2727
ocv1 "github.com/operator-framework/operator-controller/api/v1"
2828
"github.com/operator-framework/operator-controller/internal/operator-controller/authorization"
29-
"github.com/operator-framework/operator-controller/internal/operator-controller/features"
30-
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source"
3129
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/preflights/crdupgradesafety"
3230
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util"
33-
imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image"
3431
)
3532

3633
const (
@@ -56,15 +53,19 @@ type Preflight interface {
5653
Upgrade(context.Context, *release.Release) error
5754
}
5855

59-
type BundleToHelmChartConverter interface {
60-
ToHelmChart(bundle source.BundleSource, installNamespace string, watchNamespace string) (*chart.Chart, error)
56+
// HelmChartLoader loads helm charts from bundle filesystems
57+
type HelmChartLoader interface {
58+
// Load loads a Helm Chart from the filesytem and applies the Install- and WatchNamespace values (if appropriate).
59+
// If the returned chart is nil and there is no error, this indicates that the underlying filesystem is not a
60+
// Helm bundle. If there is an error, this is indeterminate.
61+
Load(bundleFS fs.FS, installNamespace string, watchNamespace string) (*chart.Chart, error)
6162
}
6263

6364
type Helm struct {
64-
ActionClientGetter helmclient.ActionClientGetter
65-
Preflights []Preflight
66-
PreAuthorizer authorization.PreAuthorizer
67-
BundleToHelmChartConverter BundleToHelmChartConverter
65+
ActionClientGetter helmclient.ActionClientGetter
66+
Preflights []Preflight
67+
PreAuthorizer authorization.PreAuthorizer
68+
HelmChartLoader HelmChartLoader
6869
}
6970

7071
// shouldSkipPreflight is a helper to determine if the preflight check is CRDUpgradeSafety AND
@@ -122,7 +123,7 @@ func (h *Helm) runPreAuthorizationChecks(ctx context.Context, ext *ocv1.ClusterE
122123
}
123124

124125
func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels map[string]string, storageLabels map[string]string) ([]client.Object, string, error) {
125-
chrt, err := h.buildHelmChart(contentFS, ext)
126+
chrt, err := h.loadHelmChart(contentFS, ext)
126127
if err != nil {
127128
return nil, "", err
128129
}
@@ -203,26 +204,15 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
203204
return relObjects, state, nil
204205
}
205206

206-
func (h *Helm) buildHelmChart(bundleFS fs.FS, ext *ocv1.ClusterExtension) (*chart.Chart, error) {
207-
if h.BundleToHelmChartConverter == nil {
208-
return nil, errors.New("BundleToHelmChartConverter is nil")
207+
func (h *Helm) loadHelmChart(bundleFS fs.FS, ext *ocv1.ClusterExtension) (*chart.Chart, error) {
208+
if h.HelmChartLoader == nil {
209+
return nil, fmt.Errorf("HelmChartLoader is not set")
209210
}
210211
watchNamespace, err := GetWatchNamespace(ext)
211212
if err != nil {
212213
return nil, err
213214
}
214-
if features.OperatorControllerFeatureGate.Enabled(features.HelmChartSupport) {
215-
meta := new(chart.Metadata)
216-
if ok, _ := imageutil.IsBundleSourceChart(bundleFS, meta); ok {
217-
return imageutil.LoadChartFSWithOptions(
218-
bundleFS,
219-
fmt.Sprintf("%s-%s.tgz", meta.Name, meta.Version),
220-
imageutil.WithInstallNamespace(ext.Spec.Namespace),
221-
)
222-
}
223-
}
224-
225-
return h.BundleToHelmChartConverter.ToHelmChart(source.FromFS(bundleFS), ext.Spec.Namespace, watchNamespace)
215+
return h.HelmChartLoader.Load(bundleFS, ext.Spec.Namespace, watchNamespace)
226216
}
227217

228218
func (h *Helm) renderClientOnlyRelease(ctx context.Context, ext *ocv1.ClusterExtension, chrt *chart.Chart, values chartutil.Values, post postrender.PostRenderer) (*release.Release, error) {

‎internal/operator-controller/applier/helm_test.go‎

Lines changed: 58 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"errors"
66
"io"
7+
"io/fs"
78
"os"
89
"testing"
910
"testing/fstest"
@@ -26,8 +27,6 @@ import (
2627
"github.com/operator-framework/operator-controller/internal/operator-controller/applier"
2728
"github.com/operator-framework/operator-controller/internal/operator-controller/authorization"
2829
"github.com/operator-framework/operator-controller/internal/operator-controller/features"
29-
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source"
30-
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/convert"
3130
)
3231

3332
type mockPreflight struct {
@@ -197,8 +196,8 @@ func TestApply_Base(t *testing.T) {
197196
t.Run("fails trying to obtain an action client", func(t *testing.T) {
198197
mockAcg := &mockActionGetter{actionClientForErr: errors.New("failed getting action client")}
199198
helmApplier := applier.Helm{
200-
ActionClientGetter: mockAcg,
201-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
199+
ActionClientGetter: mockAcg,
200+
HelmChartLoader: noOpHelmChartLoader(),
202201
}
203202

204203
objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
@@ -211,8 +210,8 @@ func TestApply_Base(t *testing.T) {
211210
t.Run("fails getting current release and !driver.ErrReleaseNotFound", func(t *testing.T) {
212211
mockAcg := &mockActionGetter{getClientErr: errors.New("failed getting current release")}
213212
helmApplier := applier.Helm{
214-
ActionClientGetter: mockAcg,
215-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
213+
ActionClientGetter: mockAcg,
214+
HelmChartLoader: noOpHelmChartLoader(),
216215
}
217216

218217
objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
@@ -230,8 +229,8 @@ func TestApply_Installation(t *testing.T) {
230229
dryRunInstallErr: errors.New("failed attempting to dry-run install chart"),
231230
}
232231
helmApplier := applier.Helm{
233-
ActionClientGetter: mockAcg,
234-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
232+
ActionClientGetter: mockAcg,
233+
HelmChartLoader: noOpHelmChartLoader(),
235234
}
236235

237236
objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
@@ -248,9 +247,9 @@ func TestApply_Installation(t *testing.T) {
248247
}
249248
mockPf := &mockPreflight{installErr: errors.New("failed during install pre-flight check")}
250249
helmApplier := applier.Helm{
251-
ActionClientGetter: mockAcg,
252-
Preflights: []applier.Preflight{mockPf},
253-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
250+
ActionClientGetter: mockAcg,
251+
Preflights: []applier.Preflight{mockPf},
252+
HelmChartLoader: noOpHelmChartLoader(),
254253
}
255254

256255
objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
@@ -266,8 +265,8 @@ func TestApply_Installation(t *testing.T) {
266265
installErr: errors.New("failed installing chart"),
267266
}
268267
helmApplier := applier.Helm{
269-
ActionClientGetter: mockAcg,
270-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
268+
ActionClientGetter: mockAcg,
269+
HelmChartLoader: noOpHelmChartLoader(),
271270
}
272271

273272
objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
@@ -286,8 +285,8 @@ func TestApply_Installation(t *testing.T) {
286285
},
287286
}
288287
helmApplier := applier.Helm{
289-
ActionClientGetter: mockAcg,
290-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
288+
ActionClientGetter: mockAcg,
289+
HelmChartLoader: noOpHelmChartLoader(),
291290
}
292291

293292
objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
@@ -306,8 +305,8 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) {
306305
dryRunInstallErr: errors.New("failed attempting to dry-run install chart"),
307306
}
308307
helmApplier := applier.Helm{
309-
ActionClientGetter: mockAcg,
310-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
308+
ActionClientGetter: mockAcg,
309+
HelmChartLoader: noOpHelmChartLoader(),
311310
}
312311

313312
objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
@@ -328,10 +327,10 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) {
328327
}
329328
mockPf := &mockPreflight{installErr: errors.New("failed during install pre-flight check")}
330329
helmApplier := applier.Helm{
331-
ActionClientGetter: mockAcg,
332-
Preflights: []applier.Preflight{mockPf},
333-
PreAuthorizer: &mockPreAuthorizer{nil, nil},
334-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
330+
ActionClientGetter: mockAcg,
331+
Preflights: []applier.Preflight{mockPf},
332+
PreAuthorizer: &mockPreAuthorizer{nil, nil},
333+
HelmChartLoader: noOpHelmChartLoader(),
335334
}
336335

337336
objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
@@ -350,9 +349,9 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) {
350349
},
351350
}
352351
helmApplier := applier.Helm{
353-
ActionClientGetter: mockAcg,
354-
PreAuthorizer: &mockPreAuthorizer{nil, errPreAuth},
355-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
352+
ActionClientGetter: mockAcg,
353+
PreAuthorizer: &mockPreAuthorizer{nil, errPreAuth},
354+
HelmChartLoader: noOpHelmChartLoader(),
356355
}
357356
// Use a ClusterExtension with valid Spec fields.
358357
validCE := &ocv1.ClusterExtension{
@@ -379,9 +378,9 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) {
379378
},
380379
}
381380
helmApplier := applier.Helm{
382-
ActionClientGetter: mockAcg,
383-
PreAuthorizer: &mockPreAuthorizer{missingRBAC, nil},
384-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
381+
ActionClientGetter: mockAcg,
382+
PreAuthorizer: &mockPreAuthorizer{missingRBAC, nil},
383+
HelmChartLoader: noOpHelmChartLoader(),
385384
}
386385
// Use a ClusterExtension with valid Spec fields.
387386
validCE := &ocv1.ClusterExtension{
@@ -408,9 +407,9 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) {
408407
},
409408
}
410409
helmApplier := applier.Helm{
411-
ActionClientGetter: mockAcg,
412-
PreAuthorizer: &mockPreAuthorizer{nil, nil},
413-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
410+
ActionClientGetter: mockAcg,
411+
PreAuthorizer: &mockPreAuthorizer{nil, nil},
412+
HelmChartLoader: noOpHelmChartLoader(),
414413
}
415414

416415
// Use a ClusterExtension with valid Spec fields.
@@ -442,8 +441,8 @@ func TestApply_Upgrade(t *testing.T) {
442441
dryRunUpgradeErr: errors.New("failed attempting to dry-run upgrade chart"),
443442
}
444443
helmApplier := applier.Helm{
445-
ActionClientGetter: mockAcg,
446-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
444+
ActionClientGetter: mockAcg,
445+
HelmChartLoader: noOpHelmChartLoader(),
447446
}
448447

449448
objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
@@ -464,9 +463,9 @@ func TestApply_Upgrade(t *testing.T) {
464463
}
465464
mockPf := &mockPreflight{upgradeErr: errors.New("failed during upgrade pre-flight check")}
466465
helmApplier := applier.Helm{
467-
ActionClientGetter: mockAcg,
468-
Preflights: []applier.Preflight{mockPf},
469-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
466+
ActionClientGetter: mockAcg,
467+
Preflights: []applier.Preflight{mockPf},
468+
HelmChartLoader: noOpHelmChartLoader(),
470469
}
471470

472471
objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
@@ -488,7 +487,7 @@ func TestApply_Upgrade(t *testing.T) {
488487
mockPf := &mockPreflight{}
489488
helmApplier := applier.Helm{
490489
ActionClientGetter: mockAcg, Preflights: []applier.Preflight{mockPf},
491-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
490+
HelmChartLoader: noOpHelmChartLoader(),
492491
}
493492

494493
objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
@@ -509,9 +508,9 @@ func TestApply_Upgrade(t *testing.T) {
509508
}
510509
mockPf := &mockPreflight{}
511510
helmApplier := applier.Helm{
512-
ActionClientGetter: mockAcg,
513-
Preflights: []applier.Preflight{mockPf},
514-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
511+
ActionClientGetter: mockAcg,
512+
Preflights: []applier.Preflight{mockPf},
513+
HelmChartLoader: noOpHelmChartLoader(),
515514
}
516515

517516
objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
@@ -530,8 +529,8 @@ func TestApply_Upgrade(t *testing.T) {
530529
desiredRel: &testDesiredRelease,
531530
}
532531
helmApplier := applier.Helm{
533-
ActionClientGetter: mockAcg,
534-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
532+
ActionClientGetter: mockAcg,
533+
HelmChartLoader: noOpHelmChartLoader(),
535534
}
536535

537536
objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
@@ -556,8 +555,8 @@ func TestApply_InstallationWithSingleOwnNamespaceInstallSupportEnabled(t *testin
556555
Manifest: validManifest,
557556
},
558557
},
559-
BundleToHelmChartConverter: &fakeBundleToHelmChartConverter{
560-
fn: func(bundle source.BundleSource, installNamespace string, watchNamespace string) (*chart.Chart, error) {
558+
HelmChartLoader: fakeHelmChartLoader{
559+
fn: func(bundleFS fs.FS, installNamespace string, watchNamespace string) (*chart.Chart, error) {
561560
require.Equal(t, expectedWatchNamespace, watchNamespace)
562561
return nil, nil
563562
},
@@ -589,8 +588,8 @@ func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) {
589588
Manifest: validManifest,
590589
},
591590
},
592-
BundleToHelmChartConverter: &fakeBundleToHelmChartConverter{
593-
fn: func(bundle source.BundleSource, installNamespace string, watchNamespace string) (*chart.Chart, error) {
591+
HelmChartLoader: fakeHelmChartLoader{
592+
fn: func(bundleFS fs.FS, installNamespace string, watchNamespace string) (*chart.Chart, error) {
594593
require.Equal(t, expectedWatchNamespace, watchNamespace)
595594
return nil, nil
596595
},
@@ -609,8 +608,8 @@ func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) {
609608
Manifest: validManifest,
610609
},
611610
},
612-
BundleToHelmChartConverter: &fakeBundleToHelmChartConverter{
613-
fn: func(bundle source.BundleSource, installNamespace string, watchNamespace string) (*chart.Chart, error) {
611+
HelmChartLoader: fakeHelmChartLoader{
612+
fn: func(bundleFS fs.FS, installNamespace string, watchNamespace string) (*chart.Chart, error) {
614613
return nil, errors.New("some error")
615614
},
616615
},
@@ -621,10 +620,18 @@ func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) {
621620
})
622621
}
623622

624-
type fakeBundleToHelmChartConverter struct {
625-
fn func(source.BundleSource, string, string) (*chart.Chart, error)
623+
func noOpHelmChartLoader() applier.HelmChartLoader {
624+
return fakeHelmChartLoader{
625+
fn: func(bundleFS fs.FS, _ string, _ string) (*chart.Chart, error) {
626+
return nil, nil
627+
},
628+
}
629+
}
630+
631+
type fakeHelmChartLoader struct {
632+
fn func(fs.FS, string, string) (*chart.Chart, error)
626633
}
627634

628-
func (f fakeBundleToHelmChartConverter) ToHelmChart(bundle source.BundleSource, installNamespace string, watchNamespace string) (*chart.Chart, error) {
629-
return f.fn(bundle, installNamespace, watchNamespace)
635+
func (f fakeHelmChartLoader) Load(bundleFS fs.FS, installNamespace string, watchNamespace string) (*chart.Chart, error) {
636+
return f.fn(bundleFS, installNamespace, watchNamespace)
630637
}

0 commit comments

Comments
 (0)