✨ Modify ClusterExtension to use Helm under the hood to apply contents into the cluster

[bentito]

Description

ClusterExtension reconciler needs to perform the following actions:

[See this doc for more detail if possible.]

1. Based on the cluster Extension spec, get the list of bundles from the catalog and perform resolution.

   • The resolution here need not use dep.py, it can be simple filtering and sorting as done in extension controller currently.
     1. Ref:Extension controller code
     2. Resolution:Extension controller resolution
     3. The resolution step involves fetching the installed version. Since there are no APIs (like BundleDeployment or App) present in clusters that reference the installed bundle, we need to investigate on listing the installed helm charts or use helm secrets. This also means that when the helm chart is created, we need to store metadata that uniquely identifies an extension.

2. Unpack the contents of the resolved bundle. For this iteration we can cover only registryV1 bundles from an image source.

   • Unpacker interface reference: Unpacker interface
   • Image unpacker: Image image unpacker
   • Instead of the BundleDeployment API we need to pass in the cluster extension directly.

3. Conversion of registryV1 to plain bundle.

   • Conversion logic: Convert registryV1

4. Creating a Helm chart from the resources present in plain bundle.

   • Plain bundle provisioner

5. Installing helm chart onto the cluster.

   • Helm chart deployer
   • We need to be able to inject metadata into the chart that will be required for resolution as mentioned in 1.(iii).
     • Currently, we run a post render hook, that adds labels on the objects created by the chart: Post render hook details. Will need to figure out on how to store this info in the helm secret or on the helm chart directory.

Reviewer Checklist

• API Go Documentation
• Tests: Unit Tests (and E2E Tests, if appropriate)
• Comprehensive Commit Messages
• Links to related GitHub Issue(s)

[netlify]

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	eafc251
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/6658a2435c1e0c00082a1028
😎 Deploy Preview	https://deploy-preview-846--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[codecov]

Codecov Report

Attention: Patch coverage is 63.88309% with 173 lines in your changes are missing coverage. Please review.

Project coverage is 75.38%. Comparing base (a458282) to head (60f4c80).
Report is 9 commits behind head on main.

Files	Patch %	Lines
...nternal/controllers/clusterextension_controller.go	62.02%	94 Missing and 26 partials ⚠️
internal/handler/registry.go	62.26%	10 Missing and 10 partials ⚠️
internal/controllers/clusterextension_status.go	45.71%	19 Missing ⚠️
cmd/manager/main.go	75.86%	9 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #846 +/- ##
==========================================
- Coverage 83.18% 75.38% -7.80% 
==========================================
Files 15 18 +3 Lines 874 1170 +296 ==========================================
+ Hits 727 882 +155 - Misses 102 208 +106 - Partials 45 80 +35 

Flag	Coverage Δ
e2e	56.06% <59.91%> (-6.18%)	⬇️
unit	50.60% <21.03%> (-24.33%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bentito]

@tmshort thanks for deconflicting main.go 👍

[m1kola]

I haven't reviewed the controller itself properly and some other bits. Some comments so far.

---

I think this PR too big to be adequately reviewed. Is there a way we can split this into multiple smaller PRs? I might be missing something, but it looks like unpacker is fairly independent and can be a separate PR. Then changes to goreleaser to make it buildable can be a separate PR. A bunch of helper functions can be moved into a separate PR, I think.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 68.35443% with 150 lines in your changes are missing coverage. Please review.

Project coverage is 77.21%. Comparing base (7727f8f) to head (eafc251).
Report is 2 commits behind head on main.

Files	Patch %	Lines
...nternal/controllers/clusterextension_controller.go	67.43%	75 Missing and 24 partials ⚠️
internal/handler/registry.go	62.26%	10 Missing and 10 partials ⚠️
internal/controllers/common_controller.go	58.53%	17 Missing ⚠️
cmd/manager/main.go	75.43%	9 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #846 +/- ##
==========================================
- Coverage 83.18% 77.21% -5.97% 
==========================================
Files 15 17 +2 Lines 874 1176 +302 ==========================================
+ Hits 727 908 +181 - Misses 102 189 +87 - Partials 45 79 +34 

Flag	Coverage Δ
e2e	59.86% <64.55%> (-2.38%)	⬇️
unit	58.52% <33.42%> (-16.42%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tmshort]

@everettraven@m1kola@acornett21 PTAL
We've rebased the PR and updated the unit tests, and will be going forward with this. Other significant changes to be done have been noted in various issues.

[everettraven]

Why did this go from a method to a global variable (package scoped)? Doing this opens us up to bugs if this variable is somehow overwritten. I would highly recommend avoiding this.

[everettraven]

Instead of using a global variable, I would recommend creating a new interface like:

typeInstalledBundleGetterinterface {
GetInstalledBundle(...) (...)
}

and make use of it via a field in the reconciler like:

typeReconcilerstruct {
...InstalledBundleGetterInstalledBundleGetter...
}
func (r*Reconciler) reconcile(...) (...) {
...installedBundle, err:=r.InstalledBundleGetter.GetInstalledBundle(...)
...
}

This has the ability to be mocked for testing while also preventing potential issues with using global variables

[varshaprasad96]

The reason for not creating an interface was GetInstalledBundle need not be exposed to the user in any way, it’s just for mocking purposes that we need to do this. If we are exposing it, then we may also have to expose the resolver logic and make it a separate interface.

Both of them were too much of a change, and since the var is inside an internal pkg, the chances of external users changing it are super minimal.

[everettraven]

Coming back around to this, something like:

typeReconcilerstruct {
GetInstalledBundleFuncfunc(...)(...)
}

might be even better due to it being an explicit type that I don't believe would be allowed to be set as nil, alleviating the need to do a potential validation check for a nil reference

[varshaprasad96]

Though not the best design for now, since it's internal and has no chances of external users breaking it - I'd suggest to do it in a follow up. Especially because it'll require a lot more bits of controller to be changed. An even better way would be for us to instead modify the way we have designed unit tests to mock better.

[everettraven]

And I think ^ would result in less work to change while getting the benefits of no longer using a global variable

[everettraven]

If you really prefer this being done in a follow up I won't stop this from going in, but this is something that I would think we want fixed very quickly. I don't think this is a good practice, even if it is in an internal package.

[bentito]

well I'm part-way through with the swap out to interface, just doing the mocks needed in the tests now. If you are okay with it in a soon to be added PR, can we can get an approve on this PR and merge?