Fix skipping of controls to actually do what it says

[kekaichinose]

What problem are we solving?

Should be a few sentences. Describe the user stories and the target market.

As a Chef customer, I want to be able to skip controls that are not applicable for my needs

Today, though InSpec claims to allow users to "skip" controls, on the back end, its still executing the control, but just reporting the result as "skipped". This:

• misleads the user as they were told a control was skipped, but was in fact run
• wastes cycles on evaluating/running a control that the user does not care about

We want to restore the customer's trust in InSpec's skip (and even waiver) functionalities by ensuring that a control that is skipped is actually skipped.

Objective and Key Results

What's the ideal outcome? How will we measure that we're successful?

Objective	Key Results
Ensure that controls that a user wants to skip are actually skipped (i.e. not run)	•   Customer can trust that a control has been skipped during an audit run •   (secondary) customer will notice performance improvements as non-running controls will save execution cycles

Acceptance Criteria

How will we know when we're done?

• Skipped controls are not actually evaluated/run
• Waived controls that are run: false are actually not evaluated/run
• Profiles that had controls that were previously skipped but still running are now properly not evaluated/run

Dependencies

What other work needs to come first? What will block us from success?

Spike of RSpec test aggregation - understanding what it will take to pull aggregation out of RSpec

Out of Scope

What problems are not included in this epic?

• Deprecating skip_control for waivers
  • The skip_control feature was good to address an initial need from customers to skip non-applicable controls in a given profile. However, as the compliance journey evolved, we also recognized there needed to be auditing around why a control was skipped, by who, whether it was temporary or permanent, etc.
  • Waivers launched to address this need back in Q1 2020
  • As a result, we should deprecate the skip_control feature in favor of waivers

Open Questions

• What questions still need to be addressed?
• Why was this missed during initial implementation?
• How will fixing control skipping impact existing customer use cases? Will customers that are using InSpec's skip control functionality as-is today be broken once we address this issue?

GitHub Epic

Aha! epics should be tracked in GitHub and attached to tactical issues and tasks. What is the link to that epic?

https://github.com/inspec/inspec/issues/5068

Other Links

List links to other documents or artifacts that are mentioned/inform this epic

• Fixes #4886
• Fixes #5050

Aha! Link: https://chef.aha.io/features/SH-91

[james-stocks]

Thanks @kekaichinose - the Acceptance Criteria is perfect.

The problem here is that there are roughly 3 phases to executing a profile:

1. evaluate the profile's Ruby source code, resulting in a set of InSpec DSL objects (controls, describes, resources, matchers). This is what we're calling "aggregation".
2. manage the controls that were aggregated (waive them; or include/exclude them from a wrapper profile).
3. evaluate the final set of controls to determine Pass/Fail/Skip results.

Waiving/skipping/excluding a control will absolutely stop phase 3 (pass/fail/skip evaluation of InSpec DSL) from executing. But phase 1 will have already executed, by nature of InSpec's input being Ruby source files.

The intention for InSpec was for all test execution to happen in phase 3, by writing controls that are expressed as InSpec DSL. The reality is that people needing to implement industry standards like CIS are forced to pepper controls with bare Ruby to get the compliance logic they need. This ostensibly works, but when end users try to create wrapper profiles or waiver files they hit the issue that their bare Ruby is always evaluated.

So 3 options we have are:

1. Change nothing in InSpec and have profile developers move all their code to pure InSpec DSL; with all bare Ruby expressions being inside a resource class.
2. Enable lets in the InSpec DSL to add a supported way to declare expressions that are not evaluated until controls are evaluated.
3. Change phase 1 (aggregation) of loading user code so that we aggregate the controls the user has declared without evaluating the code for that control. All code evaluation would happen after skips and waivers have been applied.

[aaronlippold]

Hi, I absolutely agree with you guys and we've actually had multiple conversations in various ways around this issue.

https://github.com/inspec/inspec/issues/1677

This issue is actually one of the first times this was brought up and it gets to the root cause and we have been working on doing some research to try to figure out how to not load controls onto the runner object which meet a particular condition. The challenge has been we don't actually know the status of some of these conditions like skipped or a tag etc. Until we evaluate the context of the Ruby code.

This is also actually an issue when you pass the - - controls flag as well the same issue actually applies. As you can see from the conversation I think the community at large is in agreement that we need to find a more efficient way to only load the controls that are of interest in any of these scenarios.

My team is interested in engaging and we look forward to moving ahead on this.

[Schwad]

Thanks all for this.

I do not have much to add, but a small note for those who may come here and be unfamiliar with the issue.

Anything inside a describe block is not evaluated when a controller is waived, so most tests would actually not ever be run and would be functionally waived. You can trial this yourself by injecting a puts statement inside a waived control directly and also inside a describe block.

However yes, independent CPU-intensive ruby that a developer writes outside of describe blocks (especially anything calling each!) is getting run at file load time.

I believe that prior to diving on this, as it is in large part a performance-affecting issue, we should consider doing a few benchmark measurements against some real-world scenarios. (That way we can show off how much faster InSpec is after any improvements). If y'all like I can spin up an issue to that accord.

[aaronlippold]

Agreed real tests are always the metric.

If we can find a way to 'correctly load the runner' given a condition, such as --controls, or --waivers, or --tags etc. I think it would be a great win for the community as a whole.

[clintoncwolfe]

#5215 reflects the same problem when using the CLI --controls option.

[Schwad]

N/B: #5225 above should be considered part of this epic

[Schwad]

This epic should also circle in https://github.com/inspec/inspec/issues/5215 so when resolved we can close off that issue

[Schwad]

Possible strategies to explore (more of a longstanding note to self once I pick this back up):

-> control_eval_context.rb ; see if we can check waiver status before register_control in #control. If not, is there contextual data we can pass along to be available at this point.

-> Reviewing RSpec-core to look at #let implementations/approach to see if there's anything that can be mirrored in #describe or #control definitions in InSpec to allow for code loading without evaluation.

-> Reviewing RSpec reference texts (i.e. Marston/Dees) to see if there are alternative strategies/methods we can review to cleanly achieve this.

[clintoncwolfe]

#3760 (original issue, discussion converges into this issue) is a related UX issue

[kekaichinose]

Comment added by Lisa Stidham in Aha! - View

Noting linked Issue 5068

[kidbrax]

Just wanted to reiterate how big an issue is. Currently I may have hundreds of controls but may only care about a single control at some point (trying to change and iterate quickly until it passes). But it is actually running every single control? And by using debug-level logging, I can see that is is literally making thousands of SSH connections to run these controls I don't care about. So in my case, a single control took over 7 minutes.

[clintoncwolfe]

The --control CLI option has now been fixed (#5434) to filter the control prior to full evaluation - giving the ability to narrow focus to a single control. That should help the debugging use case. The general problem of skipped controls being evaluated still stands, but we are chipping away at the edges of the problem.

[kidbrax]

The --control CLI option has now been fixed (#5434) to filter the control prior to full evaluation - giving the ability to narrow focus to a single control. That should help the debugging use case. The general problem of skipped controls being evaluated still stands, but we are chipping away at the edges of the problem.

@clintoncwolfe Just to clarify, are you saying the controls would still report as skipped but all the unnecessary ssh connections would not actually be made now?

[clintoncwolfe]

No, I'm referring to the --control CLI filter option, which is unrelated to skipping controls. Previously, its behavior was to evaluate all controls, then filter the output to only include the specified controls. Its behavior has been changed to no longer evaluate controls, instead filtering earlier in the process. I mentioned the feature because it is useful in the use case when one is trying to isolate a single control for debugging, as you mentioned. It does not address the issue of skipped controls being evaluated.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We value your input and contribution. Please leave a comment if this issue still affects you.

[wyardley]

One other thing that I think is a little confusing is that if you skip a test in a control, inspec seems to mark the control as skipped in the summary as well, e.g.,

Profile Summary: 4 successful controls, 0 control failures, 1 control skipped
Test Summary: 132 successful, 0 failures, 1 skipped

in the case where 5 controls are run, but 1 control has at least one passing test, the whole control gets marked as "skipped" in the summary, which seems a little off.

[clintoncwolfe]

A major progress point on this was the introduction of --filter-waived-controls in v4.49.0 2021-10-27. That moves the filtration of waivers to much earlier in the process, though it is not without risks.

So at this point, the problem is solved for debugging (you can isolate a control using --controls at the CLI) and for running profiles you can waiver individual controls and have them not run using the --filter-waived-controls option.