Adding hostname format validation #16714
Conversation
douglasgabriel
force-pushed
the
ipr_hostname_validation
branch
from
4327009
to
c259d40
Compare
lib/validators/hostname.rb
Outdated
| IS_HOSTNAME = /^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])$/ | ||
|
|
||
| def self.ipv4_or_ipv6?(hostname) | ||
| (hostname =~ IS_IPV4_OR_IPV6) ? true : false |
There was a problem hiding this comment.
You can use hostname.ipaddress? instead of adding the regex here.
See https://github.com/ManageIQ/more_core_extensions/blob/master/lib/more_core_extensions/core_ext/string/formats.rb#L28 if there are any missing edge cases.
lib/validators/hostname.rb
Outdated
| end | ||
|
|
||
| def self.valid_hostname?(hostname) | ||
| (hostname =~ IS_HOSTNAME) ? true : false |
There was a problem hiding this comment.
Can you use .domain_name? instead? https://github.com/ManageIQ/more_core_extensions/blob/master/lib/more_core_extensions/core_ext/string/formats.rb#L12
Again, if there are any other cases not handled in more_core_extensions, I'd prefer to fix / add them there for others to use.
There was a problem hiding this comment.
Well, I'm sorry, I didn't know this file. But I believe that I must update this file adding the validation for hostname, this because maybe we want to deal with domain names and hostnames in a different way.
I mean, maybe we want to domain names looks like "subdomain.domain" (as the regex that you put here said) and the hostname looks more flexible so can assume values like "h", "hostname", "host.name" and so on.
We can see more about this discussion in some places:
- https://tools.ietf.org/html/rfc1034#section-3.1
- https://support.suso.com/supki/What_is_the_difference_between_a_hostname_and_a_domain_name
What do you think?
douglasgabriel
force-pushed
the
ipr_hostname_validation
branch
3 times, most recently
from
0fb13be
to
ba0b230
Compare
douglasgabriel
changed the title
douglasgabriel
changed the title
| @@ -92,6 +92,12 @@ def hostname_uniqueness_valid? | |||
| errors.add(:hostname, N_("has to be unique per provider type")) if existing_hostnames.include?(hostname.downcase) | |||
| end | |||
|
|
|||
| def hostname_format_valid? | |||
| return if hostname.ipaddress? || hostname.hostname? | |||
There was a problem hiding this comment.
It would be a good idea to update the Gemfile setting more_core_extensions to '~> 3.5'https://github.com/ManageIQ/manageiq/blob/master/Gemfile#L49
douglasgabriel
force-pushed
the
ipr_hostname_validation
branch
from
ba0b230
to
2cd6878
Compare
douglasgabriel
changed the title
app/models/ext_management_system.rb
Outdated
| def hostname_format_valid? | ||
| return if hostname.ipaddress? || hostname.hostname? | ||
| error_msg = N_("is in a wrong format.") | ||
| errors.add(:hostname, _(error_msg)) |
There was a problem hiding this comment.
I think you want:
errors.add(:hostname, _("format is invalid."))Is that correct @mzazrivec ?
https://github.com/ManageIQ/guides/blob/master/i18n.md#ruby--haml
douglasgabriel
force-pushed
the
ipr_hostname_validation
branch
from
2cd6878
to
7697bbe
Compare
|
Checked commit douglasgabriel@7697bbe with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0 |
Adding hostname format validation (cherry picked from commit 9d63cd7)
|
Gaprindashvili backport details: |
|
This PR breaks ui-classic travis. Failure: https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/326581784#L4126 Should be fixed by ManageIQ/manageiq-ui-classic#3205 |
ManageIQ/manageiq#16714 Caused the specs which use invalid hostnames to fail in different ways then expected. The test checking for nil ems hostname is now invalid because that isn't possible and the other changed the hostname to something with a space which can be replaced with a '-'
|
Looks like this broke the VMware tests also, ManageIQ/manageiq-providers-vmware#173 |
This PR is based on this discussion:
ManageIQ/manageiq-ui-classic#2455
This PR depends on:
ManageIQ/more_core_extensions#58
This PR extends the UI validation to the backend model.
This PR is able to: