bugfix : fix image auth bug #2900
Conversation
kubesphere-prow
bot
added
release-note
kind/bug
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zuoxuesong-worker The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubesphere-prow
bot
added
the
approved
Summary of ChangesHello @zuoxuesong-worker, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the image authentication mechanism where the system would incorrectly select authentication credentials, particularly when multiple authentication entries existed for different levels of a repository path. By introducing a new function to intelligently select the most specific authentication based on the image's full path, this change ensures that image pull and push operations use the correct credentials and TLS verification settings, thereby improving the reliability and security of image handling. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
kubesphere-prow
bot
added
the
size/M
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a bug in image credential selection by introducing a longest-prefix match logic in the new selectAuth function. The changes to use this function for Credential and InsecureSkipVerify are well-implemented. However, I've identified a few areas for improvement. The plainHTTP setting is still determined using the old, incorrect logic and should be updated to use the newly selected auth. Additionally, the tag-stripping logic within selectAuth has a bug when image names include a port, and there's a potential issue with how duplicate auth entries are handled. I've left specific comments with suggestions to address these points.
| if err != nil { | ||
| return errors.Wrapf(err, "failed to get remote image %s", img) | ||
| } | ||
| selectedAuth := selectAuth(img, i.auths) |
There was a problem hiding this comment.
While Credential and InsecureSkipVerify are now correctly configured using selectedAuth, the plainHTTP setting is determined later on line 208 using plainHTTPFunc(img, i.auths, false), which still contains the old logic that doesn't respect longest-prefix matching. For consistency and correctness, plainHTTPFunc should be refactored to accept selectedAuth (similar to the change made for skipTLSVerifyFunc), and then used to set src.PlainHTTP.
pkg/modules/image.go
Outdated
|
|
||
| func selectAuth(image string, authList []imageAuth) imageAuth { | ||
| repoPart := image | ||
| if colonIdx := strings.LastIndex(image, ":"); colonIdx != -1 { |
There was a problem hiding this comment.
The current logic for stripping the image tag is not robust. It will incorrectly handle image names that include a port in the registry name, for example, my.registry:5000/my/image. In this case, it would strip from the port colon, resulting in my.registry. A better approach is to check if the colon appears after the last slash.
| if colonIdx := strings.LastIndex(image, ":"); colonIdx != -1 { | |
| if colonIdx := strings.LastIndex(image, ":"); colonIdx > strings.LastIndex(image, "/") { |
| if err != nil { | ||
| return errors.Wrapf(err, "failed to get remote repository %q", dest) | ||
| } | ||
| selectedAuth := selectAuth(dest, i.auths) |
There was a problem hiding this comment.
Similar to the pull function, while Credential and InsecureSkipVerify are now correctly configured using selectedAuth, the plainHTTP setting is determined later on line 356 using plainHTTPFunc(dest, i.auths, false), which still contains the old logic. For consistency and correctness, plainHTTPFunc should be refactored to accept selectedAuth and be used to set dst.PlainHTTP.
pkg/modules/image.go
Outdated
| authMap := make(map[string]imageAuth) | ||
| for _, a := range authList { | ||
| authMap[a.Repo] = a | ||
| } |
There was a problem hiding this comment.
When building authMap, if authList contains multiple entries for the same repository, the last one will silently overwrite the previous ones. This could lead to unexpected behavior if a user accidentally provides duplicate configurations. It would be good to either document this behavior or log a warning when an entry is overwritten.
Signed-off-by: [email protected] <[email protected]> bugfix : fix image auth bug Signed-off-by: [email protected] <[email protected]>
zuoxuesong-worker
force-pushed
the
bugfix/image-auth-bug
branch
from
fba0039 to
5e8a9e7
Compare
kubesphere-prow
bot
added
size/L
|
Signed-off-by: [email protected] <[email protected]> bugfix : fix image auth bug Signed-off-by: [email protected] <[email protected]>
|
This PR has multiple commits, and the default merge method is: squash. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: 227eb7fd9392dfaf913e1c28e739dc4b770017d8 |
2 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
fix a bug that image auth selector select an empty auth
for example, when user use auth like
{"repo":"somehub","username":"aaa","password":"bbb"},
{"repo":"somehub/someproj","username":"ccc","password":"ddd"},
{"repo":"somehub/otherproj"","username":"eee","password":"fff"}
and then push image :
"somehub/someproj/sub.proj/name:tag",
"somehub/otherproj/image:latest",
"somehub/myimage:v1.0",
"somehub/someproj/another:tag",
"otherhub/image:latest",
which auth should selected for each image?
according to the old func ,they would all select aaa/bbb
but when image should push to someproj ,acturaly we should choose ccc/ddd
so add a func to select the currect auth
Which issue(s) this PR fixes:
Fixes #
#2898
Special notes for reviewers:
Does this PR introduced a user-facing change?
Additional documentation, usage docs, etc.: