diff --git a/changes/907.fixed b/changes/907.fixed new file mode 100644 index 00000000..6983d592 --- /dev/null +++ b/changes/907.fixed @@ -0,0 +1 @@ +Fixed an indented banner aborting the parse of an entire config. `is_banner_start` matched the stripped line while the banner patterns anchor at the start of it, and where the two disagreed the resulting `ValueError` left `__init__`. Affected the Cisco IOS, NX-OS, IOS-XR and Arista EOS parsers. diff --git a/netutils/config/parser.py b/netutils/config/parser.py index 09c9f6e0..e3882cbc 100644 --- a/netutils/config/parser.py +++ b/netutils/config/parser.py @@ -580,6 +580,14 @@ def is_banner_one_line(config_line: str) -> bool: def is_banner_start(self, line: str) -> bool: """Determine if the line starts a banner config.""" state = super(CiscoConfigParser, self).is_banner_start(line) + if state and not self.regex_banner.match(line): + # The inherited check matches the stripped line, while regex_banner + # anchors `banner` at the start of it. Where the two disagree the + # line cannot yield a delimiter, and setting banner_end below would + # raise out of __init__ and lose the whole config. Treat it as an + # ordinary line instead, which is what ASAConfigParser already does + # by not stripping in its own is_banner_start. + return False if state: self.banner_end = line return state @@ -728,6 +736,17 @@ class EOSConfigParser(BaseSpaceConfigParser): banner_end = "EOF" + def is_banner_start(self, line: str) -> bool: + """Determine if the line starts a banner config. + + Deliberately does not strip the line. The inherited check does, so an + indented `banner` was taken for the start of a banner, and the search + for the "EOF" terminator then ran to the end of the config and raised + out of __init__, losing everything. ASAConfigParser does the same thing + for the same reason. + """ + return any(line.startswith(banner_start) for banner_start in self.banner_start) + def _build_banner(self, config_line: str) -> t.Optional[str]: """Handle banner config lines. diff --git a/tests/unit/test_parser.py b/tests/unit/test_parser.py index f98f9751..0ec06645 100644 --- a/tests/unit/test_parser.py +++ b/tests/unit/test_parser.py @@ -70,6 +70,39 @@ def test_incorrect_banner_ios(): compliance.parser_map["cisco_ios"](banner_cfg).config_lines # pylint: disable=expression-not-assigned +def test_indented_banner_is_not_a_banner_start(): + """An indented banner must not abort the parse of the whole config. + + is_banner_start matched the stripped line while the banner patterns anchor + at the start of it. Where the two disagreed the line could not yield a + delimiter, and the resulting ValueError left __init__, so one such line lost + the entire config. ASAConfigParser already avoided this by not stripping. + """ + cfg = "group-policy P attributes\n banner value hello\n" + for network_os in ("cisco_ios", "cisco_nxos", "cisco_asa", "arista_eos", "cisco_iosxr"): + config_lines = compliance.parser_map[network_os](cfg).config_lines + assert [line.config_line for line in config_lines] == [ + "group-policy P attributes", + " banner value hello", + ], network_os + + +@pytest.mark.parametrize( + ("network_os", "banner"), + [ + ("cisco_ios", "banner motd ^C\nhello\n^C\n"), + ("cisco_nxos", "banner motd ^C\nhello\n^C\n"), + ("arista_eos", "banner motd EOF\nhello\nEOF\n"), + ], +) +def test_top_level_banner_still_folds(network_os, banner): + """The guard above must not stop a real, unindented banner being folded.""" + config_lines = compliance.parser_map[network_os](banner + "hostname r1\n").config_lines + assert config_lines[0].config_line.startswith("banner motd") + assert "hello" in config_lines[1].config_line + assert config_lines[-1].config_line == "hostname r1" + + def test_duplicate_line(): logging = ( "!\n"