microsoft / PSRule

Validate infrastructure as code (IaC) and objects using PowerShell rules.
https://microsoft.github.io/PSRule/v2/
MIT License
385 stars 49 forks source link

Rule incorrectly reports error when error condition is handled #564

Closed nightroman closed 3 years ago

nightroman commented 3 years ago

Sample script: SomeApp.Rule.ps1, given SomeApp.exe does not exist.

Rule SomeApp {
    try {
        $null = Get-Command SomeApp.exe -ErrorAction Stop
        $true
    }
    catch {
        $false
    }
}

Expected result

The rule's try/catch is honoured, the rule returns false on exception. The result is Fail.

RuleName                            Outcome    Recommendation
--------                            -------    --------------
SomeApp                             Fail

Actual result

Somehow (really, how?) try/catch is ignored and the rule is treated as having an error. The result is Error, not clear in the rule scenario, the exception is expected and handled by the rule.

RuleName                            Outcome    Recommendation
--------                            -------    --------------
SomeApp                             Error

Environment

PSRule: 0.20.0

$PSVersionTable

Name                           Value
----                           -----
PSVersion                      5.1.17763.1432
PSEdition                      Desktop
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
BuildVersion                   10.0.17763.1432
CLRVersion                     4.0.30319.42000
WSManStackVersion              3.0
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
nightroman commented 3 years ago

Another attempt without try/catch but with -ErrorAction Ignore:

Rule SomeApp {
    $result = Get-Command SomeApp.exe -ErrorAction Ignore
    $null -ne $result
}

Same unexpected result Error instead of expected Fail.

BernieWhite commented 3 years ago

@nightroman Thanks for reporting this issue.

Agreed, a resulting $False to the pipeline should be reporting as a Fail instead of Error. Interesting the PowerShell object internally is still flagging ps.HadErrors even though none are written to the error stream.

https://github.com/microsoft/PSRule/blob/a1e9dd6bf9232218cafef3ecf31243e65e0cdbb6/src/PSRule/Host/HostHelper.cs#L236-L245

Should be able to address this by looking for observable errors instead.

BernieWhite commented 3 years ago

@nightroman Question: Would you expect that -ErrorAction SilentlyContinue instead of -ErrorAction Ignore on Get-Command return an error result?

nightroman commented 3 years ago

If a rule completes and returns $true or $false then I would expect the result Pass or Fail regardless of intermediate errors, even written to the error stream.

Otherwise some rules are difficult to compose. Especially if they call tools which emit non terminating errors for various reasons and I do not own their code to avoid errors.

nightroman commented 3 years ago

Some concern. When this issue is resolved and noise non-terminating errors are ignored (one way or another) there might be another lesser issue caused by the unfortunate default error preference Continue.

Example: see the rule of another issue #565. It currently fails (let's ignore the usefulness of failure information for now).

The rule body emits an error and returns $true. So after fixing #564 (this issue) the rule outcome is going to be Pass. Because it completes and returns true.

Maybe it's OK, i.e. that is what a user "wants" by not using $ErrorActionPreference = 'Stop' in the rule body. But practically, it's unlikely that one wants such a code to pass. This leads to a suggestion: consider PSRule using the default error preference Stop on invoking rules.

BernieWhite commented 3 years ago

@nightroman Hmm it's a tricky one, but I get your points. I'm not sure I entirely agree that if a rule failed to execute correctly that it should be treated as a pass. Early on I had issues assuming that.

Consider: PSRule will allow more then one condition in a rule block, so a pass of single condition may not indicate that all conditions passed or executed in this case.

That said, I can see why you would want to suppress errors for a rule. So I'd propose that it isn't the default option, but could be opt-in for the rule author.

Initial suggestion would be an implementation something like:

Rule SomeApp -ErrorAction Ignore {
    $result = Get-Command SomeApp.exe
    $null -ne $result
}

As for defaulting the error preference to Stop, I think your suggestion makes a lot of sense for the most common use case. i.e. Error in a rule stops the rule, but other rules continue to evaluate.

An edge case might be something like this:

Rule SomeApp {
    $result = Get-Command SomeApp.exe
    $null -ne $result
        Recommend 'SomeApp failed to run'
}

In this case Recommend never runs.

nightroman commented 3 years ago

In this case Recommend never runs.

But this is how it is right now, i.e. before any further changes. Example of an empty recommendation:

rule test {
    # some code that fails (imagine a script call that throws), but here we just throw
    throw 'Oops'

    $true
    Recommend 'My recommendation'
}

Thus, Recommend just should be called the sooner the better in a rule body, in any case.

Offtopic maybe... This would not be the case if Recommend was implemented as a parameter. I am not criticizing the current design, it has some advantages. But it has some issues, too.

OTOH, if a rule fails due to a terminating error, it is very likely that it does not matter what recommendation is. The recommendation is designed for the "return $false" scenario, not for a rule terminating error. The terminating error should be resolved and "recommendation" is its error message.

NB I am just getting familiar with PSRule. I probably miss some concepts and misinterpret design decisions.

BernieWhite commented 3 years ago

@nightroman Good points. All good. Appreciate the discussion.

OTOH, if a rule fails due to a terminating error, it is very likely that it does not matter what recommendation is.

You are right, thinking it through, I can't really think of an instance where the original recommendation would be valid with an error.


FYI. Recommendations can also be supplied via markdown documentation. The Recommend keyword can also be used with $LocalizedData.