MCO-1956: Extract image utils from build package #5400
Conversation
openshift-ci
bot
added
the
approved
| return nil, nil, fmt.Errorf("error retrieving image digest: %q: %w", imageName, err) | ||
| } | ||
|
|
||
| defer src.Close() |
There was a problem hiding this comment.
I moved this line a bit earlier as I think that the previous error conditions may make the function return without proper cleanup.
|
|
||
| if err := retry.RetryIfNecessary(ctx, func() error { | ||
| var imgInspect *types.ImageInspectInfo | ||
| if err := retry.IfNecessary(ctx, func() error { |
There was a problem hiding this comment.
Changed to IfNecessary as I saw we were already using it and RetryIfNecessary is deprecated.
There was a problem hiding this comment.
Good catch! 💯 I think there are a few things we use in our code base that are deprecated. Maybe a good task for AI would be getting it to find what's deprecated and suggest solutions. 🤔 Maybe I'll try that out once the release freeze is behind us.
There was a problem hiding this comment.
I catched it myself this time, but Claude has already told me about deprecated code. When I was coding the mco-sanitize Claude told me the GPG packet was about to be discontinued.
If you have the time... Everybody will welcome your effort :)
| temporalDir: temporalDir, | ||
| } | ||
|
|
||
| certsDir, err := sysContext.buildCertsFromControllerConfig(cc) |
There was a problem hiding this comment.
Really relevant change in this copied code: We were using a Deepcopy of the ControllerConfig as we were calling UpdateControllerConfigCerts.
That function, UpdateControllerConfigCerts, would require a revisit to clean up it (I don't know why it's removing certs from the controller FS, well, I know, it's because the remove logic was used as part ofr UTs, that never were shipped, but.... The cleanup logic and the wrongly performed computation of the modified flag require a cleanup too).
From the point of view of this function, it consumed the Spec side of the ControllerConfig, that is never modified so calling UpdateControllerConfigCerts makes no effect on Spec.ImageRegistryBundleData or Spec.ImageRegistryBundleUserData and thus, calling UpdateControllerConfigCerts is not required.
There was a problem hiding this comment.
So, the reason why the previous code used a DeepCopy() was to avoid accidentally mutating the lister cache when we mutate the ControllerConfig. I just wanted to add that extra bit of context here.
There was a problem hiding this comment.
| // writeCertFromImageRegistryBundle writes a certificate from an image registry bundle | ||
| // to the specified certificates directory. Path traversal attempts (..) are sanitized to colons. | ||
| func writeCertFromImageRegistryBundle(certsDir string, irb mcfgv1.ImageRegistryBundle) error { | ||
| caFile := strings.ReplaceAll(irb.File, "..", ":") |
There was a problem hiding this comment.
praise: Great idea!
In general, I think we all could do a lot better with practices like this.
There was a problem hiding this comment.
Yeah. It's just a copy of what you did, and what I see we are doing in the daemon. It can be improved, as that replace may lead to some awful paths, but I'd prefer to stick to what we have to, at least, be consistent.
There was a problem hiding this comment.
Another thing we could consider in the future is this: https://medium.com/@ajitem/secure-filesystem-access-in-go-1-24-introducing-os-root-dcb031732516. As a caveat, we won't be able to backport anything that uses it beyond 4.19 since it requires Golang 1.24+.
(Just to be clear: I'm not asking you to adopt that solution here. 😄)
| legacySecret := `{"registry.hostname.com": {"username": "user", "password": "s3kr1t", "auth": "s00pers3kr1t", "email": "[email protected]"}}` | ||
| newSecret := `{"auths":` + legacySecret + `}` | ||
|
|
||
| testCert := []byte(`-----BEGIN CERTIFICATE----- |
There was a problem hiding this comment.
thought (non-blocking): We may need to add a comment here to make the linter happy.
There was a problem hiding this comment.
Which linter is blaming? The CI ones are passing.
There was a problem hiding this comment.
If it's happy, then we don't need anything 😄. I just thought I'd mention it preemptively since I've had gosec complain about things like that in the past.
|
|
||
| // fakeImageInspector is a fake image inspector implementation used for testing | ||
| // the ImagePruner without requiring an actual image registry. | ||
| type fakeImageInspector struct { |
There was a problem hiding this comment.
issue: We may want to keep this fakeImageInspector implementation around along with a subset of the assertions that we make about the ImagePruner doing its job. However, they can be much higher-level than they are before making this change. They can just ensure that ImageInspect() and DeleteImage() get called.
There was a problem hiding this comment.
Agree. I'll fix this tomorrow's morning.
pablintino
force-pushed
the
image-utils
branch
from
e66ff5b to
df3cab5
Compare
This change extracts some containers/image common logic to a dedicated image utils package to improve reusability.
pablintino
force-pushed
the
image-utils
branch
from
df3cab5 to
e712da2
Compare
pablintino
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@pablintino: This pull request references MCO-1956 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In 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. |
isabella-janssen
left a comment
isabella-janssen
left a comment
There was a problem hiding this comment.
/lgtm
This change looks clean to me & previous comments have been addressed.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: isabella-janssen, pablintino The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test e2e-gcp-op-ocl |
|
/payload periodic-ci-openshift-openshift-tests-private-release-4.21-amd64-nightly-aws-ipi-longduration-mco-p3-f7 |
|
@sergiordlr: it appears that you have attempted to use some version of the payload command, but your comment was incorrectly formatted and cannot be acted upon. See the docs for usage info. |
|
/payload-job periodic-ci-openshift-openshift-tests-private-release-4.21-amd64-nightly-aws-ipi-longduration-mco-p3-f7 |
|
@sergiordlr: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/82a1d740-c070-11f0-94a5-37979c3da954-0 |
|
No issues found in the automated regression. All failed tests were known. /label qe-approved |
openshift-ci
bot
added
the
qe-approved
openshift-ci-robot
added
the
verified
|
@sergiordlr: This PR has been marked as verified by In 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. |
|
@pablintino: This pull request references MCO-1956 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In 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. |
|
/test e2e-hypershift |
|
/override ci/prow/e2e-hypershift |
|
@pablintino: Overrode contexts on behalf of pablintino: ci/prow/e2e-hypershift In 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 kubernetes-sigs/prow repository. |
|
/retest-required |
1 similar comment
|
/retest-required |
|
/hold Revision e712da2 was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest-required |
1 similar comment
|
/retest-required |
|
@pablintino: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
openshift-merge-bot
bot
merged commit 3ca2fc0
into
openshift:main
5 participants
This change extracts some containers/image common logic to a dedicated image utils package to improve reusability.
- What I did
Extract shared image utilities into pkg/imageutils to eliminate code duplication
- How to verify it
pre-submit tests and UTs should cover the changes.
- Description for the changelog
Extract shared image utilities into pkg/imageutils to eliminate code duplication