From c7d4ec1421f7507c9bdec6c514a1a05414101cdc Mon Sep 17 00:00:00 2001 From: Francesco Zardi Date: Sat, 14 Sep 2024 17:12:33 +0200 Subject: [PATCH 1/3] Add image.push() unit test with stream response Signed-off-by: Francesco Zardi --- tests/unit/api_image_test.py | 27 +++++++++++++++++++++++++++ tests/unit/api_test.py | 26 ++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/tests/unit/api_image_test.py b/tests/unit/api_image_test.py index 148109d3..692f4cf7 100644 --- a/tests/unit/api_image_test.py +++ b/tests/unit/api_image_test.py @@ -288,6 +288,33 @@ class ImageTest(BaseAPIClientTest): timeout=DEFAULT_TIMEOUT_SECONDS ) + + def test_push_image_stream_with_auth(self): + auth_config = { + 'username': "test_user", + 'password': "test_password", + 'serveraddress': "test_server", + } + encoded_auth = auth.encode_header(auth_config) + self.client.push( + fake_api.FAKE_IMAGE_NAME, tag=fake_api.FAKE_TAG_NAME, + auth_config=auth_config, stream=True + ) + + fake_request.assert_called_with( + 'POST', + f"{url_prefix}images/test_image/push", + params={ + 'tag': fake_api.FAKE_TAG_NAME, + }, + data='{}', + headers={'Content-Type': 'application/json', + 'X-Registry-Auth': encoded_auth}, + stream=True, + timeout=DEFAULT_TIMEOUT_SECONDS + ) + + def test_tag_image(self): self.client.tag(fake_api.FAKE_IMAGE_ID, fake_api.FAKE_REPO_NAME) diff --git a/tests/unit/api_test.py b/tests/unit/api_test.py index 3ce127b3..9d652dd6 100644 --- a/tests/unit/api_test.py +++ b/tests/unit/api_test.py @@ -42,6 +42,30 @@ def response(status_code=200, content='', headers=None, reason=None, elapsed=0, return res +def stream_response(status_code=200, content='', **kwargs): + if isinstance(content, str): + content_bytes = content.encode('ascii') + elif isinstance(content, bytes): + content_bytes = content + else: + content_bytes = json.dumps(content).encode('ascii') + + body = io.BytesIO(content_bytes) + + # mock a stream interface + raw_resp = urllib3.HTTPResponse(body=body) + raw_resp._fp.chunked = True + raw_resp._fp.chunk_left = len(body.getvalue()) - 1 + raw_resp._fp.seek(0) + + return response( + status_code=status_code, + content=content_bytes, + raw=raw_resp, + **kwargs, + ) + + def fake_resolve_authconfig(authconfig, registry=None, *args, **kwargs): return None @@ -59,6 +83,8 @@ def fake_resp(method, url, *args, **kwargs): if not key: raise Exception(f'{method} {url}') status_code, content = fake_api.fake_responses[key]() + if kwargs.get("stream", False): + return stream_response(status_code=status_code, content=content) return response(status_code=status_code, content=content) From bcf3e11daa194e8c69da6f56fff1bfdfe32e578a Mon Sep 17 00:00:00 2001 From: Francesco Zardi Date: Sat, 14 Sep 2024 17:16:05 +0200 Subject: [PATCH 2/3] Simplify the json_stream() utility The previous implementation was generic over the kind of decoder and separator. However, the only use was with JSON decoder and newline-based splitting. Signed-off-by: Francesco Zardi --- docker/utils/json_stream.py | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/docker/utils/json_stream.py b/docker/utils/json_stream.py index 41d25920..c7fd46fa 100644 --- a/docker/utils/json_stream.py +++ b/docker/utils/json_stream.py @@ -1,4 +1,3 @@ -import json import json.decoder from ..errors import StreamParseError @@ -37,30 +36,12 @@ def json_stream(stream): This handles streams which are inconsistently buffered (some entries may be newline delimited, and others are not). """ - return split_buffer(stream, json_splitter, json_decoder.decode) - - -def line_splitter(buffer, separator='\n'): - index = buffer.find(str(separator)) - if index == -1: - return None - return buffer[:index + 1], buffer[index + 1:] - - -def split_buffer(stream, splitter=None, decoder=lambda a: a): - """Given a generator which yields strings and a splitter function, - joins all input, splits on the separator and yields each chunk. - Unlike string.split(), each chunk includes the trailing - separator, except for the last one if none was found on the end - of the input. - """ - splitter = splitter or line_splitter buffered = '' for data in stream_as_text(stream): buffered += data while True: - buffer_split = splitter(buffered) + buffer_split = json_splitter(buffered) if buffer_split is None: break @@ -69,6 +50,13 @@ def split_buffer(stream, splitter=None, decoder=lambda a: a): if buffered: try: - yield decoder(buffered) + yield json_decoder.decode(buffered) except Exception as e: raise StreamParseError(e) from e + + +def line_splitter(buffer: str, separator='\n'): + index = buffer.find(str(separator)) + if index == -1: + return None + return buffer[:index + 1], buffer[index + 1:] From ee44930ab2a3268225020adc1c93f4ce2ba23049 Mon Sep 17 00:00:00 2001 From: Francesco Zardi Date: Sat, 14 Sep 2024 17:25:37 +0200 Subject: [PATCH 3/3] Check presence of errors in server response to image push When pushing an image, the server response might have status code 200 (OK) even though the operation has failed. To detect the occurrence of an error, inspect each JSON chunk in the server response and verify that no "error" field is present. Fixes: #3277 Signed-off-by: Francesco Zardi --- docker/api/image.py | 45 +++++++++++++++++++++++++++++- tests/unit/api_image_test.py | 53 ++++++++++++++++++++++++++++++++++++ tests/unit/api_test.py | 2 ++ tests/unit/fake_api.py | 11 ++++++++ 4 files changed, 110 insertions(+), 1 deletion(-) diff --git a/docker/api/image.py b/docker/api/image.py index 85109473..5f34d362 100644 --- a/docker/api/image.py +++ b/docker/api/image.py @@ -1,8 +1,11 @@ +import itertools +import json import logging import os from .. import auth, errors, utils from ..constants import DEFAULT_DATA_CHUNK_SIZE +from ..utils.json_stream import json_stream log = logging.getLogger(__name__) @@ -433,6 +436,29 @@ class ImageApiMixin: return self._result(response) + @staticmethod + def _raise_if_error(chunk, response): + """ + Raise an exception if the given chunk of the JSON server response is a + dictionary and contains an "error" field. Otherwise, return the chunk + as-is. + + Args: + chunk (object): A chunk of the server response. + response (Response): The full server response. This will be attached + to the exception in the event that chunk indicates an error. + + Returns: + (object): The input chunk. + + Raises: + :py:class:`docker.errors.APIError` + If the chunk of the server response contains an error message. + """ + if isinstance(chunk, dict) and 'error' in chunk: + raise errors.APIError(chunk['error'], response=response) + return chunk + def push(self, repository, tag=None, stream=False, auth_config=None, decode=False): """ @@ -494,8 +520,25 @@ class ImageApiMixin: self._raise_for_status(response) + # The server response might have status code 200 (OK) even though the + # push operation has failed. To detect errors, inspect each JSON chunk + # of the server response and check if an "error" entry is present. + # See: https://github.com/docker/docker-py/issues/3277 if stream: - return self._stream_helper(response, decode=decode) + if decode: + return (self._raise_if_error(chunk, response) for chunk in + self._stream_helper(response, decode=True)) + else: + result_stream, internal_stream = itertools.tee( + self._stream_helper(response, decode=False)) + for chunk_json in json_stream(internal_stream): + self._raise_if_error(chunk_json, response) + return result_stream + + for chunk_str in response.text.splitlines(): + chunk_json = json.loads(chunk_str) + if 'error' in chunk_json: + raise errors.APIError(chunk_json['error'], response=response) return self._result(response) diff --git a/tests/unit/api_image_test.py b/tests/unit/api_image_test.py index 692f4cf7..321421b0 100644 --- a/tests/unit/api_image_test.py +++ b/tests/unit/api_image_test.py @@ -271,6 +271,33 @@ class ImageTest(BaseAPIClientTest): timeout=DEFAULT_TIMEOUT_SECONDS ) + + def test_push_image_with_auth_error(self): + auth_config = { + 'username': "test_user", + 'password': "test_password", + 'serveraddress': "test_server", + } + encoded_auth = auth.encode_header(auth_config) + with pytest.raises(docker.errors.APIError, match='bad auth'): + self.client.push( + fake_api.FAKE_IMAGE_NAME_ERROR, tag=fake_api.FAKE_TAG_NAME, + auth_config=auth_config + ) + + fake_request.assert_called_with( + 'POST', + f"{url_prefix}images/test_image_error/push", + params={ + 'tag': fake_api.FAKE_TAG_NAME, + }, + data='{}', + headers={'Content-Type': 'application/json', + 'X-Registry-Auth': encoded_auth}, + stream=False, + timeout=DEFAULT_TIMEOUT_SECONDS + ) + def test_push_image_stream(self): with mock.patch('docker.auth.resolve_authconfig', fake_resolve_authconfig): @@ -315,6 +342,32 @@ class ImageTest(BaseAPIClientTest): ) + def test_push_image_stream_with_auth_error(self): + auth_config = { + 'username': "test_user", + 'password': "test_password", + 'serveraddress': "test_server", + } + encoded_auth = auth.encode_header(auth_config) + with pytest.raises(docker.errors.APIError, match='bad auth'): + self.client.push( + fake_api.FAKE_IMAGE_NAME_ERROR, tag=fake_api.FAKE_TAG_NAME, + auth_config=auth_config, stream=True + ) + + fake_request.assert_called_with( + 'POST', + f"{url_prefix}images/test_image_error/push", + params={ + 'tag': fake_api.FAKE_TAG_NAME, + }, + data='{}', + headers={'Content-Type': 'application/json', + 'X-Registry-Auth': encoded_auth}, + stream=True, + timeout=DEFAULT_TIMEOUT_SECONDS + ) + def test_tag_image(self): self.client.tag(fake_api.FAKE_IMAGE_ID, fake_api.FAKE_REPO_NAME) diff --git a/tests/unit/api_test.py b/tests/unit/api_test.py index 9d652dd6..2882ad5c 100644 --- a/tests/unit/api_test.py +++ b/tests/unit/api_test.py @@ -31,6 +31,8 @@ def response(status_code=200, content='', headers=None, reason=None, elapsed=0, request=None, raw=None): res = requests.Response() res.status_code = status_code + if isinstance(content, str): + content = content.encode('ascii') if not isinstance(content, bytes): content = json.dumps(content).encode('ascii') res._content = content diff --git a/tests/unit/fake_api.py b/tests/unit/fake_api.py index 03e53cc6..e44ed5bb 100644 --- a/tests/unit/fake_api.py +++ b/tests/unit/fake_api.py @@ -1,3 +1,5 @@ +import json + from docker import constants from . import fake_stat @@ -9,6 +11,7 @@ FAKE_IMAGE_ID = 'sha256:fe7a8fc91d3f17835cbb3b86a1c60287500ab01a53bc79c4497d09f0 FAKE_EXEC_ID = 'b098ec855f10434b5c7c973c78484208223a83f663ddaefb0f02a242840cb1c7' FAKE_NETWORK_ID = '1999cfb42e414483841a125ade3c276c3cb80cb3269b14e339354ac63a31b02c' FAKE_IMAGE_NAME = 'test_image' +FAKE_IMAGE_NAME_ERROR = 'test_image_error' FAKE_TARBALL_PATH = '/path/to/tarball' FAKE_REPO_NAME = 'repo' FAKE_TAG_NAME = 'tag' @@ -359,6 +362,12 @@ def post_fake_push(): return status_code, response +def post_fake_push_error(): + status_code = 200 + response = '{"status": "intermediate update"}\r\n{"error": "bad auth"}\r\n' + return status_code, response + + def post_fake_build_container(): status_code = 200 response = {'Id': FAKE_CONTAINER_ID} @@ -603,6 +612,8 @@ fake_responses = { get_fake_insert_image, f'{prefix}/{CURRENT_VERSION}/images/test_image/push': post_fake_push, + f'{prefix}/{CURRENT_VERSION}/images/test_image_error/push': + post_fake_push_error, f'{prefix}/{CURRENT_VERSION}/commit': post_fake_commit, f'{prefix}/{CURRENT_VERSION}/containers/create':