Merge pull request #6428 from thaJeztah/28.x_backport_avoid_client_types_in_opts

[28.x backport] don't wrap client options

Author: austinvazquez

cli/command/cli.go

@@ -324,7 +324,14 @@ func newAPIClientFromEndpoint(ep docker.Endpoint, configFile *configfile.ConfigF
 	if len(configFile.HTTPHeaders) > 0 {
 		opts = append(opts, client.WithHTTPHeaders(configFile.HTTPHeaders))
 	}
-	opts = append(opts, withCustomHeadersFromEnv(), client.WithUserAgent(UserAgent()))
+	withCustomHeaders, err := withCustomHeadersFromEnv()
+	if err != nil {
+		return nil, err
+	}
+	if withCustomHeaders != nil {
+		opts = append(opts, withCustomHeaders)
+	}
+	opts = append(opts, client.WithUserAgent(UserAgent()))
 	return client.NewClientWithOpts(opts...)
 }
 

cli/command/cli_options.go

@@ -180,61 +180,59 @@ const envOverrideHTTPHeaders = "DOCKER_CUSTOM_HEADERS"
 // override headers with the same name).
 //
 // TODO(thaJeztah): this is a client Option, and should be moved to the client. It is non-exported for that reason.
-func withCustomHeadersFromEnv() client.Opt {
-	return func(apiClient *client.Client) error {
-		value := os.Getenv(envOverrideHTTPHeaders)
-		if value == "" {
-			return nil
-		}
-		csvReader := csv.NewReader(strings.NewReader(value))
-		fields, err := csvReader.Read()
-		if err != nil {
-			return invalidParameter(errors.Errorf(
-				"failed to parse custom headers from %s environment variable: value must be formatted as comma-separated key=value pairs",
-				envOverrideHTTPHeaders,
-			))
-		}
-		if len(fields) == 0 {
-			return nil
-		}
-
-		env := map[string]string{}
-		for _, kv := range fields {
-			k, v, hasValue := strings.Cut(kv, "=")
-
-			// Only strip whitespace in keys; preserve whitespace in values.
-			k = strings.TrimSpace(k)
+func withCustomHeadersFromEnv() (client.Opt, error) {
+	value := os.Getenv(envOverrideHTTPHeaders)
+	if value == "" {
+		return nil, nil
+	}
+	csvReader := csv.NewReader(strings.NewReader(value))
+	fields, err := csvReader.Read()
+	if err != nil {
+		return nil, invalidParameter(errors.Errorf(
+			"failed to parse custom headers from %s environment variable: value must be formatted as comma-separated key=value pairs",
+			envOverrideHTTPHeaders,
+		))
+	}
+	if len(fields) == 0 {
+		return nil, nil
+	}
 
-			if k == "" {
-				return invalidParameter(errors.Errorf(
-					`failed to set custom headers from %s environment variable: value contains a key=value pair with an empty key: '%s'`,
-					envOverrideHTTPHeaders, kv,
-				))
-			}
+	env := map[string]string{}
+	for _, kv := range fields {
+		k, v, hasValue := strings.Cut(kv, "=")
 
-			// We don't currently allow empty key=value pairs, and produce an error.
-			// This is something we could allow in future (e.g. to read value
-			// from an environment variable with the same name). In the meantime,
-			// produce an error to prevent users from depending on this.
-			if !hasValue {
-				return invalidParameter(errors.Errorf(
-					`failed to set custom headers from %s environment variable: missing "=" in key=value pair: '%s'`,
-					envOverrideHTTPHeaders, kv,
-				))
-			}
+		// Only strip whitespace in keys; preserve whitespace in values.
+		k = strings.TrimSpace(k)
 
-			env[http.CanonicalHeaderKey(k)] = v
+		if k == "" {
+			return nil, invalidParameter(errors.Errorf(
+				`failed to set custom headers from %s environment variable: value contains a key=value pair with an empty key: '%s'`,
+				envOverrideHTTPHeaders, kv,
+			))
 		}
 
-		if len(env) == 0 {
-			// We should probably not hit this case, as we don't skip values
-			// (only return errors), but we don't want to discard existing
-			// headers with an empty set.
-			return nil
+		// We don't currently allow empty key=value pairs, and produce an error.
+		// This is something we could allow in future (e.g. to read value
+		// from an environment variable with the same name). In the meantime,
+		// produce an error to prevent users from depending on this.
+		if !hasValue {
+			return nil, invalidParameter(errors.Errorf(
+				`failed to set custom headers from %s environment variable: missing "=" in key=value pair: '%s'`,
+				envOverrideHTTPHeaders, kv,
+			))
 		}
 
-		// TODO(thaJeztah): add a client.WithExtraHTTPHeaders() function to allow these headers to be _added_ to existing ones, instead of _replacing_
-		//  see https://github.com/docker/cli/pull/5098#issuecomment-2147403871  (when updating, also update the WARNING in the function and env-var GoDoc)
-		return client.WithHTTPHeaders(env)(apiClient)
+		env[http.CanonicalHeaderKey(k)] = v
+	}
+
+	if len(env) == 0 {
+		// We should probably not hit this case, as we don't skip values
+		// (only return errors), but we don't want to discard existing
+		// headers with an empty set.
+		return nil, nil
 	}
+
+	// TODO(thaJeztah): add a client.WithExtraHTTPHeaders() function to allow these headers to be _added_ to existing ones, instead of _replacing_
+	//  see https://github.com/docker/cli/pull/5098#issuecomment-2147403871  (when updating, also update the WARNING in the function and env-var GoDoc)
+	return client.WithHTTPHeaders(env), nil
 }

cli/context/docker/load.go

@@ -101,7 +101,22 @@ func (ep *Endpoint) ClientOpts() ([]client.Opt, error) {
 				if err != nil {
 					return nil, err
 				}
-				result = append(result, withHTTPClient(tlsConfig))
+
+				// If there's no tlsConfig available, we use the default HTTPClient.
+				if tlsConfig != nil {
+					result = append(result,
+						client.WithHTTPClient(&http.Client{
+							Transport: &http.Transport{
+								TLSClientConfig: tlsConfig,
+								DialContext: (&net.Dialer{
+									KeepAlive: 30 * time.Second,
+									Timeout:   30 * time.Second,
+								}).DialContext,
+							},
+							CheckRedirect: client.CheckRedirect,
+						}),
+					)
+				}
 			}
 			result = append(result, client.WithHost(ep.Host))
 		} else {
@@ -133,25 +148,6 @@ func isSocket(addr string) bool {
 	}
 }
 
-func withHTTPClient(tlsConfig *tls.Config) func(*client.Client) error {
-	return func(c *client.Client) error {
-		if tlsConfig == nil {
-			// Use the default HTTPClient
-			return nil
-		}
-		return client.WithHTTPClient(&http.Client{
-			Transport: &http.Transport{
-				TLSClientConfig: tlsConfig,
-				DialContext: (&net.Dialer{
-					KeepAlive: 30 * time.Second,
-					Timeout:   30 * time.Second,
-				}).DialContext,
-			},
-			CheckRedirect: client.CheckRedirect,
-		})(c)
-	}
-}
-
 // EndpointFromContext parses a context docker endpoint metadata into a typed EndpointMeta structure
 func EndpointFromContext(metadata store.Metadata) (EndpointMeta, error) {
 	ep, ok := metadata.Endpoints[DockerEndpoint]