-
Notifications
You must be signed in to change notification settings - Fork 64
✨ 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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
|
@@ -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" | ||
|
@@ -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)) | ||
} | ||
} | ||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? So maybe we should distinguish between two concepts:
Does that distinction make sense? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Since the contents of the Chart.yaml would be loaded into a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,7 @@ type LayerData struct { | |
} | ||
|
||
type Cache interface { | ||
ExtendCache | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that valid for any or only Helm There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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 | ||
|
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) | ||
OchiengEd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
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 | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would we not need to do further verifications? 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need:
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
}
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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) | ||
} |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.