Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace docker.io as the default registry with <local> #448

Closed
wants to merge 1 commit into from

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Apr 24, 2018

Signed-off-by: Daniel J Walsh [email protected]

@rhatdan
Copy link
Member Author

rhatdan commented Apr 24, 2018

@baude @TomSweeneyRedHat @mheon @runcom @mtrmac WDYT

This would change the default for buildah commit to create
localhost/library/IMAGENAME:latest
rather then
docker.io/library/IMAGENAME:latest

@rhatdan
Copy link
Member Author

rhatdan commented Apr 24, 2018

@nalind WDYT

@runcom
Copy link
Member

runcom commented Apr 24, 2018

localhost may refer to a locally deployed container registry which is ... wrong maybe since the intention is to have local images in the sense they're not on any registry

@rhatdan
Copy link
Member Author

rhatdan commented Apr 24, 2018

Sure I would be fine with image:latest as well. but the code is putting a "/" in in that case and gets an invalid name.

@rhatdan rhatdan changed the title Replace docker.io as the default registry with localhost [WIP] Replace docker.io as the default registry with localhost Apr 24, 2018
@rhatdan rhatdan changed the title [WIP] Replace docker.io as the default registry with localhost [WIP] Replace docker.io as the default registry with <local> Apr 25, 2018
@rhatdan
Copy link
Member Author

rhatdan commented Apr 26, 2018

@runcom @mtrmac PTAL

@rhatdan
Copy link
Member Author

rhatdan commented Apr 26, 2018

@runcom Do you know why git is failing?

@rhatdan
Copy link
Member Author

rhatdan commented Apr 26, 2018

@nalind @umohnani8 @mheon @baude FYI

@rhatdan rhatdan changed the title [WIP] Replace docker.io as the default registry with <local> Replace docker.io as the default registry with <local> Apr 26, 2018
@mheon
Copy link
Member

mheon commented Apr 26, 2018

Is there a reason it is <local>/library/$IMAGENAME as opposed to just <local>/$IMAGENAME?

@rhatdan
Copy link
Member Author

rhatdan commented Apr 26, 2018

@mheon Just this is the way the it worked in docker.io.
/library/$IMAGENAME == /$IMAGENAME

@mheon
Copy link
Member

mheon commented Apr 26, 2018

@rhatdan I'm just worried about library being in the default being potentially confusing to people seeing it in the output of podman images or similar

@rhatdan
Copy link
Member Author

rhatdan commented Apr 26, 2018

@mheon I would say lets get this in first and then we can look at that.

@mtrmac
Copy link
Collaborator

mtrmac commented Apr 26, 2018

FWIW the library behavior is specialized to docker.io, it does not happen for other registry hostnames, at least nowadays.

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed yesterday, allowing this kind of input for containers-storage: may well make sense.

Wholesale redefining the meaning of existing strings without considering each individual string <-> c/image/docker/reference.* conversion is not really practical.

{"//busybox", "docker.io/library/busybox:latest"}, // Default tag
{"//busybox:notlatest", "<local>/library/busybox:notlatest"}, // Explicit tag
{"//busybox" + sha256digest, "<local>/library/busybox" + sha256digest}, // Explicit digest
{"//busybox", "<local>/library/busybox:latest"}, // Default tag
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still essentially breaks the world; skopeo inspect docker://busybox will no longer do anything useful.

{"busybox:latest", "", "docker.io/library/busybox:latest"}, // Explicit tag
{"busybox@" + sha256digest, "", "docker.io/library/busybox@" + sha256digest}, // Explicit digest
{"busybox:latest", "", "<local>/library/busybox:latest"}, // Explicit tag
{"busybox@" + sha256digest, "", "<local>/library/busybox@" + sha256digest}, // Explicit digest
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the long comment in docker/tarfile/Destination.PutManifest, this would break interoperability with projectatomic/docker.

@@ -136,7 +136,7 @@ var prmExactMatchTestTable = []prmSymmetricTableTest{
{"busybox" + digestSuffix, "busybox" + digestSuffix, true}, // NOTE: This is not documented; signing digests is not recommended at this time.
// Non-canonical reference format is canonicalized
{"library/busybox:latest", "busybox:latest", true},
{"docker.io/library/busybox:latest", "busybox:latest", true},
{"<local>/library/busybox:latest", "busybox:latest", true},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This redefines the meaning of existing signatures, e.g. https://rhelblog.redhat.com/2016/07/22/container-image-signing/ shows a reference that does not contain a host name.

(Recent code uses the fully explicit format in created signatures, I’m afraid that hasn’t been originally the case.)

@mtrmac
Copy link
Collaborator

mtrmac commented Apr 26, 2018

i.e. basically replace the reference.ParseNormalizedNamed parse/normalization, and its uses, in storageTransport.parseStoreReference and storageTransport.ValidatePolicyConfigurationScope with something that allows <local>/.

That could be simpler to do on top of #426, which eliminates a fair amount of redundancy in the storageReference type.

@rhatdan
Copy link
Member Author

rhatdan commented Apr 26, 2018

Can we get #426 merged?

@rhatdan
Copy link
Member Author

rhatdan commented May 2, 2018

CLosing this PR in favor of containers/buildah#648

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants