Bug 2039256: kubebuilder validation format "hostname" is not RFC compliant #1111
Conversation
openshift-ci
bot
added
bugzilla/severity-low
|
@candita: This pull request references Bugzilla bug 2039256, which is invalid:
Comment In 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 kubernetes/test-infra repository. |
|
/bugzilla refresh |
|
@candita: This pull request references Bugzilla bug 2039256, which is invalid:
Comment In 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 kubernetes/test-infra repository. |
|
@candita: This pull request references Bugzilla bug 2039256, which is invalid:
Comment In 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 kubernetes/test-infra repository. |
|
/bugzilla refresh |
openshift-ci
bot
added
bugzilla/valid-bug
|
@candita: This pull request references Bugzilla bug 2039256, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: In 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 kubernetes/test-infra repository. |
candita
force-pushed
the
BZ-2039256-componentRoutesHostnameRegex
branch
from
b472575
to
be5cc87
Compare
|
@candita: This pull request references Bugzilla bug 2039256, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In 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 kubernetes/test-infra repository. |
1 similar comment
|
@candita: This pull request references Bugzilla bug 2039256, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In 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 kubernetes/test-infra repository. |
candita
changed the title
|
@candita: This pull request references Bugzilla bug 2039256, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In 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 kubernetes/test-infra repository. |
|
/bugzilla refresh |
openshift-ci
bot
added
bugzilla/severity-high
|
@candita: This pull request references Bugzilla bug 2039256, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In 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 kubernetes/test-infra repository. |
config/v1/types_ingress.go
Outdated
| @@ -91,7 +91,7 @@ type IngressSpec struct { | |||
| type ConsumingUser string | |||
|
|
|||
| // Hostname is an alias for hostname string validation. | |||
| // +kubebuilder:validation:Format=hostname | |||
| // +kubebuilder:validation:Pattern="^([a-zA-Z0-9\\p{S}\\p{L}]((-?[a-zA-Z0-9\\p{S}\\p{L}]{0,62})?)|([a-zA-Z0-9\\p{S}\\p{L}](([a-zA-Z0-9-\\p{S}\\p{L}]{0,61}[a-zA-Z0-9\\p{S}\\p{L}])?)(\\.)){1,}([a-zA-Z0-9-\\p{L}]){2,63})$" | |||
There was a problem hiding this comment.
A few suggestions and observations:
- You can use
``to avoid the need to escape backslashes:
| // +kubebuilder:validation:Pattern="^([a-zA-Z0-9\\p{S}\\p{L}]((-?[a-zA-Z0-9\\p{S}\\p{L}]{0,62})?)|([a-zA-Z0-9\\p{S}\\p{L}](([a-zA-Z0-9-\\p{S}\\p{L}]{0,61}[a-zA-Z0-9\\p{S}\\p{L}])?)(\\.)){1,}([a-zA-Z0-9-\\p{L}]){2,63})$" | |
| // +kubebuilder:validation:Pattern=`^([a-zA-Z0-9\p{S}\p{L}]((-?[a-zA-Z0-9\p{S}\p{L}]{0,62})?)|([a-zA-Z0-9\p{S}\p{L}](([a-zA-Z0-9-\p{S}\p{L}]{0,61}[a-zA-Z0-9\p{S}\p{L}])?)(\.)){1,}([a-zA-Z0-9-\p{L}]){2,63})$` |
- I don't believe we should allow a leading or trailing digit or dash in the TLD:
| // +kubebuilder:validation:Pattern="^([a-zA-Z0-9\\p{S}\\p{L}]((-?[a-zA-Z0-9\\p{S}\\p{L}]{0,62})?)|([a-zA-Z0-9\\p{S}\\p{L}](([a-zA-Z0-9-\\p{S}\\p{L}]{0,61}[a-zA-Z0-9\\p{S}\\p{L}])?)(\\.)){1,}([a-zA-Z0-9-\\p{L}]){2,63})$" | |
| // +kubebuilder:validation:Pattern=`^([a-zA-Z0-9\p{S}\p{L}]((-?[a-zA-Z0-9\p{S}\p{L}]{0,62})?)|([a-zA-Z0-9\p{S}\p{L}](([a-zA-Z0-9-\p{S}\p{L}]{0,61}[a-zA-Z0-9\p{S}\p{L}])?)(\.)){1,}[a-zA-Z\p{L}][a-zA-Z0-9-\p{L}]{0,61}[a-zA-Z\p{L}])$` |
- I think the upstream pattern is incorrect because it does not allow a dash in an unqualified TLD; for example,
foo-bar.tldis accepted, butfoo-baris rejected. Would the API and router admitfoo-bar? If so, we should allow it here too, something like this (this is getting harried and needs heaps of scrutiny):
| // +kubebuilder:validation:Pattern="^([a-zA-Z0-9\\p{S}\\p{L}]((-?[a-zA-Z0-9\\p{S}\\p{L}]{0,62})?)|([a-zA-Z0-9\\p{S}\\p{L}](([a-zA-Z0-9-\\p{S}\\p{L}]{0,61}[a-zA-Z0-9\\p{S}\\p{L}])?)(\\.)){1,}([a-zA-Z0-9-\\p{L}]){2,63})$" | |
| // +kubebuilder:validation:Pattern=`^([a-zA-Z0-9\p{S}\p{L}][a-zA-Z0-9-\p{S}\p{L}]{0,61}[a-zA-Z\p{S}\p{L}]|([a-zA-Z0-9\p{S}\p{L}](([a-zA-Z0-9-\p{S}\p{L}]{0,61}[a-zA-Z0-9\p{S}\p{L}])?)(\.)){1,}[a-zA-Z\p{L}][a-zA-Z0-9-\p{L}]{0,61}[a-zA-Z\p{L}])$` |
-
The Route API validation does not allow upper-case, so I'm not so sure that including the
A-Zcharacter class is appropriate. Punycode should be allowed for IDN, but allowing digits and dashes should suffice for that; I'm not sure we should be using the\p{S}, and\p{L}character classes here. We generally cannot tighten validation of a shipped API, but maybe we can in this case—if a user has an installed cluster with a host name that requires theA-Z,\p{S}, or\p{L}character class for the pattern to match, do the API's and router's validation render the cluster broken anyway? -
I'm beginning to wonder whether it would be better to loosen the validation here to simplify the validation at the cost of potentially allowing users to configure invalid names; the API or router would still ultimately reject the invalid names, so maybe it would suffice to do some basic validation here, and if users give us garbage that's hard to verify is garbage, those users will just have do to a little more legwork to figure out why the garbage input weren't resulting in a working route.
There was a problem hiding this comment.
I agree, the upsteam pattern has a number of issues I did not address in this PR. I tried to minimize changes in fixing the TLD issue, for this shipped API. I can loosen the validation.
There is no Route-style validation on componentRoutes - they are different types. This PR introduces a new controller for ComponentRoutes and only validates RBAC values.
There was a problem hiding this comment.
There was some discussion in openshift/enhancements#577 (comment)
candita
force-pushed
the
BZ-2039256-componentRoutesHostnameRegex
branch
from
be5cc87
to
b632c5f
Compare
|
@candita: This pull request references Bugzilla bug 2039256. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state. In 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 kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: candita The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/reopen |
|
@candita: Failed to re-open PR: state cannot be changed. There are no new commits on the candita:BZ-2039256-componentRoutesHostnameRegex branch. In 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 kubernetes/test-infra repository. |
|
@candita: all tests passed! Full PR test history. Your PR dashboard. 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 kubernetes/test-infra repository. I understand the commands that are listed here. |
We use the go-openapi/strfmt library for the "hostname" kubebuilder validation format in
componentRoutes. This format does not accept numbers or hyphens in the TLD as noted in https://bugzilla.redhat.com/show_bug.cgi?id=2039256#c3.I checked with the openshift/api team and they recommended replacing the hostname format with my own regex, but I have to be careful with downgrades. Therefore I only changed the regex pattern to allow TLD with numbers and hyphen. There are some other problems with the existing regex, but are not addressed in this PR in order to maintain backward compatibility for
componentRoutehostnames.I also checked with go-openapi owners in go-openapi/strfmt#94 about changing the library regex, but the solution they prefer would remove the regex.
Also fixes duplicate bug (with higher priority) https://bugzilla.redhat.com/show_bug.cgi?id=2049473