Skip to content

🌱 Refactor applier to use HelmChartLoader #2089

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+198 −147
Draft

🌱 Refactor applier to use HelmChartLoader #2089

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
20 changes: 16 additions & 4 deletions cmd/operator-controller/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,14 +439,26 @@ func run() error {
isWebhookSupportEnabled = true
}

// create and configure chart loaders
bundleLoaders := map[applier.BundleType]applier.HelmChartLoader{
applier.BundleTypeRegistryV1: &applier.RegistryV1BundleLoader{
BundleToHelmChartConverter: convert.BundleToHelmChartConverter{
BundleRenderer: registryv1.Renderer,
CertificateProvider: certProvider,
IsWebhookSupportEnabled: isWebhookSupportEnabled,
},
},
}
if features.OperatorControllerFeatureGate.Enabled(features.HelmChartSupport) {
bundleLoaders[applier.BundleTypeHelm] = &applier.HelmBundleLoader{}
}

// now initialize the helmApplier, assigning the potentially nil preAuth
helmApplier := &applier.Helm{
ActionClientGetter: acg,
Preflights: preflights,
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{
BundleRenderer: registryv1.Renderer,
CertificateProvider: certProvider,
IsWebhookSupportEnabled: isWebhookSupportEnabled,
HelmChartLoader: applier.BundleFSChartLoader{
HelmChartLoaders: bundleLoaders,
},
PreAuthorizer: preAuth,
}
Expand Down
40 changes: 15 additions & 25 deletions internal/operator-controller/applier/helm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,8 @@ import (

ocv1 "github.com/operator-framework/operator-controller/api/v1"
"github.com/operator-framework/operator-controller/internal/operator-controller/authorization"
"github.com/operator-framework/operator-controller/internal/operator-controller/features"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/preflights/crdupgradesafety"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util"
imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image"
)

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

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

type Helm struct {
ActionClientGetter helmclient.ActionClientGetter
Preflights []Preflight
PreAuthorizer authorization.PreAuthorizer
BundleToHelmChartConverter BundleToHelmChartConverter
ActionClientGetter helmclient.ActionClientGetter
Preflights []Preflight
PreAuthorizer authorization.PreAuthorizer
HelmChartLoader HelmChartLoader
}

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

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) {
chrt, err := h.buildHelmChart(contentFS, ext)
chrt, err := h.loadHelmChart(contentFS, ext)
if err != nil {
return nil, "", err
}
Expand Down Expand Up @@ -203,26 +204,15 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
return relObjects, state, nil
}

func (h *Helm) buildHelmChart(bundleFS fs.FS, ext *ocv1.ClusterExtension) (*chart.Chart, error) {
if h.BundleToHelmChartConverter == nil {
return nil, errors.New("BundleToHelmChartConverter is nil")
func (h *Helm) loadHelmChart(bundleFS fs.FS, ext *ocv1.ClusterExtension) (*chart.Chart, error) {
if h.HelmChartLoader == nil {
return nil, fmt.Errorf("HelmChartLoader is not set")
}
watchNamespace, err := GetWatchNamespace(ext)
if err != nil {
return nil, err
}
if features.OperatorControllerFeatureGate.Enabled(features.HelmChartSupport) {
meta := new(chart.Metadata)
if ok, _ := imageutil.IsBundleSourceChart(bundleFS, meta); ok {
return imageutil.LoadChartFSWithOptions(
bundleFS,
fmt.Sprintf("%s-%s.tgz", meta.Name, meta.Version),
imageutil.WithInstallNamespace(ext.Spec.Namespace),
)
}
}

return h.BundleToHelmChartConverter.ToHelmChart(source.FromFS(bundleFS), ext.Spec.Namespace, watchNamespace)
return h.HelmChartLoader.Load(bundleFS, ext.Spec.Namespace, watchNamespace)
}

func (h *Helm) renderClientOnlyRelease(ctx context.Context, ext *ocv1.ClusterExtension, chrt *chart.Chart, values chartutil.Values, post postrender.PostRenderer) (*release.Release, error) {
Expand Down
109 changes: 58 additions & 51 deletions internal/operator-controller/applier/helm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"errors"
"io"
"io/fs"
"os"
"testing"
"testing/fstest"
Expand All @@ -26,8 +27,6 @@ import (
"github.com/operator-framework/operator-controller/internal/operator-controller/applier"
"github.com/operator-framework/operator-controller/internal/operator-controller/authorization"
"github.com/operator-framework/operator-controller/internal/operator-controller/features"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/convert"
)

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

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

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

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

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

objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
Expand All @@ -286,8 +285,8 @@ func TestApply_Installation(t *testing.T) {
},
}
helmApplier := applier.Helm{
ActionClientGetter: mockAcg,
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
ActionClientGetter: mockAcg,
HelmChartLoader: noOpHelmChartLoader(),
}

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

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

objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
Expand All @@ -350,9 +349,9 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) {
},
}
helmApplier := applier.Helm{
ActionClientGetter: mockAcg,
PreAuthorizer: &mockPreAuthorizer{nil, errPreAuth},
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
ActionClientGetter: mockAcg,
PreAuthorizer: &mockPreAuthorizer{nil, errPreAuth},
HelmChartLoader: noOpHelmChartLoader(),
}
// Use a ClusterExtension with valid Spec fields.
validCE := &ocv1.ClusterExtension{
Expand All @@ -379,9 +378,9 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) {
},
}
helmApplier := applier.Helm{
ActionClientGetter: mockAcg,
PreAuthorizer: &mockPreAuthorizer{missingRBAC, nil},
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
ActionClientGetter: mockAcg,
PreAuthorizer: &mockPreAuthorizer{missingRBAC, nil},
HelmChartLoader: noOpHelmChartLoader(),
}
// Use a ClusterExtension with valid Spec fields.
validCE := &ocv1.ClusterExtension{
Expand All @@ -408,9 +407,9 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) {
},
}
helmApplier := applier.Helm{
ActionClientGetter: mockAcg,
PreAuthorizer: &mockPreAuthorizer{nil, nil},
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
ActionClientGetter: mockAcg,
PreAuthorizer: &mockPreAuthorizer{nil, nil},
HelmChartLoader: noOpHelmChartLoader(),
}

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

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

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

objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
Expand All @@ -509,9 +508,9 @@ func TestApply_Upgrade(t *testing.T) {
}
mockPf := &mockPreflight{}
helmApplier := applier.Helm{
ActionClientGetter: mockAcg,
Preflights: []applier.Preflight{mockPf},
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
ActionClientGetter: mockAcg,
Preflights: []applier.Preflight{mockPf},
HelmChartLoader: noOpHelmChartLoader(),
}

objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
Expand All @@ -530,8 +529,8 @@ func TestApply_Upgrade(t *testing.T) {
desiredRel: &testDesiredRelease,
}
helmApplier := applier.Helm{
ActionClientGetter: mockAcg,
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
ActionClientGetter: mockAcg,
HelmChartLoader: noOpHelmChartLoader(),
}

objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
Expand All @@ -556,8 +555,8 @@ func TestApply_InstallationWithSingleOwnNamespaceInstallSupportEnabled(t *testin
Manifest: validManifest,
},
},
BundleToHelmChartConverter: &fakeBundleToHelmChartConverter{
fn: func(bundle source.BundleSource, installNamespace string, watchNamespace string) (*chart.Chart, error) {
HelmChartLoader: fakeHelmChartLoader{
fn: func(bundleFS fs.FS, installNamespace string, watchNamespace string) (*chart.Chart, error) {
require.Equal(t, expectedWatchNamespace, watchNamespace)
return nil, nil
},
Expand Down Expand Up @@ -589,8 +588,8 @@ func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) {
Manifest: validManifest,
},
},
BundleToHelmChartConverter: &fakeBundleToHelmChartConverter{
fn: func(bundle source.BundleSource, installNamespace string, watchNamespace string) (*chart.Chart, error) {
HelmChartLoader: fakeHelmChartLoader{
fn: func(bundleFS fs.FS, installNamespace string, watchNamespace string) (*chart.Chart, error) {
require.Equal(t, expectedWatchNamespace, watchNamespace)
return nil, nil
},
Expand All @@ -609,8 +608,8 @@ func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) {
Manifest: validManifest,
},
},
BundleToHelmChartConverter: &fakeBundleToHelmChartConverter{
fn: func(bundle source.BundleSource, installNamespace string, watchNamespace string) (*chart.Chart, error) {
HelmChartLoader: fakeHelmChartLoader{
fn: func(bundleFS fs.FS, installNamespace string, watchNamespace string) (*chart.Chart, error) {
return nil, errors.New("some error")
},
},
Expand All @@ -621,10 +620,18 @@ func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) {
})
}

type fakeBundleToHelmChartConverter struct {
fn func(source.BundleSource, string, string) (*chart.Chart, error)
func noOpHelmChartLoader() applier.HelmChartLoader {
return fakeHelmChartLoader{
fn: func(bundleFS fs.FS, _ string, _ string) (*chart.Chart, error) {
return nil, nil
},
}
}

type fakeHelmChartLoader struct {
fn func(fs.FS, string, string) (*chart.Chart, error)
}

func (f fakeBundleToHelmChartConverter) ToHelmChart(bundle source.BundleSource, installNamespace string, watchNamespace string) (*chart.Chart, error) {
return f.fn(bundle, installNamespace, watchNamespace)
func (f fakeHelmChartLoader) Load(bundleFS fs.FS, installNamespace string, watchNamespace string) (*chart.Chart, error) {
return f.fn(bundleFS, installNamespace, watchNamespace)
}
Loading
Loading