Suggest login on pull denial by alfred-landrum · Pull Request #33005 · moby/moby

alfred-landrum · 2017-05-03T22:52:27Z

- What I did
We've seen a few new users of Docker Store subscriptions see the "no pull access" message when they try to pull content for the first time, as they don't realize they need to login first.

To improve their experience with Store, or any other registry, make the returned error suggest that docker login may be needed.

- How to verify it
Use docker pull with any non-existent repository, or to a known repository that requires credentials w/o providing them first.

- Description for the changelog
Suggest login on pull denial

thaJeztah · 2017-05-04T01:49:39Z

we should check if not found is used for the HTTP status code that's returned by the API (there's some ugly string-matching in a couple of locations)

thanks - I looked through image_routes.go on the engine side, and image_create.go and image_pull.go on the cli side, as well as some general grepping & digging, and didn't discover anything that was matching on the existing error message.

let me know if there are other places I should look!

The nasty matching is here;

moby/api/server/httputils/errors.go

Lines 56 to 68 in 6486924

{"not found", http.StatusNotFound},

{"cannot find", http.StatusNotFound},

{"no such", http.StatusNotFound},

{"bad parameter", http.StatusBadRequest},

{"no command", http.StatusBadRequest},

{"conflict", http.StatusConflict},

{"impossible", http.StatusNotAcceptable},

{"wrong login/password", http.StatusUnauthorized},

{"unauthorized", http.StatusUnauthorized},

{"hasn't been activated", http.StatusForbidden},

{"this node", http.StatusServiceUnavailable},

{"needs to be unlocked", http.StatusServiceUnavailable},

{"certificates have expired", http.StatusServiceUnavailable},

looks like it's not depending on these 👍

thaJeztah

LGTM

thaJeztah · 2017-05-04T22:56:03Z

hm, looks like it needs changes in docker-py tests though 😞 /cc @shin-

17:25:09 =================================== FAILURES ===================================
17:25:09 _______ ContainerCollectionTest.test_run_with_image_that_does_not_exist ________
17:25:09 /docker-py/tests/integration/models_containers_test.py:34: in test_run_with_image_that_does_not_exist
17:25:09     client.containers.run("dockerpytest_does_not_exist")
17:25:09 /docker-py/docker/models/containers.py:659: in run
17:25:09     self.client.images.pull(image)
17:25:09 /docker-py/docker/models/images.py:259: in pull
17:25:09     self.client.api.pull(name, **kwargs)
17:25:09 /docker-py/docker/api/image.py:370: in pull
17:25:09     self._raise_for_status(response)
17:25:09 /docker-py/docker/api/client.py:216: in _raise_for_status
17:25:09     raise create_api_error_from_http_exception(e)
17:25:09 /docker-py/docker/errors.py:30: in create_api_error_from_http_exception
17:25:09     raise cls(e, response=response, explanation=explanation)
17:25:09 E   APIError: 500 Server Error: Internal Server Error ("pull access denied for dockerpytest_does_not_exist, repository does not exist or may require 'docker login'")

thaJeztah · 2017-05-04T22:57:30Z

Looks like the change is needed here; https://github.com/docker/docker-py/blob/cb68acbaaef235f5dc7ee7e1744e8d88a6c10070/docker/errors.py#L23-L30

docker-py needs an update :/

Support new error message returned for image creation in: moby/moby#33005 Signed-off-by: Alfred Landrum <[email protected]>

alfred-landrum · 2017-05-04T23:24:30Z

thanks @thaJeztah for pointing me in the right direction. Opened this PR on docker-py: docker/docker-py#1590 .

alfred-landrum · 2017-05-08T14:52:30Z

@thaJeztah - now that docker/docker-py#1590 is merged, do I need to adjust the moby/moby source at all?

thaJeztah · 2017-05-08T16:57:18Z

@alfred-landrum yes, I think the DOCKER_PY_COMMIT needs to be updated to point to a commit that contains your changes; https://github.com/moby/moby/search?utf8=✓&q=DOCKER_PY_COMMIT&type=

alfred-landrum · 2017-05-08T23:05:14Z

looks like there is some string matching going on, will investigate

alfred-landrum · 2017-05-09T04:37:20Z

I believe the remaining CI errors are unrelated to this change.

thaJeztah · 2017-05-09T13:14:42Z

Please don't add this here. This is a really bad pattern we're trying to remove.

I think the right way to handle this is to have TranslatePullError() return a specific error, and then translate that error into a StatusNotFound in the api/server/router/ function for pull

I'm sad we merged this PR without addressing this problem.

thaJeztah

changes LGTM (although we really need to find a different approach for the error handling)

can you squash the first and last commit?

alfred-landrum · 2017-05-09T20:01:48Z

@thaJeztah - commits squashed; there's a single non-related test failure.

mlaventure · 2017-05-09T22:08:37Z

docker-py on experimental fails:

22:04:59 =================================== FAILURES ===================================
22:04:59 ________________ CreateContainerTest.test_create_with_init_path ________________
22:04:59 /docker-py/tests/integration/api_container_test.py:464: in test_create_with_init_path
22:04:59     assert config['HostConfig']['InitPath'] == "/usr/libexec/docker-init"
22:04:59 E   KeyError: 'InitPath'
22:04:59 ______________________ PruneImagesTest.test_prune_images _______________________
22:04:59 /docker-py/tests/integration/api_image_test.py:306: in test_prune_images
22:04:59     result = self.client.prune_images()
22:04:59 /docker-py/docker/utils/decorators.py:35: in wrapper
22:04:59     return f(self, *args, **kwargs)
22:04:59 /docker-py/docker/api/image.py:300: in prune_images
22:04:59     return self._result(self._post(url, params=params), True)
22:04:59 /docker-py/docker/utils/decorators.py:47: in inner
22:04:59     return f(self, *args, **kwargs)
22:04:59 /docker-py/docker/api/client.py:179: in _post
22:04:59     return self.post(url, **self._set_request_timeout(kwargs))
22:04:59 /usr/lib/python2.7/dist-packages/requests/sessions.py:500: in post
22:04:59     return self.request('POST', url, data=data, json=json, **kwargs)
22:04:59 /usr/lib/python2.7/dist-packages/requests/sessions.py:457: in request
22:04:59     resp = self.send(prep, **send_kwargs)
22:04:59 /usr/lib/python2.7/dist-packages/requests/sessions.py:569: in send
22:04:59     r = adapter.send(request, **kwargs)
22:04:59 /usr/lib/python2.7/dist-packages/requests/adapters.py:407: in send
22:04:59     raise ConnectionError(err, request=request)
22:04:59 E   ConnectionError: ('Connection aborted.', BadStatusLine("''",))
22:04:59 ________________________ ServiceTest.test_service_logs _________________________
22:04:59 /docker-py/tests/helpers.py:62: in wrapped
22:04:59     return f(self, *args, **kwargs)
22:04:59 /docker-py/tests/integration/api_service_test.py:111: in test_service_logs
22:04:59     log_line = next(logs)
22:04:59 E   StopIteration
22:04:59 ========= 3 failed, 237 passed, 3 skipped, 2 xfailed in 319.97 seconds =========

alfred-landrum · 2017-05-11T04:57:09Z

I updated the docker-py bump to docker/docker-py@2bdaf7f to get the xfail on test_create_with_init_path. It looks like there's still a docker-py testing issue that's unrelated to the error string change, will investigate further tomorrow.

alfred-landrum · 2017-05-12T16:47:18Z

I rebased & learned how to run the python integration tests locally; when I did, I didn't see the issue, so have pushed up to see if the experimental test still fails in Jenkins.

mlaventure · 2017-05-14T04:13:12Z

Some of the tests seem to be out of date (cc @dnephin ?)

dnephin · 2017-05-15T14:41:27Z

I'm not that familiar with the test. Why do you think they are out of date?

Maybe they need to check different things for different API versions?

cc @shin-

shin- · 2017-05-15T20:54:53Z

I answered here: docker/docker-py#1603 (comment)

mlaventure · 2017-05-15T21:18:46Z

I thought they were out of date because of this:

18:51:51 XFAIL ../../../../../docker-py/tests/integration/api_container_test.py::CreateContainerTest::test_create_with_init_path
18:51:51   init-path removed in 17.05.0

At least this test seems to be deprecated now.

shin- · 2017-05-15T21:48:50Z

@mlaventure xfail do not cause the test suite to fail - they're expected failures.

The apparent issue with this PR is with the image pruning endpoint. The Connection Aborted message seems to suggest a possible engine crash.

alfred-landrum · 2017-05-26T21:06:45Z

merging in the image prune fix from #33409

thaJeztah · 2017-05-30T10:12:47Z

#33409 was merged, so this can now be rebased 😅

alfred-landrum · 2017-05-31T15:21:40Z

@thaJeztah - the windows test failed to start; would you please trigger a re-test (or tell me how to)?

thaJeztah · 2017-05-31T16:32:20Z

For reference; docker-py changes; docker/docker-py@4a08d04...0832898

alfred-landrum · 2017-06-01T19:50:15Z

💚 at last! @thaJeztah anything else I should do?

vieux · 2017-06-01T20:06:05Z

@thaJeztah the list is huge ? is it expected ?

thaJeztah · 2017-06-02T00:07:20Z

@vieux docker-py change relevant for this PR is docker/docker-py#1590, but afaik docker-py 2.3.0 is the one "matching" 17.06

Signed-off-by: Alfred Landrum <[email protected]>

alfred-landrum · 2017-06-09T19:44:56Z

rebased to get the new docker-py commit in master.

tiborvass · 2017-06-12T23:49:52Z

LGTM

thaJeztah · 2017-06-12T23:51:47Z

It's in! Thanks for your patience, @alfred-landrum 👍

GordonTheTurtle added the status/0-triage label May 3, 2017

thaJeztah added area/distribution Image Distribution kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. status/2-code-review and removed status/0-triage labels May 4, 2017

thaJeztah reviewed May 4, 2017

View reviewed changes

alfred-landrum force-pushed the denied-error branch from 5e0b5b0 to 64940cd Compare May 4, 2017 15:12

thaJeztah previously approved these changes May 4, 2017

View reviewed changes

thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label May 4, 2017

alfred-landrum added a commit to alfred-landrum/docker-py that referenced this pull request May 4, 2017

Update image create error parsing

02c5914

Support new error message returned for image creation in: moby/moby#33005 Signed-off-by: Alfred Landrum <[email protected]>

alfred-landrum mentioned this pull request May 4, 2017

Update image create error parsing docker/docker-py#1590

Merged

thaJeztah reviewed May 9, 2017

View reviewed changes

thaJeztah approved these changes May 9, 2017

View reviewed changes

thaJeztah removed the status/failing-ci Indicates that the PR in its current state fails the test suite label May 9, 2017

alfred-landrum force-pushed the denied-error branch from 9c31fbe to 4349f4c Compare May 9, 2017 14:36

thaJeztah added the rebuild/experimental label May 9, 2017

GordonTheTurtle removed the rebuild/experimental label May 9, 2017

thaJeztah added this to the 17.06.0 milestone May 9, 2017

alfred-landrum force-pushed the denied-error branch 2 times, most recently from 254aede to 580354d Compare May 10, 2017 21:40

alfred-landrum mentioned this pull request May 11, 2017

test_prune_images failing with master docker/docker-py#1603

Closed

alfred-landrum force-pushed the denied-error branch from 580354d to eee4e3d Compare May 12, 2017 16:40

alfred-landrum mentioned this pull request May 16, 2017

choose rpc code to determine status code #32122

Merged

alfred-landrum force-pushed the denied-error branch from eee4e3d to 17a2817 Compare May 17, 2017 13:28

GordonTheTurtle assigned johnstep May 18, 2017

alfred-landrum mentioned this pull request May 26, 2017

test_prune_images failing with recent docker-py #33407

Closed

alfred-landrum force-pushed the denied-error branch from 17a2817 to af58ff3 Compare May 26, 2017 21:06

alfred-landrum force-pushed the denied-error branch from af58ff3 to a6c24d0 Compare May 30, 2017 15:35

alfred-landrum force-pushed the denied-error branch from a6c24d0 to e173aaf Compare June 1, 2017 14:40

Suggest login on pull denial

8d9f51e

Signed-off-by: Alfred Landrum <[email protected]>

alfred-landrum force-pushed the denied-error branch from e173aaf to 8d9f51e Compare June 9, 2017 19:44

thaJeztah merged commit f4bf61c into moby:master Jun 12, 2017

thaJeztah removed this from the 17.06.0 milestone Jun 12, 2017

	{"not found", http.StatusNotFound},
	{"cannot find", http.StatusNotFound},
	{"no such", http.StatusNotFound},
	{"bad parameter", http.StatusBadRequest},
	{"no command", http.StatusBadRequest},
	{"conflict", http.StatusConflict},
	{"impossible", http.StatusNotAcceptable},
	{"wrong login/password", http.StatusUnauthorized},
	{"unauthorized", http.StatusUnauthorized},
	{"hasn't been activated", http.StatusForbidden},
	{"this node", http.StatusServiceUnavailable},
	{"needs to be unlocked", http.StatusServiceUnavailable},
	{"certificates have expired", http.StatusServiceUnavailable},

Conversation

alfred-landrum commented May 3, 2017

Uh oh!

thaJeztah May 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alfred-landrum May 4, 2017

Choose a reason for hiding this comment

Uh oh!

alfred-landrum May 4, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah May 4, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented May 4, 2017

Uh oh!

thaJeztah commented May 4, 2017

Uh oh!

alfred-landrum commented May 4, 2017

Uh oh!

alfred-landrum commented May 8, 2017

Uh oh!

thaJeztah commented May 8, 2017

Uh oh!

alfred-landrum commented May 8, 2017

Uh oh!

alfred-landrum commented May 9, 2017

Uh oh!

thaJeztah May 9, 2017

Choose a reason for hiding this comment

Uh oh!

dnephin May 14, 2017

Choose a reason for hiding this comment

Uh oh!

dnephin Jun 13, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

alfred-landrum commented May 9, 2017

Uh oh!

mlaventure commented May 9, 2017

Uh oh!

alfred-landrum commented May 11, 2017

Uh oh!

alfred-landrum commented May 12, 2017

Uh oh!

mlaventure commented May 14, 2017

Uh oh!

dnephin commented May 15, 2017

Uh oh!

shin- commented May 15, 2017

Uh oh!

mlaventure commented May 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shin- commented May 15, 2017

Uh oh!

alfred-landrum commented May 26, 2017

Uh oh!

thaJeztah commented May 30, 2017

Uh oh!

alfred-landrum commented May 31, 2017

Uh oh!

thaJeztah commented May 31, 2017

Uh oh!

alfred-landrum commented Jun 1, 2017

Uh oh!

vieux commented Jun 1, 2017

Uh oh!

thaJeztah commented Jun 2, 2017

Uh oh!

alfred-landrum commented Jun 9, 2017

Uh oh!

thaJeztah May 4, 2017 •

edited

Loading

mlaventure commented May 15, 2017 •

edited

Loading