Skip to content
This repository was archived by the owner on Oct 10, 2020. It is now read-only.

Always use fq names for dest image#959

Closed
baude wants to merge 1 commit intoprojectatomic:masterfrom
baude:fq
Closed

Always use fq names for dest image#959
baude wants to merge 1 commit intoprojectatomic:masterfrom
baude:fq

Conversation

@baude
Copy link
Member

@baude baude commented Mar 31, 2017

Bugzilla 1437447 has brought up an issue where if a user runs:

atomic pull rhel7

The resulting image in dockerd is docker.io/rhel7. The Atomic CLI
does tell skopeo the right information"

/usr/bin/skopeo --debug --policy=/etc/containers/policy.json --debug copy --remove-signatures docker://registry.access.redhat.com/rhel7:latest docker-daemon:rhel7:latest

But somewhere along the line, we think in skopeo, docker.io is prepending to the destination
image name. One way to resolve this is to always use the fq name for the destination and not
what the user wanted.

This is a change in the default behaviour of atomic and I am not sure I am confortable with this. But
given that we have several folks on PTO or travelling, I'm putting this PR together so that
if we decide this is the proper route for fixing, it will be done.

@baude baude requested review from mtrmac, rhatdan and runcom March 31, 2017 15:40
@runcom
Copy link
Contributor

runcom commented Mar 31, 2017

But somewhere along the line, we think in skopeo, docker.io is prepending to the destination
image name. One way to resolve this is to always use the fq name for the destination and not
what the user wanted.

This has nothing to do with how skopeo works. Skopeo just uses whatever atomic passes in and this patch is OK to me since skopeo can't be fixed for this case since, again, it just accepts whatever atomic passes.

Copy link
Contributor

@aweiteka aweiteka left a comment

Choose a reason for hiding this comment

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

Since we're already interpreting for the user a FQDN I think this clarifies for the user what was actually pulled.

@miabbott
Copy link
Contributor

Didn't work so great:

# ./atomic pull rhel7
Pulling registry.access.redhat.com/rhel7:latest ...
FATA[0000] Invalid destination name registry.access.redhat.com/rhel7:latest: Invalid image name "registry.access.redhat.com/rhel7:latest", unknown transport "registry.access.redhat.com/rhel7" 

# ./atomic --debug pull rhel7                                                                                                                                                   
Namespace(_class=<class 'Atomic.pull.Pull'>, assumeyes=False, debug=True, func='pull_image', ignore=False, image='rhel7', reg_type=None, storage=None)
Pulling registry.access.redhat.com/rhel7:latest ...
Executing: /usr/bin/skopeo --policy=/etc/containers/policy.json --debug copy docker://registry.access.redhat.com/rhel7:latest registry.access.redhat.com/rhel7:latest
FATA[0000] Invalid destination name registry.access.redhat.com/rhel7:latest: Invalid image name "registry.access.redhat.com/rhel7:latest", unknown transport "registry.access.redhat.com/rhel7" 

Traceback (most recent call last):
  File "./atomic", line 191, in <module>
    sys.exit(_func())
  File "/root/atomic/Atomic/pull.py", line 53, in pull_image
    be.pull_image(self.args.image, remote_image_obj, debug=self.args.debug, assumeyes=self.args.assumeyes)
  File "/root/atomic/Atomic/backends/_docker.py", line 294, in pull_image
    policy_filename=trust.policy_filename)
  File "/root/atomic/Atomic/util.py", line 374, in skopeo_copy
    return check_call(cmd)
  File "/root/atomic/Atomic/util.py", line 148, in check_call
    return subprocess.check_call(cmd, env=env, stdin=stdin, stderr=stderr, stdout=stdout, close_fds=True)
  File "/usr/lib64/python2.7/subprocess.py", line 542, in check_call
    raise CalledProcessError(retcode, cmd)
CalledProcessError: Command '['/usr/bin/skopeo', '--policy=/etc/containers/policy.json', '--debug', 'copy', 'docker://registry.access.redhat.com/rhel7:latest', 'registry.access.redhat.com/rhel7:latest']' returned non-zero exit status 1

@baude
Copy link
Member Author

baude commented Mar 31, 2017

@runcom What you said is not quite true. Try:

/usr/bin/skopeo --debug --policy=/etc/containers/policy.json --debug copy --remove-signatures docker://registry.access.redhat.com/rhel7:latest docker-daemon:rhel7:latest

Make sure the image is not already present.

@baude
Copy link
Member Author

baude commented Mar 31, 2017

@miabbott nice catch, fixed now.

@miabbott
Copy link
Contributor

Works for me. 👍

@rhatdan
Copy link
Member

rhatdan commented Apr 3, 2017

@rh-atomic-bot r+ 39793b1

@mtrmac
Copy link
Contributor

mtrmac commented Apr 3, 2017

/usr/bin/skopeo --debug --policy=/etc/containers/policy.json --debug copy --remove-signatures docker://registry.access.redhat.com/rhel7:latest docker-daemon:rhel7:latest

But somewhere along the line, we think in skopeo, docker.io is prepending to the destination
image name. One way to resolve this is to always use the fq name for the destination and not
what the user wanted.

This is https://github.com/containers/image/blob/master/docker/daemon/daemon_dest.go#L217 , which was done for consistency with how docker pull in projectatomic/docker tags images.

Also, per the analysis in containers/image#72 , using a fully-qualified name when tagging an image (i.e. registry/…/rhel7, not just rhel7) is more interoperable (i.e both qualified and unqualified references to it work) than using an unqualified name (only unqualified references work).

If this is all as expected, atomic should indeed pass a name which uses the correct registry, and ACK on this patch.


However the wording in

The Atomic CLI does tell skopeo the right information"

seems to suggest that using the fully-qualified name is actually undesirable for some reason. Has projectatomic/docker changed how it processes references since Nov 2016, or is there another reason why containers/image should not be making the references fully qualified?

rh-atomic-bot pushed a commit that referenced this pull request Apr 4, 2017
Bugzilla 1437447 has brought up an issue where if a user runs:

atomic pull rhel7

The resulting image in dockerd is docker.io/rhel7. The Atomic CLI
does tell skopeo the right information"

/usr/bin/skopeo --debug --policy=/etc/containers/policy.json --debug copy --remove-signatures docker://registry.access.redhat.com/rhel7:latest docker-daemon:rhel7:latest

But somewhere along the line, we think in skopeo, docker.io is prepending to the destination
image name.  One way to resolve this is to always use the fq name for the destination and not
what the user wanted.

This is a change in the default behaviour of atomic and I am not sure I am confortable with this.  But
given that we have several folks on PTO or travelling, I'm putting this PR together so that
if we decide this is the proper route for fixing, it will be done.

Closes: #959
Approved by: rhatdan
@rh-atomic-bot
Copy link

⌛ Testing commit 39793b1 with merge 8f4cff5...

@jlebon
Copy link
Contributor

jlebon commented Apr 4, 2017

@rh-atomic-bot retry

@rh-atomic-bot
Copy link

⌛ Testing commit 39793b1 with merge 80de410...

rh-atomic-bot pushed a commit that referenced this pull request Apr 4, 2017
Bugzilla 1437447 has brought up an issue where if a user runs:

atomic pull rhel7

The resulting image in dockerd is docker.io/rhel7. The Atomic CLI
does tell skopeo the right information"

/usr/bin/skopeo --debug --policy=/etc/containers/policy.json --debug copy --remove-signatures docker://registry.access.redhat.com/rhel7:latest docker-daemon:rhel7:latest

But somewhere along the line, we think in skopeo, docker.io is prepending to the destination
image name.  One way to resolve this is to always use the fq name for the destination and not
what the user wanted.

This is a change in the default behaviour of atomic and I am not sure I am confortable with this.  But
given that we have several folks on PTO or travelling, I'm putting this PR together so that
if we decide this is the proper route for fixing, it will be done.

Closes: #959
Approved by: rhatdan
@cgwalters
Copy link
Member

@rh-atomic-bot retry

1 similar comment
@cgwalters
Copy link
Member

@rh-atomic-bot retry

@rh-atomic-bot
Copy link

⌛ Testing commit 39793b1 with merge e763149...

rh-atomic-bot pushed a commit that referenced this pull request Apr 4, 2017
Bugzilla 1437447 has brought up an issue where if a user runs:

atomic pull rhel7

The resulting image in dockerd is docker.io/rhel7. The Atomic CLI
does tell skopeo the right information"

/usr/bin/skopeo --debug --policy=/etc/containers/policy.json --debug copy --remove-signatures docker://registry.access.redhat.com/rhel7:latest docker-daemon:rhel7:latest

But somewhere along the line, we think in skopeo, docker.io is prepending to the destination
image name.  One way to resolve this is to always use the fq name for the destination and not
what the user wanted.

This is a change in the default behaviour of atomic and I am not sure I am confortable with this.  But
given that we have several folks on PTO or travelling, I'm putting this PR together so that
if we decide this is the proper route for fixing, it will be done.

Closes: #959
Approved by: rhatdan
@rh-atomic-bot
Copy link

💔 Test failed - status-redhatci

Bugzilla 1437447 has brought up an issue where if a user runs:

atomic pull rhel7

The resulting image in dockerd is docker.io/rhel7. The Atomic CLI
does tell skopeo the right information"

/usr/bin/skopeo --debug --policy=/etc/containers/policy.json --debug copy --remove-signatures docker://registry.access.redhat.com/rhel7:latest docker-daemon:rhel7:latest

But somewhere along the line, we think in skopeo, docker.io is prepending to the destination
image name.  One way to resolve this is to always use the fq name for the destination and not
what the user wanted.

This is a change in the default behaviour of atomic and I am not sure I am confortable with this.  But
given that we have several folks on PTO or travelling, I'm putting this PR together so that
if we decide this is the proper route for fixing, it will be done.
@baude
Copy link
Member Author

baude commented Apr 5, 2017

@rh-atomic-bot r+ 4c5c2e0

@rh-atomic-bot
Copy link

⌛ Testing commit 4c5c2e0 with merge d5d1eb8...

@rh-atomic-bot
Copy link

☀️ Test successful - status-redhatci
Approved by: baude
Pushing d5d1eb8 to master...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants