client: omit empty auth headers and use registry.RequestAuthConfig by thaJeztah · Pull Request #50256 · moby/moby

thaJeztah · 2025-06-23T09:34:39Z

client: Client.ServiceUpdate: don't manually construct header value

client: add staticAuth utility

Add a small utility to create a "RequestAuthConfig" from

client: client.tryImageCreate: accept registry.RequestAuthConfig

Directly accept a privilege-func, and set the auth-header optionally.

client: client.tryImagePush: accept registry.RequestAuthConfig

Directly accept a privilege-func, and set the auth-header optionally.

- Human readable description for the release notes

- A picture of a cute animal (not mandatory but encouraged)

client/request_test.go

Copilot

Pull Request Overview

This PR refactors header handling across client request methods to omit empty headers and to use a registry.RequestAuthConfig for authentication. Key changes include:

Updating header assignments to use Set and omitting headers with empty values.
Refactoring tryImagePush, tryImageCreate, and ImagePull/ImagePush to accept a RequestAuthConfig.
Introducing a new staticAuth utility function in the client/auth.go file.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
client/service_update.go	Changed header assignment to use headers.Set to handle auth header.
client/request.go	Added logic to omit empty headers and introduced helper notEmpty.
client/image_push.go	Updated tryImagePush signature to use RequestAuthConfig and adjusted logic.
client/image_pull.go	Updated tryImageCreate call and privilege function usage for pull.
client/image_create.go	Refactored tryImageCreate to accept RequestAuthConfig.
client/auth.go	Added staticAuth utility to create a RequestAuthConfig from a static value.

Comments suppressed due to low confidence (2)

client/request.go:302

[nitpick] Consider renaming the helper function 'notEmpty' to 'hasNonEmptyValue' for improved clarity about its purpose.

func notEmpty(vals []string) bool {

client/auth.go:10

[nitpick] Consider renaming 'staticAuth' to a more descriptive name such as 'createStaticAuthConfig' to better convey its functionality.

func staticAuth(registryAuth string) registry.RequestAuthConfig {

thaJeztah · 2025-06-24T13:32:14Z

client/image_create.go

-	return cli.post(ctx, "/images/create", query, nil, http.Header{
-		registry.AuthHeader: {registryAuth},
-	})
+func (cli *Client) tryImageCreate(ctx context.Context, query url.Values, resolveAuth registry.RequestAuthConfig) (*http.Response, error) {


Basically, I want to start migrating towards always using a "privilege-func" instead of assuming it's static; that way we have more consistent integration points to fetch auth (which could be dynamically)

vvoland

LGTM

vvoland · 2025-06-27T10:46:15Z

client/image_create.go


-func (cli *Client) tryImageCreate(ctx context.Context, query url.Values, registryAuth string) (*http.Response, error) {
-	return cli.post(ctx, "/images/create", query, nil, http.Header{
-		registry.AuthHeader: {registryAuth},


Wondering if we break Hyrum's law with this one 😅

Still, I think this makes sense!

This one should probably be ok; more thinking now of we should include the first commit; I realised that addHeaders is also called as part of the hijack, which does the h2c upgrade, and I just found some mention somewhere that HTTP2-Settings (obsolete though!) may have an empty string as value.

I can remove the first commit here, so that we just limit the change to not sending the empty Auth headers.

I dropped the first commit here; we can still discuss that one separately.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Add a small utility to create a "RequestAuthConfig" from a static value. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Directly accept a privilege-func, and set the auth-header optionally. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2025-06-27T15:23:29Z

Failure is a flaky test; #49348

=== Failed
=== FAIL: github.com/docker/docker/integration/container TestCreateByImageID/image_ID_with_algorithm (60.42s)
    create_test.go:139: assertion failed: error is not nil: error during connect: Post "http://%2F%2F.%2Fpipe%2Fdocker_engine/v1.51/containers/create": EOF

thaJeztah added this to the 29.0.0 milestone Jun 23, 2025

thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code area/authentication labels Jun 23, 2025

thaJeztah changed the title ~~client_auth_RequestAuthConfig~~ client: omit empty headers and use registry.RequestAuthConfig Jun 23, 2025

thaJeztah marked this pull request as ready for review June 23, 2025 12:11

thaJeztah commented Jun 23, 2025

View reviewed changes

client/request_test.go Outdated Show resolved Hide resolved

thaJeztah force-pushed the client_auth_RequestAuthConfig branch from a042766 to 45e2dcb Compare June 23, 2025 12:14

thaJeztah requested review from Copilot and vvoland June 23, 2025 12:15

This comment was marked as outdated.

Sign in to view

thaJeztah force-pushed the client_auth_RequestAuthConfig branch 4 times, most recently from 950c36b to 0a2e9ea Compare June 23, 2025 15:24

thaJeztah requested a review from Copilot June 23, 2025 15:29

This comment was marked as outdated.

Sign in to view

thaJeztah force-pushed the client_auth_RequestAuthConfig branch from 0a2e9ea to 334698f Compare June 23, 2025 15:36

thaJeztah requested a review from Copilot June 23, 2025 15:37

Copilot AI reviewed Jun 23, 2025

View reviewed changes

thaJeztah commented Jun 24, 2025

View reviewed changes

Benehiko approved these changes Jun 24, 2025

View reviewed changes

thaJeztah force-pushed the client_auth_RequestAuthConfig branch from 334698f to 769149d Compare June 26, 2025 18:54

vvoland approved these changes Jun 27, 2025

View reviewed changes

thaJeztah added 4 commits June 27, 2025 13:12

client: Client.ServiceUpdate: don't manually construct header value

871543a

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

client: add staticAuth utility

79b4e18

Add a small utility to create a "RequestAuthConfig" from a static value. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

client: client.tryImageCreate: accept registry.RequestAuthConfig

ca0afe9

Directly accept a privilege-func, and set the auth-header optionally. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

client: client.tryImagePush: accept registry.RequestAuthConfig

1c0d381

Directly accept a privilege-func, and set the auth-header optionally. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the client_auth_RequestAuthConfig branch from 769149d to 1c0d381 Compare June 27, 2025 11:12

thaJeztah changed the title ~~client: omit empty headers and use registry.RequestAuthConfig~~ client: omit empty auth headers and use registry.RequestAuthConfig Jun 27, 2025

thaJeztah merged commit b59ba94 into moby:master Jun 27, 2025
229 of 232 checks passed

thaJeztah deleted the client_auth_RequestAuthConfig branch June 27, 2025 17:13

This was referenced Jul 10, 2025

[28.x backport] Add support for chained RequestAuthConfigs for push/pull #50368

Closed

client: always send (empty) body on push #50415

Merged

api/types/registry: EncodeAuthConfig: use empty string for zero value #50426

Merged

thaJeztah mentioned this pull request Jul 22, 2025

[28.x backport] client: always send (empty) body on push #50471

Merged

dmcgowan mentioned this pull request Aug 5, 2025

Prepare release notes for client/v0.1.0-alpha.0 #50637

Merged

austinvazquez mentioned this pull request Aug 5, 2025

Bump api version in client and main module to v1.52.0-alpha.1 #50641

Merged

dmcgowan mentioned this pull request Sep 5, 2025

Prepare release notes for client/v0.1.0-beta.0 #50913

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: omit empty auth headers and use registry.RequestAuthConfig#50256

client: omit empty auth headers and use registry.RequestAuthConfig#50256
thaJeztah merged 4 commits intomoby:masterfrom
thaJeztah:client_auth_RequestAuthConfig

thaJeztah commented Jun 23, 2025 •

edited

Loading

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Uh oh!

thaJeztah Jun 24, 2025

Uh oh!

vvoland left a comment

Uh oh!

vvoland Jun 27, 2025

Uh oh!

thaJeztah Jun 27, 2025

Uh oh!

thaJeztah Jun 27, 2025

Uh oh!

thaJeztah commented Jun 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

thaJeztah commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

client: Client.ServiceUpdate: don't manually construct header value

client: add staticAuth utility

client: client.tryImageCreate: accept registry.RequestAuthConfig

client: client.tryImagePush: accept registry.RequestAuthConfig

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

thaJeztah Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

vvoland Jun 27, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jun 27, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jun 27, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jun 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thaJeztah commented Jun 23, 2025 •

edited

Loading