Windows/FirewallRulesList incorrectly skips set with unspecifiedRules applied

[SteveL-MSFT]

Prerequisites

• Write a descriptive title.
• Make sure you are able to repro it on the latest version
• Search the existing issues.

Summary

Taken from #1666 (comment):

The new blocker: unspecifiedRules never executes under dsc config set while the declared rules are converged.

Steps to reproduce

Repro: on a host whose declared rules all match actual state, add one undeclared enabled rule, then run a real dsc config set with unspecifiedRules: {action: disable}. The rule stays enabled. The same config under --what-if reports Would disable unspecified firewall rule '...' — what-if and set disagree.

Expected behavior

`set` should still be applied since `unspecifiedRules` can still cause a change

Actual behavior

since `unspecifiedRules` is `writeOnly`, the synthetic check if already in desired state appears to be true

Error details

Diagnosis (all with the same input):

-l trace of the real config set shows windows_firewall invoked exactly once, with get (the synthetic pretest) — the set operation is never launched.
Cause is in invoke_set (command_resource.rs): with implementsPretest: false, the engine tests first and early-returns on in_desired_state && ExecutionKind::Actual. Since https://github.com/PowerShell/DSC/pull/1674 that test correctly skips writeOnly properties — but the same test now gates execution of the only operation that acts on the instruction. The guard being Actual-only is also why --what-if still reaches the resource.
dsc resource set with the byte-identical input does sweep (that path skips the pretest), and if any declared rule drifts in the same run, config set executes and sweeps correctly too — so the resource's sweep logic is fine; it's purely the config-engine skip.
Possible fixes, in rough preference order: (a) mark the resource implementsPretest: true — its set is already idempotent (already-disabled rules are skipped), so this looks like a one-line manifest change; (b) more generally, don't skip set when the desired state contains a writeOnly property — by definition the test cannot see the work set would do; (c) purge-aware get/test, which would also make test honest about unspecified-rule drift — the biggest change, and (a) or (b) are enough for us.

One doc note regardless of fix: with writeOnly, dsc config test reports converged even when enabled undeclared rules sit in scope — enforcement happens at set time only. Worth a line so nobody builds compliance reporting on test with this property.

Environment data

3.3-rc.2 pre-release

Version

3.3-rc.2 pre-release

Visuals

No response

[SteveL-MSFT]

Instead of misusing implementsPretest: true even though it's not implemented to force set to occur, I think the approach I want to take is:

• if resource declares implementsPretest: false (explicitly or implicitly)
• does not implement test explicitly (which falls into the synthetic test using get)
• and a writeOnly property is used

then set is always called

[michaeltlombardi]

I think that this is a case where the resource design includes functionality that makes synthetic testing dysfunctional for the resource.

Because the resource supports an operation on data that isn't represented by the resource instance data, only the resource can indicate whether the instance is actually in the desired state or how it will modify the system in what-if mode.

It's partially correct to indicate that the system has converged to the desired state except that the ignored write-only property extends the purview of the resource beyond the defined rules.

In this case I think it would be more correct to implement the test operation for the resource and emit info messages about rules that aren't managed and would be disabled or removed. Only if the instance is actually fully in the desired state should the resource return _inDesiredState as true.

I'm not sure I see a neat way to surface the set of rules that are considered out-of-state with the top-level unspecifiedRules being a write-only object. Adding a read-only field to the object doesn't work with the engine design. Defining it with the oneOf keyword for defined-vs-returned data feels a bit complex and would raise more complications for the synthetic test, probably requiring the resource to implement test anyway.

I don't think we should just blanket indicate that resources with any write-only properties that rely on synthetic testing should always invoke set. For example, any resource that accepts credentials would be affected by this change. While resources should be defined idempotently, this guard exists to minimize that risk (and in the case of synthetic testing, eke a performance benefit since we're just comparing data in memory without spinning up a separate process).

[SteveL-MSFT]

Perhaps for now, we go with the simple and targeted approach of simply declaring the Firewall resource implements preTest so that it results in set being called. Thinking about it further, I tend to agree that a general heuristic may cause problems for other types of resources.