Allow `inspec check` to ignore `only_if` #2250

jerryaldrichiii · 2017-10-17T19:40:33Z

This fixes #2175
This fixes #2237

When using inspec check a mock Train backend is created. This means that the following would raise an error because os.name is nil

only_if { os.name.include?('anything') }

Since inspec check isn't concerned with the evaluation of only_if this skips those checks if the block given raises an error.

chris-rock · 2017-10-18T09:06:44Z

lib/inspec/control_eval_context.rb

@@ -133,7 +134,16 @@ def to_s

        define_method :only_if do |&block|
          return unless block
-          return if @skip_file == true || block.yield == true
+          return if @skip_file == true


Can't we just set @skip_file to false if we are in @backend.mock_transport?

I don't think we can because Unit tests also use a mock backend. Doing so would make it impossible to test only_if.

adamleff

Just two quick clean-up items and this looks good.

I wonder if there's a better way to do this without having to pass around data as to whether we're mocking or not. We already tightly-couple ourselves to the mock backend for unit tests, so I'm afraid of future edge cases.

Would it make sense to introduce the concept of an "operating mode" class var we can set and query? For example, in the Thor method when inspec check is run:

InSpec::OperatingMode.set_mode(:check)

... and we can flat-out skip only_if evaluation with something like:

return if InSpec::OperatingMode.is_check?
# or
return if InSpec::OperatingMode.current_mode == :check

@jerryaldrichiii @arlimus @chris-rock -- thoughts?

adamleff · 2017-10-24T21:04:08Z

lib/inspec/control_eval_context.rb

+            # Ignore error if a mock connection
+            # Example: `inspec check` with `only_if { os.name.include?('windows' }`
+            return if @backend.mock_transport?
+            raise e


You can eliminate the e here - raise within a rescue will re-raise the original exception.

adamleff · 2017-10-24T21:04:21Z

lib/inspec/rule.rb

+        @__skip_rule ||= !yield
+      rescue => e
+        return if @__mock == true
+        raise e


You can eliminate the e here - raise within a rescue will re-raise the original exception.

jerryaldrichiii · 2017-10-24T21:17:29Z

Thanks @adamleff didn't know that about raise in a rescue.

In regards to OperatingMode, I'm all for it. I think that is a cleaner approach.

chris-rock · 2017-10-26T14:29:33Z

The more I think about it the more I am not sure we are solving the right thing here. We have two problems that I see here:

1. os.name is set to nil and crashed the execution
1. only_if { os.name.include?('anything') } any only if can crash

The first issue can also happen outside of the only_if block, therefore the proposed solution is not helping for those cases. An alternative would be to set the operating system in train if we are using the mock backend

For the second one. We could try to catch any exceptions for only_if blocks, since this can also happen during runtime. As proposed by @adamleff What do you think?

adamleff · 2017-10-26T15:48:17Z

@chris-rock I think the os.name being nil and your only_if exception issue are symptoms of a larger issue... that issue is that we evaluate only_if blocks during an inspec check. In all other phases of InSpec operation, everything is fine. But given that only_if can contain other resources that could perhaps interact with a system not expected to be tested (i.e. the person's workstation), I think it's safe for us to declare that only_if blocks should never be eval'd during an inspec check

That's why I think we solve this with exposing a way for InSpec to communicate that we're in check mode... rather than catching exceptions. I'd much rather us never eval an only_if block unless we need to. That will solve both problems.

adamleff · 2017-11-06T17:59:56Z

Hey @jerryaldrichiii! @chris-rock and I met today to discuss this change.

First off, we agree that we should probably add a PR to Train to correctly build an os object that responds to all the expected methods, and return "mock" or "unknown" such that issues like the one described in this PR don't happen. I'll open an issue on that in case you want to try your hand at it.

Secondly, we'd like to take a different approach with this PR. Essentially, rather than tightly coupling ourselves to the mock backend, we'd like to pass through a flag that indicates we're in "check mode" and then, if so, tell the ControlEvalContext to short-circuit the only_if eval. Essentially, it will look like this:

CLI: #check should plumb a check_mode = true option and pass it to Inspec::Profile
Inspec::ProfileContext.for_profile will need to accept a new parameter indicating whether it's in check mode, and Inspec::Profile needs to pass it when creating the context
Inspec::ControlEvalContext should accept a new "skip_only_if_eval" option. Inspec::ProfileContext should pass it if check_mode is true
The eval of the only_if should now return early if "skip_only_if_eval" rather than catching an exception and determining whether the mock backend is in use

Let me know if you have any questions.

adamleff · 2017-11-08T17:57:58Z

lib/inspec/control_eval_context.rb

@@ -53,6 +53,7 @@ def initialize(backend, conf, dependencies, require_loader)
          @dependencies = dependencies
          @require_loader = require_loader
          @skip_file = false
+          @skip_only_if_eval = conf['check_mode'] || false


I'd like to draw a line at what things care about the concept of "check mode". In my opinion, the line needs to be drawn at the profile. Controls (and their evaluation contexts) should not care about the mode in which InSpec was executed but rather whether they should care about only_if or not.

Can we find a way to gracefully pass a bool for skip_only_if_eval from the profile to the control instead of passing check_mode here?

What are your thoughts about just passing it all the way from the CLI, e.g. o[:skip_only_if_eval] = true vs o[:check_mode] = true?

While you can claim YAGNI, what if something else in the future needs to care about whether we're in check_mode or not? And should the CLI really be directing how a control is evaluated?

The CLI knows it's in check mode, the profile needs to know it's in check mode (because it's the thing being checked), and then the profile should use that information to determine whether or not certain activities should take place. I still believe the line I drew and described above is the right separation of duties and logic.

I agree, I moved the line up to Profile and then passed skip_only_if_eval it to the ControlEvalContext

When using `inspec check` a mock Train backend is created. This means that the following would raise an error because `os.name` is `nil` ``` only_if { os.name.include?('anything') } ``` Since `inspec check` isn't concerned with the evaluation of `only_if` this skips those checks if the block given raises an error. Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

adamleff · 2017-11-30T22:29:00Z

lib/inspec/profile.rb

@@ -96,6 +96,7 @@ def initialize(source_reader, options = {})
      @attr_values = options[:attributes]
      @tests_collected = false
      @libraries_loaded = false
+      @check_mode = options[:check_mode] || false


Little Ruby nugget - careful with || and default booleans. For example, if you wanted the default to be true and you wrote this like:

@check_mode = options[:check_mode] || true

... it could never be false! If the user passed in false as the value in options[:check_mode], it would fail the left side of the || and then right side would evaluate.

I tend to use #fetch on Hash for this instead, just in case:

@check_mode = options.fetch(:check_mode, false)

Ah, thanks for the tip. Never thought of that.

adamleff · 2017-11-30T22:30:40Z

test/unit/profiles/control_eval_context_test.rb

@@ -31,7 +31,7 @@ def foobar
  let(:eval_context) do
    c = Inspec::ControlEvalContext.create(profile_context, resource_dsl)
    # A lot of mocking here :(
-    c.new(backend, mock(), mock(), mock())
+    c.new(backend, {}, mock(), mock(), false)


Can we add a comment here that enumerates the options ControlEvalContext expects so the next reader doesn't have to hop back to understand? I think that would help greatly.

Great idea, I pushed a commit that list the options that are mocked. Let me know if I need to modify the wording.

adamleff

One request-for-comment, but otherwise, this is good to go.

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

arlimus

Fantastic fix, thank you @jerryaldrichiii !!

jerryaldrichiii requested a review from a team as a code owner October 17, 2017 19:40

jerryaldrichiii added the in progress label Oct 17, 2017

chris-rock reviewed Oct 18, 2017

View reviewed changes

adamleff reviewed Oct 24, 2017

View reviewed changes

adamleff added the Type: Bug Feature not working as expected label Oct 24, 2017

adamleff mentioned this pull request Nov 6, 2017

Build a complete mock OS object inspec/train#206

Merged

jerryaldrichiii force-pushed the ja/skip-only-if-during-check branch 2 times, most recently from 7130ca1 to 497a0de Compare November 7, 2017 00:00

adamleff suggested changes Nov 8, 2017

View reviewed changes

jerryaldrichiii force-pushed the ja/skip-only-if-during-check branch from c2492c4 to 2d5b12e Compare November 9, 2017 05:05

adamleff approved these changes Nov 13, 2017

View reviewed changes

jerryaldrichiii added 4 commits November 29, 2017 12:57

Remove unnecessary e in rescue

871c3d9

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Modify implementation to use check_mode

fbf42c5

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Move check_mode concept to the Profile scope

e39e11e

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

jerryaldrichiii force-pushed the ja/skip-only-if-during-check branch from 2d5b12e to e39e11e Compare November 29, 2017 18:57

Fix lint after rubocop upgrade

68df07b

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

jerryaldrichiii requested a review from a team November 30, 2017 06:55

adamleff reviewed Nov 30, 2017

View reviewed changes

adamleff approved these changes Nov 30, 2017

View reviewed changes

Add comment for mocked ControlEvalContext options

1bd9a1e

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

arlimus approved these changes Dec 5, 2017

View reviewed changes

arlimus merged commit 49d36de into master Dec 5, 2017

arlimus deleted the ja/skip-only-if-during-check branch December 5, 2017 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow `inspec check` to ignore `only_if` #2250

Allow `inspec check` to ignore `only_if` #2250

jerryaldrichiii commented Oct 17, 2017

chris-rock Oct 18, 2017

jerryaldrichiii Oct 18, 2017

adamleff left a comment

adamleff Oct 24, 2017

adamleff Oct 24, 2017

jerryaldrichiii commented Oct 24, 2017

chris-rock commented Oct 26, 2017 •

edited

Loading

adamleff commented Oct 26, 2017

adamleff commented Nov 6, 2017

adamleff Nov 8, 2017

jerryaldrichiii Nov 8, 2017

adamleff Nov 8, 2017

jerryaldrichiii Nov 8, 2017

adamleff Nov 30, 2017

jerryaldrichiii Dec 4, 2017

adamleff Nov 30, 2017

jerryaldrichiii Dec 4, 2017

adamleff left a comment

arlimus left a comment

Allow inspec check to ignore only_if #2250

Allow inspec check to ignore only_if #2250

Conversation

jerryaldrichiii commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryaldrichiii commented Oct 24, 2017

chris-rock commented Oct 26, 2017 • edited Loading

adamleff commented Oct 26, 2017

adamleff commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamleff left a comment

Choose a reason for hiding this comment

arlimus left a comment

Choose a reason for hiding this comment

Allow `inspec check` to ignore `only_if` #2250

Allow `inspec check` to ignore `only_if` #2250

chris-rock commented Oct 26, 2017 •

edited

Loading