[ops_controller/settings/zones.rb] Use nested auth attrs

[NickLaMuro]

Takes changes from ManageIQ/manageiq#21047 and implements them in the controller. MUST BE MERGED WITH THAT PR

Links

• [WIP][ZonesController] Allow authentications to be created/edited via a single zone update manageiq-api#916
• [Zone] accepts_nested_attributes_for :authentication manageiq#21047

[miq-bot]

Checked commit NickLaMuro@305fc17 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 🏆

[bdunne]

👎🏻 I think this is the wrong direction and we should move toward allowing more authentications as mentioned in ManageIQ/manageiq-api#1014 (comment)

[NickLaMuro]

@bdunne I disagree... and don't really get where you are coming from...

There was comments in that discussion like this:

ManageIQ/manageiq-api#898 (comment)

I really don't understand zones having authentications...I think this might be a legacy thing that maybe we should remove... it was added by (ironically) @chessbyte in 2011 here:

Which is where most of the rest of that conversation was heading.

Now, if you were referring to this bit:

ManageIQ/manageiq-api#898 (comment)

Talked with @NickLaMuro and there already is a precedent for an authentications subcollection, so thinking that might be best. Is it possible to create a parent record and include subcollection records in a single shot? If so, that would seem to be the best of both worlds.

That was referring to authentications being a subcollection for other resources in general, not specifically to the Zone record. In general, with that conversation, I was trying to make sure we weren't adding custom one-off code to support this request for Zones that is making the API non-standard in a bunch of places, since I expect that RFEs like this wouldn't be a one time thing.

The PR that originally came out of that was this one:

ManageIQ/manageiq-api#916

And that had a number of flaws and a bunch of hackery to code that is mostly unused to make it work. It also exposed a bunch of flaws in the routes in the API that I was just not at liberty to fix and was completely out of scope, so using allow_nested_attributes_for was a much more succinct standard going forward that I think requires less code overall to support (since that is built into Rails).

---

So from my perspective, I only see Zone#authentications as relic code from 2011, and zero people suggesting that this is something we should add more authentications types to, unless there is a feature request out there somewhere that I am unaware of.

[bdunne]

ManageIQ/manageiq-api#898 (comment) mentions that this is used for scanning.
ManageIQ/manageiq-api#898 (comment) mentions adding SSH credentials.

Since this credential isn't used by the zone I think it's best to keep the windows_domain context and not make it appear as though this is a default credential for the zone.

[NickLaMuro]

@bdunne The two things you are mentioning are taken out of context, and are bad reasons to be pointing to for expanding credentials:

ManageIQ/manageiq-api#898 (comment) mentions adding SSH credentials.

He was only stating there is other places in code base where this is being done differently with SSH credentials, not that we should add things to Zone:

Strangely for individual VMs and hosts for SSH access we require a user to enter it on each individual machine. I like the idea of not needing a user to enter credentials more than once, but perhaps there's a better pattern we can do here for both windows and ssh creds.

The previous sentence above basically confirms this is the case:

Yeah, I'm thinking "there was nowhere else to hang it off of", which isn't a great reason.

And more importantly, the reason this is being discussed is that we need a way to expose this in the API that also doesn't take two API calls to do (per the request), which is what my core PR is trying to solve.

ManageIQ/manageiq-api#898 (comment) mentions that this is used for scanning.

It is, but it is specifically for "Windows VMs" to collect running processes, and the validation basically prove that this is a feature that doesn't exist for anything but Windows VMs:

https://github.com/ManageIQ/manageiq/blob/dbcbefa3a684e3ad30faa15a1e2e653e93c2c963/app/models/vm/operations.rb#L65-L96

And since that method, or any one of the other 2 places in the entire ManageIQ/* code base, basically never call Zone#authentications outside of a Windows Context (and one of the 2 is now dead code), I don't think that is a good reason to expand this feature.

---

This is an undocumented, and probably underutilized (if used at all) feature that just happens to be a roadblock for the React UI conversion.

Bottom line... I am not changing how anything is working here, just how it is defined, and moving that definition from out of the controllers (which is NOT where it should live), to the model, where manageiq-api can also make use of it, or anything else for that matter. The change in this PR just adjusts itself to work with that new refactoring, that is all.