Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow client to talk to an older server #27745

Merged
merged 1 commit into from
Nov 9, 2016

Conversation

vieux
Copy link
Contributor

@vieux vieux commented Oct 25, 2016

  • removed user-agent middleware (to prevent Client and server don't have the same version error)
  • removed check for client is newer than server error
  • added API-Version header on all API calls
  • Ping() always calls /_ping, not /v1.xx/_ping
  • started added a few if where flags/features are only supported in 1.25

ping @dnephin

@vieux
Copy link
Contributor Author

vieux commented Oct 26, 2016

$ docker version
Client:
 Version:      1.13.0-dev
 API version:  1.24 (downgraded from 1.25)
 Go version:   go1.7.3
 Git commit:   ec3a34b
 Built:        Wed Oct 26 00:54:51 2016
 OS/Arch:      linux/amd64

Server:
 Version:      1.12.2
 API version:  1.24
 Go version:   go1.6.3
 Git commit:   bb80604
 Built:        Tue Oct 11 17:00:50 2016
 OS/Arch:      linux/amd64
 Experimental: false


$ docker create --stop-timeout 5 test
Server only supports API version stop timeout, "1.24" support requires at least 1.25

@duglin
Copy link
Contributor

duglin commented Oct 26, 2016

@vieux I don't understand that error message - perhaps a rewording would help?
Also, it seems a bit odd that while I see the Client and Server servers are different (1.13 vs 1.12), the API versions are the same - you'd think that if those matched then no error would be generated. No?

@vieux
Copy link
Contributor Author

vieux commented Oct 26, 2016

@duglin thanks, I'll reword the error message. and thanks good catch, the client server should either say 1.25 or 1.24 (downgraded from 1.25)

@vieux vieux force-pushed the cli_backward_compose_api branch 5 times, most recently from 6ac2bb8 to 3395d09 Compare October 27, 2016 00:43
@duglin
Copy link
Contributor

duglin commented Oct 27, 2016

@vieux in this mode of operation am I correct that this is using the data itself (ie. incoming fields vs what the server knows about) to determine if it can support the incoming request rather than the version string itself?

@icecrime icecrime added this to the 1.13.0 milestone Oct 27, 2016
@bfirsh
Copy link
Contributor

bfirsh commented Oct 27, 2016

@vieux As an extended response to your comment on #27830, instead of using a header, perhaps an additional API endpoint could be used that reports the latest API version.

Something like:

GET / HTTP/1.1

{
  "LatestVersion": "1.25",
  "SupportedVersions": [
    "1.25",
    "1.24",
    "1.23",
    ...
  ]
}

(could also be something like /_api-versions if we didn't want to block out the root for this purpose)

In a potential future where a version is required, this could be used as the entrypoint for clients to figure out what versions can be used. Seems more solid and explicit than using a header on another API endpoint.

return nil
}

// OriginalClientVersion returns the origin client version.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returns the original client version ? 😝

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

@@ -183,9 +183,9 @@ func (cli *Client) Close() error {

// getAPIPath returns the versioned request path to call the api.
// It appends the query parameters to the path if they are not empty.
func (cli *Client) getAPIPath(p string, query url.Values) string {
func (cli *Client) getAPIPath(p string, query url.Values, ignoreVersion bool) string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignoreVersion is only for the _ping endpoint, right ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it better to have a ignoreVersion bool or handle the _ping endpoint by itself in the method 😛

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a good idea (have _ping not call this)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still need to go through the rest of the function:

    u := &url.URL{
        Path: apiPath,
    }
    if len(query) > 0 {
        u.RawQuery = query.Encode()
    }
    return u.String()

I don't like this solution either, but from client.Ping to getAPIPath it goes through rougly 4 functions. not sure how to handle that better. I could replace the Context part by a bool but not sure it would be enough ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and by that I mean:

Client.Ping -> Client.get
Client.get -> Client.sendRequest
Client.sendRequest -> Client.sendClientRequest
Client.sendClientRequest -> Client.newRequest
Client.newRequest -> Client.getAPIPath

The best I could see is to add a bool to newRequest and getAPIPath and do:

Client.Ping -> Client.sendClientRequestIgnoreVersion
Client.sendClientRequestIgnoreVersion -> Client.newRequest(ignoreVersion=true)
Client.newRequest -> Client.getAPIPath(ignoreVersion=true)

wdyt @dnephin @vdemeester

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vieux this client/request.go has a few issues I think. I've been meaning to refactor these methods for a while now. I think the problem we're seeing here is a symptom of those issues.

I refactored the send methods here: master...dnephin:refactor-client

That branch includes a commit which uses the refactored functions to call Ping() without the api version in the path. It removes the need to have the ignoreVersion parameter entirely.

If this works for you, would you like to cherry pick the first commit into this branch? Otherwise I can drop the change to ping.go and open it as a PR. Let me know which you prefer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #27916

@vieux
Copy link
Contributor Author

vieux commented Oct 28, 2016

@bfirsh the current idea here is to piggyback on the work on regarding experimental. that's how it was designed: call /_ping and get the experimental status from a header.

I'm not against adding this new endpoint ,but it seems a bit overkill to me.

WDYT @dnephin ?

@dnephin
Copy link
Member

dnephin commented Oct 28, 2016

I think adding it to /_ping makes sense. We already have an api endpoint for returning version information at /version. We could use /version instead of /_ping (and have it respond without a version in the path), but I don't think we need a new one.

keyFile string
client client.APIClient
hasExperimental bool
originalClientVersion string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about another word for original ?

Maybe defaultVersion, maxSupportedVersion, or preferredVersion ?

@@ -72,13 +72,19 @@ func AddCommands(cmd *cobra.Command, dockerCli *command.DockerCli) {
hide(system.NewInspectCommand(dockerCli)),
)

if versions.GreaterThanOrEqualTo(dockerCli.Client().ClientVersion(), "1.25") {
cmd.AddCommand(system.NewVersionCommand(dockerCli))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, why is the version command gated by server version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I tought "system." was for the new data management commands, I'll remove that.

@@ -183,9 +183,9 @@ func (cli *Client) Close() error {

// getAPIPath returns the versioned request path to call the api.
// It appends the query parameters to the path if they are not empty.
func (cli *Client) getAPIPath(p string, query url.Values) string {
func (cli *Client) getAPIPath(p string, query url.Values, ignoreVersion bool) string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a good idea (have _ping not call this)


if config != nil && config.StopTimeout != nil && versions.LessThan(cli.version, "1.25") {
return response, fmt.Errorf("Server only supports API version %s, %q support requires at least %s", cli.version, "stop timeout", "1.25")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make this a function, so that all the error messages have a consistent format and wording.

func NewVersionError(version, required, feature string) error {
    ...
}

If you make it a method on Client you could even drop the version argument.

For the wording how about:

"stop timeout" requires API version 1.25, but the Docker server is version 1.24.

}

return true, nil
return true, apiVersion, nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of three return values, maybe a struct like we use for other responses?

@@ -50,6 +48,11 @@ func newDockerCommand(dockerCli *command.DockerCli) *cobra.Command {
flags.StringVar(&opts.ConfigDir, "config", cliconfig.ConfigDir(), "Location of client config files")
opts.Common.InstallFlags(flags)

// flags must be the top-level command flags, not cmd.Flags()
opts.Common.SetDefaultOptions(flags)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is wrong. The flags have not been parsed yet so the values that it sets are going to be wrong I believe.

}

// Ping pings the server and retrieve the version to use and if experimental is enabled.
func (cli *DockerCli) Ping() (err error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping() is maybe not the best name for this, how about making this an unexpected func that is called from Initialize() ? initializeVersion() ?

@@ -50,6 +48,11 @@ func newDockerCommand(dockerCli *command.DockerCli) *cobra.Command {
flags.StringVar(&opts.ConfigDir, "config", cliconfig.ConfigDir(), "Location of client config files")
opts.Common.InstallFlags(flags)

// flags must be the top-level command flags, not cmd.Flags()
opts.Common.SetDefaultOptions(flags)
dockerCli.Initialize(opts)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, this is too early to call Initialize() because the flags haven't been parsed yet.

// flags must be the top-level command flags, not cmd.Flags()
opts.Common.SetDefaultOptions(flags)
dockerCli.Initialize(opts)
dockerCli.Ping()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this presents a serious UX problem.

If I run docker or docker --help to find out how to connect to an alternative host I'm going to get a connection error instead of the help text.

We can't make any API calls before flag parsing is complete, because we won't know which host we're connecting to until after flag parsing. But flag parsing has to happen after we setup the child commands. Which means the experimental support also has this issue. I don't think we can ship 1.13 this way.

The order of events needs to be this:

  1. setup child commands and flags
  2. parse flags
  3. initialize client
  4. execute command

That means that flags and child commands can not depend on the server version.

I think we should do this:

  1. do not try and ping the server
  2. go back to the original plan with the version header, where we make a request first then fall back on error.
  3. for experimental use Hidden: isExperimental on the experimental commands. Add an environment variable DOCKER_EXPERIMENTAL that enables showing the commands by setting isExperimental=true`.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I run docker or docker --help to find out how to connect to an alternative host I'm going to get a connection error instead of the help text.

This is not correct, the error is not checked at this point. (At least that's how I planned it, maybe I did a mistake in the code)

In my option it works ok:

  • docker --help that is able to talk to an experimental server will display experimental commands and commands supported by the server
  • docker --help that is able to talk to a stable server will not display experimental commands and commands supported by the server
  • docker --help that is not able to connect to anything will not display experimental commands and all the commands known to the cli

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @mlaventure since it's not related specifically to the API versioning, but also experimental.

Copy link
Member

@dnephin dnephin Oct 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I guess my example was wrong, but I think the rest is still correct.

The server that it checks will be either the default (or the one set by DOCKER_HOST maybe, I'm not sure if that's read at this time). The --host or -H flag isn't parsed yet, so the behaviour is wrong if you have to use --host.

This could lead to some really strange behaviour where there is both a local daemon and some cloud edition. The command line is based on the local daemon, but then after flag parsing it points at the remote engine, which results in the incorrect behaviour when using any unsupported flags, instead of the helpful error message from the client.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, yes correct. It does not work with --host (it works with DOCKER_HOST)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh ok I get what you mean @dnephin .

So how about we remove all the checks from the cli pkg, so we always display commands/flags but we error out in the client package only instead ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might fix the issue. I think we could still hide the commands with this strategy: #27745 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dnephin WDYT about #28008

@vieux vieux force-pushed the cli_backward_compose_api branch 2 times, most recently from c698021 to d65d16e Compare October 28, 2016 21:10
@vieux vieux force-pushed the cli_backward_compose_api branch 3 times, most recently from 591eabe to e98e4a7 Compare November 9, 2016 00:59
@vieux vieux merged commit d3c780b into moby:master Nov 9, 2016
@vieux vieux deleted the cli_backward_compose_api branch November 9, 2016 02:27
thaJeztah added a commit to thaJeztah/docker that referenced this pull request Jan 16, 2017
Pull request moby#27745 added support for the
client to talk to older versions of the daemon. Various flags were added to
docker 1.13 that are not compatible with older daemons.

This PR adds annotations to those flags, so that they are automatically hidden
if the daemon is older than docker 1.13 (API 1.25).

Not all new flags affect the API (some are client-side only). The following
PR's added new flags to docker 1.13 that affect the API;

- moby#23430 added `--cpu-rt-period`and `--cpu-rt-runtime`
- moby#27800 / moby#25317 added `--group` / `--group-add` / `--group-rm`
- moby#27702 added `--network` to `docker build`
- moby#25962 added `--attachable` to `docker network create`
- moby#27998 added `--compose-file` to `docker stack deploy`
- moby#22566 added `--stop-timeout` to `docker run` and `docker create`
- moby#26061 added `--init` to `docker run` and `docker create`
- moby#26941 added `--init-path` to `docker run` and `docker create`
- moby#27958 added `--cpus` on `docker run` / `docker create`
- moby#27567 added `--dns`, `--dns-opt`, and `--dns-search` to `docker service create`
- moby#27596 added `--force` to `docker service update`
- moby#27857 added `--hostname` to `docker service create`
- moby#28031 added `--hosts`, `--host-add` / `--host-rm` to `docker service create` and `docker service update`
- moby#28076 added `--tty` on `docker service create` / `docker service update`
- moby#26421 added `--update-max-failure-ratio`, `--update-monitor` and `--rollback` on `docker service update`
- moby#27369 added `--health-cmd`, `--health-interval`, `--health-retries`, `--health-timeout` and `--no-healthcheck` options to `docker service create` and `docker service update`

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@deviantony
Copy link

Maybe I'm missing something but it looks like I cannot use a Docker 1.13 client to send an info request to a Docker 1.10 server.

Reproduction steps:

# Start a Docker 1.10 in Docker container and expose the Docker API port locally
$ docker run --privileged --name docker110 -d -p 7110:2375 docker:1.10-dind
e937dbff2126d02d0b33c337b50bf3069261a131c04eaf77dc219964a0efdf0d

# Query the Docker 1.10 engine
$ docker -H localhost:7110 info
Error response from daemon: client is newer than server (client API version: 1.24, server API version: 1.22)

# Docker version output
$ docker version
Client:
 Version:      1.13.0
 API version:  1.25
 Go version:   go1.7.3
 Git commit:   49bf474
 Built:        Tue Jan 17 09:50:17 2017
 OS/Arch:      linux/amd64

Server:
 Version:      1.13.0
 API version:  1.25 (minimum version 1.12)
 Go version:   go1.7.3
 Git commit:   49bf474
 Built:        Tue Jan 17 09:50:17 2017
 OS/Arch:      linux/amd64
 Experimental: false

@thaJeztah
Copy link
Member

@deviantony correct, "auto negotiation" works with a header that's sent by the daemon; since that header is added in docker 1.13, docker will only automatically fall back to version 1.12, which is the last version that did not set that header (see cli/command/cli.go#L169-L172)

If you want the client to talk to an even older version of the daemon, you can manually force it to use this API version through the DOCKER_API_VERSION environment variable, e.g.;

DOCKER_API_VERSION=1.21 ./docker version

@gcbartlett
Copy link

gcbartlett commented Jan 23, 2017

@thaJeztah, regarding interoperability with servers older than 1.12 (the blog does not note that restriction by the way), even without the header, it appears that the client does know the server's API version (e.g. the error message displays it...server API version: 1.21), so couldn't it automatically perform the equivalent of setting DOCKER_API_VERSION to automatically support earlier server versions?

Or could docker-machine env be updated to also return the appropriate DOCKER_API_VERSION version?

@thaJeztah
Copy link
Member

the blog does not note that restriction by the way

Thanks for noticing that; I'll see if I can find the right person to update the blog and clarify that

it appears that the client does know the server's API version

The client does not know the daemon version, but only prints the error; if you look at the error;

Error response from daemon: ......

So the client tried to connect to the daemon through API 1.24, but the daemon did not support anything above 1.22. While this information could be used for negotiation, the current design is to use a dedicated API endpoint for that.

Also note that DOCKER_API_VERSION was added for "debugging" purposes. Most things should work, but there may be corner-cases where the client does not take differences in the old API into account. Versions that are automatically negotiated with the 1.13 client do take differences into account (e.g. in some cases values need to be specified slightly different, or options are not supported by the older API version, and therefore silently ignored).

Or could docker-machine env be updated to also return the appropriate DOCKER_API_VERSION version?

This was implemented for a short while, but led to incorrect issues where the original machine was a 1.12 daemon, but upgraded to 1.13, but the environment variable still forced the client to use the old API.

@gcbartlett
Copy link

Thanks @thaJeztah. So is the recommendation for using older than 1.12 servers to continue to use dvm?

@thaJeztah
Copy link
Member

I think the recommendation would be to update those servers 😇 , but yes, using dvm (i.e, the correct client version to match the server) is for now the best solution.

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017
allow client to talk to an older server
andrewhsu pushed a commit to docker-archive/docker-ce that referenced this pull request May 19, 2017
Pull request moby/moby#27745 added support for the
client to talk to older versions of the daemon. Various flags were added to
docker 1.13 that are not compatible with older daemons.

This PR adds annotations to those flags, so that they are automatically hidden
if the daemon is older than docker 1.13 (API 1.25).

Not all new flags affect the API (some are client-side only). The following
PR's added new flags to docker 1.13 that affect the API;

- moby/moby#23430 added `--cpu-rt-period`and `--cpu-rt-runtime`
- moby/moby#27800 / moby/moby#25317 added `--group` / `--group-add` / `--group-rm`
- moby/moby#27702 added `--network` to `docker build`
- moby/moby#25962 added `--attachable` to `docker network create`
- moby/moby#27998 added `--compose-file` to `docker stack deploy`
- moby/moby#22566 added `--stop-timeout` to `docker run` and `docker create`
- moby/moby#26061 added `--init` to `docker run` and `docker create`
- moby/moby#26941 added `--init-path` to `docker run` and `docker create`
- moby/moby#27958 added `--cpus` on `docker run` / `docker create`
- moby/moby#27567 added `--dns`, `--dns-opt`, and `--dns-search` to `docker service create`
- moby/moby#27596 added `--force` to `docker service update`
- moby/moby#27857 added `--hostname` to `docker service create`
- moby/moby#28031 added `--hosts`, `--host-add` / `--host-rm` to `docker service create` and `docker service update`
- moby/moby#28076 added `--tty` on `docker service create` / `docker service update`
- moby/moby#26421 added `--update-max-failure-ratio`, `--update-monitor` and `--rollback` on `docker service update`
- moby/moby#27369 added `--health-cmd`, `--health-interval`, `--health-retries`, `--health-timeout` and `--no-healthcheck` options to `docker service create` and `docker service update`

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Upstream-commit: 5d2722f83db9e301c6dcbe1c562c2051a52905db
Component: cli
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.