CNTRLPLANE-1961: feat(nodepool )Add support for ImageTagMirrorSet config#7566
Open
mariusbertram wants to merge 1 commit intoopenshift:mainfrom
Open
CNTRLPLANE-1961: feat(nodepool )Add support for ImageTagMirrorSet config#7566mariusbertram wants to merge 1 commit intoopenshift:mainfrom
mariusbertram wants to merge 1 commit intoopenshift:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (27)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis change adds support for Sequence DiagramsequenceDiagram
participant User
participant HostedCluster
participant HostedCluster Controller
participant ConfigMap
participant Config Parser
participant IDMS/ITMS Reconciler
participant Ignition Generator
participant Guest Cluster
User->>HostedCluster: Create with imageMirrorConfigRef
HostedCluster Controller->>ConfigMap: Read referenced ConfigMap
ConfigMap-->>HostedCluster Controller: Return YAML content (idms.yaml, itms.yaml)
HostedCluster Controller->>Config Parser: Parse IDMS + ITMS from ConfigMap
Config Parser->>Config Parser: Unmarshal YAML into typed objects
Config Parser-->>HostedCluster Controller: Return ImageDigestMirrorSet + ImageTagMirrorSet
HostedCluster Controller->>IDMS/ITMS Reconciler: Reconcile IDMS with base imageContentSources
IDMS/ITMS Reconciler->>IDMS/ITMS Reconciler: Merge imageContentSources + additional IDMS
HostedCluster Controller->>IDMS/ITMS Reconciler: Reconcile ITMS (when present)
IDMS/ITMS Reconciler->>IDMS/ITMS Reconciler: Apply worker role labels
HostedCluster Controller->>Ignition Generator: Generate ignition config from IDMS + ITMS
Ignition Generator->>Ignition Generator: Serialize IDMS and ITMS to YAML
Ignition Generator-->>HostedCluster Controller: Return combined ignition config
HostedCluster Controller->>Guest Cluster: Create/update IDMS, ITMS, and ignition ConfigMap
Guest Cluster-->>Guest Cluster: Worker nodes apply registries.conf from ignition
🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
openshift-ci
Bot
added
do-not-merge/work-in-progress
Contributor
|
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}
Contributor
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] + }
Contributor
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.
Author
|
@coderabbitai help |
Contributor
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Contributor
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.
Contributor
|
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. |
Author
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
|
|
@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. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml (1)
2535-2537: Use JSON field names in the user-facing description.On Line 2535-Line 2537, the text uses
ImageContentSourcesandImageMirrorConfigRef. UsingimageContentSourcesandimageMirrorConfigRefwould better match what users write in YAML.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml` around lines 2535 - 2537, The user-facing description uses Go-style field names `ImageContentSources` and `ImageMirrorConfigRef`; change those to the JSON/YAML field names `imageContentSources` and `imageMirrorConfigRef` in the description text so it matches what users write in manifests (update the lines referencing ImageContentSources and ImageMirrorConfigRef accordingly).api/hypershift/v1beta1/hostedcluster_types.go (1)
752-765: Consider aligning documentation betweenHostedClusterSpecandHostedControlPlaneSpec.The
ImageMirrorConfigRefdocumentation inHostedClusterSpecis more comprehensive than the corresponding field inHostedControlPlaneSpec(inhosted_controlplane.go). While users primarily interact withHostedCluster, consider adding the merge behavior explanation toHostedControlPlaneSpecfor consistency, especially if developers or operators inspect the HCP directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/hostedcluster_types.go` around lines 752 - 765, The docs for ImageMirrorConfigRef on HostedClusterSpec explain merge behavior and rollout triggers but the same field on HostedControlPlaneSpec (hosted_controlplane.go) lacks that detail; update the comment for ImageMirrorConfigRef in the HostedControlPlaneSpec to match: mention that ConfigMap entries are merged after ImageContentSources (allowing overrides), that controllers perform the merge during reconciliation, and that changes trigger NodePool rollouts; ensure you reference the same field name ImageMirrorConfigRef and keep the +optional and +rollout annotations consistent with the HostedClusterSpec comment.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml (1)
3010-3013: Prefer CRD JSON field names in description text for clarity.Using
imageContentSourcesandimageMirrorConfigRef(JSON names) instead of Go-style names would better match what users set in YAML manifests andkubectl explain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml` around lines 3010 - 3013, The description currently uses Go-style field names "ImageContentSources" and "ImageMirrorConfigRef"; update the human-facing description to use CRD/JSON field names "imageContentSources" and "imageMirrorConfigRef" so it matches YAML manifests and kubectl explain (search for the description text that mentions ImageContentSources/ImageMirrorConfigRef and replace with the JSON names, and scan adjacent descriptions to update any other Go-style field name occurrences).control-plane-operator/controllers/hostedcontrolplane/ignition/reconcile.go (1)
63-75: Extract shared image-content YAML serializer construction to one helper.Line 63-Line 75 duplicates Line 126-Line 137. Centralizing this avoids drift in scheme registration/serializer options.
Refactor sketch
+func newImageContentYAMLSerializer() (*runtime.Scheme, *jsonserializer.Serializer, error) { + scheme := runtime.NewScheme() + if err := operatorv1alpha1.Install(scheme); err != nil { + return nil, nil, err + } + if err := configv1.Install(scheme); err != nil { + return nil, nil, err + } + serializer := jsonserializer.NewSerializerWithOptions( + jsonserializer.DefaultMetaFactory, scheme, scheme, + jsonserializer.SerializerOptions{Yaml: true, Pretty: true, Strict: true}, + ) + return scheme, serializer, nil +} + func ReconcileImageSourceMirrorsIgnitionConfigFromIDMSAndITMS(...) error { - scheme := runtime.NewScheme() - err := operatorv1alpha1.Install(scheme) - ... - yamlSerializer := jsonserializer.NewSerializerWithOptions(...) + _, yamlSerializer, err := newImageContentYAMLSerializer() + if err != nil { + return err + } ... } func reconcileImageContentTypeIgnitionConfigMap(...) error { - scheme := runtime.NewScheme() - err := operatorv1alpha1.Install(scheme) - ... - yamlSerializer := jsonserializer.NewSerializerWithOptions(...) + _, yamlSerializer, err := newImageContentYAMLSerializer() + if err != nil { + return err + } ... }Also applies to: 125-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/ignition/reconcile.go` around lines 63 - 75, Duplicate construction of the image-content YAML serializer (runtime.NewScheme + operatorv1alpha1.Install + configv1.Install + jsonserializer.NewSerializerWithOptions) should be extracted into a single helper function; create a helper (e.g., newImageContentYAMLSerializer) that builds a scheme, calls operatorv1alpha1.Install(scheme) and configv1.Install(scheme), then returns the serializer created by jsonserializer.NewSerializerWithOptions using the same SerializerOptions {Yaml:true, Pretty:true, Strict:true}, and replace both inline blocks that assign yamlSerializer with a call to this helper so scheme registration and serializer options remain identical.cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yaml (1)
3106-3125: Enforce non-emptyimageMirrorConfigRef.namewhen the ref is setRight now,
imageMirrorConfigRefcan be provided as an empty object ({}) becausenameis optional with default""(Line 3115 onward). That permits an invalid reconcile target and pushes validation failure to runtime.Proposed schema validation
imageMirrorConfigRef: description: |- imageMirrorConfigRef is a local reference to a ConfigMap containing both ImageDigestMirrorSet (IDMS) and ImageTagMirrorSet (ITMS) configurations. The ConfigMap should contain keys "idms.yaml" and/or "itms.yaml" with the respective mirror configurations. This provides a unified and GitOps-friendly way to manage image mirror configurations for both digest-based and tag-based mirrors. When set, the controllers will parse the ConfigMap and reconcile both IDMS and ITMS configurations. properties: name: default: "" description: |- Name of the referent. This field is effectively required, but due to backwards compatibility is allowed to be empty. Instances of this type with an empty value here are almost certainly wrong. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names type: string type: object x-kubernetes-map-type: atomic + x-kubernetes-validations: + - message: imageMirrorConfigRef.name must not be empty when imageMirrorConfigRef is set + rule: self.name != ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yaml` around lines 3106 - 3125, The schema currently allows imageMirrorConfigRef to be an empty object because imageMirrorConfigRef.name has a default of "" and isn't required; change the CRD schema for imageMirrorConfigRef so that name is mandatory and cannot be empty by removing the default "" and adding a required constraint (e.g., add required: ["name"] under imageMirrorConfigRef or set a non-empty validation like minLength: 1 or a non-empty pattern on the property). Update the imageMirrorConfigRef.name property (notably the symbol imageMirrorConfigRef.name) and keep x-kubernetes-map-type: atomic; ensure validators and defaults are adjusted so an empty object is rejected at validation time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml`:
- Around line 2542-2550: The CRD currently permits imageMirrorConfigRef: {} or
imageMirrorConfigRef.name = ""; update the AutoNodeKarpenter CRD schema so that
the imageMirrorConfigRef object requires a name and that name is non-empty: add
required: ["name"] to the imageMirrorConfigRef schema and add a minLength: 1 (or
an equivalent non-empty string validation) to the imageMirrorConfigRef.name
property so empty objects or empty strings are rejected at API validation.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yaml`:
- Around line 2412-2431: The schema currently allows imageMirrorConfigRef: {} or
name: "" which can't resolve a ConfigMap; update the imageMirrorConfigRef schema
by removing the default: "" on the name property, add validation to require a
non-empty string (e.g., add minLength: 1 to the name property) and mark the name
as required for the imageMirrorConfigRef object (add required: ["name"]) so that
an empty object is rejected; reference the imageMirrorConfigRef.name property in
the AAA_ungated.yaml fragment and ensure the changes prevent
imageMirrorConfigRef from being provided without a resolvable name.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yaml`:
- Around line 3351-3361: The schema currently defaults imageMirrorConfigRef.name
to "" which allows an empty reference; update the CRD schema for the field
imageMirrorConfigRef.name to disallow empty strings by removing the default ""
and adding a validation constraint such as minLength: 1 (or an equivalent
non-empty pattern) so that if imageMirrorConfigRef is provided its name must be
non-empty; locate the imageMirrorConfigRef.name definition in the CRD (the
property block with description "Name of the referent.") and apply these
changes.
---
Nitpick comments:
In `@api/hypershift/v1beta1/hostedcluster_types.go`:
- Around line 752-765: The docs for ImageMirrorConfigRef on HostedClusterSpec
explain merge behavior and rollout triggers but the same field on
HostedControlPlaneSpec (hosted_controlplane.go) lacks that detail; update the
comment for ImageMirrorConfigRef in the HostedControlPlaneSpec to match: mention
that ConfigMap entries are merged after ImageContentSources (allowing
overrides), that controllers perform the merge during reconciliation, and that
changes trigger NodePool rollouts; ensure you reference the same field name
ImageMirrorConfigRef and keep the +optional and +rollout annotations consistent
with the HostedClusterSpec comment.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml`:
- Around line 2535-2537: The user-facing description uses Go-style field names
`ImageContentSources` and `ImageMirrorConfigRef`; change those to the JSON/YAML
field names `imageContentSources` and `imageMirrorConfigRef` in the description
text so it matches what users write in manifests (update the lines referencing
ImageContentSources and ImageMirrorConfigRef accordingly).
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml`:
- Around line 3010-3013: The description currently uses Go-style field names
"ImageContentSources" and "ImageMirrorConfigRef"; update the human-facing
description to use CRD/JSON field names "imageContentSources" and
"imageMirrorConfigRef" so it matches YAML manifests and kubectl explain (search
for the description text that mentions ImageContentSources/ImageMirrorConfigRef
and replace with the JSON names, and scan adjacent descriptions to update any
other Go-style field name occurrences).
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yaml`:
- Around line 3106-3125: The schema currently allows imageMirrorConfigRef to be
an empty object because imageMirrorConfigRef.name has a default of "" and isn't
required; change the CRD schema for imageMirrorConfigRef so that name is
mandatory and cannot be empty by removing the default "" and adding a required
constraint (e.g., add required: ["name"] under imageMirrorConfigRef or set a
non-empty validation like minLength: 1 or a non-empty pattern on the property).
Update the imageMirrorConfigRef.name property (notably the symbol
imageMirrorConfigRef.name) and keep x-kubernetes-map-type: atomic; ensure
validators and defaults are adjusted so an empty object is rejected at
validation time.
In `@control-plane-operator/controllers/hostedcontrolplane/ignition/reconcile.go`:
- Around line 63-75: Duplicate construction of the image-content YAML serializer
(runtime.NewScheme + operatorv1alpha1.Install + configv1.Install +
jsonserializer.NewSerializerWithOptions) should be extracted into a single
helper function; create a helper (e.g., newImageContentYAMLSerializer) that
builds a scheme, calls operatorv1alpha1.Install(scheme) and
configv1.Install(scheme), then returns the serializer created by
jsonserializer.NewSerializerWithOptions using the same SerializerOptions
{Yaml:true, Pretty:true, Strict:true}, and replace both inline blocks that
assign yamlSerializer with a call to this helper so scheme registration and
serializer options remain identical.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (39)
api/hypershift/v1beta1/hosted_controlplane.goapi/hypershift/v1beta1/hostedcluster_types.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/NetworkDiagnosticsConfig.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/NetworkDiagnosticsConfig.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlclient/applyconfiguration/hypershift/v1beta1/hostedclusterspec.goclient/applyconfiguration/hypershift/v1beta1/hostedcontrolplanespec.gocmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlcontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/ignition/params.gocontrol-plane-operator/controllers/hostedcontrolplane/ignition/reconcile.go
🚧 Files skipped from review as they are similar to previous changes (14)
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml
- client/applyconfiguration/hypershift/v1beta1/hostedclusterspec.go
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yaml
- cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yaml
- cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yaml
|
@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. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
docs/content/reference/aggregated-docs.md (1)
45214-45214:⚠️ Potential issue | 🟡 MinorICSP removal timeline remains incorrect.
Line 45214 states ICSP "will be removed in OpenShift 4.17," which contradicts the official documentation. Per past review verification, ICSP is "deprecated in OpenShift 4.17 and will be removed in a future release" (no specific removal version).
This issue was previously flagged and marked as addressed, but the incorrect wording persists in the current code.
📝 Suggested correction
-ImageContentSourcePolicy (ICSP) is deprecated and will be removed in OpenShift 4.17. HyperShift automatically: +ImageContentSourcePolicy (ICSP) is deprecated in OpenShift 4.17 and will be removed in a future release. HyperShift automatically:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/reference/aggregated-docs.md` at line 45214, Update the incorrect removal timeline text: find the sentence "ImageContentSourcePolicy (ICSP) is deprecated and will be removed in OpenShift 4.17." and change it to reflect the official guidance, e.g. "ImageContentSourcePolicy (ICSP) is deprecated in OpenShift 4.17 and will be removed in a future release." Ensure the revised sentence replaces the original exact phrase so the documentation accurately states deprecation in 4.17 with removal in a future release.cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yaml (1)
3348-3358:⚠️ Potential issue | 🟡 MinorReject empty
imageMirrorConfigRef.nameat the schema level.As written,
imageMirrorConfigRef: {}is valid becausenamedefaults to"". For a new field, that defers a malformed reference to reconciliation instead of failing fast at admission.🔧 Suggested schema change
imageMirrorConfigRef: properties: name: - default: "" + minLength: 1 description: |- Name of the referent. - This field is effectively required, but due to backwards compatibility is - allowed to be empty. Instances of this type with an empty value here are - almost certainly wrong. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names type: string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yaml` around lines 3348 - 3358, The schema allows imageMirrorConfigRef.name to default to "" so empty objects pass validation; update the CRD schema for the imageMirrorConfigRef property so that name is required and cannot be empty: remove the default: "" for name, add a required: ["name"] entry on the imageMirrorConfigRef object and enforce a non-empty constraint (e.g., minLength: 1 or a pattern) for imageMirrorConfigRef.name so imageMirrorConfigRef: {} is rejected at admission.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yaml (1)
2412-2431:⚠️ Potential issue | 🟡 MinorRequire a non-empty
imageMirrorConfigRef.name.This schema still admits
imageMirrorConfigRef: {}/name: "", which cannot resolve a ConfigMap and defers a bad spec to reconcile time. Please makenamerequired and non-empty in the source API markers so the generated CRD rejects it at admission.Suggested schema hardening
imageMirrorConfigRef: description: |- imageMirrorConfigRef is a local reference to a ConfigMap containing both ImageDigestMirrorSet (IDMS) and ImageTagMirrorSet (ITMS) configurations. The ConfigMap should contain keys "idms.yaml" and/or "itms.yaml" with the respective mirror configurations. This provides a unified and GitOps-friendly way to manage image mirror configurations for both digest-based and tag-based mirrors. When set, the controllers will parse the ConfigMap and reconcile both IDMS and ITMS configurations. properties: name: - default: "" description: |- Name of the referent. - This field is effectively required, but due to backwards compatibility is - allowed to be empty. Instances of this type with an empty value here are - almost certainly wrong. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + minLength: 1 type: string + required: + - name type: object x-kubernetes-map-type: atomic🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yaml` around lines 2412 - 2431, The imageMirrorConfigRef.name currently allows empty values; update the API markers on the ImageMirrorConfigRef struct's Name field to make it required and non-empty by adding validation markers (e.g. +kubebuilder:validation:Required and +kubebuilder:validation:MinLength=1) and remove "omitempty" from the json tag if present so the generated CRD enforces presence and a non-empty string for name.
🧹 Nitpick comments (6)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yaml (1)
2867-2874: Document how this combines withimageContentSources.The new field sits next to
imageContentSources, but the CRD text does not say whether the two inputs are merged, whether one overrides the other, or what happens on conflicts. Since this description is part of the user-facing API contract, please spell that out here as well sokubectl explainusers do not have to infer controller behavior from code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yaml` around lines 2867 - 2874, Update the description for imageMirrorConfigRef to explicitly state how it interacts with imageContentSources: explain whether the controller merges entries from the referenced ConfigMap with the cluster's imageContentSources, which source takes precedence on conflict (e.g., imageMirrorConfigRef overrides imageContentSources or vice versa), how duplicate entries are resolved (last-wins, namespace-scoped, or key-based deduplication), and note that both IDMS and ITMS from the ConfigMap will be reconciled together with any existing imageContentSources by the controller; reference the fields imageMirrorConfigRef and imageContentSources in the text so kubectl explain users can see the merge/override and conflict-resolution behavior clearly.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yaml (1)
2448-2455: Document how this combines withspec.imageContentSources.This CRD text introduces a second image-mirror input right next to
imageContentSources, but it never states whether the two are additive, which source wins on conflicts, or whether duplicates are deduplicated/rejected. Since this text becomes the API contract users see, please encode the merge/precedence rule in the source type comment so every generated CRD stays unambiguous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yaml` around lines 2448 - 2455, Update the imageMirrorConfigRef property description to explicitly describe how it interacts with spec.imageContentSources: state whether the entries from imageMirrorConfigRef (idms.yaml and itms.yaml) are merged with spec.imageContentSources, which source takes precedence on conflicts (e.g., imageMirrorConfigRef overrides spec.imageContentSources or vice versa), and whether duplicate entries are deduplicated or rejected; reference the specific config keys (idms.yaml, itms.yaml) and the field name spec.imageContentSources so the CRD comment unambiguously defines merge semantics, conflict resolution, and validation behavior for IDMS/ITMS vs spec.imageContentSources.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yaml (1)
2422-2428: Document merge and rollout semantics in the CRD description.The description explains the ConfigMap shape, but not the two behaviors operators need to know up front: whether this content is merged with
spec.imageContentSources, and whether changing the ref/content triggers NodePool rollouts. Please add that to the source Go doc so the generated CRD matches runtime behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yaml` around lines 2422 - 2428, Update the Go doc comment for the imageMirrorConfigRef field (the ImageMirrorConfigRef field on the HostedControlPlane API type) to explicitly state the runtime merge semantics with spec.imageContentSources (e.g., whether the ConfigMap entries are merged into, appended to, or replace spec.imageContentSources) and the rollout semantics (whether changing the ConfigMap reference or its contents will trigger NodePool rollouts and how that rollout is performed). Put this text directly above the ImageMirrorConfigRef field declaration so the generated CRD description includes the behavior, and then regenerate the CRD manifests so the updated description appears in ImageStreamImportMode.yaml.cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yaml (1)
3107-3114: Document how this combines withimageContentSources.This new public field sits next to
imageContentSources, but the description never says whether entries are merged, deduplicated, or which input wins on conflicts. That ambiguity makes overlapping mirror configs hard to reason about for callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yaml` around lines 3107 - 3114, Update the imageMirrorConfigRef description to explicitly state how it interacts with imageContentSources: explain whether entries are merged or replaced, how deduplication is performed, and which source wins on conflicts (e.g., "entries from imageMirrorConfigRef are merged with imageContentSources; duplicates are deduplicated by exact source+mirror match; imageMirrorConfigRef entries take precedence on conflict"). Mention that controllers will parse both sources and reconcile combined configuration accordingly, and include examples of keys expected ("idms.yaml"/"itms.yaml") only as supporting detail.control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go (1)
2037-2055: Design consideration: IDMS/ITMS configuration asymmetry.The implementation correctly branches between IDMS+ITMS and IDMS-only ignition config reconciliation. However, per the PR discussion, this introduces an asymmetry in configuration approaches:
- IDMS: Configured via
HostedCluster.spec.imageContentSourceswith automatic reconciliation viaReconcileImageDigestMirrors- ITMS: Configured only via
ImageMirrorConfigRefConfigMap without a direct spec fieldThe code implementation is correct, but this design divergence may impact user experience consistency. Consider documenting this difference or aligning both approaches in a future iteration as suggested by reviewer jparrill.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go` around lines 2037 - 2055, The code branches correctly between IDMS+ITMS and IDMS-only flows (see globalconfig.ReconcileImageTagMirrors and ignition.ReconcileImageSourceMirrorsIgnitionConfigFromIDMSAndITMS / ReconcileImageSourceMirrorsIgnitionConfigFromIDMS), but this creates a UX asymmetry because IDMS is driven from HostedCluster.spec.imageContentSources while ITMS is only provided via ImageMirrorConfigRef ConfigMap; update the codebase by adding a concise TODO and user-facing documentation: add a TODO comment near the conditional referencing the functions above and the fields HostedCluster.spec.imageContentSources and ImageMirrorConfigRef to signal planned alignment, and add or update documentation (release notes or relevant README) explaining the current asymmetry and recommended user workflow so consumers know the difference and the intended future alignment.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml (1)
2926-2932: Clarify how this composes withimageContentSources.
imageContentSourcesis still available immediately above this field, so the CRD description should say whether the two inputs are merged, whether one wins on conflicts, and whether this ref augments or replaces digest mirrors. Right now the API docs are ambiguous for users readingoc explain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml` around lines 2926 - 2932, Update the description for imageMirrorConfigRef to explicitly state how it composes with imageContentSources: specify that controllers will merge imageMirrorConfigRef-derived ImageDigestMirrorSet (IDMS) and ImageTagMirrorSet (ITMS) entries with imageContentSources entries (not replace them), explain conflict resolution (e.g., entries with identical source repository keys are deduplicated and the ConfigMap entries take precedence or vice versa), and clarify whether the ref augments both digest and tag mirrors or only one type; keep the existing note that controllers parse the ConfigMap and reconcile IDMS and ITMS so readers of oc explain understand the merge behavior and precedence between imageMirrorConfigRef and imageContentSources.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml`:
- Around line 2494-2505: The schema currently allows imageMirrorConfigRef.name
to be empty; update the source API type that generates
ClusterVersionOperatorConfiguration (the imageMirrorConfigRef field/type) so
that the name property is required or has minLength: 1, ensuring the generated
CRD for imageMirrorConfigRef includes either a "required: ['name']" entry or
"minLength: 1" for the name string; change the struct/tag or openAPI annotations
on the imageMirrorConfigRef type (the field/type referenced by
imageMirrorConfigRef and the name property) so the CRD rejects empty names at
admission time.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yaml`:
- Around line 2957-2968: The schema for the reference currently inherits
empty-name behavior allowing imageMirrorConfigRef: {} — remove the default: ""
on the properties.name and add a required constraint so the object forces a
non-empty name; specifically update the object schema for the reference (the
properties.name entry and its enclosing type: object) to remove default and
include required: ["name"] (and optionally add a minLength: 1 on name) so
imageMirrorConfigRef must contain a valid name.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yaml`:
- Around line 2517-2527: The schema currently allows imageMirrorConfigRef.name
to be empty; make the field required and non-empty by removing the permissive
empty default and adding a non-empty constraint (e.g., mark
imageMirrorConfigRef.name as required on the parent object and/or add minLength:
1) so that imageMirrorConfigRef cannot be admitted as {} — update the schema
entries referencing imageMirrorConfigRef and the name property
(imageMirrorConfigRef.name) to enforce a non-empty string and remove the legacy
“allowed to be empty” default.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml`:
- Around line 2411-2422: The manifest allows an empty LocalObjectReference for
spec.imageMirrorConfigRef which lets spec.imageMirrorConfigRef: {} pass
admission; update the API to require a non-empty name by making the `name` field
required (or add a non-empty validation like minLength: 1) on the source type
used by imageMirrorConfigRef (the LocalObjectReference-derived field) and then
regenerate the CRD so ClusterVersionOperatorConfiguration's schema enforces a
non-empty `name`; target the schema for imageMirrorConfigRef /
LocalObjectReference and ensure the generated CRD includes `required: ["name"]`
or an equivalent non-empty constraint.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml`:
- Around line 2933-2944: Add an API-level validation so an empty
LocalObjectReference.name is rejected when imageMirrorConfigRef is provided:
update the CRD schema for imageMirrorConfigRef/LocalObjectReference to include a
conditional validation (e.g., an allOf/anyOf JSONSchema entry) that enforces
name is a non-empty string (minLength: 1 or pattern: .+) whenever
imageMirrorConfigRef is present; target the properties "imageMirrorConfigRef"
and "name" (the LocalObjectReference.name) and ensure the validation is
expressed using x-kubernetes validation-compatible schema constructs so CRD
validation fails for imageMirrorConfigRef: {}.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml`:
- Around line 3229-3238: The CRD description incorrectly states the ConfigMap
can "add or override" entries; instead update the description for
ImageMirrorConfigRef/ImageContentSources to reflect actual behavior in
support/globalconfig/imagecontentsource.go and ReconcileImageTagMirrors: for
IDMS entries are concatenated in order (HostedCluster ImageContentSources first,
then ConfigMap entries) with no deduplication or override semantics (duplicates
may appear), and for ITMS the controller currently ignores HostedCluster
ImageContentSources so only ConfigMap entries are used; mention this asymmetry
explicitly so the documentation matches implementation.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yaml`:
- Around line 3150-3169: The schema currently allows imageMirrorConfigRef: {}
because the property "name" defaults to "" and is not required; update the CRD
schema for imageMirrorConfigRef to require a valid non-empty name by adding a
required: ["name"] entry under imageMirrorConfigRef and remove the default empty
string on the name property (or add minLength: 1 to name) so empty objects are
rejected; also consider setting additionalProperties: false on
imageMirrorConfigRef to prevent silent empty payloads. Ensure you modify the
imageMirrorConfigRef and its name property (as seen in the diff) to enforce
these constraints.
In `@docs/content/reference/api.md`:
- Around line 8172-8187: The docs for
HostedControlPlaneSpec.imageMirrorConfigRef are missing the same merge-order and
rollout behaviour details that exist for HostedClusterSpec.imageMirrorConfigRef;
update the source comment that generates the HostedControlPlaneSpec section to
include the same wording about how the ConfigMap is merged with
imageContentSources (specifying precedence/merge order) and how controllers
trigger rollout/reconciliation when mirrors change so both generated references
describe identical semantics for imageMirrorConfigRef in HostedControlPlaneSpec
and HostedClusterSpec.
---
Duplicate comments:
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yaml`:
- Around line 2412-2431: The imageMirrorConfigRef.name currently allows empty
values; update the API markers on the ImageMirrorConfigRef struct's Name field
to make it required and non-empty by adding validation markers (e.g.
+kubebuilder:validation:Required and +kubebuilder:validation:MinLength=1) and
remove "omitempty" from the json tag if present so the generated CRD enforces
presence and a non-empty string for name.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yaml`:
- Around line 3348-3358: The schema allows imageMirrorConfigRef.name to default
to "" so empty objects pass validation; update the CRD schema for the
imageMirrorConfigRef property so that name is required and cannot be empty:
remove the default: "" for name, add a required: ["name"] entry on the
imageMirrorConfigRef object and enforce a non-empty constraint (e.g., minLength:
1 or a pattern) for imageMirrorConfigRef.name so imageMirrorConfigRef: {} is
rejected at admission.
In `@docs/content/reference/aggregated-docs.md`:
- Line 45214: Update the incorrect removal timeline text: find the sentence
"ImageContentSourcePolicy (ICSP) is deprecated and will be removed in OpenShift
4.17." and change it to reflect the official guidance, e.g.
"ImageContentSourcePolicy (ICSP) is deprecated in OpenShift 4.17 and will be
removed in a future release." Ensure the revised sentence replaces the original
exact phrase so the documentation accurately states deprecation in 4.17 with
removal in a future release.
---
Nitpick comments:
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yaml`:
- Around line 2448-2455: Update the imageMirrorConfigRef property description to
explicitly describe how it interacts with spec.imageContentSources: state
whether the entries from imageMirrorConfigRef (idms.yaml and itms.yaml) are
merged with spec.imageContentSources, which source takes precedence on conflicts
(e.g., imageMirrorConfigRef overrides spec.imageContentSources or vice versa),
and whether duplicate entries are deduplicated or rejected; reference the
specific config keys (idms.yaml, itms.yaml) and the field name
spec.imageContentSources so the CRD comment unambiguously defines merge
semantics, conflict resolution, and validation behavior for IDMS/ITMS vs
spec.imageContentSources.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml`:
- Around line 2926-2932: Update the description for imageMirrorConfigRef to
explicitly state how it composes with imageContentSources: specify that
controllers will merge imageMirrorConfigRef-derived ImageDigestMirrorSet (IDMS)
and ImageTagMirrorSet (ITMS) entries with imageContentSources entries (not
replace them), explain conflict resolution (e.g., entries with identical source
repository keys are deduplicated and the ConfigMap entries take precedence or
vice versa), and clarify whether the ref augments both digest and tag mirrors or
only one type; keep the existing note that controllers parse the ConfigMap and
reconcile IDMS and ITMS so readers of oc explain understand the merge behavior
and precedence between imageMirrorConfigRef and imageContentSources.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yaml`:
- Around line 2867-2874: Update the description for imageMirrorConfigRef to
explicitly state how it interacts with imageContentSources: explain whether the
controller merges entries from the referenced ConfigMap with the cluster's
imageContentSources, which source takes precedence on conflict (e.g.,
imageMirrorConfigRef overrides imageContentSources or vice versa), how duplicate
entries are resolved (last-wins, namespace-scoped, or key-based deduplication),
and note that both IDMS and ITMS from the ConfigMap will be reconciled together
with any existing imageContentSources by the controller; reference the fields
imageMirrorConfigRef and imageContentSources in the text so kubectl explain
users can see the merge/override and conflict-resolution behavior clearly.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yaml`:
- Around line 2422-2428: Update the Go doc comment for the imageMirrorConfigRef
field (the ImageMirrorConfigRef field on the HostedControlPlane API type) to
explicitly state the runtime merge semantics with spec.imageContentSources
(e.g., whether the ConfigMap entries are merged into, appended to, or replace
spec.imageContentSources) and the rollout semantics (whether changing the
ConfigMap reference or its contents will trigger NodePool rollouts and how that
rollout is performed). Put this text directly above the ImageMirrorConfigRef
field declaration so the generated CRD description includes the behavior, and
then regenerate the CRD manifests so the updated description appears in
ImageStreamImportMode.yaml.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yaml`:
- Around line 3107-3114: Update the imageMirrorConfigRef description to
explicitly state how it interacts with imageContentSources: explain whether
entries are merged or replaced, how deduplication is performed, and which source
wins on conflicts (e.g., "entries from imageMirrorConfigRef are merged with
imageContentSources; duplicates are deduplicated by exact source+mirror match;
imageMirrorConfigRef entries take precedence on conflict"). Mention that
controllers will parse both sources and reconcile combined configuration
accordingly, and include examples of keys expected ("idms.yaml"/"itms.yaml")
only as supporting detail.
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`:
- Around line 2037-2055: The code branches correctly between IDMS+ITMS and
IDMS-only flows (see globalconfig.ReconcileImageTagMirrors and
ignition.ReconcileImageSourceMirrorsIgnitionConfigFromIDMSAndITMS /
ReconcileImageSourceMirrorsIgnitionConfigFromIDMS), but this creates a UX
asymmetry because IDMS is driven from HostedCluster.spec.imageContentSources
while ITMS is only provided via ImageMirrorConfigRef ConfigMap; update the
codebase by adding a concise TODO and user-facing documentation: add a TODO
comment near the conditional referencing the functions above and the fields
HostedCluster.spec.imageContentSources and ImageMirrorConfigRef to signal
planned alignment, and add or update documentation (release notes or relevant
README) explaining the current asymmetry and recommended user workflow so
consumers know the difference and the intended future alignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f276e166-1ce8-4cdd-9582-f7f0b8aac125
📒 Files selected for processing (37)
api/hypershift/v1beta1/hostedcluster_types.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/NetworkDiagnosticsConfig.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/NetworkDiagnosticsConfig.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlcontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.godocs/content/reference/aggregated-docs.mddocs/content/reference/api.md
🚧 Files skipped from review as they are similar to previous changes (15)
- api/hypershift/v1beta1/hostedcluster_types.go
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yaml
- cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yaml
|
@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. |
Contributor
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 (1)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go (1)
2017-2037:⚠️ Potential issue | 🟠 MajorConfigMap-based ITMS/IDMS will be ignored when no inline
ImageContentSourcesare specified.The early return at lines 2018-2025 occurs before parsing
ImageMirrorConfigRef(lines 2027-2037). If a user uses only the ConfigMap-based approach without inlineImageContentSources,p.HasImageMirrorPolicieswill be false, causing the function to return early without applying the ConfigMap contents.🐛 Proposed fix to account for ConfigMap reference
imageContentPolicyIgnitionConfig := manifests.ImageContentPolicyIgnitionConfig(hcp.GetNamespace()) - if !p.HasImageMirrorPolicies { + if !p.HasImageMirrorPolicies && hcp.Spec.ImageMirrorConfigRef == nil { _, err := util.DeleteIfNeeded(ctx, r.Client, imageContentPolicyIgnitionConfig) if err != nil { return fmt.Errorf("failed to delete image content source policy configuration configmap: %w", err) } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go` around lines 2017 - 2037, The code returns early when p.HasImageMirrorPolicies is false before ever parsing hcp.Spec.ImageMirrorConfigRef, causing ConfigMap-based ImageTag/ ImageDigestMirrorSets to be ignored; fix by moving the ParseImageMirrorConfigMap call (globalconfig.ParseImageMirrorConfigMap) so it runs before the p.HasImageMirrorPolicies check (populate additionalIDMS/additionalITMS from hcp.Spec.ImageMirrorConfigRef first), then change the early-return logic to only delete imageContentPolicyIgnitionConfig and return if both p.HasImageMirrorPolicies is false AND no additionalIDMS/additionalITMS were parsed; references: imageContentPolicyIgnitionConfig, p.HasImageMirrorPolicies, hcp.Spec.ImageMirrorConfigRef, globalconfig.ParseImageMirrorConfigMap.
🤖 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
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`:
- Around line 2017-2037: The code returns early when p.HasImageMirrorPolicies is
false before ever parsing hcp.Spec.ImageMirrorConfigRef, causing ConfigMap-based
ImageTag/ ImageDigestMirrorSets to be ignored; fix by moving the
ParseImageMirrorConfigMap call (globalconfig.ParseImageMirrorConfigMap) so it
runs before the p.HasImageMirrorPolicies check (populate
additionalIDMS/additionalITMS from hcp.Spec.ImageMirrorConfigRef first), then
change the early-return logic to only delete imageContentPolicyIgnitionConfig
and return if both p.HasImageMirrorPolicies is false AND no
additionalIDMS/additionalITMS were parsed; references:
imageContentPolicyIgnitionConfig, p.HasImageMirrorPolicies,
hcp.Spec.ImageMirrorConfigRef, globalconfig.ParseImageMirrorConfigMap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ad18ab53-d009-4a37-8a5c-75b73a1f4bd9
⛔ Files ignored due to path filters (35)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/NetworkDiagnosticsConfig.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/NetworkDiagnosticsConfig.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.md
📒 Files selected for processing (5)
api/hypershift/v1beta1/hosted_controlplane.goapi/hypershift/v1beta1/hostedcluster_types.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
🚧 Files skipped from review as they are similar to previous changes (4)
- api/hypershift/v1beta1/hosted_controlplane.go
- hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
- api/hypershift/v1beta1/hostedcluster_types.go
Contributor
|
[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 |
mariusbertram
had a problem deploying
to
docs-preview
GitHub Actions
Failure
|
@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. |
|
@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. |
mariusbertram
had a problem deploying
to
docs-preview
GitHub Actions
Failure
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
support/globalconfig/imagecontentsource.go (1)
157-160: Use strict decoding for ConfigMap-backed mirror manifests.
yaml.Unmarshalis permissive here, so unknown fields or the wrong kind can decode into a zero-value IDMS/ITMS and fail later, if at all. Failing fast at parse time will make GitOps misconfigurations much easier to diagnose.Possible direction
- if err := yaml.Unmarshal([]byte(idmsYAML), idms); err != nil { + if err := sigsyaml.UnmarshalStrict([]byte(idmsYAML), idms); err != nil { return nil, nil, fmt.Errorf("failed to parse idms.yaml from ConfigMap: %w", err) } + if idms.APIVersion != configv1.GroupVersion.String() || idms.Kind != "ImageDigestMirrorSet" { + return nil, nil, fmt.Errorf("idms.yaml must contain %s ImageDigestMirrorSet", configv1.GroupVersion.String()) + } @@ - if err := yaml.Unmarshal([]byte(itmsYAML), itms); err != nil { + if err := sigsyaml.UnmarshalStrict([]byte(itmsYAML), itms); err != nil { return nil, nil, fmt.Errorf("failed to parse itms.yaml from ConfigMap: %w", err) } + if itms.APIVersion != configv1.GroupVersion.String() || itms.Kind != "ImageTagMirrorSet" { + return nil, nil, fmt.Errorf("itms.yaml must contain %s ImageTagMirrorSet", configv1.GroupVersion.String()) + }Also applies to: 165-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@support/globalconfig/imagecontentsource.go` around lines 157 - 160, Replace the permissive yaml.Unmarshal call that parses idmsYAML into idms (type ImageDigestMirrorSet) with a strict decoder (e.g., yaml.UnmarshalStrict or a YAML decoder configured to disallow unknown fields) so unknown fields cause a parse error; after strict-unmarshal, also validate the decoded object's TypeMeta/Kind (ensure it/its Kind equals "ImageDigestMirrorSet" or that required fields are present) and return a clear error on mismatch. Do the same change for the analogous ITMS parsing code paths (the other yaml.Unmarshal usages) so ConfigMap-backed mirror manifests fail fast on unknown fields or wrong kinds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@support/globalconfig/imagecontentsource.go`:
- Around line 123-137: The caller currently invokes ReconcileImageDigestMirrors
unconditionally but only calls ReconcileImageTagMirrors when additionalITMS !=
nil, which leaves stale ITMS objects when the config is removed; update the
caller to invoke ReconcileImageTagMirrors unconditionally (passing nil when no
additionalITMS) just like ReconcileImageDigestMirrors, or if you prefer, add an
explicit cleanup branch in the caller that calls ReconcileImageTagMirrors with
additionalITMS=nil to clear itms.Spec.ImageTagMirrors; locate the calls to
ReconcileImageDigestMirrors and ReconcileImageTagMirrors and make the invocation
behavior consistent so ReconcileImageTagMirrors always runs.
---
Nitpick comments:
In `@support/globalconfig/imagecontentsource.go`:
- Around line 157-160: Replace the permissive yaml.Unmarshal call that parses
idmsYAML into idms (type ImageDigestMirrorSet) with a strict decoder (e.g.,
yaml.UnmarshalStrict or a YAML decoder configured to disallow unknown fields) so
unknown fields cause a parse error; after strict-unmarshal, also validate the
decoded object's TypeMeta/Kind (ensure it/its Kind equals "ImageDigestMirrorSet"
or that required fields are present) and return a clear error on mismatch. Do
the same change for the analogous ITMS parsing code paths (the other
yaml.Unmarshal usages) so ConfigMap-backed mirror manifests fail fast on unknown
fields or wrong kinds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e7bff817-9dd1-49d1-b092-3d3a0ff8062d
⛔ Files ignored due to path filters (40)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/hostedclusterspec.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/hostedcontrolplanespec.gois excluded by!client/**cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hosted_controlplane.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go
📒 Files selected for processing (14)
api/hypershift/v1beta1/hosted_controlplane.goapi/hypershift/v1beta1/hostedcluster_types.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/ignition/params.gocontrol-plane-operator/controllers/hostedcontrolplane/ignition/reconcile.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.godocs/content/how-to/configure-image-registry-mirrors.mddocs/content/reference/image-registry-mirrors.mdhypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/nto_test.gosupport/capabilities/management_cluster_capabilities.gosupport/globalconfig/imagecontentsource.gosupport/globalconfig/imagecontentsource_test.go
✅ Files skipped from review due to trivial changes (4)
- hypershift-operator/controllers/nodepool/nto_test.go
- docs/content/how-to/configure-image-registry-mirrors.md
- docs/content/reference/image-registry-mirrors.md
- api/hypershift/v1beta1/hostedcluster_types.go
🚧 Files skipped from review as they are similar to previous changes (6)
- control-plane-operator/controllers/hostedcontrolplane/ignition/params.go
- hypershift-operator/controllers/nodepool/config.go
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
- hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
- control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
- control-plane-operator/controllers/hostedcontrolplane/ignition/reconcile.go
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
|
@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. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@support/globalconfig/imagecontentsource.go`:
- Around line 156-169: The idms.yaml and itms.yaml parsing accepts wrong
payloads silently; after unmarshalling into idms (configv1.ImageDigestMirrorSet)
and itms (configv1.ImageTagMirrorSet) validate that the parsed object's
TypeMeta.Kind equals "ImageDigestMirrorSet" or "ImageTagMirrorSet" respectively
(or at minimum that critical spec fields like ImageDigestMirrors/ImageTagMirrors
are non-empty) and return a descriptive error if the kind/required fields don't
match, so the reconciler won't accept swapped/malformed configs from the
"idms.yaml" and "itms.yaml" keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 4d9c5548-0bf6-4450-86ce-c477e4821338
⛔ Files ignored due to path filters (40)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/hostedclusterspec.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/hostedcontrolplanespec.gois excluded by!client/**cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hosted_controlplane.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go
📒 Files selected for processing (14)
api/hypershift/v1beta1/hosted_controlplane.goapi/hypershift/v1beta1/hostedcluster_types.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/ignition/params.gocontrol-plane-operator/controllers/hostedcontrolplane/ignition/reconcile.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.godocs/content/how-to/configure-image-registry-mirrors.mddocs/content/reference/image-registry-mirrors.mdhypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/nto_test.gosupport/capabilities/management_cluster_capabilities.gosupport/globalconfig/imagecontentsource.gosupport/globalconfig/imagecontentsource_test.go
✅ Files skipped from review due to trivial changes (5)
- hypershift-operator/controllers/nodepool/nto_test.go
- docs/content/how-to/configure-image-registry-mirrors.md
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
- docs/content/reference/image-registry-mirrors.md
- api/hypershift/v1beta1/hostedcluster_types.go
🚧 Files skipped from review as they are similar to previous changes (3)
- control-plane-operator/controllers/hostedcontrolplane/ignition/params.go
- hypershift-operator/controllers/nodepool/config.go
- hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
|
@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. |
mariusbertram
had a problem deploying
to
docs-preview
GitHub Actions
Failure
|
Stale PRs are closed after 21d of inactivity. If this PR is still relevant, comment to refresh it or remove the stale label. If this PR is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
lifecycle/stale
|
Now I have the full picture for both failures. Let me produce the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryNeither failure is caused by the PR's code changes. The tide error is a merge gate rejection because the PR has a merge conflict with Root CauseTide Error — Merge Conflict: Deploy Preview — Cloudflare Auth Failure: A secondary non-fatal error ( RecommendationsFor the PR author (mariusbertram):
For repository maintainers: Evidence
|
everettraven
reviewed
May 4, 2026
Implement support for HostedCluster.spec.imageMirrorConfig 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 imageMirrorConfig 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 imageMirrorConfig to HostedControlPlane - HostedControlPlane controller parses and reconciles IDMS/ITMS from ConfigMap - Add ReconcileImageSourceMirrorsIgnitionConfigFromIDMSAndITMS() for combined configs - Update globalConfigString() to include imageMirrorConfig in rollout hash - Update HasImageMirrorPolicies check to include imageMirrorConfig - Update getCoreConfigs() to expect additional config when imageMirrorConfig 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. Signed-off-by: Marius Bertram <marius@brtrm.de>
openshift-ci
Bot
removed
the
needs-rebase
|
@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 "5.0.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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
Summary by CodeRabbit
Release Notes
New Features
imageMirrorConfigReffield to enable ConfigMap-based image mirror configuration through a unified reference.Documentation