feat: migrate zarf tools registry ls from crane to oras-go - #5199
feat: migrate zarf tools registry ls from crane to oras-go#5199abhishekgit03 wants to merge 4 commits into
zarf tools registry ls from crane to oras-go#5199Conversation
Signed-off-by: Abhishek Dasgupta <abhishek20dgp@gmail.com>
✅ Deploy Preview for zarf-docs canceled.
|
|
This is a personal question, but do you mind expanding on this?
|
Codecov Report❌ Patch coverage is
... and 28 files with indirect coverage changes 🚀 New features to boost your workflow:
|
image layers used to always be gzipped tar archives (a filesystem diff), so Crane's layer-handling code assumes that. The OCI spec has since moved on, a manifest's layers can now be arbitrary blobs (WASM binaries, SBOMs, signatures, anything), not just tar diffs. Crane never updated that assumption, so it breaks when it tries to read non-tar content as if it were a tar stream. |
Thank you very much for that description! |
|
Hi @AustinAbro321 @brandtkeller can you have a look at this when you get a chance? let me know if the approach is fine. |
AustinAbro321
left a comment
There was a problem hiding this comment.
Overall design looks good, a few comments on keeping parity.
| // Credential must be keyed to the host ORAS actually connects to: the tunnel when tunneling, otherwise the registry's own address. | ||
| credentialHost := s.RegistryInfo.Address | ||
| if tunnel != nil { | ||
| credentialHost = endpoint |
There was a problem hiding this comment.
The credentialHost should be set from a function like below which will take the conn.Ref.
func registryHost(repoRef string) (string, error) {
ref, err := registry.ParseReference(repoRef)
if err != nil {
return "", fmt.Errorf("parsing repo %q: %w", repoRef, err)
}
return ref.Host(), nil
}There was a problem hiding this comment.
fixed by using registryHost() method.
| client.Client.Transport = t | ||
| } | ||
|
|
||
| plainHTTP, err := s.RegistryInfo.ResolvePlainHTTP(ctx, credentialHost, false, ocischeme.ProbeOptions{InsecureSkipTLSVerify: insecure}) |
There was a problem hiding this comment.
In crane --insecure means both --plain-http and --insecure-tls-skip-verify. I'd like to get rid of this design as it overloads a flag, but I don't want to have a breaking change without a deprecation process.
Deprecate the insecure flag (for this command specifically), and add the --plain-http and --insecure-skip-tls-verify flags to zarf tools registry ls. As we go through other commands, we'll follow the same process.
There was a problem hiding this comment.
done. added --plain-http and --insecure-skip-tls-verify
…tion and HTTP usage Signed-off-by: Abhishek Dasgupta <abhishek20dgp@gmail.com>
Signed-off-by: Abhishek Dasgupta <abhishek20dgp@gmail.com>
Signed-off-by: Abhishek Dasgupta <abhishek20dgp@gmail.com>
AustinAbro321
left a comment
There was a problem hiding this comment.
Noticed one more regression, overall looks good
There was a problem hiding this comment.
I noticed one more regression where docker.io is not the default registry after this change. For instance, on main, zarf tools registry ls stefanprodan/podinfo:6.4.0 will work, but it will fail on this branch.
You should be able to use reference.ParseNormalizedNamed for this, we use similar functions in other spots in the repository.
Related to #5003
First PR in the phased migration of
zarf tools registryfrom Crane to ORAS, as discussed in the issue. This one coverslist(ls), the smallest command, to establish the pattern the rest will follow (digest/manifestnext, per the agreed ordering).Why
zarf tools registrycurrently wraps Crane's own cobra commands directly, backed bygo-containerregistry. Crane assumes every image layer is a tar file, which breaks on OCI images with non-tar layers.oras-godoesn't make that assumption.What changed
src/cmd/registry_list.goreimplementslsusingoras-goinstead ofcraneCmd.NewCmdList, preserving the exact same flags (--full-ref,-O/--omit-digest-tags), example text, and behavior.src/cmd/crane.go: swapped the Crane-wrappedlistregistration for the new command. One line changed.images.NewAuthClientFromDockerfor the Docker-credential fallback path, the sameorasRemote.Repositoryconstruction shape already used insrc/pkg/images/pull.go.Zarf-managed registry support
When the target repo matches a Zarf-managed cluster's registry, this transparently tunnels to it and authenticates with Zarf's own registry credentials (mirroring what
zarfCraneInternalWrapperdoes for the not-yet-migrated commands). Two things worth flagging for review:RegistryInfo.ResolvePlainHTTP, which already knows the answer from state without needing to probe the network - a generic probe would need to present a client certificate it doesn't have.Test plan
src/cmd/registry_list_test.go(default/--full-ref/--omit-digest-tagsbehavior, invalid-ref error path, a regression test proving the known-scheme value is trusted rather than silently re-probed)make docs-and-schema- zero diff, confirming generated CLI docs are unchanged from the Crane-wrapped versiongo build ./...,golangci-lint run ./...,pre-commit run --all-files