Add function to refresh credentials from config.json #1586

terminalmage · 2017-05-02T17:47:48Z

SCENARIO

docker-py performs an action that sets the X-Registry-Auth header in the API request (push, pull, etc.)
Password is changed for a registry.
docker login is performed on the CLI to update the config.json.
Pushes for the client instance now fail, since it has cached the auth configuration.

SOLUTION

~~Add a refresh param (default: False) to docker.auth.get_config_header() which, when True, forces a refresh of the config.json.~~
Update all functions which call docker.auth.get_config_header() to support a refresh_credentials argument (default: False) which is passed through as refresh when get_config_header() is called.
~~Using client.push('myuser/myimage', tag='mytag', refresh_credentials=True), docker-py now has up-to-date auth configuration.~~
A function has been added to the DaemonApiMixin which allows for the auth to be reloaded for an existing APIClient instance (e.g. client.reload_config())

GordonTheTurtle · 2017-05-02T17:47:49Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "refresh_credentials" git@github.com:terminalmage/docker-py.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

shin-

That feels like the wrong approach. You could have a Client.reload_config() instead that would be a one-liner, and much more flexible.

terminalmage · 2017-05-02T19:15:30Z

@shin- That's a good idea. Would this be best added to the DaemonApiMixin, or to the APIClient itself?

terminalmage · 2017-05-02T19:17:55Z

Since the _auth_configs are initially created in the APIClient, it's probably best to locate it there.

terminalmage · 2017-05-02T19:21:48Z

Hmm, maybe not. It looks like, from a design perspective, the functionality in the APIClient is largely from the mixins, there are only a couple public functions in APIClient proper.

terminalmage · 2017-05-02T19:51:44Z

@shin- I've removed the earlier commit and added the function to the DaemonApiMixin. Let me know if you think other changes need to be made.

This allows the client to reload the config.json for an existing APIClient instance. Signed-off-by: Erik Johnson <palehose@gmail.com>

See docker/docker-py#1586 This allows an existing client instance to disregard its cached auth config and use the most up-to-date login info from the config.json.

shin-

Thanks!

This is headed in the right direction. Just a few comments to address.

shin- · 2017-05-02T20:52:53Z

docker/api/daemon.py

        url = self._url("/version", versioned_api=api_version)
        return self._result(self._get(url), json=True)
+
+    def reload_config(self, dockercfg_path=None):


Actually, since this is not an API call, it should probably be added to the APIClient class instead.

shin- · 2017-05-02T20:53:30Z

docker/api/daemon.py

        # if so load that config.
        if dockercfg_path and os.path.exists(dockercfg_path):
-            self._auth_configs = auth.load_config(dockercfg_path)
+            self._auth_configs = self.reload_config(dockercfg_path)


reload_config returns None, so this is probably not correct.

Oh, duh. I was thinking of code re-use and obviously did not test this part of the code. Good catch. Since I'm moving this to the APIClient I'll just revert these two changes in the login func.

shin- · 2017-05-02T20:53:39Z

docker/api/daemon.py

+            self._auth_configs = self.reload_config(dockercfg_path)
        elif not self._auth_configs:
-            self._auth_configs = auth.load_config()
+            self._auth_configs = self.reload_config()


shin- · 2017-05-02T20:54:28Z

docker/api/daemon.py

+
+    def reload_config(self, dockercfg_path=None):
+        """
+        Forces a reload of the auth configuration


nit: "Force" instead of "Forces"

Also revert an incorrect change in the DaemonApiMixin's login func Signed-off-by: Erik Johnson <palehose@gmail.com>

terminalmage · 2017-05-02T21:27:27Z

@shin- Thanks for the comments, I believe I have addressed them all.

shin-

Thank you, LGTM!

terminalmage · 2017-05-02T21:57:55Z

👍

GordonTheTurtle added the dco/no label May 2, 2017

terminalmage force-pushed the refresh_credentials branch from 0ca2663 to c6645c8 Compare May 2, 2017 17:53

GordonTheTurtle removed the dco/no label May 2, 2017

terminalmage force-pushed the refresh_credentials branch from c6645c8 to 59e1ad0 Compare May 2, 2017 17:57

This was referenced May 2, 2017

Use proposed docker-py reload_config() function saltstack/salt#41011

Merged

[2016.11.4] Dockerng state not authing properly saltstack/salt#40884

Closed

shin- suggested changes May 2, 2017

View reviewed changes

terminalmage changed the title ~~Add argument to refresh credentials from config.json~~ Add function to refresh credentials from config.json May 2, 2017

terminalmage force-pushed the refresh_credentials branch from 59e1ad0 to 81927f0 Compare May 2, 2017 19:51

Add a reload_config function to the DaemonApiMixin

0f84341

This allows the client to reload the config.json for an existing APIClient instance. Signed-off-by: Erik Johnson <palehose@gmail.com>

terminalmage force-pushed the refresh_credentials branch from 81927f0 to 0f84341 Compare May 2, 2017 20:10

shin- suggested changes May 2, 2017

View reviewed changes

GordonTheTurtle added the dco/no label May 2, 2017

Move reload_config func into the APIClient

550c31e

Also revert an incorrect change in the DaemonApiMixin's login func Signed-off-by: Erik Johnson <palehose@gmail.com>

terminalmage force-pushed the refresh_credentials branch from 313cc9e to 550c31e Compare May 2, 2017 21:26

GordonTheTurtle removed the dco/no label May 2, 2017

shin- added this to the 2.3.0 milestone May 2, 2017

shin- approved these changes May 2, 2017

View reviewed changes

shin- merged commit 35b8c0a into docker:master May 2, 2017

terminalmage deleted the refresh_credentials branch October 26, 2017 15:27

Add function to refresh credentials from config.json #1586

Add function to refresh credentials from config.json #1586

Uh oh!

Conversation

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

Uh oh!

SCENARIO

SOLUTION

Uh oh!

GordonTheTurtle commented May 2, 2017

Uh oh!

shin- left a comment

Choose a reason for hiding this comment

Uh oh!

terminalmage commented May 2, 2017

Uh oh!

terminalmage commented May 2, 2017

Uh oh!

terminalmage commented May 2, 2017

Uh oh!

terminalmage commented May 2, 2017

Uh oh!

shin- left a comment

Choose a reason for hiding this comment

Uh oh!

shin- May 2, 2017

Choose a reason for hiding this comment

Uh oh!

shin- May 2, 2017

Choose a reason for hiding this comment

Uh oh!

terminalmage May 2, 2017

Choose a reason for hiding this comment

Uh oh!

shin- May 2, 2017

Choose a reason for hiding this comment

Uh oh!

shin- May 2, 2017

Choose a reason for hiding this comment

Uh oh!

terminalmage commented May 2, 2017

Uh oh!

shin- left a comment

Choose a reason for hiding this comment

Uh oh!

terminalmage commented May 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

terminalmage commented May 2, 2017 •

edited

Loading