-
Notifications
You must be signed in to change notification settings - Fork 102
CLID-513: operator catalog pinned by digest in ISC #1333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@adolfo-ab: This pull request references CLID-513 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.22.0" version, but no target version was set. 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. |
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: adolfo-ab 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 |
404c489 to
795d87c
Compare
|
@adolfo-ab: This pull request references CLID-513 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.22.0" version, but no target version was set. 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. |
484d802 to
7ffacbb
Compare
r4f4
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran m2d
$ oc-mirror --v2 --config isc.yaml file://data/pinned
and then d2m with the pinned ISC but it failed
$ oc-mirror --v2 --config data/pinned/working_dir/isc_pinned_....yaml --from file://data/pinned docker://localhost:5000 --dest-tls-verify=false
2026/01/07 12:29:36 [INFO] : 👋 Hello, welcome to oc-mirror
2026/01/07 12:29:36 [INFO] : ⚙️ setting up the environment for you...
2026/01/07 12:29:36 [INFO] : ⚙️ environment version: v0.2.0-alpha.1-508-g7ffacbb
2026/01/07 12:29:36 [INFO] : 🔀 workflow mode: diskToMirror
2026/01/07 12:29:36 [INFO] : Verified we can authenticate against registry "localhost:5000"
2026/01/07 12:29:36 [INFO] : 📦 Extracting mirror archive(s)...
data/pinnned/mirror_000001.tar (5.8 GiB / 5.8 GiB) [=======================================================================================================================================] 5s
2026/01/07 12:29:41 [INFO] : 🕵 going to discover the necessary images...
2026/01/07 12:29:41 [INFO] : 🔍 collecting release images...
2026/01/07 12:29:41 [INFO] : 🔍 collecting operator images...
✗ () Collecting catalog registry.redhat.io/redhat/redhat-operator-index@sha256:b3ae0162b3de1e193d86659681ad81f02a603cff133ec42a0be131d4fec9aefd
2026/01/07 12:29:41 [INFO] : 🔍 collecting additional images...
2026/01/07 12:29:41 [INFO] : 🔍 collecting helm images...
2026/01/07 12:29:41 [INFO] : mirror time : 5.668364559s
2026/01/07 12:29:41 [INFO] : 👋 Goodbye, thank you for using oc-mirror
2026/01/07 12:29:41 [ERROR] : [Executor] collection error: collect catalog "registry.redhat.io/redhat/redhat-operator-index@sha256:b3ae0162b3de1e193d86659681ad81f02a603cff133ec42a0be131d4fec9aefd": get manifest: error when creating a new image source: reading manifest sha256-b3ae0162b3de1e193d86659681ad81f02a603cff133ec42a0be131d4fec9aefd in localhost:55000/redhat/redhat-operator-index: manifest unknown
Running with the original ISC works:
$ oc-mirror --v2 --config isc.yaml --from file://data/pinned docker://localhost:5000 --dest-tls-verify=false
2026/01/07 12:34:51 [INFO] : 👋 Hello, welcome to oc-mirror
2026/01/07 12:34:51 [INFO] : ⚙️ setting up the environment for you...
2026/01/07 12:34:51 [INFO] : ⚙️ environment version: v0.2.0-alpha.1-508-g7ffacbb
2026/01/07 12:34:51 [INFO] : 🔀 workflow mode: diskToMirror
2026/01/07 12:34:51 [INFO] : Verified we can authenticate against registry "localhost:5000"
2026/01/07 12:34:51 [INFO] : 📦 Extracting mirror archive(s)...
data/pinnned/mirror_000001.tar (5.8 GiB / 5.8 GiB) [=======================================================================================================================================] 5s
2026/01/07 12:34:56 [INFO] : 🕵 going to discover the necessary images...
2026/01/07 12:34:56 [INFO] : 🔍 collecting release images...
2026/01/07 12:34:56 [INFO] : 🔍 collecting operator images...
✓ () Collecting catalog registry.redhat.io/redhat/redhat-operator-index:v4.18
2026/01/07 12:34:56 [INFO] : 🔍 collecting additional images...
2026/01/07 12:34:56 [INFO] : 🔍 collecting helm images...
2026/01/07 12:34:56 [INFO] : 🚀 Start copying the images...
2026/01/07 12:34:56 [INFO] : 📌 images to copy 5
✓ (0s) node-observability-operator-bundle-rhel8@sha256:0fbbb0b32adadc5746251103d25bba6bd365b2ff60e289e4694061fc92c3ce6e ➡️ localhost:5000/noo/
✓ (0s) node-observability-rhel8-operator@sha256:53ba77f648154ac8aca1ae6c83815078dfde211dba1e42f597e69844e4954c01 ➡️ localhost:5000/noo/
✓ (0s) node-observability-agent-rhel8@sha256:a8e60d9f63e2ed0cf1e27309aac54014e3f1efc947b51ca0a99376338d220875 ➡️ localhost:5000/noo/
✓ (2s) redhat-operator-index:v4.18 ➡️ localhost:5000/redhat/
5 / 5 (2s) [============================================================================================================================================================================] 100 %
✓ (2s) ose-kube-rbac-proxy@sha256:e3dad360d0351237a16593ca0862652809c41a2127c2f98b9e0a559568efbd10 ➡️ localhost:5000/openshift4/
2026/01/07 12:34:59 [INFO] : === Results ===
2026/01/07 12:34:59 [INFO] : ✓ 5 / 5 operator images mirrored successfully
2026/01/07 12:34:59 [INFO] : 📄 Generating IDMS file...
2026/01/07 12:34:59 [INFO] : data/pinnned/working-dir/cluster-resources/idms-oc-mirror.yaml file created
2026/01/07 12:34:59 [INFO] : 📄 No images by tag were mirrored. Skipping ITMS generation.
2026/01/07 12:34:59 [INFO] : 📄 Generating CatalogSource file...
2026/01/07 12:34:59 [INFO] : data/pinnned/working-dir/cluster-resources/cs-redhat-operator-index-v4-18.yaml file created
2026/01/07 12:34:59 [INFO] : 📄 Generating ClusterCatalog file...
2026/01/07 12:34:59 [INFO] : data/pinnned/working-dir/cluster-resources/cc-redhat-operator-index-v4-18.yaml file created
2026/01/07 12:34:59 [INFO] : mirror time : 8.553087761s
2026/01/07 12:34:59 [INFO] : 👋 Goodbye, thank you for using oc-mirror
60d1e9c to
084e0d7
Compare
aguidirh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @aaguirre,
Thanks for submmiting this PR, I have added some comments.
Please feel free to let me know in case of questions.
internal/pkg/config/pin_catalogs.go
Outdated
|
|
||
| // PinAndWriteConfigs pins catalogs and writes both ISC and DISC files. | ||
| // Returns paths to both written files (ISC path, DISC path). | ||
| func PinAndWriteConfigs( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only func used outside of config pkg, so it is correct to expose it. The other funcs PinCatalogDigests, WritePinnedISC and CreateDISCFromISC are used only internally in the config pkg so they could be lower-case which makes the funcs not exposed externally.
Another suggestion is about exposing PinAndWriteConfigs as an interface, this could allow us changing the implementation easily in future and also mocking tests for this func outside of the config pkg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partially done, want to discuss about the interface a bit more. Maybe do that in a follow-up PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@r4f4 the interface suggestion, what do you think about it?
24eea4a to
2e34655
Compare
2e34655 to
3f8031f
Compare
|
@adolfo-ab: all tests passed! 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. |
aguidirh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the PR looks good to me, I will approve as soon as @r4f4 reply my questions and adds LGTM.
| return "", fmt.Errorf("failed to marshal %s to YAML: %w", strings.ToUpper(configType), err) | ||
| } | ||
|
|
||
| if err := os.WriteFile(filePath, yamlData, 0o600); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When writing the file should we allow others to read? Probably it is not mandatory that the user who wrote the file is the same who is going to read it in another phase.
| // CLID-513: Normalize catalog reference to ensure consistent rebuiltTag | ||
| // whether catalog is specified by tag or digest in the ISC. | ||
| // Use catalog name + digest to ensure: | ||
| // - Same catalog with same filter → same rebuiltTag (both v4.18 and the corresponding digest produce same hash) | ||
| // - Different catalog versions with same filter → different rebuiltTag (v4.18 and v4.19 produce different hashes) | ||
| if c.Catalog != "" && catalogDigest != "" { | ||
| imgSpec, err := image.ParseRef(c.Catalog) | ||
| if err == nil { | ||
| // Normalize to: name@sha256:digest | ||
| c.Catalog = imgSpec.Name + "@sha256:" + catalogDigest | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@r4f4 I was wondering if changing the digestOfFilter could break something, but I was not able to find a scenario where it would go wrong. Do you think of any?
|
@adolfo-ab could you please run a test for me? Build an oc-mirror with the code of main branch run a m2d, then build a new oc-mirror with the code of your PR and run a d2m with the same image set config and on top of the tarball generated by the previous m2d (keep only the isc and the tar ball, delete the working-dir and the oc-mirror cache). Also, run the d2m without internet connection. Let's see if this is going to break something. |
Description
Add a feature to pin operator catalogs by digest, even if the original ImageSetConfig operator catalogs are referenced by tag.
When running the m2d or m2m workflows, a pinned ISC and the corresponding DISC are created in the working directory.
Github / Jira issue:
N/A
Please delete options that are not relevant.
How Has This Been Tested?
Expected Outcome
Please describe the outcome expected from the tests.