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

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

Merged
merged 1 commit into from
Sep 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion client/build_prune.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (

// BuildCachePrune requests the daemon to delete unused cache data
func (cli *Client) BuildCachePrune(ctx context.Context, opts types.BuildCachePruneOptions) (*types.BuildCachePruneReport, error) {
if err := cli.NewVersionError("1.31", "build prune"); err != nil {
if err := cli.NewVersionError(ctx, "1.31", "build prune"); err != nil {
return nil, err
}

Expand Down
14 changes: 11 additions & 3 deletions client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -254,13 +254,21 @@ func (cli *Client) Close() error {
return nil
}

// checkVersion manually triggers API version negotiation (if configured).
// This allows for version-dependent code to use the same version as will
// be negotiated when making the actual requests, and for which cases
// we cannot do the negotiation lazily.
func (cli *Client) checkVersion(ctx context.Context) {
if cli.negotiateVersion && !cli.negotiated {
cli.NegotiateAPIVersion(ctx)
}
}

// 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(ctx context.Context, p string, query url.Values) string {
var apiPath string
if cli.negotiateVersion && !cli.negotiated {
cli.NegotiateAPIVersion(ctx)
}
cli.checkVersion(ctx)
if cli.version != "" {
v := strings.TrimPrefix(cli.version, "v")
apiPath = path.Join(cli.basePath, "/v"+v, p)
Expand Down
2 changes: 1 addition & 1 deletion client/config_create.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
// ConfigCreate creates a new config.
func (cli *Client) ConfigCreate(ctx context.Context, config swarm.ConfigSpec) (types.ConfigCreateResponse, error) {
var response types.ConfigCreateResponse
if err := cli.NewVersionError("1.30", "config create"); err != nil {
if err := cli.NewVersionError(ctx, "1.30", "config create"); err != nil {
return response, err
}
resp, err := cli.post(ctx, "/configs/create", nil, config, nil)
Expand Down
2 changes: 1 addition & 1 deletion client/config_inspect.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ func (cli *Client) ConfigInspectWithRaw(ctx context.Context, id string) (swarm.C
if id == "" {
return swarm.Config{}, nil, objectNotFoundError{object: "config", id: id}
}
if err := cli.NewVersionError("1.30", "config inspect"); err != nil {
if err := cli.NewVersionError(ctx, "1.30", "config inspect"); err != nil {
return swarm.Config{}, nil, err
}
resp, err := cli.get(ctx, "/configs/"+id, nil, nil)
Expand Down
2 changes: 1 addition & 1 deletion client/config_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (

// ConfigList returns the list of configs.
func (cli *Client) ConfigList(ctx context.Context, options types.ConfigListOptions) ([]swarm.Config, error) {
if err := cli.NewVersionError("1.30", "config list"); err != nil {
if err := cli.NewVersionError(ctx, "1.30", "config list"); err != nil {
return nil, err
}
query := url.Values{}
Expand Down
2 changes: 1 addition & 1 deletion client/config_remove.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import "context"

// ConfigRemove removes a config.
func (cli *Client) ConfigRemove(ctx context.Context, id string) error {
if err := cli.NewVersionError("1.30", "config remove"); err != nil {
if err := cli.NewVersionError(ctx, "1.30", "config remove"); err != nil {
return err
}
resp, err := cli.delete(ctx, "/configs/"+id, nil, nil)
Expand Down
2 changes: 1 addition & 1 deletion client/config_update.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (

// ConfigUpdate attempts to update a config
func (cli *Client) ConfigUpdate(ctx context.Context, id string, version swarm.Version, config swarm.ConfigSpec) error {
if err := cli.NewVersionError("1.30", "config update"); err != nil {
if err := cli.NewVersionError(ctx, "1.30", "config update"); err != nil {
return err
}
query := url.Values{}
Expand Down
13 changes: 10 additions & 3 deletions client/container_create.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,20 @@ type configWrapper struct {
func (cli *Client) ContainerCreate(ctx context.Context, config *container.Config, hostConfig *container.HostConfig, networkingConfig *network.NetworkingConfig, platform *ocispec.Platform, containerName string) (container.CreateResponse, error) {
var response container.CreateResponse

if err := cli.NewVersionError("1.25", "stop timeout"); config != nil && config.StopTimeout != nil && err != nil {
// Make sure we negotiated (if the client is configured to do so),
// as code below contains API-version specific handling of options.
//
// Normally, version-negotiation (if enabled) would not happen until
// the API request is made.
cli.checkVersion(ctx)
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 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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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 😄

Copy link
Member

@akerouanton akerouanton Sep 20, 2023

Choose a reason for hiding this comment

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

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.


if err := cli.NewVersionError(ctx, "1.25", "stop timeout"); config != nil && config.StopTimeout != nil && err != nil {
return response, err
}
if err := cli.NewVersionError("1.41", "specify container image platform"); platform != nil && err != nil {
if err := cli.NewVersionError(ctx, "1.41", "specify container image platform"); platform != nil && err != nil {
return response, err
}
if err := cli.NewVersionError("1.44", "specify health-check start interval"); config != nil && config.Healthcheck != nil && config.Healthcheck.StartInterval != 0 && err != nil {
if err := cli.NewVersionError(ctx, "1.44", "specify health-check start interval"); config != nil && config.Healthcheck != nil && config.Healthcheck.StartInterval != 0 && err != nil {
return response, err
}

Expand Down
9 changes: 8 additions & 1 deletion client/container_exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,14 @@ import (
func (cli *Client) ContainerExecCreate(ctx context.Context, container string, config types.ExecConfig) (types.IDResponse, error) {
var response types.IDResponse

if err := cli.NewVersionError("1.25", "env"); len(config.Env) != 0 && err != nil {
// Make sure we negotiated (if the client is configured to do so),
// as code below contains API-version specific handling of options.
//
// Normally, version-negotiation (if enabled) would not happen until
// the API request is made.
cli.checkVersion(ctx)

if err := cli.NewVersionError(ctx, "1.25", "env"); len(config.Env) != 0 && err != nil {
return response, err
}
if versions.LessThan(cli.ClientVersion(), "1.42") {
Expand Down
2 changes: 1 addition & 1 deletion client/container_prune.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
func (cli *Client) ContainersPrune(ctx context.Context, pruneFilters filters.Args) (types.ContainersPruneReport, error) {
var report types.ContainersPruneReport

if err := cli.NewVersionError("1.25", "container prune"); err != nil {
if err := cli.NewVersionError(ctx, "1.25", "container prune"); err != nil {
return report, err
}

Expand Down
12 changes: 10 additions & 2 deletions client/container_restart.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,16 @@ func (cli *Client) ContainerRestart(ctx context.Context, containerID string, opt
if options.Timeout != nil {
query.Set("t", strconv.Itoa(*options.Timeout))
}
if options.Signal != "" && versions.GreaterThanOrEqualTo(cli.version, "1.42") {
query.Set("signal", options.Signal)
if options.Signal != "" {
// Make sure we negotiated (if the client is configured to do so),
// as code below contains API-version specific handling of options.
//
// Normally, version-negotiation (if enabled) would not happen until
// the API request is made.
cli.checkVersion(ctx)
if versions.GreaterThanOrEqualTo(cli.version, "1.42") {
query.Set("signal", options.Signal)
}
}
resp, err := cli.post(ctx, "/containers/"+containerID+"/restart", query, nil, nil)
ensureReaderClosed(resp)
Expand Down
12 changes: 10 additions & 2 deletions client/container_stop.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,16 @@ func (cli *Client) ContainerStop(ctx context.Context, containerID string, option
if options.Timeout != nil {
query.Set("t", strconv.Itoa(*options.Timeout))
}
if options.Signal != "" && versions.GreaterThanOrEqualTo(cli.version, "1.42") {
query.Set("signal", options.Signal)
if options.Signal != "" {
// Make sure we negotiated (if the client is configured to do so),
// as code below contains API-version specific handling of options.
//
// Normally, version-negotiation (if enabled) would not happen until
// the API request is made.
cli.checkVersion(ctx)
if versions.GreaterThanOrEqualTo(cli.version, "1.42") {
query.Set("signal", options.Signal)
}
}
resp, err := cli.post(ctx, "/containers/"+containerID+"/stop", query, nil, nil)
ensureReaderClosed(resp)
Expand Down
6 changes: 6 additions & 0 deletions client/container_wait.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@ const containerWaitErrorMsgLimit = 2 * 1024 /* Max: 2KiB */
// synchronize ContainerWait with other calls, such as specifying a
// "next-exit" condition before issuing a ContainerStart request.
func (cli *Client) ContainerWait(ctx context.Context, containerID string, condition container.WaitCondition) (<-chan container.WaitResponse, <-chan error) {
// Make sure we negotiated (if the client is configured to do so),
// as code below contains API-version specific handling of options.
//
// Normally, version-negotiation (if enabled) would not happen until
// the API request is made.
cli.checkVersion(ctx)
if versions.LessThan(cli.ClientVersion(), "1.30") {
return cli.legacyContainerWait(ctx, containerID)
}
Expand Down
2 changes: 1 addition & 1 deletion client/distribution_inspect.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ func (cli *Client) DistributionInspect(ctx context.Context, image, encodedRegist
return distributionInspect, objectNotFoundError{object: "distribution", id: image}
}

if err := cli.NewVersionError("1.30", "distribution inspect"); err != nil {
if err := cli.NewVersionError(ctx, "1.30", "distribution inspect"); err != nil {
return distributionInspect, err
}

Expand Down
16 changes: 13 additions & 3 deletions client/errors.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package client // import "github.com/docker/docker/client"

import (
"context"
"fmt"

"github.com/docker/docker/api/types/versions"
Expand Down Expand Up @@ -48,9 +49,18 @@ func (e objectNotFoundError) Error() string {
return fmt.Sprintf("Error: No such %s: %s", e.object, e.id)
}

// NewVersionError returns an error if the APIVersion required
// if less than the current supported version
func (cli *Client) NewVersionError(APIrequired, feature string) error {
// NewVersionError returns an error if the APIVersion required is less than the
// current supported version.
//
// It performs API-version negotiation if the Client is configured with this
// option, otherwise it assumes the latest API version is used.
func (cli *Client) NewVersionError(ctx context.Context, APIrequired, feature string) error {
// Make sure we negotiated (if the client is configured to do so),
// as code below contains API-version specific handling of options.
//
// Normally, version-negotiation (if enabled) would not happen until
// the API request is made.
cli.checkVersion(ctx)
if cli.version != "" && versions.LessThan(cli.version, APIrequired) {
return fmt.Errorf("%q requires API version %s, but the Docker daemon API version is %s", feature, APIrequired, cli.version)
}
Expand Down
8 changes: 4 additions & 4 deletions client/image_build.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import (
// The Body in the response implements an io.ReadCloser and it's up to the caller to
// close it.
func (cli *Client) ImageBuild(ctx context.Context, buildContext io.Reader, options types.ImageBuildOptions) (types.ImageBuildResponse, error) {
query, err := cli.imageBuildOptionsToQuery(options)
query, err := cli.imageBuildOptionsToQuery(ctx, options)
if err != nil {
return types.ImageBuildResponse{}, err
}
Expand All @@ -43,7 +43,7 @@ func (cli *Client) ImageBuild(ctx context.Context, buildContext io.Reader, optio
}, nil
}

func (cli *Client) imageBuildOptionsToQuery(options types.ImageBuildOptions) (url.Values, error) {
func (cli *Client) imageBuildOptionsToQuery(ctx context.Context, options types.ImageBuildOptions) (url.Values, error) {
query := url.Values{
"t": options.Tags,
"securityopt": options.SecurityOpt,
Expand Down Expand Up @@ -73,7 +73,7 @@ func (cli *Client) imageBuildOptionsToQuery(options types.ImageBuildOptions) (ur
}

if options.Squash {
if err := cli.NewVersionError("1.25", "squash"); err != nil {
if err := cli.NewVersionError(ctx, "1.25", "squash"); err != nil {
return query, err
}
query.Set("squash", "1")
Expand Down Expand Up @@ -123,7 +123,7 @@ func (cli *Client) imageBuildOptionsToQuery(options types.ImageBuildOptions) (ur
query.Set("session", options.SessionID)
}
if options.Platform != "" {
if err := cli.NewVersionError("1.32", "platform"); err != nil {
if err := cli.NewVersionError(ctx, "1.32", "platform"); err != nil {
return query, err
}
query.Set("platform", strings.ToLower(options.Platform))
Expand Down
7 changes: 7 additions & 0 deletions client/image_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@ import (

// ImageList returns a list of images in the docker host.
func (cli *Client) ImageList(ctx context.Context, options types.ImageListOptions) ([]types.ImageSummary, error) {
// Make sure we negotiated (if the client is configured to do so),
// as code below contains API-version specific handling of options.
//
// Normally, version-negotiation (if enabled) would not happen until
// the API request is made.
cli.checkVersion(ctx)

var images []types.ImageSummary
query := url.Values{}

Expand Down
2 changes: 1 addition & 1 deletion client/image_prune.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
func (cli *Client) ImagesPrune(ctx context.Context, pruneFilters filters.Args) (types.ImagesPruneReport, error) {
var report types.ImagesPruneReport

if err := cli.NewVersionError("1.25", "image prune"); err != nil {
if err := cli.NewVersionError(ctx, "1.25", "image prune"); err != nil {
return report, err
}

Expand Down
7 changes: 7 additions & 0 deletions client/network_create.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@ import (

// NetworkCreate creates a new network in the docker host.
func (cli *Client) NetworkCreate(ctx context.Context, name string, options types.NetworkCreate) (types.NetworkCreateResponse, error) {
// Make sure we negotiated (if the client is configured to do so),
// as code below contains API-version specific handling of options.
//
// Normally, version-negotiation (if enabled) would not happen until
// the API request is made.
cli.checkVersion(ctx)

networkCreateRequest := types.NetworkCreateRequest{
NetworkCreate: options,
Name: name,
Expand Down
2 changes: 1 addition & 1 deletion client/network_prune.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
func (cli *Client) NetworksPrune(ctx context.Context, pruneFilters filters.Args) (types.NetworksPruneReport, error) {
var report types.NetworksPruneReport

if err := cli.NewVersionError("1.25", "network prune"); err != nil {
if err := cli.NewVersionError(ctx, "1.25", "network prune"); err != nil {
return report, err
}

Expand Down
2 changes: 1 addition & 1 deletion client/plugin_upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (

// PluginUpgrade upgrades a plugin
func (cli *Client) PluginUpgrade(ctx context.Context, name string, options types.PluginInstallOptions) (rc io.ReadCloser, err error) {
if err := cli.NewVersionError("1.26", "plugin upgrade"); err != nil {
if err := cli.NewVersionError(ctx, "1.26", "plugin upgrade"); err != nil {
return nil, err
}
query := url.Values{}
Expand Down
2 changes: 1 addition & 1 deletion client/secret_create.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
// SecretCreate creates a new secret.
func (cli *Client) SecretCreate(ctx context.Context, secret swarm.SecretSpec) (types.SecretCreateResponse, error) {
var response types.SecretCreateResponse
if err := cli.NewVersionError("1.25", "secret create"); err != nil {
if err := cli.NewVersionError(ctx, "1.25", "secret create"); err != nil {
return response, err
}
resp, err := cli.post(ctx, "/secrets/create", nil, secret, nil)
Expand Down
2 changes: 1 addition & 1 deletion client/secret_inspect.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (

// SecretInspectWithRaw returns the secret information with raw data
func (cli *Client) SecretInspectWithRaw(ctx context.Context, id string) (swarm.Secret, []byte, error) {
if err := cli.NewVersionError("1.25", "secret inspect"); err != nil {
if err := cli.NewVersionError(ctx, "1.25", "secret inspect"); err != nil {
return swarm.Secret{}, nil, err
}
if id == "" {
Expand Down
2 changes: 1 addition & 1 deletion client/secret_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (

// SecretList returns the list of secrets.
func (cli *Client) SecretList(ctx context.Context, options types.SecretListOptions) ([]swarm.Secret, error) {
if err := cli.NewVersionError("1.25", "secret list"); err != nil {
if err := cli.NewVersionError(ctx, "1.25", "secret list"); err != nil {
return nil, err
}
query := url.Values{}
Expand Down
2 changes: 1 addition & 1 deletion client/secret_remove.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import "context"

// SecretRemove removes a secret.
func (cli *Client) SecretRemove(ctx context.Context, id string) error {
if err := cli.NewVersionError("1.25", "secret remove"); err != nil {
if err := cli.NewVersionError(ctx, "1.25", "secret remove"); err != nil {
return err
}
resp, err := cli.delete(ctx, "/secrets/"+id, nil, nil)
Expand Down
2 changes: 1 addition & 1 deletion client/secret_update.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (

// SecretUpdate attempts to update a secret.
func (cli *Client) SecretUpdate(ctx context.Context, id string, version swarm.Version, secret swarm.SecretSpec) error {
if err := cli.NewVersionError("1.25", "secret update"); err != nil {
if err := cli.NewVersionError(ctx, "1.25", "secret update"); err != nil {
return err
}
query := url.Values{}
Expand Down
7 changes: 7 additions & 0 deletions client/service_create.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@ import (
func (cli *Client) ServiceCreate(ctx context.Context, service swarm.ServiceSpec, options types.ServiceCreateOptions) (types.ServiceCreateResponse, error) {
var response types.ServiceCreateResponse

// Make sure we negotiated (if the client is configured to do so),
// as code below contains API-version specific handling of options.
//
// Normally, version-negotiation (if enabled) would not happen until
// the API request is made.
cli.checkVersion(ctx)

// Make sure containerSpec is not nil when no runtime is set or the runtime is set to container
if service.TaskTemplate.ContainerSpec == nil && (service.TaskTemplate.Runtime == "" || service.TaskTemplate.Runtime == swarm.RuntimeContainer) {
service.TaskTemplate.ContainerSpec = &swarm.ContainerSpec{}
Expand Down
7 changes: 7 additions & 0 deletions client/service_update.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@ import (
// It should be the value as set *before* the update. You can find this value in the Meta field
// of swarm.Service, which can be found using ServiceInspectWithRaw.
func (cli *Client) ServiceUpdate(ctx context.Context, serviceID string, version swarm.Version, service swarm.ServiceSpec, options types.ServiceUpdateOptions) (types.ServiceUpdateResponse, error) {
// Make sure we negotiated (if the client is configured to do so),
// as code below contains API-version specific handling of options.
//
// Normally, version-negotiation (if enabled) would not happen until
// the API request is made.
cli.checkVersion(ctx)

var (
query = url.Values{}
response = types.ServiceUpdateResponse{}
Expand Down
2 changes: 1 addition & 1 deletion client/volume_prune.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
func (cli *Client) VolumesPrune(ctx context.Context, pruneFilters filters.Args) (types.VolumesPruneReport, error) {
var report types.VolumesPruneReport

if err := cli.NewVersionError("1.25", "volume prune"); err != nil {
if err := cli.NewVersionError(ctx, "1.25", "volume prune"); err != nil {
return report, err
}

Expand Down
Loading