-
Notifications
You must be signed in to change notification settings - Fork 30
Add Fortigate FortiOS Driver #160
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.
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
base: master
Are you sure you want to change the base?
Conversation
| self, config: HConfigChild, other_children: Iterable[HConfigChild] | ||
| ) -> Optional[HConfigChild]: | ||
| """Override idempotent_for to only consider a config idempotent | ||
| if the same command exists in the other set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should describe the set command idempotency convention being assumed in the docstring. Something like "FortiOS set command idempotency can be assumed by examining the first two words of the command. Take set subnet 1.1.1.1 255.255.255.0 and set subnet 2.2.2.1 255.255.255.0 for example, we only need to compare the first two words set subnet to establish idempotency. Other FortiOS set commands seem to follow the same convention."
| ), | ||
| ], | ||
| parent_allows_duplicate_child=[ | ||
| ParentAllowsDuplicateChildRule(match_rules=()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are some examples of duplicate commands at the root level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I reset the modifications that I made and comment out the parent_allows_duplicate_child section, I get:
__________________________________________ test_swap_negation ___________________________________________
def test_swap_negation() -> None:
platform = Platform.FORTINET_FORTIOS
> running_config = get_hconfig_fast_load(
platform,
(
"config system interface",
" edit port1",
" set description 'Port 1'",
" set status down",
" next",
"end",
"config system dns",
" set primary 192.0.2.1",
" set secondary 192.0.2.2",
"end",
),
)
tests/test_driver_fortinet_fortios.py:8:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
hier_config/constructors.py:151: in get_hconfig_fast_load
most_recent_item, current_section = _analyze_indent(
hier_config/constructors.py:186: in _analyze_indent
most_recent_item = current_section.add_child(line)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = HConfig(driver=HConfigDriverFortinetFortiOS, lines=('config system interface', ' edit port1', " set description 'Port 1'", ' set status down', ' next', 'end', 'config system dns', ' set primary 192.0.2.1', ' set secondary 192.0.2.2'))
text = 'end'
def add_child(
self,
text: str,
*,
return_if_present: bool = False,
check_if_present: bool = True,
) -> HConfigChild:
"""Add a child instance of HConfigChild."""
if not text:
message = "text was empty"
raise ValueError(message)
if check_if_present and (child := self.children.get(text)):
if self._is_duplicate_child_allowed():
new_child = self.instantiate_child(text)
self.children.append(new_child, update_mapping=False)
return new_child
if return_if_present:
return child
message = f"Found a duplicate section: {(*self.path(), text)}"
> raise DuplicateChildError(message)
E hier_config.exceptions.DuplicateChildError: Found a duplicate section: ('end',)
hier_config/base.py:88: DuplicateChildError
____________________________________________________________ test_idempotent_for ____________________________________________________________
def test_idempotent_for() -> None:
platform = Platform.FORTINET_FORTIOS
> running_config = get_hconfig_fast_load(
platform,
(
"config system interface",
" edit port1",
" set description 'Old Description'",
" set status up",
" next",
"end",
"config system dns",
" set primary 192.0.2.1",
" set secondary 192.0.2.2",
"end",
),
)
tests/test_driver_fortinet_fortios.py:51:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
hier_config/constructors.py:151: in get_hconfig_fast_load
most_recent_item, current_section = _analyze_indent(
hier_config/constructors.py:186: in _analyze_indent
most_recent_item = current_section.add_child(line)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = HConfig(driver=HConfigDriverFortinetFortiOS, lines=('config system interface', ' edit port1', " set description 'Old Description'", ' set status up', ' next', 'end', 'config system dns', ' set primary 192.0.2.1', ' set secondary 192.0.2.2'))
text = 'end'
def add_child(
self,
text: str,
*,
return_if_present: bool = False,
check_if_present: bool = True,
) -> HConfigChild:
"""Add a child instance of HConfigChild."""
if not text:
message = "text was empty"
raise ValueError(message)
if check_if_present and (child := self.children.get(text)):
if self._is_duplicate_child_allowed():
new_child = self.instantiate_child(text)
self.children.append(new_child, update_mapping=False)
return new_child
if return_if_present:
return child
message = f"Found a duplicate section: {(*self.path(), text)}"
> raise DuplicateChildError(message)
E hier_config.exceptions.DuplicateChildError: Found a duplicate section: ('end',)
hier_config/base.py:88: DuplicateChildError
========================================================== short test summary info ==========================================================
FAILED tests/test_driver_fortinet_fortios.py::test_swap_negation - hier_config.exceptions.DuplicateChildError: Found a duplicate section: ('end',)
FAILED tests/test_driver_fortinet_fortios.py::test_idempotent_for - hier_config.exceptions.DuplicateChildError: Found a duplicate section: ('end',)
======================================================= 2 failed, 88 passed in 0.98s ========================================================
| @staticmethod | ||
| def _instantiate_rules() -> HConfigDriverRules: | ||
| return HConfigDriverRules( | ||
| sectional_exiting=[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would next need a sectional exiting rule?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
next is the exit text for the edit sections`.
|
I confirmed that the DuplciateChild error is impacting XR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds initial support for Fortinet FortiOS in the configuration management library and refactors driver lookup.
- Introduces
Platform.FORTINET_FORTIOSand implementsHConfigDriverFortinetFortiOSwith custom rules and idempotency behavior. - Refactors
get_hconfig_driverto use a mapping for platform‐to‐driver resolution. - Enhances
get_hconfig_fast_loadto apply per‐line substitutions before parsing, and adds tests for both FortiOS and Cisco XR.
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_driver_fortinet_fortios.py | New unit tests covering negation swapping, idempotency, future |
| tests/test_driver_cisco_xr.py | New test for duplicate‐child removal in Cisco XR driver |
| hier_config/platforms/fortinet_fortios/driver.py | New HConfigDriverFortinetFortiOS class with rule overrides |
| hier_config/models.py | Added FORTINET_FORTIOS to Platform enum |
| hier_config/constructors.py | Refactored get_hconfig_driver to use a dictionary and updated fast‐load parsing logic |
Comments suppressed due to low confidence (2)
hier_config/constructors.py:35
- [nitpick] The
platform_driversmapping is defined inline withinget_hconfig_driver. For better readability and reuse, consider moving this dictionary to a module‐level constant.
platform_drivers: dict[Platform, type[HConfigDriverBase]] = {
tests/test_driver_cisco_xr.py:1
- [nitpick] This PR includes a new Cisco XR test without any corresponding driver changes for XR. It may be clearer to split that test into a separate PR or update the driver in the same change to keep concerns focused.
from hier_config import get_hconfig_fast_load
closes #159