Rein in message passing to Object in collection config

[chrisarcand]

manageiq-api/lib/api/collection_config.rb

Lines 7 to 13 in c5cc04a

	def [](collection_name)
	# Config::Options implements a system method causing self[:system] to error.
	# i.e. subcollection name system in an arbitraty path /api/automate/manageiq/system
	@cfg[collection_name.to_sym]
	rescue
	nil
	end

As brought up on Gitter by @himdel, we should rein in how we handle the issue fixed by this rescue. We're currently still sending the message (calling the method) unrestricted on the config object, so you can send things like display (which writes to stdout), etc. That's less than ideal.

At the very least, if this is indeed the best solution, I wonder what error is being rescued here.

[himdel]

Traceback of the case where this makes the API output a lot of stuff on any /api access:

 "/home/milan/.rbenv/versions/2.4.1/lib/ruby/2.4.0/ostruct.rb:281:in `inspect'",
 "/home/milan/.rbenv/versions/2.4.1/lib/ruby/gems/2.4.0/gems/config-1.3.0/lib/config/options.rb:135:in `write'",
 "/home/milan/.rbenv/versions/2.4.1/lib/ruby/gems/2.4.0/gems/config-1.3.0/lib/config/options.rb:135:in `display'",
 "/home/milan/.rbenv/versions/2.4.1/lib/ruby/gems/2.4.0/gems/config-1.3.0/lib/config/options.rb:135:in `[]'",
 "/home/milan/src/manage-iq/manageiq-api/lib/api/collection_config.rb:10:in `[]'",
 "/home/milan/src/manage-iq/manageiq-api/lib/api/collection_config.rb:98:in `resource_identifier'",
 "/home/milan/src/manage-iq/manageiq-api/app/controllers/api/base_controller/normalizer.rb:14:in `normalize_hash'",
 "/home/milan/src/manage-iq/manageiq-api/app/controllers/api/base_controller/normalizer.rb:35:in `normalize_attr'",
 "/home/milan/src/manage-iq/manageiq-api/app/controllers/api/base_controller/normalizer.rb:22:in `block in normalize_hash'",
 "/home/milan/src/manage-iq/manageiq-api/app/controllers/api/base_controller/normalizer.rb:21:in `each'",
 "/home/milan/src/manage-iq/manageiq-api/app/controllers/api/base_controller/normalizer.rb:21:in `normalize_hash'",
 "/home/milan/src/manage-iq/manageiq-api/app/controllers/api/base_controller/normalizer.rb:35:in `normalize_attr'",
 "/home/milan/src/manage-iq/manageiq-api/app/controllers/api/base_controller/normalizer.rb:22:in `block in normalize_hash'",
 "/home/milan/src/manage-iq/manageiq-api/app/controllers/api/base_controller/normalizer.rb:21:in `each'",
 "/home/milan/src/manage-iq/manageiq-api/app/controllers/api/base_controller/normalizer.rb:21:in `normalize_hash'",
 "/home/milan/src/manage-iq/manageiq-api/app/controllers/api/base_controller/normalizer.rb:35:in `normalize_attr'",
 "/home/milan/src/manage-iq/manageiq-api/app/controllers/api/base_controller/normalizer.rb:22:in `block in normalize_hash'",
 "/home/milan/src/manage-iq/manageiq-api/app/controllers/api/base_controller/normalizer.rb:21:in `each'",
 "/home/milan/src/manage-iq/manageiq-api/app/controllers/api/base_controller/normalizer.rb:21:in `normalize_hash'",
 "/home/milan/src/manage-iq/manageiq-api/app/controllers/api/base_controller/renderer.rb:79:in `resource_to_jbuilder'",
 "/home/milan/src/manage-iq/manageiq-api/app/controllers/api/base_controller/renderer.rb:17:in `render_resource'",
 "/home/milan/src/manage-iq/manageiq-api/app/controllers/api/api_controller.rb:20:in `index'",

(that write is called by display .. https://ruby-doc.org/core-2.4.2/Object.html#method-i-display)

[abellotti]

ping @jntullo, the outstanding issue we have with the normalizer where the recursion is not calling normalize_hash with the type might be the reason why these exceptions are happening in collection config [] in the first place. Can you take a look ? in which case with your fix we could actually remove the above rescue. Thanks.

[jntullo]

@abellotti I did investigate this, and for the cases that I saw, the type is being passed in, because it is the name of the attribute. Did you have a more specific case that I could take a look at?
edit: think I found one

[abellotti]

remove that rescue and run the automate_specs rspecs, you'll see that exception in action.

[imtayadeway]

@chrisarcand @himdel I think the discussion in #83 (comment) is relevant in here. My proposal is if we merge ManageIQ/manageiq#16138 we can go ahead and remove the calling code that is causing us to send all these messages to Config::Options and only send it sane things i.e. things that we have a reasonable expectation of returning something meaningful.

[himdel]

IMO the problem is a bit further away:

By using Config::Options instead of something else, we're arbitrarily limiting the names of allowed keys in the config. Furthermore, we're not limiting it to a set that's under our control, we're limitting it to a set determined by the ruby standard library and a couple of gems.

That sounds like a recipe for disaster for any use :).

(Unless I'm missing something?)

[chrisarcand]

As I mentioned in the blacklist PR, replacing that gem is very likely not trivial unless @Fryguy can give some other insight. A solution such as the blacklist or what @imtayadeway is describing seems appropriate for now (and perhaps permanently)

[Fryguy]

Yeah, it's non-trivial to replace that gem. config gem has an internal blacklist, and they are pretty good about accepting PRs to update it. I could also see having our own blacklist in Vmdb::Settings, which is easier to update in a pinch, and then we can add some kind of spec that ensures that people don't add keys to the config that are blacklisted.

I don't think this is the API's concern to be doing the blacklisting.

[Fryguy]

I thought I had an open issue for this, but one of my "future plans" with settings.yml is to have some kind of validation on it, so that people touching Advanced Settings can't break the system. So, we would have a way to walk the entire settings, validate against a whitelist of acceptable keys and perhaps values, or value types, as well. Closest I could find was this comment I made a while ago in a BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1358433#c3 .

[jntullo]

@Fryguy the issue isn't that those keys are added to the config, it's that we may be checking to see if display or system exist in the config while we are attempting to normalize the objects. I still think it's a good idea to have a blacklist with specs that ensures the new keys added are valid, but it seems to be a slightly different issue. So, perhaps these blacklisted values can get moved up to the normalizer instead

[Fryguy]

If you are just checking for the presence of a key, then perhaps Settings.to_h.include?(key)

The method to_h just returns a Hash of the keys, which you can then use as a lookup table.