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

Changes for generic object method calls via REST API. #74

Merged
merged 2 commits into from
Oct 4, 2017

Conversation

lfu
Copy link
Member

@lfu lfu commented Sep 21, 2017

The original use case for generic object method call is from the automate scripts.
Add changes to support calling the generic object method via REST API.

@miq-bot assign @mkanoor
@miq-bot add_label enhancement, fine/no

cc @gmcculloug @abellotti @jntullo

Generic object method calls are coming in from automate engine instead of Drb server via REST API.
@lfu lfu force-pushed the go_method_call_via_api branch from d68e466 to 16fe96e Compare September 21, 2017 20:22
@miq-bot
Copy link
Member

miq-bot commented Sep 21, 2017

Checked commits lfu/manageiq-automation_engine@b55bbeb~...16fe96e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. ⭐

@abellotti
Copy link
Member

abellotti commented Sep 21, 2017

Thank you @lfu and @mkanoor for the quick turnaround !!!

Copy link
Contributor

@mkanoor mkanoor left a comment

Choose a reason for hiding this comment

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

@lfu When methods are directly called from the generic object method, the generic object should have the ability to set the user and group.
https://github.com/ManageIQ/manageiq/blob/master/app/models/generic_object.rb#L108
The REST API would have to set this first before calling the actual method on the instance, its a 2 step process not sure if the REST API is dealing with it.

These changes look ok from the engine side but the feature might need to be reviewed from an integration perspective @abellotti @gmcculloug

@gmcculloug
Copy link
Member

@abellotti I thought the API sets User.current_user. Can we reference that when called through the API?

@abellotti
Copy link
Member

yes we set both the User.current_user and User.current_group based on the authenticated/authorized user.

@jntullo
Copy link

jntullo commented Sep 22, 2017

@abellotti we want to queue the request, correct? If we need to queue the request, then we would not want to call two methods, but just one that we could pass the user into.

@mkanoor
Copy link
Contributor

mkanoor commented Sep 22, 2017

Its going to be a challenge setting the (userid, group_id, tenant_id) when executing a method via the Queue.
https://github.com/ManageIQ/manageiq/blob/b7ade2263bbbda6b9715d46f7b98ddc1ae5fa527/app/models/miq_queue.rb#L408
we would have to differentiate between generic method parameters and our identity parameters. Maybe we can send them via the queue as a hash with two keys {:identity => {:userid_id => id, :group_id => group_id} :method_parameters => {arg1, arg2, arg3}}

The User.current_user and User.current_group in the REST API can be used to create the args for the MiqTask, just setting them won't help since we are going to toss it into the Queue.

@jntullo
Copy link

jntullo commented Sep 22, 2017

@mkanoor that should work 👍

@abellotti
Copy link
Member

@jntullo we can do something similar to when we kick off vm operations, app/controllers/api/base_controller/action.rb method queue_object_action. that puppy sets the userid as part of the task_options, maybe there's provision for the user group too (authorization). This would also give us the Task Id we need for the task_id/task_href for the action result signature.

@mkanoor
Copy link
Contributor

mkanoor commented Sep 22, 2017

@Fryguy
Instead of each method implementing this userid, group setting would it make sense for the Queue to set the User.current_user and User.current_group before dispatching a method. It gives us a poor mans impersonation.

@gmcculloug
Copy link
Member

These changes look ok from the engine side but the feature might need to be reviewed from an integration perspective

@mkanoor What is your thinking with this PR? Should this PR be merged and deal with the integration problem in a git issue? Or do we need to figure out the integration part in order to determine if these changes are still needed?

@mkanoor
Copy link
Contributor

mkanoor commented Oct 4, 2017

We have added a separate PR that solves the concerns raised in this PR.
ManageIQ/manageiq#16089

👍

@mkanoor mkanoor merged commit 7d0268f into ManageIQ:master Oct 4, 2017
@mkanoor mkanoor added this to the Sprint 71 Ending Oct 16, 2017 milestone Oct 4, 2017
@lfu lfu deleted the go_method_call_via_api branch September 29, 2018 14:32
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.

6 participants