Issue #327 Creating Docker client from env settings

[hferentschik]

Superseding pull request #149 and creating Docker client in daemon_dest as well as daemon_src from env settings.

Fix for issue #327

[TomSweeneyRedHat]

LGTM

[runcom]

@mtrmac ptal

[mtrmac]

In #149 we’ve talked about using types.SystemContext preferably to the environment; this still uses the environment.

@runcom do we still prefer types.SystemContext, or has anything changed in the past year to change the trade-offs and decision?

(I do prefer an explicit typed API to the environment, but only fairly weakly, more as a matter of style/taste; of course either can be made to work for any purpose by a sufficiently determined caller :) )

[runcom]

Yes we prefer having control via the API about how the client is initialized so I'm not in favor of this

[hferentschik]

I do prefer an explicit typed API to the environment, but only fairly weakly, more as a matter of style/taste;

Why not both? I agree, having programmatic control makes sense, but a simple solution which works probably a lot of times out of the box, is the environment approach. If nothing else, it is the better default implementation (compared to what there is now).

So why not merge this as as is and then in a follow up pull request, modify types.SystemContext to pass the required information. The code would then check types.SystemContext whether it contains explicitly set Docker daemon connection information. If so, it uses it. If not, it tries to create a connection using environment variables. IMO, there is nothing to loose by merging this pull request and can easily be extended in a backwards compatible way.

[mtrmac]

Why not both?

The point of not having the implicit environment use is that the caller can be certain that the library only uses the information passed via the API, and not something obtained from an (any) other information source. Having both an API mechanism and an environment mechanism would not give the caller this assurance.

Please add a new // === docker/daemon.Transport overrides === section to types.SystemContext, and either add the variables which you need to override (DockerDaemonHostname, maybe DockerDaemonCertPath + DockerDaemonInsecureSkipTLSVerify, or something like that), or at the very least an opt-in into using the environment (DockerDaemonUseEnvironmentVariables).

Then use these values in the calls to client.NewClient/client.NewEnvClient or whatever—maybe in a shared helper function to minimize code duplication.

[hferentschik]

Please add a new // === docker/daemon.Transport overrides === section to types.SystemContext, and either add the variables which you need to override (DockerDaemonHostname, maybe DockerDaemonCertPath + DockerDaemonInsecureSkipTLSVerify, or something like that), or at the very least an opt-in into using the environment (DockerDaemonUseEnvironmentVariables).

Ok, will do. Expect an update soon ... :-)

[hferentschik]

@mtrmac, I pushed an update to the pull request which does use SystemConext in order to create the Docker client. Works fine for programmatic use of the API. Will this work with Skopeo as well? Is there a way to configure the SystemContext via the Skopeo CLI as well?

Do you think we also should add a flag DockerClientFromEnv to _ SystemConext_ as well? We talked about it at some stage. Setting it to true would mean NewEnvClient would be used. The difference being of course that it would become an explicit choice.

[mtrmac]

ACK on the overall approach.

[mtrmac]

DockerCertPath is defined for docker:, not for docker-daemon:, and more importantly with a different semantics.

Please add a separate DockerDeamonCertPath, or something like that.

[hferentschik]

I see, will do.

[mtrmac]

Does using api.DefaultVersion here, instead of hard-coding "1.22", work? Wouldn’t that mean that random rebases would break compatibility with earlier servers?

Cc: @runcom

[hferentschik]

Wouldn’t that mean that random rebases would break compatibility with earlier servers

Not sure what you mean with random rebases? I find "1.22" random tbh. I think the default should be the default as per used library. If you need a different version, you should set it explicitly.

[mtrmac]

A ”random rebase” = the docker/docker package is re-vendored to any newer version, for whatever reason (new feature needed elsewhere, a security fix, whatever).

The API version is not a magic string that one should cargo-cult from api.DefaultVersion; it is used to determine compatibility, and always using the latest one seems unwarranted.

To be fair, it’s entirely possible that 1.22 is an incorrect value (it’s likely enough a frozen value of api.DefaultVersion as of one time) and the code could in fact work with an earlier version; but that doesn’t make always using the latest one any more correct.

[hferentschik]

To be fair, it’s entirely possible that 1.22 is an incorrect value (it’s likely enough a frozen value of api.DefaultVersion as of one time)

Right, that's what I think as well. It seems neither 1.22 nor api.DefaultVersion might be ideal, but given the choice, I'd go for the latter.

That said, I could introduce a constant within this package and we can decide whether we want to set it to "1.22" or api.DefaultVersion.

and the code could in fact work with an earlier version; but that doesn’t make always using the latest one any more correct.

[mtrmac]

and the code could in fact work with an earlier version; but that doesn’t make always using the latest one any more correct.

It does make it less useful, though, because using an earlier API version allows the client to work with more versions of the server. The lower the version specified by the client, the more server versions work; that also makes api.DefaultVersion, which is always the highest version available, the worst possible choice if I understand the situation correctly.

(Unless different versions of the API could be equally compatible but have different semantics, which is I’ve asked @runcom to confirm.)

[hferentschik]

hmm, not sure, but I guess we can just keep the 1.22. I would love to hear @runcom's opinion as well

[mtrmac]

Non-blocking: I’d prefer the path for systemCtx == nil and “systemCtx.WhateverMember is unset”, which are defined to be equivalent, to be shared, to minimize duplication and risk of different behaviors.

[hferentschik]

for me this is a guard clause. I prefer to have this out of my way and then being able to write the code knowing that I don't have to deal with nil

[mtrmac]

If you want to, guard using if systemCtx == nil { systemCtx = &types.SystemContext{} } then.

But I’d really prefer to only have one path with one set of defaults. It already takes non-trivial code inspection to verify that the httpClient/nil difference is actually equivalent.

[mtrmac]

Why does it make sense to make this overridable? The required API version is determined by the API operations the subpackage calls. Using an older API version would not work, using a newer one should not make a difference.

Cc: @runcom to keep me honest.

[hferentschik]

Using a newer version "should" work, but it might be that in some cases it won't. In these cases setting an explicit version might help. I am trying to mimic what client.NewEnvClient does and there it is possible to set the DOCKER_API_VERSION environment variable to enforce the API version. Since we are not going to use client.NewEnvClient I want to at least provide a solution using SystemContext which has equivalent functionality.

[mtrmac]

but it might be that in some cases it won't.

Cases like what?

If we don’t now know what this can be good for, I’d rather not add it; it’s extra burden to maintain for no known benefit, and removing it in the future would break API compatibility. We can always add features in the future, with less disruption than removing something, even if useless.

All uses of the environment variable I can find from a quick googling are caused by the client defaulting to a too new version when it doesn’t need those features, and using DOCKER_API_VERSION to downgrade to an lower but acceptable version. Using a low enough version in the first place should make DOCKER_API_VERSION / this override unnecessary.

[hferentschik]

If we don’t now know what this can be good for, I’d rather not add it; it’s extra burden to maintain for no known benefit, and removing it in the future would break API compatibility. We can always add features in the future, with less disruption than removing something, even if useless.

I disagree. For me it was important to create something which provides the same functionality as client.NewEnvClient. If the programmatic approach cannot offer this, I think we should also add a flag to the context which makes the code use client.NewEnvClient.

For what it's worth the Minishift command minishift docker-env makes use of the variable as well (I cannot tight now recall the exact details why this got added to the output of this command)

[mtrmac]

For me it was important to create something which provides the same functionality as client.NewEnvClient.

What for? We are making exactly two calls to the client, for which we can in advance determine the exact API version. The users can not, by environment variables, or anything else, cause this package to make any other API calls, so I can’t see any situation in which the callers could in any way benefit from changing the declared clients’ API version. The only thing that can do is break things.

[mtrmac]

Still outstanding.

[masaeedu]

using a newer one should not make a difference.

@mtrmac Using a newer one does make a difference, because certain implementations of the Docker engine API simply don't support the old interface at all. Specifically, the Docker engine API on Windows just errors out if you use API version v1.22:

Error loading image from docker engine: Error response from daemon: client version 1.22 is too old. Minimum supported API version is 1.24, please upgrade your client to a newer version

This makes this library unusable against a Windows Docker host.

[masaeedu]

Whether they actually made enough breaking changes from 1.22 to 1.24 to warrant rejecting any calls made by a client requesting the older version of the API is probably a discussion for a separate thread in https://github.com/moby/moby.

[mtrmac]

That would be a good reason to add some kind of API version detection, based on docker/docker/client.Client.Ping (and actually check what were the relevant API changes / version boundaries for our purposes); not a very good reason to give users a “I don’t know what changed and what will break, just do it” hammer and to force users to set an environment variable every time manually.

[mtrmac]

NO this inverts the semantics.

Also, the default (i.e. when the boolean is not set by the user in types.SystemContext, must be be secure; i.e. the SystemContext member should be …SkipTLSVerify, preferably with the Insecure warning in there as well.

[hferentschik]

ok

[hferentschik]

The confusion came from me trying to mimic/align with DOCKER_TLS_VERIFY. Logic is inverted here. I'll add the 'Skip'

[mtrmac]

Instead, create a fixtures/certs subdirectory inside the package, and refer to it by path. No need to create temporary directories, create copies of the files, or to carry them as strings inside Go code.

[hferentschik]

I guess its a matter of choice, no?

[mtrmac]

It’s much more code with embedding everything into *.go, and it’s more difficult to inspect the certificates. Are there any upsides to doing it this way?

[mtrmac]

Why the rename from ctx to systemCtx? ctx is used throughout the library.

[hferentschik]

to align daemon_dest and daemon_src. But if the guideline is to use ctx, I am fine with that as well.

[mtrmac]

TLS verification must be on by default. See also the other comment.

[mtrmac]

Please create a separate === docker/daemon.Transport overrides === section, and name the new members DockerDaemon to make them separate from the docker/distribution registry client.

[hferentschik]

Got it now. This is a quite subtle difference.

[mtrmac]

Will this work with Skopeo as well?

Not automatically :)

Is there a way to configure the SystemContext via the Skopeo CLI as well?

Sure, extend skopeo’s cmd/skopeo/utils.go:contextFromGlobalOptions.

Do you think we also should add a flag DockerClientFromEnv to _ SystemConext_ as well?

Do you need it? If you need it, you need it; if you don’t, let’s not add unnecessary complexity.

[hferentschik]

Sure, extend skopeo’s cmd/skopeo/utils.go:contextFromGlobalOptions.

I see. I was just wondering. For my purposes, I am fine with the current SystemContext.

Do you think we also should add a flag DockerClientFromEnv to _ SystemConext_ as well?

Do you need it? If you need it, you need it; if you don’t, let’s not add unnecessary complexity.

I don't need it. Let's leave it like is then. It is a potential addition one can make if the needs should come up.

BTW, seems CI fails due to some Skopeo compilation issues. Not quite sure what to do about this? Is it not a bit odd anyway that this runs as part of the pull request build?

[mtrmac]

We have that test to ensure that skopeo does not break. This might be a simple vendoring issue, see the third part of https://github.com/projectatomic/skopeo/blob/master/README.md#dependencies-management for how to deal with that. But, in the first place, does the api.DefaultVersion use make sense?

[hferentschik]

@mtrmac Please have a look at the latest changes.

[hferentschik]

Any other changes required here?

[mtrmac]

(After handling the review comments, please follow https://github.com/projectatomic/skopeo/blob/master/README.md#dependencies-management to prepare a skopeo PR showing passing skopeo tests.)

[mtrmac]

This means that DockerDaemonInsecureSkipTLSVerify does not work if DockerDaemonCertPath is not set.

[hferentschik]

If there is no cert, there is also TLS, right? This is aligned with the implementation of client.NewEnvClient and effectively is behaves now as if no context is specified.

I think this behaviour is reasonable.

[mtrmac]

No; the default behavior is to use the system-wide CA store to authenticate the server, which works fine without supplying certificates.

It’s the other way around AFAICS: because DockerDaemonCertPath fills all three values of tlsconfig.Options, once DockerDaemonCertPath is set, all three certificates must exist or tlsconfig.Client fails if I’m not mistaken. That would force a caller who just wants to disable TLS verification to provide three cert/key files, of which at least the CA one is completely useless. (Yeah, NewEnvClient seems to do it the same way; it still does not make sense.)

[hferentschik]

fair enough, let me change this a bit

[mtrmac]

It seems strange that this CheckRedirect value is set only if ctx.DockerDaemonCertPath != "".

[hferentschik]

If it is "", then we are back to the case where we pass no http.client into client.NewClient and CheckRedirect occurs there:

	if client != nil {
		if _, ok := client.Transport.(http.RoundTripper); !ok {
			return nil, fmt.Errorf("unable to verify TLS configuration, invalid transport %v", client.Transport)
		}
	} else {
		transport := new(http.Transport)
		sockets.ConfigureTransport(transport, proto, addr)
		client = &http.Client{
			Transport:     transport,
			CheckRedirect: CheckRedirect,
		}
	}

[mtrmac]

Ah, true. But just looking at the above, the ctx.DockaerDaemonCertPath is missing the sockets.ConfigureTransport call (i.e. *FromEnvironment support).

Having two paths with which can (i.e. long-term will) diverge like this just seems painful.

If all that we want to edit is the TLS configuration, the cleanest way to do that is if the only difference between the settings we supply is the TLS configuration.

[hferentschik]

I think this will be implicitly fixed by the change I am making for #328 (comment)

[mtrmac]

That change removed the difference caused by ctx.DockerDaemonCertPath, but the behavior still depends on ctx != nil.

Really, figuring that the various paths are equivalent is too hard to write, let alone to maintain. Please just implement exactly one path; the cost of a few ctx != nil checks in if ctx != nil && ctx.DockerDaemon… {} isn’t that much.

[hferentschik]

That change removed the difference caused by ctx.DockerDaemonCertPath, but the behavior still depends on ctx != nil.

right, if I don't get a context, nil is returned. This is different from returning tlsconfig.Options{}.

if ctx != nil && ctx.DockerDaemon… {} isn’t that much.

This will guard against a panic and potentially accessing DockerDaemon in case ctx is nil, but at some stage I still would have to return nil or a ctx.DockerDaemon instance (provided I want to keep the semantics of the function)

[mtrmac]

This is different from returning tlsconfig.Options{}.

Yes, but it is defined to be the same. docker-daemon: behavior should not change just because the caller needed to define a completely docker-daemon:-unrelated setting like SignaturePolicyPath.

but at some stage I still would have to return nil or a ctx.DockerDaemon instance (provided I want to keep the semantics of the function)

I don’t understand what you mean by this. ctx == nil and ctx.DockerDaemon… /* is unset */ should always behave precisely equivalently.

// NewDockerClient initializes a new API client based on the passed SystemContext.
func newDockerClient(ctx *types.SystemContext) (*dockerclient.Client, error) {
	httpClient, err := tlsConfig(ctx)
	if err != nil {
		return nil, err
	}

	host := dockerclient.DefaultDockerHost
	if ctx != nil & ctx.DockerDaemonHost != "" {
		host = ctx.DockerDaemonHost
	}

	return dockerclient.NewClient(host, defaultAPIVersion, httpClient, nil)
}

func tlsConfig(ctx *types.SystemContext) (*http.Client, error) {
	options := tlsconfig.Options{}
	if ctx != nil && ctx.DockerDaemonInsecureSkipTLSVerify {
		options.InsecureSkipVerify = true
	}

	if ctx != nil && ctx.DockerDaemonCertPath != "" {
		options.CAFile = filepath.Join(ctx.DockerDaemonCertPath, "ca.pem")
		options.CertFile = filepath.Join(ctx.DockerDaemonCertPath, "cert.pem")
		options.KeyFile = filepath.Join(ctx.DockerDaemonCertPath, "key.pem")
	}

	tlsc, err := tlsconfig.Client(options)
	if err != nil {
		return nil, err
	}

	return &http.Client{
		Transport: &http.Transport{
			TLSClientConfig: tlsc,
		},
		CheckRedirect: dockerclient.CheckRedirect,
	}, nil
}

?

[hferentschik]

Right, this is nicer. However, this also changes a bit of the semantics. Now you always create an http.Client which is passed to dockerclient.NewClient. The original code was passing nil. My intent was to keep this in place. dockerclient.NewClient does actually do a check whether httpClient is nil and if so get the TLS config from the default transport. I wanted to avoid any potential change in behaviour. I am good with this as well, but just want to point out the difference.

[mtrmac]

I wanted to avoid any potential change in behaviour.

That needs to be balanced with 1) &types.SystemContext{} not changing behavior, and 2) non-empty SystemContext{DockerDaemonSomething} not changing unrelated aspects of the configuration.

I agree it would be nice to be able to get a http.Client exactly equivalent to passing nil out of the …/docker/client package, but that’s not available AFAICT.

[hferentschik]

Sure. It's always a trade-off. So we are good now?

[hferentschik]

After handling the review comments, please follow https://github.com/projectatomic/skopeo/blob/master/README.md#dependencies-management to prepare a skopeo PR showing passing skopeo tests

See containers/skopeo#440

[hferentschik]

See containers/skopeo#440

CI passed :-)

[hferentschik]

@mtrmac could you have a look at my latest change. I hope this addresses your last concerns.

[mtrmac]

The DockerDaemonAPIVersion option existence is still bothering me.

Perhaps I am just being ignorant and there is a reason it must exist; if so, please point me to something I can study. But having this option just because the generic docker/docker/client does is not sufficient when the only known effect of that for docker/daemon.Transport identified so far is to break it. Why would we have an option like that?

[mtrmac]

While dropping the DockerDaemonCertPath from here is an improvement, this is never true because newDockerClient has a similar check.

[hferentschik]

true, but imo the function needs to be consistent in itself and is responsible for its own verification of parameters. Who says that newDockerClient will always be the only caller if this method.

[hferentschik]

so what's about I drop the 'if ctx == nil' case? In this case, there will only be a single path. _ newDockerClient_ is the only caller and it does already the nil check as you pointed out.

[mtrmac]

No, there would still be two paths: the newDockerClient if ctx == nil path (with nil httpClient, i.e. *FromEnvironment called), and the tlsConfig path.

[mtrmac]

That change removed the difference caused by ctx.DockerDaemonCertPath, but the behavior still depends on ctx != nil.

Really, figuring that the various paths are equivalent is too hard to write, let alone to maintain. Please just implement exactly one path; the cost of a few ctx != nil checks in if ctx != nil && ctx.DockerDaemon… {} isn’t that much.

[hferentschik]

Perhaps I am just being ignorant and there is a reason it must exist; if so, please point me to something I can study.

From the Minishift side, it relates to this issue projectatomic/vagrant-service-manager#113. Effectively it was about avoiding this error:

Error response from daemon: client is newer than server (client API version: 1.22, server API version: 1.20)

This was an issue pre Docker 1.12 - moby/moby#21930 (comment). After 1.12, the version negotiation was introduced. On the CDK/Minishift side we initially used Docker 1.10, so for us, it was needed. Now we are using 1.12.6 and in fact don't have the need for it anymore either.

AFAIU, version negotiating as well as DOCKER_API_VERSION have their space though. If DOCKER_API_VERSION is set version negotiating is bypassed and the specified version is used. This might still be useful for the case where a Docker version pre 1.12 is run on the daemon side.

I'll remove it if you feel so strongly about it. For me it would make sense to keep it. Let me know what you want me to do.

[mtrmac]

Thanks for the pointers.

From the Minishift side, it relates to this issue projectatomic/vagrant-service-manager#113. Effectively it was about avoiding this error:

Error response from daemon: client is newer than server (client API version: 1.22, server API version: 1.20)

As a general principle, this should not be worked around by making the client API version overridable, but by hard-coding a low enough version: hard-coding the API version because it is a property of the client code; not something the environment / sysadmin knows better than the client code. That seems to have been an acceptable client behavior at least back in 2015, looking at the history of docker/docker/api/server/middleware.

This failure has basically been specific to the /usr/bin/docker client, and needed because that client supports ~all possible API calls, so by the same generic logic the “low enough version” must be the latest version. The DOCKER_API_VERSION workaround (which has not been 100% reliable, see the IPv6 example in moby/moby#21930), in some sense, told the /usr/bin/docker binary to pretend that some of its functionality does not exist.

https://github.com/moby/moby/pull/27745/files#diff-b4a7ebaa86bd3765289ff28c6934850fR109 + https://github.com/moby/moby/pull/27745/files#diff-411aa998ea86f3e82a88f184f4f3135fR24 change the client to choose a version appropriate for the specific API call, downgrading with limited functionality if necessary; but again, it is hard-coding a “low enough version” for each particular API call.

These /usr/bin/docker specifics do not apply in the general client case, and in particular do not apply to the docker-daemon: client: This dynamic logic is not needed for our client because it is only making exactly two calls, we need neither to use the latest version (we wouldn’t benefit from the newer API) nor we can degrade to an earlier version (there would be no functionality left).

(There also has been the Client and server don't have the same version (client: %s, server: %s) failure case in /usr/bin/docker, removed in the same PR, but that does not apply to containers/image at all because only /usr/bin/docker sets the User-Agent: Docker-Client/… header.)

AFAICT, because we hard-code the 1.22 API, the API call should work fine for any daemon version ≥ API 1.22, i.e. Docker version ≥ 1.10.

So, yes, please remove it.

[hferentschik]

So, yes, please remove it.

fair enough

[hferentschik]

@mtrmac I removed the Docker API version from the SystemContext and changed newDockerClient and tlsConfig as per your suggestion. WDYT?

[mtrmac]

👍

Thanks!

[runcom]

This just needs a rebase i guess.

[runcom]

LGTM

[hferentschik]

rebased

[runcom]

Merging, please go ahead and sync up containers/image in your skopeo PR containers/skopeo#440

[hferentschik]

Merging

Awesome. Thanks.

please go ahead and sync up containers/image in your skopeo PR containers/skopeo#440

Ok. will do

[hferentschik]

@runcom - Skopeo pull request updated - containers/skopeo#440 and passing