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

feat: negotiate docker client version #2258

Closed
wants to merge 1 commit into from
Closed
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
18 changes: 10 additions & 8 deletions docker/daemon/client.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package daemon

import (
"context"
"net/http"
"path/filepath"

Expand All @@ -9,21 +10,15 @@ import (
"github.com/docker/go-connections/tlsconfig"
)

const (
// The default API version to be used in case none is explicitly specified
defaultAPIVersion = "1.22"
)

// NewDockerClient initializes a new API client based on the passed SystemContext.
func newDockerClient(sys *types.SystemContext) (*dockerclient.Client, error) {
func newDockerClient(ctx context.Context, sys *types.SystemContext) (*dockerclient.Client, error) {
host := dockerclient.DefaultDockerHost
if sys != nil && sys.DockerDaemonHost != "" {
host = sys.DockerDaemonHost
}

opts := []dockerclient.Opt{
dockerclient.WithHost(host),
dockerclient.WithVersion(defaultAPIVersion),
}

// We conditionalize building the TLS configuration only to TLS sockets:
Expand Down Expand Up @@ -63,7 +58,14 @@ func newDockerClient(sys *types.SystemContext) (*dockerclient.Client, error) {
opts = append(opts, dockerclient.WithHTTPClient(hc))
}

return dockerclient.NewClientWithOpts(opts...)
cli, err := dockerclient.NewClientWithOpts(opts...)
if err != nil {
return nil, err
}

cli.NegotiateAPIVersion(ctx)

return cli, nil
}

func tlsConfig(sys *types.SystemContext) (*http.Client, error) {
Expand Down
31 changes: 25 additions & 6 deletions docker/daemon/client_test.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
package daemon

import (
"context"
"net/http"
"os"
"path/filepath"
"strconv"
"strings"
"testing"

"github.com/containers/image/v5/types"
Expand All @@ -12,13 +15,23 @@ import (
)

func TestDockerClientFromNilSystemContext(t *testing.T) {
client, err := newDockerClient(nil)
client, err := newDockerClient(context.Background(), nil)

assert.Nil(t, err, "There should be no error creating the Docker client")
assert.NotNil(t, client, "A Docker client reference should have been returned")

assert.Equal(t, dockerclient.DefaultDockerHost, client.DaemonHost(), "The default docker host should have been used")
assert.Equal(t, defaultAPIVersion, client.ClientVersion(), "The default api version should have been used")

clientVersionNumbers := strings.Split(client.ClientVersion(), ".")

major, err := strconv.Atoi(clientVersionNumbers[0])
assert.NoError(t, err, "The client major version should be a number")

minor, err := strconv.Atoi(clientVersionNumbers[1])
assert.NoError(t, err, "The client minor version should be a number")

// The client defaults to 1.24 if negotiation fails.
assert.True(t, major == 1 && minor > 24, client.ClientVersion(), "Should have successfully negotiated a client version")

assert.NoError(t, client.Close())
}
Expand All @@ -33,13 +46,19 @@ func TestDockerClientFromCertContext(t *testing.T) {
DockerDaemonInsecureSkipTLSVerify: true,
}

client, err := newDockerClient(systemCtx)
client, err := newDockerClient(context.Background(), systemCtx)

assert.Nil(t, err, "There should be no error creating the Docker client")
assert.NotNil(t, client, "A Docker client reference should have been returned")

assert.Equal(t, host, client.DaemonHost())
assert.Equal(t, "1.22", client.ClientVersion())

clientVersionNumbers := strings.Split(client.ClientVersion(), ".")

major, err := strconv.Atoi(clientVersionNumbers[0])
assert.NoError(t, err, "The client major version should be a number")

assert.Equal(t, 1, major, "The client major version should be 1")

assert.NoError(t, client.Close())
}
Expand Down Expand Up @@ -88,11 +107,11 @@ func TestSkipTLSVerifyOnly(t *testing.T) {

func TestSpecifyPlainHTTPViaHostScheme(t *testing.T) {
host := "http://127.0.0.1:2376"
ctx := &types.SystemContext{
systemCtx := &types.SystemContext{
DockerDaemonHost: host,
}

client, err := newDockerClient(ctx)
client, err := newDockerClient(context.Background(), systemCtx)

assert.Nil(t, err, "There should be no error creating the Docker client")
assert.NotNil(t, client, "A Docker client reference should have been returned")
Expand Down
2 changes: 1 addition & 1 deletion docker/daemon/daemon_dest.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func newImageDestination(ctx context.Context, sys *types.SystemContext, ref daem
mustMatchRuntimeOS = false
}

c, err := newDockerClient(sys)
c, err := newDockerClient(ctx, sys)
if err != nil {
return nil, fmt.Errorf("initializing docker engine client: %w", err)
}
Expand Down
2 changes: 1 addition & 1 deletion docker/daemon/daemon_src.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ type daemonImageSource struct {
// is the config, and that the following len(RootFS) files are the layers, but that feels
// way too brittle.)
func newImageSource(ctx context.Context, sys *types.SystemContext, ref daemonReference) (private.ImageSource, error) {
c, err := newDockerClient(sys)
c, err := newDockerClient(ctx, sys)
if err != nil {
return nil, fmt.Errorf("initializing docker engine client: %w", err)
}
Expand Down