Catch exceptions in control blocks and fail the control #2987

clintoncwolfe · 2018-04-24T04:07:39Z

When a control block has any kind of exception thrown, we currently abort the run. If it's an Inspec exception, we (sometimes regrettably) suppress the stacktrace and exit with code 1. Other exceptions explode in the normal way.

This behavior goes again doctrine - that nothing in the profile code can derail the inspec run. In this PR, we catch the exception as early as possible; then to fail the control, we inject a dummy resource constructed so that the source location and exception error are preserved.

A functional test has been added to verify the new behavior. Additionally, a document has been added for future developers outlining how profile code is evaluated.

For special review: is the messaging produced when a control fails useful? Could it be reworded, or should we add some of the stack trace or other information available in the caught exception?

[cwolfe@lodi inspec-aux]$ be inspec exec test/unit/mock/profiles/exception-in-control

Profile: InSpec Profile (profile-01)
Version: 0.1.0
Target:  local://

  ✔  sshd01 using sshd_config resource nonexistant path with no property access: Test block
     ✔  Test block should include "Test"
  ×  sshd02 sshd_config resource nonexistant path with contents access in test block: Protocol
     ×  Test block Protocol
     Can't find file: /i/do/not/exist
  ✔  sshd03 sshd_config resource nil path with contents access in test block: Protocol
     ✔  Test block Protocol should be nil
  ✔  sshd04 sshd_config resource default path with contents access in test block: Protocol
     ✔  Test block Protocol should be nil
  ×  sshd05 sshd_config resource nonexistant path with contents access control block: test/unit/mock/profiles/exception-in-control/controls/02-sshd_config.rb:38
     ×  Control Source Code Error test/unit/mock/profiles/exception-in-control/controls/02-sshd_config.rb:38
     Can't find file: /i/do/not/exist
  ✔  c01 using file resource on nonexistant file with no property access: Test block
     ✔  Test block should include "Test"
  ✔  c02 using file resource on nonexistant file with contents access in test block: Test block
     ✔  Test block should be nil
  ✔  c03 using file resource on nonexistant file with contents access control block: Test block
     ✔  Test block should be nil
  ×  a control with an undefined method: test/unit/mock/profiles/exception-in-control/controls/03-syntax.rb:3
     ×  Control Source Code Error test/unit/mock/profiles/exception-in-control/controls/03-syntax.rb:3
     undefined local variable or method `swords' for #<#<Class:0x00007fb0dc12fa18>:0x00007fb0df6f1b60>
  ×  a control with a divide-by-zero: test/unit/mock/profiles/exception-in-control/controls/03-syntax.rb:7
     ×  Control Source Code Error test/unit/mock/profiles/exception-in-control/controls/03-syntax.rb:7
     divided by 0


Profile Summary: 6 successful controls, 4 control failures, 0 controls skipped
Test Summary: 6 successful, 4 failures, 0 skipped
[cwolfe@lodi inspec-aux]$

Fixes #2981

Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>

…al test Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>

Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>

jquick

Fantastic work @clintoncwolfe. Thanks for the excellent documentation also!

jerryaldrichiii

Looks great! I've added some comments. As usual 100% nitpicks. Feel free to disregard most.

jerryaldrichiii · 2018-04-26T19:19:38Z

docs/dev/control-eval.md

+
+## A profile context is created
+
+Like many things in InSpec core, a profile context is an anonymous class. (verify)


Could we remove (verify)?

This is a document for us, the inspec team. I wouldn't expect it to be consumed publicly, or if so, only by people trying to work on the guts of inspec. Much lower standdards - the goal is to capture tribal knowledge. It's incomplete, both structurally and content-wise. Some areas might even be factually incorrect. Here, I'm just tempering my certainty; the particular snipe-hunt I was on didn't entail me looking very hard at the profile context. Next time someone needs to know this sort of tribal knowledge, perhaps they can shore it up a bit more, if their research leads them in that direction.

Ah, that makes sense. I suppose leaving those sorts of things in are fine then.

jerryaldrichiii · 2018-04-26T19:20:17Z

docs/dev/control-eval.md

+
+Like many things in InSpec core, a profile context is an anonymous class. (verify)
+
+Additionally, a control_eval_context is created.  It is an instance of an anonymous class; it has a class<->relationship with its profile context.  See `lib/inspec/control_eval_context.rb`.


I think we could do a reference link to that file if you so chose. I don't mind either way.

jerryaldrichiii · 2018-04-26T19:21:07Z

docs/dev/control-eval.md

+
+### Each control and their block are wrapped in an anonymous class
+
+The anonymous class generator is located at `lib/inspec/control_eval_context.rb:24`.  At this point, the terminology switches from `control` to `rule`.  Each context class inherits from Inspec::Rule, which provides the constructor.


Same as above, link is nice but not required.

jerryaldrichiii · 2018-04-26T19:21:54Z

docs/dev/control-eval.md

+
+### The block is instance_eval'd against the control context class
+
+See `lib/inspec/rule.rb:50`.  We're now in two levels of instance eval'ing - the file is gradually being eval'd against the profile context anonymous class, and the current control's block is being instance eval'd against a control context anonymous class.


Another potential link

jerryaldrichiii · 2018-04-26T19:22:29Z

docs/dev/control-eval.md

+
+And, the describe and describe.one blocks are executed.
+
+### TODO: describe blocks are *not* instance-evaled


Should we remove the TODO?

jerryaldrichiii · 2018-04-26T19:23:02Z

docs/dev/control-eval.md

+Note: can skip a control with:
+Inspec::Rule.set_skip_rule(control, msg)
+
+## What else?


Could we drop this line?

jerryaldrichiii · 2018-04-26T19:26:30Z

test/unit/mock/profiles/exception-in-control/inspec.yml

@@ -0,0 +1,8 @@
+name: profile-01
+title: InSpec Profile
+maintainer: The Authors


Some of the other inspec.yml (all others?) use different metadata values. Not sure if it important though.

See: https://github.com/chef/inspec/blob/master/test/unit/mock/profiles/aws-profile/inspec.yml

I just used whatever inspec init profile generated. Nothing in the metadata was relevant to the test underway.

I figured, I'll mark it as approved, I don't think the copyright lines and such really matter. Was just going for consistency.

miah

Excellent documentation. =)

jerryaldrichiii

Thanks for all the work @clintoncwolfe. Especially the documentation. It's always needed!

clintoncwolfe added 3 commits April 23, 2018 22:17

Add a regression test profile for issue #2981

907503e

Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>

Add a document for devs detailing how controls are evaluated

145ae1e

Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>

Add code to catch exception, and move regression test into a function…

f9ad955

…al test Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>

clintoncwolfe added the Type: Bug Feature not working as expected label Apr 24, 2018

clintoncwolfe requested a review from a team as a code owner April 24, 2018 04:07

adamhjk added the in progress label Apr 24, 2018

Loosen failedd control test count, since travis anddd I disagree

d5497fa

Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>

jquick approved these changes Apr 26, 2018

View reviewed changes

clintoncwolfe added the Component: Loader Loading and Evaling profiles and libraries. label Apr 26, 2018

jerryaldrichiii suggested changes Apr 26, 2018

View reviewed changes

miah approved these changes Apr 26, 2018

View reviewed changes

jerryaldrichiii approved these changes Apr 26, 2018

View reviewed changes

clintoncwolfe merged commit 1a00853 into master Apr 26, 2018

clintoncwolfe deleted the cw/exception-in-control branch April 26, 2018 19:44

adamhjk removed the in progress label Apr 26, 2018

This was referenced Jun 8, 2018

Exceptions in bare control code trigger stacktrace in inspec 1.x #3094

Closed

Backport control-level exception catching to 1.x #3126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catch exceptions in control blocks and fail the control #2987

Catch exceptions in control blocks and fail the control #2987

clintoncwolfe commented Apr 24, 2018 •

edited

jquick left a comment

jerryaldrichiii left a comment

jerryaldrichiii Apr 26, 2018

clintoncwolfe Apr 26, 2018

jerryaldrichiii Apr 26, 2018

jerryaldrichiii Apr 26, 2018

jerryaldrichiii Apr 26, 2018

jerryaldrichiii Apr 26, 2018

jerryaldrichiii Apr 26, 2018

jerryaldrichiii Apr 26, 2018

jerryaldrichiii Apr 26, 2018

clintoncwolfe Apr 26, 2018

jerryaldrichiii Apr 26, 2018

miah left a comment

jerryaldrichiii left a comment


		## A profile context is created

		Like many things in InSpec core, a profile context is an anonymous class. (verify)


		Like many things in InSpec core, a profile context is an anonymous class. (verify)

		Additionally, a control_eval_context is created. It is an instance of an anonymous class; it has a class<->relationship with its profile context. See `lib/inspec/control_eval_context.rb`.


		### Each control and their block are wrapped in an anonymous class

		The anonymous class generator is located at `lib/inspec/control_eval_context.rb:24`. At this point, the terminology switches from `control` to `rule`. Each context class inherits from Inspec::Rule, which provides the constructor.


		### The block is instance_eval'd against the control context class

		See `lib/inspec/rule.rb:50`. We're now in two levels of instance eval'ing - the file is gradually being eval'd against the profile context anonymous class, and the current control's block is being instance eval'd against a control context anonymous class.


		And, the describe and describe.one blocks are executed.

		### TODO: describe blocks are not instance-evaled

Catch exceptions in control blocks and fail the control #2987

Catch exceptions in control blocks and fail the control #2987

Conversation

clintoncwolfe commented Apr 24, 2018 • edited

jquick left a comment

Choose a reason for hiding this comment

jerryaldrichiii left a 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

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

miah left a comment

Choose a reason for hiding this comment

jerryaldrichiii left a comment

Choose a reason for hiding this comment

clintoncwolfe commented Apr 24, 2018 •

edited