client: omit empty auth headers and use registry.RequestAuthConfig #50256
Conversation
a042766 to
45e2dcb
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
950c36b to
0a2e9ea
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
0a2e9ea to
334698f
Compare
There was a problem hiding this comment.
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 {
| 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) { |
There was a problem hiding this comment.
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)
334698f to
769149d
Compare
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return resp.Body, nil | ||
| } | ||
|
|
||
| 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}, |
There was a problem hiding this comment.
Wondering if we break Hyrum's law with this one 😅
Still, I think this makes sense!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
Directly accept a privilege-func, and set the auth-header optionally. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
769149d to
1c0d381
Compare
|
Failure is a flaky test; #49348 |
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)