-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
yang/zebra: migrate route map to northbound #5183
yang/zebra: migrate route map to northbound #5183
Conversation
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-9295/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings
CLANG Static Analyzer Summary
Fixed warnings:
New warnings:
Static Analysis warning summary compared to base:
1 Static Analyzer issues remaining.See details at |
120afb0
to
9eb6b53
Compare
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.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/609648c977242dcab9342caf6d09e3c6/raw/64ce04cb674a30932202680f38d1e9110fbcb8f3/cr_5183_1580923348.diff | git apply
diff --git a/zebra/zebra_routemap.c b/zebra/zebra_routemap.c
index 49eeb60f2..ba25e525b 100644
--- a/zebra/zebra_routemap.c
+++ b/zebra/zebra_routemap.c
@@ -351,14 +351,12 @@ static int ip_nht_rm_del(struct zebra_vrf *zvrf, const char *rmap, int rtype,
return CMD_SUCCESS;
}
-DEFPY(
- match_ip_address_prefix_len, match_ip_address_prefix_len_cmd,
- "match ip address prefix-len (0-32)$length",
- MATCH_STR
- IP_STR
- "Match prefix length of IP address\n"
- "Match prefix length of IP address\n"
- "Prefix length\n")
+DEFPY(match_ip_address_prefix_len, match_ip_address_prefix_len_cmd,
+ "match ip address prefix-len (0-32)$length",
+ MATCH_STR IP_STR
+ "Match prefix length of IP address\n"
+ "Match prefix length of IP address\n"
+ "Prefix length\n")
{
const char *xpath = "./match-condition[condition='ipv4-prefix-length']";
char xpath_value[XPATH_MAXLEN];
@@ -371,15 +369,12 @@ DEFPY(
return nb_cli_apply_changes(vty, NULL);
}
-DEFPY(
- no_match_ip_address_prefix_len, no_match_ip_address_prefix_len_cmd,
- "no match ip address prefix-len [(0-32)]",
- NO_STR
- MATCH_STR
- IP_STR
- "Match prefix length of IP address\n"
- "Match prefix length of IP address\n"
- "Prefix length\n")
+DEFPY(no_match_ip_address_prefix_len, no_match_ip_address_prefix_len_cmd,
+ "no match ip address prefix-len [(0-32)]",
+ NO_STR MATCH_STR IP_STR
+ "Match prefix length of IP address\n"
+ "Match prefix length of IP address\n"
+ "Prefix length\n")
{
const char *xpath = "./match-condition[condition='ipv4-prefix-length']";
@@ -388,14 +383,12 @@ DEFPY(
return nb_cli_apply_changes(vty, NULL);
}
-DEFPY(
- match_ipv6_address_prefix_len, match_ipv6_address_prefix_len_cmd,
- "match ipv6 address prefix-len (0-128)$length",
- MATCH_STR
- IPV6_STR
- "Match prefix length of IPv6 address\n"
- "Match prefix length of IPv6 address\n"
- "Prefix length\n")
+DEFPY(match_ipv6_address_prefix_len, match_ipv6_address_prefix_len_cmd,
+ "match ipv6 address prefix-len (0-128)$length",
+ MATCH_STR IPV6_STR
+ "Match prefix length of IPv6 address\n"
+ "Match prefix length of IPv6 address\n"
+ "Prefix length\n")
{
const char *xpath = "./match-condition[condition='ipv6-prefix-length']";
char xpath_value[XPATH_MAXLEN];
@@ -408,15 +401,12 @@ DEFPY(
return nb_cli_apply_changes(vty, NULL);
}
-DEFPY(
- no_match_ipv6_address_prefix_len, no_match_ipv6_address_prefix_len_cmd,
- "no match ipv6 address prefix-len [(0-128)]",
- NO_STR
- MATCH_STR
- IPV6_STR
- "Match prefix length of IPv6 address\n"
- "Match prefix length of IPv6 address\n"
- "Prefix length\n")
+DEFPY(no_match_ipv6_address_prefix_len, no_match_ipv6_address_prefix_len_cmd,
+ "no match ipv6 address prefix-len [(0-128)]",
+ NO_STR MATCH_STR IPV6_STR
+ "Match prefix length of IPv6 address\n"
+ "Match prefix length of IPv6 address\n"
+ "Prefix length\n")
{
const char *xpath = "./match-condition[condition='ipv6-prefix-length']";
@@ -425,14 +415,12 @@ DEFPY(
return nb_cli_apply_changes(vty, NULL);
}
-DEFPY(
- match_ip_nexthop_prefix_len, match_ip_nexthop_prefix_len_cmd,
- "match ip next-hop prefix-len (0-32)$length",
- MATCH_STR
- IP_STR
- "Match prefixlen of nexthop IP address\n"
- "Match prefixlen of given nexthop\n"
- "Prefix length\n")
+DEFPY(match_ip_nexthop_prefix_len, match_ip_nexthop_prefix_len_cmd,
+ "match ip next-hop prefix-len (0-32)$length",
+ MATCH_STR IP_STR
+ "Match prefixlen of nexthop IP address\n"
+ "Match prefixlen of given nexthop\n"
+ "Prefix length\n")
{
const char *xpath =
"./match-condition[condition='ipv4-next-hop-prefix-length']";
@@ -446,15 +434,12 @@ DEFPY(
return nb_cli_apply_changes(vty, NULL);
}
-DEFPY(
- no_match_ip_nexthop_prefix_len, no_match_ip_nexthop_prefix_len_cmd,
- "no match ip next-hop prefix-len [(0-32)]",
- NO_STR
- MATCH_STR
- IP_STR
- "Match prefixlen of nexthop IP address\n"
- "Match prefix length of nexthop\n"
- "Prefix length\n")
+DEFPY(no_match_ip_nexthop_prefix_len, no_match_ip_nexthop_prefix_len_cmd,
+ "no match ip next-hop prefix-len [(0-32)]",
+ NO_STR MATCH_STR IP_STR
+ "Match prefixlen of nexthop IP address\n"
+ "Match prefix length of nexthop\n"
+ "Prefix length\n")
{
const char *xpath =
"./match-condition[condition='ipv4-next-hop-prefix-length']";
@@ -464,12 +449,10 @@ DEFPY(
return nb_cli_apply_changes(vty, NULL);
}
-DEFPY(
- match_source_protocol, match_source_protocol_cmd,
- "match source-protocol " FRR_REDIST_STR_ZEBRA "$proto",
- MATCH_STR
- "Match protocol via which the route was learnt\n"
- FRR_REDIST_HELP_STR_ZEBRA)
+DEFPY(match_source_protocol, match_source_protocol_cmd,
+ "match source-protocol " FRR_REDIST_STR_ZEBRA "$proto",
+ MATCH_STR
+ "Match protocol via which the route was learnt\n" FRR_REDIST_HELP_STR_ZEBRA)
{
const char *xpath = "./match-condition[condition='source-protocol']";
char xpath_value[XPATH_MAXLEN];
@@ -482,13 +465,10 @@ DEFPY(
return nb_cli_apply_changes(vty, NULL);
}
-DEFPY(
- no_match_source_protocol, no_match_source_protocol_cmd,
- "no match source-protocol [" FRR_REDIST_STR_ZEBRA "]",
- NO_STR
- MATCH_STR
- "Match protocol via which the route was learnt\n"
- FRR_REDIST_HELP_STR_ZEBRA)
+DEFPY(no_match_source_protocol, no_match_source_protocol_cmd,
+ "no match source-protocol [" FRR_REDIST_STR_ZEBRA "]",
+ NO_STR MATCH_STR
+ "Match protocol via which the route was learnt\n" FRR_REDIST_HELP_STR_ZEBRA)
{
const char *xpath = "./match-condition[condition='source-protocol']";
@@ -497,12 +477,11 @@ DEFPY(
return nb_cli_apply_changes(vty, NULL);
}
-DEFPY(
- match_source_instance, match_source_instance_cmd,
- "match source-instance (0-255)$instance",
- MATCH_STR
- "Match the protocol's instance number\n"
- "The instance number\n")
+DEFPY(match_source_instance, match_source_instance_cmd,
+ "match source-instance (0-255)$instance",
+ MATCH_STR
+ "Match the protocol's instance number\n"
+ "The instance number\n")
{
const char *xpath = "./match-condition[condition='source-instance']";
char xpath_value[XPATH_MAXLEN];
@@ -515,12 +494,11 @@ DEFPY(
return nb_cli_apply_changes(vty, NULL);
}
-DEFPY(
- no_match_source_instance, no_match_source_instance_cmd,
- "no match source-instance [(0-255)]",
- NO_STR MATCH_STR
- "Match the protocol's instance number\n"
- "The instance number\n")
+DEFPY(no_match_source_instance, no_match_source_instance_cmd,
+ "no match source-instance [(0-255)]",
+ NO_STR MATCH_STR
+ "Match the protocol's instance number\n"
+ "The instance number\n")
{
const char *xpath = "./match-condition[condition='source-instance']";
@@ -531,13 +509,11 @@ DEFPY(
/* set functions */
-DEFPY(
- set_src, set_src_cmd,
- "set src <A.B.C.D$addrv4|X:X::X:X$addrv6>",
- SET_STR
- "src address for route\n"
- "IPv4 src address\n"
- "IPv6 src address\n")
+DEFPY(set_src, set_src_cmd, "set src <A.B.C.D$addrv4|X:X::X:X$addrv6>",
+ SET_STR
+ "src address for route\n"
+ "IPv4 src address\n"
+ "IPv6 src address\n")
{
const char *xpath = "./set-action[action='source']";
char xpath_value[XPATH_MAXLEN];
@@ -558,14 +534,11 @@ DEFPY(
return nb_cli_apply_changes(vty, NULL);
}
-DEFPY(
- no_set_src, no_set_src_cmd,
- "no set src [<A.B.C.D|X:X::X:X>]",
- NO_STR
- SET_STR
- "Source address for route\n"
- "IPv4 address\n"
- "IPv6 address\n")
+DEFPY(no_set_src, no_set_src_cmd, "no set src [<A.B.C.D|X:X::X:X>]",
+ NO_STR SET_STR
+ "Source address for route\n"
+ "IPv4 address\n"
+ "IPv6 address\n")
{
const char *xpath = "./set-action[action='source']";
If you are a new contributor to FRR, please see our contributing guidelines.
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10636/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
clang_check |
Rebased and tested lightly again. |
type inet:ipv6-address; | ||
} | ||
} | ||
} |
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.
Are below protocol interaction commands not required to be taken care?
ip protocol bgp route-map
ROUTE-MAP Route map name
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 I missed this one, I was more interested in getting the route-map library part working as an example for other daemons.
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.
so you would be update the same PR or you want to have separate PR for this?
This new type will be used by zebra route map match command which is IP version agnostic. Signed-off-by: Rafael Zalamena <[email protected]>
* Extend the route map yang model to have zebra enumerations; * Add zebra route map specific match/set values; Signed-off-by: Rafael Zalamena <[email protected]>
Add skeleton code for zebra northbound, but implement route map commands. Signed-off-by: Rafael Zalamena <[email protected]>
Lets use the newly implemented zebra northbound to configure route maps. Signed-off-by: Rafael Zalamena <[email protected]>
Fix copy & paste on YANG description and add new route types that appeared. Signed-off-by: Rafael Zalamena <[email protected]>
9eb6b53
to
f8978cc
Compare
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDTest incomplete. See below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: IncompleteAddresssanitizer topotest: Incomplete(check logs for details)Successful on other platforms/tests
|
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11385/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
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.
Looks good to me, only issue I found is that the cli_show()
callbacks are missing.
Also, it would be better if we could split the zebra northbound callbacks into multiple files like we are doing in the other daemons (please see #5186). This is a non-blocking comment though.
Adapt the zebra route map code to use the transaction CLI output (e.g. the CLI show callbacks) instead of the fallback compatibility. Signed-off-by: Rafael Zalamena <[email protected]>
5fa8d33
to
f42cc96
Compare
Yes, sorry I missed this one because the fallback was making it work anyways. Now the code is in place and |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
case NB_EV_VALIDATE: | ||
memset(&p, 0, sizeof(p)); | ||
yang_dnode_get_ipv4p(&p, dnode, NULL); | ||
if (zebra_check_addr(&p) == 0) { |
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.
Do we need this check? In yang we are using ipv4-address for this leaf so check should already happen.
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.
That check does more than that and I want to keep the previous behavior.
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 these checks can be added in Yang itself with range as well.
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.
int zebra_check_addr(const struct prefix *p)
{
if (p->family == AF_INET) {
uint32_t addr;
addr = p->u.prefix4.s_addr;
addr = ntohl(addr);
if (IPV4_NET127(addr) || IN_CLASSD(addr)
|| IPV4_LINKLOCAL(addr))
return 0;
}
if (p->family == AF_INET6) {
if (IN6_IS_ADDR_LOOPBACK(&p->u.prefix6))
return 0;
if (IN6_IS_ADDR_LINKLOCAL(&p->u.prefix6))
return 0;
}
return 1;
}
This is quite a lot to check using YANG, I personally think code-level validation is fine in this case. If you think otherwise, maybe propose something different on a separate PR later? I'd like to unlock this one since it's opened for several months already.
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.
Sure. Having this check does not harm but we can revisit this with separate PR.
case NB_EV_VALIDATE: | ||
memset(&p, 0, sizeof(p)); | ||
yang_dnode_get_ipv6p(&p, dnode, NULL); | ||
if (zebra_check_addr(&p) == 0) { |
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.
Same comment as above. Check not required.
|
||
/* Set destroy information. */ | ||
switch (condition) { | ||
case 100: /* ipv4-prefix-length */ |
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.
Can we use macros here?
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.
Sure, is there any automatic generation? Otherwise I'd keep that for another PR (lib/routemap itself would also need this to keep consistency).
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.
Nothing I know of. We can write one later if not already present.
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11403/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
Use a union to join the route types declaration instead of copying/pasting them. Signed-off-by: Rafael Zalamena <[email protected]>
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.
Looks good to me!
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11435/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
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.
LGTM. We can revisit few changes for front-end validation later in different PR.
Summary
Migrate zebra's route-map specific commands to yang/northbound.
zebra/yang
specific commits start withyang: add all route types enumeration
.This PR requires #5104 merged first (or just merge this one with everything).