✨ 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 73.82% 73.83% +0.01%
==========================================
Files 81 82 +1
Lines 7365 7530 +165
==========================================
+ Hits 5437 5560 +123
- Misses 1588 1618 +30
- Partials 340 352 +12
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
OchiengEd
force-pushed
the
helm_explorations
branch
from
272dcbf to
39948a0
Compare
OchiengEd
force-pushed
the
helm_explorations
branch
4 times, most recently
from
6dfc6c0 to
059008d
Compare
|
When pulling a Helm chart with a provenance file, at this time we have chosen to skip pulling the layer to the cache filesystem since we have no logic in place at this time to verify the chart integrity. operator-controller/internal/shared/util/image/helm.go Lines 62 to 65 in 059008d |
OchiengEd
requested review from
joelanford,
camilamacedo86,
trgeiger and
kevinrizza
OchiengEd
force-pushed
the
helm_explorations
branch
from
059008d to
797bddb
Compare
OchiengEd
force-pushed
the
helm_explorations
branch
from
b569fb8 to
9f59039
Compare
OchiengEd
force-pushed
the
helm_explorations
branch
from
9f59039 to
c08ce64
Compare
| ociImg, err := img.OCIConfig(ctx) | ||
| if err != nil { | ||
| return nil, time.Time{}, err | ||
| } | ||
|
|
||
| layerIter := iter.Seq[LayerData](func(yield func(LayerData) bool) { | ||
| for i, layerInfo := range img.LayerInfos() { | ||
| ld := LayerData{Index: i} | ||
| ld := LayerData{Index: i, MediaType: layerInfo.MediaType} |
There was a problem hiding this comment.
It is added with the helm chart, but I think it is OK to exist on the default code as well.
@tmshort @thetechnick WDYT?
OchiengEd
force-pushed
the
helm_explorations
branch
from
c08ce64 to
9b366d3
Compare
internal/shared/util/image/cache.go
Outdated
| 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) | ||
| } |
There was a problem hiding this comment.
Should it not be protected behind the feature flag as well?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
actually - the comment is below =P
There was a problem hiding this comment.
Learning moment for me here. How have we removed feature gated code that we have decided not to land in the past? Is using the git history to undo code that is associated with a feature gate sufficient or acceptable?
There was a problem hiding this comment.
I think it’d be helpful to keep all changes behind the feature gate clearly wrapped and centralized. That way, it’s easy to track what’s gated just by reading the code—since we’ll likely be iterating over multiple PRs, relying on Git history isn’t ideal.
Totally open to any approach that makes the boundary clear. The primary goal is to maintain cohesion and ensure we all share a common understanding of what lies behind the gate. 😊
There was a problem hiding this comment.
Understood. With that being said, from my perspective our options might be:
- Execute tests with feature gates enabled
- Extend the cache to allow tests to run even when the feature gate is disabled
- Don't wrap the Helm logic in the cache
Store()method which will allow tests to run even with feature gate disabled
Open to hear any other ideas or if we might have a preference for one of these options over others.
There was a problem hiding this comment.
@camilamacedo86 I was able to get a way to wrap the code and allow tests to run without issue. Hopefully that could suffice for now as we deliberate if to land the feature or not.
operator-controller/internal/shared/util/image/cache.go
Lines 137 to 141 in fb98756
| "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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I would be for adding documentation and a demo as a follow up.
OchiengEd
force-pushed
the
helm_explorations
branch
2 times, most recently
from
735a6d9 to
036e4c8
Compare
OchiengEd
force-pushed
the
helm_explorations
branch
from
036e4c8 to
0dfea34
Compare
| @@ -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) { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I do not have any concerns with having feature checks in main. I think that would be the ideal scenario as it would allow for feature-gated functionality to be tested at build time.
Trying to look at the issue from afar, our challenge in this case is we are trying to run tests on a feature gated function, pullChart() while we are not allowing a portion of the code in the cache's Store() method not to run since the feature gate is not enabled. The big question therefore is how do we navigate this quandary?
There was a problem hiding this comment.
I do not have any concerns with having feature checks in main. I think that would be the ideal scenario as it would allow for feature-gated functionality to be tested at build time.
We are working towards to it
See; #1980
We will use the experimental CRD and have all feature flags enabled for our e2e tests.
OchiengEd
force-pushed
the
helm_explorations
branch
2 times, most recently
from
fb98756 to
78069c9
Compare
internal/shared/util/image/cache.go
Outdated
| if layer.MediaType == registry.ChartLayerMediaType { | ||
| if features.OperatorControllerFeatureGate.Enabled(features.HelmChartSupport) || testing.Testing() { | ||
| if err := storeChartLayer(dest, layer); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } else { | ||
| if _, err := archive.Apply(ctx, dest, layer.Reader, applyOpts...); err != nil { | ||
| return fmt.Errorf("error applying layer[%d]: %w", layer.Index, err) | ||
| } |
There was a problem hiding this comment.
I may have mentioned this elsewhere. If I did, apologies for repeating myself...
I feel like a better way of handling this would be to register handlers for media types. Then this section of the code would lookup the media type of the layer and then call whatever handler is found (or error if there isn't one).
With that setup, the registration of the helm chart media type/handler would be feature gated, but this code would not.
There was a problem hiding this comment.
Code has been refactored. Hopefully I got your suggestion right.
OchiengEd
force-pushed
the
helm_explorations
branch
from
78069c9 to
742cb8a
Compare
* added support for deploying OCI helm charts which sits behind the HelmChartSupport feature gate * extend the Cache Store() method to allow storing of Helm charts * inspect chart archive contents * added MediaType to the LayerData struct Signed-off-by: Edmund Ochieng <ochienged@gmail.com>
OchiengEd
force-pushed
the
helm_explorations
branch
from
742cb8a to
46af89e
Compare
There was a problem hiding this comment.
Thank you a lot for your contribution 🥇
I’m OK with the changes 👍 Well done 🎉
/lgtm
IHMO: It is an initial implementation, and as follow-up, I think we need to do:
- Documentation (docs/draft) and a demo automated for this one
- A discussion around the remaining caveats, particularly how we plan to store the Helm chart bundle in the catalog via OPM
Would be great to get a PTAL from or at least one of them @joelanford, @thetechnick, and @perdasilva before get this one merged.
Thank you 🙌
|
Hi @OchiengEd, Just to clarify: In the F2F, we talked about using a single internal type and converting everything to it. We also discussed having only one runtime—Buxcutter. PoC: #1946 Supporting Helm fully will take time, and it is a big journey as we have been discussing. There are still open questions and blockers. If we go with this approach (internal format), we probably want OPM/OLM to support the internal type first before anything else. I created an internal Slack channel so that we can discuss all details and caveats. I’m OK merging this since it’s behind a feature gate—safe to experiment and doesn’t break anything. But it may change later. PS: If we get it merged, it would be great to have a small doc with automated demo steps (as done for others feature gates), so people can understand what was done and iterate Leaving final call to @joelanford, @thetechnick, and @perdasilva. 🔨 |
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