From 191bf91100cd2181927be77f4b9ba54953d74ae4 Mon Sep 17 00:00:00 2001 From: Per Goncalves da Silva Date: Wed, 10 Sep 2025 17:47:36 +0200 Subject: [PATCH 1/2] move helm converter to applier package Signed-off-by: Per Goncalves da Silva --- cmd/operator-controller/main.go | 3 +- .../operator-controller/applier/boxcutter.go | 3 +- internal/operator-controller/applier/helm.go | 22 +--- .../operator-controller/applier/helm_test.go | 121 +++++------------- .../convert/helm.go => applier/provider.go} | 21 +-- .../helm_test.go => applier/provider_test.go} | 98 +++++++++----- 6 files changed, 120 insertions(+), 148 deletions(-) rename internal/operator-controller/{rukpak/convert/helm.go => applier/provider.go} (80%) rename internal/operator-controller/{rukpak/convert/helm_test.go => applier/provider_test.go} (72%) diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index 2ba4bc9f0..f1e7142ba 100644 --- a/cmd/operator-controller/main.go +++ b/cmd/operator-controller/main.go @@ -71,7 +71,6 @@ import ( "github.com/operator-framework/operator-controller/internal/operator-controller/features" "github.com/operator-framework/operator-controller/internal/operator-controller/finalizers" "github.com/operator-framework/operator-controller/internal/operator-controller/resolve" - "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/convert" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/preflights/crdupgradesafety" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render/certproviders" @@ -655,7 +654,7 @@ func setupHelm( ceReconciler.Applier = &applier.Helm{ ActionClientGetter: acg, Preflights: preflights, - BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{ + HelmChartProvider: &applier.RegistryV1HelmChartProvider{ BundleRenderer: registryv1.Renderer, CertificateProvider: certProvider, IsWebhookSupportEnabled: certProvider != nil, diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index 74f5574ee..51b5b88b5 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -367,10 +367,11 @@ func latestRevisionNumber(prevRevisions []ocv1.ClusterExtensionRevision) int64 { return prevRevisions[len(prevRevisions)-1].Spec.Revision } +// TODO: in the next refactor iteration BundleRenderer and RegistryV1BundleRenderer into the RegistryV1ChartProvider + type BundleRenderer interface { Render(bundleFS fs.FS, ext *ocv1.ClusterExtension) ([]client.Object, error) } - type RegistryV1BundleRenderer struct { BundleRenderer render.BundleRenderer CertificateProvider render.CertificateProvider diff --git a/internal/operator-controller/applier/helm.go b/internal/operator-controller/applier/helm.go index e758f42da..29684bf53 100644 --- a/internal/operator-controller/applier/helm.go +++ b/internal/operator-controller/applier/helm.go @@ -29,14 +29,14 @@ import ( "github.com/operator-framework/operator-controller/internal/operator-controller/authorization" "github.com/operator-framework/operator-controller/internal/operator-controller/contentmanager" "github.com/operator-framework/operator-controller/internal/operator-controller/features" - "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util" imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image" ) -type BundleToHelmChartConverter interface { - ToHelmChart(bundle source.BundleSource, installNamespace string, config map[string]interface{}) (*chart.Chart, error) +// HelmChartProvider provides helm charts from bundle sources and cluster extensions +type HelmChartProvider interface { + HelmChart(bundle source.BundleSource, clusterExtension *ocv1.ClusterExtension) (*chart.Chart, error) } type HelmReleaseToObjectsConverter struct { @@ -62,7 +62,7 @@ type Helm struct { ActionClientGetter helmclient.ActionClientGetter Preflights []Preflight PreAuthorizer authorization.PreAuthorizer - BundleToHelmChartConverter BundleToHelmChartConverter + HelmChartProvider HelmChartProvider HelmReleaseToObjectsConverter HelmReleaseToObjectsConverterInterface Manager contentmanager.Manager @@ -199,12 +199,8 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte } func (h *Helm) buildHelmChart(bundleFS fs.FS, ext *ocv1.ClusterExtension) (*chart.Chart, error) { - if h.BundleToHelmChartConverter == nil { - return nil, errors.New("BundleToHelmChartConverter is nil") - } - watchNamespace, err := GetWatchNamespace(ext) - if err != nil { - return nil, err + if h.HelmChartProvider == nil { + return nil, errors.New("HelmChartProvider is nil") } if features.OperatorControllerFeatureGate.Enabled(features.HelmChartSupport) { meta := new(chart.Metadata) @@ -216,11 +212,7 @@ func (h *Helm) buildHelmChart(bundleFS fs.FS, ext *ocv1.ClusterExtension) (*char ) } } - - bundleConfig := map[string]interface{}{ - bundle.BundleConfigWatchNamespaceKey: watchNamespace, - } - return h.BundleToHelmChartConverter.ToHelmChart(source.FromFS(bundleFS), ext.Spec.Namespace, bundleConfig) + return h.HelmChartProvider.HelmChart(source.FromFS(bundleFS), ext) } 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 fdf5eead0..a4e217937 100644 --- a/internal/operator-controller/applier/helm_test.go +++ b/internal/operator-controller/applier/helm_test.go @@ -3,7 +3,6 @@ package applier_test import ( "context" "errors" - "fmt" "io" "os" "testing" @@ -14,11 +13,7 @@ import ( "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/release" "helm.sh/helm/v3/pkg/storage/driver" - corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - featuregatetesting "k8s.io/component-base/featuregate/testing" "sigs.k8s.io/controller-runtime/pkg/client" helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client" @@ -28,10 +23,7 @@ import ( "github.com/operator-framework/operator-controller/internal/operator-controller/authorization" "github.com/operator-framework/operator-controller/internal/operator-controller/contentmanager" cmcache "github.com/operator-framework/operator-controller/internal/operator-controller/contentmanager/cache" - "github.com/operator-framework/operator-controller/internal/operator-controller/features" - registryv1Bundle "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source" - "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/convert" ) var _ contentmanager.Manager = (*mockManagedContentCacheManager)(nil) @@ -246,8 +238,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, + HelmChartProvider: &applier.RegistryV1HelmChartProvider{}, } installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) @@ -260,8 +252,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, + HelmChartProvider: &applier.RegistryV1HelmChartProvider{}, } installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) @@ -279,8 +271,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, + HelmChartProvider: &applier.RegistryV1HelmChartProvider{}, } installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) @@ -299,7 +291,7 @@ func TestApply_Installation(t *testing.T) { helmApplier := applier.Helm{ ActionClientGetter: mockAcg, Preflights: []applier.Preflight{mockPf}, - BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{}, + HelmChartProvider: &applier.RegistryV1HelmChartProvider{}, HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{}, } @@ -317,7 +309,7 @@ func TestApply_Installation(t *testing.T) { } helmApplier := applier.Helm{ ActionClientGetter: mockAcg, - BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{}, + HelmChartProvider: &applier.RegistryV1HelmChartProvider{}, HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{}, } @@ -338,7 +330,7 @@ func TestApply_Installation(t *testing.T) { } helmApplier := applier.Helm{ ActionClientGetter: mockAcg, - BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{}, + HelmChartProvider: &applier.RegistryV1HelmChartProvider{}, HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{}, Manager: &mockManagedContentCacheManager{ cache: &mockManagedContentCache{}, @@ -359,8 +351,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, + HelmChartProvider: &applier.RegistryV1HelmChartProvider{}, } installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) @@ -384,7 +376,7 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { ActionClientGetter: mockAcg, Preflights: []applier.Preflight{mockPf}, PreAuthorizer: &mockPreAuthorizer{nil, nil}, - BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{}, + HelmChartProvider: &applier.RegistryV1HelmChartProvider{}, HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{}, } @@ -404,9 +396,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}, + HelmChartProvider: &applier.RegistryV1HelmChartProvider{}, } // Use a ClusterExtension with valid Spec fields. validCE := &ocv1.ClusterExtension{ @@ -433,9 +425,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}, + HelmChartProvider: &applier.RegistryV1HelmChartProvider{}, } // Use a ClusterExtension with valid Spec fields. validCE := &ocv1.ClusterExtension{ @@ -464,7 +456,7 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { helmApplier := applier.Helm{ ActionClientGetter: mockAcg, PreAuthorizer: &mockPreAuthorizer{nil, nil}, - BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{}, + HelmChartProvider: &applier.RegistryV1HelmChartProvider{}, HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{}, Manager: &mockManagedContentCacheManager{ cache: &mockManagedContentCache{}, @@ -498,8 +490,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, + HelmChartProvider: &applier.RegistryV1HelmChartProvider{}, } installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) @@ -522,7 +514,7 @@ func TestApply_Upgrade(t *testing.T) { helmApplier := applier.Helm{ ActionClientGetter: mockAcg, Preflights: []applier.Preflight{mockPf}, - BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{}, + HelmChartProvider: &applier.RegistryV1HelmChartProvider{}, HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{}, } @@ -545,7 +537,7 @@ func TestApply_Upgrade(t *testing.T) { mockPf := &mockPreflight{} helmApplier := applier.Helm{ ActionClientGetter: mockAcg, Preflights: []applier.Preflight{mockPf}, - BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{}, + HelmChartProvider: &applier.RegistryV1HelmChartProvider{}, HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{}, } @@ -569,7 +561,7 @@ func TestApply_Upgrade(t *testing.T) { helmApplier := applier.Helm{ ActionClientGetter: mockAcg, Preflights: []applier.Preflight{mockPf}, - BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{}, + HelmChartProvider: &applier.RegistryV1HelmChartProvider{}, HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{}, } @@ -590,7 +582,7 @@ func TestApply_Upgrade(t *testing.T) { } helmApplier := applier.Helm{ ActionClientGetter: mockAcg, - BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{}, + HelmChartProvider: &applier.RegistryV1HelmChartProvider{}, HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{}, Manager: &mockManagedContentCacheManager{ cache: &mockManagedContentCache{}, @@ -604,53 +596,8 @@ func TestApply_Upgrade(t *testing.T) { }) } -func TestApply_InstallationWithSingleOwnNamespaceInstallSupportEnabled(t *testing.T) { - featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.SingleOwnNamespaceInstallSupport, true) - t.Run("generates bundle resources using the configured watch namespace", func(t *testing.T) { - var expectedWatchNamespace = "watch-namespace" - - helmApplier := applier.Helm{ - ActionClientGetter: &mockActionGetter{ - getClientErr: driver.ErrReleaseNotFound, - desiredRel: &release.Release{ - Info: &release.Info{Status: release.StatusDeployed}, - Manifest: validManifest, - }, - }, - BundleToHelmChartConverter: &fakeBundleToHelmChartConverter{ - fn: func(bundle source.BundleSource, installNamespace string, config map[string]interface{}) (*chart.Chart, error) { - require.Equal(t, expectedWatchNamespace, config[registryv1Bundle.BundleConfigWatchNamespaceKey]) - return nil, nil - }, - }, - HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{}, - Manager: &mockManagedContentCacheManager{ - cache: &mockManagedContentCache{}, - }, - } - - testExt := &ocv1.ClusterExtension{ - ObjectMeta: metav1.ObjectMeta{ - Name: "testExt", - }, - Spec: ocv1.ClusterExtensionSpec{ - Config: &ocv1.ClusterExtensionConfig{ - ConfigType: ocv1.ClusterExtensionConfigTypeInline, - Inline: &apiextensionsv1.JSON{ - Raw: []byte(fmt.Sprintf(`{"%s":"%s"}`, registryv1Bundle.BundleConfigWatchNamespaceKey, expectedWatchNamespace)), - }, - }, - }, - } - - _, _, _ = helmApplier.Apply(context.TODO(), validFS, testExt, testObjectLabels, testStorageLabels) - }) -} - func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) { t.Run("generates bundle resources in AllNamespaces install mode", func(t *testing.T) { - var expectedWatchNamespace = corev1.NamespaceAll - helmApplier := applier.Helm{ ActionClientGetter: &mockActionGetter{ getClientErr: driver.ErrReleaseNotFound, @@ -659,9 +606,9 @@ func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) { Manifest: validManifest, }, }, - BundleToHelmChartConverter: &fakeBundleToHelmChartConverter{ - fn: func(bundle source.BundleSource, installNamespace string, config map[string]interface{}) (*chart.Chart, error) { - require.Equal(t, expectedWatchNamespace, config[registryv1Bundle.BundleConfigWatchNamespaceKey]) + HelmChartProvider: &fakeRegistryV1HelmChartProvider{ + fn: func(bundle source.BundleSource, ext *ocv1.ClusterExtension) (*chart.Chart, error) { + require.Equal(t, testCE, ext) return nil, nil }, }, @@ -683,8 +630,8 @@ func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) { Manifest: validManifest, }, }, - BundleToHelmChartConverter: &fakeBundleToHelmChartConverter{ - fn: func(bundle source.BundleSource, installNamespace string, config map[string]interface{}) (*chart.Chart, error) { + HelmChartProvider: &fakeRegistryV1HelmChartProvider{ + fn: func(bundle source.BundleSource, ext *ocv1.ClusterExtension) (*chart.Chart, error) { return nil, errors.New("some error") }, }, @@ -698,10 +645,10 @@ func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) { }) } -type fakeBundleToHelmChartConverter struct { - fn func(source.BundleSource, string, map[string]interface{}) (*chart.Chart, error) +type fakeRegistryV1HelmChartProvider struct { + fn func(source.BundleSource, *ocv1.ClusterExtension) (*chart.Chart, error) } -func (f fakeBundleToHelmChartConverter) ToHelmChart(bundle source.BundleSource, installNamespace string, config map[string]interface{}) (*chart.Chart, error) { - return f.fn(bundle, installNamespace, config) +func (f fakeRegistryV1HelmChartProvider) HelmChart(bundle source.BundleSource, ext *ocv1.ClusterExtension) (*chart.Chart, error) { + return f.fn(bundle, ext) } diff --git a/internal/operator-controller/rukpak/convert/helm.go b/internal/operator-controller/applier/provider.go similarity index 80% rename from internal/operator-controller/rukpak/convert/helm.go rename to internal/operator-controller/applier/provider.go index 4a354b569..5879dd9de 100644 --- a/internal/operator-controller/rukpak/convert/helm.go +++ b/internal/operator-controller/applier/provider.go @@ -1,4 +1,4 @@ -package convert +package applier import ( "crypto/sha256" @@ -7,30 +7,33 @@ import ( "helm.sh/helm/v3/pkg/chart" - bundlepkg "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle" + ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render" ) -type BundleToHelmChartConverter struct { +type RegistryV1HelmChartProvider struct { BundleRenderer render.BundleRenderer CertificateProvider render.CertificateProvider IsWebhookSupportEnabled bool } -func (r *BundleToHelmChartConverter) ToHelmChart(bundle source.BundleSource, installNamespace string, config map[string]interface{}) (*chart.Chart, error) { +func (r *RegistryV1HelmChartProvider) HelmChart(bundle source.BundleSource, ext *ocv1.ClusterExtension) (*chart.Chart, error) { rv1, err := bundle.GetBundle() if err != nil { return nil, err } + watchNamespace, err := GetWatchNamespace(ext) + if err != nil { + return nil, err + } + opts := []render.Option{ render.WithCertificateProvider(r.CertificateProvider), } - if config != nil { - if watchNs, ok := config[bundlepkg.BundleConfigWatchNamespaceKey].(string); ok { - opts = append(opts, render.WithTargetNamespaces(watchNs)) - } + if watchNamespace != "" { + opts = append(opts, render.WithTargetNamespaces(watchNamespace)) } if len(rv1.CSV.Spec.APIServiceDefinitions.Owned) > 0 { @@ -49,7 +52,7 @@ func (r *BundleToHelmChartConverter) ToHelmChart(bundle source.BundleSource, ins return nil, fmt.Errorf("unsupported bundle: webhookDefinitions are not supported") } - objs, err := r.BundleRenderer.Render(rv1, installNamespace, opts...) + objs, err := r.BundleRenderer.Render(rv1, ext.Spec.Namespace, opts...) if err != nil { return nil, fmt.Errorf("error rendering bundle: %w", err) diff --git a/internal/operator-controller/rukpak/convert/helm_test.go b/internal/operator-controller/applier/provider_test.go similarity index 72% rename from internal/operator-controller/rukpak/convert/helm_test.go rename to internal/operator-controller/applier/provider_test.go index 0151f7305..7ff733e81 100644 --- a/internal/operator-controller/rukpak/convert/helm_test.go +++ b/internal/operator-controller/applier/provider_test.go @@ -1,4 +1,4 @@ -package convert_test +package applier_test import ( "errors" @@ -6,34 +6,38 @@ import ( "github.com/stretchr/testify/require" 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/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/operator-framework/api/pkg/operators/v1alpha1" + ocv1 "github.com/operator-framework/operator-controller/api/v1" + "github.com/operator-framework/operator-controller/internal/operator-controller/applier" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source" - "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/convert" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render" . "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util/testing" ) func Test_BundleToHelmChartConverter_ToHelmChart_ReturnsBundleSourceFailures(t *testing.T) { - converter := convert.BundleToHelmChartConverter{} + provider := applier.RegistryV1HelmChartProvider{} var failingBundleSource FakeBundleSource = func() (bundle.RegistryV1, error) { return bundle.RegistryV1{}, errors.New("some error") } - config := map[string]interface{}{ - bundle.BundleConfigWatchNamespaceKey: "watch-namespace", + ext := &ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{ + Namespace: "install-namespace", + }, } - _, err := converter.ToHelmChart(failingBundleSource, "install-namespace", config) + _, err := provider.HelmChart(failingBundleSource, ext) require.Error(t, err) require.Contains(t, err.Error(), "some error") } func Test_BundleToHelmChartConverter_ToHelmChart_ReturnsBundleRendererFailures(t *testing.T) { - converter := convert.BundleToHelmChartConverter{ + provider := applier.RegistryV1HelmChartProvider{ BundleRenderer: render.BundleRenderer{ ResourceGenerators: []render.ResourceGenerator{ func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) { @@ -49,16 +53,18 @@ func Test_BundleToHelmChartConverter_ToHelmChart_ReturnsBundleRendererFailures(t }, ) - config := map[string]interface{}{ - bundle.BundleConfigWatchNamespaceKey: "", + ext := &ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{ + Namespace: "install-namespace", + }, } - _, err := converter.ToHelmChart(b, "install-namespace", config) + _, err := provider.HelmChart(b, ext) require.Error(t, err) require.Contains(t, err.Error(), "some error") } func Test_BundleToHelmChartConverter_ToHelmChart_NoAPIServiceDefinitions(t *testing.T) { - converter := convert.BundleToHelmChartConverter{} + provider := applier.RegistryV1HelmChartProvider{} b := source.FromBundle( bundle.RegistryV1{ @@ -66,16 +72,19 @@ func Test_BundleToHelmChartConverter_ToHelmChart_NoAPIServiceDefinitions(t *test }, ) - config := map[string]interface{}{ - bundle.BundleConfigWatchNamespaceKey: "", + ext := &ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{ + Namespace: "install-namespace", + }, } - _, err := converter.ToHelmChart(b, "install-namespace", config) + + _, err := provider.HelmChart(b, ext) require.Error(t, err) require.Contains(t, err.Error(), "unsupported bundle: apiServiceDefintions are not supported") } func Test_BundleToHelmChartConverter_ToHelmChart_NoWebhooksWithoutCertProvider(t *testing.T) { - converter := convert.BundleToHelmChartConverter{ + provider := applier.RegistryV1HelmChartProvider{ IsWebhookSupportEnabled: true, } @@ -85,16 +94,19 @@ func Test_BundleToHelmChartConverter_ToHelmChart_NoWebhooksWithoutCertProvider(t }, ) - config := map[string]interface{}{ - bundle.BundleConfigWatchNamespaceKey: "", + ext := &ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{ + Namespace: "install-namespace", + }, } - _, err := converter.ToHelmChart(b, "install-namespace", config) + + _, err := provider.HelmChart(b, ext) require.Error(t, err) require.Contains(t, err.Error(), "webhookDefinitions are not supported") } func Test_BundleToHelmChartConverter_ToHelmChart_WebhooksSupportDisabled(t *testing.T) { - converter := convert.BundleToHelmChartConverter{ + provider := applier.RegistryV1HelmChartProvider{ IsWebhookSupportEnabled: false, } @@ -104,16 +116,19 @@ func Test_BundleToHelmChartConverter_ToHelmChart_WebhooksSupportDisabled(t *test }, ) - config := map[string]interface{}{ - bundle.BundleConfigWatchNamespaceKey: "", + ext := &ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{ + Namespace: "install-namespace", + }, } - _, err := converter.ToHelmChart(b, "install-namespace", config) + + _, err := provider.HelmChart(b, ext) require.Error(t, err) require.Contains(t, err.Error(), "webhookDefinitions are not supported") } func Test_BundleToHelmChartConverter_ToHelmChart_WebhooksWithCertProvider(t *testing.T) { - converter := convert.BundleToHelmChartConverter{ + provider := applier.RegistryV1HelmChartProvider{ CertificateProvider: FakeCertProvider{}, IsWebhookSupportEnabled: true, } @@ -127,10 +142,13 @@ func Test_BundleToHelmChartConverter_ToHelmChart_WebhooksWithCertProvider(t *tes }, ) - config := map[string]interface{}{ - bundle.BundleConfigWatchNamespaceKey: "", + ext := &ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{ + Namespace: "install-namespace", + }, } - _, err := converter.ToHelmChart(b, "install-namespace", config) + + _, err := provider.HelmChart(b, ext) require.NoError(t, err) } @@ -139,7 +157,7 @@ func Test_BundleToHelmChartConverter_ToHelmChart_BundleRendererIntegration(t *te expectedWatchNamespace := "" expectedCertProvider := FakeCertProvider{} - converter := convert.BundleToHelmChartConverter{ + provider := applier.RegistryV1HelmChartProvider{ BundleRenderer: render.BundleRenderer{ ResourceGenerators: []render.ResourceGenerator{ func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) { @@ -160,15 +178,24 @@ func Test_BundleToHelmChartConverter_ToHelmChart_BundleRendererIntegration(t *te }, ) - config := map[string]interface{}{ - bundle.BundleConfigWatchNamespaceKey: expectedWatchNamespace, + ext := &ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{ + Namespace: "install-namespace", + Config: &ocv1.ClusterExtensionConfig{ + ConfigType: ocv1.ClusterExtensionConfigTypeInline, + Inline: &apiextensionsv1.JSON{ + Raw: []byte(`{"watchNamespace": "` + expectedWatchNamespace + `"}`), + }, + }, + }, } - _, err := converter.ToHelmChart(b, expectedInstallNamespace, config) + + _, err := provider.HelmChart(b, ext) require.NoError(t, err) } func Test_BundleToHelmChartConverter_ToHelmChart_Success(t *testing.T) { - converter := convert.BundleToHelmChartConverter{ + provider := applier.RegistryV1HelmChartProvider{ BundleRenderer: render.BundleRenderer{ ResourceGenerators: []render.ResourceGenerator{ func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) { @@ -202,10 +229,13 @@ func Test_BundleToHelmChartConverter_ToHelmChart_Success(t *testing.T) { }, ) - config := map[string]interface{}{ - bundle.BundleConfigWatchNamespaceKey: "", + ext := &ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{ + Namespace: "install-namespace", + }, } - chart, err := converter.ToHelmChart(b, "install-namespace", config) + + chart, err := provider.HelmChart(b, ext) require.NoError(t, err) require.NotNil(t, chart) require.NotNil(t, chart.Metadata) From f721a076e418ffb82efe744722cd125dbf8ef233 Mon Sep 17 00:00:00 2001 From: Per Goncalves da Silva Date: Fri, 12 Sep 2025 10:33:24 +0200 Subject: [PATCH 2/2] Address reviewer comments Signed-off-by: Per Goncalves da Silva --- .../operator-controller/applier/boxcutter.go | 1 + internal/operator-controller/applier/helm.go | 4 +- .../operator-controller/applier/helm_test.go | 2 +- .../operator-controller/applier/provider.go | 2 +- .../applier/provider_test.go | 108 ++++++++++++------ 5 files changed, 75 insertions(+), 42 deletions(-) diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index 51b5b88b5..616cace69 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -372,6 +372,7 @@ func latestRevisionNumber(prevRevisions []ocv1.ClusterExtensionRevision) int64 { type BundleRenderer interface { Render(bundleFS fs.FS, ext *ocv1.ClusterExtension) ([]client.Object, error) } + type RegistryV1BundleRenderer struct { BundleRenderer render.BundleRenderer CertificateProvider render.CertificateProvider diff --git a/internal/operator-controller/applier/helm.go b/internal/operator-controller/applier/helm.go index 29684bf53..3723fbc0e 100644 --- a/internal/operator-controller/applier/helm.go +++ b/internal/operator-controller/applier/helm.go @@ -36,7 +36,7 @@ import ( // HelmChartProvider provides helm charts from bundle sources and cluster extensions type HelmChartProvider interface { - HelmChart(bundle source.BundleSource, clusterExtension *ocv1.ClusterExtension) (*chart.Chart, error) + Get(bundle source.BundleSource, clusterExtension *ocv1.ClusterExtension) (*chart.Chart, error) } type HelmReleaseToObjectsConverter struct { @@ -212,7 +212,7 @@ func (h *Helm) buildHelmChart(bundleFS fs.FS, ext *ocv1.ClusterExtension) (*char ) } } - return h.HelmChartProvider.HelmChart(source.FromFS(bundleFS), ext) + return h.HelmChartProvider.Get(source.FromFS(bundleFS), ext) } 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 a4e217937..7bed5e263 100644 --- a/internal/operator-controller/applier/helm_test.go +++ b/internal/operator-controller/applier/helm_test.go @@ -649,6 +649,6 @@ type fakeRegistryV1HelmChartProvider struct { fn func(source.BundleSource, *ocv1.ClusterExtension) (*chart.Chart, error) } -func (f fakeRegistryV1HelmChartProvider) HelmChart(bundle source.BundleSource, ext *ocv1.ClusterExtension) (*chart.Chart, error) { +func (f fakeRegistryV1HelmChartProvider) Get(bundle source.BundleSource, ext *ocv1.ClusterExtension) (*chart.Chart, error) { return f.fn(bundle, ext) } diff --git a/internal/operator-controller/applier/provider.go b/internal/operator-controller/applier/provider.go index 5879dd9de..e0b715360 100644 --- a/internal/operator-controller/applier/provider.go +++ b/internal/operator-controller/applier/provider.go @@ -18,7 +18,7 @@ type RegistryV1HelmChartProvider struct { IsWebhookSupportEnabled bool } -func (r *RegistryV1HelmChartProvider) HelmChart(bundle source.BundleSource, ext *ocv1.ClusterExtension) (*chart.Chart, error) { +func (r *RegistryV1HelmChartProvider) Get(bundle source.BundleSource, ext *ocv1.ClusterExtension) (*chart.Chart, error) { rv1, err := bundle.GetBundle() if err != nil { return nil, err diff --git a/internal/operator-controller/applier/provider_test.go b/internal/operator-controller/applier/provider_test.go index 7ff733e81..3e2003b63 100644 --- a/internal/operator-controller/applier/provider_test.go +++ b/internal/operator-controller/applier/provider_test.go @@ -9,19 +9,21 @@ import ( apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + featuregatetesting "k8s.io/component-base/featuregate/testing" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/operator-framework/api/pkg/operators/v1alpha1" ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/operator-controller/applier" + "github.com/operator-framework/operator-controller/internal/operator-controller/features" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render" . "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util/testing" ) -func Test_BundleToHelmChartConverter_ToHelmChart_ReturnsBundleSourceFailures(t *testing.T) { +func Test_RegistryV1HelmChartProvider_Get_ReturnsBundleSourceFailures(t *testing.T) { provider := applier.RegistryV1HelmChartProvider{} var failingBundleSource FakeBundleSource = func() (bundle.RegistryV1, error) { return bundle.RegistryV1{}, errors.New("some error") @@ -31,12 +33,12 @@ func Test_BundleToHelmChartConverter_ToHelmChart_ReturnsBundleSourceFailures(t * Namespace: "install-namespace", }, } - _, err := provider.HelmChart(failingBundleSource, ext) + _, err := provider.Get(failingBundleSource, ext) require.Error(t, err) require.Contains(t, err.Error(), "some error") } -func Test_BundleToHelmChartConverter_ToHelmChart_ReturnsBundleRendererFailures(t *testing.T) { +func Test_RegistryV1HelmChartProvider_Get_ReturnsBundleRendererFailures(t *testing.T) { provider := applier.RegistryV1HelmChartProvider{ BundleRenderer: render.BundleRenderer{ ResourceGenerators: []render.ResourceGenerator{ @@ -58,12 +60,12 @@ func Test_BundleToHelmChartConverter_ToHelmChart_ReturnsBundleRendererFailures(t Namespace: "install-namespace", }, } - _, err := provider.HelmChart(b, ext) + _, err := provider.Get(b, ext) require.Error(t, err) require.Contains(t, err.Error(), "some error") } -func Test_BundleToHelmChartConverter_ToHelmChart_NoAPIServiceDefinitions(t *testing.T) { +func Test_RegistryV1HelmChartProvider_Get_NoAPIServiceDefinitions(t *testing.T) { provider := applier.RegistryV1HelmChartProvider{} b := source.FromBundle( @@ -78,12 +80,12 @@ func Test_BundleToHelmChartConverter_ToHelmChart_NoAPIServiceDefinitions(t *test }, } - _, err := provider.HelmChart(b, ext) + _, err := provider.Get(b, ext) require.Error(t, err) require.Contains(t, err.Error(), "unsupported bundle: apiServiceDefintions are not supported") } -func Test_BundleToHelmChartConverter_ToHelmChart_NoWebhooksWithoutCertProvider(t *testing.T) { +func Test_RegistryV1HelmChartProvider_Get_NoWebhooksWithoutCertProvider(t *testing.T) { provider := applier.RegistryV1HelmChartProvider{ IsWebhookSupportEnabled: true, } @@ -100,12 +102,12 @@ func Test_BundleToHelmChartConverter_ToHelmChart_NoWebhooksWithoutCertProvider(t }, } - _, err := provider.HelmChart(b, ext) + _, err := provider.Get(b, ext) require.Error(t, err) require.Contains(t, err.Error(), "webhookDefinitions are not supported") } -func Test_BundleToHelmChartConverter_ToHelmChart_WebhooksSupportDisabled(t *testing.T) { +func Test_RegistryV1HelmChartProvider_Get_WebhooksSupportDisabled(t *testing.T) { provider := applier.RegistryV1HelmChartProvider{ IsWebhookSupportEnabled: false, } @@ -122,12 +124,12 @@ func Test_BundleToHelmChartConverter_ToHelmChart_WebhooksSupportDisabled(t *test }, } - _, err := provider.HelmChart(b, ext) + _, err := provider.Get(b, ext) require.Error(t, err) require.Contains(t, err.Error(), "webhookDefinitions are not supported") } -func Test_BundleToHelmChartConverter_ToHelmChart_WebhooksWithCertProvider(t *testing.T) { +func Test_RegistryV1HelmChartProvider_Get_WebhooksWithCertProvider(t *testing.T) { provider := applier.RegistryV1HelmChartProvider{ CertificateProvider: FakeCertProvider{}, IsWebhookSupportEnabled: true, @@ -148,53 +150,83 @@ func Test_BundleToHelmChartConverter_ToHelmChart_WebhooksWithCertProvider(t *tes }, } - _, err := provider.HelmChart(b, ext) + _, err := provider.Get(b, ext) require.NoError(t, err) } -func Test_BundleToHelmChartConverter_ToHelmChart_BundleRendererIntegration(t *testing.T) { +func Test_RegistryV1HelmChartProvider_Get_BundleRendererIntegration(t *testing.T) { expectedInstallNamespace := "install-namespace" - expectedWatchNamespace := "" expectedCertProvider := FakeCertProvider{} + watchNamespace := "some-namespace" - provider := applier.RegistryV1HelmChartProvider{ - BundleRenderer: render.BundleRenderer{ - ResourceGenerators: []render.ResourceGenerator{ - func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) { - // ensure correct options are being passed down to the bundle renderer - require.Equal(t, expectedInstallNamespace, opts.InstallNamespace) - require.Equal(t, []string{expectedWatchNamespace}, opts.TargetNamespaces) - require.Equal(t, expectedCertProvider, opts.CertificateProvider) - return nil, nil + ext := &ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{ + Namespace: "install-namespace", + Config: &ocv1.ClusterExtensionConfig{ + ConfigType: ocv1.ClusterExtensionConfigTypeInline, + Inline: &apiextensionsv1.JSON{ + Raw: []byte(`{"watchNamespace": "` + watchNamespace + `"}`), }, }, }, - CertificateProvider: expectedCertProvider, } b := source.FromBundle( bundle.RegistryV1{ - CSV: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces)), + CSV: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeSingleNamespace)), }, ) - ext := &ocv1.ClusterExtension{ - Spec: ocv1.ClusterExtensionSpec{ - Namespace: "install-namespace", - Config: &ocv1.ClusterExtensionConfig{ - ConfigType: ocv1.ClusterExtensionConfigTypeInline, - Inline: &apiextensionsv1.JSON{ - Raw: []byte(`{"watchNamespace": "` + expectedWatchNamespace + `"}`), + t.Run("SingleOwnNamespace install mode support off", func(t *testing.T) { + provider := applier.RegistryV1HelmChartProvider{ + BundleRenderer: render.BundleRenderer{ + ResourceGenerators: []render.ResourceGenerator{ + func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) { + // ensure correct options are being passed down to the bundle renderer + require.Equal(t, expectedInstallNamespace, opts.InstallNamespace) + require.Equal(t, expectedCertProvider, opts.CertificateProvider) + + // target namespaces should not set to {""} (AllNamespaces) if the SingleOwnNamespace feature flag is off + t.Log("check that targetNamespaces option is set to AllNamespaces") + require.Equal(t, []string{""}, opts.TargetNamespaces) + return nil, nil + }, }, }, - }, - } + CertificateProvider: expectedCertProvider, + } + + _, err := provider.Get(b, ext) + require.NoError(t, err) + }) + + t.Run("feature on", func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.SingleOwnNamespaceInstallSupport, true) + + provider := applier.RegistryV1HelmChartProvider{ + BundleRenderer: render.BundleRenderer{ + ResourceGenerators: []render.ResourceGenerator{ + func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) { + // ensure correct options are being passed down to the bundle renderer + require.Equal(t, expectedInstallNamespace, opts.InstallNamespace) + require.Equal(t, expectedCertProvider, opts.CertificateProvider) + + // targetNamespace must be set if the feature flag is on + t.Log("check that targetNamespaces option is set") + require.Equal(t, []string{watchNamespace}, opts.TargetNamespaces) + return nil, nil + }, + }, + }, + CertificateProvider: expectedCertProvider, + } - _, err := provider.HelmChart(b, ext) - require.NoError(t, err) + _, err := provider.Get(b, ext) + require.NoError(t, err) + }) } -func Test_BundleToHelmChartConverter_ToHelmChart_Success(t *testing.T) { +func Test_RegistryV1HelmChartProvider_Get_Success(t *testing.T) { provider := applier.RegistryV1HelmChartProvider{ BundleRenderer: render.BundleRenderer{ ResourceGenerators: []render.ResourceGenerator{ @@ -235,7 +267,7 @@ func Test_BundleToHelmChartConverter_ToHelmChart_Success(t *testing.T) { }, } - chart, err := provider.HelmChart(b, ext) + chart, err := provider.Get(b, ext) require.NoError(t, err) require.NotNil(t, chart) require.NotNil(t, chart.Metadata)