Implement `helm pull` for OCI registries #8843

pmengelbert · 2020-10-02T23:55:17Z

This PR is in line with the HIP for OCI Proposal here: helm/community#136

New helm pull does not depend on registry cache
Pull OCI-stored chart with helm pull oci://<registryURL> --version <tag>
Implement helm dep update for oci dependencies, in this format:

dependencies
  - name: ocichart
    version: 0.1.0
    repository: "oci://localhost:3000/myregistry"
# equivalent to "oci://localhost:3000/myregistry/ocichart:0.1.0

New unit tests for helm pull and helm dep update
Implement OCIGetter implementation of Getter interface

Signed-off-by: Peter Engelbert pmengelbert@gmail.com

What this PR does / why we need it:

Special notes for your reviewer:

If applicable:

this PR contains documentation
this PR contains unit tests
this PR has been tested for backwards compatibility

go.sum

internal/experimental/registry/client.go

internal/resolver/resolver.go

pkg/action/pull.go

pkg/downloader/manager.go

pkg/repo/repotest/server.go

bacongobbler · 2020-10-05T20:02:44Z

Hi @pmengelbert and @jdolitsky. Have you reached out to @phroggyy on this discussion topic? Based on his last comment it did appear that he was still interested in pursuing this effort. I'm curious why another competing PR is being proposed here instead of working together with @phroggyy.

jdolitsky · 2020-10-05T21:44:38Z

No ill intentions here - just trying to move things forward based on the new proposal. (Let it be known that I had approved those changes back in February)

I've reached out to Leo in Slack. and will wait for response. We're happy to drop this PR if that one is updated to reflect the proposal.

jdolitsky

Please see comment regarding the OCI boolean, should not be needed

phoenix-bjoern · 2020-10-06T04:10:08Z

If this feature is removed again from roadmap for the next release because of „new considerations“ I am going nuts. We are waiting for progress on the OCI support for almost a year and „new options“ simply show that people are tired of waiting.
Please make your choice and move on. There are more things to solve like recursive dependencies but without some progress we get really stuck here.
Thanks.

phroggyy · 2020-10-06T04:21:22Z

@bacongobbler to clear up on my end: I have no issues with this going forward. I did not see your link to the HIP in my PR until I read Josh's message today, and so even without checking the code, I'd assume this is a lot closer to done based on that spec. As I said to Josh in DM, if any effort should "go to waste" it should be the older, more faulty one, imho.

phroggyy · 2020-10-06T05:15:24Z

@phoenix-bjoern I'd consider this a massive step forwards! Sure, it's frustrating that it's been open for so long, but let's keep in mind that there was no spec for this back in #7613. Then a spec (HIP) was written, and now there's a compliant implementation – this to me seems like a massive step forwards and I would believe brings us a lot closer to getting this feature in!

phoenix-bjoern · 2020-10-06T07:30:57Z

@phroggyy thanks for your comment and all the efforts. Any implementation is fine for us as long as it means we do not have to wait another 3 months till OCI dependencies are supported in a GA version.

bacongobbler · 2020-10-06T15:58:27Z

@phoenix-bjoern while I understand the frustration, I want to make things clear... I was asking to make sure that @phroggyy was okay with this PR. Since he's put in the most time into this, we want to make sure that credit to the original author is due. If he's okay with @pmengelbert moving this forward (as he's done so), then that's fine with me. It's important to us to make sure that the original author is given credit when they put in the work.

On timelines: significant features to a stable project take time, and need careful consideration. This undertaking is replacing a significant part of Helm's architecture, so it's not unusual for this to take as long as it has. It's worth keeping in mind how HUGE the Helm community is, and we need to factor that in when we're asking our entire community to migrate to a new system.

Even after this PR is merged, it is highly unlikely this feature will be marked as stable. If you read the proposed HIP 6 at helm/community#136, we still need the OCI to approve the OCI Artifacts proposal until we can mark this as a stable feature. Please refer to Section 7: "Experimental until clear messaging from OCI" for more information.

If I were to assume a timeline, I wouldn't be expecting this to be marked as a stable feature until Helm 4. We've been waiting for months for the OCI to approve the Artifacts proposal. It's unlikely to be approved any time soon.

Hope that clears things up.

phoenix-bjoern · 2020-10-16T14:09:03Z

@bacongobbler the OCI feature is marked as experimental, clear. The OCI dependency issue, which I've created in #6593, has celebrated already it's first birthday ;-)
We now have two(!) working solutions. If everyone favors this implementation, could you at least agree to add this issue to the milestone list for 3.4.0? @phroggyy 's implementation in #7613 was planed for 3.2, 3.3 and 3.4, where it has been removed lately. So I just kindly ask, also for other folks who voted for this feature already in my initial issue, for some progress in this OCI functionality in order to be usable without additional workarounds/commands. Thank you.

bacongobbler · 2020-10-16T14:21:35Z

We've been focused on the Helm 2 deprecation, which happens in less than a month's time from now. Any Helm 3 work has been put on the side-burner for the time being while we support those folks still migrating to Helm 3. Thanks for your patience.

jdolitsky · 2020-10-28T19:37:04Z

FYI - The HIP related to this change has been accepted. Please see https://github.com/helm/community/blob/master/hips/hip-0006.md

WyriHaximus

Tested this out locally and it worked fine, aside from a file deletion error after the first time installing a chart.

One question though. Will this support semver versions ranges such as ^1.2 in the future? So we don't have to update the version we want by hand

jdolitsky · 2020-11-17T15:17:29Z

@WyriHaximus can you expand on "file deletion error after the first time installing a chart" ? The support for version ranges might be supported in the future if we are able to query the registry for available versions (tag listing).

WyriHaximus · 2020-11-17T18:22:55Z

@WyriHaximus can you expand on "file deletion error after the first time installing a chart" ?

@jdolitsky the exact error is:

Error: unable to move current charts to tmp dir: link error: cannot rename charts to tmpcharts: rename charts tmpcharts: file exists

The support for version ranges might be supported in the future if we are able to query the registry for available versions (tag listing).

Was expecting as such, don't know OCI well enough to know if this is build in. But from a user perspective this would be very welcome and put it at the same level of use as the old skool index.yaml based repositories.

jdolitsky · 2020-11-18T15:44:38Z

The "tmpcharts" piece seems unrelated:

helm/pkg/downloader/manager.go

Line 247 in af2d302

tmpPath := filepath.Join(m.ChartPath, "tmpcharts")

Unsure if this is an existing bug or specific to this change

jdolitsky · 2020-11-18T15:46:07Z

Seems people have had this issue in the past @WyriHaximus https://github.com/helm/helm/issues?q=is%3Aissue+cannot+rename+charts+to+tmpcharts+is%3Aclosed

WyriHaximus · 2020-11-19T12:51:57Z

@jdolitsky Looks like I had some naming crossed inside the OCI repository. But name made sure that the chart name is be found under oci://repository.host.name as oci://repository.host.name/helm-oci-test and a tag as oci://repository.host.name/helm-oci-test:0.5.0.

Getting this error now:

Error: unable to move local charts to charts dir: link error: cannot rename tmpcharts/helm-oci-test to charts/helm-oci-test: rename tmpcharts/helm-oci-test charts/helm-oci-test: file exists

When running this command:

~/Projects/Helm/helm/bin/helm dependency update

Now this happens as long as the charts/helm-oci-test directory exists. When I remove charts/helm-oci-test but leave charts present the error becomes:

Error: unable to move current charts to tmp dir: link error: cannot rename charts to tmpcharts: rename charts tmpcharts: file exists

Not sure how helm does this internally but it seems to expect that he charts directory is removed before putting the just pulled versions in place. Not sure if it's related to this change, but going to test this now on a chart that uses the chartmuseum way of helm charts available to see if it also happens there.

WyriHaximus · 2020-11-19T12:57:42Z

but going to test this now on a chart that uses the chartmuseum way of helm charts available to see if it also happens there.

@jdolitsky Ok just tested this and there is no issue with the helm I build from this PR using charts from a chartmuseum kind of repository

cobexer · 2020-12-01T11:21:15Z

There is an important loss of functionality here compared to old repositories:
oci://localhost:3000/myregistry may be a valid OCI registry in a CI/Test system, but production may want to store it's helm charts in something like: oci://registry.prod.example.com/myregistry - while our customer may want to store our Helm Charts in their own internal OCI registry...

This syntax needs a way of saying "in the same repository/registry as the parent chart" or "in any of the configured repositories/registries"

Am I missing something?

jdolitsky · 2020-12-01T14:51:33Z

@cobexer - you're asking for symbolic key (such as @myrepo), which is dynamic depending on environment?

In any case - I don't believe this PR does not prevent that feature being added in the future.

If you want, please make PR to modify this document: https://github.com/helm/community/blob/master/hips/hip-0006.md

darren-bell-nanthealth · 2020-12-07T12:39:19Z

Hi. Is there a potential release date for this feature? I'm hoping to use it for a DR scenario. Thanks

* Implement `helm dep update` for oci dependencies * New unit tests * Remove `helm chart pull` command * New `helm pull` does not depend on registry cache Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Additionally, revert `NewPull()` to its existing signature. Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

mattfarina

When I tried to run helm dep up (my local chart has a dependency on a chart in an OCI repo) more than once I got the following error if the chart already existed in the target location...

Error: unable to move local charts to charts dir: link error: cannot rename tmpcharts/test-chart to charts/test-chart: rename tmpcharts/test-chart charts/test-chart: file exists

Shouldn't it remove the old version and replace it with the new version?

I also noticed that oci:// works even when the OCI experiment isn't enabled for dependencies in the Chart.yaml file. Is this intentional?

I've not completed my whole review but I wanted to pass on the couple issues and question I had right away.

mattfarina · 2020-12-14T16:36:21Z

pkg/getter/getter_test.go

@@ -57,7 +57,7 @@ func TestAll(t *testing.T) {
 	env.PluginsDirectory = pluginDir

 	all := All(env)
-	if len(all) != 3 {
+	if len(all) != 4 {


While the length changed the error message below it was not updated to the new situation.

I've not completed my whole review but I wanted to pass on the couple issues and question I had right away.

@mattfarina Thanks so much for the review. I just pushed a commit that fixes the bug you pointed out and gives the proper error message in getter_test.go. Let me know what you think of the changes, and I look forward to the rest of your review!

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

mattfarina · 2021-01-05T20:21:04Z

internal/experimental/registry/client.go

@@ -144,7 +146,60 @@ func (c *Client) PushChart(ref *Reference) error {
 }

 // PullChart downloads a chart from a registry
-func (c *Client) PullChart(ref *Reference) error {
+func (c *Client) PullChart(ref *Reference) (*bytes.Buffer, error) {


Note to reviewers... this function is internal and part of the experiment. It's ok to change it.

technosophos

This looks to me like a clear step forward on the experimental OCI front. I am good with merging/shipping this iteration.

pmengelbert · 2021-01-05T20:43:34Z

@mattfarina would you do us a favor and mark the requested changes as resolved? Once that's taken care of we can finally get this merged! Really happy to see this moving forward

technosophos · 2021-01-05T20:44:33Z

@pmengelbert we're actually both working on it right now

helm-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 2, 2020

jdolitsky requested changes Oct 5, 2020

View reviewed changes

mattfarina mentioned this pull request Oct 5, 2020

Add support for using OCI references as the dependency repository #7613

Closed

pmengelbert mentioned this pull request Oct 5, 2020

HIP for OCI support helm/community#136

Merged

jdolitsky approved these changes Oct 5, 2020

View reviewed changes

jdolitsky requested changes Oct 6, 2020

View reviewed changes

bacongobbler mentioned this pull request Oct 6, 2020

Extend Getter Options with context.Context #8551

Closed

3 tasks

jdolitsky approved these changes Oct 8, 2020

View reviewed changes

bergerx mentioned this pull request Oct 28, 2020

Azure Container Registries now supports Helm repos Praqma/helmsman#99

Closed

bacongobbler mentioned this pull request Oct 30, 2020

I want be able to install charts from the Azure Container Registry #8962

Closed

tegioz mentioned this pull request Nov 6, 2020

Add experimental OCI support for Helm repositories artifacthub/hub#830

Merged

WyriHaximus approved these changes Nov 17, 2020

View reviewed changes

pmengelbert added 3 commits December 11, 2020 21:44

Implement helm pull for OCI registries

3ad08f3

* Implement `helm dep update` for oci dependencies * New unit tests * Remove `helm chart pull` command * New `helm pull` does not depend on registry cache Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Clean up imports and add doc comments

3028c55

Additionally, revert `NewPull()` to its existing signature. Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Remove OCI boolean from struct

f666fce

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

pmengelbert force-pushed the oci-pull branch from d804ebe to f666fce Compare December 12, 2020 03:46

jdolitsky added this to the 3.5.0 milestone Dec 14, 2020

mattfarina self-requested a review December 14, 2020 16:12

jdolitsky approved these changes Dec 14, 2020

View reviewed changes

mattfarina requested changes Dec 14, 2020

View reviewed changes

Address error on deletion of old dependencies

beda5e1

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

ErikUmhoe mentioned this pull request Jan 5, 2021

Can't use helm chart from an Azure container registry as dependency chart MicrosoftDocs/azure-docs#68351

Closed

mattfarina reviewed Jan 5, 2021

View reviewed changes

technosophos approved these changes Jan 5, 2021

View reviewed changes

mattfarina self-requested a review January 5, 2021 20:55

mattfarina approved these changes Jan 5, 2021

View reviewed changes

mattfarina merged commit 49f895d into helm:master Jan 5, 2021

bacongobbler mentioned this pull request Jan 5, 2021

Helm3: Use registry as dependency URL #6593

Closed

mattfarina mentioned this pull request Jan 6, 2021

helm dep build fails with oci:// #9199

Closed

lebenitza mentioned this pull request Jan 13, 2021

Source controller resource for AWS ECR helm registry fluxcd/source-controller#250

Closed

This was referenced Feb 20, 2021

Install chart from registry #6982

Closed

initialize registry client in oci getter #9409

Merged

bacongobbler mentioned this pull request Mar 31, 2021

OCI: Segfault on helm show chart #9515

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `helm pull` for OCI registries #8843

Implement `helm pull` for OCI registries #8843

pmengelbert commented Oct 2, 2020 •

edited

Loading

bacongobbler commented Oct 5, 2020

jdolitsky commented Oct 5, 2020

jdolitsky left a comment

phoenix-bjoern commented Oct 6, 2020

phroggyy commented Oct 6, 2020

phroggyy commented Oct 6, 2020

phoenix-bjoern commented Oct 6, 2020 •

edited

Loading

bacongobbler commented Oct 6, 2020 •

edited

Loading

phoenix-bjoern commented Oct 16, 2020

bacongobbler commented Oct 16, 2020

jdolitsky commented Oct 28, 2020

WyriHaximus left a comment

jdolitsky commented Nov 17, 2020

WyriHaximus commented Nov 17, 2020

jdolitsky commented Nov 18, 2020

jdolitsky commented Nov 18, 2020

WyriHaximus commented Nov 19, 2020

WyriHaximus commented Nov 19, 2020

cobexer commented Dec 1, 2020

jdolitsky commented Dec 1, 2020

darren-bell-nanthealth commented Dec 7, 2020

mattfarina left a comment

mattfarina Dec 14, 2020

pmengelbert Dec 18, 2020

mattfarina Jan 5, 2021

technosophos left a comment

pmengelbert commented Jan 5, 2021

technosophos commented Jan 5, 2021

Implement helm pull for OCI registries #8843

Implement helm pull for OCI registries #8843

Conversation

pmengelbert commented Oct 2, 2020 • edited Loading

bacongobbler commented Oct 5, 2020

jdolitsky commented Oct 5, 2020

jdolitsky left a comment

Choose a reason for hiding this comment

phoenix-bjoern commented Oct 6, 2020

phroggyy commented Oct 6, 2020

phroggyy commented Oct 6, 2020

phoenix-bjoern commented Oct 6, 2020 • edited Loading

bacongobbler commented Oct 6, 2020 • edited Loading

phoenix-bjoern commented Oct 16, 2020

bacongobbler commented Oct 16, 2020

jdolitsky commented Oct 28, 2020

WyriHaximus left a comment

Choose a reason for hiding this comment

jdolitsky commented Nov 17, 2020

WyriHaximus commented Nov 17, 2020

jdolitsky commented Nov 18, 2020

jdolitsky commented Nov 18, 2020

WyriHaximus commented Nov 19, 2020

WyriHaximus commented Nov 19, 2020

cobexer commented Dec 1, 2020

jdolitsky commented Dec 1, 2020

darren-bell-nanthealth commented Dec 7, 2020

mattfarina left a comment

Choose a reason for hiding this comment

mattfarina Dec 14, 2020

Choose a reason for hiding this comment

pmengelbert Dec 18, 2020

Choose a reason for hiding this comment

mattfarina Jan 5, 2021

Choose a reason for hiding this comment

technosophos left a comment

Choose a reason for hiding this comment

pmengelbert commented Jan 5, 2021

technosophos commented Jan 5, 2021

Implement `helm pull` for OCI registries #8843

Implement `helm pull` for OCI registries #8843

pmengelbert commented Oct 2, 2020 •

edited

Loading

phoenix-bjoern commented Oct 6, 2020 •

edited

Loading

bacongobbler commented Oct 6, 2020 •

edited

Loading