kubeapps
kubeapps copied to clipboard
vmware-tanzu
Adapt the current Helm's OCI code to use the upstream version
After the discussion held in https://github.com/kubeapps/kubeapps/pull/4154#issuecomment-1020460749, we agree that we should migrate to Helm 3.8.X, but their current interface to consume the OCI registry client is not enough for us.
This issue is to track the two required steps:
- [x] Make a PR in the Helm repository (related issue: https://github.com/helm/helm/issues/10623)
- [x] Adapt our code to the new Helm 3.8.X release.
- [x] Wait until they release a version including our PR
- [ ] Re-apply the changes in the linked PR manually with the now refactored code.
Helm folks have suggested some minor changes (mostly just targeting another ongoing PR). Ideally, it would be ready for the 3.8.1 release due on March 9th, so I'll perform those changes next week.
3.9.0 is planned for May 11th, but they have tagged an rc1 and our changes have not been included. We are still blocked by https://github.com/helm/helm/pull/10408. So the update is... there are no updates :(
3.9.1 will contain only bug fixes and will be released on June 8, 2022
3.10.0 is the next feature release and will be released on September 14, 2022
Note that Helm 3.9.X includes some breaking changes with regards to the k8s version it uses in its deps.
More info: https://github.com/vmware-tanzu/kubeapps/pull/4743
I've taken a look at the current status and... the PR was closed without a merge. However, I've noticed they added a way to set bearer tokens (just passing a blank username). Perhaps this change is enough for what we aimed to do, adding to the next interaction discussion.
After a second analysis, we can't leverage from what I said. We are using the resolver and other fields for both production and testing codes. An alternative idea is to use reflection to unsafely set fields... but it is not a proper choice.
I have created another PR with the initially proposed approach, so that our PR does no longer depend on the tls flags one. Let's see if I get better feedback there: https://github.com/helm/helm/pull/11129
Another idea: get rid of Helm for OCI... and just use the underlying ORAS support directly (although it is mostly what we are doing right now: using the Helm's code directly). So, let's wait and push forward this new PR I've sent...
An alternative idea is to use reflection to unsafely set fields... but it is not a proper choice.
IMHO Reflection is tricky and we better avoid it. Not initially, but down the road it will give us headaches for maintenance.
Let's see if I get better feedback there: https://github.com/helm/helm/pull/11129
Great, I hope it gets approved and merged!
Another idea: get rid of Helm for OCI.
+1 to the idea. This way OCI support wouldn't be tied to Helm lifecycle. Agree to wait for your PR to get approved, easier solution probably.
It seems more people are interested in setting a custom resolver, or, at least, modifying some of its default options: https://github.com/helm/helm/pull/10408#discussion_r919329730, but still I haven't heard back from the maintainers :(
PR just approved!! https://github.com/helm/helm/pull/11129#pullrequestreview-1065422180
After Kapp released a new version, we should be able to upgrade to the Helm latest version. This should be the first step, then this issue will get blocked again, until they release aversion with the approved fix.
Currently, our deps have been successfully updated and helm finally was upgraded to v3.9.4., which will ease the upgrade path to subsequent versions.
According to their release process, 3.10.0 is the next feature release and will be on September 14, 2022, which could mean that our (accepted, but not merged yet) PR may be included. Let's stay tuned!
3.10.0-rc.1 has been released without our PR. I've pinged Helm folks to see if we could get it merged for the next rc release.
Helm 3.10 has been finally released, but our PR is still blocked. Apparently, they have several OCI-related PRs stuck, some of them with some overlap... meaning we will have to wait even more time :(
Edit: See cmd\kubeapps-apis\plugins\fluxv2\packages\v1alpha1\integration_utils_test.go, the copyOptions.Run() function now requires no parameters.
More info at: https://github.com/helm/helm/issues/11352
Some updates: they have requested me to rebase my PR with the latest changes! We are closer to getting it merged. Keep you posted!
3.12.3 is the next patch/bug fix release and will be on August 9, 2023.
3.13.0 is the next feature release and be on September 13, 2023.
And... they merged the PR today!! I assume next September release will include the changes. Unfortunately, we are still blocked by the k8s 1.27 dependencies, so perhaps we won't be able to upgrade it straight away.
Excellent. I assume we'll be able to update to the k8s 1.27 deps soon though (haven't checked again in the past week or two, but it looked like a temporary situation at the time, which is affecting lots of projects).
v3.13 has been just released today!! 🎉 Finally!! https://github.com/helm/helm/releases/tag/v3.13.0
Hi Antonio, what should be the next steps in this issue? Maybe now that Helm released a version including our PR we should use it. I remember you were working on it, but don't know the status and if PR is ready to be reviewed. Could you give me some info about next steps? Thanks.
Sure thing! Most of the required changes are at https://github.com/vmware-tanzu/kubeapps/pull/4216 (it used the code from my fork). However, as I'm a bit disconnected working on different projects, I don't know the current status of the relevant pieces of code. Since we have been refactoring the OCI thing, perhaps it got refactored somehow and this is no longer required. I don't know.
The main point of this issue is that we were using pieces of code from the Helm codebase instead of just importing it as a dependency. However, I don't really know if it is really true nowadays.
I'm happy to go through the original PR and recent changes to merge what's relevant in. When doing the OCI work I've noted a number of places where I'd want to replace our custom code to instead use the oras client (which helm also uses), but only updated the parts that I was touching. I didn't change existing working code, and pkg/helm/helm.go is still there, and the ChartPuller helper it provides is still used, so I'm assuming the PR is still very relevant.
I'm happy to go through the original PR and recent changes to merge what's relevant in. When doing the OCI work I've noted a number of places where I'd want to replace our custom code to instead use the oras client (which helm also uses), but only updated the parts that I was touching. I didn't change existing working code, and
pkg/helm/helm.gois still there, and theChartPullerhelper it provides is still used, so I'm assuming the PR is still very relevant.
Took a quick look this morning and yes, there's enough changes (including file deletions, code refactors) that I'll need to manually start a new branch and apply the changes in the PR piece-meal and test. I'm going to move this issue into TODO and pick it up from there once the current OCI metadata work is completed.
It seems the change we introduced for this (the ClientOptResolver option in the registry package) is blocking the migration from ORAS v1 to v2 (see https://github.com/helm/helm/pull/12310#discussion_r1501065719).
(Un)fortunatelly, we didn't actually get to use it for different reasons... so I believe it is safe for them to go ahead and remove the registry option. Later on, we can think about alternative solutions (like just moving to ORAS directly, as already suggested in this issue).
I'm gonna check with some colleagues using this option we implemented at distribution-tooling-for-helm before replying to he helm folks.