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.

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

Permit subscription classes to be serialized in queue args #8964

Conversation

jrafanie
Copy link
Member

@jrafanie jrafanie commented Nov 16, 2023

Part of ManageIQ/manageiq#22696

This is for ruby 3.1/psych 4 defaults using safe_load and activerecord using safe_load in serialized yaml columns.

@jrafanie jrafanie requested a review from a team as a code owner November 16, 2023 20:19
@jrafanie jrafanie force-pushed the ensure_subscription_class_is_yaml_column_permitted branch from 25640dd to 287d5c9 Compare November 16, 2023 20:22
@@ -182,6 +182,7 @@ def pglogical_save_subscriptions
task_opts = {:action => "Save subscriptions for global region", :userid => session[:userid]}
queue_opts = {:class_name => "MiqPglogical", :method_name => "save_global_region",
:args => [subscriptions_to_save, subsciptions_to_remove]}
ActiveRecord::Base.yaml_column_permitted_classes = ActiveRecord::Base.yaml_column_permitted_classes | [subscriptions_to_save.first.class, subsciptions_to_remove.first.class]
Copy link
Member

Choose a reason for hiding this comment

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

Minor, but you can one-shot this

Suggested change
ActiveRecord::Base.yaml_column_permitted_classes = ActiveRecord::Base.yaml_column_permitted_classes | [subscriptions_to_save.first.class, subsciptions_to_remove.first.class]
ActiveRecord::Base.yaml_column_permitted_classes |= [subscriptions_to_save.first.class, subsciptions_to_remove.first.class]

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 an emoji... -1 NACK. 🤣

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, these are really out there to show what would be required to do config.active_record.use_yaml_unsafe_load = false in the application. I feel like the proper solution is to not serialize these objects so anything easy to grep can enable that in the future.

I can change it. Just giving background as there are a few like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, fixed

@jrafanie jrafanie force-pushed the ensure_subscription_class_is_yaml_column_permitted branch from 287d5c9 to e228bd8 Compare November 17, 2023 14:35
@miq-bot
Copy link
Member

miq-bot commented Nov 17, 2023

Checked commit jrafanie@e228bd8 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@kbrock
Copy link
Member

kbrock commented Nov 21, 2023

it would be nice if in development we display a warning or blow up.
But having this code is identical to unsafe_load. (anything that goes in is allowed)

@jrafanie
Copy link
Member Author

it would be nice if in development we display a warning or blow up.
But having this code is identical to unsafe_load. (anything that goes in is allowed)

@kbrock I opened ManageIQ/manageiq#22795 to track next steps... I'm going to list all the places we did this temporary hack and that suggestion would go nicely there.

@jrafanie
Copy link
Member Author

jrafanie commented Dec 1, 2023

This is ready to be merged, ManageIQ/manageiq#22795 has been opened and this PR is linked as a work item to fix going forward to support safe serialized columns.

@jeffibm
Copy link
Member

jeffibm commented Dec 5, 2023

Hey @jrafanie , @Fryguy , is this good to merge?

@jrafanie
Copy link
Member Author

jrafanie commented Dec 5, 2023

Hey @jrafanie , @Fryguy , is this good to merge?

yes, the workaround added here will be removed when we work on ManageIQ/manageiq#22795

@Fryguy Fryguy self-assigned this Dec 6, 2023
@jeffibm
Copy link
Member

jeffibm commented Dec 7, 2023

Hey @jrafanie , @Fryguy , is this good to merge?

yes, the workaround added here will be removed when we work on ManageIQ/manageiq#22795

ok, we will merge it today.

@jeffibm jeffibm merged commit a7d66f2 into ManageIQ:master Dec 8, 2023
8 checks passed
@jrafanie jrafanie deleted the ensure_subscription_class_is_yaml_column_permitted branch January 31, 2024 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants