From 1a11d9cd1e5305db560bb5cbb8200c9ad699d1ff Mon Sep 17 00:00:00 2001 From: Emanuele Di Pascale Date: Wed, 9 Oct 2019 17:24:33 +0200 Subject: [PATCH 1/5] tools: non hardcoded paths in frr-reload allow command line parameters to specify different folder for the vtysh binary, config file location and temporary file. Keep the old hardcoded paths as default values for those options to preserve current functionality. Signed-off-by: Emanuele Di Pascale --- tools/frr-reload.py | 57 ++++++++++++++++++++++++++++++--------------- 1 file changed, 38 insertions(+), 19 deletions(-) diff --git a/tools/frr-reload.py b/tools/frr-reload.py index e182c77c78..56be1b29b1 100755 --- a/tools/frr-reload.py +++ b/tools/frr-reload.py @@ -114,7 +114,7 @@ class Config(object): self.lines = [] self.contexts = OrderedDict() - def load_from_file(self, filename): + def load_from_file(self, filename, bindir, confdir): """ Read configuration from specified file and slurp it into internal memory The internal representation has been marked appropriately by passing it @@ -123,7 +123,7 @@ class Config(object): log.info('Loading Config object from file %s', filename) try: - file_output = subprocess.check_output(['/usr/bin/vtysh', '-m', '-f', filename], + file_output = subprocess.check_output([str(bindir + '/vtysh'), '-m', '-f', filename, '--config_dir', confdir], stderr=subprocess.STDOUT) except subprocess.CalledProcessError as e: ve = VtyshMarkException(e) @@ -144,7 +144,7 @@ class Config(object): self.load_contexts() - def load_from_show_running(self): + def load_from_show_running(self, bindir, confdir): """ Read running configuration and slurp it into internal memory The internal representation has been marked appropriately by passing it @@ -154,7 +154,7 @@ class Config(object): try: config_text = subprocess.check_output( - "/usr/bin/vtysh -c 'show run' | /usr/bin/tail -n +4 | /usr/bin/vtysh -m -f -", + bindir + "/vtysh --config_dir " + confdir + " -c 'show run' | /usr/bin/tail -n +4 | " + bindir + "/vtysh --config_dir " + confdir + " -m -f -", shell=True, stderr=subprocess.STDOUT) except subprocess.CalledProcessError as e: ve = VtyshMarkException(e) @@ -528,13 +528,15 @@ end self.save_contexts(ctx_keys, current_context_lines) -def line_to_vtysh_conft(ctx_keys, line, delete): +def line_to_vtysh_conft(ctx_keys, line, delete, bindir, confdir): """ Return the vtysh command for the specified context line """ cmd = [] - cmd.append('vtysh') + cmd.append(str(bindir + '/vtysh')) + cmd.append('--config_dir') + cmd.append(confdir) cmd.append('-c') cmd.append('conf t') @@ -1074,7 +1076,7 @@ def compare_context_objects(newconf, running): -def vtysh_config_available(): +def vtysh_config_available(bindir, confdir): """ Return False if no frr daemon is running or some other vtysh session is in 'configuration terminal' mode which will prevent us from making any @@ -1082,7 +1084,7 @@ def vtysh_config_available(): """ try: - cmd = ['/usr/bin/vtysh', '-c', 'conf t'] + cmd = [str(bindir + '/vtysh'), '--config_dir', confdir, '-c', 'conf t'] output = subprocess.check_output(cmd, stderr=subprocess.STDOUT).strip() if 'VTY configuration is locked by other VTY' in output.decode('utf-8'): @@ -1110,6 +1112,9 @@ if __name__ == '__main__': parser.add_argument('--stdout', action='store_true', help='Log to STDOUT', default=False) parser.add_argument('filename', help='Location of new frr config file') parser.add_argument('--overwrite', action='store_true', help='Overwrite frr.conf with running config output', default=False) + parser.add_argument('--bindir', help='path to the vtysh executable', default='/usr/bin') + parser.add_argument('--confdir', help='path to the daemon config files', default='/etc/frr') + parser.add_argument('--rundir', help='path for the temp config file', default='/var/run/frr') args = parser.parse_args() # Logging @@ -1149,8 +1154,22 @@ if __name__ == '__main__': log.error(msg) sys.exit(1) + # Verify that confdir is correct + if not os.path.isdir(args.confdir): + msg = "Confdir %s is not a valid path" % args.confdir + print(msg) + log.error(msg) + sys.exit(1) + + # Verify that bindir is correct + if not os.path.isdir(args.bindir) or not os.path.isfile(args.bindir + '/vtysh'): + msg = "Bindir %s is not a valid path to vtysh" % args.bindir + print(msg) + log.error(msg) + sys.exit(1) + # Verify that 'service integrated-vtysh-config' is configured - vtysh_filename = '/etc/frr/vtysh.conf' + vtysh_filename = args.confdir + '/vtysh.conf' service_integrated_vtysh_config = True if os.path.isfile(vtysh_filename): @@ -1175,7 +1194,7 @@ if __name__ == '__main__': # Create a Config object from the config generated by newconf newconf = Config() - newconf.load_from_file(args.filename) + newconf.load_from_file(args.filename, args.bindir, args.confdir) reload_ok = True if args.test: @@ -1184,9 +1203,9 @@ if __name__ == '__main__': running = Config() if args.input: - running.load_from_file(args.input) + running.load_from_file(args.input, args.bindir, args.confdir) else: - running.load_from_show_running() + running.load_from_show_running(args.bindir, args.confdir) (lines_to_add, lines_to_del) = compare_context_objects(newconf, running) lines_to_configure = [] @@ -1220,7 +1239,7 @@ if __name__ == '__main__': elif args.reload: # We will not be able to do anything, go ahead and exit(1) - if not vtysh_config_available(): + if not vtysh_config_available(args.bindir, args.confdir): sys.exit(1) log.debug('New Frr Config\n%s', newconf.get_lines()) @@ -1264,7 +1283,7 @@ if __name__ == '__main__': for x in range(2): running = Config() - running.load_from_show_running() + running.load_from_show_running(args.bindir, args.confdir) log.debug('Running Frr Config (Pass #%d)\n%s', x, running.get_lines()) (lines_to_add, lines_to_del) = compare_context_objects(newconf, running) @@ -1296,7 +1315,7 @@ if __name__ == '__main__': # 'no' commands are tricky, we can't just put them in a file and # vtysh -f that file. See the next comment for an explanation # of their quirks - cmd = line_to_vtysh_conft(ctx_keys, line, True) + cmd = line_to_vtysh_conft(ctx_keys, line, True, args.bindir, args.confdir) original_cmd = cmd # Some commands in frr are picky about taking a "no" of the entire line. @@ -1352,7 +1371,7 @@ if __name__ == '__main__': string.ascii_uppercase + string.digits) for _ in range(6)) - filename = "/var/run/frr/reload-%s.txt" % random_string + filename = args.rundir + "/reload-%s.txt" % random_string log.info("%s content\n%s" % (filename, pformat(lines_to_configure))) with open(filename, 'w') as fh: @@ -1360,15 +1379,15 @@ if __name__ == '__main__': fh.write(line + '\n') try: - subprocess.check_output(['/usr/bin/vtysh', '-f', filename], stderr=subprocess.STDOUT) + subprocess.check_output([str(args.bindir + '/vtysh'), '--config_dir', args.confdir, '-f', filename], stderr=subprocess.STDOUT) except subprocess.CalledProcessError as e: log.warning("frr-reload.py failed due to\n%s" % e.output) reload_ok = False os.unlink(filename) # Make these changes persistent - if args.overwrite or args.filename != '/etc/frr/frr.conf': - subprocess.call(['/usr/bin/vtysh', '-c', 'write']) + if args.overwrite or args.filename != str(args.confdir + '/frr.conf'): + subprocess.call([str(args.bindir + '/vtysh'), '--config_dir', args.confdir, '-c', 'write']) if not reload_ok: sys.exit(1) From 984fd9614d1fa12ec127da81c5c0671f4b359b6e Mon Sep 17 00:00:00 2001 From: Emanuele Di Pascale Date: Tue, 15 Oct 2019 15:25:15 +0200 Subject: [PATCH 2/5] lib: include ldpd to the list of daemons ... for commands such as 'show running-config X'. Signed-off-by: Emanuele Di Pascale --- lib/command.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/command.h b/lib/command.h index d5dc129c72..2176b14615 100644 --- a/lib/command.h +++ b/lib/command.h @@ -414,9 +414,9 @@ struct cmd_node { /* Daemons lists */ #define DAEMONS_STR \ - "For the zebra daemon\nFor the rip daemon\nFor the ripng daemon\nFor the ospf daemon\nFor the ospfv6 daemon\nFor the bgp daemon\nFor the isis daemon\nFor the pbr daemon\nFor the fabricd daemon\nFor the pim daemon\nFor the static daemon\nFor the sharpd daemon\nFor the vrrpd daemon\n" + "For the zebra daemon\nFor the rip daemon\nFor the ripng daemon\nFor the ospf daemon\nFor the ospfv6 daemon\nFor the bgp daemon\nFor the isis daemon\nFor the pbr daemon\nFor the fabricd daemon\nFor the pim daemon\nFor the static daemon\nFor the sharpd daemon\nFor the vrrpd daemon\nFor the ldpd daemon\n" #define DAEMONS_LIST \ - "" + "" /* Prototypes. */ extern void install_node(struct cmd_node *, int (*)(struct vty *)); From d9730542a9b899222bf218be0df94122e7151b15 Mon Sep 17 00:00:00 2001 From: Emanuele Di Pascale Date: Tue, 15 Oct 2019 17:55:19 +0200 Subject: [PATCH 3/5] tools: frr-reload.py for single daemon allow frr-reload.py to be invoked with a --daemon option to specify an individual daemon for which the configuration diff should be computed. This is useful when integrated config is not used and we want to apply a patch to a single daemon config file. No attempt to integrate this with 'service frr reload' has been done. Making watchfrr work with per-daemon config is outside the scope of this simple patch. Signed-off-by: Emanuele Di Pascale --- tools/frr-reload.py | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/tools/frr-reload.py b/tools/frr-reload.py index 56be1b29b1..8a437e8ba3 100755 --- a/tools/frr-reload.py +++ b/tools/frr-reload.py @@ -144,7 +144,7 @@ class Config(object): self.load_contexts() - def load_from_show_running(self, bindir, confdir): + def load_from_show_running(self, bindir, confdir, daemon): """ Read running configuration and slurp it into internal memory The internal representation has been marked appropriately by passing it @@ -154,7 +154,7 @@ class Config(object): try: config_text = subprocess.check_output( - bindir + "/vtysh --config_dir " + confdir + " -c 'show run' | /usr/bin/tail -n +4 | " + bindir + "/vtysh --config_dir " + confdir + " -m -f -", + bindir + "/vtysh --config_dir " + confdir + " -c 'show run " + daemon + "' | /usr/bin/tail -n +4 | " + bindir + "/vtysh --config_dir " + confdir + " -m -f -", shell=True, stderr=subprocess.STDOUT) except subprocess.CalledProcessError as e: ve = VtyshMarkException(e) @@ -1115,6 +1115,8 @@ if __name__ == '__main__': parser.add_argument('--bindir', help='path to the vtysh executable', default='/usr/bin') parser.add_argument('--confdir', help='path to the daemon config files', default='/etc/frr') parser.add_argument('--rundir', help='path for the temp config file', default='/var/run/frr') + parser.add_argument('--daemon', help='daemon for which want to replace the config', default='') + args = parser.parse_args() # Logging @@ -1168,6 +1170,13 @@ if __name__ == '__main__': log.error(msg) sys.exit(1) + # verify that the daemon, if specified, is valid + if args.daemon and args.daemon not in ['zebra', 'bgpd', 'fabricd', 'isisd', 'ospf6d', 'ospfd', 'pbrd', 'pimd', 'ripd', 'ripngd', 'sharpd', 'staticd', 'vrrpd', 'ldpd']: + msg = "Daemon %s is not a valid option for 'show running-config'" % args.daemon + print(msg) + log.error(msg) + sys.exit(1) + # Verify that 'service integrated-vtysh-config' is configured vtysh_filename = args.confdir + '/vtysh.conf' service_integrated_vtysh_config = True @@ -1181,7 +1190,7 @@ if __name__ == '__main__': service_integrated_vtysh_config = False break - if not service_integrated_vtysh_config: + if not service_integrated_vtysh_config and not args.daemon: msg = "'service integrated-vtysh-config' is not configured, this is required for 'service frr reload'" print(msg) log.error(msg) @@ -1205,7 +1214,7 @@ if __name__ == '__main__': if args.input: running.load_from_file(args.input, args.bindir, args.confdir) else: - running.load_from_show_running(args.bindir, args.confdir) + running.load_from_show_running(args.bindir, args.confdir, args.daemon) (lines_to_add, lines_to_del) = compare_context_objects(newconf, running) lines_to_configure = [] @@ -1283,7 +1292,7 @@ if __name__ == '__main__': for x in range(2): running = Config() - running.load_from_show_running(args.bindir, args.confdir) + running.load_from_show_running(args.bindir, args.confdir, args.daemon) log.debug('Running Frr Config (Pass #%d)\n%s', x, running.get_lines()) (lines_to_add, lines_to_del) = compare_context_objects(newconf, running) @@ -1386,7 +1395,8 @@ if __name__ == '__main__': os.unlink(filename) # Make these changes persistent - if args.overwrite or args.filename != str(args.confdir + '/frr.conf'): + target = str(args.confdir + '/frr.conf') + if args.overwrite or (not args.daemon and args.filename != target): subprocess.call([str(args.bindir + '/vtysh'), '--config_dir', args.confdir, '-c', 'write']) if not reload_ok: From ccef6e47a3eda4adb4fd521bc2c4c036b6119d0e Mon Sep 17 00:00:00 2001 From: Emanuele Di Pascale Date: Fri, 11 Oct 2019 12:37:53 +0200 Subject: [PATCH 4/5] tools, vtysh: fix ldpd + frr-reload.py frr-reload.py has many special case rules that did not consider ldpd at all. Specifically: 1. The bulk of ldp configuration comes in a big 'mpls ldp' context, which was previously considered a single-line context as it started with 'mpls'. This rule should only apply to labels and lsps. 2. ldp has a 'router-id' config line that fell into the same rule as the above one. It should not be considered a single-line context as more ldp configuration can follow. 3. enabled interfaces should not end their context. A better fix would actually require popping a new context for each interface in case there is any interface-specific config, but at least this fix will address the most common use case. 4. when declaring pseudowires, any line with 'member pseudowire XXX' should be considered a sub-context of the 'l2vpn YYY type ZZZ' context. Without this fix, changes in the first psuedowire declared would not correctly be processed (e.g. removing a 'control-word exclude' line would not be picked up). Signed-off-by: Emanuele Di Pascale --- tools/frr-reload.py | 17 ++++++++++++----- vtysh/vtysh.c | 4 +--- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/tools/frr-reload.py b/tools/frr-reload.py index 8a437e8ba3..cf31da38b8 100755 --- a/tools/frr-reload.py +++ b/tools/frr-reload.py @@ -404,7 +404,8 @@ end "ip ", "ipv6 ", "log ", - "mpls", + "mpls lsp", + "mpls label", "no ", "password ", "ptm-enable", @@ -424,7 +425,12 @@ end continue # one line contexts - if new_ctx is True and any(line.startswith(keyword) for keyword in oneline_ctx_keywords): + # there is one exception though: ldpd accepts a 'router-id' clause + # as part of its 'mpls ldp' config context. If we are processing + # ldp configuration and encounter a router-id we should NOT switch + # to a new context + if new_ctx is True and any(line.startswith(keyword) for keyword in oneline_ctx_keywords) and not ( + ctx_keys and ctx_keys[0].startswith("mpls ldp") and line.startswith("router-id ")): self.save_contexts(ctx_keys, current_context_lines) # Start a new context @@ -489,7 +495,8 @@ end elif (line.startswith("address-family ") or line.startswith("vnc defaults") or line.startswith("vnc l2-group") or - line.startswith("vnc nve-group")): + line.startswith("vnc nve-group") or + line.startswith("member pseudowire")): main_ctx_key = [] # Save old context first @@ -498,9 +505,9 @@ end main_ctx_key = copy.deepcopy(ctx_keys) log.debug('LINE %-50s: entering sub-context, append to ctx_keys', line) - if line == "address-family ipv6": + if line == "address-family ipv6" and not ctx_keys[0].startswith("mpls ldp"): ctx_keys.append("address-family ipv6 unicast") - elif line == "address-family ipv4": + elif line == "address-family ipv4" and not ctx_keys[0].startswith("mpls ldp"): ctx_keys.append("address-family ipv4 unicast") elif line == "address-family evpn": ctx_keys.append("address-family l2vpn evpn") diff --git a/vtysh/vtysh.c b/vtysh/vtysh.c index 643dcb7edc..f8292c530f 100644 --- a/vtysh/vtysh.c +++ b/vtysh/vtysh.c @@ -725,19 +725,17 @@ int vtysh_mark_file(const char *filename) switch (vty->node) { case LDP_IPV4_IFACE_NODE: if (strncmp(vty_buf_copy, " ", 3)) { - vty_out(vty, " end\n"); vty->node = LDP_IPV4_NODE; } break; case LDP_IPV6_IFACE_NODE: if (strncmp(vty_buf_copy, " ", 3)) { - vty_out(vty, " end\n"); vty->node = LDP_IPV6_NODE; } break; case LDP_PSEUDOWIRE_NODE: if (strncmp(vty_buf_copy, " ", 2)) { - vty_out(vty, " end\n"); + vty_out(vty, " exit\n"); vty->node = LDP_L2VPN_NODE; } break; From 609ac8dd49402e01a8a13d19e38b2eb8b33ff1f2 Mon Sep 17 00:00:00 2001 From: Emanuele Di Pascale Date: Tue, 12 Nov 2019 15:36:15 +0100 Subject: [PATCH 5/5] tools, vtysh: improved fix for ldpd ifaces instead of suppressing the 'exit' markers at the end of each 'interface XXX' clause in the mpls ldp configuration, mark those with a special marker 'exit-ldp-if' and teach the reload script to correctly recognize the new sub-subcontext Signed-off-by: Emanuele Di Pascale --- tools/frr-reload.py | 14 +++++++++++++- vtysh/vtysh.c | 2 ++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/tools/frr-reload.py b/tools/frr-reload.py index cf31da38b8..cc383e06d8 100755 --- a/tools/frr-reload.py +++ b/tools/frr-reload.py @@ -473,7 +473,7 @@ end current_context_lines = [] log.debug('LINE %-50s: popping from subcontext to ctx%-50s', line, ctx_keys) - elif line == "exit-vni": + elif line in ["exit-vni", "exit-ldp-if"]: if sub_main_ctx_key: self.save_contexts(ctx_keys, current_context_lines) @@ -525,6 +525,18 @@ end sub_main_ctx_key = copy.deepcopy(ctx_keys) log.debug('LINE %-50s: entering sub-sub-context, append to ctx_keys', line) ctx_keys.append(line) + + elif ((line.startswith("interface ") and + len(ctx_keys) == 2 and + ctx_keys[0].startswith('mpls ldp') and + ctx_keys[1].startswith('address-family'))): + + # Save old context first + self.save_contexts(ctx_keys, current_context_lines) + current_context_lines = [] + sub_main_ctx_key = copy.deepcopy(ctx_keys) + log.debug('LINE %-50s: entering sub-sub-context, append to ctx_keys', line) + ctx_keys.append(line) else: # Continuing in an existing context, add non-commented lines to it diff --git a/vtysh/vtysh.c b/vtysh/vtysh.c index f8292c530f..5c4e8a313b 100644 --- a/vtysh/vtysh.c +++ b/vtysh/vtysh.c @@ -725,11 +725,13 @@ int vtysh_mark_file(const char *filename) switch (vty->node) { case LDP_IPV4_IFACE_NODE: if (strncmp(vty_buf_copy, " ", 3)) { + vty_out(vty, " exit-ldp-if\n"); vty->node = LDP_IPV4_NODE; } break; case LDP_IPV6_IFACE_NODE: if (strncmp(vty_buf_copy, " ", 3)) { + vty_out(vty, " exit-ldp-if\n"); vty->node = LDP_IPV6_NODE; } break;