fix endpoint url uniqueness validation, and disable it for cloud providers

[AlexanderZagaynov]

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1658207

[Fryguy]

@agrare @jrafanie Please review.

[jrafanie]

@AlexanderZagaynov Can you describe why we can remove this?

This was added in #12068 for https://bugzilla.redhat.com/show_bug.cgi?id=1382627

Would allow_nil on the validation fix the problem you're trying to solve?

[jrafanie]

cc @bdunne

[AlexanderZagaynov]

@jrafanie as my BZ says, it's impossible to add two or more ems providers with the same proxy URI. However, such option can make sense, if you want to have AWS for some US region, and another one with same proxy for some EU for example.
Even more, it's not a discussion should we have that abilitiy or not. That is a blocker BZ which states clearly that we should.

[AlexanderZagaynov]

P.S. I see that your BZ should also be considered. So, we need to find a way to satisfy both. Any ideas?

[jrafanie]

@jrafanie as my BZ says, it's impossible to add two or more ems providers with the same proxy URI. However, such option can make sense, if you want to have AWS for some US region, and another one with same proxy for some EU for example.
Even more, it's not a discussion should we have that abilitiy or not. That is a blocker BZ which states clearly that we should.

yeah, it's unclear if a nil URL is the only case your BZ needs to be fixed. If yes, allow_nil should resolve it. If not, then we have to be careful not to "break" what @bdunne's BZ was fixing (https://bugzilla.redhat.com/show_bug.cgi?id=1382627)

[AlexanderZagaynov]

@jrafanie sorry, looks like I missed a point in my BZ, that it's not about two endpoints with the same value, but about empty field values. Hovewer, there was already a condition for that validation if: :uri, and that problem didn't arise before, considering that previous change was made quite a lot time ago, and AWS proxy feature was added somewhere in this summer. So, I should ask current BZ author about details.

[agrare]

@AlexanderZagaynov the bz states that the url was left empty:

Probably Endpoint URL is now unique? I left that field empty for both providers.

I just tried this and can confirm leaving the url blank and trying to add two regions fails with that error. I tried adding allow_nil and I was able to validate credentials but was not able to save the provider. Assuming this was fixed though this doesn't help if someone actually wants to use a different endpoint url (either a proxy or a custom region) and add multiple regions.

I've always struggled with preventing people from adding the same provider multiple times, just checking for the same hostname doesn't cover the case where the user wants to add the same provider with different credentials. I've always thought it'd be easier to just warn the user in case they didn't intend to add the same provider again but there's not really any way of really preventing the same provider being added multiple times (can be added multiple times with different IP addresses, dns names, etc... anyway).

I don't think its possible to successfully prevent the same endpoint being added twice in some cases while also allowing it in others.

[AlexanderZagaynov]

@agrare I believe we should not prevent adding same provider multiple times, I showed example above with two regions. Even more, I have had such setup in my dev env not so long ago. And added two AWSes without any problem at that moment. Maybe something was changed recently.
And about different checks for different provider types - it can be easily made with if: :some_checking_method.

[AlexanderZagaynov]

I've found possible reason why that problem did arise. Probably previously empty url field was saved with a nil value, and condition if: :url was working. But now I see that it saves with an empty string in that field, and that condition doesn't apply. I've added allow_blank: true to make it work in both cases, and disabled it for cloud managers.

@jrafanie @agrare please review again

[JPrause]

@miq-bot add_label blocker

[JPrause]

@AlexanderZagaynov if this will be able to be backported, can you add the hammer/yes label.

[AlexanderZagaynov]

@miq-bot add_label hammer/yes

[Fryguy]

One specific reason we had prevented people from adding the same provider multiple times was in places where we "resurrect" archived VMs. For example, in VMware, if a VM moves from one provider to another, we find it in the archived list and reattach it to the new provider (e.g. the user is migrating from one physical vSphere to another, and transfers the underlying disks to a different storage and then attaches it to the new provider). If we didn't have the uniqueness in place, then this mechanism causes the VM to jump back and forth between the systems. Now, one can argue that we shouldn't have this feature in place, and I'd be ok with discussing removal of that, but the reality at the moment is that this feature exists.

EDIT: The VM won't jump back and forth, as we only pull from the same EMS or ems_id.nil?

[AlexanderZagaynov]

@Fryguy we speak about endpoints here, not about same providers

[agrare]

we speak about endpoints here, not about same providers

Provider uniqueness is handled by the endpoints since ext_management_system doesn't actually have a hostname/url it delegates that to the default endpoint.

[AlexanderZagaynov]

in that case we have bad design I guess. but let's stay with simple workarounds for now :)

[miq-bot]

Checked commits AlexanderZagaynov/manageiq@f17d894~...ab9cc07 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. ⭐

[agrare]

in that case we have bad design I guess. but let's stay with simple workarounds for now :)

I blame @juliancheal since he did the endpoint stuff 😆

Seriously though multiple endpoints for a provider is a big improvement over how it used to be, and uniquely identifying a provider is a genuinely hard problem that even hostname checks can't accomplish 100% (see my comment here #18298 (comment)) and not every provider has a "uuid" like a vSphere vCenter does which would allow us to do the unique check properly.

[juliancheal]

I blame @juliancheal since he did the endpoint stuff 😆

I tried to avoid this PR, but thanks @agrare 😂

I'm not totally caught up on this PR, but let me know if you need any help 🎩

[agrare]

If we didn't have the uniqueness in place, then this mechanism causes the VM to jump back and forth between the systems

We'd actually be okay here since when reconnecting we look for vms with the same ems_id or nil so an active vm on another provider won't be found (https://github.com/ManageIQ/manageiq/blob/master/app/models/ems_refresh/save_inventory.rb#L102)

[simaishi]

Hammer backport details:

$ git log -1
commit 11c820248cb09df93f3d7b4d66e43eac6bbd1006
Author: Adam Grare <agrare@redhat.com>
Date:   Thu Dec 20 12:29:13 2018 -0500

    Merge pull request #18298 from AlexanderZagaynov/BZ-1658207
    
    fix endpoint url uniqueness validation, and disable it for cloud providers
    
    (cherry picked from commit b8927b0e9ff881f55152967a870a98df22a59c58)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1658207

[Fryguy]

@agrare Ah yeah, I edited my comment but too late 😄