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 148109d37..692f4cf73 100644 --- a/tests/unit/api_image_test.py +++ b/tests/unit/api_image_test.py @@ -288,6 +288,33 @@ def test_push_image_stream(self): 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 3ce127b34..9d652dd6a 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 41d25920c..c7fd46fa8 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 85109473b..5f34d3625 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 @@ def pull(self, repository, tag=None, stream=False, auth_config=None, 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 @@ def push(self, repository, tag=None, stream=False, auth_config=None, 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 692f4cf73..321421b0e 100644 --- a/tests/unit/api_image_test.py +++ b/tests/unit/api_image_test.py @@ -271,6 +271,33 @@ def test_push_image_with_auth(self): 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 @@ def test_push_image_stream_with_auth(self): ) + 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 9d652dd6a..2882ad5c3 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 03e53cc64..e44ed5bbb 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_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 @@ def post_fake_config(): 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':