OCPBUGS-79425: adding cred helpers to dockerconfigjson struct#5795
OCPBUGS-79425: adding cred helpers to dockerconfigjson struct#5795dalemcgavin wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @dalemcgavin. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dalemcgavin The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughSupport for credential helpers in Docker configuration was added by extending the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/secrets/secrets.go (2)
159-167:⚠️ Potential issue | 🟠 Major
CredHelpersnot copied during DockerConfigJSON conversion.The function copies
Authsbut omitsCredHelpers, causing credential helper data to be silently dropped when creating anImageRegistrySecretfrom aDockerConfigJSON. If the intent is to preserve the full config, consider copyingCredHelpersas well.Proposed fix
func newImageRegistrySecretFromDockerConfigJSON(dcj DockerConfigJSON) ImageRegistrySecret { cfg := newDockerConfigJSON() for key, val := range dcj.Auths { cfg.Auths[key] = val } + + if dcj.CredHelpers != nil { + cfg.CredHelpers = make(map[string]string, len(dcj.CredHelpers)) + for key, val := range dcj.CredHelpers { + cfg.CredHelpers[key] = val + } + } return &imageRegistrySecretImpl{cfg: cfg} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/secrets/secrets.go` around lines 159 - 167, newImageRegistrySecretFromDockerConfigJSON currently copies only DockerConfigJSON.Auths into cfg.Auths, dropping DockerConfigJSON.CredHelpers; update the function (newImageRegistrySecretFromDockerConfigJSON) to also copy dcj.CredHelpers into cfg.CredHelpers so credential helper entries are preserved when constructing the imageRegistrySecretImpl (cfg). Ensure you reference the DockerConfigJSON.CredHelpers map and assign its entries into the newly created cfg.CredHelpers alongside the existing loop that copies Auths.
316-331:⚠️ Potential issue | 🟡 Minor
Equalmethod ignoresCredHelpers.Two
ImageRegistrySecretinstances with identicalAuthsbut differentCredHelperswill be considered equal. If semantic equality should include credential helpers, this comparison needs updating.Proposed fix
func (i *imageRegistrySecretImpl) Equal(is2 ImageRegistrySecret) bool { dcj1 := i.DockerConfigJSON() dcj2 := is2.DockerConfigJSON() if len(dcj1.Auths) != len(dcj2.Auths) { return false } for key, val := range dcj1.Auths { if dcj2.Auths[key] != val { return false } } + if len(dcj1.CredHelpers) != len(dcj2.CredHelpers) { + return false + } + + for key, val := range dcj1.CredHelpers { + if dcj2.CredHelpers[key] != val { + return false + } + } + return true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/secrets/secrets.go` around lines 316 - 331, The Equal method on imageRegistrySecretImpl currently only compares DockerConfigJSON().Auths and thus treats instances with different CredHelpers as equal; update imageRegistrySecretImpl.Equal (and use ImageRegistrySecret.DockerConfigJSON()) to also compare the CredHelpers maps: first check lengths of dcj1.CredHelpers vs dcj2.CredHelpers, then iterate keys and ensure values match for every entry (and keep the existing Auths comparison). Ensure both Auths and CredHelpers are treated as part of semantic equality.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/secrets/secrets.go`:
- Around line 159-167: newImageRegistrySecretFromDockerConfigJSON currently
copies only DockerConfigJSON.Auths into cfg.Auths, dropping
DockerConfigJSON.CredHelpers; update the function
(newImageRegistrySecretFromDockerConfigJSON) to also copy dcj.CredHelpers into
cfg.CredHelpers so credential helper entries are preserved when constructing the
imageRegistrySecretImpl (cfg). Ensure you reference the
DockerConfigJSON.CredHelpers map and assign its entries into the newly created
cfg.CredHelpers alongside the existing loop that copies Auths.
- Around line 316-331: The Equal method on imageRegistrySecretImpl currently
only compares DockerConfigJSON().Auths and thus treats instances with different
CredHelpers as equal; update imageRegistrySecretImpl.Equal (and use
ImageRegistrySecret.DockerConfigJSON()) to also compare the CredHelpers maps:
first check lengths of dcj1.CredHelpers vs dcj2.CredHelpers, then iterate keys
and ensure values match for every entry (and keep the existing Auths
comparison). Ensure both Auths and CredHelpers are treated as part of semantic
equality.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68c682c6-68d2-4ed0-a629-7dd785ac9e24
📒 Files selected for processing (2)
pkg/controller/common/dockerconfig_test.gopkg/secrets/secrets.go
|
/retitle OCPBUGS-79425: adding cred helpers to dockerconfigjson struct |
openshift-ci
bot
changed the title
openshift-ci-robot
added
jira/severity-important
|
@dalemcgavin: This pull request references Jira Issue OCPBUGS-79425, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@isabella-janssen: This pull request references Jira Issue OCPBUGS-79425, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
|
@dalemcgavin: This pull request references Jira Issue OCPBUGS-79425, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@dalemcgavin: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
There is something to be said about a future change that persists the credHelpers into the |
3 participants
- What I did
Added "credHelpers" field to
DockerConfigJSON.From Openshift 4.19 -> 4.20 there is stricter decoding in place which causes secrets with this field to be rejected.
JSON decoder that is used has
DisallowUnknownFieldsenabled.machine-config-operator/pkg/secrets/secrets.go
Lines 243 to 247 in d4008e5
- How to verify it
I have written a new test, below shows the test output before and after the change.
- Description for the changelog
Adding
CredHelperstoDockerConfigJSON.- Background
Struggling to find a definitive definition of this config file.
Decided it was okay to only add credHelpers because of containers/image.
docker/cli also shows a huge definition here