ansible-podman-collections icon indicating copy to clipboard operation
ansible-podman-collections copied to clipboard

Do not strip image name+tag from push destination when transport=dir

Open lowne opened this issue 3 years ago • 3 comments

/kind bug

Description

This isn't true when transport=dir, which results in all the images ending up into one giant broken pile if the the naked (i.e. unmangled) tag is used as part of the destination path (who's afraid of colons in filenames? :shark: )

Steps to reproduce the issue:

- containers.podman.podman_image:
    state: build
    path: "{{ build_dir }}"
    build:
      format: docker
      rm: no
    name: "{{ image_tag }}"
    push: yes
    push_args:
      transport: dir
      dest: "{{ some_dir }}/{{ image_tag }}"

Describe the results you received: [WARNING]: Image name and tag are automatically added to push_args['dest']. Destination changed to {{some_dir}} + a giant mess

Describe the results you expected: A neat directory structure

lowne avatar May 25 '21 15:05 lowne

Seems same as #218 and #205

sshnaidm avatar May 25 '21 16:05 sshnaidm

Those are indeed related. The section from line 640 to 672 muddies every transport together and sometimes doesn't properly discriminate for urlful-vs-urlless image name, thus making some wrong assumptions.

Unfortunately I don't have the wherewhital to setup the infrastructure to submit a proper PR :disappointed: but here's a proof-of-concept for properly dealing with each transport, which would replace that section of code:


# already parsed/built self.name, self.tag and self.image_name, 
# but `name` and `image_name` still have the registry url (if given)

self.bare_name = self.name.rsplit('/',1)[1] if '/' in self.name else self.name

dest = self.push_args.get('dest')

# push to registry if `transport` is omitted (as per docs, and podman-push)
transport = self.push_args.get('transport') or 'docker'
append_dest = True

if transport == 'docker':
    if not dest:
        append_dest = False
        dest_format_string = 'docker://{image_name}'
        if '/' not in self.name:
            self.module.fail_json(msg="'push_args['dest']' is required when pushing images that do not have the remote registry in the image name")
    else:
        dest = self.purge_image_name(dest)
        dest_format_string = 'docker://{dest}/{image_name}'
        if '/' in self.image_name:
            # we could fail
            #self.module.fail_json(msg="'push_args['dest']' was provided but the image name already contains the remote registry")
            # or, possibly better:
            dest_format_string = 'docker://{dest}/{bare_name}:{tag}'

# btw `ostree` was removed: https://github.com/containers/podman/commit/2046be6ae04603a581d3caffb26fc970f43765a2
# ~~   elif transport == 'ostree':                          ~~
# ~~       dest_format_string = '{transport}:{name}@{dest}' ~~

elif transport == 'docker-daemon':
    dest_format_string = 'docker-daemon:{image_name}'
    # allow overwriting the reference, similar to `docker` case above
    if dest: 
        dest = self.purge_image_name(dest)
        dest_format_string = 'docker-daemon:{dest}/{bare_name}:{tag}'

else:
    # `dir`, `docker-archive`, `oci-archive` all work with paths
    if not dest:
        self.module.fail_json(msg="'push_args['transport']'='{transport}'' requires 'push_args['dest']' but it was not provided.".format(transport=transport))
    dest_format_string = '{transport}:{dest}'
    #EDIT: oci-archive works the same way, no special treatment

# unsure why this is needed
#if dest and dest.endswith('/'):
#    dest = dest[:-1]

dest_string = dest_format_string.format(transport=transport, dest=dest, image_name=self.image_name, bare_name=self.bare_name, tag=self.tag)

# (almost) always append the destination argument 
if append_dest:
    args.append(dest_string)

which also requires this refactored-out helper method

def purge_image_name(self, dest):
    regexp = re.compile(r'/{name}(:{tag})?'.format(name=self.bare_name, tag=self.tag))
    if regexp.search(dest):
        dest = regexp.sub('', dest)
        self.module.warn("Image name and tag are automatically added to push_args['dest']. Destination changed to {dest}".format(dest=dest))
    return dest

This allows cross-registry push (#218) and makes no assumptions for dir, docker-archive and oci-archive destination paths (#205 and #264). Note that this is just back-of-the-napkin and might not even be legal python :1st_place_medal:

lowne avatar Jun 02 '21 11:06 lowne

I had the same issue when pushing to my own registry... For me this change fixed it ->

https://github.com/containers/ansible-podman-collections/blob/ba763e2988c4d3f94940695a5699f26fb94ed31f/plugins/modules/podman_image.py#L642

adding $ to regex in Line 642 -> regexp = re.compile(r'/{name}(:{tag})?$'.format(name=self.name, tag=self.tag))

xTreame avatar Jan 03 '23 13:01 xTreame