api: move version-related code to api/types/versions package by thaJeztah · Pull Request #46464 · moby/moby

thaJeztah · 2023-09-12T17:12:31Z

relates to client: negotiate api version before handling version-specific code #46463 (client: negotiate api version before handling version-specific code #46463 (comment))

implement versions.FromContext
implement versions.WithVersion to return a context with the version attached.
deprecates the APIVersionKey type in favor of WithVersion.

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

thaJeztah · 2023-10-12T11:10:54Z

Ah, dang. Needs another rebase 😂

thaJeztah · 2023-10-12T11:44:12Z

Discussing this PR with @rumpl and we both considered that versions (plural) is a bit of an awkward name (and not very idiomatic go) for the package.

While this package existed for some time already, usage outside of this repository is limited (some locations in docker/cli, and for some of those, we should consider moving that logic into the client code).

As we're moving things in this PR, we might as well rip of the bandaid and rename the package to version (singular); I'll have a look at doing so later.

corhere · 2023-10-12T19:09:05Z

+package versions
+
+// DefaultVersion of Current REST API
+const DefaultVersion = "1.44"


Suggested change

const DefaultVersion = "1.44"

const Default = "1.44"

corhere · 2023-10-12T19:09:14Z

+
+// MinVersion represents the Minimum REST API version supported.
+// by the API server.
+const MinVersion = minVersion


Suggested change

const MinVersion = minVersion

const Min = minVersion

corhere · 2023-10-12T19:22:24Z

I don't think we should make this part of the API surface. It's only used by the server and in my opinion it would be inappropriate to pass API version through client contexts.

corhere

An API version is not an API type. I do not understand the motivation behind this change.

corhere · 2025-07-29T20:22:14Z

+// NoBaseImageSpecifier is the symbol used by the FROM
+// command to specify that no base image is to be used.
+//
+// Deprecated: this const is no longer used and will be removed in the next release.
+const NoBaseImageSpecifier = "scratch"


Why include deprecated things in the very first release of the github.com/moby/moby/api module? Any existing importers will be importing them from a different import path: github.com/docker/[email protected]+incompatible

Yes, we won't be needing this. This PR (and this change) was before it was fully clear how we'd make the transition, but with the aliasing PR (#50475) we can tape over the differences (keep deprecated ones only in that branch, and technically no need to mark them deprecated there as they would never be deprecated in docker/docker (that module just stops to exist / receive updates).

Opened a PR to remove it; api: remove deprecated NoBaseImageSpecifier #50574

corhere · 2025-07-29T20:30:15Z

+// API requests for API versions lower than the configured version produce
+// an error.
+const Min = "1.24"


What is the meaning of this constant in the context of the github.com/moby/moby/api module? The client may have a minimum API version it can talk to a daemon with, and the daemon certainly has a minimum supported API version it can handle requests for, but what are the semantics of the "minimum version" for the api module?

This PR needs some updating (I rebased it to see what remained, and to prevent it fully bit-rot).

w.r.t. a const for Min - even if we not end up using it as it's used in this PR, I think we need a const in the api module that defines the minimum version of the API that's implemented. i.e., there's structs from long-gone API versions that may no longer exist in the API module, so nobody should implement either an API server, or client with the expectations that it's able to fully handle those versions.

I don't think we need a const in the module. The minimum supported API version will never change for the Go module github.com/moby/moby/api since it would be a breaking change to drop support for an old API version. Documenting the minimum supported API version would be sufficient.

corhere · 2025-07-29T20:43:55Z

+// APIVersionKey is the API version used for the request's context.
+//
+// Deprecated: use [WithVersion].
+type APIVersionKey = apiVersionKey


There is no good reason to add deprecated items to a brand new package in a brand new Go module! If you're going to provide utilities to convey API version through contexts in the moby/moby/api module (which I vehemently oppose doing!) then for sanity's sake please don't export the context key! Importers can only use it to do one thing that the exported functions do not already afford doing: violate the invariants of versions.FromContext.

corhere · 2025-07-29T21:03:52Z

+// WithVersion creates a new context from the given parent context, with
+// the given API version attached as value. It returns the parent context
+// unmodified if the API version is empty. Similarly, if the parent context
+// already has the given version as value, no new context is created, and
+// the parent is returned unmodified.
+//
+// WithVersion uses [context.WithValue], and it panics if a nil-context
+// is passed as parent.
+func WithVersion(parent context.Context, version string) context.Context {
+	if version == "" {
+		return parent
+	}
+	if v := FromContext(parent); v == version {
+		return parent
+	}
+	return context.WithValue(parent, apiVersionKey{}, version)
+}
+
+// FromContext returns an API version from the context. It panics if the
+// context value does not have the right type.
+func FromContext(ctx context.Context) string {
+	if ctx == nil {
+		return ""
+	}
+
+	if val := ctx.Value(apiVersionKey{}); val != nil {
+		return val.(string)
+	}
+
+	return ""
+}


Okay, sure; affording clients the ability to override the API version per request is a potentially interesting idea that may be worth exploring. And there may even be arguments in favour of utilizing contexts to do so within the client. (There are also arguments against, but I digress...) Just please don't tie ourselves into knots in an effort to deduplicate the implementations of disparate concepts that happen to be superficially similar! It's okay to have two copies of WithVersion and FromContext in the daemon internals and client module! Client applications have no use for versions.FromContext: the only application-provided functions which the client would call with a context are the dialer callbacks, which have no use for the client's API version. Applications which want to pass versioning information in contexts may do so by attaching their own custom values to the context which don't collide with our context values.

(IMO it would be more natural to model multi-version clients by just having multiple *client.Client instances configured to make requests with different API versions. We could provide e.g. (*client.Client).Clone() to make this even more convenient. Passing optional function parameters through contexts is not best practice.)

It's okay to have two copies of WithVersion and FromContext in the daemon internals and client module!

That's an idea worth exploring (again, this PR was originally opened before client and api became separate modules).

I do indeed consider binding the API version to use to an operation (which likely would mean "for the lifecycle of the context"). One consideration (but requires a larger refactor) is to try to move away from the "god object" we currently have, and how we conflate the http client ("request") with the client. Separating those could mean we attach these properties to that client (we tried to make the version handling "lazy", which in that case would mean; when it's time to construct that). The "Client" could still query that client to see what version is gonna be used for some of the version-specific feature-detection / downgrading we have. Having those parts (and how to handle) exposed possibly would help with developing "api extensions" out of band as well.

Haven't fully thought that through yet, but have it in the back of my head.

Copilot

Pull Request Overview

This PR refactors version-related code by moving it from the api package to a new api/types/versions package, implementing context-based version management functions and deprecating the old APIVersionKey type in favor of the new WithVersion function.

Moves version constants from api/common.go to api/types/versions/consts.go
Implements versions.FromContext and versions.WithVersion for context-based version management
Deprecates httputils.APIVersionKey and httputils.VersionFromContext in favor of the new versions package

Reviewed Changes

Copilot reviewed 23 out of 30 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
api/common.go	Removes deprecated version constants
api/types/versions/consts.go	Adds version constants (Min, Default)
api/types/versions/context.go	Implements context utilities for version management
api/types/versions/context_test.go	Tests for context version functions
daemon/server/httputils/httputils.go	Removes deprecated version utilities
daemon/server/httputils/httputils_deprecated.go	Adds deprecated aliases for backward compatibility
daemon/server/middleware/version.go	Updates to use new versions package
daemon/config/config.go	Updates import to use versions.Min
Multiple route files	Updates all version context access to use new API

thompson-shaun · 2025-07-31T18:39:41Z

Based on discussion with @corhere and @thaJeztah

move api/types/versions to somewhere else (not the destination in this PR as originally proposed)
drop version consts
keep the context in daemon/server/httputils

- implement versions.FromContext - implement versions.WithVersion to return a context with the version attached. - deprecates the APIVersionKey type in favor of WithVersion. Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah added area/api API status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Sep 12, 2023

thaJeztah force-pushed the context_version branch 2 times, most recently from 2ab5b9e to 115b39d Compare September 12, 2023 18:02

thaJeztah commented Sep 13, 2023

View reviewed changes

Comment thread api/types/versions/context.go Outdated

thaJeztah force-pushed the context_version branch from 115b39d to 2fdc8c2 Compare September 13, 2023 11:14

thaJeztah mentioned this pull request Sep 20, 2023

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

Merged

thaJeztah force-pushed the context_version branch from 2fdc8c2 to 9ce4002 Compare September 20, 2023 19:22

vvoland approved these changes Oct 12, 2023

View reviewed changes

thaJeztah force-pushed the context_version branch from 9ce4002 to bb0170d Compare October 12, 2023 11:12

rumpl approved these changes Oct 12, 2023

View reviewed changes

corhere requested changes Oct 12, 2023

View reviewed changes

thaJeztah force-pushed the context_version branch from bb0170d to 1a52c97 Compare August 20, 2024 15:02

thaJeztah force-pushed the context_version branch from 1a52c97 to 6a1d6e6 Compare July 20, 2025 16:13

thaJeztah added this to the 29.0.0 milestone Jul 20, 2025

thaJeztah force-pushed the context_version branch 8 times, most recently from c26bc6c to e5aedee Compare July 26, 2025 08:27

corhere requested changes Jul 29, 2025

View reviewed changes

corhere reviewed Jul 29, 2025

View reviewed changes

corhere added this to 🔦 Maintainer spotlight Jul 29, 2025

github-project-automation Bot moved this to New in 🔦 Maintainer spotlight Jul 29, 2025

thaJeztah mentioned this pull request Jul 30, 2025

api: remove deprecated NoBaseImageSpecifier #50574

Merged

thaJeztah force-pushed the context_version branch from e5aedee to 53c0ddd Compare July 30, 2025 20:44

thompson-shaun requested a review from Copilot July 31, 2025 18:34

Copilot AI reviewed Jul 31, 2025

View reviewed changes

Comment thread daemon/server/router/volume/volume_routes_test.go Outdated

Comment thread daemon/server/router/build/build_routes.go

thompson-shaun moved this from New to Complete in 🔦 Maintainer spotlight Jul 31, 2025

thaJeztah mentioned this pull request Jul 31, 2025

api: remove unused DefaultVersion, MinSupportedAPIVersion consts #50587

Merged

thaJeztah mentioned this pull request Sep 9, 2025

Add go modules to version response #50934

Closed

thaJeztah force-pushed the context_version branch from 53c0ddd to 4056cd6 Compare September 9, 2025 12:19

vvoland modified the milestones: 29.0.0, 29.1.0 Nov 10, 2025

vvoland modified the milestones: 29.1.0, v-future Nov 26, 2025

Conversation

thaJeztah commented Sep 12, 2023

Uh oh!

Uh oh!

thaJeztah commented Oct 12, 2023

Uh oh!

thaJeztah commented Oct 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corhere left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

thompson-shaun commented Jul 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants