Get rid of the condition modifier.

[lfu]

Get rid of the condition modifier which is confusing and not necessary.

Control and compliance policy actions can be set basing on condition success, failure or both.

However, the action for built in policies is defined only for a true condition and is executed when condition is met. But the old logic would invoke actions only upon false condition result. So the old logic uses condition modifier 'deny' to force a true condition result into false and vice versa.
This flipping around seems not necessary and should be avoid to make a simple person's life easier and happier.

If a control policy has condition modifier set as 'deny', the actions for false condition would be invoked when the condition is actually met and vice versa.

There is no way to specify the condition modifier when adding a policy via UI. It can be done only via yaml file.

I won't be surprised if you find this confusing :)

Depends on ManageIQ/manageiq-schema#95

cc @gtanzillo @moolitayer
@miq-bot assign @gmcculloug
@miq-bot add_label bug, fine/yes

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

[gmcculloug]

@lfu Does the import logic need to be modified to handle older policy imports that would still contain the modifier key?

[gtanzillo]

Does this change require the logic in existing policy conditions to be flipped around to ensure same outcome as was with the modifier?

[lfu]

@gtanzillo Depends on how the condition is specified.
If it is in the format as in built in policies, the condition doesn't need to change.
If it is in the format as in miq_policy_set and has modifier: deny, the condition needs to be flipped around to ensure the same outcome as was with the modifier.

@moolitayer Can you verify this for OpenSCAP profile?

[lfu]

@gmcculloug Condition modifier is really meant for built in policies only.

Control and compliance policies can set up actions for both success and failure conditions. There is no need to use the condition modifier. And there is no way to set the condition modifier when adding a policy via UI.

In short, I think we don't need to touch the policy import/export logic.

[moolitayer]

@moolitayer Can you verify this for OpenSCAP profile?

Yes @lfu , tomorrow hopefully

[moolitayer]

I've downloaded this PR @lfu and prepared to test it after upgrade (attach policy in master then checkout your branch and test if that works)

I'm afraid I haven't been able to run bin/setup since that (probably not related to the PR)
I'm getting

You don't have bcrypt installed in your application. Please add it to your Gemfile and run bundle install
rails aborted!
LoadError: cannot load such file -- bcrypt

[lfu]

@moolitayer I hit the same issue when reviewing another PR :)
I added this line to Gemfile.dev.rbfor now so the server may come up running.

gem 'bcrypt', git: 'https://github.com/codahale/bcrypt-ruby.git', :require => 'bcrypt'

[lfu]

@gtanzillo @dclarizio Please review.

[lfu]

@moolitayer Do we need to negate the condition for OpenSCAP?

[moolitayer]

I'm sorry @lfu I still haven't been able to run refresh @cben @enoodle can one of you maybe help test this

I've been stuck on ManageIQ/manageiq-providers-kubernetes#136 for a few days now for testing this, but it seems like a local problem

See:
#16213 (comment)

[enoodle]

@lfu I tested this for OpenSCAP and the conditions should be negated. (except the one with the empty condition).

[moolitayer]

@lfu I was finally able to test this. The behavior now is not as expected[1]

We could negate the conditions for modifier=deny (or update failure|success_sequence and failure|success_synchronous).

Unfortunately users can copy policies (In the user guide we are even suggesting users copy the OpenSCAP policy in order to modify it)

I love the simplification done here, AFAIU we will need a different migration

[1]
MASTER:
[----] I, [2017-10-29T14:00:27.422206 #12506:880f7c] INFO -- : Q-task_id([job_dispatcher]) MIQ(policy-enforce_policy): Event: [containerimage_compliance_check], To: [centos/ruby-22-centos7]
[----] I, [2017-10-29T14:00:27.430269 #12506:880f7c] INFO -- : Q-task_id([job_dispatcher]) MIQ(policy-enforce_policy): Resolving policy [OpenSCAP]...
[----] I, [2017-10-29T14:00:27.447797 #12506:880f7c] INFO -- : Q-task_id([job_dispatcher]) MIQ(condition-eval): Name: Has high severity OpenSCAP rule results, Expression evaluation result: [false]
[----] I, [2017-10-29T14:00:37.691756 #12506:880f7c] INFO -- : Q-task_id([job_dispatcher]) MIQ(policy-enforce_policy): Event: [containerimage_compliance_passed], To: [centos/ruby-22-centos7]

pull/16213:
[----] I, [2017-10-29T14:11:03.779232 #19567:4a2f7c] INFO -- : Q-task_id([job_dispatcher]) MIQ(policy-enforce_policy): Event: [containerimage_compliance_check], To: [centos/ruby-22-centos7]
[----] I, [2017-10-29T14:11:03.800644 #19567:4a2f7c] INFO -- : Q-task_id([job_dispatcher]) MIQ(policy-enforce_policy): Resolving policy [OpenSCAP]...
[----] I, [2017-10-29T14:11:03.823525 #19567:4a2f7c] INFO -- : Q-task_id([job_dispatcher]) MIQ(condition-eval): Name: Has high severity OpenSCAP rule results, Expression evaluation result: [false]
[----] I, [2017-10-29T14:11:03.828767 #19567:4a2f7c] INFO -- : Q-task_id([job_dispatcher]) MIQ(action-invoke) Invoking action [Mark as Non-Compliant] for failed policy [OpenSCAP], event: [Container Image Compli
ance Check], entity name: [centos/ruby-22-centos7], entity type: [ManageIQ::Providers::Openshift::ContainerManager::ContainerImage], sequence: [1], synchronous? [true]
[----] I, [2017-10-29T14:11:03.828822 #19567:4a2f7c] INFO -- : Q-task_id([job_dispatcher]) MIQ(action_compliance_failed): Now executing [Mark as Non-Compliant] of ManageIQ::Providers::Openshift::ContainerManage
r::ContainerImage [centos/ruby-22-centos7]
[----] I, [2017-10-29T14:11:13.735953 #19576:4a2f7c] INFO -- : Q-task_id([job_dispatcher]) MIQ(policy-enforce_policy): Event: [containerimage_compliance_failed], To: [centos/ruby-22-centos7]

[miq-bot]

This pull request is not mergeable. Please rebase and repush.

[moolitayer]

@lfu let me know if you want me to test how the new version works with the OpenSCAP policy.
I think the test should be: copy the build in policy, attach the copy to one provider and the original to another. run the associated migration[1], make sure both policies work as expected.

[1] I haven't run a migration from manageiq-schema lately, is defining manageiq-schema as a local plugin and running db:migrate from the root ManageIQ enough?

[cben]

[1] I haven't run a migration from manageiq-schema lately, is defining manageiq-schema as a local plugin and running db:migrate from the root ManageIQ enough?

I've done this recently, yes, it's enough. And db:migrate says which migrations it runs so it's easy to verify. (If you need to operate *inside* the manageiq-schema checkout on its dummy DB, there you need `bin/setup` and for many things `export RAILS_ENV=test`)

[lfu]

@gtanzillo @dclarizio @gmcculloug Anything to update?

[miq-bot]

Checked commits lfu/manageiq@4a05784~...9938c08 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 1 offense detected

app/models/miq_policy.rb

• ❗ - Line 335, Col 3 - Style/RescueStandardError - Avoid rescuing without specifying an error class.

[gtanzillo]

LGTM 👍