Support Ruby 3.1 #22698

jrafanie · 2023-09-11T17:54:33Z

TODO:

resolve the YAML safe_load default so we don't need to use unsafe_load by default.
- Try to safe_load, retry with loading aliases (via true) and minimal permitted classes, retry again with aliases and a larger list of permitted classes.
- Add warning logging for disallowed classes so we can build a list of classes we need to allow in the next step
- Enumerate a list of commonly permitted classes, perhaps the list of models + some others, store this and use it with the rails activerecord yaml_column_permitted_classes similar to https://stackoverflow.com/a/75001610
update YAML.load/safe_load using positional permitted_classes to kwargs since positional is gone in psych 4
upgrade qpid_proton to one with ruby 3.1 support
Cross repo test: [BOT] Cross repo test for ManageIQ/manageiq#22698 manageiq-cross_repo-tests#824

Part of #22696

Note, qpid_proton and bumping ruby 3.1 in test has moved to a followup in #22808

jrafanie · 2023-09-11T17:59:54Z

@miq-bot cross-repo-tests /all

From Pull Request: ManageIQ/manageiq#22698

Fryguy · 2023-09-11T21:05:47Z

Gemfile

@@ -55,6 +55,7 @@ gem "manageiq-ssh-util",                "~>0.1.1",           :require => false
 gem "memoist",                          "~>0.16.0",          :require => false
 gem "money",                            "~>6.13.5",          :require => false
 gem "more_core_extensions"                                                     # min version should be set in manageiq-gems-pending, not here
+gem "net-ftp",                          "~>0.1.2",           :require => false


Not sure if this should belong here or manageiq-gems-pending

yeah, it also blew up in gems pending in the same require so they both will need the same solution... note, it was blowing up in core so will need to either move the calling code out of core or let gems pending drive the gem specification

Fryguy · 2023-09-11T21:06:48Z

spec/lib/task_helpers/exports/custom_buttons_spec.rb

@@ -70,7 +70,7 @@
      TaskHelpers::Exports::CustomButtons.new.export(:directory => export_dir)
      file_contents = File.read("#{export_dir}/CustomButtons.yaml")

-      expect(YAML.safe_load(file_contents, [Symbol])).to contain_exactly(*custom_button_export_test)
+      expect(YAML.safe_load(file_contents, permitted_classes: [Symbol])).to contain_exactly(*custom_button_export_test)


I think there's a way to set permitted_classes globally, so we can add most of the reasonable defaults of Date, Time, DateTime, Symbol, etc.

This was a fast pass through to see what's busted as it's far easier to comment on code 😉

Extracted from ManageIQ#22698 Part of ManageIQ#22696 See: rails/rails#45816 https://www.ruby-lang.org/en/news/2021/12/25/ruby-3-1-0-released/ FileDepotFtp depends on this gem so it should be a dependency in core.

jrafanie · 2023-10-05T16:32:30Z

lib/extensions/yaml_load_aliases.rb

+  def safe_load(*args, **kwargs)
+    super(*args, **kwargs.merge(:aliases => true, :permitted_classes => DEFAULT_PERMITTED_CLASSES))
+  rescue NameError
+    super(*args, **kwargs.merge(:aliases => true, :permitted_classes => DEFAULT_PERMITTED_CLASSES + [MiqExpression, MiqReport, Ruport::Data::Table, Ruport::Data::Record]))


Note, we're enumerating these classes at method call time to avoid autoloading them at code load time. This file is loaded very early in the load time. I suppose we can put this list in an memoized instance method to avoid creating so many arrays.

kbrock · 2023-10-05T16:47:40Z

only request:

use hash rockets
use **reverse_merge() / **{}.merge(kwargs)

jrafanie · 2023-10-05T17:07:29Z

only request:

use hash rockets

use **reverse_merge() / **{}.merge(kwargs)

Will do, thanks! Yes, it makes sense to any passed in kwargs and not override them.

kbrock · 2023-10-16T17:45:35Z

lib/extensions/yaml_load_aliases.rb

+  def safe_load(*args, **kwargs)
+    super(*args, **kwargs.reverse_merge(:aliases => true, :permitted_classes => DEFAULT_PERMITTED_CLASSES))
+  rescue NameError
+    super(*args, **kwargs.reverse_merge(:aliases => true, :permitted_classes => DEFAULT_PERMITTED_CLASSES + [MiqExpression, MiqReport, Ruport::Data::Table, Ruport::Data::Record]))


were you going to add a warning to this code? (and possibly below too?

Yes and yes. Just chatted with @Fryguy about this. I need to check alias: true is ok to do by default and add warnings.

Hopefully, this "expanded permitted classes" list can moved to a memoized method since we need the class objects so we want to avoid autoloading models too early at load time, but memoized so we don't build this list over and over again.

I'll have to go through and build a list of these additional classes through trial and error with various yamls. We can then decide if we want to the final fallback of loading disallowed classes with unsafe_load. I think this fallback (line 13), is a worst case scenario. I'd like to get rid of it and fix bugs when we find places we missed.

kbrock · 2023-10-16T17:47:15Z

app/models/dialog_import_validator.rb

@@ -11,7 +11,7 @@ def initialize(dialog_field_association_validator = DialogFieldAssociationValida
  end

  def determine_validity(import_file_upload)
-    potential_dialogs = YAML.safe_load(import_file_upload.uploaded_content, [Symbol])
+    potential_dialogs = YAML.safe_load(import_file_upload.uploaded_content, :permitted_classes => [Symbol])


Do these changes fix the this code without the YamlLoadAliases?

So are we deciding if we want this change or the Yaml monkey patch?

Good question. When possible, safe_load is best. For tests, it's ok for them to use unsafe_load if enumerating the list isn't worth it. Ultimately, the suggestion to log warnings below will help us build the list of permitted classes that we can use in most places.

Looking at psych code, it seems to call to_s on the passing in permitted classes so maybe I can try another way of doing it. If I recall, the rails config.active_record.yaml_column_permitted_classes didn't allow string/symbols but maybe if I can get in the middle, I can call safe_load directly with string/symbols and avoid the issue with reload and eager loading class objects.

I feel like there are 2 use cases:

use safe_load when ever our code references YAML. I think this is a good trend

use safe_load when rails models have a yaml column -- this one I believe should use unsafe_load via the configuration parameter

adding to that comment

I believe when we use YAML.safe_load, the list of models is relatively small. MiqExpression and a few others for reports. Also, since this is from external parties, this should be locked down more.

I believe when we use active record yaml serialization (e.g. with MiqQueue), we want many more models. And theoretically, this is not externally accessible, so we could be more liberal with the number of models we support here. (e.g.: all?)

.github/workflows/ci.yaml

jrafanie · 2023-10-18T21:24:22Z

lib/yaml_permitted_classes.rb

+class YamlPermittedClasses
+  DEFAULT_PERMITTED_CLASSES = [Object, Range, Regexp, Symbol, Date, Time, DateTime, ActiveSupport::Duration, ActiveSupport::HashWithIndifferentAccess, ActiveSupport::TimeWithZone, ActiveSupport::TimeZone]
+  def self.app_yaml_permitted_classes
+    @app_yaml_permitted_classes ||= DEFAULT_PERMITTED_CLASSES + [MiqExpression, MiqReport, Ruport::Data::Table, Ruport::Data::Record, User, ConfigurationScript, ContainerImage, ContainerTemplate, OrchestrationTemplate, ManageIQ::Providers::Vmware::InfraManager, ManageIQ::Providers::InfraManager::Vm]


This is going to be pretty annoying if we basically eager load all of our models we permit to be yaml loaded from serialized column.

DEFAULT_PERMITTED_CLASSES contains libraries so that looks ok.
I am concerned about @app_yml_permitted_classes since that has classes from manageiq/app/models which can be dynamically reloaded.

Is there a rails hook we can use to clear the @app_ cache variable?

Agreed. There is likely a reload hook we can wrap this and clear it.

I'm not in love with the current way to specify classes that can be loaded via YAML specifically because of the point you're using. It's taking actual class objects, not a symbol/string of the class so we need to eager load the classes and as you said, deal with the class being reloaded.

kbrock · 2023-10-24T20:35:30Z

lib/extensions/yaml_load_aliases.rb

+  # constants at that time, such as MiqExpression, Ruport, so we have two sets of permitted classes.
+  def safe_load(yaml, permitted_classes: [], aliases: false, **kwargs)
+    # permitted_classes kwarg is provided because rails 6.1.7.x expects it as a defined kwarg.  See: https://github.com/rails/rails/blob/9ab33753b6bab1809fc73d35b98a5c1d0c96ba1b/activerecord/lib/active_record/coders/yaml_column.rb#L52
+    permitted_classes += YamlPermittedClasses.initialized? ? YamlPermittedClasses.app_yaml_permitted_classes : YamlPermittedClasses.default_permitted_classes


there is a lot of text here.

Can we move this check into YamlPermittedClasses?

Something like:

module YamlLoadAliases def safe_load(yaml ...) permitted_classes += YamlPermittedClasses.permitted_classes [...] end end class YmlPermittedClasses def self.permitted_classes YamlPermittedClasses.initialized? ? YamlPermittedClasses.app_yaml_permitted_classes : YamlPermittedClasses.default_permitted_classes end end

napkin code - hopefully you can use for inspiration

Thanks... now that I'm done looking at the yaml columns in other repos I'm going to come back to this.

spec/lib/task_helpers/exports/custom_buttons_spec.rb

Depends on the interface to add these classes to the list in this PR: ManageIQ/manageiq#22698

Todo: Move to using config.active_record.yaml_column_permitted_classes with a mechanism for handling YAML.load with aliases and non-permitted classes outside of yaml_columns. See: https://bugs.ruby-lang.org/issues/17866 https://stackoverflow.com/questions/71191685/visit-psych-nodes-alias-unknown-alias-default-psychbadalias/71192990#71192990 Use unsafe_load explicitly and fallback to psych 3 behavior When we can, use unsafe_load or define the permitted_classes. If we can't, allow the fallback of psych 3 behavior where we first try to load safely and ultimatley will finally try using unsafe load. We can convert specific locations to use unsafe_load or specify their own permitted_classes. Until then, this fallback will allow us to transition to psych 4.

See: https://bugs.ruby-lang.org/issues/17866 YAML.load does safe_load by default in psych 4. It also sets aliases: false.

If we encounter a DisallowedClass, retry with our app's yaml_column_permitted_classes If that still doesn't work, fallback to warning so we can add the classes to our app's allowed list. Keep compatibility with psych 3.1+ since permitted_classes and aliases were added as keyword arguments to safe_load.

load now calls safe_load by default safe_load has a tiny list of permitted_classes by default safe_load aliases defaults to false, so our 'base' alias in database.pg.yml would not load by default. In prior commits, we now populate the permitted_classes with a fallback we can eventually removed. Additionally, we default aliases to true.

We're adding active model classes due to: ``` puts MiqReport.first.to_yaml --- !ruby/object:MiqReport concise_attributes: - !ruby/object:ActiveModel::Attribute::FromDatabase name: id value_before_type_cast: 1 - !ruby/object:ActiveModel::Attribute::FromDatabase name: name ... ``` Could be related to https://github.com/rails/rails/issues #25145

YamlLoadAliases doesn't need to know these details. YamlPermittedClasses is where this logic should live.

We can permit these classes for tests in the specs themselves. When we tackle the serialized columns, we can further refine this list.

For example, we can do this in tests: RSpec.configure do |config| config.before do YamlPermittedClasses.app_yaml_permitted_classes |= [VimHash, VimString, VimArray] end end

* MiqReport: ``` puts MiqReport.first.to_yaml --- !ruby/object:MiqReport concise_attributes: - !ruby/object:ActiveModel::Attribute::FromDatabase name: id value_before_type_cast: 1 - !ruby/object:ActiveModel::Attribute::FromDatabase name: name ... ``` Could be related to https://github.com/rails/rails/issues #25145 * Ruport objects

miq-bot · 2023-12-18T19:25:30Z

Checked commits jrafanie/manageiq@ed10b7c~...1d6a861 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
7 files checked, 6 offenses detected

lib/extensions/yaml_load_aliases.rb

⚠️ - Line 4, Col 46 - Lint/UnusedMethodArgument - Unused method argument - aliases.
❗ - Line 7, Col 17 - Style/HashSyntax - Use hash rockets syntax.
❗ - Line 7, Col 55 - Style/HashSyntax - Use hash rockets syntax.

spec/lib/extensions/ar_yaml_spec.rb

❗ - Line 13, Col 144 - Style/ColonMethodCall - Do not use :: for method calls.
❗ - Line 21, Col 144 - Style/ColonMethodCall - Do not use :: for method calls.
❗ - Line 28, Col 144 - Style/ColonMethodCall - Do not use :: for method calls.

Followup to ManageIQ#22698 Part of ManageIQ#22696

jrafanie added the ruby3.1 label Sep 11, 2023

jrafanie requested a review from Fryguy as a code owner September 11, 2023 17:54

miq-bot added the wip label Sep 11, 2023

miq-bot added the dependencies label Sep 11, 2023

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Sep 11, 2023

Running tests for jrafanie

3df0a41

From Pull Request: ManageIQ/manageiq#22698

miq-bot mentioned this pull request Sep 11, 2023

[BOT] Cross repo test for ManageIQ/manageiq#22698 ManageIQ/manageiq-cross_repo-tests#824

Open

jrafanie requested review from agrare and kbrock as code owners September 11, 2023 19:59

jrafanie changed the title ~~[WIP] Suppport Ruby 3.1~~ [WIP] Support Ruby 3.1 Sep 11, 2023

Fryguy reviewed Sep 11, 2023

View reviewed changes

This was referenced Sep 13, 2023

Ruby 3.1 - net/ftp was moved out of standard library to a gem in ruby 3.1 #22702

Merged

Upgrade to ruby 3.1 #22696

Closed

jrafanie force-pushed the ruby31 branch 3 times, most recently from 5d5f8a3 to 6bd2c77 Compare October 5, 2023 16:16

jrafanie closed this Oct 5, 2023

jrafanie reopened this Oct 5, 2023

jrafanie commented Oct 5, 2023

View reviewed changes

jrafanie force-pushed the ruby31 branch from 6bd2c77 to 38fae52 Compare October 5, 2023 20:48

kbrock reviewed Oct 16, 2023

View reviewed changes

Fryguy reviewed Oct 16, 2023

View reviewed changes

.github/workflows/ci.yaml Outdated Show resolved Hide resolved

jrafanie commented Oct 18, 2023

View reviewed changes

kbrock reviewed Oct 24, 2023

View reviewed changes

spec/lib/task_helpers/exports/custom_buttons_spec.rb Outdated Show resolved Hide resolved

jrafanie added a commit to jrafanie/manageiq-providers-vmware that referenced this pull request Dec 18, 2023

Specify vim classes as permitted in test

dcb2784

Depends on the interface to add these classes to the list in this PR: ManageIQ/manageiq#22698

jrafanie mentioned this pull request Dec 18, 2023

Ruby 3.1: Specify vim classes as permitted in test ManageIQ/manageiq-providers-vmware#892

Merged

2 tasks

jrafanie added 2 commits December 18, 2023 14:16

Psych 4 passes aliases: false, so must override this

7b23dbf

See: https://bugs.ruby-lang.org/issues/17866 YAML.load does safe_load by default in psych 4. It also sets aliases: false.

jrafanie force-pushed the ruby31 branch 2 times, most recently from df775fb to 70bacfb Compare December 18, 2023 19:17

jrafanie added 7 commits December 18, 2023 14:20

Move the complexity to the correct location

12c026e

YamlLoadAliases doesn't need to know these details. YamlPermittedClasses is where this logic should live.

Raise in dev/test/non-prod environments so we fix all callers

ff9ec2d

Drop permitted classes needed for either tests or serialized columns

62a5b24

We can permit these classes for tests in the specs themselves. When we tackle the serialized columns, we can further refine this list.

Add a mechanism to specify the application yaml permitted classes

cfd913c

For example, we can do this in tests: RSpec.configure do |config| config.before do YamlPermittedClasses.app_yaml_permitted_classes |= [VimHash, VimString, VimArray] end end

jrafanie force-pushed the ruby31 branch from 70bacfb to 4b5a240 Compare December 18, 2023 19:21

Sort and list classes one per line for readability

e4fa768

jrafanie force-pushed the ruby31 branch 2 times, most recently from 0ea7860 to 0240fdf Compare December 18, 2023 19:24

jrafanie force-pushed the ruby31 branch from 0240fdf to 1d6a861 Compare December 18, 2023 19:24

jrafanie mentioned this pull request Dec 18, 2023

Allow StateVarHash to YAML.load from runtime instantiate ManageIQ/manageiq-automation_engine#538

Merged

2 tasks

agrare approved these changes Dec 18, 2023

View reviewed changes

agrare merged commit 634efa7 into ManageIQ:master Dec 18, 2023
6 checks passed

agrare assigned agrare and unassigned jrafanie Dec 18, 2023

jrafanie added a commit to jrafanie/manageiq that referenced this pull request Dec 18, 2023

Add ruby 3.1 to test matrix

dd31926

Followup to ManageIQ#22698 Part of ManageIQ#22696

jrafanie deleted the ruby31 branch December 18, 2023 20:31

jrafanie added the quinteros/yes? label Dec 20, 2023

jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jan 3, 2024

Add ruby 3.1 to test matrix

915d964

Followup to ManageIQ#22698 Part of ManageIQ#22696

Fryguy removed the quinteros/yes? label Jan 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Ruby 3.1 #22698

Support Ruby 3.1 #22698

jrafanie commented Sep 11, 2023 •

edited

Loading

jrafanie commented Sep 11, 2023

Fryguy Sep 11, 2023

jrafanie Sep 11, 2023 •

edited

Loading

Fryguy Sep 11, 2023

jrafanie Sep 11, 2023

jrafanie Oct 5, 2023

kbrock commented Oct 5, 2023

jrafanie commented Oct 5, 2023

kbrock Oct 16, 2023

jrafanie Oct 16, 2023

kbrock Oct 16, 2023

jrafanie Oct 16, 2023

jrafanie Oct 24, 2023

kbrock Oct 25, 2023

kbrock Oct 30, 2023

jrafanie Oct 18, 2023

kbrock Oct 24, 2023

jrafanie Oct 24, 2023

kbrock Oct 24, 2023 •

edited

Loading

jrafanie Nov 21, 2023

miq-bot commented Dec 18, 2023 •

edited by jrafanie

Loading

Support Ruby 3.1 #22698

Support Ruby 3.1 #22698

Conversation

jrafanie commented Sep 11, 2023 • edited Loading

jrafanie commented Sep 11, 2023

Choose a reason for hiding this comment

jrafanie Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock commented Oct 5, 2023

jrafanie commented Oct 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Dec 18, 2023 • edited by jrafanie Loading

jrafanie commented Sep 11, 2023 •

edited

Loading

jrafanie Sep 11, 2023 •

edited

Loading

kbrock Oct 24, 2023 •

edited

Loading

miq-bot commented Dec 18, 2023 •

edited by jrafanie

Loading