OCPBUGS-91634: CAPI: Fix ImageDigestFormat validation to allow deep registry paths#2918
OCPBUGS-91634: CAPI: Fix ImageDigestFormat validation to allow deep registry paths#2918simkam wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Hello @simkam! Some important instructions when contributing to openshift/api: |
|
@simkam: This pull request references Jira Issue OCPBUGS-91634, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change broadens OCI image reference validation across the API to accept multiple nested namespace path segments ( 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
machineconfiguration/v1/types_machineconfignode.go (1)
221-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCross-file inconsistency: host validation pattern differs from
ImageDigestFormatandInternalReleaseImageBundleStatus.Image.This field's regex allows
localhostas a host (localhost|([a-zA-Z0-9-]+\\.)+...), while theImageDigestFormattype inmachineconfiguration/v1/types_machineosconfig.go:206and theInternalReleaseImageBundleStatus.Imagefield inmachineconfiguration/v1/types_internalreleaseimage.go:140require a dot-qualified domain name with nolocalhostalternative. This means the same image pull spec (e.g.,localhost:5000/openshift/image@sha256:...) would pass validation here but fail forImageDigestFormatfields.This is pre-existing, but since this PR touches all these regex patterns together, consider aligning the host validation across all image reference fields — either by adding
localhostsupport toImageDigestFormator documenting whyMachineConfigNodeStatusInternalReleaseImageRef.Imagehas stricter host requirements.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@machineconfiguration/v1/types_machineconfignode.go` at line 221, Align the image host validation used by MachineConfigNodeStatusInternalReleaseImageRef.Image with the regexes in ImageDigestFormat and InternalReleaseImageBundleStatus.Image so they enforce the same host rules. Right now the XValidation on the machineconfiguration/v1/types_machineconfignode.go field allows localhost while the other image reference types do not, which creates inconsistent behavior for the same pull spec. Update the shared pattern or explicitly document why this field should be stricter/different, and make the choice consistent across the relevant type definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@operator/v1alpha1/types_clusterapi.go`:
- Around line 247-253: Add tests for the new ImageDigestFormat validation to
ensure the Cluster API schema accepts valid image references and rejects invalid
ones. Update the existing test suite under
operator/v1alpha1/tests/clusterapis.operator.openshift.io/ to cover the
ImageDigestFormat rules, including multi-segment paths, single-segment paths,
and malformed references that should fail the XValidation constraints. Use the
ImageDigestFormat type and its validation behavior as the target for the new
cases so the tests stay aligned if implementation details move.
---
Nitpick comments:
In `@machineconfiguration/v1/types_machineconfignode.go`:
- Line 221: Align the image host validation used by
MachineConfigNodeStatusInternalReleaseImageRef.Image with the regexes in
ImageDigestFormat and InternalReleaseImageBundleStatus.Image so they enforce the
same host rules. Right now the XValidation on the
machineconfiguration/v1/types_machineconfignode.go field allows localhost while
the other image reference types do not, which creates inconsistent behavior for
the same pull spec. Update the shared pattern or explicitly document why this
field should be stricter/different, and make the choice consistent across the
relevant type definitions.
🪄 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), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: bdb19428-d635-4868-88cc-62d86408cf4b
📒 Files selected for processing (6)
machineconfiguration/v1/types_internalreleaseimage.gomachineconfiguration/v1/types_machineconfignode.gomachineconfiguration/v1/types_machineosconfig.gomachineconfiguration/v1alpha1/types.gomachineconfiguration/v1alpha1/types_internalreleaseimage.gooperator/v1alpha1/types_clusterapi.go
| // ImageDigestFormat is a type that conforms to the format host[:port][/namespace[/namespace...]]/name@sha256:<digest>. | ||
| // The digest must be 64 characters long, and consist only of lowercase hexadecimal characters, a-f and 0-9. | ||
| // The length of the field must be between 1 to 447 characters. | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=447 | ||
| // +kubebuilder:validation:XValidation:rule=`(self.split('@').size() == 2 && self.split('@')[1].matches('^sha256:[a-f0-9]{64}$'))`,message="the OCI Image reference must end with a valid '@sha256:<digest>' suffix, where '<digest>' is 64 characters long" | ||
| // +kubebuilder:validation:XValidation:rule=`(self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]*?$'))`,message="the OCI Image name should follow the host[:port][/namespace]/name format, resembling a valid URL without the scheme" | ||
| // +kubebuilder:validation:XValidation:rule=`(self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?(/[a-zA-Z0-9-_.]+)+$'))`,message="the OCI Image name should follow the host[:port][/namespace[/namespace...]]/name format, resembling a valid URL without the scheme" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for existing test suites that cover ImageDigestFormat or internal release image validation
rg -l 'ImageDigestFormat\|InternalReleaseImageBundle\|MachineConfigNodeStatusInternalReleaseImage' --glob '*.testsuite.yaml' --glob '*test*'Repository: openshift/api
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== File outline =="
ast-grep outline operator/v1alpha1/types_clusterapi.go --view expanded || true
echo
echo "== Relevant lines =="
sed -n '220,290p' operator/v1alpha1/types_clusterapi.go
echo
echo "== Test suites in operator/v1alpha1 =="
fd -a --type f . operator/v1alpha1 | rg '\.testsuite\.yaml$|test'
echo
echo "== Search for image reference validation in tests =="
rg -n 'sha256:|ImageDigestFormat|image reference|internal release image|bundle' operator/v1alpha1 operator/v1 tests config route install -g '*.testsuite.yaml' -g '*.go' || trueRepository: openshift/api
Length of output: 38932
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the edited API type and any nearby tests/testdata in the same API group.
echo "== operator/v1alpha1 files =="
git ls-files 'operator/v1alpha1/**' | sed -n '1,200p'
echo
echo "== types_clusterapi.go around the change =="
sed -n '1,340p' operator/v1alpha1/types_clusterapi.go
echo
echo "== Any testsuite YAML under operator/v1alpha1 =="
git ls-files 'operator/v1alpha1/**/*.testsuite.yaml' 'operator/v1alpha1/**/tests/**' | sed -n '1,200p' || true
echo
echo "== Search for validation markers related to the changed regex =="
rg -n 'XValidation:rule=.*split\(@\)|ImageDigestFormat|InternalReleaseImageBundle|MachineConfigNodeStatusInternalReleaseImage|sha256:\[a-f0-9\]\{64\}' operator/v1alpha1 -g '*.go' -g '*.testsuite.yaml' || trueRepository: openshift/api
Length of output: 18544
Add tests for the new image-reference validation. Cover multi-segment paths, single-segment paths, and invalid references in operator/v1alpha1/tests/clusterapis.operator.openshift.io/.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@operator/v1alpha1/types_clusterapi.go` around lines 247 - 253, Add tests for
the new ImageDigestFormat validation to ensure the Cluster API schema accepts
valid image references and rejects invalid ones. Update the existing test suite
under operator/v1alpha1/tests/clusterapis.operator.openshift.io/ to cover the
ImageDigestFormat rules, including multi-segment paths, single-segment paths,
and malformed references that should fail the XValidation constraints. Use the
ImageDigestFormat type and its validation behavior as the target for the new
cases so the tests stay aligned if implementation details move.
Source: Coding guidelines
|
/hold local testing |
|
This fix looks correct at a quick glance, but any validation this gnarly needs tests. |
33cb756 to
63d0a7b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
payload-manifests/crds/0000_80_machine-config_01_machineosconfigs.crd.yaml (1)
256-274: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winField description text not updated to match the new deep-path validation.
currentImagePullSpec's description (line 262) still readshost[:port][/namespace]/name@sha256:<digest>while the validation regex at lines 272-274 now permits[/namespace[/namespace...]]. Same drift as in other files in this cohort.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-manifests/crds/0000_80_machine-config_01_machineosconfigs.crd.yaml` around lines 256 - 274, Update the currentImagePullSpec field description to match the deeper namespace validation used by the x-kubernetes-validations rules. In the MachineOSConfigs CRD, align the documented image pull spec format with the actual regex in currentImagePullSpec so it reflects host[:port][/namespace[/namespace...]]/name@sha256:<digest> instead of the older single-namespace wording.Source: Coding guidelines
payload-manifests/crds/0000_80_machine-config_01_osimagestreams-SelfManagedHA.crd.yaml (1)
119-158: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
osExtensionsImage/osImagedescriptions not updated to reflect deep-path support.Both fields' descriptions (lines 126-128, 146-148) still say
host[:port][/namespace]/name@sha256:<digest>, but the validation rules at lines 136-138 and 156-158 were updated to accept multiple path segments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-manifests/crds/0000_80_machine-config_01_osimagestreams-SelfManagedHA.crd.yaml` around lines 119 - 158, Update the osExtensionsImage and osImage descriptions in the CRD so they match the current validation rules: the documented image pull spec still implies only a single namespace segment, while the x-kubernetes-validations in the osExtensionsImage and osImage schema allow multiple deep path segments. Adjust the description text to describe host[:port][/namespace[/namespace...]]/name@sha256:<digest> consistently for both fields so the docs align with the actual schema behavior.Source: Coding guidelines
payload-manifests/crds/0000_30_cluster-api_01_clusterapis.crd.yaml (1)
161-181: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
reffield description not updated to reflect deep-path support.The description at line 164 still says the reference
must be a valid image digest reference in the format host[:port][/namespace]/name@sha256:<digest>, but the validation rule at lines 175-177 was updated to accept[/namespace[/namespace...]]. Note the upstreamImageDigestFormattype doc comment already uses the updated wording (per the provided context snippet fromoperator/v1alpha1/types_clusterapi.go), so only this field-specific description text lags behind.📝 Suggested description fix
- ref is an image reference to the image containing the component manifests. The reference - must be a valid image digest reference in the format host[:port][/namespace]/name@sha256:<digest>. + ref is an image reference to the image containing the component manifests. The reference + must be a valid image digest reference in the format host[:port][/namespace[/namespace...]]/name@sha256:<digest>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-manifests/crds/0000_30_cluster-api_01_clusterapis.crd.yaml` around lines 161 - 181, The Cluster API CRD schema for the ref field has an outdated description that still documents only host[:port][/namespace]/name@sha256:<digest>, while the validation now supports deeper namespace paths. Update the ref description text in the CRD definition to match the ImageDigestFormat wording and reflect host[:port][/namespace[/namespace...]]/name@sha256:<digest>, keeping it consistent with the type docs used elsewhere.Source: Coding guidelines
payload-manifests/crds/0000_80_machine-config_01_pinnedimagesets.crd.yaml (1)
69-84: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
namedescription not updated to reflect deep-path support.Description at line 72 still says
host[:port][/namespace]/name@sha256:<digest>while the validation rule at lines 82-84 now accepts multiple/namespacesegments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-manifests/crds/0000_80_machine-config_01_pinnedimagesets.crd.yaml` around lines 69 - 84, Update the `name` field description in the `pinnedimagesets` CRD to match the current validation logic: the description still says `host[:port][/namespace]/name@sha256:<digest>` but `x-kubernetes-validations` in the same schema now allows multiple nested path segments. Adjust the wording in the `name` property documentation so it reflects deep-path support consistently with the rule on `name`.Source: Coding guidelines
payload-manifests/crds/0000_80_machine-config_01_machineosbuilds.crd.yaml (1)
306-321: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winField description text is stale relative to the updated validation rule.
The
digestedImagePushSpecdescription (line 309) still says the format ishost[:port][/namespace]/name@sha256:<digest>(singular namespace), while the validation rule at lines 319-321 now accepts[/namespace[/namespace...]]. This mismatch is misleading to API consumers reading the field docs. As per coding guidelines,"the comment must explicitly state ... Valid enum values ... Validation constraints", so the prose description should match the actual accepted format.📝 Suggested description fix
- The format of the push spec is: host[:port][/namespace]/name@sha256:<digest>, + The format of the push spec is: host[:port][/namespace[/namespace...]]/name@sha256:<digest>,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-manifests/crds/0000_80_machine-config_01_machineosbuilds.crd.yaml` around lines 306 - 321, The digestedImagePushSpec field documentation is stale and no longer matches the validation rule. Update the description in the MachineOSBuilds CRD so it reflects the accepted push spec format with repeated path namespaces, matching the x-kubernetes-validations on digestedImagePushSpec. Keep the prose aligned with the actual rule in the same schema block, including the sha256 digest requirement and the allowed host[:port]/namespace.../name structure.Source: Coding guidelines
payload-manifests/crds/0000_80_machine-config_01_osimagestreams-Hypershift.crd.yaml (1)
120-159: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winSame description drift as the SelfManagedHA variant.
osExtensionsImage/osImagedescriptions (lines 127-129, 147-149) still sayhost[:port][/namespace]/name@sha256:<digest>, not matching the updated multi-segment validation rules at lines 137-139 and 157-159.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-manifests/crds/0000_80_machine-config_01_osimagestreams-Hypershift.crd.yaml` around lines 120 - 159, The osExtensionsImage and osImage descriptions are out of sync with their validation rules. Update the description text in the CRD entries for osExtensionsImage and osImage so it matches the current multi-segment image name format enforced by the x-kubernetes-validations rules, keeping the wording aligned with the existing validation patterns and unique symbols osExtensionsImage and osImage.Source: Coding guidelines
payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yaml (1)
513-529: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftv1alpha1 still rejects deep registry paths
status.releases[].imagein the servedv1alpha1schema still uses the old regex with only an optional single namespace segment, so valid deep mirror paths likehost/ns1/ns2/name@sha256:...are rejected. Update this version to match thev1pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yaml` around lines 513 - 529, The v1alpha1 schema for status.releases[].image still uses the old single-namespace regex, so deep registry paths are rejected. Update the x-kubernetes-validations rule on the image field in this CRD to match the newer v1 pattern and allow multiple nested namespace segments while keeping the `@sha256` digest validation unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@machineconfiguration/v1/tests/machineconfignodes.machineconfiguration.openshift.io/NoRegistryClusterInstall.yaml`:
- Around line 160-312: The new onUpdate test cases are missing required
MachineConfigNode spec fields, so the resource cannot be created before the
status update is validated. Update each affected case in
NoRegistryClusterInstall.yaml to include spec.node and spec.configVersion
alongside spec.pool in both the initial and updated objects, using the existing
MachineConfigNode schema as the reference. Keep the test names and status
expectations unchanged so the cases still exercise the intended image validation
paths.
---
Outside diff comments:
In `@payload-manifests/crds/0000_30_cluster-api_01_clusterapis.crd.yaml`:
- Around line 161-181: The Cluster API CRD schema for the ref field has an
outdated description that still documents only
host[:port][/namespace]/name@sha256:<digest>, while the validation now supports
deeper namespace paths. Update the ref description text in the CRD definition to
match the ImageDigestFormat wording and reflect
host[:port][/namespace[/namespace...]]/name@sha256:<digest>, keeping it
consistent with the type docs used elsewhere.
In
`@payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yaml`:
- Around line 513-529: The v1alpha1 schema for status.releases[].image still
uses the old single-namespace regex, so deep registry paths are rejected. Update
the x-kubernetes-validations rule on the image field in this CRD to match the
newer v1 pattern and allow multiple nested namespace segments while keeping the
`@sha256` digest validation unchanged.
In `@payload-manifests/crds/0000_80_machine-config_01_machineosbuilds.crd.yaml`:
- Around line 306-321: The digestedImagePushSpec field documentation is stale
and no longer matches the validation rule. Update the description in the
MachineOSBuilds CRD so it reflects the accepted push spec format with repeated
path namespaces, matching the x-kubernetes-validations on digestedImagePushSpec.
Keep the prose aligned with the actual rule in the same schema block, including
the sha256 digest requirement and the allowed host[:port]/namespace.../name
structure.
In `@payload-manifests/crds/0000_80_machine-config_01_machineosconfigs.crd.yaml`:
- Around line 256-274: Update the currentImagePullSpec field description to
match the deeper namespace validation used by the x-kubernetes-validations
rules. In the MachineOSConfigs CRD, align the documented image pull spec format
with the actual regex in currentImagePullSpec so it reflects
host[:port][/namespace[/namespace...]]/name@sha256:<digest> instead of the older
single-namespace wording.
In
`@payload-manifests/crds/0000_80_machine-config_01_osimagestreams-Hypershift.crd.yaml`:
- Around line 120-159: The osExtensionsImage and osImage descriptions are out of
sync with their validation rules. Update the description text in the CRD entries
for osExtensionsImage and osImage so it matches the current multi-segment image
name format enforced by the x-kubernetes-validations rules, keeping the wording
aligned with the existing validation patterns and unique symbols
osExtensionsImage and osImage.
In
`@payload-manifests/crds/0000_80_machine-config_01_osimagestreams-SelfManagedHA.crd.yaml`:
- Around line 119-158: Update the osExtensionsImage and osImage descriptions in
the CRD so they match the current validation rules: the documented image pull
spec still implies only a single namespace segment, while the
x-kubernetes-validations in the osExtensionsImage and osImage schema allow
multiple deep path segments. Adjust the description text to describe
host[:port][/namespace[/namespace...]]/name@sha256:<digest> consistently for
both fields so the docs align with the actual schema behavior.
In `@payload-manifests/crds/0000_80_machine-config_01_pinnedimagesets.crd.yaml`:
- Around line 69-84: Update the `name` field description in the
`pinnedimagesets` CRD to match the current validation logic: the description
still says `host[:port][/namespace]/name@sha256:<digest>` but
`x-kubernetes-validations` in the same schema now allows multiple nested path
segments. Adjust the wording in the `name` property documentation so it reflects
deep-path support consistently with the rule on `name`.
🪄 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), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 25cd856c-072f-40aa-a1a7-8f317a2bf74a
⛔ Files ignored due to path filters (32)
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineosbuilds.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineosconfigs.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-Hypershift.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-SelfManagedHA.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_pinnedimagesets.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.featuregated-crd-manifests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/machineconfignodes.machineconfiguration.openshift.io/ImageModeStatusReporting.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/machineconfignodes.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/machineosbuilds.machineconfiguration.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/machineosconfigs.machineconfiguration.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/osimagestreams.machineconfiguration.openshift.io/OSStreams.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/pinnedimagesets.machineconfiguration.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-Hypershift.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-SelfManagedHA.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1alpha1/zz_generated.featuregated-crd-manifests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1alpha1/zz_generated.featuregated-crd-manifests/osimagestreams.machineconfiguration.openshift.io/OSStreams.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1alpha1/zz_generated.featuregated-crd-manifests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (22)
machineconfiguration/v1/tests/machineconfignodes.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlmachineconfiguration/v1/types_internalreleaseimage.gomachineconfiguration/v1/types_machineconfignode.gomachineconfiguration/v1/types_machineosconfig.gomachineconfiguration/v1alpha1/types.gomachineconfiguration/v1alpha1/types_internalreleaseimage.gooperator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yamloperator/v1alpha1/types_clusterapi.gopayload-manifests/crds/0000_30_cluster-api_01_clusterapis.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineosbuilds.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineosconfigs.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_osimagestreams-Hypershift.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_osimagestreams-SelfManagedHA.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_pinnedimagesets.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- machineconfiguration/v1/types_internalreleaseimage.go
- machineconfiguration/v1/types_machineconfignode.go
- machineconfiguration/v1alpha1/types.go
- operator/v1alpha1/types_clusterapi.go
- machineconfiguration/v1/types_machineosconfig.go
- machineconfiguration/v1alpha1/types_internalreleaseimage.go
63d0a7b to
7d7608b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yaml (1)
513-529: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRegenerate the v1alpha1 CRDs
machineconfiguration/v1alpha1/types_internalreleaseimage.gonow allowshost[:port][/namespace[/namespace...]]/name, but bothpayload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yamlandpayload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yamlstill use the old single-namespace regex/message forimage. Runmake updateso the generated manifests match the type definition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yaml` around lines 513 - 529, The generated CRD validation for image is still using the old single-namespace pattern and message, while machineconfiguration/v1alpha1/types_internalreleaseimage.go now permits host[:port][/namespace[/namespace...]]/name. Regenerate the v1alpha1 CRDs via make update so the x-kubernetes-validations on InternalReleaseImage.image in both generated manifests reflect the updated type definition and messaging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@machineconfiguration/v1alpha1/tests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yaml`:
- Line 306: The test expectation for the internal release image OCI name error
is out of sync with the v1alpha1 CRD schema. Update the expectedStatusError in
the NoRegistryClusterInstall test to match the actual v1alpha1 message emitted
by the schema, or regenerate the CRD if the v1alpha1 type change is intended.
Use the existing test case and the CRD-generated error text as the source of
truth, and keep the substring exactly aligned with what the generator matches in
tests/generator.go.
---
Outside diff comments:
In
`@payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yaml`:
- Around line 513-529: The generated CRD validation for image is still using the
old single-namespace pattern and message, while
machineconfiguration/v1alpha1/types_internalreleaseimage.go now permits
host[:port][/namespace[/namespace...]]/name. Regenerate the v1alpha1 CRDs via
make update so the x-kubernetes-validations on InternalReleaseImage.image in
both generated manifests reflect the updated type definition and messaging.
🪄 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), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: dd081f4a-86c9-4542-9b9f-9d092ec06755
⛔ Files ignored due to path filters (32)
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineosbuilds.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineosconfigs.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-Hypershift.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-SelfManagedHA.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_pinnedimagesets.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.featuregated-crd-manifests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/machineconfignodes.machineconfiguration.openshift.io/ImageModeStatusReporting.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/machineconfignodes.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/machineosbuilds.machineconfiguration.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/machineosconfigs.machineconfiguration.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/osimagestreams.machineconfiguration.openshift.io/OSStreams.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/pinnedimagesets.machineconfiguration.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-Hypershift.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-SelfManagedHA.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1alpha1/zz_generated.featuregated-crd-manifests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1alpha1/zz_generated.featuregated-crd-manifests/osimagestreams.machineconfiguration.openshift.io/OSStreams.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1alpha1/zz_generated.featuregated-crd-manifests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (27)
machineconfiguration/v1/tests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlmachineconfiguration/v1/tests/machineconfignodes.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlmachineconfiguration/v1/tests/machineosconfigs.machineconfiguration.openshift.io/AAA_ungated.yamlmachineconfiguration/v1/tests/osimagestreams.machineconfiguration.openshift.io/OSStreams.yamlmachineconfiguration/v1/types_internalreleaseimage.gomachineconfiguration/v1/types_machineconfignode.gomachineconfiguration/v1/types_machineosconfig.gomachineconfiguration/v1alpha1/tests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlmachineconfiguration/v1alpha1/tests/osimagestreams.machineconfiguration.openshift.io/OSStreams.yamlmachineconfiguration/v1alpha1/types.gomachineconfiguration/v1alpha1/types_internalreleaseimage.gooperator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yamloperator/v1alpha1/types_clusterapi.gopayload-manifests/crds/0000_30_cluster-api_01_clusterapis.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineosbuilds.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineosconfigs.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_osimagestreams-Hypershift.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_osimagestreams-SelfManagedHA.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_pinnedimagesets.crd.yaml
✅ Files skipped from review due to trivial changes (3)
- machineconfiguration/v1alpha1/tests/osimagestreams.machineconfiguration.openshift.io/OSStreams.yaml
- machineconfiguration/v1/tests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yaml
- machineconfiguration/v1/tests/osimagestreams.machineconfiguration.openshift.io/OSStreams.yaml
🚧 Files skipped from review as they are similar to previous changes (21)
- payload-manifests/crds/0000_80_machine-config_01_osimagestreams-Hypershift.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_osimagestreams-SelfManagedHA.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_pinnedimagesets.crd.yaml
- machineconfiguration/v1alpha1/types.go
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-DevPreviewNoUpgrade.crd.yaml
- machineconfiguration/v1alpha1/types_internalreleaseimage.go
- payload-manifests/crds/0000_80_machine-config_01_machineosconfigs.crd.yaml
- operator/v1alpha1/types_clusterapi.go
- payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yaml
- machineconfiguration/v1/types_machineconfignode.go
- payload-manifests/crds/0000_30_cluster-api_01_clusterapis.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-CustomNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_machineosbuilds.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-TechPreviewNoUpgrade.crd.yaml
- operator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yaml
- machineconfiguration/v1/types_internalreleaseimage.go
- machineconfiguration/v1/tests/machineconfignodes.machineconfiguration.openshift.io/NoRegistryClusterInstall.yaml
- machineconfiguration/v1/types_machineosconfig.go
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-CustomNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml
7d7608b to
499880b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@payload-manifests/crds/0000_80_machine-config_01_machineosbuilds.crd.yaml`:
- Around line 118-123: The renderedImagePushSpec documentation is out of sync
with the actual ImageTagFormat validation and still describes only a single
namespace segment. Update the Go doc comment on renderedImagePushSpec in the
machineconfiguration/v1 types (both MachineOSConfig and MachineOSBuild) to
describe nested namespace segments as supported by the regex, then regenerate
the CRD so the generated schema description matches the updated comment.
In `@payload-manifests/crds/0000_80_machine-config_01_machineosconfigs.crd.yaml`:
- Around line 181-186: The `renderedImagePushSpec` docs are out of sync with the
validator and should be updated to describe the accepted OCI image formats.
Change the field comment in `machineconfiguration/v1/types_machineosconfig.go`
so it matches the deeper `host[:port][/namespace[/namespace...]]/name:<tag>`
shape and the `.svc`-with-port form, then regenerate the CRD manifests so
`MachineOSConfig` documentation and validation stay aligned.
🪄 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), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0b89bff3-479c-4864-815a-42e55edc5cfa
⛔ Files ignored due to path filters (32)
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineosbuilds.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineosconfigs.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-Hypershift.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-SelfManagedHA.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_pinnedimagesets.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.featuregated-crd-manifests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/machineconfignodes.machineconfiguration.openshift.io/ImageModeStatusReporting.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/machineconfignodes.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/machineosbuilds.machineconfiguration.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/machineosconfigs.machineconfiguration.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/osimagestreams.machineconfiguration.openshift.io/OSStreams.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/pinnedimagesets.machineconfiguration.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-Hypershift.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-SelfManagedHA.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1alpha1/zz_generated.featuregated-crd-manifests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1alpha1/zz_generated.featuregated-crd-manifests/osimagestreams.machineconfiguration.openshift.io/OSStreams.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1alpha1/zz_generated.featuregated-crd-manifests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (27)
machineconfiguration/v1/tests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlmachineconfiguration/v1/tests/machineconfignodes.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlmachineconfiguration/v1/tests/machineosconfigs.machineconfiguration.openshift.io/AAA_ungated.yamlmachineconfiguration/v1/tests/osimagestreams.machineconfiguration.openshift.io/OSStreams.yamlmachineconfiguration/v1/types_internalreleaseimage.gomachineconfiguration/v1/types_machineconfignode.gomachineconfiguration/v1/types_machineosconfig.gomachineconfiguration/v1alpha1/tests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlmachineconfiguration/v1alpha1/tests/osimagestreams.machineconfiguration.openshift.io/OSStreams.yamlmachineconfiguration/v1alpha1/types.gomachineconfiguration/v1alpha1/types_internalreleaseimage.gooperator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yamloperator/v1alpha1/types_clusterapi.gopayload-manifests/crds/0000_30_cluster-api_01_clusterapis.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineosbuilds.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineosconfigs.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_osimagestreams-Hypershift.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_osimagestreams-SelfManagedHA.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_pinnedimagesets.crd.yaml
✅ Files skipped from review due to trivial changes (4)
- machineconfiguration/v1/tests/osimagestreams.machineconfiguration.openshift.io/OSStreams.yaml
- machineconfiguration/v1alpha1/tests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yaml
- machineconfiguration/v1/tests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yaml
- machineconfiguration/v1alpha1/tests/osimagestreams.machineconfiguration.openshift.io/OSStreams.yaml
🚧 Files skipped from review as they are similar to previous changes (21)
- payload-manifests/crds/0000_80_machine-config_01_osimagestreams-SelfManagedHA.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_pinnedimagesets.crd.yaml
- machineconfiguration/v1/types_internalreleaseimage.go
- payload-manifests/crds/0000_80_machine-config_01_osimagestreams-Hypershift.crd.yaml
- machineconfiguration/v1/tests/machineosconfigs.machineconfiguration.openshift.io/AAA_ungated.yaml
- payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_30_cluster-api_01_clusterapis.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yaml
- machineconfiguration/v1alpha1/types_internalreleaseimage.go
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml
- machineconfiguration/v1alpha1/types.go
- machineconfiguration/v1/types_machineconfignode.go
- operator/v1alpha1/types_clusterapi.go
- operator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yaml
- machineconfiguration/v1/types_machineosconfig.go
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-CustomNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-CustomNoUpgrade.crd.yaml
- machineconfiguration/v1/tests/machineconfignodes.machineconfiguration.openshift.io/NoRegistryClusterInstall.yaml
| // +kubebuilder:validation:MaxLength=447 | ||
| // +kubebuilder:validation:XValidation:rule=`(self.split('@').size() == 2 && self.split('@')[1].matches('^sha256:[a-f0-9]{64}$'))`,message="the OCI Image reference must end with a valid '@sha256:<digest>' suffix, where '<digest>' is 64 characters long" | ||
| // +kubebuilder:validation:XValidation:rule=`(self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]*?$'))`,message="the OCI Image name should follow the host[:port][/namespace]/name format, resembling a valid URL without the scheme" | ||
| // +kubebuilder:validation:XValidation:rule=`(self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?(/[a-zA-Z0-9-_.]+)+$'))`,message="the OCI Image name should follow the host[:port][/namespace[/namespace...]]/name format, resembling a valid URL without the scheme" |
There was a problem hiding this comment.
Are we sure about dropping the length validation here? Also, the two character sets are not quite the same, I don't think this is the right change
I was expecting just the namespace to change from a zero or one repeat to a multi repeat?
There was a problem hiding this comment.
ok, let's go with minimal change. I'll update this. I couldn't figure out why the length validation is here. It doesn't seem to be elsewhere.
|
We need to check with owners in each area we are touching that they are in agreement with relaxing this validation, can we blame on the authors and work out who we need to be talking to? |
499880b to
1394f97
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@machineconfiguration/v1/types_machineosconfig.go`:
- Line 206: The XValidation rule on the machine OS config image name still
allows empty path components and an empty final image name. Update the regex in
the kubebuilder validation for the OCI image field so the repeated
namespace/path segment and terminal name both require non-empty components, and
keep the host[:port][/namespace[/namespace...]]/name shape enforced in the
validation attached to the MachineOSConfig types. This should make malformed
refs like registry.example.com/@sha256:... and
registry.example.com//image@sha256:... fail validation.
In
`@payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yaml`:
- Around line 258-260: The validation for the image name in the CRD is still
using the old single-namespace pattern and allows empty path segments. Update
the upstream marker used by the schema generation so the rule enforces non-empty
repeated path segments, then regenerate the CRD so both the v1 and v1alpha1
versions pick up the same corrected message and rule. Use the existing
image-name validation marker in the relevant API type as the source of truth
rather than patching only the generated YAML.
In `@payload-manifests/crds/0000_80_machine-config_01_machineosbuilds.crd.yaml`:
- Around line 319-321: The digestedImagePushSpec documentation and validation
are out of sync: the field comment still describes only a single optional
namespace while the rule in the CRD now allows nested path segments and empty
name/path components. Update the source comment for digestedImagePushSpec to
match the actual host[:port][/namespace[/namespace...]]/name behavior, and
tighten the validation in the related rule so repeated path segments are
non-empty before regenerating the CRD.
In `@payload-manifests/crds/0000_80_machine-config_01_machineosconfigs.crd.yaml`:
- Around line 272-274: The generated validation docs for currentImagePullSpec
are stale and the rule still allows empty path/name segments; update the Go
field comment so it matches the nested namespace wording, then regenerate the
CRD from the currentImagePullSpec source so the validation rule uses a non-empty
repeated path segment pattern instead of permitting empty components.
🪄 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), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 60121916-d623-4b80-bf29-78e6cfa50734
⛔ Files ignored due to path filters (32)
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineosbuilds.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineosconfigs.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-Hypershift.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-SelfManagedHA.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_pinnedimagesets.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.featuregated-crd-manifests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/machineconfignodes.machineconfiguration.openshift.io/ImageModeStatusReporting.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/machineconfignodes.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/machineosbuilds.machineconfiguration.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/machineosconfigs.machineconfiguration.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/osimagestreams.machineconfiguration.openshift.io/OSStreams.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/pinnedimagesets.machineconfiguration.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-Hypershift.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-SelfManagedHA.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1alpha1/zz_generated.featuregated-crd-manifests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1alpha1/zz_generated.featuregated-crd-manifests/osimagestreams.machineconfiguration.openshift.io/OSStreams.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1alpha1/zz_generated.featuregated-crd-manifests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (26)
machineconfiguration/v1/tests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlmachineconfiguration/v1/tests/machineconfignodes.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlmachineconfiguration/v1/tests/osimagestreams.machineconfiguration.openshift.io/OSStreams.yamlmachineconfiguration/v1/types_internalreleaseimage.gomachineconfiguration/v1/types_machineconfignode.gomachineconfiguration/v1/types_machineosconfig.gomachineconfiguration/v1alpha1/tests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlmachineconfiguration/v1alpha1/tests/osimagestreams.machineconfiguration.openshift.io/OSStreams.yamlmachineconfiguration/v1alpha1/types.gomachineconfiguration/v1alpha1/types_internalreleaseimage.gooperator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yamloperator/v1alpha1/types_clusterapi.gopayload-manifests/crds/0000_30_cluster-api_01_clusterapis.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineosbuilds.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineosconfigs.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_osimagestreams-Hypershift.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_osimagestreams-SelfManagedHA.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_pinnedimagesets.crd.yaml
✅ Files skipped from review due to trivial changes (2)
- machineconfiguration/v1/tests/osimagestreams.machineconfiguration.openshift.io/OSStreams.yaml
- machineconfiguration/v1alpha1/tests/osimagestreams.machineconfiguration.openshift.io/OSStreams.yaml
🚧 Files skipped from review as they are similar to previous changes (20)
- payload-manifests/crds/0000_80_machine-config_01_osimagestreams-SelfManagedHA.crd.yaml
- machineconfiguration/v1alpha1/tests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yaml
- machineconfiguration/v1/types_internalreleaseimage.go
- machineconfiguration/v1/tests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yaml
- payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yaml
- operator/v1alpha1/types_clusterapi.go
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-TechPreviewNoUpgrade.crd.yaml
- machineconfiguration/v1alpha1/types_internalreleaseimage.go
- payload-manifests/crds/0000_80_machine-config_01_pinnedimagesets.crd.yaml
- operator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yaml
- machineconfiguration/v1alpha1/types.go
- payload-manifests/crds/0000_80_machine-config_01_osimagestreams-Hypershift.crd.yaml
- payload-manifests/crds/0000_30_cluster-api_01_clusterapis.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-CustomNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-TechPreviewNoUpgrade.crd.yaml
- machineconfiguration/v1/types_machineconfignode.go
- machineconfiguration/v1/tests/machineconfignodes.machineconfiguration.openshift.io/NoRegistryClusterInstall.yaml
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-CustomNoUpgrade.crd.yaml
| // +kubebuilder:validation:MaxLength=447 | ||
| // +kubebuilder:validation:XValidation:rule=`(self.split('@').size() == 2 && self.split('@')[1].matches('^sha256:[a-f0-9]{64}$'))`,message="the OCI Image reference must end with a valid '@sha256:<digest>' suffix, where '<digest>' is 64 characters long" | ||
| // +kubebuilder:validation:XValidation:rule=`(self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]*?$'))`,message="the OCI Image name should follow the host[:port][/namespace]/name format, resembling a valid URL without the scheme" | ||
| // +kubebuilder:validation:XValidation:rule=`(self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)*[a-zA-Z0-9-_.]*?$'))`,message="the OCI Image name should follow the host[:port][/namespace[/namespace...]]/name format, resembling a valid URL without the scheme" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Tighten the path regex to reject empty components.
Line 206 now documents /name as required, but the rule still accepts empty path components and an empty image name because of {0,61} plus the final *. Use the intended repeated non-empty segment pattern so malformed refs like registry.example.com/@sha256:... and registry.example.com//image@sha256:... fail validation.
Proposed fix
-// +kubebuilder:validation:XValidation:rule=`(self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)*[a-zA-Z0-9-_.]*?$'))`,message="the OCI Image name should follow the host[:port][/namespace[/namespace...]]/name format, resembling a valid URL without the scheme"
+// +kubebuilder:validation:XValidation:rule=`(self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?(/[a-zA-Z0-9-_.]+)+$'))`,message="the OCI Image name should follow the host[:port][/namespace[/namespace...]]/name format, resembling a valid URL without the scheme"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // +kubebuilder:validation:XValidation:rule=`(self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)*[a-zA-Z0-9-_.]*?$'))`,message="the OCI Image name should follow the host[:port][/namespace[/namespace...]]/name format, resembling a valid URL without the scheme" | |
| // +kubebuilder:validation:XValidation:rule=`(self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?(/[a-zA-Z0-9-_.]+)+$'))`,message="the OCI Image name should follow the host[:port][/namespace[/namespace...]]/name format, resembling a valid URL without the scheme" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@machineconfiguration/v1/types_machineosconfig.go` at line 206, The
XValidation rule on the machine OS config image name still allows empty path
components and an empty final image name. Update the regex in the kubebuilder
validation for the OCI image field so the repeated namespace/path segment and
terminal name both require non-empty components, and keep the
host[:port][/namespace[/namespace...]]/name shape enforced in the validation
attached to the MachineOSConfig types. This should make malformed refs like
registry.example.com/@sha256:... and registry.example.com//image@sha256:... fail
validation.
| - message: the OCI Image name should follow the host[:port][/namespace[/namespace...]]/name | ||
| format, resembling a valid URL without the scheme | ||
| rule: (self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]*?$')) | ||
| rule: (self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)*[a-zA-Z0-9-_.]*?$')) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Regenerate this validation with non-empty path segments for every served version.
The v1 rule still accepts empty names/components for the same reason as the Go marker, and this CRD’s v1alpha1 schema below still has the old single-namespace rule/message. Fix the upstream marker to use non-empty repeated path segments and regenerate both versions.
Proposed generated-rule shape
- rule: (self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)*[a-zA-Z0-9-_.]*?$'))
+ rule: (self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?(/[a-zA-Z0-9-_.]+)+$'))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - message: the OCI Image name should follow the host[:port][/namespace[/namespace...]]/name | |
| format, resembling a valid URL without the scheme | |
| rule: (self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]*?$')) | |
| rule: (self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)*[a-zA-Z0-9-_.]*?$')) | |
| - message: the OCI Image name should follow the host[:port][/namespace[/namespace...]]/name | |
| format, resembling a valid URL without the scheme | |
| rule: (self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?(/[a-zA-Z0-9-_.]+)+$')) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yaml`
around lines 258 - 260, The validation for the image name in the CRD is still
using the old single-namespace pattern and allows empty path segments. Update
the upstream marker used by the schema generation so the rule enforces non-empty
repeated path segments, then regenerate the CRD so both the v1 and v1alpha1
versions pick up the same corrected message and rule. Use the existing
image-name validation marker in the relevant API type as the source of truth
rather than patching only the generated YAML.
| - message: the OCI Image name should follow the host[:port][/namespace[/namespace...]]/name | ||
| format, resembling a valid URL without the scheme | ||
| rule: (self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]*?$')) | ||
| rule: (self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)*[a-zA-Z0-9-_.]*?$')) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep digestedImagePushSpec docs and validation aligned.
Line 309 still documents only [/namespace], while this message now advertises nested namespaces. The rule also permits empty path/name segments. Update the source field comment and use non-empty repeated path segments before regenerating this CRD.
Proposed generated-rule shape
- rule: (self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)*[a-zA-Z0-9-_.]*?$'))
+ rule: (self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?(/[a-zA-Z0-9-_.]+)+$'))As per coding guidelines, validation constraints must be documented in field comments and enforced by validation rules.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@payload-manifests/crds/0000_80_machine-config_01_machineosbuilds.crd.yaml`
around lines 319 - 321, The digestedImagePushSpec documentation and validation
are out of sync: the field comment still describes only a single optional
namespace while the rule in the CRD now allows nested path segments and empty
name/path components. Update the source comment for digestedImagePushSpec to
match the actual host[:port][/namespace[/namespace...]]/name behavior, and
tighten the validation in the related rule so repeated path segments are
non-empty before regenerating the CRD.
Source: Coding guidelines
| - message: the OCI Image name should follow the host[:port][/namespace[/namespace...]]/name | ||
| format, resembling a valid URL without the scheme | ||
| rule: (self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]*?$')) | ||
| rule: (self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)*[a-zA-Z0-9-_.]*?$')) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Update currentImagePullSpec source docs and reject empty path segments.
The generated description still says host[:port][/namespace]/name, but the validation message now says nested namespaces are supported. The regex also accepts empty path/name components. Update the Go field comment and regenerate with a non-empty repeated segment rule.
Proposed generated-rule shape
- rule: (self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)*[a-zA-Z0-9-_.]*?$'))
+ rule: (self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?(/[a-zA-Z0-9-_.]+)+$'))As per coding guidelines, validation constraints must be documented in field comments and enforced by validation rules.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - message: the OCI Image name should follow the host[:port][/namespace[/namespace...]]/name | |
| format, resembling a valid URL without the scheme | |
| rule: (self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]*?$')) | |
| rule: (self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)*[a-zA-Z0-9-_.]*?$')) | |
| - message: the OCI Image name should follow the host[:port][/namespace[/namespace...]]/name | |
| format, resembling a valid URL without the scheme | |
| rule: (self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?(/[a-zA-Z0-9-_.]+)+$')) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@payload-manifests/crds/0000_80_machine-config_01_machineosconfigs.crd.yaml`
around lines 272 - 274, The generated validation docs for currentImagePullSpec
are stale and the rule still allows empty path/name segments; update the Go
field comment so it matches the nested namespace wording, then regenerate the
CRD from the currentImagePullSpec source so the validation rule uses a non-empty
repeated path segment pattern instead of permitting empty components.
Source: Coding guidelines
1394f97 to
f344405
Compare
|
@andfasano @yuqi-zhang since this changes the parts you added, would you mind taking a look at this? |
Hi @simkam please revert all the IRI related changes (here and here). They belong to the |
I think the bug raised here applies as well to IRI, no? If there's a bug in IRI, we should fix it before we promote |
| // +kubebuilder:validation:MaxLength=447 | ||
| // +kubebuilder:validation:XValidation:rule=`(self.split('@').size() == 2 && self.split('@')[1].matches('^sha256:[a-f0-9]{64}$'))`,message="the OCI Image reference must end with a valid '@sha256:<digest>' suffix, where '<digest>' is 64 characters long" | ||
| // +kubebuilder:validation:XValidation:rule=`(self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]*?$'))`,message="the OCI Image name should follow the host[:port][/namespace]/name format, resembling a valid URL without the scheme" | ||
| // +kubebuilder:validation:XValidation:rule=`(self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)*[a-zA-Z0-9-_.]*?$'))`,message="the OCI Image name should follow the host[:port][/namespace[/namespace...]]/name format, resembling a valid URL without the scheme" |
There was a problem hiding this comment.
Breaking this down:
# SOL anchor
^
# Hostname
([a-zA-Z0-9-]+\.)+ # At least one leading hostname segment, alphanumerics or '-' followed by '.'
[a-zA-Z0-9-]+ # Mandatory final hostname segment
# Optional port
(:[0-9]{2,5})?
# image path
/ # Mandatory leading slash
([a-zA-Z0-9-_]{0,61}/)* # Zero or more path segments, each alphanumeric + '-' or '_' with length between 0 and 61 characters followed by a slash
[a-zA-Z0-9-_.]*? # Optional final path segment, alphanumeric + '-', '_', or '.', no length limit
# EOL anchor
$
The only thing we've changed here is in the image path. The assertion used by be 'zero or one path segments', we've change it to 'zero or more path segments'.
Here's the canonical reference parser: https://github.com/distribution/reference/blob/0965666a6ade2e06035fe352e38344be1e68951a/reference.go#L6-L27
I see many potential pre-existing problems here.
To be clear, I'm the original author of those problems. But it was copy-pasta, so they exist elsewhere in the repo too.
There was a problem hiding this comment.
I traced this regex back to #1773. There's no indication of where this came from.
I'm going to propose we write a new one from scratch based on the canonical reference implementation here: https://github.com/distribution/reference/blob/main/regexp.go.
Attempting to use the reference jargon, we're explicitly matching only the 'name', i.e. excluding tag/digest here. By this reference, I believe that is namePat, except that in namePat specifically domainAndPort is optional. We don't want it to be optional, so I think we want here is:
domainAndPort+`/`+remoteNameI propose we:
- Construct this exact regexp ourselves (manually or just get a clanker to do it)
- Ensure a code comment (but not the published godoc) references exactly where it came from
- Update the tests to cover ipv6 addresses.
f344405 to
b66efaa
Compare
|
As discussed here and offline, the regexp was updated to match the reference implementation and the change is limited only to |
ATM in IRI we don't have any scenario where deeper registry paths are required, they are all in a pretty basic form (originally we build the registry content by mirroring official OCP release + some OLM operators), ie: Whereas I will agree that a more flexible CEL validation rule would be useful (in the past we also had to relax it a little bit to match the specific IRI requirements), this particular case does not seem involving IRI. |
| // It matches: domainAndPort "/" remoteName | ||
| // | ||
| // domainAndPort = (domainName | "[" ipv6 "]") [ ":" port ] | ||
| // domainName = label ("." label)+ — at least one dot required |
There was a problem hiding this comment.
This is a deviation from the reference, which has:
// domainName defines the structure of potential domain components
// that may be part of image names. This is purposely a subset of what is
// allowed by DNS to ensure backwards compatibility with Docker image
// names. This includes IPv4 addresses on decimal format.
domainName = domainNameComponent + `(?:\.` + domainNameComponent + `)*`And I confirm this deviation is reflected in the validation below. Did you do this deliberately? Assuming the reason is good, we should call out the reason for it specifically in this comment.
…eep registry paths https://redhat.atlassian.net/browse/OCPBUGS-91634 The `ImageDigestFormat` CEL validation rejected OCI image references with more than one path segment after the registry host, breaking disconnected environments that mirror images to deep paths like: ``` registry.example.com/team/project/product/component/image@sha256:... ``` The fix rewrites the validation regex based on the canonical OCI distribution reference implementation at: https://github.com/distribution/reference/blob/main/regexp.go Key changes: - Supports multiple path segments (fixes the original bug) - Enforces lowercase-only path components per OCI spec - Adds IPv6 address support in brackets: `[fd00::1]` - IPv4 addresses supported via domain name pattern - Validates domain labels must start/end with alphanumeric (no leading/trailing hyphens) - Supports structured path separators: `.`, `_`, `__`, or `-+` The regex matches: `domainAndPort "/" remoteName` - domainAndPort = (domainName | "[" ipv6 "]") [ ":" port ] - domainName = label ("." label)* - remoteName = pathComponent ("/" pathComponent)* - pathComponent = [a-z0-9]+ separated by ".", "_", "__", or "-+" Added comprehensive test coverage: - IPv4 addresses with/without port - IPv6 addresses with/without port - Deep path validation (5+ segments) - Uppercase rejection in both digest and path components
b66efaa to
1b05df0
Compare
|
/hold cancel |
|
@simkam: This pull request references Jira Issue OCPBUGS-91634, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
|
@simkam: This pull request references Jira Issue OCPBUGS-91634, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@simkam: This pull request references Jira Issue OCPBUGS-91634, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mdbooth 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 |
|
@simkam: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
https://redhat.atlassian.net/browse/OCPBUGS-91634
The
ImageDigestFormatCEL validation rejected OCI image references withmore than one path segment after the registry host, breaking disconnected
environments that mirror images to deep paths like:
The fix rewrites the validation regex based on the canonical OCI
distribution reference implementation at:
https://github.com/distribution/reference/blob/main/regexp.go
Key changes:
[fd00::1].,_,__, or-+The regex matches:
domainAndPort "/" remoteNameAdded comprehensive test coverage: