From babdc6f4969ee312166f5cb64e23e7ebf86aa3d1 Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Mon, 2 Aug 2021 15:46:07 +0200 Subject: [PATCH] * Fixed: [OpenConfig BGP afi-safi and when condition issues #249](https://github.com/clicon/clixon/issues/249) * YANG when was not properly implemented for default values * Improved error message on leafref validation errors --- CHANGELOG.md | 2 + lib/clixon/clixon_xml_map.h | 1 + lib/src/clixon_validate.c | 63 ++++++++------------------- lib/src/clixon_xml_map.c | 82 ++++++++++++++++++++++++++++++++---- lib/src/clixon_yang.c | 4 +- test/test_leafref.sh | 4 +- test/test_leafref_augment.sh | 2 +- test/test_leafref_state.sh | 4 +- test/test_when_must.sh | 2 +- test/test_xpath_functions.sh | 8 ++-- 10 files changed, 107 insertions(+), 65 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c3d768ad..d27b9226 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,8 @@ Users may have to change how they access the system ### Corrected Bugs +* Fixed: [OpenConfig BGP afi-safi and when condition issues #249](https://github.com/clicon/clixon/issues/249) + * YANG when was not properly implemented for default values * Fixed: SEGV in clixon_netconf_lib functions from internal errors including validation. * Check xerr argument both before and after call on netconf lib functions * Fixed: RFC 8040 yang-data extension allows non-key lists diff --git a/lib/clixon/clixon_xml_map.h b/lib/clixon/clixon_xml_map.h index 8f4a288f..feba656e 100644 --- a/lib/clixon/clixon_xml_map.h +++ b/lib/clixon/clixon_xml_map.h @@ -76,5 +76,6 @@ int assign_namespace_body(cxobj *x0, cxobj *x1); int xml_merge(cxobj *x0, cxobj *x1, yang_stmt *yspec, char **reason); int yang_enum_int_value(cxobj *node, int32_t *val); int xml_copy_marked(cxobj *x0, cxobj *x1); +int yang_when_xpath(cxobj *xn, cxobj *xp, yang_stmt *yn, int *hit, int *nrp, char **xpathp); #endif /* _CLIXON_XML_MAP_H_ */ diff --git a/lib/src/clixon_validate.c b/lib/src/clixon_validate.c index 802330c8..48070400 100644 --- a/lib/src/clixon_validate.c +++ b/lib/src/clixon_validate.c @@ -114,6 +114,7 @@ validate_leafref(cxobj *xt, cvec *nsc = NULL; cbuf *cberr = NULL; char *path; + yang_stmt *ymod; if ((leafrefbody = xml_body(xt)) == NULL) goto ok; @@ -140,7 +141,8 @@ validate_leafref(cxobj *xt, clicon_err(OE_UNIX, errno, "cbuf_new"); goto done; } - cprintf(cberr, "Leafref validation failed: No leaf %s matching path %s", leafrefbody, path); + ymod = ys_module(ys); + cprintf(cberr, "Leafref validation failed: No leaf %s matching path %s in module %s", leafrefbody, path, yang_argument_get(ymod)); if (xret && netconf_bad_element_xml(xret, "application", leafrefbody, cbuf_get(cberr)) < 0) goto done; goto fail; @@ -1136,6 +1138,7 @@ xml_yang_validate_all(clicon_handle h, char *ns = NULL; cbuf *cb = NULL; cvec *nsc = NULL; + int hit = 0; /* if not given by argument (overide) use default link and !Node has a config sub-statement and it is false */ @@ -1227,53 +1230,21 @@ xml_yang_validate_all(clicon_handle h, nsc = NULL; } } - /* First variant of when, actual "when" sub-node RFC 7950 Sec 7.21.5. Can only be one. */ - if ((yc = yang_find(ys, Y_WHEN, NULL)) != NULL){ - xpath = yang_argument_get(yc); /* "when" has xpath argument */ - /* WHEN xpath needs namespace context */ - if (xml_nsctx_yang(ys, &nsc) < 0) + if (yang_when_xpath(xt, xml_parent(xt), ys, &hit, &nr, &xpath) < 0) + goto done; + if (hit && nr == 0){ + if ((cb = cbuf_new()) == NULL){ + clicon_err(OE_UNIX, errno, "cbuf_new"); goto done; - if ((nr = xpath_vec_bool(xt, nsc, "%s", xpath)) < 0) + } + cprintf(cb, "Failed WHEN condition of %s in module %s (WHEN xpath is %s)", + xml_name(xt), + yang_argument_get(ys_module(ys)), + xpath); + if (xret && netconf_operation_failed_xml(xret, "application", + cbuf_get(cb)) < 0) goto done; - if (nsc){ - xml_nsctx_free(nsc); - nsc = NULL; - } - if (nr == 0){ - if ((cb = cbuf_new()) == NULL){ - clicon_err(OE_UNIX, errno, "cbuf_new"); - goto done; - } - cprintf(cb, "Failed WHEN condition of %s in module %s", - xml_name(xt), - yang_argument_get(ys_module(ys))); - if (xret && netconf_operation_failed_xml(xret, "application", - cbuf_get(cb)) < 0) - goto done; - goto fail; - } - } - /* Second variants of WHEN: - * Augmented and uses when using special info in node - */ - if ((xpath = yang_when_xpath_get(ys)) != NULL){ - if ((nr = xpath_vec_bool(xml_parent(xt), yang_when_nsc_get(ys), - "%s", xpath)) < 0) - goto done; - if (nr == 0){ - if ((cb = cbuf_new()) == NULL){ - clicon_err(OE_UNIX, errno, "cbuf_new"); - goto done; - } - cprintf(cb, "Failed augmented 'when' condition '%s' of node '%s' in module '%s'", - xpath, - xml_name(xt), - yang_argument_get(ys_module(ys))); - if (xret && netconf_operation_failed_xml(xret, "application", - cbuf_get(cb)) < 0) - goto done; - goto fail; - } + goto fail; } } x = NULL; diff --git a/lib/src/clixon_xml_map.c b/lib/src/clixon_xml_map.c index 0b933992..5c77c22d 100644 --- a/lib/src/clixon_xml_map.c +++ b/lib/src/clixon_xml_map.c @@ -1139,10 +1139,11 @@ xml_default1(yang_stmt *yt, int retval = -1; yang_stmt *yc; cxobj *xc; - int top=0; /* Top symbol (set default namespace) */ + int top = 0; /* Top symbol (set default namespace) */ int create = 0; char *xpath; - int nr; + int nr = 0; + int hit = 0; if (xt == NULL){ /* No xml */ clicon_err(OE_XML, EINVAL, "No XML argument"); @@ -1181,12 +1182,10 @@ xml_default1(yang_stmt *yt, case Y_CONTAINER: if (yang_find(yc, Y_PRESENCE, NULL) == NULL){ /* Check when statement from uses or augment */ - if ((xpath = yang_when_xpath_get(yc)) != NULL){ - if ((nr = xpath_vec_bool(xt, yang_when_nsc_get(yc), "%s", xpath)) < 0) - goto done; - if (nr == 0) - break; /* Do not create default if xpath fails */ - } + if (yang_when_xpath(NULL, xt, yc, &hit, &nr, &xpath) < 0) + goto done; + if (hit && nr == 0) + break; /* Do not create default if xpath fails */ /* If this is non-presence, (and it does not exist in xt) call * recursively and create nodes if any default value exist first. * Then continue and populate? @@ -2257,3 +2256,70 @@ xml_copy_marked(cxobj *x0, return retval; } +/*! Check when condition + * + * @param[in] h Clixon handle + * @param[in] xn XML node, can be NULL, in which case it is added as dummy under xp + * @param[in] xp XML parent + * @param[in] ys Yang node + * First variants of WHEN: Augmented and uses when using special info in node + * Second variant of when, actual "when" sub-node RFC 7950 Sec 7.21.5. Can only be one. + */ +int +yang_when_xpath(cxobj *xn, + cxobj *xp, + yang_stmt *yn, + int *hit, + int *nrp, + char **xpathp) +{ + int retval = 1; + yang_stmt *yc; + char *xpath = NULL; + cxobj *x = NULL; + int nr = 0; + cvec *nsc = NULL; + int xmalloc = 0; /* ugly help variable to clean temporary object */ + int nscmalloc = 0; /* ugly help variable to remove */ + + /* First variant */ + if ((xpath = yang_when_xpath_get(yn)) != NULL){ + x = xp; + nsc = yang_when_nsc_get(yn); + *hit = 1; + } + /* Second variant */ + else if ((yc = yang_find(yn, Y_WHEN, NULL)) != NULL){ + xpath = yang_argument_get(yc); /* "when" has xpath argument */ + /* Create dummy */ + if (xn == NULL){ + if ((x = xml_new(yang_argument_get(yn), xp, CX_ELMNT)) == NULL) + goto done; + xml_spec_set(x, yn); + xmalloc++; + } + else + x = xn; + if (xml_nsctx_yang(yn, &nsc) < 0) + goto done; + nscmalloc++; + *hit = 1; + } + else + *hit = 0; + if (x && xpath){ + if ((nr = xpath_vec_bool(x, nsc, "%s", xpath)) < 0) + goto done; + } + if (nrp) + *nrp = nr; + if (xpathp) + *xpathp = xpath; + retval = 0; + done: + if (xmalloc) + xml_purge(x); + if (nsc && nscmalloc) + xml_nsctx_free(nsc); + return retval; +} diff --git a/lib/src/clixon_yang.c b/lib/src/clixon_yang.c index 7c1af5a1..c90fe4ae 100644 --- a/lib/src/clixon_yang.c +++ b/lib/src/clixon_yang.c @@ -340,11 +340,13 @@ yang_flag_reset(yang_stmt *ys, * @param[in] ys Yang statement * @retval xpath xpath should evaluate to true at validation * @retval NULL Not set + * Note xpath context is PARENT which is different from when actual when child which is + * child itself */ char* yang_when_xpath_get(yang_stmt *ys) { - return ys->ys_when_xpath; + return ys->ys_when_xpath; } /*! Set yang xpath and namespace context for "when"-associated augment diff --git a/test/test_leafref.sh b/test/test_leafref.sh index a7a414c3..b425f61e 100755 --- a/test/test_leafref.sh +++ b/test/test_leafref.sh @@ -140,7 +140,7 @@ new "leafref add non-existing ref" expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLOeth3
10.0.4.6
]]>]]>" "^]]>]]>$" new "leafref validate" -expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^applicationbad-elementeth3errorLeafref validation failed: No leaf eth3 matching path /if:interfaces/if:interface/if:name]]>]]>$" +expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^applicationbad-elementeth3errorLeafref validation failed: No leaf eth3 matching path /if:interfaces/if:interface/if:name in module example]]>]]>$" #new "leafref wrong ref" #expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLOeth3
10.0.4.6
]]>]]>" "^]]>]]>$" @@ -170,7 +170,7 @@ new "leafref delete leaf" expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLOeth0]]>]]>" "^" new "leafref validate (should fail)" -expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^applicationbad-elementeth0errorLeafref validation failed: No leaf eth0 matching path /if:interfaces/if:interface/if:name]]>]]>$" +expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^applicationbad-elementeth0errorLeafref validation failed: No leaf eth0 matching path /if:interfaces/if:interface/if:name in module example]]>]]>$" new "leafref discard-changes" expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^]]>]]>$" diff --git a/test/test_leafref_augment.sh b/test/test_leafref_augment.sh index 4c4c046a..c6472be9 100755 --- a/test/test_leafref_augment.sh +++ b/test/test_leafref_augment.sh @@ -209,7 +209,7 @@ new "leafref augment+leafref config wrong ref" expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO$XML]]>]]>" "^]]>]]>$" new "leafref augment+leafref validate wrong ref" -expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^applicationbad-elementxxxerrorLeafref validation failed: No leaf xxx matching path /ex:sender/ex:name]]>]]>$" +expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^applicationbad-elementxxxerrorLeafref validation failed: No leaf xxx matching path /ex:sender/ex:name in module augment]]>]]>$" new "netconf discard-changes" expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^]]>]]>$" diff --git a/test/test_leafref_state.sh b/test/test_leafref_state.sh index 9ed123ad..feb4032e 100755 --- a/test/test_leafref_state.sh +++ b/test/test_leafref_state.sh @@ -168,10 +168,10 @@ expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^applicationoperation-failedxerrorLeafref validation failed: No leaf x matching path /ex:sender-config/ex:name. Internal error, state callback returned invalid XML]]>]]>$" +expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^applicationoperation-failedxerrorLeafref validation failed: No leaf x matching path /ex:sender-config/ex:name in module leafref. Internal error, state callback returned invalid XML]]>]]>$" new "netconf get / state-only should fail" -expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^applicationoperation-failedxerrorLeafref validation failed: No leaf x matching path /ex:sender-config/ex:name. Internal error, state callback returned invalid XML]]>]]>$" +expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^applicationoperation-failedxerrorLeafref validation failed: No leaf x matching path /ex:sender-config/ex:name in module leafref. Internal error, state callback returned invalid XML]]>]]>$" new "netconf get / config-only ok" expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^y]]>]]>$" diff --git a/test/test_when_must.sh b/test/test_when_must.sh index 12c94da3..83bc9a6a 100755 --- a/test/test_when_must.sh +++ b/test/test_when_must.sh @@ -117,7 +117,7 @@ new "when get config" expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^directr2staticr1]]>]]>$" new "when: validate fail" -expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^applicationoperation-failederrorFailed WHEN condition of static-routes in module example]]>]]>$" +expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^applicationoperation-failederrorFailed WHEN condition of static-routes in module example (WHEN xpath is ../type='static')]]>]]>$" new "when: discard-changes" expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^]]>]]>$" diff --git a/test/test_xpath_functions.sh b/test/test_xpath_functions.sh index f996ad1a..3b2cfbee 100755 --- a/test/test_xpath_functions.sh +++ b/test/test_xpath_functions.sh @@ -124,7 +124,7 @@ new "Set site to fie which invalidates the when contains" expecteof "$clixon_netconf -qf $cfg -D $DBG" 0 "$DEFAULTHELLOfie]]>]]>" "^]]>]]>" new "netconf validate not OK" -expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^applicationoperation-failederrorFailed WHEN condition of site in module example]]>]]>$" +expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^applicationoperation-failederrorFailed WHEN condition of site in module example (WHEN xpath is contains(../../class,'foo') or contains(../../class,'bar'))]]>]]>$" new "netconf discard-changes" expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^]]>]]>$" @@ -140,13 +140,13 @@ new "Change type to atm" expecteof "$clixon_netconf -qf $cfg -D $DBG" 0 "$DEFAULTHELLOe0atm]]>]]>" "^]]>]]>" new "netconf validate not OK (mtu not allowed)" -expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^applicationoperation-failederrorFailed augmented 'when' condition 'derived-from(type, \"ex:ethernet\")' of node 'mtu' in module 'example']]>]]>$" +expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^applicationoperation-failederrorFailed WHEN condition of mtu in module example (WHEN xpath is derived-from(type, \"ex:ethernet\"))]]>]]>$" new "Change type to ethernet (self)" expecteof "$clixon_netconf -qf $cfg -D $DBG" 0 "$DEFAULTHELLOe0ethernet]]>]]>" "^]]>]]>" new "netconf validate not OK (mtu not allowed on self)" -expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^applicationoperation-failederrorFailed augmented 'when' condition 'derived-from(type, \"ex:ethernet\")' of node 'mtu' in module 'example']]>]]>$" +expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^applicationoperation-failederrorFailed WHEN condition of mtu in module example (WHEN xpath is derived-from(type, \"ex:ethernet\"))]]>]]>$" new "netconf discard-changes" expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^]]>]]>$" @@ -162,7 +162,7 @@ new "Change type to atm" expecteof "$clixon_netconf -qf $cfg -D $DBG" 0 "$DEFAULTHELLOe0atm]]>]]>" "^]]>]]>" new "netconf validate not OK (crc not allowed)" -expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^applicationoperation-failederrorFailed augmented 'when' condition 'derived-from-or-self(type, \"ex:ethernet\")' of node 'crc' in module 'example']]>]]>$" +expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^applicationoperation-failederrorFailed WHEN condition of crc in module example (WHEN xpath is derived-from-or-self(type, \"ex:ethernet\"))]]>]]>$" new "Change type to ethernet (self)" expecteof "$clixon_netconf -qf $cfg -D $DBG" 0 "$DEFAULTHELLOe0ethernet]]>]]>" "^]]>]]>"