From fde8af8d0b5634883393922079c3a688dcec9dd5 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Mon, 25 Nov 2019 00:45:24 -0500 Subject: [PATCH 1/8] pbrd: don't set rule removed on fail Don't treat a remove failure as a successful remove. This can cause us to get out of sync with the kernel. Pbrd makes decisions on rule handling based on its installed state so this needs to be as close to accurate as possible. Signed-off-by: Stephen Worley --- pbrd/pbr_zebra.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pbrd/pbr_zebra.c b/pbrd/pbr_zebra.c index b0a689a7e4..f0b0f8891a 100644 --- a/pbrd/pbr_zebra.c +++ b/pbrd/pbr_zebra.c @@ -244,6 +244,11 @@ static int rule_notify_owner(ZAPI_CALLBACK_ARGS) __PRETTY_FUNCTION__, pbrms->installed); break; case ZAPI_RULE_FAIL_REMOVE: + /* Don't change state on rule removal failure */ + DEBUGD(&pbr_dbg_zebra, + "%s: Received RULE_FAIL_REMOVED: %" PRIu64, + __PRETTY_FUNCTION__, pbrms->installed); + break; case ZAPI_RULE_REMOVED: pbrms->installed &= ~installed; DEBUGD(&pbr_dbg_zebra, "%s: Received RULE REMOVED: %" PRIu64, From 0dcff6f46397c4d7d5acbf9b71f63d2528c60300 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Mon, 25 Nov 2019 14:45:02 -0500 Subject: [PATCH 2/8] pbrd: don't silently fail on atomic match IP change attempts Currently pbrd does not support the abilitity to make atomic changes to a config. ex) `match src-ip 1.1.1.1/32` `match src-ip 1.1.1.0/24` We would overwrite the first one but never actually install it. In the `set nexthop commands` we explicitly fail if there is already a `set nexthop` config present. This patch extends the match src/dest-ip configs to do the same. In the future we should make all these commands atomic but for now its better to not fail silently at the very least. Signed-off-by: Stephen Worley --- pbrd/pbr_vty.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/pbrd/pbr_vty.c b/pbrd/pbr_vty.c index bc4aa947a9..5bc94a2b33 100644 --- a/pbrd/pbr_vty.c +++ b/pbrd/pbr_vty.c @@ -127,10 +127,13 @@ DEFPY(pbr_map_match_src, pbr_map_match_src_cmd, pbrms->family = prefix->family; if (!no) { - if (prefix_same(pbrms->src, prefix)) + if (pbrms->src && prefix_same(pbrms->src, prefix)) return CMD_SUCCESS; - - if (!pbrms->src) + else if (pbrms->src) { + vty_out(vty, + "A `match src-ip XX` command already exists, please remove that first\n"); + return CMD_WARNING_CONFIG_FAILED; + } else pbrms->src = prefix_new(); prefix_copy(pbrms->src, prefix); } else @@ -154,10 +157,13 @@ DEFPY(pbr_map_match_dst, pbr_map_match_dst_cmd, pbrms->family = prefix->family; if (!no) { - if (prefix_same(pbrms->dst, prefix)) + if (pbrms->dst && prefix_same(pbrms->dst, prefix)) return CMD_SUCCESS; - - if (!pbrms->dst) + else if (pbrms->dst) { + vty_out(vty, + "A `match dst-ip XX` command already exists, please remove that first\n"); + return CMD_WARNING_CONFIG_FAILED; + } else pbrms->dst = prefix_new(); prefix_copy(pbrms->dst, prefix); } else From 46b0382056881b346624d340d8d835de742f18fd Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Mon, 25 Nov 2019 15:16:35 -0500 Subject: [PATCH 3/8] pbrd: don't silently fail on atomic match MARK change attempts Also don't silently fail when we attempt to atomically change a match MARK to a new one. We would overwrite the frist one but never actually install it. Change it to explicitly fail if a config is already present for now. Signed-off-by: Stephen Worley --- pbrd/pbr_vty.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/pbrd/pbr_vty.c b/pbrd/pbr_vty.c index 5bc94a2b33..24d3c0fd13 100644 --- a/pbrd/pbr_vty.c +++ b/pbrd/pbr_vty.c @@ -189,9 +189,14 @@ DEFPY(pbr_map_match_mark, pbr_map_match_mark_cmd, #endif if (!no) { - if (pbrms->mark == (uint32_t) mark) + if (pbrms->mark && pbrms->mark == (uint32_t)mark) return CMD_SUCCESS; - pbrms->mark = (uint32_t) mark; + else if (pbrms->mark) { + vty_out(vty, + "A `match mark XX` command already exists, please remove that first\n"); + return CMD_WARNING_CONFIG_FAILED; + } else + pbrms->mark = (uint32_t)mark; } else { pbrms->mark = 0; } From 6c4c9a6cc7dffd83f70f36082d2b4e7e15907773 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Mon, 25 Nov 2019 15:22:35 -0500 Subject: [PATCH 4/8] pbrd: use dst string in match dst-ip vty description The vty description for the `set match dst-ip` command was using "src ip" in its description. Change it to use "dst ip". Signed-off-by: Stephen Worley --- pbrd/pbr_vty.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pbrd/pbr_vty.c b/pbrd/pbr_vty.c index 24d3c0fd13..42be654372 100644 --- a/pbrd/pbr_vty.c +++ b/pbrd/pbr_vty.c @@ -148,7 +148,7 @@ DEFPY(pbr_map_match_dst, pbr_map_match_dst_cmd, "[no] match dst-ip $prefix", NO_STR "Match the rest of the command\n" - "Choose the src ip or ipv6 prefix to use\n" + "Choose the dst ip or ipv6 prefix to use\n" "v4 Prefix\n" "v6 Prefix\n") { From 47f94d175a6f52cde83c3ff319304cd918acbb93 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Mon, 25 Nov 2019 15:25:08 -0500 Subject: [PATCH 5/8] pbrd: Add newlines in `set nexthop*` vty output We were missing some newlines in handling vty outputs for the `set nexthop*` commands. Add them in there. Signed-off-by: Stephen Worley --- pbrd/pbr_vty.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pbrd/pbr_vty.c b/pbrd/pbr_vty.c index 42be654372..9f4e5e7fc9 100644 --- a/pbrd/pbr_vty.c +++ b/pbrd/pbr_vty.c @@ -243,7 +243,7 @@ DEFPY(pbr_map_nexthop_group, pbr_map_nexthop_group_cmd, pbr_map_delete_nexthops(pbrms); else { vty_out(vty, - "Nexthop Group specified: %s does not exist to remove", + "Nexthop Group specified: %s does not exist to remove\n", name); return CMD_WARNING_CONFIG_FAILED; } @@ -251,7 +251,7 @@ DEFPY(pbr_map_nexthop_group, pbr_map_nexthop_group_cmd, if (pbrms->nhgrp_name) { if (strcmp(name, pbrms->nhgrp_name) != 0) { vty_out(vty, - "Please delete current nexthop group before modifying current one"); + "Please delete current nexthop group before modifying current one\n"); return CMD_WARNING_CONFIG_FAILED; } @@ -288,7 +288,7 @@ DEFPY(pbr_map_nexthop, pbr_map_nexthop_cmd, if (pbrms->nhgrp_name) { vty_out(vty, - "Please unconfigure the nexthop group before adding an individual nexthop"); + "Please unconfigure the nexthop group before adding an individual nexthop\n"); return CMD_WARNING_CONFIG_FAILED; } @@ -370,7 +370,7 @@ DEFPY(pbr_map_nexthop, pbr_map_nexthop_cmd, if (pbrms->nhg->nexthop) { vty_out(vty, - "If you would like more than one nexthop please use nexthop-groups"); + "If you would like more than one nexthop please use nexthop-groups\n"); return CMD_WARNING_CONFIG_FAILED; } From eb5d458b3f5e2813ad6c473323b2b37e243aad4f Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Tue, 3 Dec 2019 16:14:34 -0500 Subject: [PATCH 6/8] lib: Add zapi_rule_notify_owner2str() function Add a function for converting the zapi_rule_notify_owner enum type to a string for ease of use. Signed-off-by: Stephen Worley --- lib/zclient.h | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/lib/zclient.h b/lib/zclient.h index 7adb294a31..99ed34138e 100644 --- a/lib/zclient.h +++ b/lib/zclient.h @@ -485,6 +485,29 @@ enum zapi_iptable_notify_owner { ZAPI_IPTABLE_FAIL_REMOVE, }; +static inline const char * +zapi_rule_notify_owner2str(enum zapi_rule_notify_owner note) +{ + const char *ret = "UNKNOWN"; + + switch (note) { + case ZAPI_RULE_FAIL_INSTALL: + ret = "ZAPI_RULE_FAIL_INSTALL"; + break; + case ZAPI_RULE_INSTALLED: + ret = "ZAPI_RULE_INSTALLED"; + break; + case ZAPI_RULE_FAIL_REMOVE: + ret = "ZAPI_RULE_FAIL_REMOVE"; + break; + case ZAPI_RULE_REMOVED: + ret = "ZAPI_RULE_REMOVED"; + break; + } + + return ret; +} + /* Zebra MAC types */ #define ZEBRA_MACIP_TYPE_STICKY 0x01 /* Sticky MAC*/ #define ZEBRA_MACIP_TYPE_GW 0x02 /* gateway (SVI) mac*/ From 23e8679f0d1872a1d55ee74a827427b907fd2309 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Tue, 3 Dec 2019 16:25:40 -0500 Subject: [PATCH 7/8] pbrd: consolidate rule_notify debugs into one call Consolidate the rule_notify_owner() debugs based on type into one call, making use of zapi_rule_notify_owner2str() to do so. Signed-off-by: Stephen Worley --- pbrd/pbr_zebra.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/pbrd/pbr_zebra.c b/pbrd/pbr_zebra.c index f0b0f8891a..06ad0f40a4 100644 --- a/pbrd/pbr_zebra.c +++ b/pbrd/pbr_zebra.c @@ -234,28 +234,21 @@ static int rule_notify_owner(ZAPI_CALLBACK_ARGS) switch (note) { case ZAPI_RULE_FAIL_INSTALL: pbrms->installed &= ~installed; - DEBUGD(&pbr_dbg_zebra, - "%s: Received RULE_FAIL_INSTALL: %" PRIu64, - __PRETTY_FUNCTION__, pbrms->installed); break; case ZAPI_RULE_INSTALLED: pbrms->installed |= installed; - DEBUGD(&pbr_dbg_zebra, "%s: Received RULE_INSTALLED: %" PRIu64, - __PRETTY_FUNCTION__, pbrms->installed); break; case ZAPI_RULE_FAIL_REMOVE: /* Don't change state on rule removal failure */ - DEBUGD(&pbr_dbg_zebra, - "%s: Received RULE_FAIL_REMOVED: %" PRIu64, - __PRETTY_FUNCTION__, pbrms->installed); break; case ZAPI_RULE_REMOVED: pbrms->installed &= ~installed; - DEBUGD(&pbr_dbg_zebra, "%s: Received RULE REMOVED: %" PRIu64, - __PRETTY_FUNCTION__, pbrms->installed); break; } + DEBUGD(&pbr_dbg_zebra, "%s: Received %s: %" PRIu64, __func__, + zapi_rule_notify_owner2str(note), pbrms->installed); + pbr_map_final_interface_deletion(pbrms->parent, pmi); return 0; From 5d0e49c4fc4d53866bd8fac77acd4adcb7b100c0 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Tue, 3 Dec 2019 16:59:21 -0500 Subject: [PATCH 8/8] pbrd: make vty `match *` code more readable Make the vty match src|dst|mark code a bit more readable in its conditonal logic. Signed-off-by: Stephen Worley --- pbrd/pbr_vty.c | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/pbrd/pbr_vty.c b/pbrd/pbr_vty.c index 9f4e5e7fc9..53248f5aaf 100644 --- a/pbrd/pbr_vty.c +++ b/pbrd/pbr_vty.c @@ -127,14 +127,16 @@ DEFPY(pbr_map_match_src, pbr_map_match_src_cmd, pbrms->family = prefix->family; if (!no) { - if (pbrms->src && prefix_same(pbrms->src, prefix)) - return CMD_SUCCESS; - else if (pbrms->src) { + if (pbrms->src) { + if (prefix_same(pbrms->src, prefix)) + return CMD_SUCCESS; + vty_out(vty, "A `match src-ip XX` command already exists, please remove that first\n"); return CMD_WARNING_CONFIG_FAILED; - } else - pbrms->src = prefix_new(); + } + + pbrms->src = prefix_new(); prefix_copy(pbrms->src, prefix); } else prefix_free(&pbrms->src); @@ -157,14 +159,16 @@ DEFPY(pbr_map_match_dst, pbr_map_match_dst_cmd, pbrms->family = prefix->family; if (!no) { - if (pbrms->dst && prefix_same(pbrms->dst, prefix)) - return CMD_SUCCESS; - else if (pbrms->dst) { + if (pbrms->dst) { + if (prefix_same(pbrms->dst, prefix)) + return CMD_SUCCESS; + vty_out(vty, "A `match dst-ip XX` command already exists, please remove that first\n"); return CMD_WARNING_CONFIG_FAILED; - } else - pbrms->dst = prefix_new(); + } + + pbrms->dst = prefix_new(); prefix_copy(pbrms->dst, prefix); } else prefix_free(&pbrms->dst); @@ -189,17 +193,18 @@ DEFPY(pbr_map_match_mark, pbr_map_match_mark_cmd, #endif if (!no) { - if (pbrms->mark && pbrms->mark == (uint32_t)mark) - return CMD_SUCCESS; - else if (pbrms->mark) { + if (pbrms->mark) { + if (pbrms->mark == (uint32_t)mark) + return CMD_SUCCESS; + vty_out(vty, "A `match mark XX` command already exists, please remove that first\n"); return CMD_WARNING_CONFIG_FAILED; - } else - pbrms->mark = (uint32_t)mark; - } else { + } + + pbrms->mark = (uint32_t)mark; + } else pbrms->mark = 0; - } pbr_map_check(pbrms);