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

[2.9] docker_image: do not crash in load_image for docker-py < 2.5.0. #73638

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -0,0 +1,2 @@
bugfixes:
- "docker_image - fix crash on loading images with versions of Docker SDK for Python before 2.5.0 (https://github.com/ansible-collections/community.docker/issues/72, https://github.com/ansible-collections/community.docker/pull/73)."
78 changes: 53 additions & 25 deletions lib/ansible/modules/cloud/docker/docker_image.py
Expand Up @@ -788,15 +788,41 @@ def load_image(self):
'''
# Load image(s) from file
load_output = []
has_output = False
try:
self.log("Opening image %s" % self.load_path)
with open(self.load_path, 'rb') as image_tar:
self.log("Loading image from %s" % self.load_path)
for line in self.client.load_image(image_tar):
self.log(line, pretty_print=True)
if "stream" in line or "status" in line:
load_line = line.get("stream") or line.get("status") or ''
load_output.append(load_line)
output = self.client.load_image(image_tar)
if output is not None:
# Old versions of Docker SDK of Python (before version 2.5.0) do not return anything.
# (See https://github.com/docker/docker-py/commit/7139e2d8f1ea82340417add02090bfaf7794f159)
# Note that before that commit, something else than None was returned, but that was also
# only introduced in a commit that first appeared in 2.5.0 (see
# https://github.com/docker/docker-py/commit/9e793806ff79559c3bc591d8c52a3bbe3cdb7350).
# So the above check works for every released version of Docker SDK for Python.
has_output = True
for line in output:
self.log(line, pretty_print=True)
if "stream" in line or "status" in line:
load_line = line.get("stream") or line.get("status") or ''
load_output.append(load_line)
else:
if LooseVersion(docker_version) < LooseVersion('2.5.0'):
self.client.module.warn(
'The installed version of the Docker SDK for Python does not return the loading results'
' from the Docker daemon. Therefore, we cannot verify whether the expected image was'
' loaded, whether multiple images where loaded, or whether the load actually succeeded.'
' If you are not stuck with Python 2.6, *please* upgrade to a version newer than 2.5.0'
' (2.5.0 was released in August 2017).'
)
else:
self.client.module.warn(
'The API version of your Docker daemon is < 1.23, which does not return the image'
Copy link
Member

@relrod relrod Mar 8, 2021

Choose a reason for hiding this comment

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

@felixfontein Do we know for sure this (API < 1.23) is the only other way to get None above? Would it be better to check self.client.version() and ensure this is true before we claim it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's say it like this: according to Docker's API documentation, this is the only way this could/should happen. It could be that this results in the wrong message if Docker implemented something else than they claim, or in case the user uses another API (like Podman's docker API - I have no idea if that still is a thing, and if it is, whether it suports this API endpoint at all, and if it does, what it returns). I think it's better to return this message in case None is returned instead of crashing, or having another branch for the (hopefully) unlikely case that Docker did something wrong or that a more or less "compatible" API is used that turns out not to be compatible enough.

Copy link
Member

Choose a reason for hiding this comment

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

@felixfontein sounds good to me, just wanted to check. :-)

' loading result from the Docker daemon. Therefore, we cannot verify whether the'
' expected image was loaded, whether multiple images where loaded, or whether the load'
' actually succeeded. You should consider upgrading your Docker daemon.'
)
except EnvironmentError as exc:
if exc.errno == errno.ENOENT:
self.client.fail("Error opening image %s - %s" % (self.load_path, str(exc)))
Expand All @@ -805,26 +831,28 @@ def load_image(self):
self.client.fail("Error loading image %s - %s" % (self.name, str(exc)), stdout='\n'.join(load_output))

# Collect loaded images
loaded_images = set()
for line in load_output:
if line.startswith('Loaded image:'):
loaded_images.add(line[len('Loaded image:'):].strip())

if not loaded_images:
self.client.fail("Detected no loaded images. Archive potentially corrupt?", stdout='\n'.join(load_output))

expected_image = '%s:%s' % (self.name, self.tag)
if expected_image not in loaded_images:
self.client.fail(
"The archive did not contain image '%s'. Instead, found %s." % (
expected_image, ', '.join(["'%s'" % image for image in sorted(loaded_images)])),
stdout='\n'.join(load_output))
loaded_images.remove(expected_image)

if loaded_images:
self.client.module.warn(
"The archive contained more images than specified: %s" % (
', '.join(["'%s'" % image for image in sorted(loaded_images)]), ))
if has_output:
# We can only do this when we actually got some output from Docker daemon
loaded_images = set()
for line in load_output:
if line.startswith('Loaded image:'):
loaded_images.add(line[len('Loaded image:'):].strip())

if not loaded_images:
self.client.fail("Detected no loaded images. Archive potentially corrupt?", stdout='\n'.join(load_output))

expected_image = '%s:%s' % (self.name, self.tag)
if expected_image not in loaded_images:
self.client.fail(
"The archive did not contain image '%s'. Instead, found %s." % (
expected_image, ', '.join(["'%s'" % image for image in sorted(loaded_images)])),
stdout='\n'.join(load_output))
loaded_images.remove(expected_image)

if loaded_images:
self.client.module.warn(
"The archive contained more images than specified: %s" % (
', '.join(["'%s'" % image for image in sorted(loaded_images)]), ))

return self.client.find_image(self.name, self.tag)

Expand Down
10 changes: 10 additions & 0 deletions test/integration/targets/docker_image/tasks/tests/options.yml
Expand Up @@ -223,6 +223,14 @@
register: load_image_3
ignore_errors: true

- name: load image (invalid image, old API version)
docker_image:
name: foo:bar
load_path: "{{ output_dir }}/image-invalid.tar"
source: load
api_version: "1.22"
register: load_image_4

- assert:
that:
- load_image is changed
Expand All @@ -233,6 +241,8 @@
"The archive did not contain image 'foo:bar'. Instead, found 'quay.io/ansible/docker-test-containers:hello-world'." == load_image_2.msg
- load_image_3 is failed
- '"Detected no loaded images. Archive potentially corrupt?" == load_image_3.msg'
- load_image_4 is changed
- "'The API version of your Docker daemon is < 1.23, which does not return the image loading result from the Docker daemon. Therefore, we cannot verify whether the expected image was loaded, whether multiple images where loaded, or whether the load actually succeeded. You should consider upgrading your Docker daemon.' in load_image_4.warnings"

####################################################################
## path ############################################################
Expand Down