client: negotiate api version before handling version-specific code by thaJeztah · Pull Request #46463 · moby/moby

thaJeztah · 2023-09-12T12:16:13Z

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 · 2023-09-12T14:44:25Z

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 · 2023-09-20T12:02:21Z

+// 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.


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.

DOH! Thanks.

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

Fixed, and slightly rephrased PTAL

akerouanton · 2023-09-20T12:11:56Z

+	//
+	// Normally, version-negotiation (if enabled) would not happen until
+	// the API request is made.
+	cli.checkVersion(ctx)


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).

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

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.

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 Signed-off-by: Sebastiaan van Stijn <[email protected]>

akerouanton

LGTM

thaJeztah added area/api API area/cli Client status/2-code-review kind/bugfix PR's that fix bugs labels Sep 12, 2023

thaJeztah mentioned this pull request Sep 12, 2023

libnet: Make sure network names are unique #46251

Merged

thaJeztah force-pushed the fix_version_checks branch 2 times, most recently from bd2e92a to bca86fa Compare September 12, 2023 12:35

thaJeztah force-pushed the fix_version_checks branch from bca86fa to b4c1aad Compare September 12, 2023 14:57

thaJeztah mentioned this pull request Sep 12, 2023

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

Open

thaJeztah marked this pull request as ready for review September 13, 2023 12:29

vvoland reviewed Sep 19, 2023

View reviewed changes

Comment thread client/client.go Outdated

thaJeztah force-pushed the fix_version_checks branch from b4c1aad to e1e2dbe Compare September 20, 2023 11:07

thaJeztah added this to the 25.0.0 milestone Sep 20, 2023

vvoland approved these changes Sep 20, 2023

View reviewed changes

akerouanton reviewed Sep 20, 2023

View reviewed changes

thaJeztah force-pushed the fix_version_checks branch from e1e2dbe to e690724 Compare September 20, 2023 13:23

akerouanton approved these changes Sep 20, 2023

View reviewed changes

thaJeztah merged commit d8a51d2 into moby:master Sep 20, 2023

thaJeztah deleted the fix_version_checks branch September 20, 2023 17:48

This was referenced Feb 23, 2024

client: fix connection-errors being shadowed by API version errors #47440

Merged

Misleading error message for docker secret ls docker/cli#4890

Closed

neersighted mentioned this pull request Feb 29, 2024

[25.0 backport] client: fix connection-errors being shadowed by API version errors #47470

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: negotiate api version before handling version-specific code#46463

client: negotiate api version before handling version-specific code#46463
thaJeztah merged 1 commit intomoby:masterfrom
thaJeztah:fix_version_checks

thaJeztah commented Sep 12, 2023 •

edited

Loading

Uh oh!

thaJeztah commented Sep 12, 2023

Uh oh!

Uh oh!

vvoland left a comment

Uh oh!

akerouanton Sep 20, 2023

Uh oh!

thaJeztah Sep 20, 2023

Uh oh!

thaJeztah Sep 20, 2023

Uh oh!

akerouanton Sep 20, 2023

Uh oh!

thaJeztah Sep 20, 2023

Uh oh!

akerouanton Sep 20, 2023 •

edited

Loading

Uh oh!

akerouanton left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

thaJeztah commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Sep 12, 2023

Uh oh!

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

akerouanton Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

akerouanton Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

akerouanton Sep 20, 2023 • 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!

akerouanton left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thaJeztah commented Sep 12, 2023 •

edited

Loading

akerouanton Sep 20, 2023 •

edited

Loading