From 4a0578426aac62197bda2de035bd6a037cbc6910 Mon Sep 17 00:00:00 2001 From: Lucy Fu Date: Mon, 16 Oct 2017 14:25:32 -0400 Subject: [PATCH 1/2] Get rid of the condition modifier. https://bugzilla.redhat.com/show_bug.cgi?id=1499161 --- app/models/condition.rb | 9 ++++++--- app/models/miq_policy.rb | 1 - db/fixtures/miq_policy_sets.yml | 28 ++++++++++++++-------------- product/policy/built_in_policies.yml | 4 ---- spec/factories/condition.rb | 1 - spec/models/condition_spec.rb | 13 +++++++++++++ 6 files changed, 33 insertions(+), 23 deletions(-) diff --git a/app/models/condition.rb b/app/models/condition.rb index f636b55e999..a18f42b0495 100644 --- a/app/models/condition.rb +++ b/app/models/condition.rb @@ -2,9 +2,8 @@ class Condition < ApplicationRecord include UuidMixin before_validation :default_name_to_guid, :on => :create - validates_presence_of :name, :description, :guid, :modifier, :expression, :towhat - validates_uniqueness_of :name, :description, :guid - validates_inclusion_of :modifier, :in => %w( allow deny ) + validates :name, :description, :guid, :expression, :towhat, :presence => true + validates :name, :description, :guid, :uniqueness => true acts_as_miq_taggable acts_as_miq_set_member @@ -257,6 +256,10 @@ def export_to_array end def self.import_from_hash(condition, options = {}) + # To delete condition modifier in policy from versions 5.8 and older + condition["expression"].exp = {"not" => condition["expression"].exp} if condition["modifier"] == 'deny' + condition.delete("modifier") + status = {:class => name, :description => condition["description"]} c = Condition.find_by(:guid => condition["guid"]) msg_pfx = "Importing Condition: guid=[#{condition["guid"]}] description=[#{condition["description"]}]" diff --git a/app/models/miq_policy.rb b/app/models/miq_policy.rb index 55eeeafdbd9..53fc2aee4ab 100644 --- a/app/models/miq_policy.rb +++ b/app/models/miq_policy.rb @@ -63,7 +63,6 @@ def self.built_in_policies :name => policy.attributes["name"], :description => policy.attributes["description"], :expression => MiqExpression.new(policy.condition), - :modifier => policy.modifier, :towhat => "Vm" )] else diff --git a/db/fixtures/miq_policy_sets.yml b/db/fixtures/miq_policy_sets.yml index f1fb7c609a4..f54aab9f6c8 100644 --- a/db/fixtures/miq_policy_sets.yml +++ b/db/fixtures/miq_policy_sets.yml @@ -72,18 +72,18 @@ Condition: - name: if container image has high severity openscap rule results description: Has high severity OpenSCAP rule results - modifier: deny expression: !ruby/object:MiqExpression exp: - FIND: - search: - "=": - field: ContainerImage.openscap_rule_results-result - value: fail - checkany: - "=": - field: ContainerImage.openscap_rule_results-severity - value: High + not: + FIND: + search: + "=": + field: ContainerImage.openscap_rule_results-result + value: fail + checkany: + "=": + field: ContainerImage.openscap_rule_results-severity + value: High context_type: towhat: ContainerImage file_mtime: @@ -123,12 +123,12 @@ Condition: - name: "Do not scan image-inspector's image" description: "Don't scan image-inspector's image" - modifier: deny expression: !ruby/object:MiqExpression exp: - ENDS WITH: - field: ContainerImage-name - value: "/image-inspector" + not: + ENDS WITH: + field: ContainerImage-name + value: "/image-inspector" context_type: towhat: ContainerImage file_mtime: diff --git a/product/policy/built_in_policies.yml b/product/policy/built_in_policies.yml index d10adc961fd..b00ff6becca 100644 --- a/product/policy/built_in_policies.yml +++ b/product/policy/built_in_policies.yml @@ -13,7 +13,6 @@ - "=": field: Vm-power_state value: 'on' - :modifier: deny :mode: control :action: vm_stop - :name: Prevent Retired VM from Starting @@ -26,7 +25,6 @@ "=": field: Vm-retired value: true - :modifier: deny :mode: control :action: prevent - :name: Prevent Retired Instance from Starting @@ -39,7 +37,6 @@ "=": field: Vm-retired value: true - :modifier: deny :mode: control :action: vm_suspend - :name: Stop Retired VM @@ -52,6 +49,5 @@ "=": field: Vm-retired value: true - :modifier: deny :mode: control :action: vm_stop diff --git a/spec/factories/condition.rb b/spec/factories/condition.rb index 6208ce005d9..8fe173e0bf0 100644 --- a/spec/factories/condition.rb +++ b/spec/factories/condition.rb @@ -2,7 +2,6 @@ factory :condition do sequence(:name) { |num| "condition_#{seq_padded_for_sorting(num)}" } sequence(:description) { |num| "Condition #{seq_padded_for_sorting(num)}" } - modifier "allow" towhat "Vm" expression { MiqExpression.new(">=" => {"field" => "Vm-num_cpu", "value" => "2"}) } end diff --git a/spec/models/condition_spec.rb b/spec/models/condition_spec.rb index 47082de439e..e7bf9bdd358 100644 --- a/spec/models/condition_spec.rb +++ b/spec/models/condition_spec.rb @@ -151,4 +151,17 @@ expect(Condition.subst_matches?(expr, vm1)).not_to be_truthy end end + + describe ".import_from_hash" do + it "removes condition modifier" do + cond_hash = { + "description" => "test condition", + "expression" => MiqExpression.new(">" => {"field" => "Vm-cpu_num", "value" => 2}), + "modifier" => 'deny', + "towhat" => "Vm" + } + condition, _s = Condition.import_from_hash(cond_hash) + expect(condition.expression.exp).to eq("not" => {">" => {"field" => "Vm-cpu_num", "value" => 2}}) + end + end end From 9938c089fe789b95b57fa9512a4fb4e9d64dac67 Mon Sep 17 00:00:00 2001 From: Lucy Fu Date: Mon, 16 Oct 2017 14:29:01 -0400 Subject: [PATCH 2/2] Always apply actions when condition is met for compliance policy. https://bugzilla.redhat.com/show_bug.cgi?id=1499161 --- app/models/miq_action.rb | 21 +++++++++++---------- app/models/miq_policy.rb | 10 +++------- spec/models/miq_policy_spec.rb | 20 +++++++++++++++++--- 3 files changed, 31 insertions(+), 20 deletions(-) diff --git a/app/models/miq_action.rb b/app/models/miq_action.rb index 1852511f611..e12a078d955 100644 --- a/app/models/miq_action.rb +++ b/app/models/miq_action.rb @@ -106,15 +106,14 @@ def self.invoke_actions(apply_policies_to, inputs, succeeded, failed) results = {} begin - failed.each do |p| + succeeded.each do |p| actions = case p - when MiqPolicy then p.actions_for_event(inputs[:event], :failure).uniq + when MiqPolicy then p.actions_for_event(inputs[:event], :success).uniq else p.actions_for_event end actions.each do |a| - # merge in the synchronous flag and possibly the sequence if not already sorted by this - inputs = inputs.merge(:policy => p, :result => false, :sequence => a.sequence, :synchronous => a.synchronous) + inputs = inputs.merge(:policy => p, :result => true, :sequence => a.sequence, :synchronous => a.synchronous) _log.debug("action: [#{a.name}], seq: [#{a.sequence}], sync: [#{a.synchronous}], inputs to action: seq: [#{inputs[:sequence]}], sync: [#{inputs[:synchronous]}]") if a.name == "prevent" @@ -124,15 +123,17 @@ def self.invoke_actions(apply_policies_to, inputs, succeeded, failed) name = a.action_type == "default" ? a.name.to_sym : a.action_type.to_sym results[name] ||= [] - results[name] << {:policy_id => p.kind_of?(MiqPolicy) ? p.id : nil, :policy_status => :failure, :result => a.invoke(apply_policies_to, inputs)} + results[name] << {:policy_id => p.kind_of?(MiqPolicy) ? p.id : nil, :policy_status => :success, :result => a.invoke(apply_policies_to, inputs)} end end - succeeded.each do |p| - next unless p.kind_of?(MiqPolicy) # built-in policies are OpenStructs whose actions will be invoked only on failure - actions = p.actions_for_event(inputs[:event], :success).uniq + failed.each do |p| + next unless p.kind_of?(MiqPolicy) # built-in policies are OpenStructs whose actions will be invoked only on success + actions = p.actions_for_event(inputs[:event], :failure).uniq + actions.each do |a| - inputs = inputs.merge(:policy => p, :result => true, :sequence => a.sequence, :synchronous => a.synchronous) + # merge in the synchronous flag and possibly the sequence if not already sorted by this + inputs = inputs.merge(:policy => p, :result => false, :sequence => a.sequence, :synchronous => a.synchronous) _log.debug("action: [#{a.name}], seq: [#{a.sequence}], sync: [#{a.synchronous}], inputs to action: seq: [#{inputs[:sequence]}], sync: [#{inputs[:synchronous]}]") if a.name == "prevent" @@ -142,7 +143,7 @@ def self.invoke_actions(apply_policies_to, inputs, succeeded, failed) name = a.action_type == "default" ? a.name.to_sym : a.action_type.to_sym results[name] ||= [] - results[name] << {:policy_id => p.kind_of?(MiqPolicy) ? p.id : nil, :policy_status => :success, :result => a.invoke(apply_policies_to, inputs)} + results[name] << {:policy_id => p.kind_of?(MiqPolicy) ? p.id : nil, :policy_status => :failure, :result => a.invoke(apply_policies_to, inputs)} end end diff --git a/app/models/miq_policy.rb b/app/models/miq_policy.rb index 53fc2aee4ab..fbea44a7a53 100644 --- a/app/models/miq_policy.rb +++ b/app/models/miq_policy.rb @@ -331,13 +331,9 @@ def applies_to?(rec, inputs = {}) end def self.eval_condition(c, rec, inputs = {}) - possible_results = c['modifier'] == 'allow' ? %w(allow deny) : %w(deny allow) - begin - index = Condition.evaluate(c, rec, inputs) ? 0 : 1 - possible_results[index] - rescue => err - logger.log_backtrace(err) - end + Condition.evaluate(c, rec, inputs) ? 'allow' : 'deny' + rescue => err + logger.log_backtrace(err) end private_class_method :eval_condition diff --git a/spec/models/miq_policy_spec.rb b/spec/models/miq_policy_spec.rb index aa83db28013..5a41b3246b1 100644 --- a/spec/models/miq_policy_spec.rb +++ b/spec/models/miq_policy_spec.rb @@ -149,7 +149,7 @@ let(:policies) do [ FactoryGirl.create(:miq_policy, :conditions => [conds[0]], :active => true, :mode => 'control').tap do |p| - p.replace_actions_for_event(events[0], [[actions[0], {:qualifier => :success}]]) + p.replace_actions_for_event(events[0], [[actions[0], {:qualifier => :success}], [actions[1], {:qualifier => :failure}]]) end, FactoryGirl.create(:miq_policy, :conditions => [conds[1]], :active => false).tap do |p| p.replace_actions_for_event(events[1], [[actions[1], {:qualifier => :success}]]) @@ -230,6 +230,20 @@ described_class.enforce_policy(target, events[0].name) end end + + describe ".eval_condition" do + it "returns 'allow' when condition is met" do + vm = FactoryGirl.create(:vm_vmware, :hardware => FactoryGirl.create(:hardware, :cpu_sockets => 2)) + result = described_class.send(:eval_condition, conds[0], vm) + + expect(result).to eq('allow') + end + + it "returns 'deny' when condition is not met" do + result = described_class.send(:eval_condition, conds[0], target) + expect(result).to eq('deny') + end + end end describe ".built_in_policies" do @@ -258,7 +272,7 @@ it 'prevents retired instance from starting' do MiqQueue.destroy_all @vm.update_attributes(:retired => true) - expect(subject[:result]).to be false + expect(subject[:result]).to be true expect(subject[:actions].size).to eq(1) expect(subject[:details].first["name"]).to eq("(Built-in) Prevent Retired Instance from Starting") q = MiqQueue.first @@ -268,7 +282,7 @@ end it 'allows active vm to start' do - expect(subject[:result]).to be true + expect(subject[:result]).to be false expect(subject[:actions].size).to eq(0) expect(subject[:details].first["name"]).to eq("(Built-in) Prevent Retired Instance from Starting") end