vsphere: Validate vcenter user input to match RFC standards#4708
Conversation
|
/assign @staebler |
|
/test e2e-vsphere |
|
/retest |
1 similar comment
|
/retest |
|
/cc jstuever This validation is only being applied to survey. It should also be applied to the install config. |
rna-afk
force-pushed
the
vsphere_vcenter_validation
branch
3 times, most recently
from
8d598fd to
7096efc
Compare
There was a problem hiding this comment.
This check is not quite what we need.
- It is valid for the vCenter hostname to be an IP address.
- We should not validate the length of the hostname.
There was a problem hiding this comment.
I don't think that we need to show this much detail in the error here. The user is not picking a hostname: The user is inputting what should be an existing hostname. This error should say something simpler like
must be the hostname of the vCenter
There was a problem hiding this comment.
Thanks for adding this test?
There was a problem hiding this comment.
Was using that function and it didn't have a test so thought I'd add it.
rna-afk
force-pushed
the
vsphere_vcenter_validation
branch
3 times, most recently
from
a9206b4 to
3be360e
Compare
|
/test e2e-vsphere |
|
/retest |
rna-afk
force-pushed
the
vsphere_vcenter_validation
branch
2 times, most recently
from
c35d506 to
b4662da
Compare
staebler
left a comment
staebler
left a comment
There was a problem hiding this comment.
Looks good. Just two small nits.
There was a problem hiding this comment.
For consistency,
| Help: "The hostname or IP address of the vCenter to be used for installation.", | |
| Help: "The domain name or IP address of the vCenter to be used for installation.", |
There was a problem hiding this comment.
| // Host validates that a given string is either a valid URI host. | |
| // Host validates that a given string is a valid URI host. |
There was a problem hiding this comment.
| return errors.New("cluster name must begin with a lower-case letter") | |
| return errors.New("domain name must begin with a lower-case letter") |
Vcenter input must always only be the hostname yet it is possible for the user to provide other types of input like URLs. URLs would cause an error to occur only during the time of cluster creation as connection to the vcenter would succeed. Adding a validation check for the user input for the vcenter value so that it conforms to the RFC-1035 standard and forces the user to re-enter the value rather than throw an error during cluster creation.
rna-afk
force-pushed
the
vsphere_vcenter_validation
branch
from
b4662da to
7c98e02
Compare
|
/test e2e-vsphere |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: staebler 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-robot
added
the
approved
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
7 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@rna-afk: The following test 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/test-infra repository. I understand the commands that are listed here. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
6 participants
Vcenter input must always only be the hostname yet it is possible
for the user to provide other types of input like URLs. URLs would
cause an error to occur only during the time of cluster creation
as connection to the vcenter would succeed.
Adding a validation check for the user input for the vcenter value
so that it conforms to the RFC-1035 standard and forces the user
to re-enter the value rather than throw an error during cluster
creation.