search

vmware-tanzu / kubeapps

A web-based UI for deploying and managing applications in Kubernetes clusters
Other
4.94k stars 707 forks source link

Adapt the current Helm's OCI code to use the upstream version #4194

Open antgamdia opened 2 years ago

antgamdia commented 2 years ago

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:

antgamdia commented 2 years ago

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.

antgamdia commented 2 years ago

The 3.8.2 has been released, but it does not include our PR. It has been target to v3.9.0 😞

antgamdia commented 2 years ago

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
antgamdia commented 2 years ago

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

antgamdia commented 2 years ago

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.

antgamdia commented 2 years ago

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...

castelblanque commented 2 years ago

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.

antgamdia commented 2 years ago

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 :(

antgamdia commented 2 years ago

PR just approved!! https://github.com/helm/helm/pull/11129#pullrequestreview-1065422180

antgamdia commented 2 years ago

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.

antgamdia commented 2 years ago

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!

antgamdia commented 2 years ago

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.

antgamdia commented 2 years ago

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

antgamdia commented 1 year ago

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.
antgamdia commented 1 year ago

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.

absoludity commented 1 year ago

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).

antgamdia commented 12 months ago

v3.13 has been just released today!! 🎉 Finally!! https://github.com/helm/helm/releases/tag/v3.13.0

ppbaena commented 10 months ago

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.

antgamdia commented 10 months ago

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.

absoludity commented 10 months ago

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.

absoludity commented 10 months ago

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.

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.

antgamdia commented 7 months ago

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.