Skip to content

🌱 Refactor rukpack package for configurable registry+v1 rendering behavior #1968

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

Merged
merged 4 commits into from
May 15, 2025
Merged

🌱 Refactor rukpack package for configurable registry+v1 rendering behavior #1968

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9b29cf4
Refactor render package
May 13, 2025
443874b
Remove plain converter
May 13, 2025
6606530
RegistryV1ToHelmChart to BundleToHelmChartConverter
May 13, 2025
eae2c24
Configure bundle to helm converter based on feature flags
May 14, 2025
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
26 changes: 21 additions & 5 deletions cmd/operator-controller/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ import (
"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"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render/registryv1"
"github.com/operator-framework/operator-controller/internal/operator-controller/scheme"
fsutil "github.com/operator-framework/operator-controller/internal/shared/util/fs"
httputil "github.com/operator-framework/operator-controller/internal/shared/util/http"
Expand Down Expand Up @@ -189,7 +192,7 @@ func run() error {
secretParts := strings.Split(cfg.globalPullSecret, "/")
if len(secretParts) != 2 {
err := fmt.Errorf("incorrect number of components")
setupLog.Error(err, "value of global-pull-secret should be of the format <namespace>/<name>")
setupLog.Error(err, "Value of global-pull-secret should be of the format <namespace>/<name>")
return err
}
globalPullSecretKey = &k8stypes.NamespacedName{Name: secretParts[1], Namespace: secretParts[0]}
Expand Down Expand Up @@ -421,12 +424,25 @@ func run() error {
preAuth = authorization.NewRBACPreAuthorizer(mgr.GetClient())
}

// determine if a certificate provider should be set in the bundle renderer and feature support for the provider
// based on the feature flag
var certProvider render.CertificateProvider
var isWebhookSupportEnabled bool
if features.OperatorControllerFeatureGate.Enabled(features.WebhookProviderCertManager) {
certProvider = certproviders.CertManagerCertificateProvider{}
isWebhookSupportEnabled = true
Copy link
Contributor

@camilamacedo86 camilamacedo86 May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I see you are trying to allow more options without bringing the complexity
If we enable the Webhook then automatically we either enable WebhookProviderCertManager

}

// now initialize the helmApplier, assigning the potentially nil preAuth
helmApplier := &applier.Helm{
ActionClientGetter: acg,
Preflights: preflights,
BundleToHelmChartFn: convert.RegistryV1ToHelmChart,
PreAuthorizer: preAuth,
ActionClientGetter: acg,
Preflights: preflights,
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{
BundleRenderer: registryv1.Renderer,
CertificateProvider: certProvider,
IsWebhookSupportEnabled: isWebhookSupportEnabled,
},
PreAuthorizer: preAuth,
}

cm := contentmanager.NewManager(clientRestConfigMapper, mgr.GetConfig(), mgr.GetRESTMapper())
Expand Down
19 changes: 11 additions & 8 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,6 +26,7 @@ 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/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"
)
Expand Down Expand Up @@ -53,13 +54,15 @@ type Preflight interface {
Upgrade(context.Context, *release.Release) error
}

type BundleToHelmChartFn func(rv1 fs.FS, installNamespace string, watchNamespace string) (*chart.Chart, error)
type BundleToHelmChartConverter interface {
ToHelmChart(bundle source.BundleSource, installNamespace string, watchNamespace string) (*chart.Chart, error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so we are moving away to have a function to define a contract/interface to convert any BundleSource to a HelmChart.

For HelmChart, we’d skip the convert call anyway, right?
But for something like a registry/v2 source, having the interface could help.
Is that what you had in mind?

}

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

// shouldSkipPreflight is a helper to determine if the preflight check is CRDUpgradeSafety AND
Expand Down Expand Up @@ -199,14 +202,14 @@ 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.BundleToHelmChartFn == nil {
return nil, errors.New("BundleToHelmChartFn is nil")
if h.BundleToHelmChartConverter == nil {
return nil, errors.New("BundleToHelmChartConverter is nil")
}
watchNamespace, err := GetWatchNamespace(ext)
if err != nil {
return nil, err
}
return h.BundleToHelmChartFn(bundleFS, ext.Spec.Namespace, watchNamespace)
return h.BundleToHelmChartConverter.ToHelmChart(source.FromFS(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
110 changes: 62 additions & 48 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,7 +4,6 @@ import (
"context"
"errors"
"io"
"io/fs"
"os"
"testing"
"testing/fstest"
Expand All @@ -27,6 +26,7 @@ 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"
)

Expand Down Expand Up @@ -197,8 +197,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,
BundleToHelmChartFn: convert.RegistryV1ToHelmChart,
ActionClientGetter: mockAcg,
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
}

objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
Expand All @@ -211,8 +211,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,
BundleToHelmChartFn: convert.RegistryV1ToHelmChart,
ActionClientGetter: mockAcg,
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
}

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

objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
Expand All @@ -248,9 +248,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},
BundleToHelmChartFn: convert.RegistryV1ToHelmChart,
ActionClientGetter: mockAcg,
Preflights: []applier.Preflight{mockPf},
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
}

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

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

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

objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
Expand All @@ -328,10 +328,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},
BundleToHelmChartFn: convert.RegistryV1ToHelmChart,
ActionClientGetter: mockAcg,
Preflights: []applier.Preflight{mockPf},
PreAuthorizer: &mockPreAuthorizer{nil, nil},
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
}

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

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

objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
Expand All @@ -464,9 +464,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},
BundleToHelmChartFn: convert.RegistryV1ToHelmChart,
ActionClientGetter: mockAcg,
Preflights: []applier.Preflight{mockPf},
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
}

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

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

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

objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
Expand All @@ -556,9 +556,11 @@ func TestApply_InstallationWithSingleOwnNamespaceInstallSupportEnabled(t *testin
Manifest: validManifest,
},
},
BundleToHelmChartFn: func(rv1 fs.FS, installNamespace string, watchNamespace string) (*chart.Chart, error) {
require.Equal(t, expectedWatchNamespace, watchNamespace)
return convert.RegistryV1ToHelmChart(rv1, installNamespace, watchNamespace)
BundleToHelmChartConverter: &fakeBundleToHelmChartConverter{
fn: func(bundle source.BundleSource, installNamespace string, watchNamespace string) (*chart.Chart, error) {
require.Equal(t, expectedWatchNamespace, watchNamespace)
return nil, nil
},
},
}

Expand Down Expand Up @@ -587,9 +589,11 @@ func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) {
Manifest: validManifest,
},
},
BundleToHelmChartFn: func(rv1 fs.FS, installNamespace string, watchNamespace string) (*chart.Chart, error) {
require.Equal(t, expectedWatchNamespace, watchNamespace)
return convert.RegistryV1ToHelmChart(rv1, installNamespace, watchNamespace)
BundleToHelmChartConverter: &fakeBundleToHelmChartConverter{
fn: func(bundle source.BundleSource, installNamespace string, watchNamespace string) (*chart.Chart, error) {
require.Equal(t, expectedWatchNamespace, watchNamespace)
return nil, nil
},
},
}

Expand All @@ -605,12 +609,22 @@ func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) {
Manifest: validManifest,
},
},
BundleToHelmChartFn: func(rv1 fs.FS, installNamespace string, watchNamespace string) (*chart.Chart, error) {
return nil, errors.New("some error")
BundleToHelmChartConverter: &fakeBundleToHelmChartConverter{
fn: func(bundle source.BundleSource, installNamespace string, watchNamespace string) (*chart.Chart, error) {
return nil, errors.New("some error")
},
},
}

_, _, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
require.Error(t, err)
})
}

type fakeBundleToHelmChartConverter struct {
fn func(source.BundleSource, 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)
}
15 changes: 15 additions & 0 deletions internal/operator-controller/rukpak/bundle/registryv1.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package bundle

import (
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"

"github.com/operator-framework/api/pkg/operators/v1alpha1"
)

type RegistryV1 struct {
PackageName string
CSV v1alpha1.ClusterServiceVersion
CRDs []apiextensionsv1.CustomResourceDefinition
Others []unstructured.Unstructured
}
Loading
Loading