Avoiding duplicated physical provider connection #16952
Conversation
|
@miq-bot add_label gaprindashvili/yes |
|
@miq-bot add_label bug |
|
Hey @agrare, could you please take a look on this? |
|
Hi @agrare Did you have time to review this? |
|
@douglasgabriel how is this related to #16714 ? Seems at least the trailing |
app/models/ext_management_system.rb
Outdated
| existing_hostnames = (self.class.all - [self]).map(&:hostname).compact.map(&:downcase) | ||
|
|
||
| errors.add(:hostname, N_("has to be unique per provider type")) if existing_hostnames.include?(hostname.downcase) | ||
| hostname_without_slash = hostname.downcase.gsub(/\/$/, '') |
There was a problem hiding this comment.
You're doing hostname.downcase.gsub() in a lot of places, can you add a normalized_hostname method that does this in one place?
app/models/ext_management_system.rb
Outdated
| if hostname.ipaddress? | ||
| existing_ipaddress = (self.class.all - [self]).map(&:ipaddress).compact.map { |ipaddress| ipaddress.gsub(/\/$/, '') } | ||
| errors.add(:ipaddress, N_("has to be unique per provider type")) if existing_ipaddress.include?(hostname_without_slash) | ||
| end |
There was a problem hiding this comment.
Under what circumstance would there be an EMS with an :ipaddress property that isn't equal to the :hostname?
There was a problem hiding this comment.
You can bind a provider by the hostname or the ip address, and the ip address field always will be filled with the IP of the provider, in summary if the hostname is an ip address both hostname and ipaddress will be equals, else, will be differents. So when try to add a new provider by its ip address, the system must verify against hostname and ipaddress on the DB to ensure that will avoid duplicated providers.
There was a problem hiding this comment.
and the ip address field always will be filled with the IP of the provider
Is this something the lenovo provider does? Because I don't think this is true across the board, I just tested with a VMware provider through the UI and the ipaddress wasn't filled in.
I also tested creating a provider with just an ipaddress (and no hostname) through a rails console and it threw an exception:
manageiq/app/models/ext_management_system.rb:99:in hostname_format_valid?': undefined method ipaddress?' for nil:NilClass (NoMethodError)
There was a problem hiding this comment.
According with my tests, seems like that others providers doesn't fill the ipaddress field on the refresh routine, however all providers has this field, once that it is attached to an ExtManagementSystem and is shown on view through the "Discovered IP address" field on provider's table, as we can see on the image below:
In the particular case of Lenovo provider, the IP address is filled even the user regist the provider through its hostname, as we can see below:
So, without this verification added on this PR the user would be able to add the provider 10.243.9.123 (for example) again, if inform only its IP address on the hostname input.
douglasgabriel
force-pushed
the
fix_bz_1505610
branch
from
64732f5 to
2033b2a
Compare
|
Checked commit douglasgabriel@2033b2a with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0 |
|
You are right @agrare, there's no need to treat the slash once that |
|
I don't think that this is required, we've covered the majority of cases in your original PR. If someone adds a provider by ipaddress then later by hostname should we nslookup the hostname to try to find if there is one matching the IP address? We can go crazy trying to prevent people from adding a duplicate provider but I don't think it is needed. As long as they can remove the duplicate provider without any impacting the original provider then no harm is really done IMO. |
|
Yeah, I agree with you. And if @rodneyhbrown7 also agree, I can just close this PR |
|
@douglasgabriel we can close this PR |
This PR fix:
https://bugzilla.redhat.com/show_bug.cgi?id=1505610
This PR: