Skip to content
Open
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
1 change: 1 addition & 0 deletions changes/10.20.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Added support for unordered XML element comparison in compliance checks when config_ordered=False.
40 changes: 37 additions & 3 deletions nautobot_golden_config/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from django.utils.module_loading import import_string
from hier_config import WorkflowRemediation, get_hconfig
from hier_config.utils import hconfig_v2_os_v3_platform_mapper, load_hconfig_v2_options
from lxml import etree
from nautobot.apps.models import RestrictedQuerySet
from nautobot.apps.utils import render_jinja2
from nautobot.core.models.generics import PrimaryModel
Expand Down Expand Up @@ -138,17 +139,50 @@
formatted_diff.append(formatted_operation)
return "\n".join(formatted_diff)

def _sort_xml(xml_string):
"""Sort XML elements and attributes recursively for deterministic comparison."""

def canonical_sort(elem):
"""Recursively sort XML elements and attributes."""
elem.attrib.update(dict(sorted(elem.attrib.items())))
elem[:] = sorted(
elem,
key=lambda e: (
e.tag,
tuple(sorted(e.attrib.items())),
(e.text or "").strip(),
),
)
for child in elem:
canonical_sort(child)

try:
parser = etree.XMLParser(remove_blank_text=True)
root = etree.fromstring(xml_string, parser=parser) # noqa: S320
canonical_sort(root)
return etree.tostring(root, encoding="unicode", pretty_print=True)
except etree.ParseError:
return xml_string

Check warning on line 165 in nautobot_golden_config/models.py

View workflow job for this annotation

GitHub Actions / unittest_report (3.12, postgresql, stable)

Missing coverage

Missing coverage on lines 164-165

# Options for the diff operation. These are set to prefer updates over node insertions/deletions.
diff_options = {
"F": 0.1,
"fast_match": True,
}
missing = main.diff_texts(obj.actual, obj.intended, diff_options=diff_options)
extra = main.diff_texts(obj.intended, obj.actual, diff_options=diff_options)

if not obj.rule.config_ordered:
actual_xml = _sort_xml(obj.actual)
intended_xml = _sort_xml(obj.intended)
else:
actual_xml = obj.actual
intended_xml = obj.intended

missing = main.diff_texts(actual_xml, intended_xml, diff_options=diff_options)
extra = main.diff_texts(intended_xml, actual_xml, diff_options=diff_options)

compliance = not missing and not extra
compliance_int = int(compliance)
ordered = obj.ordered
ordered = obj.rule.config_ordered
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this is accurate. I think that ordered here showed if it was in fact ordered. Meaning "I may or may not care it's ordered, but this config is fact ordered"

I could be wrong on that, but at least that is how it works on the cli side. It allowed me to short circuit certain conditions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i see, i think we could replicate this but would need to diff twice if they didn't have order checked in the rule

      ordered_missing = main.diff_texts(obj.actual, obj.intended, diff_options=diff_options)
      ordered_extra = main.diff_texts(obj.intended, obj.actual, diff_options=diff_options)
      ordered_compliant = not ordered_missing and not ordered_extra

      if obj.rule.config_ordered:
          missing = ordered_missing
          extra = ordered_extra
      else:
          actual_xml = _sort_xml(obj.actual)
          intended_xml = _sort_xml(obj.intended)
          missing = main.diff_texts(actual_xml, intended_xml, diff_options=diff_options)
          extra = main.diff_texts(intended_xml, actual_xml, diff_options=diff_options)

      compliance = not missing and not extra
      compliance_int = int(compliance)
      ordered = ordered_compliant

missing = _null_to_empty(_normalize_diff(missing))
extra = _null_to_empty(_normalize_diff(extra))

Expand Down
13 changes: 13 additions & 0 deletions nautobot_golden_config/tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,19 @@ def create_feature_rule_xml_with_remediation(device, feature="foo5", rule="xml")
return rule


def create_feature_rule_xml_ordered(device, feature="foo6", rule="xml"):
"""Creates a Feature/Rule Mapping with config_ordered=True and Returns the rule."""
feature_obj, _ = ComplianceFeature.objects.get_or_create(slug=feature, name=feature)
rule = ComplianceRule(
feature=feature_obj,
platform=device.platform,
config_type=ComplianceRuleConfigTypeChoice.TYPE_XML,
config_ordered=True,
)
rule.save()
return rule


def create_feature_rule_cli(device, feature="foo_cli"):
"""Creates a Feature/Rule Mapping and Returns the rule."""
feature_obj, _ = ComplianceFeature.objects.get_or_create(slug=feature, name=feature)
Expand Down
84 changes: 84 additions & 0 deletions nautobot_golden_config/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
create_feature_rule_cli_with_remediation,
create_feature_rule_json,
create_feature_rule_xml,
create_feature_rule_xml_ordered,
create_job_result,
create_saved_queries,
)
Expand All @@ -41,6 +42,7 @@ def setUpTestData(cls):
cls.device = create_device()
cls.compliance_rule_json = create_feature_rule_json(cls.device)
cls.compliance_rule_xml = create_feature_rule_xml(cls.device)
cls.compliance_rule_xml_ordered = create_feature_rule_xml_ordered(cls.device)
cls.compliance_rule_cli = create_feature_rule_cli_with_remediation(cls.device)

def test_create_config_compliance_success_json(self):
Expand Down Expand Up @@ -71,6 +73,88 @@ def test_create_config_compliance_success_xml(self):
self.assertEqual(cc_obj.missing, "/root/foo/bar-1[1], baz")
self.assertEqual(cc_obj.extra, "/root/foo/bar-1[1], notbaz")

def test_create_config_compliance_success_xml_unordered(self):
"""XML elements in different order should be compliant when config_ordered=False."""
actual = '<root><user id="2"><name>bob</name></user><user id="1"><name>alice</name></user></root>'
intended = '<root><user id="1"><name>alice</name></user><user id="2"><name>bob</name></user></root>'
cc_obj = create_config_compliance(
self.device, actual=actual, intended=intended, compliance_rule=self.compliance_rule_xml
)

self.assertTrue(cc_obj.compliance)
self.assertEqual(cc_obj.missing, "")
self.assertEqual(cc_obj.extra, "")

def test_create_config_compliance_failure_xml_ordered(self):
"""XML elements in different order should NOT be compliant when config_ordered=True."""
actual = "<root><acl>deny 2.2.2.2</acl><acl>permit 1.1.1.1</acl></root>"
intended = "<root><acl>permit 1.1.1.1</acl><acl>deny 2.2.2.2</acl></root>"
cc_obj = create_config_compliance(
self.device, actual=actual, intended=intended, compliance_rule=self.compliance_rule_xml_ordered
)

self.assertFalse(cc_obj.compliance)

def test_create_config_compliance_xml_no_attributes(self):
"""XML elements with no attributes should be sorted by tag name only."""
actual = "<root><zebra>test</zebra><apple>test</apple></root>"
intended = "<root><apple>test</apple><zebra>test</zebra></root>"
cc_obj = create_config_compliance(
self.device, actual=actual, intended=intended, compliance_rule=self.compliance_rule_xml
)

self.assertTrue(cc_obj.compliance)
self.assertEqual(cc_obj.missing, "")
self.assertEqual(cc_obj.extra, "")

def test_create_config_compliance_xml_custom_attributes(self):
"""XML elements with arbitrary attributes (not just name/id) should sort correctly."""
actual = (
'<root><interface port="2" vlan="20">eth2</interface><interface port="1" vlan="10">eth1</interface></root>'
)
intended = (
'<root><interface port="1" vlan="10">eth1</interface><interface port="2" vlan="20">eth2</interface></root>'
)
cc_obj = create_config_compliance(
self.device, actual=actual, intended=intended, compliance_rule=self.compliance_rule_xml
)

self.assertTrue(cc_obj.compliance)
self.assertEqual(cc_obj.missing, "")
self.assertEqual(cc_obj.extra, "")

def test_create_config_compliance_xml_text_content_only(self):
"""XML elements with same tag but different text content should sort by text."""
actual = "<root><item>zebra</item><item>apple</item></root>"
intended = "<root><item>apple</item><item>zebra</item></root>"
cc_obj = create_config_compliance(
self.device, actual=actual, intended=intended, compliance_rule=self.compliance_rule_xml
)

self.assertTrue(cc_obj.compliance)
self.assertEqual(cc_obj.missing, "")
self.assertEqual(cc_obj.extra, "")

def test_create_config_compliance_xml_mixed_attributes(self):
"""XML with mixed attribute scenarios should handle all cases."""
actual = """<root>
<config type="vlan" priority="high">vlan100</config>
<config status="active">default</config>
<config type="interface">eth0</config>
</root>"""
intended = """<root>
<config status="active">default</config>
<config type="interface">eth0</config>
<config type="vlan" priority="high">vlan100</config>
</root>"""
cc_obj = create_config_compliance(
self.device, actual=actual, intended=intended, compliance_rule=self.compliance_rule_xml
)

self.assertTrue(cc_obj.compliance)
self.assertEqual(cc_obj.missing, "")
self.assertEqual(cc_obj.extra, "")

def test_create_config_compliance_unique_failure(self):
"""Raises error when attempting to create duplicate."""
ConfigCompliance.objects.create(
Expand Down
Loading