diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index d426793d4..9e088185f 100644 --- a/cmd/operator-controller/main.go +++ b/cmd/operator-controller/main.go @@ -439,14 +439,26 @@ func run() error { isWebhookSupportEnabled = true } + // create and configure chart loaders + bundleLoaders := map[applier.BundleType]applier.HelmChartLoader{ + applier.BundleTypeRegistryV1: &applier.RegistryV1BundleLoader{ + RegistryV1BundleToHelmChartConverter: &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, } diff --git a/go.mod b/go.mod index 80092f371..e208b07b9 100644 --- a/go.mod +++ b/go.mod @@ -30,6 +30,7 @@ require ( golang.org/x/sync v0.16.0 golang.org/x/tools v0.34.0 gopkg.in/yaml.v2 v2.4.0 + gopkg.in/yaml.v3 v3.0.1 helm.sh/helm/v3 v3.18.4 k8s.io/api v0.33.2 k8s.io/apiextensions-apiserver v0.33.2 @@ -243,7 +244,6 @@ require ( gopkg.in/evanphx/json-patch.v4 v4.12.0 // indirect gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/warnings.v0 v0.1.2 // indirect - gopkg.in/yaml.v3 v3.0.1 // indirect k8s.io/controller-manager v0.33.2 // indirect k8s.io/kubectl v0.33.2 // indirect oras.land/oras-go/v2 v2.6.0 // indirect diff --git a/internal/operator-controller/applier/helm.go b/internal/operator-controller/applier/helm.go index ecfb3fdc2..7a72175a5 100644 --- a/internal/operator-controller/applier/helm.go +++ b/internal/operator-controller/applier/helm.go @@ -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 ( @@ -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 @@ -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 } @@ -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) { diff --git a/internal/operator-controller/applier/helm_test.go b/internal/operator-controller/applier/helm_test.go index 89c94df88..d345f1927 100644 --- a/internal/operator-controller/applier/helm_test.go +++ b/internal/operator-controller/applier/helm_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "io" + "io/fs" "os" "testing" "testing/fstest" @@ -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 { @@ -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) @@ -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) @@ -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) @@ -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) @@ -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) @@ -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) @@ -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) @@ -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) @@ -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{ @@ -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{ @@ -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. @@ -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) @@ -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) @@ -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) @@ -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) @@ -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) @@ -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 }, @@ -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 }, @@ -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") }, }, @@ -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) } diff --git a/internal/operator-controller/applier/loader.go b/internal/operator-controller/applier/loader.go new file mode 100644 index 000000000..40c6d23f6 --- /dev/null +++ b/internal/operator-controller/applier/loader.go @@ -0,0 +1,59 @@ +package applier + +import ( + "fmt" + "io/fs" + + "helm.sh/helm/v3/pkg/chart" + + "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source" + imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image" +) + +const ( + BundleTypeRegistryV1 BundleType = "registry-v1" + BundleTypeHelm BundleType = "helm" +) + +type BundleType string + +type HelmChartLoaderMap map[BundleType]HelmChartLoader + +type BundleFSChartLoader struct { + HelmChartLoaders HelmChartLoaderMap +} + +func (b BundleFSChartLoader) Load(bundleFS fs.FS, installNamespace string, watchNamespace string) (*chart.Chart, error) { + bundleType := b.determineBundleType(bundleFS) + if loader, ok := b.HelmChartLoaders[bundleType]; ok { + return loader.Load(bundleFS, installNamespace, watchNamespace) + } + return nil, fmt.Errorf("unsupported bundle type: %s", bundleType) +} + +// determineBundleType is a stand-in method to helm determine the bundle type until +// the bundle metadata can be given together with the FS to the Load method +func (b BundleFSChartLoader) determineBundleType(bundleFS fs.FS) BundleType { + if imageutil.IsBundleSourceChart(bundleFS) { + return BundleTypeHelm + } + return BundleTypeRegistryV1 +} + +type HelmBundleLoader struct{} + +func (h HelmBundleLoader) Load(bundleFS fs.FS, installNamespace string, _ string) (*chart.Chart, error) { + return imageutil.LoadChartFSWithOptions(bundleFS, imageutil.WithInstallNamespace(installNamespace)) +} + +type RegistryV1BundleToHelmChartConverter interface { + ToHelmChart(bundleSource source.BundleSource, installNamespace string, watchNamespace string) (*chart.Chart, error) +} + +type RegistryV1BundleLoader struct { + RegistryV1BundleToHelmChartConverter +} + +func (r RegistryV1BundleLoader) Load(bundleFS fs.FS, installNamespace string, watchNamespace string) (*chart.Chart, error) { + return r.ToHelmChart(source.FromFS(bundleFS), installNamespace, watchNamespace) +} diff --git a/internal/operator-controller/applier/loader_test.go b/internal/operator-controller/applier/loader_test.go new file mode 100644 index 000000000..2a8e8301a --- /dev/null +++ b/internal/operator-controller/applier/loader_test.go @@ -0,0 +1,239 @@ +package applier_test + +import ( + "archive/tar" + "bytes" + "compress/gzip" + "fmt" + "io/fs" + "strings" + "testing" + "testing/fstest" + + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" + "helm.sh/helm/v3/pkg/chart" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "github.com/operator-framework/operator-controller/internal/operator-controller/applier" + "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source" +) + +func Test_BundleFSChartLoader_UnsupportedBundleType(t *testing.T) { + t.Log("Check BundleFSChartLoader surfaces unsupported bundle type errors") + + t.Log("By attempting to load a chart with no helm chart loaders configured") + chartLoader := applier.BundleFSChartLoader{ + HelmChartLoaders: nil, + } + + _, err := chartLoader.Load(fstest.MapFS{}, "installNamespace", "watchNamespace") + require.Error(t, err) + require.Contains(t, err.Error(), "unsupported bundle type") +} + +func Test_BundleFSChartLoader_Fails(t *testing.T) { + t.Log("Check BundleFSChartLoader surfaces chart loading errors") + chartLoader := applier.BundleFSChartLoader{ + HelmChartLoaders: map[applier.BundleType]applier.HelmChartLoader{ + applier.BundleTypeRegistryV1: fakeHelmChartLoader{func(fs.FS, string, string) (*chart.Chart, error) { + return nil, fmt.Errorf("some error") + }}, + }, + } + _, err := chartLoader.Load(fstest.MapFS{}, "installNamespace", "watchNamespace") + require.Error(t, err) + require.Contains(t, err.Error(), "some error") +} + +func Test_BundleFSChartLoader_Succeeds(t *testing.T) { + t.Log("Check BundleFSChartLoader succeeds") + + t.Log("By creating a BundleFSChartLoader that supports both registry+v1 and helm bundles") + chartLoader := applier.BundleFSChartLoader{ + HelmChartLoaders: applier.HelmChartLoaderMap{ + applier.BundleTypeHelm: fakeHelmChartLoader{func(fs.FS, string, string) (*chart.Chart, error) { + return &chart.Chart{ + Metadata: &chart.Metadata{ + Name: "helm-chart-bundle-chart", + }, + }, nil + }}, + applier.BundleTypeRegistryV1: fakeHelmChartLoader{func(fs.FS, string, string) (*chart.Chart, error) { + return &chart.Chart{ + Metadata: &chart.Metadata{ + Name: "registry-v1-bundle-chart", + }, + }, nil + }}, + }, + } + + t.Log("Ensuring registry+v1 bundles are handled by the registry+v1 helm chart loader") + c, err := chartLoader.Load(newRegistryV1BundleFS(), "installNamespace", "watchNamespace") + require.NoError(t, err) + require.NotNil(t, c) + require.Equal(t, "registry-v1-bundle-chart", c.Metadata.Name) + + t.Log("Ensuring helm bundles are handled by the helm bundle loader") + c, err = chartLoader.Load(fstest.MapFS{ + "some-helm-chart-archive.tgz": &fstest.MapFile{ + Data: []byte(""), + }, + }, "installNamespace", "watchNamespace") + require.NoError(t, err) + require.NotNil(t, c) + require.Equal(t, "helm-chart-bundle-chart", c.Metadata.Name) +} + +func Test_RegistryV1BundleLoader_CallsConverter_Success(t *testing.T) { + t.Log("Testing integration between RegistryV1BundleLoader and RegistryV1BundleToHelmChartConverter") + chartLoader := applier.RegistryV1BundleLoader{ + RegistryV1BundleToHelmChartConverter: fakeBundleToHelmChartConverter(func(bundleSource source.BundleSource, installNamespace string, watchNamespace string) (*chart.Chart, error) { + t.Log("By checking the correct parameter values are passed in") + require.Equal(t, "installNamespace", installNamespace) + require.Equal(t, "watchNamespace", watchNamespace) + + t.Log("By checking the correct bundle fs is passed in") + b, err := bundleSource.GetBundle() + require.NoError(t, err) + require.Equal(t, "test", b.PackageName) + return &chart.Chart{ + Metadata: &chart.Metadata{ + Name: "test-chart", + }, + }, nil + }), + } + c, err := chartLoader.Load(newRegistryV1BundleFS(), "installNamespace", "watchNamespace") + t.Log("By checking the any errors in the loading process are surfaced") + require.NoError(t, err) + require.Equal(t, &chart.Chart{ + Metadata: &chart.Metadata{ + Name: "test-chart", + }, + }, c) +} + +func Test_RegistryV1BundleLoader_CallsConverter_Failure(t *testing.T) { + t.Log("Testing integration between RegistryV1BundleLoader and RegistryV1BundleToHelmChartConverter") + chartLoader := applier.RegistryV1BundleLoader{ + RegistryV1BundleToHelmChartConverter: fakeBundleToHelmChartConverter(func(bundleSource source.BundleSource, installNamespace string, watchNamespace string) (*chart.Chart, error) { + return nil, fmt.Errorf("test error") + }), + } + c, err := chartLoader.Load(fstest.MapFS{}, "installNamespace", "watchNamespace") + t.Log("By checking the any errors in the loading process are surfaced") + require.Error(t, err) + require.Nil(t, c) + require.Equal(t, "test error", err.Error()) +} + +func Test_HelmBundleLoader_Success(t *testing.T) { + t.Log("Test HelmBundleLoader can load a chart from a helm bundle") + helmArchive := newHelmChartArchive(t, map[string][]byte{ + "testchart/Chart.yaml": []byte("apiVersion: v2\nname: test-chart\nversion: 0.1.0"), + "testchart/templates/deployment.yaml": []byte("kind: Deployment\napiVersion: apps/v1\nmetadata:\n name: test-chart\n namespace: {{ .Release.Namespace }}"), + }) + bundleFS := fstest.MapFS{ + "test-chart.v0.1.0.tgz": &fstest.MapFile{ + Mode: 0600, + Data: helmArchive, + }, + } + + charLoader := applier.HelmBundleLoader{} + c, err := charLoader.Load(bundleFS, "installNamespace", "watchNamespace") + t.Log("By checking the chart loader succeeds") + require.NoError(t, err) + require.NotNil(t, c) + t.Log("By checking the chart metadata is correct") + require.Equal(t, &chart.Metadata{ + Name: "test-chart", + Version: "0.1.0", + APIVersion: "v2", + }, c.Metadata) + + t.Log("By checking the number of templates") + require.Len(t, c.Templates, 1) + + t.Log("By checking the correct install namespace is being used") + obj := map[string]interface{}{} + require.NoError(t, yaml.Unmarshal(c.Templates[0].Data, obj)) + require.Equal(t, "installNamespace", (&unstructured.Unstructured{Object: obj}).GetNamespace()) +} + +func Test_HelmBundleLoader_BadHelmChartBundle_Fails(t *testing.T) { + t.Log("Test HelmBundleLoader surfaces issues loading bad helm chart bundles") + helmArchive := newHelmChartArchive(t, map[string][]byte{}) + bundleFS := fstest.MapFS{ + "test-chart.v0.1.0.tgz": &fstest.MapFile{ + Mode: 0000, + Data: helmArchive, + }, + } + charLoader := applier.HelmBundleLoader{} + + t.Log("By attempting to load an empty helm bundle") + c, err := charLoader.Load(bundleFS, "installNamespace", "watchNamespace") + t.Log("and checking no chart is returned") + require.Nil(t, c) + t.Log("and checking an error is returned") + require.Error(t, err) +} + +type fakeBundleToHelmChartConverter func(source.BundleSource, string, string) (*chart.Chart, error) + +func (f fakeBundleToHelmChartConverter) ToHelmChart(bundleSource source.BundleSource, installNamespace string, watchNamespace string) (*chart.Chart, error) { + return f(bundleSource, installNamespace, watchNamespace) +} + +func newHelmChartArchive(t *testing.T, fsMap map[string][]byte) []byte { + var buf bytes.Buffer + tw := tar.NewWriter(&buf) + + // Add files to the chart archive + for name, content := range fsMap { + require.NoError(t, tw.WriteHeader(&tar.Header{ + Name: name, + Mode: 0600, + Size: int64(len(content)), + })) + _, _ = tw.Write(content) + } + + require.NoError(t, tw.Close()) + + var gzBuf bytes.Buffer + gz := gzip.NewWriter(&gzBuf) + _, err := gz.Write(buf.Bytes()) + require.NoError(t, err) + require.NoError(t, gz.Close()) + + return gzBuf.Bytes() +} + +func newRegistryV1BundleFS() fstest.MapFS { + annotationsYml := ` +annotations: + operators.operatorframework.io.bundle.mediatype.v1: registry+v1 + operators.operatorframework.io.bundle.package.v1: test +` + csvYml := ` +apiVersion: operators.operatorframework.io/v1alpha1 +kind: ClusterServiceVersion +metadata: + name: test.v1.0.0 + annotations: + olm.properties: '[{"type":"from-csv-annotations-key", "value":"from-csv-annotations-value"}]' +spec: + installModes: + - type: AllNamespaces + supported: true +` + + return fstest.MapFS{ + "metadata/annotations.yaml": &fstest.MapFile{Data: []byte(strings.Trim(annotationsYml, "\n"))}, + "manifests/csv.yaml": &fstest.MapFile{Data: []byte(strings.Trim(csvYml, "\n"))}, + } +} diff --git a/internal/shared/util/image/helm.go b/internal/shared/util/image/helm.go index c00d4000b..1438b27bb 100644 --- a/internal/shared/util/image/helm.go +++ b/internal/shared/util/image/helm.go @@ -103,28 +103,10 @@ func inspectChart(data []byte, metadata *chart.Metadata) (chartInspectionResult, return report, nil } -func IsBundleSourceChart(bundleFS fs.FS, metadata *chart.Metadata) (bool, error) { - var chartPath string - files, _ := fs.ReadDir(bundleFS, ".") - for _, file := range files { - if strings.HasSuffix(file.Name(), ".tgz") || - strings.HasSuffix(file.Name(), ".tar.gz") { - chartPath = file.Name() - break - } - } - - chartData, err := fs.ReadFile(bundleFS, chartPath) - if err != nil { - return false, err - } - - result, err := inspectChart(chartData, metadata) - if err != nil { - return false, fmt.Errorf("reading %s from fs: %w", chartPath, err) - } - - return (result.templatesExist && result.chartfileExists), nil +// IsBundleSourceChart is a stand-in method until metadata can accompany the bundle filesystem +// and provide a bundle type. Any bundle file system with a tar archive will be considered a helm bundle. +func IsBundleSourceChart(bundleFS fs.FS) bool { + return findChartArchive(bundleFS) != "" } type ChartOption func(*chart.Chart) @@ -139,8 +121,12 @@ func WithInstallNamespace(namespace string) ChartOption { } } -func LoadChartFSWithOptions(bundleFS fs.FS, filename string, options ...ChartOption) (*chart.Chart, error) { - ch, err := loadChartFS(bundleFS, filename) +func LoadChartFSWithOptions(bundleFS fs.FS, options ...ChartOption) (*chart.Chart, error) { + chartPath := findChartArchive(bundleFS) + if chartPath == "" { + return nil, errors.New("unsupported filesystem: no chart archive found") + } + ch, err := loadChartFS(bundleFS, chartPath) if err != nil { return nil, err } @@ -167,9 +153,22 @@ func loadChartFS(bundleFS fs.FS, filename string) (*chart.Chart, error) { return nil, fmt.Errorf("chart file name was not provided") } - tarball, err := fs.ReadFile(bundleFS, filename) + f, err := bundleFS.Open(filename) if err != nil { return nil, fmt.Errorf("reading chart %s; %+v", filename, err) } - return loader.LoadArchive(bytes.NewBuffer(tarball)) + return loader.LoadArchive(f) +} + +func findChartArchive(bundleFS fs.FS) string { + var chartPath string + files, _ := fs.ReadDir(bundleFS, ".") + for _, file := range files { + if strings.HasSuffix(file.Name(), ".tgz") || + strings.HasSuffix(file.Name(), ".tar.gz") { + chartPath = file.Name() + break + } + } + return chartPath } diff --git a/internal/shared/util/image/helm_test.go b/internal/shared/util/image/helm_test.go index d7fa6d3de..3b1634f5a 100644 --- a/internal/shared/util/image/helm_test.go +++ b/internal/shared/util/image/helm_test.go @@ -254,8 +254,7 @@ func TestIsBundleSourceChart(t *testing.T) { files []fileContent } type want struct { - value bool - errStr string + value bool } tt := []struct { name string @@ -312,7 +311,7 @@ func TestIsBundleSourceChart(t *testing.T) { }, }, want: want{ - value: false, + value: true, }, }, { @@ -327,8 +326,7 @@ func TestIsBundleSourceChart(t *testing.T) { }, }, want: want{ - value: false, - errStr: "reading testchart-0.1.0.tgz from fs: loading chart archive: Chart.yaml file is missing", + value: true, }, }, { @@ -343,8 +341,7 @@ func TestIsBundleSourceChart(t *testing.T) { }, }, want: want{ - value: false, - errStr: "reading testchart-0.1.0.tgz from fs: loading chart archive: Chart.yaml file is missing", + value: true, }, }, } @@ -352,11 +349,7 @@ func TestIsBundleSourceChart(t *testing.T) { for _, tc := range tt { t.Run(tc.name, func(t *testing.T) { chartFS, _ := createTempFS(t, mockHelmChartTgz(t, tc.args.files)) - got, err := IsBundleSourceChart(chartFS, tc.args.meta) - if tc.want.errStr != "" { - require.Error(t, err, "chart validation error required") - require.EqualError(t, err, tc.want.errStr, "chart error") - } + got := IsBundleSourceChart(chartFS) require.Equal(t, tc.want.value, got, "validata helm chart") }) } @@ -466,8 +459,7 @@ func Test_loadChartFS(t *testing.T) { func TestLoadChartFSWithOptions(t *testing.T) { type args struct { - filename string - files []fileContent + files []fileContent } type want struct { name string @@ -481,31 +473,18 @@ func TestLoadChartFSWithOptions(t *testing.T) { expect func(*chart.Chart, want, error) }{ { - name: "empty filename is provided", - args: args{ - filename: "", - files: []fileContent{ - { - name: "testchart/Chart.yaml", - content: []byte("apiVersion: v2\nname: testchart\nversion: 0.1.0"), - }, - { - name: "testchart/templates/deployment.yaml", - content: []byte("kind: Deployment\napiVersion: apps/v1"), - }, - }, - }, + name: "no chart archive in filesystem", + args: args{}, want: want{ - errMsg: "chart file name was not provided", + errMsg: "no chart archive found", }, expect: func(chart *chart.Chart, want want, err error) { - require.Error(t, err, want.errMsg) + require.Contains(t, err.Error(), want.errMsg) }, }, { name: "load sample chart", args: args{ - filename: "testchart-0.1.0.tgz", files: []fileContent{ { name: "testchart/Chart.yaml", @@ -527,35 +506,12 @@ func TestLoadChartFSWithOptions(t *testing.T) { assert.Equal(t, want.version, chart.Metadata.Version, "chart version") }, }, - { - name: "load nonexistent chart", - args: args{ - filename: "nonexistent-chart-0.1.0.tgz", - files: []fileContent{ - { - name: "testchart/Chart.yaml", - content: []byte("apiVersion: v2\nname: testchart\nversion: 0.1.0"), - }, - { - name: "testchart/templates/deployment.yaml", - content: []byte("kind: Deployment\napiVersion: apps/v1"), - }, - }, - }, - want: want{ - errMsg: "reading chart nonexistent-chart-0.1.0.tgz; open nonexistent-chart-0.1.0.tgz: no such file or directory", - }, - expect: func(chart *chart.Chart, want want, err error) { - require.Error(t, err, want.errMsg) - assert.Nil(t, chart) - }, - }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { chartFS, _ := createTempFS(t, mockHelmChartTgz(t, tc.args.files)) - got, err := LoadChartFSWithOptions(chartFS, tc.args.filename, WithInstallNamespace("metrics-server-system")) + got, err := LoadChartFSWithOptions(chartFS, WithInstallNamespace("metrics-server-system")) require.NotNil(t, tc.expect) tc.expect(got, tc.want, err) }) @@ -636,7 +592,9 @@ type fileContent struct { } func mockHelmChartTgz(t *testing.T, contents []fileContent) []byte { - require.NotEmpty(t, contents, "chart content required") + if len(contents) == 0 { + return nil + } var buf bytes.Buffer tw := tar.NewWriter(&buf) @@ -662,7 +620,6 @@ func mockHelmChartTgz(t *testing.T, contents []fileContent) []byte { } func createTempFS(t *testing.T, data []byte) (fs.FS, error) { - require.NotEmpty(t, data, "chart data") tmpDir, _ := os.MkdirTemp(t.TempDir(), "bundlefs-") if len(data) == 0 { return os.DirFS(tmpDir), nil