client: negotiate api version before handling version-specific code

[thaJeztah]

• relates to libnet: Make sure network names are unique #46251 (comment)

We try to perform API-version negotiation as lazy as possible (and only execute when we are about to make an API request). However, some code requires API-version dependent handling (to set options, or remove options based on the version of the API we're using).

Currently this code depended on the caller code to perform API negotiation (or to configure the API version) first, which may not happen, and because of that we may be missing options (or set options that are not supported on older API versions).

This patch:

• splits the code that triggered API-version negotiation to a separate Client.checkVersion() function.
• updates NewVersionError to accept a context
• updates NewVersionError to perform API-version negotiation (if enabled)
• updates various Client functions to manually trigger API-version negotiation

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

[thaJeztah]

One thing I'm considering (but probably better for a separate follow-up PR), is if we should attach the API version to the context. I recall Cory had a similar approach (but I recall that one may have been more complicated).

We already have VersionFromContext() on the daemon side;

moby/api/server/httputils/httputils.go

Lines 110 to 122 in 9641c90

	// VersionFromContext returns an API version from the context using APIVersionKey.
	// It panics if the context value does not have version.Version type.
	func VersionFromContext(ctx context.Context) string {
	if ctx == nil {
	return ""
	}
	if val := ctx.Value(APIVersionKey{}); val != nil {
	return val.(string)
	}
	return ""
	}

We could still make the version a "once" operation, but per request (not necessarily per lifetime of the Client)

And we need the API version of API requests (in which case we would (should) always have a context anyway).

But some parts of the code may need to switch from "if no api version on the context, then use default" code.

[vvoland]

LGTM

[akerouanton]

Suggested change

	// if less than the current supported version.
	//
	// It performs API-version negotiation if the Client is configured
	// with this option, otherwise it it uses client's default version.
	// is less than the current supported version.
	//
	// It performs API-version negotiation if the Client is configured
	// with this option, otherwise it uses client's default version.

[thaJeztah]

DOH! Thanks.

Also: I need to check why we don't have the dupwords linter enabled 🤔

[thaJeztah]

Fixed, and slightly rephrased PTAL

[akerouanton]

I think this call is unneeded since the calls to NewVersionError below will do it anyway. But I'm not sure how go compiler order operations (ie. does it call NewVersionError first and check all conditions after?) 🤔

Having to call cli.checkVersion whenever versions.* functions are used feels a bit weird to me, IMO it's error-prone. I think the client should have its own helper functions wrapping versions.* to automatically call cli.checkVersion(ctx).

[thaJeztah]

Yes, this one is indeed somewhat redundant (I probably should've called that out here), because cli.NewVersionError would trigger this as well.

I decided to put it here for visibility, and because I didn't want this to depend on the implicit side-effect of cli.NewVersionError doing the lookup (if the cli.NewVersionError lines would ever be removed, or moved after the other checks, we would potentially introduce a regression).

Having to call cli.checkVersion whenever versions.* functions are used feels a bit weird to me

Yes, I fully agree, and I don't really like this PR for that. My goal in this PR was to fix the immediate issue (which I think is real), and making the fix somewhat "back portable", and move from there.

We need to look at a good approach to;

• Allow API version negotiation
• Have (keep) it as "lazy" as possible (we had issues in the past where local operations caused an API call to be made, which for sure isn't desirable)
• Allow concurrency (or at least, make things more "per request") for situations where the client is used as part of a longer-lived process (many places in the current design were made based on the assumptions that it would be used as part of the CLI, and that to be a short-lived process (run a command, and exit))

I have a follow-up / separate PR pending that would allow us to use the context to communicate the version (but that PR in itself is not yet tying into that; only moving the code so that we CAN consider doing that); reviews on that are definitely welcome as well 😄

• api: move version-related code to api/types/versions package #46464

[akerouanton]

Yes, I fully agree, and I don't really like this PR for that. My goal in this PR was to fix the immediate issue (which I think is real), and making the fix somewhat "back portable", and move from there.

I'm fine with this PR as it's addressing an immediate issue 😉 But yeah, we should look into all the issues we have here.

[akerouanton]

LGTM