We explicitly serialize objects so we need to permit yaml loading them #512
Conversation
|
Checked commit jrafanie@01424e8 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint |
| @@ -40,6 +40,7 @@ | |||
| context "with container and vms jobs" do | |||
| let(:container_image_classes) { ContainerImage.descendants.collect(&:name).append('ContainerImage') } | |||
| before do | |||
| ActiveRecord::Base.yaml_column_permitted_classes |= [ManageIQ::Providers::Openshift::ContainerManager::ContainerImage] | |||
There was a problem hiding this comment.
Hey @jrafanie where is this failing for you? Worried it isn't just an issue in specs since I don't see any explicit calls to YAML.safe_load in here but I might be missing something obvious
There was a problem hiding this comment.
On my ruby 3.1 branch primarily because I have yaml unsafe disabled: https://github.com/jrafanie/manageiq/blob/ruby31/config/application.rb#L93. It also files with just that setting flipped on master core:
diff --git a/config/application.rb b/config/application.rb
index e1fcc3928b..87a64e2656 100644
--- a/config/application.rb
+++ b/config/application.rb
@@ -90,7 +90,7 @@ module Vmdb
config.action_cable.mount_path = '/ws/notifications'
# Use yaml_unsafe_load for column serialization to handle Symbols
- config.active_record.use_yaml_unsafe_load = true
+ config.active_record.use_yaml_unsafe_load = false
# Customize any additional options below...There was a problem hiding this comment.
it looks like the container images are being stored in the options of the job it's creating:
Psych::DisallowedClass:
Tried to load unspecified class: ManageIQ::Providers::Openshift::ContainerManager::ContainerImage
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/psych-3.3.4/lib/psych/class_loader.rb:99:in `find'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/psych-3.3.4/lib/psych/class_loader.rb:28:in `load'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/psych-3.3.4/lib/psych/visitors/to_ruby.rb:426:in `resolve_class'
# /Users/joerafaniello/Code/manageiq/config/initializers/yaml_autoloader.rb:15:in `resolve_class'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/psych-3.3.4/lib/psych/visitors/to_ruby.rb:95:in `deserialize'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/psych-3.3.4/lib/psych/visitors/to_ruby.rb:130:in `visit_Psych_Nodes_Scalar'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/psych-3.3.4/lib/psych/visitors/visitor.rb:30:in `visit'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/psych-3.3.4/lib/psych/visitors/visitor.rb:6:in `accept'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/psych-3.3.4/lib/psych/visitors/to_ruby.rb:35:in `accept'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/psych-3.3.4/lib/psych/visitors/to_ruby.rb:347:in `block in revive_hash'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/psych-3.3.4/lib/psych/visitors/to_ruby.rb:345:in `each'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/psych-3.3.4/lib/psych/visitors/to_ruby.rb:345:in `each_slice'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/psych-3.3.4/lib/psych/visitors/to_ruby.rb:345:in `revive_hash'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/psych-3.3.4/lib/psych/visitors/to_ruby.rb:169:in `visit_Psych_Nodes_Mapping'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/psych-3.3.4/lib/psych/visitors/visitor.rb:30:in `visit'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/psych-3.3.4/lib/psych/visitors/visitor.rb:6:in `accept'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/psych-3.3.4/lib/psych/visitors/to_ruby.rb:35:in `accept'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/psych-3.3.4/lib/psych/visitors/to_ruby.rb:320:in `visit_Psych_Nodes_Document'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/psych-3.3.4/lib/psych/visitors/visitor.rb:30:in `visit'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/psych-3.3.4/lib/psych/visitors/visitor.rb:6:in `accept'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/psych-3.3.4/lib/psych/visitors/to_ruby.rb:35:in `accept'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/psych-3.3.4/lib/psych.rb:362:in `safe_load'
# /Users/joerafaniello/Code/manageiq/lib/extensions/yaml_load_aliases.rb:7:in `safe_load'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/activerecord-6.1.7.6/lib/active_record/coders/yaml_column.rb:53:in `yaml_load'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/activerecord-6.1.7.6/lib/active_record/coders/yaml_column.rb:26:in `load'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/activerecord-6.1.7.6/lib/active_record/type/serialized.rb:22:in `deserialize'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/activemodel-6.1.7.6/lib/active_model/type/helpers/mutable.rb:8:in `cast'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/activemodel-6.1.7.6/lib/active_model/attribute.rb:175:in `type_cast'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/activemodel-6.1.7.6/lib/active_model/attribute.rb:43:in `value'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/activemodel-6.1.7.6/lib/active_model/attribute_set.rb:42:in `fetch_value'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/activerecord-6.1.7.6/lib/active_record/attribute_methods/read.rb:36:in `_read_attribute'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/activemodel-6.1.7.6/lib/active_model/attribute_methods.rb:254:in `options'
# /Users/joerafaniello/Code/manageiq/app/models/job.rb:47:in `initialize_attributes'
# /Users/joerafaniello/Code/manageiq/app/models/job.rb:25:in `create_job'
# ./app/models/manageiq/providers/kubernetes/container_manager.rb:883:in `raw_scan_job_create'
# ./spec/models/manageiq/providers/kubernetes/container_manager/scanning/job/dispatcher_spec.rb:45:in `block (5 levels) in <main>'
# ./spec/models/manageiq/providers/kubernetes/container_manager/scanning/job/dispatcher_spec.rb:45:in `map'
# ./spec/models/manageiq/providers/kubernetes/container_manager/scanning/job/dispatcher_spec.rb:45:in `block (4 levels) in <main>'
# /Users/joerafaniello/.gem/ruby/3.0.6/gems/webmock-3.19.1/lib/webmock/rspec.rb:39:in `block (2 levels) in <main>'There was a problem hiding this comment.
Okay for a Job target_class should be a string
def target_entity
target_class.constantize.find_by(:id => target_id) if target_class
end
This passes specs:
--- a/app/models/manageiq/providers/kubernetes/container_manager.rb
+++ b/app/models/manageiq/providers/kubernetes/container_manager.rb
@@ -883,7 +883,7 @@ Expecting to find com.redhat.rhsa-RHEL7.ds.xml.bz2 file there.'),
ManageIQ::Providers::Kubernetes::ContainerManager::Scanning::Job.create_job(
:userid => userid,
:name => "Container Image Analysis: '#{target_name}'",
- :target_class => target_class,
+ :target_class => target_class.name,
:target_id => target_id,
There was a problem hiding this comment.
Yeah, I think that'd be a problem outside of specs also unless one of your changes in core works around that generally?
If that's the case then I'm fine to merge this and I'll fix in a follow-up again
There was a problem hiding this comment.
@agrare good find/fix
Regardless of how we handle the unsafe_load, this looks like a good (read: necessary) change.
MiqReport column db is similar and we store a string
There was a problem hiding this comment.
Yes, I think the thought I'm going with is... "these changes demonstrate problems we should solve differently...the solutions here are not final solutions".
We can even remain using unsafe_load for serialized columns as I think there are severe downsides until we can remove some of our yaml serializations, but mainly:
Eager loading nearly our whole app because we don't know all the constants that could be loaded in serialized columns.
By adding these easy to find greppable phrases, it should be easy to come back and try to fix each column on by one.
There was a problem hiding this comment.
Okay I'll merge this to unblock 3.1 but this should be an easy fix as a follow-up
There was a problem hiding this comment.
@agrare Yea. I don't think we should do this at all. Instead, I think we should allow unsafe_load - and also make this change.
There was a problem hiding this comment.
💯 Added each of the hacks to the list: ManageIQ/manageiq#22795
Part of ManageIQ/manageiq#22696