upgrade oras to 0.5.0, refactor client oci logic to use new oras.Copy()

[joshrwolf]

What this PR does / why we need it:

Updates oras dependency to 0.5.0, using the new oras.Copy() api instead of the deprecated oras.Push() and oras.Pull().

Special notes for your reviewer:

This is still a WIP. Currently investigating why layers are being pushed out of order, resulting in failing tests:

Expected :{"schemaVersion":2,"config":{"mediaType":"application/vnd.cncf.helm.config.v1+json","digest":"sha256:8d17cb6bf6ccd8c29aace9a658495cbd5e2e87fc267876e86117c7db681c9580","size":99},"layers":[{"mediaType":"application/vnd.cncf.helm.chart.content.v1.tar+gzip","digest":"sha256:e5ef611620fb97704d8751c16bab17fedb68883bfb0edc76f78a70e9173f9b55","size":973},{"mediaType":"application/vnd.cncf.helm.chart.provenance.v1.prov","digest":"sha256:b0a02b7412f78ae93324d48df8fcc316d8482e5ad7827b5b238657a29a22f256","size":695}]}
Actual   :{"schemaVersion":2,"config":{"mediaType":"application/vnd.cncf.helm.config.v1+json","digest":"sha256:8d17cb6bf6ccd8c29aace9a658495cbd5e2e87fc267876e86117c7db681c9580","size":99},"layers":[{"mediaType":"application/vnd.cncf.helm.chart.provenance.v1.prov","digest":"sha256:b0a02b7412f78ae93324d48df8fcc316d8482e5ad7827b5b238657a29a22f256","size":695},{"mediaType":"application/vnd.cncf.helm.chart.content.v1.tar+gzip","digest":"sha256:e5ef611620fb97704d8751c16bab17fedb68883bfb0edc76f78a70e9173f9b55","size":973}]}

If applicable:

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

[jdolitsky]

I believe the source of the reordering is this helper method (see "sort.Slice"): https://github.com/oras-project/oras-go/blob/0b0339cc7dfd19cc65416d0a034ea1f6abb05a2a/pkg/content/manifest.go#L72

I suppose the order of the layers may not be significant, and maybe we just modify the test... but wondering if we ought to add an option in oras to respect the order provided cc @deitch

[scottrigby]

• An older issue Revisit the Design of ORAS oras-project/oras#304 mentions "ambiguous ordering" as a major drawback of Simplify ORAS default experience oras-project/oras#178 (comment)

Don't want to expand the scope of this PR. Noting only in the context of:

I suppose the order of the layers may not be significant

As we're moving toward a backwards compatibility contract, are there any factors to keep in mind when deciding how layer ordering should be handled?

Edit: for the scope of this PR, the ordinality question is only relevant to how to handle the failing test.

[deitch]

Yeah it is that. The order of leaf nodes (including layers) never is supposed to be significant. Sorting lets us be consistent about it, not just for tests, but for lots of things. If there is a use case where it is significant, let's lay it out here and address it.

The ordering of leafs vs branches (e.g. manifests) is significant, as many registries will not allow a manifest whose leafs are not entirely in the registry already.

[jdolitsky]

Thanks @deitch

@joshrwolf - can we just update the test and take this out of draft mode? I will raise this issue with maintainers

[bacongobbler]

Are there tests in place for pulling/pushing docker images with oras 0.5.0? While this won't affect Helm (we perform lookups for things like the signature or manifest based on content-type), I'm curious if this might affect users pulling and pushing docker images using ORAS due to the nature of the copy-on-write filesystem. I think the docker manifest specifies the layer ordering instead of the OCI manifest, but I thought I'd check and make sure :)

I know some community members have asked whether we can eventually add support for both docker images and helm charts being co-located, so that you can pull down a chart as well as all images it requires, so it is possible this could become a concern (much) further down the road.

[sajayantony]

As we're moving toward a backwards compatibility contract, are there any factors to keep in mind when deciding how layer ordering should be handled?

@scottrigby - If this doesn't affect moving a helm chart between registries or repositories and the content digest remains the same then there should be no concern. Or does it?

[joshrwolf]

Updated the test to pass by using the newly ordered manifests digest. It feels weird to change a test to make the code work, but hopefully the justification is clear.

for completeness: the only chance is the manifests layer ordering (was: [chartdata, prov], is: [prov, chartdata) due to the newly added sort method in oras pointed out above by @jdolitsky (ref). This changes the manifest's bytes which changes it's expected digest, but the manifests contents are the same, made clear by the config, chartdata, and prov digests all staying the same

In the future it might make more sense to make the test assert the struct contents of result rather than the resulting digest.

[scottrigby]

@joshrwolf That sounds right. For everyone else, I also got to pair with Josh a bit on this and saw that this was in fact not just changing the test expectations to match "bad code" lol, but truly reordering the expected results to match the order that the new ORAS helper function should always now provide.

[jdolitsky]

@joshrwolf thank you much for updating and for the explanation. Can you please fix the DCO check?

[jdolitsky]

@joshrwolf - also, please fix test make test-style

[joshrwolf]

@jdolitsky fixed the DCO check and the results from make test-style that were related the the code this PR changed, but there's still a handful failing that are outside the scope. Let me know if there's anything else!

[jdolitsky]

@bacongobbler - do you know how we should address lint errors due to updated dependencies?

Run make test-style
GO111MODULE=on golangci-lint run
cmd/helm/registry_login.go:28:2: SA1019: package github.com/docker/docker/pkg/term is deprecated: use github.com/moby/term instead (staticcheck)
	"github.com/docker/docker/pkg/term"
	^

This is unrelated to the change, seems only due to updating go.mod/go.sum.

[jdolitsky]

Considering we can fix the linting issue, this change LGTM. Nice work

[shizhMSFT]

@jdolitsky I have a concern on the ordering in the future helm development.

Currently, helm uses the unique media types to identify the content:

• application/vnd.cncf.helm.chart.content.v1.tar+gzip
• application/vnd.cncf.helm.chart.provenance.v1.prov

What if we have two layers having the same media type? What if we have one chart package but multiple provenance files from different parties?

[jdolitsky]

@shizhMSFT independent of the order of layers, we can simply find layers matching application/vnd.cncf.helm.chart.provenance.v1.prov (if multiple prov was supported).

If we were to find more than one layer with application/vnd.cncf.helm.chart.content.v1.tar+gzip, we can treat this as an error.

2 .tgz -> error
1+ .prov, no .tgz -> error

I'm still unclear on why the order might be significant. Is looking at layer index[0] and layer index[1] any more secure?

[shizhMSFT]

@jdolitsky Thanks for clarifying how helm handles the artifacts. It makes more sense to me now.

[jdolitsky]

Please see 889c70b which fixed CI

This is a bit of a bandaid, but some of these errors are completely unrelated to this change (for example golang.org/x/crypto/openpgp).

Once this PR is merged, I can open an issue to remove the nolint lines if possible.

[scottrigby]

Linting fixed! lgtm 👍

@joshrwolf nicely done 👏
Ah yes @jdolitsky, thank you! Go 1.16 vs Go 1.17 🙃
Also thanks to @hickeyma for #10347, which helped lead to discovering the linting problem here.
Also thanks to the feedback on implications of this update for helm and ORAS 🤝
Team effort! 💖