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

Add policy checking for request_host_scan. #14427

Merged
merged 1 commit into from
Mar 30, 2017

Conversation

lfu
Copy link
Member

@lfu lfu commented Mar 21, 2017

All events are sent to automate for processing since Event Switchboard PR.
Since then the policy checking for prevent action has changed from a sync process to an async process.

https://bugzilla.redhat.com/show_bug.cgi?id=1437910

@miq-bot assign @gmcculloug
@miq-bot add_label bug, control, darga/yes, euwe/yes

@miq-bot
Copy link
Member

miq-bot commented Mar 21, 2017

@lfu Cannot apply the following label because they are not recognized: drag/yes

@miq-bot
Copy link
Member

miq-bot commented Mar 21, 2017

Some comments on commit lfu@e69e061

spec/models/host_spec.rb

  • ⚠️ - 636 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 645 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 648 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

@miq-bot
Copy link
Member

miq-bot commented Mar 21, 2017

Checked commit lfu@e69e061 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 🏆

_log.warn("Error raising request scan event for #{log_target}: #{err.message}")
return
end
check_policy_prevent(:request_host_scan, :scan_queue, userid, options)
Copy link
Member

Choose a reason for hiding this comment

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

@lfu We already have a scan_from_queue method on the Host model. Can the logic here be reworked so the check_policy_prevent calls that method when the scan is not being stopped by policy? Otherwise we are calling the scan_queue method just to put the scan on the queue.

Copy link
Member Author

Choose a reason for hiding this comment

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

@gmcculloug Should not Host Scan go through the queue waiting for its turn?

Copy link
Member

Choose a reason for hiding this comment

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

The host scan does need to be queued and we already do that with the existing scan_from_queue method.

My question was asking if we could get away without having to call the new scan_queue which then queues the actual scan. Might be too much effort to do this using check_policy_prevent but it seemed worth looking into.

Copy link
Member Author

Choose a reason for hiding this comment

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

@gmcculloug scan_from_queue does not put the host scan on to the queue. It actually does the scan.

Copy link
Member

Choose a reason for hiding this comment

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

@lfu and I discussed my question above and agreed to leave the code as-is for now.

@lfu
Copy link
Member Author

lfu commented Mar 21, 2017

@miq-bot add_label darga/yes

@miq-bot
Copy link
Member

miq-bot commented Mar 21, 2017

@lfu unrecognized command 'add_lable', ignoring...

Accepted commands are: add_label, assign, close_issue, move_issue, remove_label, rm_label, set_milestone

@gmcculloug gmcculloug merged commit 5e9e1c7 into ManageIQ:master Mar 30, 2017
@gmcculloug gmcculloug added this to the Sprint 58 Ending Apr 10, 2017 milestone Mar 30, 2017
@simaishi
Copy link
Contributor

@lfu Is there a BZ for this? Can you please create one if it doesn't exist?

@lfu
Copy link
Member Author

lfu commented Mar 31, 2017

@simaishi BZ ticket was added.

simaishi pushed a commit that referenced this pull request Mar 31, 2017
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit c6614cb51632bde61a158ab7f8a6142c7ffe2452
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Thu Mar 30 14:39:54 2017 -0400

    Merge pull request #14427 from lfu/prevent_request_host_scan
    
    Add policy checking for request_host_scan.
    (cherry picked from commit 5e9e1c74b65003560351a1b1ab28513da8c2941c)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1437922

simaishi pushed a commit that referenced this pull request Apr 17, 2017
@simaishi
Copy link
Contributor

Euwe backport details:

$ git log -1
commit e6539046754955a7d65f7d8c74cdc8fbf55b6c06
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Thu Mar 30 14:39:54 2017 -0400

    Merge pull request #14427 from lfu/prevent_request_host_scan
    
    Add policy checking for request_host_scan.
    (cherry picked from commit 5e9e1c74b65003560351a1b1ab28513da8c2941c)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1437925

@lfu lfu deleted the prevent_request_host_scan branch October 16, 2017 20:14
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.

4 participants