Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

We explicitly serialize objects so we need to permit yaml loading them #512

Merged
merged 1 commit into from
Nov 21, 2023
Merged

We explicitly serialize objects so we need to permit yaml loading them #512

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions spec/models/manageiq/providers/kubernetes/container_manager/scanning/job/dispatcher_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>'

Copy link
Member

@agrare agrare Nov 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Member

@agrare agrare Nov 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@kbrock kbrock Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I'll merge this to unblock 3.1 but this should be an easy fix as a follow-up

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 Added each of the hacks to the list: ManageIQ/manageiq#22795

@jobs = (@vms + @repo_vms).collect(&:raw_scan)
User.current_user = FactoryBot.create(:user)
@jobs += @container_images.map { |img| img.ext_management_system.raw_scan_job_create(img.class, img.id) }
Expand Down