Only use JSON-safe values in settings

[cben]

Looks like we're close enough to it already, easier than alternatives?
https://bugzilla.redhat.com/show_bug.cgi?id=1558031

orig = Settings.to_hash
tripped = JSON.parse(orig.to_json, symbolize_names: true)

def diff(a, b, path = [])
  if a.class == Hash && b.class == Hash
    raise "#{path}: #{a.keys} != #{b.keys}" if a.keys != b.keys
    a.keys.each {|k| diff(a[k], b[k], path + [k])}
  else
    puts "#{path}: #{a.inspect} != #{b.inspect}" if a != b
  end
  nil
end

[29] pry(main)> diff(orig, tripped, [])
[:ems, :ems_azure, :api_versions, :availability_set]: Fri, 01 Dec 2017 != "2017-12-01"
[:ems, :ems_azure, :api_versions, :ip_address]: Wed, 01 Nov 2017 != "2017-11-01"
[:ems, :ems_azure, :api_versions, :load_balancer]: Wed, 01 Nov 2017 != "2017-11-01"
[:ems, :ems_azure, :api_versions, :managed_image]: Fri, 01 Dec 2017 != "2017-12-01"
[:ems, :ems_azure, :api_versions, :network_interface]: Wed, 01 Nov 2017 != "2017-11-01"
[:ems, :ems_azure, :api_versions, :network_security_group]: Wed, 01 Nov 2017 != "2017-11-01"
[:ems, :ems_azure, :api_versions, :resource]: Tue, 01 Aug 2017 != "2017-08-01"
[:ems, :ems_azure, :api_versions, :resource_group]: Tue, 01 Aug 2017 != "2017-08-01"
[:ems, :ems_azure, :api_versions, :storage_account]: Sun, 01 Oct 2017 != "2017-10-01"
[:ems, :ems_azure, :api_versions, :storage_disk]: Thu, 30 Mar 2017 != "2017-03-30"
[:ems, :ems_azure, :api_versions, :template_deployment]: Tue, 01 Aug 2017 != "2017-08-01"
[:ems, :ems_azure, :api_versions, :virtual_machine]: Fri, 01 Dec 2017 != "2017-12-01"
[:ems, :ems_azure, :api_versions, :virtual_network]: Wed, 01 Nov 2017 != "2017-11-01"
[:ems_refresh, :azure, :inventory_collections, :saver_strategy]: :default != "default"
[:ems_refresh, :azure_network, :inventory_collections, :saver_strategy]: :default != "default"
[:ems_refresh, :kubernetes, :inventory_collections, :saver_strategy]: :batch != "batch"
[:ems_refresh, :ec2, :inventory_collections, :saver_strategy]: :batch != "batch"
[:ems_refresh, :ec2_network, :inventory_collections, :saver_strategy]: :batch != "batch"
[:ems_refresh, :s3, :inventory_collections, :saver_strategy]: :batch != "batch"
[:ems_refresh, :ec2_ebs_storage, :inventory_collections, :saver_strategy]: :batch != "batch"
[:ems_refresh, :openshift, :inventory_collections, :saver_strategy]: :batch != "batch"
[:workers, :worker_base, :queue_worker_base, :ems_metrics_collector_worker, :defaults, :poll_method]: :escalate != "escalate"
[:workers, :worker_base, :queue_worker_base, :ems_refresh_worker, :defaults, :poll_method]: :normal != "normal"
[:workers, :worker_base, :queue_worker_base, :defaults, :dequeue_method]: :drb != "drb"
[:workers, :worker_base, :queue_worker_base, :defaults, :poll_method]: :normal != "normal"
[:workers, :worker_base, :queue_worker_base, :ems_metrics_processor_worker, :poll_method]: :escalate != "escalate"
[:workers, :worker_base, :defaults, :poll_method]: :normal != "normal"
[:server, :worker_monitor, :kill_algorithm, :name]: :used_swap_percent_gt_value != "used_swap_percent_gt_value"
[:server, :worker_monitor, :start_algorithm, :name]: :used_swap_percent_lt_value != "used_swap_percent_lt_value"

So:

• some yaml dates in azure — could use strings like "2017-12-01" easily — Convert api-version strings to actual strings manageiq-providers-azure#236
• saver_strategy — Accept non symbol values from settings #17168, Change saver_strategy value to String manageiq-providers-azure#235, Change saver_strategy value to String manageiq-providers-kubernetes#246
• couple worker_base settings
• couple worker_monitor settings

I think we want to update each settings.yml, to use strings for these places, but keep code accepting either, in case any users modified these to another date/symbol.

Either that or:

• database migration to "fix" any settings where a user has already changed the value to a symbol.

When done,

• add test that JSON.parse(Settings.to_hash.to_json, symbolize_names: true) == Settings.to_hash.

@miq-bot assign Fryguy

[Fryguy]

Merged #17168, so one down.

[Fryguy]

@djberg96 Can you comment on what we use the dates for in the ems_azure section? If we can convert those to simple strings, that would knock out one of the potential issues here.

[djberg96]

Those are used by the azure-armrest library, which is ultimately part of the REST API for Azure. That string is passed along as a param with each request, and different services have different api-version strings.

Occasionally Microsoft changes the underlying behavior and updates the api-version strings. Since our backend code might not be able to handle the change, we lock them down on our end.

[Fryguy]

@djberg96 My question is more specific to the object types being stored. It sounds like the Azure API is expecting strings, so we should probably store strings in our yaml...right now Date objects are being stored (see below), which can cause issues with JSON roundtripping (e.g. when someone gets the settings via the REST API, then POSTs back to the REST API with change).

[9] pry(main)> ::Settings.ems.ems_azure.api_versions.to_hash.values.map(&:class)
=> [Date, Date, Date, Date, Date, Date, Date, Date, Date, Date, Date, Date, Date, Date]

[djberg96]

@Fryguy I'm fine with changing them to strings since they're just passed along as strings.

[Fryguy]

@Ladas Seems there is another possible symbol key in the saver area called :inventory_object_saving_strategy

[Ladas]

@Fryguy yeah, that is also covered in https://github.com/Ladas/manageiq/blob/2d0451184f95bb7796243efb08238b3ef651a516/app/models/manager_refresh/save_inventory.rb#L17

[cben]

BZ is marked POST but a few more symbols remain (UPDATED Nov 2018):

[4] pry(main)> diff(orig, tripped, [])
[:ems, :ems_nuage, :event_handling, :event_groups, :addition, :critical]: [/^nuage_.+_create$/] != ["(?-mix:^nuage_.+_create$)"]
[:ems, :ems_nuage, :event_handling, :event_groups, :deletion, :critical]: [/^nuage_.+_delete$/] != ["(?-mix:^nuage_.+_delete$)"]
[:ems, :ems_nuage, :event_handling, :event_groups, :update, :critical]: [/^nuage_.+_update$/] != ["(?-mix:^nuage_.+_update$)"]
[:ems, :ems_nuage, :event_handling, :event_groups, :status, :critical]: [/^nuage_alarm_.+$/] != ["(?-mix:^nuage_alarm_.+$)"]
[:ems, :ems_redfish, :event_handling, :event_groups, :general, :critical]: [/^redfish_/] != ["(?-mix:^redfish_)"]
[:workers, :worker_base, :queue_worker_base, :ems_metrics_collector_worker, :defaults, :poll_method]: :escalate != "escalate"
[:workers, :worker_base, :queue_worker_base, :ems_refresh_worker, :defaults, :poll_method]: :normal != "normal"
[:workers, :worker_base, :queue_worker_base, :ems_refresh_worker, :defaults, :dequeue_method]: :sql != "sql"
[:workers, :worker_base, :queue_worker_base, :defaults, :dequeue_method]: :drb != "drb"
[:workers, :worker_base, :queue_worker_base, :defaults, :poll_method]: :normal != "normal"
[:workers, :worker_base, :queue_worker_base, :ems_metrics_processor_worker, :poll_method]: :escalate != "escalate"
[:workers, :worker_base, :defaults, :poll_method]: :normal != "normal"
[:server, :worker_monitor, :kill_algorithm, :name]: :used_swap_percent_gt_value != "used_swap_percent_gt_value"
[:server, :worker_monitor, :start_algorithm, :name]: :used_swap_percent_lt_value != "used_swap_percent_lt_value"

[miq-bot]

This issue has been automatically marked as stale because it has not been updated for at least 6 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions!

[cben]

@miq-bot remove-label stale
ManageIQ/manageiq-providers-amazon#498
And I want to add a test that the full thing round-trips...

[cben]

@miha-plesko @tadeboro you added the regexp support in #17295 and the specific regexp settings it in ManageIQ/manageiq-providers-nuage#119, ManageIQ/manageiq-providers-redfish#34.

The problem with regexps in Settings is that ManageIQ has a RESP API for settings which represents full settings as JSON, and JSON doesn't have regexp type. Merely reading then writing settings via the API turns them into strings:

Diff:
@@ -487,10 +487,10 @@
     {:blacklisted_event_names=>[],
      :event_handling=>
       {:event_groups=>
-        {:addition=>{:critical=>[/^nuage_.+_create$/]},
-         :deletion=>{:critical=>[/^nuage_.+_delete$/]},
-         :update=>{:critical=>[/^nuage_.+_update$/]},
-         :status=>{:critical=>[/^nuage_alarm_.+$/]}}}},
+        {:addition=>{:critical=>["(?-mix:^nuage_.+_create$)"]},
+         :deletion=>{:critical=>["(?-mix:^nuage_.+_delete$)"]},
+         :update=>{:critical=>["(?-mix:^nuage_.+_update$)"]},
+         :status=>{:critical=>["(?-mix:^nuage_alarm_.+$)"]}}}},
    :ems_openshift=>
     {:blacklisted_event_names=>[], :event_handling=>{:event_groups=>nil}},
    :ems_openstack=>
@@ -633,7 +633,8 @@
      :connection_manager=>{:purge_interval=>"1.hour"}},
    :ems_redfish=>
     {:blacklisted_event_names=>[],
-     :event_handling=>{:event_groups=>{:general=>{:critical=>[/^redfish_/]}}}},
+     :event_handling=>
+      {:event_groups=>{:general=>{:critical=>["(?-mix:^redfish_)"]}}}},
    :ems_scvmm=>
     {:blacklisted_event_names=>[], :event_handling=>{:event_groups=>nil}},
    :ems_vmware=>
... [omitting other diffs from symbols becoming strings] ...

What do you think? Could we always store regexps in string form? Or do you ability to distinguish regexp from simple string?

[miha-plesko]

Ouch, thanks for noticing @cben . So currently we are able to distinguish between regex and plain string just directly by type to know what kind of comparison to perform, see here; it's typ == event_type vs typ.match(event_type).

Perhaps we could workaround it like you say, by passing also regexes as strings that we would distinguish by /^ prefix and $/ suffix. When such prefix+suffix are detected, we compile it as regex, else we compare as plain string.

It should work this way, right @tadeboro ?

[tadeboro]

Storing regular expressions in string form is something I would prefer to avoid. There are two main reasons for this:

1. When settings get loaded, yaml loader automatically verifies the regular expression validity. This makes sure we have a valid regular expression early on the ManageIQ startup process.
2. Regular expressions are parsed and constructed only when the settings are loaded and not on each use.

Maybe api can perform some serialization and deserialization of regular expressions when requests are handled?

[miq-bot]

This issue has been automatically marked as stale because it has not been updated for at least 6 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions!

[JPrause]

@miq-bot remove_label stale
@miq-bot add_label pinned