OCPBUGS-82292: extend supported values for MCN IRI image status field#2800
OCPBUGS-82292: extend supported values for MCN IRI image status field#2800andfasano wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-important
|
@andfasano: This pull request references Jira Issue OCPBUGS-82292, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
Hello @andfasano! Some important instructions when contributing to openshift/api: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe PR updates image-host validation and test fixtures. Test data for ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
openshift-ci
bot
added
the
size/M
Review Summary by QodoExtend MCN IRI image status field to support localhost registries
WalkthroughsDescription• Extend OCI image hostname validation to support localhost registries • Update regex pattern to accept localhost and localhost.localdomain formats • Add test cases with localhost image references • Propagate validation changes across all CRD manifest files Diagramflowchart LR
A["OCI Image Validation Regex"] -->|Add localhost support| B["Updated Pattern"]
B -->|Applied to| C["Type Definition"]
B -->|Applied to| D["CRD Manifests"]
B -->|Applied to| E["Test Cases"]
C -->|Validates| F["localhost/path@sha256:..."]
C -->|Validates| G["localhost.localdomain/path@sha256:..."]
File Changes1. machineconfiguration/v1/types_machineconfignode.go
|
Code Review by Qodo
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
andfasano
force-pushed
the
iri-fix-mcn-status-field
branch
from
f49e19d to
373ea71
Compare
openshift-ci
bot
added
size/L
|
/jira refresh |
|
@andfasano: This pull request references Jira Issue OCPBUGS-82292, 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. |
|
/retest |
| // +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('^(localhost|([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; host must be either 'localhost' or a dot-qualified domain name" |
There was a problem hiding this comment.
I'm curious, why is using 127.0.0.1:{port} not sufficient?
There was a problem hiding this comment.
IPv6 will not be accepted with that validation (ie ::1 or [::1]:22625). We currently support either ipv4, ipv6 or dualstack, so using localhost will be a nice simplification at this level
There was a problem hiding this comment.
What if you added support for ipv6 to the validation?
I won't block this PR based on the approach, but it seems odd to me to add a localhost label to resolve this.
If you support ipv6 would it ever be reasonable for an end-user to want to be able to specify an ipv6 hostname that is not localhost, much like the current validation would allow for ipv4?
There was a problem hiding this comment.
Maybe I'm wrong but I don't think the original validation rule (coming from ImageDigestFormat and reused in many other resources) was meant to support directly an IP (either v4 or v6) as part of the host section. Neither was our intention to support that.
This field will be consumed only by the MCD IRI manager to check the status on its local node, and it's not meant to be directly used by an end-user.
The end-user will instead look to the (singleton) IRI resource, which will aggregate the various MCNs and will use api-int as a valid pullspec (this latter part currently under implementation)
| message: "" | ||
| lastTransitionTime: "2024-12-01T08:04:30Z" | ||
| - name: ocp-release-bundle-4.20.0-x86_64 | ||
| image: localhost.localdomain/openshift/release-images@sha256:f98795f7932441b30bb8bcfbbf05912875383fad1f2b3be08a22ec148d68607f |
There was a problem hiding this comment.
My understanding based on the field documentation and validation change is that something like this shouldn't be allowed - is that incorrect?
There was a problem hiding this comment.
That should be ok. Will not be accepted no-dot domains (with just the single exception of localhost), domains starting with a dot (.com) or ending with a dot (example.)
There was a problem hiding this comment.
Okay. I think I interpreted the documentation change of:
The host must be either exactly "localhost" or a dot-qualified domain name.
To mean that something like localhost.* would be considered invalid (either you use exactly localhost or a dot-qualified domain name not containing localhost. Maybe we can clarify the documentation a bit more to be less prone to incorrect interpretation?
There was a problem hiding this comment.
Just for the sake of clarity I also added Single-label hosts other than "localhost" are not permitted.. Any other dot-qualified (included localhost.* remain valid, as it was in the previous version of the rule)
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@andfasano: 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. |
3 participants
Required by the IRI MCD manager introduced in openshift/machine-config-operator#5807