Skip to content

✨ Add support for deploying OCI helm charts in OLM v1 #1971

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

✨ Add support for deploying OCI helm charts in OLM v1 #1971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 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,9 +26,11 @@ 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"
Copy link
Contributor

@camilamacedo86 camilamacedo86 Jun 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@OchiengEd

Here: https://github.com/operator-framework/operator-controller/pull/1724/files

@perdasilva added a demo and the doc under the docs/draft for another alpha feature
It would be very nice if we could do your demo within and add the doc for that. Such as it was done in this PR. However, I am okay with it being a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would be for adding documentation and a demo as a follow up.

)

const (
Expand Down Expand Up @@ -209,6 +211,17 @@ func (h *Helm) buildHelmChart(bundleFS fs.FS, ext *ocv1.ClusterExtension) (*char
if err != nil {
return nil, err
}
if features.OperatorControllerFeatureGate.Enabled(features.HelmChartSupport) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We've been trying to put the feature checks in main. Wdyt about refactoring to have another implementation of the BundleToHelmChart converter that accepts chart bundles, or maybe that can route between different bundle types? Then if the feature gate is enabled, use that one?

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)
}

Expand Down
9 changes: 9 additions & 0 deletions internal/operator-controller/features/features.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const (
SyntheticPermissions featuregate.Feature = "SyntheticPermissions"
WebhookProviderCertManager featuregate.Feature = "WebhookProviderCertManager"
WebhookProviderOpenshiftServiceCA featuregate.Feature = "WebhookProviderOpenshiftServiceCA"
HelmChartSupport featuregate.Feature = "HelmChartSupport"
)

var operatorControllerFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{
Expand Down Expand Up @@ -63,6 +64,14 @@ var operatorControllerFeatureGates = map[featuregate.Feature]featuregate.Feature
PreRelease: featuregate.Alpha,
LockToDefault: false,
},

// HelmChartSupport enables support for installing,
// updating and uninstalling Helm Charts via Cluster Extensions.
HelmChartSupport: {
Default: false,
PreRelease: featuregate.Alpha,
LockToDefault: false,
},
}

var OperatorControllerFeatureGate featuregate.MutableFeatureGate = featuregate.NewFeatureGate()
Expand Down
47 changes: 42 additions & 5 deletions internal/shared/util/image/cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,19 @@ import (
"github.com/containers/image/v5/docker/reference"
"github.com/opencontainers/go-digest"
ocispecv1 "github.com/opencontainers/image-spec/specs-go/v1"
"helm.sh/helm/v3/pkg/chart"
"helm.sh/helm/v3/pkg/registry"
"sigs.k8s.io/controller-runtime/pkg/log"

errorutil "github.com/operator-framework/operator-controller/internal/shared/util/error"
fsutil "github.com/operator-framework/operator-controller/internal/shared/util/fs"
)

type LayerData struct {
Reader io.Reader
Index int
Err error
MediaType string
Reader io.Reader
Index int
Err error
}

type Cache interface {
Expand Down Expand Up @@ -128,8 +131,15 @@ func (a *diskCache) Store(ctx context.Context, ownerID string, srcRef reference.
if layer.Err != nil {
return fmt.Errorf("error reading layer[%d]: %w", layer.Index, layer.Err)
}
if _, err := archive.Apply(ctx, dest, layer.Reader, applyOpts...); err != nil {
return fmt.Errorf("error applying layer[%d]: %w", layer.Index, err)
switch layer.MediaType {
case registry.ChartLayerMediaType:
if err := storeChartLayer(dest, layer); err != nil {
return err
}
default:
if _, err := archive.Apply(ctx, dest, layer.Reader, applyOpts...); err != nil {
return fmt.Errorf("error applying layer[%d]: %w", layer.Index, err)
}
Comment on lines +134 to +142
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it not be protected behind the feature flag as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code will be unreachable as a result of the pullChart() function being wrapped around a feature gate. However, if we need to wrap that code as well, feel tree to let me know

operator-controller/internal/shared/util/image/pull.go

Lines 228 to 232 in 9b366d3

if features.OperatorControllerFeatureGate.Enabled(features.HelmChartSupport) {
if hasChart(img) {
return pullChart(ctx, ownerID, srcRef, canonicalRef, imgSrc, cache)
}
}

Copy link
Contributor

@camilamacedo86 camilamacedo86 Jun 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I believe that all code should be categorised under a flag, allowing us to easily identify the purpose of each specific code for each alpha/beta feature. However, I think we can wait for others' input to see what they think about. My concern is:

What happens if we decide not to use the alpha/beta feature and want to delete it? If we decide not to promote feature A or B. How hard will it be if not all related code is not under the feature flag condition? But others might think that is OK

@perdasilva @tmshort WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly to my comment above about moving the feature flag checks to main, should we refactor this in a way that make the cache configurable as to the different layer media types and how they are handled?

}
l.Info("applied layer", "layer", layer.Index)
}
Expand All @@ -147,6 +157,33 @@ func (a *diskCache) Store(ctx context.Context, ownerID string, srcRef reference.
return os.DirFS(dest), modTime, nil
}

func storeChartLayer(path string, layer LayerData) error {
if layer.Err != nil {
return fmt.Errorf("error found in layer data: %w", layer.Err)
}
data, err := io.ReadAll(layer.Reader)
if err != nil {
return fmt.Errorf("error reading layer[%d]: %w", layer.Index, err)
}
meta := new(chart.Metadata)
_, err = inspectChart(data, meta)
if err != nil {
return fmt.Errorf("inspecting chart layer: %w", err)
}
filename := filepath.Join(path,
fmt.Sprintf("%s-%s.tgz", meta.Name, meta.Version),
)
fmt.Println(filename)
chart, err := os.Create(filename)
if err != nil {
return fmt.Errorf("writing chart layer: %w", err)
}
defer chart.Close()

_, err = chart.Write(data)
return err
}

func (a *diskCache) Delete(_ context.Context, ownerID string) error {
return fsutil.DeleteReadOnlyRecursive(a.ownerIDPath(ownerID))
}
Expand Down
189 changes: 189 additions & 0 deletions internal/shared/util/image/cache_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ package image

import (
"archive/tar"
"bytes"
"context"
"errors"
"fmt"
"io"
"io/fs"
"iter"
Expand All @@ -20,6 +22,7 @@ import (
ocispecv1 "github.com/opencontainers/image-spec/specs-go/v1"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"helm.sh/helm/v3/pkg/registry"

fsutil "github.com/operator-framework/operator-controller/internal/shared/util/fs"
)
Expand Down Expand Up @@ -144,6 +147,67 @@ func TestDiskCacheFetch(t *testing.T) {
}
}

func TestDiskCacheStore_HelmChart(t *testing.T) {
const myOwner = "myOwner"
myCanonicalRef := mustParseCanonical(t, "my.registry.io/ns/chart@sha256:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03")
myTaggedRef, err := reference.WithTag(reference.TrimNamed(myCanonicalRef), "test-tag")
require.NoError(t, err)

testCases := []struct {
name string
ownerID string
srcRef reference.Named
canonicalRef reference.Canonical
imgConfig ocispecv1.Image
layers iter.Seq[LayerData]
filterFunc func(context.Context, reference.Named, ocispecv1.Image) (archive.Filter, error)
setup func(*testing.T, *diskCache)
expect func(*testing.T, *diskCache, fs.FS, time.Time, error)
}{
{
name: "returns no error if layer read contains helm chart",
ownerID: myOwner,
srcRef: myTaggedRef,
canonicalRef: myCanonicalRef,
layers: func() iter.Seq[LayerData] {
testChart := mockHelmChartTgz(t,
[]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"),
},
},
)
return func(yield func(LayerData) bool) {
yield(LayerData{Reader: bytes.NewBuffer(testChart), MediaType: registry.ChartLayerMediaType})
}
}(),
expect: func(t *testing.T, cache *diskCache, fsys fs.FS, modTime time.Time, err error) {
require.NoError(t, err)
},
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
dc := &diskCache{
basePath: t.TempDir(),
filterFunc: tc.filterFunc,
}
if tc.setup != nil {
tc.setup(t, dc)
}
fsys, modTime, err := dc.Store(context.Background(), tc.ownerID, tc.srcRef, tc.canonicalRef, tc.imgConfig, tc.layers)
require.NotNil(t, tc.expect, "test case must include an expect function")
tc.expect(t, dc, fsys, modTime, err)
require.NoError(t, fsutil.DeleteReadOnlyRecursive(dc.basePath))
})
}
}

func TestDiskCacheStore(t *testing.T) {
const myOwner = "myOwner"
myCanonicalRef := mustParseCanonical(t, "my.registry.io/ns/repo@sha256:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03")
Expand Down Expand Up @@ -585,6 +649,123 @@ func TestDiskCacheGarbageCollection(t *testing.T) {
}
}

func Test_storeChartLayer(t *testing.T) {
tmp := t.TempDir()
type args struct {
path string
data LayerData
}
type want struct {
errStr string
}

tests := []struct {
name string
args args
want want
}{
{
name: "store chart layer to given path",
args: args{
path: tmp,
data: LayerData{
Index: 0,
MediaType: registry.ChartLayerMediaType,
Reader: bytes.NewBuffer(mockHelmChartTgz(t,
[]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: "store invalid chart layer",
args: args{
path: tmp,
data: LayerData{
Index: 0,
MediaType: registry.ChartLayerMediaType,
Reader: bytes.NewBuffer(mockHelmChartTgz(t,
[]fileContent{
{
name: "testchart/Chart.yaml",
content: []byte("apiVersion: v2\nname: testchart\nversion: 0.1.0"),
},
{
name: "testchart/deployment.yaml",
content: []byte("kind: Deployment\napiVersion: apps/v1"),
},
},
)),
},
},
want: want{
errStr: "inspecting chart layer: templates directory not found",
},
},
{
name: "store existing from dummy reader",
args: args{
path: tmp,
data: LayerData{
Index: 0,
MediaType: registry.ChartLayerMediaType,
Reader: &dummyReader{},
},
},
want: want{
errStr: "error reading layer[0]: something went wrong",
},
},
{
name: "handle chart layer data",
args: args{
path: tmp,
data: LayerData{
Index: 0,
MediaType: registry.ChartLayerMediaType,
Err: fmt.Errorf("invalid layer data"),
Reader: bytes.NewBuffer(mockHelmChartTgz(t,
[]fileContent{
{
name: "testchart/Chart.yaml",
content: []byte("apiVersion: v2\nname: testchart\nversion: 0.1.0"),
},
{
name: "testchart/deployment.yaml",
content: []byte("kind: Deployment\napiVersion: apps/v1"),
},
},
)),
},
},
want: want{
errStr: "error found in layer data: invalid layer data",
},
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
err := storeChartLayer(tc.args.path, tc.args.data)
if tc.want.errStr != "" {
require.Error(t, err)
require.EqualError(t, err, tc.want.errStr, "chart store error")
} else {
require.NoError(t, err)
}
})
}
}

func mustParseCanonical(t *testing.T, s string) reference.Canonical {
n, err := reference.ParseNamed(s)
require.NoError(t, err)
Expand Down Expand Up @@ -619,3 +800,11 @@ func fsTarReader(fsys fs.FS) io.ReadCloser {
}()
return pr
}

type dummyReader struct{}

var _ io.Reader = &dummyReader{}

func (r *dummyReader) Read(p []byte) (int, error) {
return 0, errors.New("something went wrong")
}
Loading
Loading