Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: snapshot comparison on dicts with different levels #109

Closed
wants to merge 2 commits into from
Closed

fix: snapshot comparison on dicts with different levels #109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e81647
fix: snapshot comparison on dicts with different levels
alperenkose Aug 8, 2023
6d7c0ac
Update panos_upgrade_assurance/utils.py
alperenkose Aug 14, 2023
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
31 changes: 31 additions & 0 deletions examples/report/snapshot_load_compare.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,37 @@ def load_snap(fname: str) -> dict:
{"arp_table": {"properties": ["!ttl"], "count_change_threshold": 10}},
{"nics": {"count_change_threshold": 10}},
{"license": {"properties": ["!serial"]}},

# {"license": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should get rid of the commented lines

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, once I add the unit tests corresponding to these I will remove them in this PR.

# "properties": ["!serial", "!issued", "!authcode", "!expires", "!custom", "non-existing"] # exclude higher level
# }},
# {"license": {
# "properties": ["!serial", "!issued", "!authcode", "!expires", "non-existing", "!_Log_Storage_TB"] # works in multi-levels
# }},
# {"license": {
# "properties": ["all"]
# }},
# {"license": {
# "properties": ["!serial", "!issued", "!authcode", "!expires", "non-existing"] # invalid config is ignored and all is appended if all other are valid exclusions..
# }},
# {"license": {
# "properties": ["serial", "non-existing"] # compare only requested
# }},
# {"license": {
# "properties": ["!issued", "all"] # compare all except
# }},
# {"license": {
# "properties": ["!issued", "serial"] # skip one and compare specific ones
# }},
# {"license": {
# "properties": ["Logging Service"] # even works for parent level
# }},
# {"license": {
# "properties": ["Logging Service", "!custom"] # combination with parent
# }},
# "!license",
# "license",

{"routes": {"properties": ["!flags"], "count_change_threshold": 10}},
"!content_version",
{
Expand Down
19 changes: 10 additions & 9 deletions panos_upgrade_assurance/snapshot_compare.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,10 +235,10 @@ def calculate_diff_on_dicts(
) -> Dict[str, dict]:
"""The static method to calculate a difference between two dictionaries.

By default dictionaries are compared by going down all nested levels, to the point where key-value pairs are just strings
or numbers. It is possible to configure which keys from these pairs should be compared (by default we compare all
available keys). This is done using the `properties` parameter. It's a list of the bottom most level keys. For example,
when comparing route tables snapshots are formatted like:
By default dictionaries are compared by going down all nested levels. It is possible to configure which keys on each
level should be compared (by default we compare all available keys). This is done using the `properties` parameter.
It's a list of keys that can be compared or skipped on each level. For example, when comparing route tables snapshots are
formatted like:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's worth adding a comment about the fact that of a key does not exist in a dict it's ignored. And that this is true in both bases:

  • we are on a level where none of the keys match
  • we are on the correct level, but the dicts are different, just like the case with licenses that led us to this change.


```python showLineNumbers
{
Expand All @@ -258,8 +258,9 @@ def calculate_diff_on_dicts(
}
```

The bottom most level keys are:
The keys to process here can be:

- 'default_0.0.0.0/0_ethernet1/3',
- `virtual-router`,
- `destination`,
- `nexthop`,
Expand Down Expand Up @@ -405,11 +406,11 @@ def calculate_diff_on_dicts(

common_keys = left_side_to_compare.keys() & right_side_to_compare.keys()
if common_keys:
next_level_value = left_side_to_compare[next(iter(common_keys))]
at_lowest_level = True if not isinstance(next_level_value, dict) else False
keys_to_check = (
ConfigParser(valid_elements=set(common_keys), requested_config=properties).prepare_config()
if at_lowest_level
ConfigParser(valid_elements=set(common_keys),
requested_config=properties,
ignore_invalid_config=True).prepare_config()
if properties
else common_keys
)

Expand Down
33 changes: 31 additions & 2 deletions panos_upgrade_assurance/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ def __init__(
self,
valid_elements: Iterable,
requested_config: Optional[List[Union[str, dict]]] = None,
ignore_invalid_config: Optional[bool] = False
):
"""ConfigParser constructor.

Expand All @@ -168,13 +169,19 @@ def __init__(
* if `requested_config` is `None` we immediately treat it as if `all` was passed implicitly
(see [`dialect`](/panos/docs/panos-upgrade-assurance/dialect)) - it's expanded to `valid_elements`
* `_requested_config_names` is introduced as `requested_config` stripped of any element configurations. Additionally, we
do verification if elements of this variable match `valid_elements`. An exception is thrown if not.
do verification if all elements of this variable match `valid_elements`, if they do not, an exception is thrown by default.
`request_config` is checked at top level level key in case of nested dictionaries within the list.
* if `ignore_invalid_config` is set to `True`, we ignore any invalid configurations passed in the `requested_config` -
(no exception thrown) and we remove these invalid configurations from `_requested_config_names` and
`requested_config`.

# Parameters

valid_elements (iterable): Valid elements against which we check the requested config.
requested_config (list, optional): (defaults to `None`) A list of requested configuration items with an optional
configuration.
ignore_invalid_config (bool, optional): (defaults to `False`) Whether to ignore invalid configurations in
`requested_config` or throw an exception.

# Raises

Expand All @@ -188,9 +195,19 @@ def __init__(
self._requested_config_names = set(
[ConfigParser._extract_element_name(config_keyword) for config_keyword in self.requested_config]
)

non_existing_keys = set()
for config_name in self._requested_config_names:
if not self._is_element_included(element=config_name):
raise exceptions.UnknownParameterException(f"Unknown configuration parameter passed: {config_name}.")
non_existing_keys.add(config_name)

if not ignore_invalid_config: # if invalid config is not accepted
if non_existing_keys: # and non existing keys found
raise exceptions.UnknownParameterException(f"Unknown configuration parameters passed: {non_existing_keys}.")
else: # invalid config is accepted
self._requested_config_names.difference_update(non_existing_keys) # remove non-existing keys
self._remove_invalid_requested_config(invalid_keys=non_existing_keys)

else:
self._requested_config_names = set(valid_elements)
self.requested_config = list(valid_elements) # Meaning 'all' valid tests
Expand Down Expand Up @@ -248,6 +265,18 @@ def _extract_element_name(config: Union[str, dict]) -> str:
else:
raise exceptions.WrongDataTypeException("Config definition is neither string or dict")

def _remove_invalid_requested_config(self, invalid_keys: set) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just wondering if it make sense to build a method for 3 lines of code that won't be used anywhere else besides the constructor

"""Remove invalid keys from the requested configuration (`self.requested_config`).

# Parameters

invalid_keys (set): A set of keys to remove from `self.requested_config`.
"""

for config_element in self.requested_config[:]:
if ConfigParser._extract_element_name(config_element) in invalid_keys:
self.requested_config.remove(config_element)

def _expand_all(self) -> None:
"""Expand key word `'all'` to `self.valid_elements`.

Expand Down
62 changes: 53 additions & 9 deletions tests/test_firewall_proxy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,27 @@ def test_get_licenses(self, fw_proxy_mock):
<base-license-name>PA-VM</base-license-name>
<authcode />
</entry>
<entry>
<feature>Logging Service</feature>
<description>Device Logging Service</description>
<serial>00000000000</serial>
<issued>June 29, 2022</issued>
<expires>August 04, 2024</expires>
<expired>no</expired>
<custom>
<_Log_Storage_TB>7</_Log_Storage_TB>
</custom>
<authcode/>
</entry>
<entry>
<feature>Premium</feature>
<description>24 x 7 phone support; advanced replacement hardware service</description>
<serial>00000000000</serial>
<issued>May 02, 2023</issued>
<expires>June 30, 2028</expires>
<expired>no</expired>
<authcode>12345678</authcode>
</entry>
</licenses>
</result>
</response>
Expand All @@ -281,15 +302,38 @@ def test_get_licenses(self, fw_proxy_mock):
fw_proxy_mock.op.return_value = raw_response

assert fw_proxy_mock.get_licenses() == {
"PAN-DB URL Filtering": {
"authcode": None,
"base-license-name": "PA-VM",
"description": "Palo Alto Networks URL Filtering " "License",
"expired": "no",
"expires": "December 31, 2023",
"feature": "PAN-DB URL Filtering",
"issued": "April 20, 2023",
"serial": "00000000000000",
'Logging Service': {
'authcode': None,
'custom': {
'_Log_Storage_TB': '7'
},
'description': 'Device Logging Service',
'expired': 'no',
'expires': 'August 04, 2024',
'feature': 'Logging Service',
'issued': 'June 29, 2022',
'serial': '00000000000'
},
'PAN-DB URL Filtering': {
'authcode': None,
'base-license-name': 'PA-VM',
'description': 'Palo Alto Networks URL Filtering '
'License',
'expired': 'no',
'expires': 'December 31, 2023',
'feature': 'PAN-DB URL Filtering',
'issued': 'April 20, 2023',
'serial': '00000000000000'
},
'Premium': {
'authcode': '12345678',
'description': '24 x 7 phone support; advanced replacement '
'hardware service',
'expired': 'no',
'expires': 'June 30, 2028',
'feature': 'Premium',
'issued': 'May 02, 2023',
'serial': '00000000000'
},
}

Expand Down
2 changes: 1 addition & 1 deletion tests/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def test_init_exception_unknown_parameter(self, valid_config_elements, requested
"""Check if exception is raised when ConfigParser is called with unknown param in requested config."""
with pytest.raises(
UnknownParameterException,
match=r"Unknown configuration parameter passed: .*$",
match=r"Unknown configuration parameters passed: .*$",
):
ConfigParser(valid_config_elements, requested_config)

Expand Down
Loading