Merge pull request #45978 from thaJeztah/client_docs_touch_up

client: touch-up docs, cleanup some tests, and minor refactoring

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 {

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"
@@ -109,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"),
@@ -384,28 +428,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 +472,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))
 			}
 		})

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,51 +132,50 @@ 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{
+		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,
 			KeyFile:            keyPath,
 			ExclusiveRootPools: true,
-		}
-		config, err := tlsconfig.Client(opts)
+		})
 		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
 	}
 }
 
 // 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)
 		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
 		}
@@ -188,7 +189,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 +202,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 +214,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