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

Image scanning: Add image name to task name #105

Merged
merged 1 commit into from
Sep 27, 2017
+9 −4
Merged

Image scanning: Add image name to task name #105

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
13 changes: 9 additions & 4 deletions app/models/manageiq/providers/kubernetes/container_manager_mixin.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,13 +173,18 @@ def scan_job_create(entity)
check_policy_prevent(:request_containerimage_scan, entity, User.current_user.userid, :raw_scan_job_create)
end

def raw_scan_job_create(target_class, target_id = nil, userid = nil)
target_class, target_id = target_class.class.name, target_class.id if target_class.kind_of?(ContainerImage)
def raw_scan_job_create(target_class, target_id = nil, userid = nil, target_name = nil)
# maintain backward compatibility pre https://github.com/ManageIQ/manageiq/pull/13722
if target_class.kind_of?(ContainerImage)
Copy link
Contributor

Choose a reason for hiding this comment

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

@moolitayer @cben can't we drop this entirely for 5.9? Maybe we can fail if target_class is an instance instead of a class name.

Copy link
Contributor

@cben cben Sep 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

we can if you decide we can ;-)
Half a year passed since the code stopped queueing AR instances, but for a user upgrading from last release, it's still simultaneous queueing & dequeing side change, they'll might have some scans with AR instances queued.
IMHO it's no big deal to drop a few scans.
Or could keep this compat in Gaprindashvili release and drop entirely in H release...

Copy link
Contributor

Choose a reason for hiding this comment

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

@moolitayer @cben if this is here to be backward-compatible with 4.2 then we can drop it in 4.6 (again, let's fail gracefully with a meaningful error message).

Let me know if I am missing something (e.g. version assumptions are incorrect, etc.).

Copy link
Contributor

@cben cben Sep 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Oh you're right, I thought it didn't make it into fine, but fine-1 was already queueing event_target.class.name, event_target.id and dequeuing with defensive if target_class.kind_of?(ContainerImage).

[upstream cheatsheet for this conversation: 5.9 & 4.6 both refer to master/Gaprindashvili, 4.5 to Fine, 4.2 to Euwe.]

Copy link

Choose a reason for hiding this comment

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

This fix is targeted for 4.5 zstream. Can we keep the check in this one for simplicity and remove it from master? (@simon3z maybe that is what you are suggesting but I'm not sure)

Copy link
Contributor Author

@zakiva zakiva Sep 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@simon3z We can merge this PR to master as is and backport it. Then, I will send a separate PR to drop this check on master.

Copy link

Choose a reason for hiding this comment

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

@zakiva 👍 Can you please send the second PR and label it as fine/no so we can view the while solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, #119

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM both

target_id = target_class.id
target_name = target_class.name
target_class = target_class.class.name
end
userid ||= User.current_user.userid
Job.create_job(
"ManageIQ::Providers::Kubernetes::ContainerManager::Scanning::Job",
:userid => userid,
:name => "Container image analysis",
:name => "Container Image Analysis: '#{target_name}'",
:target_class => target_class,
:target_id => target_id,
:zone => my_zone,
Expand All @@ -197,7 +202,7 @@ def check_policy_prevent(policy_event, event_target, userid, cb_method)
:class_name => self.class.to_s,
:instance_id => id,
:method_name => :check_policy_prevent_callback,
:args => [cb_method, event_target.class.name, event_target.id, userid],
:args => [cb_method, event_target.class.name, event_target.id, userid, event_target.name],
:server_guid => MiqServer.my_guid
}
enforce_policy(event_target, policy_event, {}, { :miq_callback => cb }) unless policy_event.nil?
Expand Down