From 8d514f4e2e8da9d9a5727b6f05d6be68217fa173 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 15 Jul 2023 17:22:06 +0200 Subject: [PATCH 1/5] client: touch-up docs, and user doc-links Signed-off-by: Sebastiaan van Stijn --- client/client.go | 51 +++++++++++++++++++------------------ client/options.go | 64 +++++++++++++++++++++++++---------------------- 2 files changed, 61 insertions(+), 54 deletions(-) diff --git a/client/client.go b/client/client.go index fa98e76135..9b0ba64666 100644 --- a/client/client.go +++ b/client/client.go @@ -86,9 +86,6 @@ import ( // [Go stdlib]: https://github.com/golang/go/blob/6244b1946bc2101b01955468f1be502dbadd6807/src/net/http/transport.go#L558-L569 const DummyHost = "api.moby.localhost" -// ErrRedirect is the error returned by checkRedirect when the request is non-GET. -var ErrRedirect = errors.New("unexpected redirect in response") - // Client is the API client that performs all operations // against a docker server. type Client struct { @@ -111,7 +108,7 @@ type Client struct { // header variables. When set to an empty string, the User-Agent header // is removed, and no header is sent. userAgent *string - // custom http headers configured by users. + // custom HTTP headers configured by users. customHTTPHeaders map[string]string // manualOverride is set to true when the version was set by users. manualOverride bool @@ -126,20 +123,25 @@ type Client struct { negotiated bool } -// CheckRedirect specifies the policy for dealing with redirect responses: -// If the request is non-GET return ErrRedirect, otherwise use the last response. +// ErrRedirect is the error returned by checkRedirect when the request is non-GET. +var ErrRedirect = errors.New("unexpected redirect in response") + +// CheckRedirect specifies the policy for dealing with redirect responses. It +// can be set on [http.Client.CheckRedirect] to prevent HTTP redirects for +// non-GET requests. It returns an [ErrRedirect] for non-GET request, otherwise +// returns a [http.ErrUseLastResponse], which is special-cased by http.Client +// to use the last response. // -// Go 1.8 changes behavior for HTTP redirects (specifically 301, 307, and 308) -// in the client. The Docker client (and by extension docker API client) can be -// made to send a request like POST /containers//start where what would normally -// be in the name section of the URL is empty. This triggers an HTTP 301 from -// the daemon. +// Go 1.8 changed behavior for HTTP redirects (specifically 301, 307, and 308) +// in the client. The client (and by extension API client) can be made to send +// a request like "POST /containers//start" where what would normally be in the +// name section of the URL is empty. This triggers an HTTP 301 from the daemon. // -// In go 1.8 this 301 will be converted to a GET request, and ends up getting +// In go 1.8 this 301 is converted to a GET request, and ends up getting // a 404 from the daemon. This behavior change manifests in the client in that // before, the 301 was not followed and the client did not generate an error, -// but now results in a message like Error response from daemon: page not found. -func CheckRedirect(req *http.Request, via []*http.Request) error { +// but now results in a message like "Error response from daemon: page not found". +func CheckRedirect(_ *http.Request, via []*http.Request) error { if via[0].Method == http.MethodGet { return http.ErrUseLastResponse } @@ -150,11 +152,11 @@ func CheckRedirect(req *http.Request, via []*http.Request) error { // default API host and version. It also initializes the custom HTTP headers to // add to each request. // -// It takes an optional list of Opt functional arguments, which are applied in +// It takes an optional list of [Opt] functional arguments, which are applied in // the order they're provided, which allows modifying the defaults when creating // the client. For example, the following initializes a client that configures -// itself with values from environment variables (client.FromEnv), and has -// automatic API version negotiation enabled (client.WithAPIVersionNegotiation()). +// itself with values from environment variables ([FromEnv]), and has automatic +// API version negotiation enabled ([WithAPIVersionNegotiation]). // // cli, err := client.NewClientWithOpts( // client.FromEnv, @@ -221,7 +223,7 @@ func (cli *Client) Close() error { return nil } -// getAPIPath returns the versioned request path to call the api. +// 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 @@ -249,8 +251,8 @@ func (cli *Client) ClientVersion() string { // by the client, it uses the client's maximum version. // // If a manual override is in place, either through the "DOCKER_API_VERSION" -// (EnvOverrideAPIVersion) environment variable, or if the client is initialized -// with a fixed version (WithVersion(xx)), no negotiation is performed. +// ([EnvOverrideAPIVersion]) environment variable, or if the client is initialized +// with a fixed version ([WithVersion]), no negotiation is performed. // // If the API server's ping response does not contain an API version, or if the // client did not get a successful ping response, it assumes it is connected with @@ -270,8 +272,8 @@ func (cli *Client) NegotiateAPIVersion(ctx context.Context) { // version. // // If a manual override is in place, either through the "DOCKER_API_VERSION" -// (EnvOverrideAPIVersion) environment variable, or if the client is initialized -// with a fixed version (WithVersion(xx)), no negotiation is performed. +// ([EnvOverrideAPIVersion]) environment variable, or if the client is initialized +// with a fixed version ([WithVersion]), no negotiation is performed. // // If the API server's ping response does not contain an API version, we assume // we are connected with an old daemon without API version negotiation support, @@ -344,9 +346,10 @@ func ParseHostURL(host string) (*url.URL, error) { } // Dialer returns a dialer for a raw stream connection, with an HTTP/1.1 header, -// that can be used for proxying the daemon connection. +// that can be used for proxying the daemon connection. It is used by +// ["docker dial-stdio"]. // -// Used by `docker dial-stdio` (docker/cli#889). +// ["docker dial-stdio"]: https://github.com/docker/cli/pull/1014 func (cli *Client) Dialer() func(context.Context) (net.Conn, error) { return func(ctx context.Context) (net.Conn, error) { if transport, ok := cli.client.Transport.(*http.Transport); ok { diff --git a/client/options.go b/client/options.go index 10c9cbecfa..d258d0cfc0 100644 --- a/client/options.go +++ b/client/options.go @@ -13,23 +13,22 @@ import ( "github.com/pkg/errors" ) -// Opt is a configuration option to initialize a client +// Opt is a configuration option to initialize a [Client]. type Opt func(*Client) error -// FromEnv configures the client with values from environment variables. +// FromEnv configures the client with values from environment variables. It +// is the equivalent of using the [WithTLSClientConfigFromEnv], [WithHostFromEnv], +// and [WithVersionFromEnv] options. // // FromEnv uses the following environment variables: // -// DOCKER_HOST (EnvOverrideHost) to set the URL to the docker server. -// -// DOCKER_API_VERSION (EnvOverrideAPIVersion) to set the version of the API to -// use, leave empty for latest. -// -// DOCKER_CERT_PATH (EnvOverrideCertPath) to specify the directory from which to -// load the TLS certificates (ca.pem, cert.pem, key.pem). -// -// DOCKER_TLS_VERIFY (EnvTLSVerify) to enable or disable TLS verification (off by -// default). +// - DOCKER_HOST ([EnvOverrideHost]) to set the URL to the docker server. +// - DOCKER_API_VERSION ([EnvOverrideAPIVersion]) to set the version of the +// API to use, leave empty for latest. +// - DOCKER_CERT_PATH ([EnvOverrideCertPath]) to specify the directory from +// which to load the TLS certificates ("ca.pem", "cert.pem", "key.pem'). +// - DOCKER_TLS_VERIFY ([EnvTLSVerify]) to enable or disable TLS verification +// (off by default). func FromEnv(c *Client) error { ops := []Opt{ WithTLSClientConfigFromEnv(), @@ -45,7 +44,8 @@ func FromEnv(c *Client) error { } // WithDialContext applies the dialer to the client transport. This can be -// used to set the Timeout and KeepAlive settings of the client. +// used to set the Timeout and KeepAlive settings of the client. It returns +// an error if the client does not have a [http.Transport] configured. func WithDialContext(dialContext func(ctx context.Context, network, addr string) (net.Conn, error)) Opt { return func(c *Client) error { if transport, ok := c.client.Transport.(*http.Transport); ok { @@ -75,7 +75,7 @@ func WithHost(host string) Opt { } // WithHostFromEnv overrides the client host with the host specified in the -// DOCKER_HOST (EnvOverrideHost) environment variable. If DOCKER_HOST is not set, +// DOCKER_HOST ([EnvOverrideHost]) environment variable. If DOCKER_HOST is not set, // or set to an empty value, the host is not modified. func WithHostFromEnv() Opt { return func(c *Client) error { @@ -86,7 +86,7 @@ func WithHostFromEnv() Opt { } } -// WithHTTPClient overrides the client http client with the specified one +// WithHTTPClient overrides the client's HTTP client with the specified one. func WithHTTPClient(client *http.Client) Opt { return func(c *Client) error { if client != nil { @@ -96,7 +96,7 @@ func WithHTTPClient(client *http.Client) Opt { } } -// WithTimeout configures the time limit for requests made by the HTTP client +// WithTimeout configures the time limit for requests made by the HTTP client. func WithTimeout(timeout time.Duration) Opt { return func(c *Client) error { c.client.Timeout = timeout @@ -114,7 +114,9 @@ func WithUserAgent(ua string) Opt { } } -// WithHTTPHeaders overrides the client default http headers +// WithHTTPHeaders appends custom HTTP headers to the client's default headers. +// It does not allow for built-in headers (such as "User-Agent", if set) to +// be overridden. Also see [WithUserAgent]. func WithHTTPHeaders(headers map[string]string) Opt { return func(c *Client) error { c.customHTTPHeaders = headers @@ -122,7 +124,7 @@ func WithHTTPHeaders(headers map[string]string) Opt { } } -// WithScheme overrides the client scheme with the specified one +// WithScheme overrides the client scheme with the specified one. func WithScheme(scheme string) Opt { return func(c *Client) error { c.scheme = scheme @@ -130,7 +132,7 @@ func WithScheme(scheme string) Opt { } } -// WithTLSClientConfig applies a tls config to the client transport. +// WithTLSClientConfig applies a TLS config to the client transport. func WithTLSClientConfig(cacertPath, certPath, keyPath string) Opt { return func(c *Client) error { opts := tlsconfig.Options{ @@ -152,16 +154,16 @@ func WithTLSClientConfig(cacertPath, certPath, keyPath string) Opt { } // WithTLSClientConfigFromEnv configures the client's TLS settings with the -// settings in the DOCKER_CERT_PATH and DOCKER_TLS_VERIFY environment variables. -// If DOCKER_CERT_PATH is not set or empty, TLS configuration is not modified. +// settings in the DOCKER_CERT_PATH ([EnvOverrideCertPath]) and DOCKER_TLS_VERIFY +// ([EnvTLSVerify]) environment variables. If DOCKER_CERT_PATH is not set or empty, +// TLS configuration is not modified. // // WithTLSClientConfigFromEnv uses the following environment variables: // -// DOCKER_CERT_PATH (EnvOverrideCertPath) to specify the directory from which to -// load the TLS certificates (ca.pem, cert.pem, key.pem). -// -// DOCKER_TLS_VERIFY (EnvTLSVerify) to enable or disable TLS verification (off by -// default). +// - DOCKER_CERT_PATH ([EnvOverrideCertPath]) to specify the directory from +// which to load the TLS certificates ("ca.pem", "cert.pem", "key.pem"). +// - DOCKER_TLS_VERIFY ([EnvTLSVerify]) to enable or disable TLS verification +// (off by default). func WithTLSClientConfigFromEnv() Opt { return func(c *Client) error { dockerCertPath := os.Getenv(EnvOverrideCertPath) @@ -188,7 +190,8 @@ func WithTLSClientConfigFromEnv() Opt { } // WithVersion overrides the client version with the specified one. If an empty -// version is specified, the value will be ignored to allow version negotiation. +// version is provided, the value is ignored to allow version negotiation +// (see [WithAPIVersionNegotiation]). func WithVersion(version string) Opt { return func(c *Client) error { if version != "" { @@ -200,8 +203,9 @@ func WithVersion(version string) Opt { } // WithVersionFromEnv overrides the client version with the version specified in -// the DOCKER_API_VERSION environment variable. If DOCKER_API_VERSION is not set, -// the version is not modified. +// the DOCKER_API_VERSION ([EnvOverrideAPIVersion]) environment variable. +// If DOCKER_API_VERSION is not set, or set to an empty value, the version +// is not modified. func WithVersionFromEnv() Opt { return func(c *Client) error { return WithVersion(os.Getenv(EnvOverrideAPIVersion))(c) @@ -211,7 +215,7 @@ func WithVersionFromEnv() Opt { // WithAPIVersionNegotiation enables automatic API version negotiation for the client. // With this option enabled, the client automatically negotiates the API version // to use when making requests. API version negotiation is performed on the first -// request; subsequent requests will not re-negotiate. +// request; subsequent requests do not re-negotiate. func WithAPIVersionNegotiation() Opt { return func(c *Client) error { c.negotiateVersion = true From 6552441b462080bbf3fc0da87da6e4270dd9673d Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 15 Jul 2023 17:45:33 +0200 Subject: [PATCH 2/5] client: TestClientRedirect: cleanup - don't use un-keyed structs - use http consts where possible - use errors.As instead of manually checking the error-type Signed-off-by: Sebastiaan van Stijn --- client/client_test.go | 41 +++++++++++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/client/client_test.go b/client/client_test.go index 44586075d1..3bf38d576a 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -3,6 +3,7 @@ package client // import "github.com/docker/docker/client" import ( "bytes" "context" + "errors" "io" "net/http" "net/url" @@ -384,28 +385,43 @@ func TestClientRedirect(t *testing.T) { CheckRedirect: CheckRedirect, Transport: roundTripFunc(func(req *http.Request) (*http.Response, error) { if req.URL.String() == "/bla" { - return &http.Response{StatusCode: 404}, nil + return &http.Response{StatusCode: http.StatusNotFound}, nil } return &http.Response{ - StatusCode: 301, - Header: map[string][]string{"Location": {"/bla"}}, + StatusCode: http.StatusMovedPermanently, + Header: http.Header{"Location": {"/bla"}}, Body: bytesBufferClose{bytes.NewBuffer(nil)}, }, nil }), } - cases := []struct { + tests := []struct { httpMethod string expectedErr *url.Error statusCode int }{ - {http.MethodGet, nil, 301}, - {http.MethodPost, &url.Error{Op: "Post", URL: "/bla", Err: ErrRedirect}, 301}, - {http.MethodPut, &url.Error{Op: "Put", URL: "/bla", Err: ErrRedirect}, 301}, - {http.MethodDelete, &url.Error{Op: "Delete", URL: "/bla", Err: ErrRedirect}, 301}, + { + httpMethod: http.MethodGet, + statusCode: http.StatusMovedPermanently, + }, + { + httpMethod: http.MethodPost, + expectedErr: &url.Error{Op: "Post", URL: "/bla", Err: ErrRedirect}, + statusCode: http.StatusMovedPermanently, + }, + { + httpMethod: http.MethodPut, + expectedErr: &url.Error{Op: "Put", URL: "/bla", Err: ErrRedirect}, + statusCode: http.StatusMovedPermanently, + }, + { + httpMethod: http.MethodDelete, + expectedErr: &url.Error{Op: "Delete", URL: "/bla", Err: ErrRedirect}, + statusCode: http.StatusMovedPermanently, + }, } - for _, tc := range cases { + for _, tc := range tests { tc := tc t.Run(tc.httpMethod, func(t *testing.T) { req, err := http.NewRequest(tc.httpMethod, "/redirectme", nil) @@ -413,10 +429,11 @@ func TestClientRedirect(t *testing.T) { resp, err := client.Do(req) assert.Check(t, is.Equal(resp.StatusCode, tc.statusCode)) if tc.expectedErr == nil { - assert.NilError(t, err) + assert.Check(t, err) } else { - urlError, ok := err.(*url.Error) - assert.Assert(t, ok, "%T is not *url.Error", err) + assert.Check(t, is.ErrorType(err, &url.Error{})) + var urlError *url.Error + assert.Assert(t, errors.As(err, &urlError), "%T is not *url.Error", err) assert.Check(t, is.Equal(*urlError, *tc.expectedErr)) } }) From 9d1e6907511f717d8476cbe3a551131855dae639 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 15 Jul 2023 17:56:15 +0200 Subject: [PATCH 3/5] client: TestGetAPIPath: don't use un-keyed structs Signed-off-by: Sebastiaan van Stijn --- client/client_test.go | 67 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 55 insertions(+), 12 deletions(-) diff --git a/client/client_test.go b/client/client_test.go index 3bf38d576a..5d74b193ea 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -110,26 +110,69 @@ func TestNewClientWithOpsFromEnv(t *testing.T) { } func TestGetAPIPath(t *testing.T) { - testcases := []struct { + tests := []struct { version string path string query url.Values expected string }{ - {"", "/containers/json", nil, "/v" + api.DefaultVersion + "/containers/json"}, - {"", "/containers/json", url.Values{}, "/v" + api.DefaultVersion + "/containers/json"}, - {"", "/containers/json", url.Values{"s": []string{"c"}}, "/v" + api.DefaultVersion + "/containers/json?s=c"}, - {"1.22", "/containers/json", nil, "/v1.22/containers/json"}, - {"1.22", "/containers/json", url.Values{}, "/v1.22/containers/json"}, - {"1.22", "/containers/json", url.Values{"s": []string{"c"}}, "/v1.22/containers/json?s=c"}, - {"v1.22", "/containers/json", nil, "/v1.22/containers/json"}, - {"v1.22", "/containers/json", url.Values{}, "/v1.22/containers/json"}, - {"v1.22", "/containers/json", url.Values{"s": []string{"c"}}, "/v1.22/containers/json?s=c"}, - {"v1.22", "/networks/kiwl$%^", nil, "/v1.22/networks/kiwl$%25%5E"}, + { + path: "/containers/json", + expected: "/v" + api.DefaultVersion + "/containers/json", + }, + { + path: "/containers/json", + query: url.Values{}, + expected: "/v" + api.DefaultVersion + "/containers/json", + }, + { + path: "/containers/json", + query: url.Values{"s": []string{"c"}}, + expected: "/v" + api.DefaultVersion + "/containers/json?s=c", + }, + { + version: "1.22", + path: "/containers/json", + expected: "/v1.22/containers/json", + }, + { + version: "1.22", + path: "/containers/json", + query: url.Values{}, + expected: "/v1.22/containers/json", + }, + { + version: "1.22", + path: "/containers/json", + query: url.Values{"s": []string{"c"}}, + expected: "/v1.22/containers/json?s=c", + }, + { + version: "v1.22", + path: "/containers/json", + expected: "/v1.22/containers/json", + }, + { + version: "v1.22", + path: "/containers/json", + query: url.Values{}, + expected: "/v1.22/containers/json", + }, + { + version: "v1.22", + path: "/containers/json", + query: url.Values{"s": []string{"c"}}, + expected: "/v1.22/containers/json?s=c", + }, + { + version: "v1.22", + path: "/networks/kiwl$%^", + expected: "/v1.22/networks/kiwl$%25%5E", + }, } ctx := context.TODO() - for _, tc := range testcases { + for _, tc := range tests { client, err := NewClientWithOpts( WithVersion(tc.version), WithHost("tcp://localhost:2375"), From 6846c6325233cbab8ea6a798bd252c67ff7e65d2 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 15 Jul 2023 18:03:34 +0200 Subject: [PATCH 4/5] client: WithTLSClientConfig, WithTLSClientConfigFromEnv: inline vars Remove intermediate variables. Signed-off-by: Sebastiaan van Stijn --- client/options.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/client/options.go b/client/options.go index d258d0cfc0..b8bc4029bc 100644 --- a/client/options.go +++ b/client/options.go @@ -135,13 +135,12 @@ func WithScheme(scheme string) Opt { // WithTLSClientConfig applies a TLS config to the client transport. func WithTLSClientConfig(cacertPath, certPath, keyPath string) Opt { return func(c *Client) error { - opts := tlsconfig.Options{ + config, err := tlsconfig.Client(tlsconfig.Options{ CAFile: cacertPath, CertFile: certPath, KeyFile: keyPath, ExclusiveRootPools: true, - } - config, err := tlsconfig.Client(opts) + }) if err != nil { return errors.Wrap(err, "failed to create tls config") } @@ -170,13 +169,12 @@ func WithTLSClientConfigFromEnv() Opt { if dockerCertPath == "" { return nil } - options := tlsconfig.Options{ + tlsc, err := tlsconfig.Client(tlsconfig.Options{ CAFile: filepath.Join(dockerCertPath, "ca.pem"), CertFile: filepath.Join(dockerCertPath, "cert.pem"), KeyFile: filepath.Join(dockerCertPath, "key.pem"), InsecureSkipVerify: os.Getenv(EnvTLSVerify) == "", - } - tlsc, err := tlsconfig.Client(options) + }) if err != nil { return err } From 077049d0b9706622babb27e8f2ecbdfbd769f054 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 15 Jul 2023 18:06:53 +0200 Subject: [PATCH 5/5] client: WithTLSClientConfig: return early if no transport is set tlsconfig.Client() does various things, including reading certs and checking them. So we may as well return early if we're not gonna be able to use the config. Signed-off-by: Sebastiaan van Stijn --- client/options.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/client/options.go b/client/options.go index b8bc4029bc..5f9c4099ea 100644 --- a/client/options.go +++ b/client/options.go @@ -135,6 +135,10 @@ func WithScheme(scheme string) Opt { // WithTLSClientConfig applies a TLS config to the client transport. func WithTLSClientConfig(cacertPath, certPath, keyPath string) Opt { return func(c *Client) error { + transport, ok := c.client.Transport.(*http.Transport) + if !ok { + return errors.Errorf("cannot apply tls config to transport: %T", c.client.Transport) + } config, err := tlsconfig.Client(tlsconfig.Options{ CAFile: cacertPath, CertFile: certPath, @@ -144,11 +148,8 @@ func WithTLSClientConfig(cacertPath, certPath, keyPath string) Opt { if err != nil { return errors.Wrap(err, "failed to create tls config") } - if transport, ok := c.client.Transport.(*http.Transport); ok { - transport.TLSClientConfig = config - return nil - } - return errors.Errorf("cannot apply tls config to transport: %T", c.client.Transport) + transport.TLSClientConfig = config + return nil } }