chore(oci): migrate to ORAS Golang library v2 #12310
Conversation
pull-request-size
bot
added
the
size/XL
zregvart
force-pushed
the
main
branch
3 times, most recently
from
7c04305
to
4255ba2
Compare
|
Thank you @Wwwsylvia for excellent comments and suggestions. |
|
Spent some time testing out the integration of the new ORAS v2 library. Below are the results
The migration looks good, but there does need to be additional work into the one validation failure scenario as described above in addition to the now conflicting |
|
@sabre1041 how would you like to handle the plain HTTP issue? With a |
|
@zregvart Personally, I would like to have feature parity with |
|
@sabre1041 added support |
|
@zregvart This looks good. Can you resolve the conflict and we should be in good shape |
zregvart
force-pushed
the
main
branch
2 times, most recently
from
a3cd03e
to
1ee13f3
Compare
|
Hi @zregvart @sabre1041, FYI, oras-go |
@zregvart if you want to update, I can retest again |
Odd, I cannot reproduce that:
|
This might be a flaky test. I managed to get the test failure on |
I ran these tests as well and they seem to pass. One test that is missing is the mTLS use case. I configured zot to enforce mTLS and managed to push and pull charts. I'm however not convinced that zot is enforcing mTLS fully cause I also managed to push charts without a client cert passed using |
|
I managed to run an mTLS test using this setup of nginx + zot. This PR changes work as expected. helm push ../playground/my-chart-1.0.0.tgz oci://127.0.0.1 --ca-file ca.crt --cert-file client.crt --key-file client.key
Pushed: 127.0.0.1/my-chart:1.0.0
Digest: sha256:e0cdbbf0ddd269b74abf748fba3e6ae80b7e5fe6d5c9285ad31c239bbd354b0aStrangely enough, the current version of helm push ../playground/my-chart-1.0.0.tgz oci://127.0.0.1 --ca-file ca.crt --cert-file client.crt --key-file client.key
Error: failed to do request: Head "http://127.0.0.1/v2/my-chart/blobs/sha256:0160276299adc66a534585cacf1af08d1dd3ac9c4eb6aa20d82af81deb40e67e": dial tcp 127.0.0.1:80: connect: connection refused
helm version
version.BuildInfo{Version:"v3.13.3", GitCommit:"c8b948945e52abba22ff885446a1486cb5fd3474", GitTreeState:"clean", GoVersion:"go1.21.5"} |
|
I is unclear to me what the expectation here is, i.e. could a release of Helm v4 be made with this or should the backward compatibility be maintained without exceptions. I think communicating the expectations would make this work a lot easier. I've made a judgement call and restored the backward compatibility in a62313e, now the |
|
@zregvart Thank you for keeping this PR up-to-date! 🙏🏻 |
zregvart
force-pushed
the
main
branch
2 times, most recently
from
05fe6fe
to
f974fac
Compare
| } | ||
| func ClientOptResolver(_ remotes.Resolver) ClientOption { | ||
| return func(c *Client) { | ||
| c.err = errDeprecatedRemote |
There was a problem hiding this comment.
This is still a breaking change and requires a Helm major version change. It's about taking functions and making them non-functional with an error.
There was a problem hiding this comment.
As previously noted support for containerd was removed from oras-go and there are no plans to reintroduce it. See oras-project/oras-go#64 (comment) for details.
I have yet to see any suggestions on how to get this addressed, no plans on the next major version of Helm either. Please do share any insights that you are privy to.
Frankly I do not see a way forward with this and will limit my effort going forward. Anyone is welcome to pick this up and continue the work.
There was a problem hiding this comment.
For the records, this client option:
- was recently added in 3.13.0 (under 6 months ago, and only two weeks before this PR was opened)
- was added without any real testing
- was in beta / RC1 for less than 10 days (3.13.0-rc.1 is identical to 3.13.0)
- was implemented by the same person that opened the issue
- has what seems like 1 known use across all of github's public codebases, barring matches that seem to be forks or vendored modules
- vmware-labs/distribution-tooling-for-helm does use the current implementation, but again, VMware was the team that opened the issue and implemented this option in helm.
- d2iq-labs/csi-driver-trusted-ca's usage is of it's own identically named option. This repository depends on helm 3.11.1 and has seemingly implemented #10623 itself
- has alternative solutions to what the original issue was asking for -- albeit yes, breaking
In terms of breaking changes, I think this is very manageable and the benefits of moving to oras v2 outweigh the costs of delaying this PR until the theoretical helm v4 release. I'm fairly sure that VMware team members @antgamdia or @alemorcuq would be okay with managing this change, as they are the source of this API in helm and still seem to be the only people affected.
There was a problem hiding this comment.
Hi @zevisert, thanks for the analysis on the current usage of this ClientOptResolver we added.
To give you all some context, we required this resolver to be configurable for use cases in Kubeapps (see vmware-tanzu/kubeapps#4194). However... due to different business reasons, we never get the bandwidth to actually make the changes on our side). We will probably bypass the Helm usage and will interact directly with ORAS, I guess.
I have checked with @alemorcuq and I think they can safely remove the usage of this option. In fact, I have just opened vmware-labs/distribution-tooling-for-helm#55 for doing so.
Both ClientOptPlainHTTP and ClientOptHTTPClient options cover their needs without the need of setting the resolver manually.
So, my (personal) two cents here: despite it being a breaking change, migrating to an up-to-date and maintained ORAS version and removing some CVEs is worthfile enough.
There was a problem hiding this comment.
Great analysis @zevisert! Just FYI https://github.com/d2iq-labs/csi-driver-trusted-ca is not maintained so does not need to be considered in any discussion.
I'm also very supportive of getting this change in.
There was a problem hiding this comment.
Hey @mattfarina, I understand if you're busy with other things, but we'd appreciate if you could come back here reconsider this sometime soon :)
zregvart
force-pushed
the
main
branch
2 times, most recently
from
dda9052
to
2f3bd30
Compare
Updates from `oras.land/oras-go` to `oras.land/oras-go/v2`. The main user facing change is that the `--plain-http` parameter is now required even when accessing OCI registries running on `localhost`. I'm new to this codebase and some tests needed to be adjusted to match the new dependency, please look over the test changes to see if there are any issues with those. Fixes helm#11821 Signed-off-by: Zoran Regvart <zoran@regvart.com>
This restores the `ClientOptResolver` function which now panics on invocation and previous signature of the `LoginOption` function. Signed-off-by: Zoran Regvart <zoran@regvart.com>
What this PR does / why we need it:
Updates from
oras.land/oras-gotooras.land/oras-go/v2. The main user facing change is that the--plain-httpparameter is now required even when accessing OCI registries running onlocalhost.Fixes #11821
Special notes for your reviewer:
I'm new to this codebase and some tests needed to be adjusted to match the new dependency, please look over the test changes to see if there are any issues with those.
If applicable: