CNTRLPLANE-1961: feat(nodepool )Add support for ImageTagMirrorSet config#7566
CNTRLPLANE-1961: feat(nodepool )Add support for ImageTagMirrorSet config#7566mariusbertram wants to merge 1 commit intoopenshift:mainfrom
Conversation
WalkthroughAdds end-to-end ImageTagMirrorSet (ITMS) support: management-cluster capability detection, parsing (including ConfigMap reference), aggregation with IDMS/ICSP, reconciliation (ignition/MCO paths), API/CRD and applyconfig additions for ImageMirrorConfigRef, tests, and documentation; also propagates ImageMirrorConfigRef from HostedCluster to HostedControlPlane. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Out-of-scope changes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
do-not-merge/work-in-progress
|
Hi @mariusbertram. 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 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. |
openshift-ci
bot
added
area/hypershift-operator
mariusbertram
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@mariusbertram: This pull request references CNTRLPLANE-1961 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. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@mariusbertram: This pull request references CNTRLPLANE-1961 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. |
openshift-ci
bot
added
the
area/documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/content/reference/image-registry-mirrors.md`:
- Around line 147-158: Update the fenced code blocks under the headings
"Management Cluster Mirror Detection", "Guest Cluster IDMS (Automatic)" and
"Guest Cluster ITMS (Manual)" (and the other similar blocks referenced around
lines 160-171 and 173-184) to include a language identifier (e.g., "text") after
the opening ``` so markdownlint MD040 is satisfied; locate the blocks by the
heading text (e.g., "Management Cluster Mirror Detection", "Guest Cluster IDMS
(Automatic)", "Guest Cluster ITMS (Manual)") and add the language token to each
opening fence.
🧹 Nitpick comments (2)
docs/content/reference/aggregated-docs.md (1)
41824-41836: Hardcoded line numbers will require frequent maintenance.The "Implementation Details" section includes specific line numbers for code locations. These will become stale quickly as the codebase evolves, creating a maintenance burden and potentially misleading readers.
Consider either:
- Removing the specific line numbers and keeping just the file paths and function names
- Adding a note that line numbers are approximate and subject to change
- Using symbolic references (e.g., "search for
CapabilityITMSin the file")♻️ Proposed approach without hardcoded line numbers
### Code Locations **NodePool Validation**: `hypershift-operator/controllers/nodepool/config.go` -- Line 297: ImageTagMirrorSet case added to validation switch +- ImageTagMirrorSet case in validation switch **Capability System**: `support/capabilities/management_cluster_capabilities.go` -- Lines 69-70: CapabilityITMS constant -- Lines 214-221: ITMS API detection logic +- CapabilityITMS constant definition +- ITMS API detection logic **Mirror Extraction**: `support/globalconfig/imagecontentsource.go` -- Lines 66-89: Factory functions for ITMS -- Lines 194-229: getImageTagMirrorSets() function +- Factory functions for ITMS +- getImageTagMirrorSets() function - Updated GetAllImageRegistryMirrors() with ITMS parameter **Control Plane Operator**: `control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go` -- Lines 2971-2991: reconcileImageContentPolicyType() - creates IDMS only +- reconcileImageContentPolicyType() - creates IDMS onlysupport/globalconfig/imagecontentsource_test.go (1)
156-162: Avoid taking the address of the range variable.
Using&itmscan alias the same variable if this list grows beyond one item. Consider indexing the slice directly (and align the other loops if you want consistency).♻️ Proposed refactor
- itmsObjs := make([]client.Object, len(tc.itms.Items)) - for i, itms := range tc.itms.Items { - itmsObjs[i] = &itms - } + itmsObjs := make([]client.Object, len(tc.itms.Items)) + for i := range tc.itms.Items { + itmsObjs[i] = &tc.itms.Items[i] + }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/content/reference/aggregated-docs.md`:
- Around line 10366-10519: The added documentation uses fenced code blocks
(```yaml and ```bash) under headings like "Configure Digest-based Mirrors
(IDMS)", "Configure Tag-based Mirrors (ITMS)" and the "Verify Configuration"
sections which triggers markdownlint MD046; fix by converting each fenced block
(the YAML snippets for hostedcluster.yaml, itms-config.yaml, nodepool.yaml and
the bash snippets for oc apply/oc get/cat commands) to indented code blocks
(prefix each code line with four spaces) or alternatively add a file-level rule
to disable MD046 for this aggregated docs page if fenced blocks are desired.
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/content/how-to/configure-image-registry-mirrors.md`:
- Line 3: Replace the unhyphenated phrase "HyperShift hosted" with the
hyphenated compound adjective "HyperShift-hosted" in the document; search for
the exact string "HyperShift hosted clusters" (or any occurrence of "HyperShift
hosted") and update it to "HyperShift-hosted clusters" to correct the compound
adjective.
In `@docs/content/reference/aggregated-docs.md`:
- Line 41682: The text incorrectly states ImageContentSourcePolicy (ICSP) was
"removed in OpenShift 4.17"; update the phrasing for the ICSP entries (search
for "ImageContentSourcePolicy" or "ICSP") to say it was "deprecated in OpenShift
4.17 and is still supported; it will be removed in a future release" (or similar
wording indicating deprecation only), and ensure the ITMS availability statement
for 4.13+ (reference "ITMS") is left unchanged.
♻️ Duplicate comments (1)
docs/content/reference/aggregated-docs.md (1)
10370-10511: Markdownlint MD046: fenced blocks vs indented blocks.The code examples use fenced code blocks (
yaml,bash), but markdownlint expects indented blocks. This was already identified in a previous review.
🧹 Nitpick comments (4)
docs/content/reference/image-registry-mirrors.md (1)
126-139: Line number references in documentation may become stale.The "Code Locations" section references specific line numbers (e.g., "Line 297", "Lines 69-70", "Lines 214-221"). These will likely become outdated as the codebase evolves.
Consider removing line numbers and referencing only file paths and function/type names, or using code search links instead.
support/globalconfig/imagecontentsource.go (1)
117-117: Consider using a struct or options pattern for capability flags.The function signature now has three consecutive boolean parameters (
mgmtClusterHasIDMSCapability, mgmtClusterHasITMSCapability, mgmtClusterHasICSPCapability), which can be error-prone at call sites. The parameter order (IDMS, ITMS, ICSP) doesn't follow alphabetical or chronological order.This is acceptable for now, but if more capability flags are added in the future, consider refactoring to use a struct or passing the
CapabiltyCheckerdirectly.support/globalconfig/imagecontentsource_test.go (1)
286-437: Consider adding ITMS test coverage toTestReconcileMgmtImageRegistryOverrides.The
TestReconcileMgmtImageRegistryOverridestest only covers ICSP and IDMS capabilities. Consider adding a test case that includesCapabilityITMSto ensure theCommonRegistryProvidercorrectly handles ITMS mirrors.docs/content/reference/aggregated-docs.md (1)
41797-41812: Implementation details reference specific line numbers.The documentation references specific code locations and line numbers (e.g., "Line 297", "Lines 69-70", "Lines 214-221"). While this is valuable for current reference, these line numbers will become outdated as the codebase evolves.
Consider adding a note that line numbers are approximate and may shift, or establish a process to update these references periodically.
|
Hey @mariusbertram thanks for the PR. I've been going through the code and I have a preliminar feedback before going deep in more detail: Concern about execution mode consistencyThe design for ImageTagMirrorSet (ITMS) differs from ImageDigestMirrorSet (IDMS), which may confuse users.
This inconsistency means users must use different approaches for similar functionality. Consider aligning ITMS with IDMS by:
This would provide a consistent experience and reduce confusion. If there’s a reason for the different approach, documenting it would help. |
Hi @jparrill, In our production environment we operate a larger HCP setup backed by oVirt. From an operational workflow and automation perspective, managing the ImageDigestMirrorSet (IDMS) configuration via a referenced ConfigMap has proven to be the most efficient and user‑friendly approach. The ConfigMap‑based method allows us to version, template, and distribute IDMS configuration through GitOps pipelines without having to touch each cluster individually. At the moment, we maintain 10 HostedControlPlane (HCP) clusters, and we plan to scale this to more than 200 clusters within this year. ProposalMy proposal is to extend the Conceptually, something like: Hostedcluster CR: spec:
imageMirrorConfigRef:
name: global-image-mirror-config
namespace: my-config-namespaceThe referenced ConfigMap could contain structured keys such as: data:
idms.yaml: |
apiVersion: config.openshift.io/v1
kind: ImageDigestMirrorSet
spec:
imageDigestMirrors:
- source: quay.io/example
mirrors:
- mirror.registry.local
itms.yaml: |
apiVersion: config.openshift.io/v1
kind: ImageTagMirrorSet
spec:
imageTagMirrors:
- source: quay.io/example/app
mirrors:
- mirror.registry.local/appBenefits
|
openshift-ci
bot
added
area/api
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml`:
- Around line 2481-2502: The CRD adds imageMirrorConfigRef but controllers do
not parse or reconcile it; either implement parsing/reconciliation or remove the
field from the CRD. Implementing: update the HostedCluster/NodePool reconcile
paths (the controller that currently handles imageContentSources) to read the
referenced ConfigMap when imageMirrorConfigRef is set, validate and parse keys
"idms.yaml" and "itms.yaml" into ImageDigestMirrorSet (IDMS) and
ImageTagMirrorSet (ITMS) structures, merge/translate them into the same payloads
used for NodePool rollout, and trigger rollouts the same way changes to
imageContentSources do; ensure errors are surfaced in status and events.
Alternatively, remove imageMirrorConfigRef from the CRD and any docs to avoid
silently ignored fields.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml`:
- Around line 2849-2870: The CRD documents imageMirrorConfigRef but it is not
implemented; either remove the imageMirrorConfigRef field and its documentation
from the HostedCluster CRD (and any generated manifests) so users aren't misled,
or implement the feature end-to-end: add parsing of the referenced ConfigMap
(keys "idms.yaml" and/or "itms.yaml") in the controllers, sync the resulting
mirror configuration into the HostedControlPlane spec (e.g., add a field in
HostedControlPlane corresponding to imageMirrorConfigRef), implement
reconciliation in the control-plane-operator to apply IDMS/ITMS changes, and
ensure changes to imageMirrorConfigRef trigger NodePool rollouts; update any
related API annotations (x-kubernetes-map-type, defaults) and unit/e2e tests
accordingly.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yaml`:
- Around line 2481-2502: The API currently allows both imageMirrorConfigRef and
imageContentSources to be set with no guidance; add explicit validation and
documentation stating their interaction and enforce it in the CRD and
controller: update the CRD schema around the properties imageMirrorConfigRef and
imageContentSources to either (a) use an OpenAPI oneOf/anyOf rule to make them
mutually exclusive, or (b) add an explicit precedence rule in the property
descriptions and implement a validating webhook (or controller-side validation)
in the reconciliation path to reject or normalize objects that set both; update
the imageMirrorConfigRef description to mention the chosen behavior and
reference imageContentSources, and ensure the controller code that consumes
imageMirrorConfigRef (reconciler handling of HostedCluster/NodePool) implements
the same precedence logic.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml`:
- Around line 2481-2502: Add a CEL x-kubernetes-validations rule to the CRD
schema to enforce mutual exclusivity between imageMirrorConfigRef and
imageContentSources: add a validation (using has() checks) that exactly one of
imageMirrorConfigRef or imageContentSources is present and return a clear
message when the rule fails; reference the schema properties
imageMirrorConfigRef and imageContentSources so the rule is attached at the same
object level where those properties are defined.
In `@docs/content/how-to/configure-image-registry-mirrors.md`:
- Around line 301-348: The example uses the legacy separate imageContentSources
+ ConfigMap/config approach; update it to the unified imageMirrorConfigRef
pattern by replacing the HostedCluster.spec.imageContentSources and the separate
ConfigMap/config entry with a single ConfigMap (e.g., name
disconnected-image-mirrors) containing idms.yaml and itms.yaml entries
(ImageDigestMirrorSet and ImageTagMirrorSet) and set
HostedCluster.spec.imageMirrorConfigRef.name to that ConfigMap name; ensure
NodePool no longer references the old ConfigMap via config and that the
ConfigMap keys are named (idms.yaml, itms.yaml) matching the unified approach.
- Line 217: Several code blocks in
docs/content/how-to/configure-image-registry-mirrors.md are using indented
blocks instead of fenced code blocks; replace each indented block (e.g., the
YAML snippet containing "mirrorSourcePolicy: NeverContactSource" and the bash
commands like "oc extract secret/my-cluster-admin-kubeconfig --to=- >
/tmp/my-cluster-kubeconfig") with fenced triple-backtick blocks and appropriate
language tags (```yaml, ```bash) so they match the rest of the document and
enable syntax highlighting; update every occurrence referenced in the comment
(lines around the mirrorSourcePolicy snippet and the
verification/troubleshooting commands) to use fenced blocks.
In `@docs/content/reference/aggregated-docs.md`:
- Around line 41819-42044: The reference doc for Image Registry Mirrors omits
the new unified imageMirrorConfigRef approach; update this file to document
imageMirrorConfigRef as a supported configuration method (referencing the field
name imageMirrorConfigRef and NodePool.spec.config), add it to the comparison
table (reflecting automatic ITMS reconciliation when imageMirrorConfigRef is
used), add a short workflow diagram/text showing the unified path
(HostedCluster/NodePool -> imageMirrorConfigRef -> reconciled IDMS/ITMS), and
add guidance clarifying when to prefer imageMirrorConfigRef vs. the legacy
NodePool ConfigMap approach (mention getImageTagMirrorSets() /
reconcileImageContentPolicyType() behavior as relevant).
In `@docs/content/reference/api.md`:
- Around line 709-725: Update the imageMirrorConfigRef documentation to
explicitly describe its interaction with imageContentSources and ConfigMap
content changes: state whether imageMirrorConfigRef and imageContentSources are
merged, which one takes precedence (or if conflicts produce an error), and how
conflicts are resolved (e.g., per-image or per-registry rules); and clarify that
changing the ConfigMap keys/values referenced by imageMirrorConfigRef (without
changing the reference itself) will/will not trigger controller reconciliation
and NodePool rollouts, including which controller behavior to expect
(controllers parse the ConfigMap keys "idms.yaml"/"itms.yaml", reconcile both
IDMS and ITMS, and perform rollouts on content changes) and mention any
debounce/aggregation or GitOps notes for operators.
In `@docs/content/reference/image-registry-mirrors.md`:
- Around line 60-96: Update the ImageTagMirrorSet docs to cover both supported
entry points: keep the NodePool-level manual ConfigMap example (ConfigMap named
in NodePool.spec.config) and add a section describing
HostedCluster.imageMirrorConfigRef — explain it accepts a ConfigMap with an
"itms.yaml" key that contains ITMS configuration and is applied cluster-wide to
all NodePools; clarify how it differs from the NodePool approach (scope and
application) and ensure references to ImageTagMirrorSet,
HostedCluster.imageMirrorConfigRef, and NodePool.spec.config are included so
readers can find the correct configuration path.
In `@support/globalconfig/imagecontentsource_test.go`:
- Around line 157-163: In the three object-building loops (variables idms, icsp,
itms) you are taking the address of the range loop variable which yields the
same pointer for every iteration; update each loop to take the address from the
slice by index (e.g., use &tc.itms.Items[i] instead of &itms) so each appended
client.Object points to the distinct element; apply the same indexed fix to the
idms and icsp loops as well.
🧹 Nitpick comments (4)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yaml (1)
2503-2524: Clarify interaction withimageContentSourcesLine 2503 introduces
imageMirrorConfigRef, butspec.imageContentSourcesremains available; the schema doesn’t state precedence or mutual exclusivity, so users can set both and get ambiguous mirror behavior. Consider documenting merge/precedence here or adding a validation to prevent simultaneous use if it isn’t supported.cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml (1)
3231-3252: Clarify interaction withimageContentSources.
imageMirrorConfigRefcan now supply IDMS as well; if users also setimageContentSources, precedence/merge behavior isn’t defined in the schema. Consider adding a spec-level validation to make them mutually exclusive or explicitly documenting precedence to avoid conflicting mirror configurations and unintended rollouts.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml (1)
2633-2654: Clarify or validate coexistence withimageContentSources.
imageMirrorConfigRefnow sits alongsideimageContentSources, but the schema doesn’t declare whether both are allowed. If controllers don’t intentionally merge these sources, users can end up with ambiguous or conflicting mirror configs. Consider adding an XValidation to make them mutually exclusive (or explicitly document merge/precedence).Suggested schema guard (add at spec-level x-kubernetes-validations)
@@ - x-kubernetes-validations: + x-kubernetes-validations: + - message: imageMirrorConfigRef and imageContentSources are mutually exclusive + rule: '!has(self.imageMirrorConfigRef) || !has(self.imageContentSources)'docs/content/reference/aggregated-docs.md (1)
41946-41962: Consider removing specific line number references.The implementation details section includes specific line numbers (e.g., "Line 297", "Lines 69-70", "Lines 214-221") that will become stale as the codebase evolves. These references require manual maintenance and can mislead readers if not kept current.
Consider either:
- Removing line numbers and keeping only file paths and function names
- Using relative descriptions (e.g., "in the validation switch statement" instead of "Line 297")
- Adding a timestamp or version note indicating when these references were accurate
...rd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml
Show resolved
Hide resolved
...enerated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml
Show resolved
Hide resolved
...generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yaml
Show resolved
Hide resolved
...z_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/content/reference/aggregated-docs.md`:
- Around line 42114-42123: The workflow header "Guest Cluster ITMS (Manual)" and
steps currently show only the NodePool ConfigMap path (NodePool.spec.config[])
but omit the cluster-wide reference HostedCluster.spec.imageMirrorConfigRef;
update the diagram to either (a) include an additional branch/prior step showing
HostedCluster.spec.imageMirrorConfigRef → cluster-level validation/deployment
(so registries.conf can be populated from the cluster-wide mirror config), or
(b) if the intent is solely NodePool-scoped, rename the header to "NodePool ITMS
(Manual)" and keep NodePool.spec.config[] as the sole path; ensure the step
labels reference HostedCluster.spec.imageMirrorConfigRef and
NodePool.spec.config[] explicitly and adjust the NodePool controller / MCO steps
to reflect which source (nodepool vs cluster) is being validated/deployed.
- Around line 42125-42131: Update the ICSP removal timeline wording in the
"ImageContentSourcePolicy (ICSP)" section so it reflects deprecation only for
4.17: replace the phrase "will be removed in OpenShift 4.17" with language such
as "is deprecated in OpenShift 4.17 and scheduled for removal in a future
release" and ensure the surrounding bullets (the lines mentioning deletion and
continued support) remain consistent with the deprecation wording for ICSP.
🧹 Nitpick comments (3)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yaml (1)
2403-2422: Clarify/validate interaction withspec.imageContentSources.
Line 2403 introducesimageMirrorConfigRefas an alternative source for mirror config, but the behavior when bothimageContentSourcesandimageMirrorConfigRefare set is unclear. Please confirm the intended precedence/merge behavior and either document it here or enforce mutual exclusivity with anx-kubernetes-validationsrule to avoid ambiguous rollouts.hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go (1)
2362-2364: Remove redundantImageMirrorConfigRefassignmentThe conditional block is overwritten by the later unconditional assignment, so it adds noise and can mislead future edits. Keep a single assignment (preferably the later one to allow clearing on nil).
♻️ Proposed fix
- if hcluster.Spec.ImageMirrorConfigRef != nil { - hcp.Spec.ImageMirrorConfigRef = hcluster.Spec.ImageMirrorConfigRef - }Also applies to: 2383-2384
support/globalconfig/imagecontentsource.go (1)
17-19: Consider strict YAML decoding for mirror ConfigMaps.
yaml.Unmarshalsilently ignores unknown fields, so mis-typed keys or wrong manifests can yield empty specs without an error. A strict decode helps fail fast for admin-provided configs. Please confirmyaml.UnmarshalStrictis available ink8s.io/apimachinery/pkg/util/yamlv0.34.3; otherwise usesigs.k8s.io/yaml.UnmarshalStrict.🔧 Proposed adjustment
- if err := yaml.Unmarshal([]byte(idmsYAML), idms); err != nil { + if err := yaml.UnmarshalStrict([]byte(idmsYAML), idms); err != nil { return nil, nil, fmt.Errorf("failed to parse idms.yaml from ConfigMap: %w", err) } @@ - if err := yaml.Unmarshal([]byte(itmsYAML), itms); err != nil { + if err := yaml.UnmarshalStrict([]byte(itmsYAML), itms); err != nil { return nil, nil, fmt.Errorf("failed to parse itms.yaml from ConfigMap: %w", err) }Also applies to: 153-186
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
support/globalconfig/imagecontentsource_test.go (1)
406-420: Address-of-range-variable bug remains in this test function.The same bug that was fixed in
TestGetAllImageRegistryMirrors(lines 143-165) still exists here. All pointers will reference the last loop element, causing incorrect test data setup.🐛 Proposed fix
if tc.idms != nil { idmsObjs := make([]client.Object, len(tc.idms.Items)) - for i, idms := range tc.idms.Items { - idmsObjs[i] = &idms + for i := range tc.idms.Items { + idmsObjs[i] = &tc.idms.Items[i] } objs = append(objs, idmsObjs...) } if tc.icsp != nil { icspObjs := make([]client.Object, len(tc.icsp.Items)) - for i, icsp := range tc.icsp.Items { - icspObjs[i] = &icsp + for i := range tc.icsp.Items { + icspObjs[i] = &tc.icsp.Items[i] } objs = append(objs, icspObjs...) }
🤖 Fix all issues with AI agents
In `@api/hypershift/v1beta1/hostedcluster_types.go`:
- Around line 742-752: Update the ImageMirrorConfigRef field comment to
explicitly state merging behavior with ImageContentSources: clarify that when
both ImageContentSources and ImageMirrorConfigRef are set on the HostedCluster,
ImageContentSources are processed first and then the ConfigMap entries
referenced by ImageMirrorConfigRef are merged in (so the ConfigMap can
add/override entries), and that this merging is performed by the controllers
during reconciliation and still triggers rollouts for NodePools when changed;
reference the symbols ImageMirrorConfigRef, ImageContentSources, and
HostedCluster in the comment to make locating the behavior clear.
In `@docs/content/reference/image-registry-mirrors.md`:
- Around line 298-303: Update the "For Tag-based Mirrors" best-practices text to
state that the recommended approach is to use
HostedCluster.spec.imageMirrorConfigRef (label it "(Recommended)"), mention
NodePool.spec.config as an alternative for per-nodepool overrides, and reference
the unified configuration approach described earlier (the
imageMirrorConfigRef/unified configuration section) as the comprehensive best
practice; ensure you reference the symbols
HostedCluster.spec.imageMirrorConfigRef and NodePool.spec.config and point
readers to the unified configuration guidance for full details.
In `@support/globalconfig/imagecontentsource.go`:
- Around line 121-151: The function ReconcileImageTagMirrors currently builds
ImageTagMirrors from hcp.Spec.ImageContentSources which converts digest-focused
config into tag-mirror entries; change it so ITMS is populated only from
explicit ITMS config by removing or disabling the loop that iterates
hcp.Spec.ImageContentSources and only appending
additionalITMS.Spec.ImageTagMirrors (if additionalITMS != nil) into
itms.Spec.ImageTagMirrors; keep the existing label setting and ensure
itms.Spec.ImageTagMirrors is initialized to an empty slice before merging
additionalITMS.
🧹 Nitpick comments (2)
support/globalconfig/imagecontentsource_test.go (1)
504-618: Consider adding ITMS-specific edge cases for completeness.The test covers IDMS-specific scenarios ("Only IDMS", "Invalid IDMS YAML") but lacks the corresponding ITMS cases. For symmetry and complete coverage:
💡 Suggested additional test cases
{ name: "Only ITMS", configMapData: map[string]string{ "itms.yaml": `apiVersion: config.openshift.io/v1 kind: ImageTagMirrorSet metadata: name: test-itms spec: imageTagMirrors: - source: registry.example.com/tags mirrors: - mirror.example.com/tags`, }, expectedIDMS: nil, expectedITMS: &configv1.ImageTagMirrorSet{ // ... populate expected fields }, expectError: false, }, { name: "Invalid ITMS YAML", configMapData: map[string]string{ "itms.yaml": `invalid yaml content {{{`, }, expectError: true, },control-plane-operator/controllers/hostedcontrolplane/ignition/reconcile.go (1)
62-88: Consider adding nil checks and reducing code duplication.Two observations:
Missing nil checks: If
imageDigestMirrorSetorimageTagMirrorSetis nil,yamlSerializer.Encodewill fail. Consider adding defensive nil checks or documenting that callers must provide non-nil values.Code duplication: The scheme setup (lines 63-74) duplicates
reconcileImageContentTypeIgnitionConfigMap(lines 126-137). Consider extracting to a helper.♻️ Suggested refactor to extract scheme setup
+func newImageContentSchemeAndSerializer() (*jsonserializer.Serializer, error) { + scheme := runtime.NewScheme() + if err := operatorv1alpha1.Install(scheme); err != nil { + return nil, err + } + if err := configv1.Install(scheme); err != nil { + return nil, err + } + return jsonserializer.NewSerializerWithOptions( + jsonserializer.DefaultMetaFactory, scheme, scheme, + jsonserializer.SerializerOptions{Yaml: true, Pretty: true, Strict: true}), nil +}Then use this helper in both functions.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/content/reference/image-registry-mirrors.md`:
- Around line 68-97: Convert the fenced YAML examples (the blocks that start
with "apiVersion: v1 / kind: ConfigMap" and the subsequent "apiVersion:
hypershift.openshift.io/v1beta1 / kind: HostedCluster", the ConfigMap/NodePool
pair with "nodepool-tag-mirrors", and the "unified-mirrors" ConfigMap) from
fenced triple-backtick style to the indented code-block style used earlier in
the file: replace each fenced block with an indented block (prefix lines with
four spaces) preserving the exact YAML content and document separators (---) so
the examples for ConfigMap, HostedCluster, NodePool, and unified-mirrors match
the surrounding indentation and avoid MD046 lint failures.
|
The close was a mistake. |
|
@mariusbertram: This pull request references CNTRLPLANE-1961 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mariusbertram 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docs/content/reference/aggregated-docs.md`:
- Around line 35471-35476: The imageMirrorConfigRef documentation is missing key
details and must be aligned with the other occurrences (e.g., in
HostedClusterSpec/HostedControlPlaneSpec) — update the imageMirrorConfigRef
paragraph to explicitly state merge behavior with ImageContentSources, that the
ConfigMap must include idms.yaml and/or itms.yaml keys (containing
ImageDigestMirrorSet and ImageTagMirrorSet respectively), and note the rollout
trigger implications for NodePools when the ConfigMap changes; ensure the
wording for imageMirrorConfigRef in this section matches the descriptions used
for imageMirrorConfigRef in the other two entries (or clearly document any
contextual differences between HostedClusterSpec and HostedControlPlaneSpec).
In `@docs/content/reference/image-registry-mirrors.md`:
- Around line 147-205: The markdown table separator rows under the "Comparison:
Cluster-wide vs Per-NodePool" and "Comparison: IDMS vs ITMS" headings use
compact separators (e.g., |--------|) causing MD060; update each separator row
to include single-space padding inside each pipe (e.g., | --- | --- |) so they
match the header cell padding and satisfy markdownlint; target the separator
lines directly in those two tables (the row immediately beneath each header row)
and make the spacing consistent across all columns.
In `@support/globalconfig/imagecontentsource_test.go`:
- Around line 504-660: The fake client in TestParseImageMirrorConfigMap is
created without registering the corev1 scheme so ConfigMap objects may not be
handled correctly; update the test to register the corev1 scheme on the fake
client builder (i.e., call WithScheme(...) like other tests) before invoking
fake.NewClientBuilder().WithObjects(...) so that corev1.ConfigMap objects used
by ParseImageMirrorConfigMap are recognized; locate this in the test function
TestParseImageMirrorConfigMap where fake.NewClientBuilder() is used and add the
same scheme registration used elsewhere in the file.
🧹 Nitpick comments (2)
support/globalconfig/imagecontentsource.go (2)
121-123: Update the ReconcileImageTagMirrors docstring to match behavior.
Line 121 says it reconciles HCP imageContentSources, but the function now only merges explicit ITMS config, which is misleading for future maintainers.✍️ Suggested doc update
-// ReconcileImageTagMirrors reconciles the ImageContentSources from the HCP spec into an ImageTagMirrorSet -// and optionally merges in additional ITMS configuration from a parsed ConfigMap +// ReconcileImageTagMirrors reconciles an ImageTagMirrorSet from explicit ITMS configuration +// and optionally merges in additional ITMS configuration from a parsed ConfigMap
154-167: Validate parsed YAML kind/version to avoid silent misconfigurations.
Right now, any YAML with a compatible shape will unmarshal, even ifkind/apiVersionare wrong. Explicit checks make failures immediate and easier to debug.🛡️ Suggested validation
if idmsYAML, ok := configMap.Data["idms.yaml"]; ok && idmsYAML != "" { idms = &configv1.ImageDigestMirrorSet{} if err := yaml.Unmarshal([]byte(idmsYAML), idms); err != nil { return nil, nil, fmt.Errorf("failed to parse idms.yaml from ConfigMap: %w", err) } + if idms.Kind != "" && idms.Kind != "ImageDigestMirrorSet" { + return nil, nil, fmt.Errorf("idms.yaml kind must be ImageDigestMirrorSet, got %q", idms.Kind) + } + if idms.APIVersion != "" && idms.APIVersion != configv1.GroupVersion.String() { + return nil, nil, fmt.Errorf("idms.yaml apiVersion must be %q, got %q", configv1.GroupVersion.String(), idms.APIVersion) + } } // Parse ITMS if present if itmsYAML, ok := configMap.Data["itms.yaml"]; ok && itmsYAML != "" { itms = &configv1.ImageTagMirrorSet{} if err := yaml.Unmarshal([]byte(itmsYAML), itms); err != nil { return nil, nil, fmt.Errorf("failed to parse itms.yaml from ConfigMap: %w", err) } + if itms.Kind != "" && itms.Kind != "ImageTagMirrorSet" { + return nil, nil, fmt.Errorf("itms.yaml kind must be ImageTagMirrorSet, got %q", itms.Kind) + } + if itms.APIVersion != "" && itms.APIVersion != configv1.GroupVersion.String() { + return nil, nil, fmt.Errorf("itms.yaml apiVersion must be %q, got %q", configv1.GroupVersion.String(), itms.APIVersion) + } }
| #### Comparison: Cluster-wide vs Per-NodePool | ||
|
|
||
| | Aspect | imageMirrorConfigRef | NodePool.spec.config | | ||
| |--------|---------------------|---------------------| | ||
| | **Scope** | All NodePools in cluster | Single NodePool only | | ||
| | **Configuration location** | HostedCluster.spec | NodePool.spec | | ||
| | **ConfigMap key** | `itms.yaml` (and optionally `idms.yaml`) | `config` | | ||
| | **Reconciliation** | HostedControlPlane controller | NodePool controller | | ||
| | **Rollout trigger** | Automatic for all NodePools | Only for configured NodePool | | ||
| | **Use case** | Fleet-wide policies, disconnected environments | NodePool-specific requirements | | ||
| | **GitOps-friendly** | ✅ Highly (single source of truth) | ⚠️ Requires per-NodePool config | | ||
|
|
||
| ### Unified Configuration (IDMS + ITMS) | ||
|
|
||
| For comprehensive mirror coverage, use `HostedCluster.spec.imageMirrorConfigRef` with both IDMS and ITMS in a single ConfigMap: | ||
|
|
||
| apiVersion: v1 | ||
| kind: ConfigMap | ||
| metadata: | ||
| name: unified-mirrors | ||
| namespace: clusters | ||
| data: | ||
| idms.yaml: | | ||
| apiVersion: config.openshift.io/v1 | ||
| kind: ImageDigestMirrorSet | ||
| metadata: | ||
| name: cluster-digest-mirrors | ||
| spec: | ||
| imageDigestMirrors: | ||
| - source: registry.redhat.io | ||
| mirrors: | ||
| - mirror.corp.com/redhat | ||
| itms.yaml: | | ||
| apiVersion: config.openshift.io/v1 | ||
| kind: ImageTagMirrorSet | ||
| metadata: | ||
| name: cluster-tag-mirrors | ||
| spec: | ||
| imageTagMirrors: | ||
| - source: quay.io/openshift-release-dev | ||
| mirrors: | ||
| - mirror.corp.com/openshift-release-dev | ||
|
|
||
| This approach: | ||
| - Provides single source of truth for all mirror configurations | ||
| - Automatically merges with `imageContentSources` if both are specified | ||
| - Simplifies fleet management across hundreds of clusters | ||
| - Ideal for GitOps workflows and disconnected environments | ||
|
|
||
| ## Comparison: IDMS vs ITMS | ||
|
|
||
| | Aspect | IDMS | ITMS | | ||
| |--------|------|------| | ||
| | **Auto-creation** | ✅ Yes (from imageContentSources) | ❌ No (explicit config required) | | ||
| | **Cluster-wide config** | ✅ imageMirrorConfigRef or imageContentSources | ✅ imageMirrorConfigRef | | ||
| | **Per-NodePool config** | ✅ Yes (via NodePool.spec.config) | ✅ Yes (via NodePool.spec.config) | | ||
| | **Management cluster detection** | ✅ Yes | ✅ Yes | | ||
| | **Image reference type** | Digest (@sha256:...) | Tag (:latest, :v1.0) | | ||
| | **API version** | config.openshift.io/v1 | config.openshift.io/v1 | |
There was a problem hiding this comment.
Fix table separator spacing to satisfy MD060. The separator rows are compact while headers are padded; this can trigger markdownlint if enabled.
🛠️ Suggested fix
-|--------|---------------------|---------------------|
+| -------- | --------------------- | --------------------- |
@@
-|--------|------|------|
+| ------ | ---- | ---- |🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
150-150: Table column style
Table pipe is missing space to the right for style "compact"
(MD060, table-column-style)
150-150: Table column style
Table pipe is missing space to the left for style "compact"
(MD060, table-column-style)
150-150: Table column style
Table pipe is missing space to the right for style "compact"
(MD060, table-column-style)
150-150: Table column style
Table pipe is missing space to the left for style "compact"
(MD060, table-column-style)
150-150: Table column style
Table pipe is missing space to the right for style "compact"
(MD060, table-column-style)
150-150: Table column style
Table pipe is missing space to the left for style "compact"
(MD060, table-column-style)
199-199: Table column style
Table pipe is missing space to the right for style "compact"
(MD060, table-column-style)
199-199: Table column style
Table pipe is missing space to the left for style "compact"
(MD060, table-column-style)
199-199: Table column style
Table pipe is missing space to the right for style "compact"
(MD060, table-column-style)
199-199: Table column style
Table pipe is missing space to the left for style "compact"
(MD060, table-column-style)
199-199: Table column style
Table pipe is missing space to the right for style "compact"
(MD060, table-column-style)
199-199: Table column style
Table pipe is missing space to the left for style "compact"
(MD060, table-column-style)
🤖 Prompt for AI Agents
In `@docs/content/reference/image-registry-mirrors.md` around lines 147 - 205, The
markdown table separator rows under the "Comparison: Cluster-wide vs
Per-NodePool" and "Comparison: IDMS vs ITMS" headings use compact separators
(e.g., |--------|) causing MD060; update each separator row to include
single-space padding inside each pipe (e.g., | --- | --- |) so they match the
header cell padding and satisfy markdownlint; target the separator lines
directly in those two tables (the row immediately beneath each header row) and
make the spacing consistent across all columns.
| func TestParseImageMirrorConfigMap(t *testing.T) { | ||
| g := NewGomegaWithT(t) | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| configMapData map[string]string | ||
| expectedIDMS *configv1.ImageDigestMirrorSet | ||
| expectedITMS *configv1.ImageTagMirrorSet | ||
| expectError bool | ||
| configMapMissing bool | ||
| }{ | ||
| { | ||
| name: "Valid IDMS and ITMS", | ||
| configMapData: map[string]string{ | ||
| "idms.yaml": `apiVersion: config.openshift.io/v1 | ||
| kind: ImageDigestMirrorSet | ||
| metadata: | ||
| name: test-idms | ||
| spec: | ||
| imageDigestMirrors: | ||
| - source: registry.example.com/repo | ||
| mirrors: | ||
| - mirror.example.com/repo`, | ||
| "itms.yaml": `apiVersion: config.openshift.io/v1 | ||
| kind: ImageTagMirrorSet | ||
| metadata: | ||
| name: test-itms | ||
| spec: | ||
| imageTagMirrors: | ||
| - source: registry.example.com/tags | ||
| mirrors: | ||
| - mirror.example.com/tags`, | ||
| }, | ||
| expectedIDMS: &configv1.ImageDigestMirrorSet{ | ||
| TypeMeta: metav1.TypeMeta{ | ||
| Kind: "ImageDigestMirrorSet", | ||
| APIVersion: "config.openshift.io/v1", | ||
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "test-idms", | ||
| }, | ||
| Spec: configv1.ImageDigestMirrorSetSpec{ | ||
| ImageDigestMirrors: []configv1.ImageDigestMirrors{ | ||
| { | ||
| Source: "registry.example.com/repo", | ||
| Mirrors: []configv1.ImageMirror{"mirror.example.com/repo"}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| expectedITMS: &configv1.ImageTagMirrorSet{ | ||
| TypeMeta: metav1.TypeMeta{ | ||
| Kind: "ImageTagMirrorSet", | ||
| APIVersion: "config.openshift.io/v1", | ||
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "test-itms", | ||
| }, | ||
| Spec: configv1.ImageTagMirrorSetSpec{ | ||
| ImageTagMirrors: []configv1.ImageTagMirrors{ | ||
| { | ||
| Source: "registry.example.com/tags", | ||
| Mirrors: []configv1.ImageMirror{"mirror.example.com/tags"}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| expectError: false, | ||
| }, | ||
| { | ||
| name: "Only IDMS", | ||
| configMapData: map[string]string{ | ||
| "idms.yaml": `apiVersion: config.openshift.io/v1 | ||
| kind: ImageDigestMirrorSet | ||
| metadata: | ||
| name: test-idms | ||
| spec: | ||
| imageDigestMirrors: | ||
| - source: registry.example.com/repo | ||
| mirrors: | ||
| - mirror.example.com/repo`, | ||
| }, | ||
| expectedIDMS: &configv1.ImageDigestMirrorSet{ | ||
| TypeMeta: metav1.TypeMeta{ | ||
| Kind: "ImageDigestMirrorSet", | ||
| APIVersion: "config.openshift.io/v1", | ||
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "test-idms", | ||
| }, | ||
| Spec: configv1.ImageDigestMirrorSetSpec{ | ||
| ImageDigestMirrors: []configv1.ImageDigestMirrors{ | ||
| { | ||
| Source: "registry.example.com/repo", | ||
| Mirrors: []configv1.ImageMirror{"mirror.example.com/repo"}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| expectedITMS: nil, | ||
| expectError: false, | ||
| }, | ||
| { | ||
| name: "Missing ConfigMap", | ||
| configMapMissing: true, | ||
| expectError: true, | ||
| }, | ||
| { | ||
| name: "Invalid IDMS YAML", | ||
| configMapData: map[string]string{ | ||
| "idms.yaml": `invalid yaml content {{{`, | ||
| }, | ||
| expectError: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| var objects []client.Object | ||
| configMapRef := &corev1.LocalObjectReference{Name: "test-configmap"} | ||
|
|
||
| if !tc.configMapMissing { | ||
| cm := &corev1.ConfigMap{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "test-configmap", | ||
| Namespace: "test-namespace", | ||
| }, | ||
| Data: tc.configMapData, | ||
| } | ||
| objects = append(objects, cm) | ||
| } | ||
|
|
||
| client := fake.NewClientBuilder().WithObjects(objects...).Build() | ||
| ctx := context.Background() | ||
|
|
||
| idms, itms, err := ParseImageMirrorConfigMap(ctx, client, configMapRef, "test-namespace") | ||
|
|
||
| if tc.expectError { | ||
| g.Expect(err).To(HaveOccurred()) | ||
| } else { | ||
| g.Expect(err).ToNot(HaveOccurred()) | ||
| if tc.expectedIDMS != nil { | ||
| g.Expect(idms).ToNot(BeNil()) | ||
| g.Expect(idms.Spec.ImageDigestMirrors).To(Equal(tc.expectedIDMS.Spec.ImageDigestMirrors)) | ||
| } else { | ||
| g.Expect(idms).To(BeNil()) | ||
| } | ||
| if tc.expectedITMS != nil { | ||
| g.Expect(itms).ToNot(BeNil()) | ||
| g.Expect(itms.Spec.ImageTagMirrors).To(Equal(tc.expectedITMS.Spec.ImageTagMirrors)) | ||
| } else { | ||
| g.Expect(itms).To(BeNil()) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n support/globalconfig/imagecontentsource_test.go | sed -n '630,645p'Repository: openshift/hypershift
Length of output: 597
🏁 Script executed:
# Check if there are imports related to scheme handling in the file
head -100 support/globalconfig/imagecontentsource_test.go | grep -E "import|scheme|corev1"Repository: openshift/hypershift
Length of output: 101
🏁 Script executed:
# Search for other usages of fake.NewClientBuilder in the codebase to see patterns
rg "fake\.NewClientBuilder" -A 3 -B 1 --max-count 10Repository: openshift/hypershift
Length of output: 50377
🏁 Script executed:
# Check the controller-runtime documentation by looking at how it's used elsewhere
rg "corev1\.AddToScheme" -B 2 -A 2 --max-count 5Repository: openshift/hypershift
Length of output: 25282
🏁 Script executed:
# Check if the test currently uses a scheme elsewhere in the function
sed -n '504,660p' support/globalconfig/imagecontentsource_test.go | grep -i "scheme"Repository: openshift/hypershift
Length of output: 46
🏁 Script executed:
# Look at how other tests in the same file handle fake client creation
grep -n "fake.NewClientBuilder" support/globalconfig/imagecontentsource_test.goRepository: openshift/hypershift
Length of output: 322
Add scheme registration to fake client for proper ConfigMap handling.
The fake client at line 636 is created without registering corev1, which is required for reliable ConfigMap storage and retrieval. Other tests in this same file use .WithScheme() when working with ConfigMap objects.
🔧 Suggested fix
- client := fake.NewClientBuilder().WithObjects(objects...).Build()
+ testScheme := runtime.NewScheme()
+ _ = corev1.AddToScheme(testScheme)
+ client := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(objects...).Build()
ctx := context.Background()🤖 Prompt for AI Agents
In `@support/globalconfig/imagecontentsource_test.go` around lines 504 - 660, The
fake client in TestParseImageMirrorConfigMap is created without registering the
corev1 scheme so ConfigMap objects may not be handled correctly; update the test
to register the corev1 scheme on the fake client builder (i.e., call
WithScheme(...) like other tests) before invoking
fake.NewClientBuilder().WithObjects(...) so that corev1.ConfigMap objects used
by ParseImageMirrorConfigMap are recognized; locate this in the test function
TestParseImageMirrorConfigMap where fake.NewClientBuilder() is used and add the
same scheme registration used elsewhere in the file.
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
…ation Implement support for HostedCluster.spec.imageMirrorConfigRef to enable unified ImageDigestMirrorSet (IDMS) and ImageTagMirrorSet (ITMS) configuration from a single ConfigMap reference. This provides a cluster-wide, GitOps-friendly approach for managing image registry mirrors that applies to all NodePools automatically. API changes: - Add imageMirrorConfigRef field to HostedCluster and HostedControlPlane specs - Add ParseImageMirrorConfigMap() to parse ConfigMap with idms.yaml/itms.yaml keys - Update ReconcileImageDigestMirrors() and ReconcileImageTagMirrors() to merge additional mirrors from ConfigMap Controller changes: - HostedCluster controller copies imageMirrorConfigRef to HostedControlPlane - HostedControlPlane controller parses and reconciles IDMS/ITMS from ConfigMap - Add ReconcileImageSourceMirrorsIgnitionConfigFromIDMSAndITMS() for combined configs - Update globalConfigString() to include imageMirrorConfigRef in rollout hash - Update HasImageMirrorPolicies check to include imageMirrorConfigRef - Update getCoreConfigs() to expect additional config when imageMirrorConfigRef is set Testing: - Add TestParseImageMirrorConfigMap with 4 test cases - Add TestReconcileImageDigestMirrorsWithAdditional - Add TestReconcileImageTagMirrorsWithAdditional - Fix loop variable pointer bug in existing tests Documentation: - Add comprehensive how-to guide for configure-image-registry-mirrors.md - Update reference docs to cover both cluster-wide and per-NodePool approaches - Convert indented code blocks to fenced blocks for syntax highlighting - Document unified configuration pattern with both IDMS and ITMS This implementation maintains backward compatibility with existing imageContentSources and per-NodePool configurations while providing a more scalable approach for fleet management.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/content/how-to/configure-image-registry-mirrors.md`:
- Around line 371-372: Update the best-practice guidance to recommend the
unified imageMirrorConfigRef approach as the primary pattern for fleet
management, mentioning imageMirrorConfigRef explicitly and noting
imageContentSources only as a legacy/backwards-compatible alternative; keep the
ITMS/Tag-based mirror note (ITMS) as a secondary instruction when tag-based
mirrors are required and reference IDMS when explaining imageContentSources for
legacy cases.
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/ignition/reconcile.go (1)
62-87: Add a nil guard for IDMS/ITMS inputs.
This avoids a potential panic if a caller ever passes a nil mirror set.Proposed defensive check
func ReconcileImageSourceMirrorsIgnitionConfigFromIDMSAndITMS(cm *corev1.ConfigMap, ownerRef config.OwnerRef, imageDigestMirrorSet *configv1.ImageDigestMirrorSet, imageTagMirrorSet *configv1.ImageTagMirrorSet) error { + if imageDigestMirrorSet == nil || imageTagMirrorSet == nil { + return fmt.Errorf("imageDigestMirrorSet and imageTagMirrorSet must be non-nil") + } scheme := runtime.NewScheme() err := operatorv1alpha1.Install(scheme)
| 1. **Use IDMS for most cases**: Configure via `imageContentSources` for automatic IDMS creation | ||
| 2. **Add ITMS when needed**: Only add ITMS ConfigMaps if you specifically need tag-based mirrors |
There was a problem hiding this comment.
Align best practice with the recommended unified approach.
Best practice #1 recommends configuring IDMS via imageContentSources, but line 19 and the section at lines 22-104 recommend the unified imageMirrorConfigRef approach for fleet management. This creates contradictory guidance.
Update the best practice to recommend the unified approach as the primary pattern, with imageContentSources as a legacy option for backward compatibility.
✍️ Suggested revision
-1. **Use IDMS for most cases**: Configure via `imageContentSources` for automatic IDMS creation
-2. **Add ITMS when needed**: Only add ITMS ConfigMaps if you specifically need tag-based mirrors
+1. **Prefer unified configuration**: Use `imageMirrorConfigRef` with a ConfigMap containing both IDMS and ITMS for GitOps-friendly fleet management
+2. **Use legacy approaches for compatibility**: Configure IDMS via `imageContentSources` or ITMS via NodePool ConfigMaps only when required for backward compatibility📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 1. **Use IDMS for most cases**: Configure via `imageContentSources` for automatic IDMS creation | |
| 2. **Add ITMS when needed**: Only add ITMS ConfigMaps if you specifically need tag-based mirrors | |
| 1. **Prefer unified configuration**: Use `imageMirrorConfigRef` with a ConfigMap containing both IDMS and ITMS for GitOps-friendly fleet management | |
| 2. **Use legacy approaches for compatibility**: Configure IDMS via `imageContentSources` or ITMS via NodePool ConfigMaps only when required for backward compatibility |
🤖 Prompt for AI Agents
In `@docs/content/how-to/configure-image-registry-mirrors.md` around lines 371 -
372, Update the best-practice guidance to recommend the unified
imageMirrorConfigRef approach as the primary pattern for fleet management,
mentioning imageMirrorConfigRef explicitly and noting imageContentSources only
as a legacy/backwards-compatible alternative; keep the ITMS/Tag-based mirror
note (ITMS) as a secondary instruction when tag-based mirrors are required and
reference IDMS when explaining imageContentSources for legacy cases.
4 participants
What this PR does / why we need it:
This PR Allows to set ImageTagMirrorSets to the config of nodepools.
Which issue(s) this PR fixes:
Fixes #5608
Special notes for your reviewer:
Checklist: