Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 113 additions & 5 deletions lib/dsc-lib/src/dscresources/dscresource.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use rust_i18n::t;
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};
use serde_json::{Map, Value};
use std::collections::HashMap;
use std::collections::{HashMap, HashSet};
use std::path::PathBuf;
use tracing::{debug, info, trace, warn};

Expand Down Expand Up @@ -657,14 +657,15 @@ pub fn get_diff(expected: &Value, actual: &Value) -> Vec<String> {

#[must_use]
/// Performs a comparison of two JSON Values using an optional JSON Schema.
/// If a property exists in `expected` but not in `actual`, the schema's `default` value
/// for that property is used for comparison when available.
/// Properties whose schema sets `writeOnly` to `true` are ignored. If a property exists
/// in `expected` but not in `actual`, the schema's `default` value for that property is
/// used for comparison when available.
///
/// # Arguments
///
/// * `expected` - The expected value
/// * `actual` - The actual value
/// * `schema` - Optional JSON Schema to look up default values for missing properties
/// * `schema` - Optional JSON Schema to identify write-only properties and default values
///
/// # Returns
///
Expand All @@ -691,6 +692,10 @@ pub(crate) fn get_diff_with_schema(expected: &Value, actual: &Value, schema: Opt
}

for (key, value) in &*map {
if is_schema_write_only(schema, key) {
continue;
}

if is_secure_value(value) {
// skip secure values as they are not comparable
continue;
Expand Down Expand Up @@ -726,7 +731,7 @@ pub(crate) fn get_diff_with_schema(expected: &Value, actual: &Value, schema: Opt
}
} else {
// Property not in actual - check schema for a default value
if let Some(default_value) = get_schema_default(schema, key) {
if let Some(default_value) = get_schema_default(schema, key) {
if value != &default_value {
info!("{}", t!("dscresources.dscresource.diffKeyMissing", key = key));
diff_properties.push(key.to_string());
Expand Down Expand Up @@ -764,6 +769,42 @@ fn get_schema_default(schema: Option<&Value>, property_name: &str) -> Option<Val
property_schema.get("default").cloned()
}

/// Returns whether a property's JSON Schema sets `writeOnly` to `true`, directly or
/// through a local JSON Pointer reference.
fn is_schema_write_only(schema: Option<&Value>, property_name: &str) -> bool {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially, I was going to recommend implementing this would be easier in the dsc-lib-jsonschema crate, but thinking about this a bit more, I think the ergonomics and simplicity will mostly improve when we implement the in-memory schema registry and retriever, when we can call deference() to get the full property definition for verification.

let Some(schema) = schema else {
return false;
};
let Some(mut property_schema) = schema
.get("properties")
.and_then(Value::as_object)
.and_then(|properties| properties.get(property_name))
else {
return false;
};
let mut visited_references = HashSet::new();

loop {
if property_schema.get("writeOnly").and_then(Value::as_bool) == Some(true) {
return true;
}

let Some(reference) = property_schema.get("$ref").and_then(Value::as_str) else {
return false;
};
let Some(pointer) = reference.strip_prefix('#') else {
return false;
};
Comment thread
michaeltlombardi marked this conversation as resolved.
if !visited_references.insert(pointer) {
return false;
}
let Some(resolved_schema) = schema.pointer(pointer) else {
return false;
};
property_schema = resolved_schema;
}
}

/// Validates the properties of a resource against its schema.
///
/// # Arguments
Expand Down Expand Up @@ -1023,6 +1064,73 @@ fn diff_with_schema_no_default_reports_missing_property() {
assert_eq!(diff, vec!["enabled".to_string()]);
}

#[test]
fn diff_with_schema_write_only_ignores_differing_property() {
use serde_json::json;
let expected = json!({"name": "test", "action": "remove"});
let actual = json!({"name": "test", "action": "ignore"});
let schema = json!({
"type": "object",
"properties": {
"name": { "type": "string" },
"action": { "type": "string", "writeOnly": true }
}
});
let diff = get_diff_with_schema(&expected, &actual, Some(&schema));
assert!(diff.is_empty(), "Expected write-only property to be ignored, got: {diff:?}");
}

#[test]
fn diff_with_schema_write_only_ignores_missing_property() {
use serde_json::json;
let expected = json!({"name": "test", "action": "remove"});
let actual = json!({"name": "test"});
let schema = json!({
"type": "object",
"properties": {
"name": { "type": "string" },
"action": { "type": "string", "writeOnly": true }
}
});
let diff = get_diff_with_schema(&expected, &actual, Some(&schema));
assert!(diff.is_empty(), "Expected write-only property to be ignored, got: {diff:?}");
}

#[test]
fn diff_with_schema_write_only_local_ref_ignores_missing_property() {
use serde_json::json;
let expected = json!({"name": "test", "action": "remove"});
let actual = json!({"name": "test"});
let schema = json!({
"type": "object",
"properties": {
"name": { "type": "string" },
"action": { "$ref": "#/$defs/action" }
},
"$defs": {
"action": { "type": "string", "writeOnly": true }
}
});
let diff = get_diff_with_schema(&expected, &actual, Some(&schema));
assert!(diff.is_empty(), "Expected referenced write-only property to be ignored, got: {diff:?}");
}

#[test]
fn diff_with_schema_write_only_false_reports_missing_property() {
use serde_json::json;
let expected = json!({"name": "test", "action": "remove"});
let actual = json!({"name": "test"});
let schema = json!({
"type": "object",
"properties": {
"name": { "type": "string" },
"action": { "type": "string", "writeOnly": false }
}
});
let diff = get_diff_with_schema(&expected, &actual, Some(&schema));
assert_eq!(diff, vec!["action".to_string()]);
}

#[test]
fn diff_without_schema_reports_missing_property() {
use serde_json::json;
Expand Down
2 changes: 1 addition & 1 deletion resources/windows_firewall/src/firewall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -587,7 +587,7 @@ pub fn set_rules(input: &FirewallRuleList, what_if: bool) -> Result<FirewallRule
_ => {} // None or Ignore: no additional action.
}

Ok(FirewallRuleList { rules: results, unspecified_rules: input.unspecified_rules.clone() })
Ok(FirewallRuleList { rules: results, unspecified_rules: None })
}

pub fn export_rules() -> Result<FirewallRuleList, FirewallError> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,11 @@ Describe 'Microsoft.Windows/FirewallRuleList - synthetic test with schema defaul
Remove-NetFirewallRule -Name $testRuleName -ErrorAction Ignore
}

It 'unspecifiedRulesAction set to default "ignore" does not report as differing' {
It 'unspecifiedRules action "ignore" does not report as differing' {
$json = @{
unspecifiedRulesAction = 'ignore'
unspecifiedRules = @{
action = 'ignore'
}
rules = @(@{
name = $testRuleName
direction = 'Inbound'
Expand All @@ -43,10 +45,10 @@ Describe 'Microsoft.Windows/FirewallRuleList - synthetic test with schema defaul

$result = $out | ConvertFrom-Json
$result.inDesiredState | Should -Be $true
$result.differingProperties | Should -Not -Contain 'unspecifiedRulesAction'
$result.differingProperties | Should -Not -Contain 'unspecifiedRules'
}

It 'unspecifiedRulesAction omitted does not report as differing' {
It 'unspecifiedRules omitted does not report as differing' {
$json = @{
rules = @(@{
name = $testRuleName
Expand All @@ -62,12 +64,14 @@ Describe 'Microsoft.Windows/FirewallRuleList - synthetic test with schema defaul

$result = $out | ConvertFrom-Json
$result.inDesiredState | Should -Be $true
$result.differingProperties | Should -Not -Contain 'unspecifiedRulesAction'
$result.differingProperties | Should -Not -Contain 'unspecifiedRules'
}

It 'non-default unspecifiedRulesAction "disable" is reported as differing' {
It 'unspecifiedRules action "disable" is ignored for comparison' {
$json = @{
unspecifiedRulesAction = 'disable'
unspecifiedRules = @{
action = 'disable'
}
rules = @(@{
name = $testRuleName
direction = 'Inbound'
Expand All @@ -81,12 +85,15 @@ Describe 'Microsoft.Windows/FirewallRuleList - synthetic test with schema defaul
$LASTEXITCODE | Should -Be 0 -Because (Get-Content -Raw $testdrive/error.log)

$result = $out | ConvertFrom-Json
$result.differingProperties | Should -Contain 'unspecifiedRulesAction'
$result.inDesiredState | Should -Be $true
$result.differingProperties | Should -Not -Contain 'unspecifiedRules'
}

It 'non-default unspecifiedRulesAction "remove" is reported as differing' {
It 'unspecifiedRules action "remove" is ignored for comparison' {
$json = @{
unspecifiedRulesAction = 'remove'
unspecifiedRules = @{
action = 'remove'
}
rules = @(@{
name = $testRuleName
direction = 'Inbound'
Expand All @@ -100,6 +107,7 @@ Describe 'Microsoft.Windows/FirewallRuleList - synthetic test with schema defaul
$LASTEXITCODE | Should -Be 0 -Because (Get-Content -Raw $testdrive/error.log)

$result = $out | ConvertFrom-Json
$result.differingProperties | Should -Contain 'unspecifiedRulesAction'
$result.inDesiredState | Should -Be $true
$result.differingProperties | Should -Not -Contain 'unspecifiedRules'
}
}
13 changes: 13 additions & 0 deletions resources/windows_firewall/tests/windows_firewall_set.tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,19 @@ Describe 'Microsoft.Windows/FirewallRuleList - set operation' -Skip:(!$isElevate
($out | ConvertFrom-Json).afterState.rules | Should -BeNullOrEmpty
}

It 'does not return unspecifiedRules in the after state' -Skip:(!$isElevated) {
$json = @{
rules = @()
unspecifiedRules = @{
action = 'ignore'
}
} | ConvertTo-Json -Compress -Depth 5
$out = $json | dsc resource set -r $resourceType -f - 2>$testdrive/error.log
$LASTEXITCODE | Should -Be 0 -Because (Get-Content -Raw $testdrive/error.log)

($out | ConvertFrom-Json).afterState.PSObject.Properties.Name | Should -Not -Contain 'unspecifiedRules'
}

It 'updates an existing rule' -Skip:(!$isElevated) {
Initialize-TestFirewallRule
$json = @{ rules = @(@{ name = $testRuleName; description = 'Updated by DSC test'; enabled = $false }) } | ConvertTo-Json -Compress -Depth 5
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
"type": "object",
"title": "Unspecified rules",
"description": "Defines the action and optional scope for firewall rules not explicitly listed in the rules array. When both direction and profiles are specified, a rule must match both filters.",
"writeOnly": true,
"additionalProperties": false,
"required": [
"action"
Expand Down
Loading