-
Notifications
You must be signed in to change notification settings - Fork 605
OCPBUGS-82292: extend supported values for MCN IRI image status field #2800
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -211,12 +211,14 @@ type MachineConfigNodeStatusInternalReleaseImageRef struct { | |
| // image is an OCP release image referenced by digest. | ||
| // The format of the image pull spec is: host[:port][/namespace]/name@sha256:<digest>, | ||
| // where the digest must be 64 characters long, and consist only of lowercase hexadecimal characters, a-f and 0-9. | ||
| // The host must be either exactly "localhost" or a dot-qualified domain name. | ||
| // Single-label hosts other than "localhost" are not permitted. | ||
| // The length of the whole spec must be between 1 to 447 characters. | ||
| // The field is optional, and it will be provided after a release will be successfully installed. | ||
| // +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('^(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" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious, why is using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IPv6 will not be accepted with that validation (ie
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I'm wrong but I don't think the original validation rule (coming from |
||
| // +optional | ||
| Image string `json:"image,omitempty"` | ||
| } | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I think I interpreted the documentation change of:
To mean that something like
localhost.*would be considered invalid (either you use exactlylocalhostor a dot-qualified domain name not containinglocalhost. Maybe we can clarify the documentation a bit more to be less prone to incorrect interpretation?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for the sake of clarity I also added
Single-label hosts other than "localhost" are not permitted.. Any other dot-qualified (includedlocalhost.*remain valid, as it was in the previous version of the rule)