✨ Add support for deploying OCI helm charts in OLM v1 #1971
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1971 +/- ##
==========================================
- Coverage 69.06% 68.06% -1.00%
==========================================
Files 79 80 +1
Lines 7011 7136 +125
==========================================
+ Hits 4842 4857 +15
- Misses 1887 1995 +108
- Partials 282 284 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
OchiengEd
force-pushed
the
helm_explorations
branch
from
3b36dfb to
272dcbf
Compare
internal/shared/util/image/pull.go
Outdated
| if hasChart(img) { | ||
| return pullChart(ctx, ownerID, imgSrc, canonicalRef, cache, layoutDir) | ||
| } | ||
|
|
||
| // Helm charts would error when getting OCI config |
There was a problem hiding this comment.
This tells me that our cache Store interface method is too specific. We need to make that generic enough to accommodate registry+v1 bundles and OCI helm charts as a first step, and rebase the feature-gated helm support on top.
There was a problem hiding this comment.
Making the Cache interface generic enough to be able to store both Helm charts and OCI images is inevitable. Will be looking at that at some point
* added support for deploying OCI helm charts which sits behind the HelmChartSupport feature gate * extended the Cache interface to allow storing of Helm charts Signed-off-by: Edmund Ochieng <[email protected]>
OchiengEd
force-pushed
the
helm_explorations
branch
from
272dcbf to
39948a0
Compare
| return chart.Metadata != nil && | ||
| chart.Metadata.Name != "" && | ||
| chart.Metadata.Version != "" && | ||
| len(chart.Templates) > 0 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| filename := fmt.Sprintf("%s-%s.tgz", | ||
| chartManifest.Annotations["org.opencontainers.image.title"], | ||
| chartManifest.Annotations["org.opencontainers.image.version"], | ||
| ) |
There was a problem hiding this comment.
I think we need:
- a) IsValidHelmChart(): (For OLM) this should ensure that required fields like
nameandversionare 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
nameandversionmust come fromChart.yaml: this is where authors are expected to define them. Theorg.opencontainers.image.titleandorg.opencontainers.image.versionannotations 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.
There was a problem hiding this comment.
@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.
| if IsBundleSourceHelmChart(bundleFS, chart) { | ||
| return enrichChart(chart, WithInstallNamespace(ext.Spec.Namespace)) | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
Sure. Working on tests right now.
| if len(chartManifest.Layers) == 1 && | ||
| (chartManifest.Layers[0].MediaType == registry.ChartLayerMediaType) { | ||
| chartDataLayerDigest = chartManifest.Layers[0].Digest | ||
| } |
There was a problem hiding this comment.
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),
)
}| @@ -29,6 +29,7 @@ type LayerData struct { | |||
| } | |||
|
|
|||
| type Cache interface { | |||
| ExtendCache | |||
There was a problem hiding this comment.
Is that valid for any or only Helm
Would be better we rename it like ChartCache, something like?
There was a problem hiding this comment.
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.
Description
This pull request aims to add logic to OLM v1 for handling OCI Helm chart support. We expect more work to go into this feature as further discussion on this occurs on issue #962 and the Arbitrary Configuration RFC which may inform how
values.ymlwould be passed to Helm charts.Reviewer Checklist