Pushing a non existent tag doesn't raise an exception

[paulcdejean]

I get this error in the error output but no exception is raised:

{"status":"The push refers to repository [485757098324.dkr.ecr.us-east-1.amazonaws.com/ssh_proxy]"}
{"errorDetail":{"message":"An image does not exist locally with the tag: 485757098324.dkr.ecr.us-east-1.amazonaws.com/ssh_proxy"},"error":"An image does not exist locally with the tag: 485757098324.dkr.ecr.us-east-1.amazonaws.com/ssh_proxy"}

Unlike related issue #1772 there's really no excuse here, since this error can be detected even if trying to push to a black box, while #1772 requires understanding the output of a repo server.

Also worth mentioning that the docker push command WILL return an error code in this case. So actually the docker push command gives MORE info than the SDK function. Since the docker push command gives output + error code. While the SDK function only gives output :(

(Not saying the function needs to BC break and return two values, just that it needs to raise exceptions when appropriate)

[paulcdejean]

Working with programmerq on irc we've discovered that when you do a docker push of a non existent tag, that the docker http API gives a 200 return code.

> 2019/01/11 20:29:03.992355  length=91 from=0 to=90
GET /_ping HTTP/1.1\r
Host: 127.0.0.1:5566\r
User-Agent: Docker-Client/18.06.1-ce (linux)\r
\r
< 2019/01/11 20:29:03.992762  length=215 from=0 to=214
HTTP/1.1 200 OK\r
Api-Version: 1.38\r
Docker-Experimental: false\r
Ostype: linux\r
Server: Docker/18.06.1-ce (linux)\r
Date: Fri, 11 Jan 2019 20:29:03 GMT\r
Content-Length: 2\r
Content-Type: text/plain; charset=utf-8\r
\r
OK> 2019/01/11 20:29:04.064731  length=232 from=91 to=322
POST /v1.38/images/485757098324.dkr.ecr.us-east-1.amazonaws.com/ssh_proxy/push?tag= HTTP/1.1\r
Host: 127.0.0.1:5566\r
User-Agent: Docker-Client/18.06.1-ce (linux)\r
Content-Length: 0\r
Content-Type: text/plain\r
X-Registry-Auth: censored=\r
\r
< 2019/01/11 20:29:04.208823  length=569 from=215 to=783
HTTP/1.1 200 OK\r
Api-Version: 1.38\r
Content-Type: application/json\r
Docker-Experimental: false\r
Ostype: linux\r
Server: Docker/18.06.1-ce (linux)\r
Date: Fri, 11 Jan 2019 20:29:04 GMT\r
Transfer-Encoding: chunked\r
\r
65\r
{"status":"The push refers to repository [485757098324.dkr.ecr.us-east-1.amazonaws.com/ssh_proxy]"}\r
\r
f3\r
{"errorDetail":{"message":"An image does not exist locally with the tag: 485757098324.dkr.ecr.us-east-1.amazonaws.com/ssh_
proxy"},"error":"An image does not exist locally with the tag: 485757098324.dkr.ecr.us-east-1.amazonaws.com/ssh_proxy"}\r
\r
< 2019/01/11 20:29:04.210393  length=5 from=784 to=788
0\r
\r

[shin-]

Yeah, it's the same as #1772 - the error isn't detected at the moment the request is made, meaning it only appears in the output stream after the HTTP status has already been set. The high-level DockerClient implementation actually does that kind of error detection for builds, and that could be extended to apply to push / pulls as well. OTOH I don't think we should change the design of the low-level APIClient to process the output beyond what we're currently doing.

Also, as far as this specific error is concerned, the Docker Engine really should do some trivial validation and detect if a tag doesn't exist locally and send a 4xx HTTP response rather than attempt a push anyway.

[paulcdejean]

Well as is we basically need to write a docker push wrapper function that attempts the push, parses the output. Then throws an exception if there's an error key in any of the json dictionary lines of the output.

That type of logic could be brought into the SDK as a stopgap before something more elegant is implemented. If performance is a concern, then it could be implemented as an alternative push function even?

As is people are either writing this sort of wrapper function, avoiding using the SDK, or just crossing their fingers and hoping none of their docker pushes ever error out.

[paulcdejean]

I'll add a code example of this sort of wrapper function soon (once I write it, probably before the end of the day), as is I'm using the finger crossing method).

[paulcdejean]

Here's the wrapper I wrote to workaround this issue.

class RemoteError(docker.errors.DockerException):
    """
    Raised if a remote docker repository returns an error. Usually due to a failed push or pull.
    """
    def __init__(self, error_dict):
        self.msg = error_dict['error']
        self.reason = error_dict['errorDetail']['message']

def push_wrapper(self, repository, **kwargs):
    push_output = self.client.api.push(repository, **kwargs)
    parsed_push_output = []
    digest = ''
    for line in push_output.splitlines():
        parsed_line = json.loads(line)
        if 'error' in parsed_line:
            raise RemoteError(parsed_line)
        if 'aux' in parsed_line:
            if 'Digest' in parsed_line['aux']:
                digest = parsed_line['aux']['Digest']

        parsed_push_output.append(parsed_line)
    return parsed_push_output, digest

docker.models.images.ImageCollection.push_wrapper = push_wrapper

It's not super tested due to many push errors being difficult to reproduce (the rate limit one is one I've been struggling with with amazon ECR in particular).

[chris-crone]

There is some work being done on the engine to provide richer errors so that they're easier to manage. Not exactly this but related: moby/moby#38467

[thaJeztah]

@chris-crone moby/moby#38467 likely won't help in this case, because the daemon sent a 200 status before it detects the error; moby/moby#38467 will make the client return the status code (instead of just a plain error).

I can try if waiting to send the "The push refers to repository" message until after the initial error detection was done could allow us to send a proper status code (and prevent the 200 status from being sent before that).

FWIW, I do think we still need the python sdk to detect errors if an errorDetail arrives; even though this initial error check could make it possible to send a 4xx status code, there may be situations where an error occurs further down the push (e.g. push is in progress, but the registry returns a failure). Due to this endpoint returning a stream, the status code cannot be updated at that point.

Also, as far as this specific error is concerned, the Docker Engine really should do some trivial validation and detect if a tag doesn't exist locally and send a 4xx HTTP response rather than attempt a push anyway.

Also see the above; in this case it looks like the validation is actually done before the actual push happens (but the initial message was already sent; I'll try and see if that particular case can be resolved)

[caeus]

So? nobody checking this?

[milas]

@caeus There is no update on this behavior in docker-py. Currently, the library exposes the Engine API as-is here. The engine will return a 200 if the push is able to start, and then streams push status back in the response. If an error is hit during push, that will be reflected in the events. The push_wrapper example from this comment shows how to handle this API response.