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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
78 changes: 78 additions & 0 deletions internal/operator-controller/applier/helm.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,14 @@ import (
"fmt"
"io"
"io/fs"
"path/filepath"
"regexp"
"slices"
"strings"

"helm.sh/helm/v3/pkg/action"
"helm.sh/helm/v3/pkg/chart"
"helm.sh/helm/v3/pkg/chart/loader"
"helm.sh/helm/v3/pkg/chartutil"
"helm.sh/helm/v3/pkg/postrender"
"helm.sh/helm/v3/pkg/release"
Expand All @@ -26,6 +29,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/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"
Expand Down Expand Up @@ -209,9 +213,83 @@ func (h *Helm) buildHelmChart(bundleFS fs.FS, ext *ocv1.ClusterExtension) (*char
if err != nil {
return nil, err
}
if features.OperatorControllerFeatureGate.Enabled(features.HelmChartSupport) {
chart := new(chart.Chart)
if IsBundleSourceHelmChart(bundleFS, chart) {
return enrichChart(chart, WithInstallNamespace(ext.Spec.Namespace))
}
}
Copy link
Contributor

@camilamacedo86 camilamacedo86 May 21, 2025

Choose a reason for hiding this comment

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

Could we add the unit tests for this code? WDYT?
I mean, could we mock the data and test it at: https://github.com/operator-framework/operator-controller/blob/main/internal/operator-controller/applier/helm_test.go ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Working on tests right now.


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

func IsBundleSourceHelmChart(bundleFS fs.FS, chart *chart.Chart) bool {
var filename string
if err := fs.WalkDir(bundleFS, ".",
func(path string, f fs.DirEntry, err error) error {
if err != nil {
return err
}

if filepath.Ext(f.Name()) == ".tgz" &&
!f.IsDir() {
filename = path
return fs.SkipAll
}

return nil
},
); err != nil &&
!errors.Is(err, fs.SkipAll) {
return false
}

ch, err := readChartFS(bundleFS, filename)
if err != nil {
return false
}
*chart = *ch

return chart.Metadata != nil &&
chart.Metadata.Name != "" &&
chart.Metadata.Version != "" &&
len(chart.Templates) > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

If a directory contains a Chart.yaml and templates, it qualifies as a Helm chart, right?
I mean, even if the name or version fields are missing from Chart.yaml, it’s still technically a chart — just not a valid one according to Helm’s requirements.

So maybe we should distinguish between two concepts:

  • IsHelmChart: checks for the presence of Chart.yaml and templates (basic structure).
  • IsValidHelmChart: additionally ensures required fields like name and version are set. ( as any other that we might to consider in the future )

Does that distinction make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you are saying certainly make sense. However, what I am working around is the chart would be stored on a filesystem of type fs.FS. Therefore, loader.LoadDir() is unable to find a Helm chart. The only option to proceed with this approach is to implement the ChartLoader interface and all other tools that error when working with files in an fs.FS filesystem.

Since the contents of the Chart.yaml would be loaded into a &chart.Manifest{} object. I think it should suffice to check that the chart.Metadata field is not nil and we have templates.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, we can keep doing both checks. But would it make sense to define both functions explicitly and call them, rather than assuming something is a HelmChart just because it has a name and version set? I mean, can we just split in 2 funcs and call them out instead of have both logics inside of IsHelmChart?

Sorry if I’m being a bit nitpicky — I just think this approach could give us more flexibility to grow and evolve the logic in the future.

}

type ChartOption func(*chart.Chart)

func WithInstallNamespace(namespace string) ChartOption {
re := regexp.MustCompile(`{{\W+\.Release\.Namespace\W+}}`)

return func(chrt *chart.Chart) {
for i, template := range chrt.Templates {
chrt.Templates[i].Data = re.ReplaceAll(template.Data, []byte(namespace))
}
}
}

func enrichChart(chart *chart.Chart, options ...ChartOption) (*chart.Chart, error) {
if chart != nil {
for _, f := range options {
f(chart)
}
return chart, nil
}
return nil, fmt.Errorf("chart can not be nil")
}

func readChartFS(bundleFS fs.FS, filename string) (*chart.Chart, error) {
if filename == "" {
return nil, fmt.Errorf("chart file name was not provided")
}

tarball, err := fs.ReadFile(bundleFS, filename)
if err != nil {
return nil, fmt.Errorf("reading chart %s; %+v", filename, err)
}
return loader.LoadArchive(bytes.NewBuffer(tarball))
}

func (h *Helm) renderClientOnlyRelease(ctx context.Context, ext *ocv1.ClusterExtension, chrt *chart.Chart, values chartutil.Values, post postrender.PostRenderer) (*release.Release, error) {
// We need to get a separate action client because our work below
// permanently modifies the underlying action.Configuration for ClientOnly mode.
Expand Down
9 changes: 9 additions & 0 deletions internal/operator-controller/features/features.go
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
1 change: 1 addition & 0 deletions internal/shared/util/image/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ type LayerData struct {
}

type Cache interface {
ExtendCache
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that valid for any or only Helm
Would be better we rename it like ChartCache, something like?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only for Helm charts at this time. We would need to see if it is possible to refactor the Store() method in the Cache interface to be able to store both Helm charts and OCI images to the cache.

Fetch(context.Context, string, reference.Canonical) (fs.FS, time.Time, error)
Store(context.Context, string, reference.Named, reference.Canonical, ocispecv1.Image, iter.Seq[LayerData]) (fs.FS, time.Time, error)
Delete(context.Context, string) error
Expand Down
90 changes: 90 additions & 0 deletions internal/shared/util/image/helm.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
package image

import (
"context"
"encoding/json"
"fmt"
"io"
"io/fs"
"os"
"path/filepath"
"time"

"github.com/containers/image/v5/docker/reference"
"github.com/containers/image/v5/types"
"github.com/opencontainers/go-digest"
specsv1 "github.com/opencontainers/image-spec/specs-go/v1"
"helm.sh/helm/v3/pkg/registry"

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

func hasChart(imgCloser types.ImageCloser) bool {
config := imgCloser.ConfigInfo()
return config.MediaType == registry.ConfigMediaType
}

type ExtendCache interface {
StoreChart(string, string, reference.Canonical, io.Reader) (fs.FS, time.Time, error)
}

func (a *diskCache) StoreChart(ownerID, filename string, canonicalRef reference.Canonical, src io.Reader) (fs.FS, time.Time, error) {
dest := a.unpackPath(ownerID, canonicalRef.Digest())

if err := fsutil.EnsureEmptyDirectory(dest, 0700); err != nil {
return nil, time.Time{}, fmt.Errorf("error ensuring empty charts directory: %w", err)
}

// Destination file
chart, err := os.Create(filepath.Join(dest, filename))
if err != nil {
return nil, time.Time{}, fmt.Errorf("creating chart file; %w", err)
}
defer chart.Close()

_, err = io.Copy(chart, src)
if err != nil {
return nil, time.Time{}, fmt.Errorf("copying chart to %s; %w", filename, err)
}

modTime, err := fsutil.GetDirectoryModTime(dest)
if err != nil {
return nil, time.Time{}, fmt.Errorf("error getting mod time of unpack directory: %w", err)
}
return os.DirFS(filepath.Dir(dest)), modTime, nil
}

func pullChart(ctx context.Context, ownerID string, img types.ImageSource, canonicalRef reference.Canonical, cache Cache, imgRef types.ImageReference) (fs.FS, time.Time, error) {
raw, _, err := img.GetManifest(ctx, nil)
if err != nil {
return nil, time.Time{}, fmt.Errorf("get OCI helm chart manifest; %w", err)
}

chartManifest := specsv1.Manifest{}
if err := json.Unmarshal(raw, &chartManifest); err != nil {
return nil, time.Time{}, fmt.Errorf("unmarshaling chart manifest; %w", err)
}

var chartDataLayerDigest digest.Digest
if len(chartManifest.Layers) == 1 &&
(chartManifest.Layers[0].MediaType == registry.ChartLayerMediaType) {
chartDataLayerDigest = chartManifest.Layers[0].Digest
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we not need to do further verifications?
Also, I think we can have more than 1 layer.

So maybe something like:

var chartDataLayerDigest digest.Digest
found := false

if len(chartManifest.Layers) == 0 {
	return nil, time.Time{}, fmt.Errorf("manifest has no layers; expected at least one chart layer")
}

for i, layer := range chartManifest.Layers {
	if layer.MediaType == registry.ChartLayerMediaType {
		chartDataLayerDigest = layer.Digest
		found = true
		break
	}
}

if !found {
	return nil, time.Time{}, fmt.Errorf(
		"no layer with media type %q found in manifest (total layers: %d)",
		registry.ChartLayerMediaType,
		len(chartManifest.Layers),
	)
}


filename := fmt.Sprintf("%s-%s.tgz",
chartManifest.Annotations["org.opencontainers.image.title"],
chartManifest.Annotations["org.opencontainers.image.version"],
)
Comment on lines +74 to +77
Copy link
Contributor

@camilamacedo86 camilamacedo86 May 21, 2025

Choose a reason for hiding this comment

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

I think we need:

  • a) IsValidHelmChart(): (For OLM) this should ensure that required fields like name and version are set. We should also design it to be extensible so we can include other validations in the future.

Example

func IsValidHelmChart(chart *chart.Chart) error {
	if chart.Metadata == nil {
		return errors.New("chart metadata is missing")
	}
	if chart.Metadata.Name == "" {
		return errors.New("chart name is required")
	}
	if chart.Metadata.Version == "" {
		return errors.New("chart version is required")
	}

	// Future validations can go here, e.g., required annotations, maintainers, etc.

	return nil
}
  • b) The name and version must come from Chart.yaml: this is where authors are expected to define them. The org.opencontainers.image.title and org.opencontainers.image.version annotations are optional and not required to be set by the author. Moreover, relying on these annotations is less intuitive and redundant compared to using the chart spec itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@camilamacedo86 The lines of code you are referring to are dealing with the OCI image that represents the Helm Chart. Therefore, we can not readily review the Chart.yaml contents. However, the helm library generates OCI annotations using the values in the Chart.yaml; specifically, the Name and Version are used to populate org.opencontainers.image.title and org.opencontainers.image.version respectively.

This can be seen in action in the registry client Push() method which starts at line 630 but more specifically the OCI annotations are added in between lines 687 and 693.


// Source file
tarball, err := os.Open(filepath.Join(
imgRef.PolicyConfigurationIdentity(), "blobs",
"sha256", chartDataLayerDigest.Encoded()),
)
if err != nil {
return nil, time.Time{}, fmt.Errorf("opening chart data; %w", err)
}
defer tarball.Close()

return cache.StoreChart(ownerID, filename, canonicalRef, tarball)
}
5 changes: 5 additions & 0 deletions internal/shared/util/image/mocks.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package image

import (
"context"
"io"
"io/fs"
"iter"
"time"
Expand Down Expand Up @@ -44,6 +45,10 @@ type MockCache struct {
GarbageCollectError error
}

func (m MockCache) StoreChart(_ string, _ string, _ reference.Canonical, _ io.Reader) (fs.FS, time.Time, error) {
return m.StoreFS, m.StoreModTime, m.StoreError
}

func (m MockCache) Fetch(_ context.Context, _ string, _ reference.Canonical) (fs.FS, time.Time, error) {
return m.FetchFS, m.FetchModTime, m.FetchError
}
Expand Down
5 changes: 5 additions & 0 deletions internal/shared/util/image/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,11 @@ func (p *ContainersImagePuller) applyImage(ctx context.Context, ownerID string,
}
}()

if hasChart(img) {
return pullChart(ctx, ownerID, imgSrc, canonicalRef, cache, srcImgRef)
}

// Helm charts would error when getting OCI config
ociImg, err := img.OCIConfig(ctx)
if err != nil {
return nil, time.Time{}, err
Expand Down
Loading