diff --git a/CHANGELOG.md b/CHANGELOG.md index 269ff08b..7083ab4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,7 +38,7 @@ Expected: June 2021 * Several cases were not implemented fully according to RFC 7950: * Do not extend default values if when statements evaluate to false * Do not allow edit-config of nodes if when statements evaluate to false (Sec 8.3.2) - * If a key leaf is defined in a grouping that is used in a list, the "uses" statement MUST NOT have a "when" statement. (See 7.21.5) + * If a key leaf is defined in a grouping that is used in a list, the "uses" statement MUST NOT have a "when" statement. (See 7.21.5) * See [yang uses's substatement when has no effect #218](https://github.com/clicon/clixon/issues/218) * YANG deviation [deviation statement not yet support #211](https://github.com/clicon/clixon/issues/211) * See RFC7950 Sec 5.6.3 @@ -81,6 +81,7 @@ Users may have to change how they access the system ### Corrected Bugs +* Fixed: [when condition error under augment in restconf #227](https://github.com/clicon/clixon/issues/227) * Fixed: [Using YANG union with decimal64 and string leads to regexp match fail #226](https://github.com/clicon/clixon/issues/226) * Fixed: [xpath function count did not work properly #224](https://github.com/clicon/clixon/issues/224) * Fixed: RESTCONF Native: Failed binding of socket in network namespace caused process zombie diff --git a/include/clixon_custom.h b/include/clixon_custom.h index a0ff95da..30e114d0 100644 --- a/include/clixon_custom.h +++ b/include/clixon_custom.h @@ -83,13 +83,32 @@ */ #define STATE_ORDERED_BY_SYSTEM -/* Top-symbol in clixon datastores +/*! Top-symbol in clixon datastores * This is traditionally same as NETCONF_INPUT_CONFIG ("config") but can be different * If you change this, you need to change test shell variable in lib.sh: DATASTORE_TOP - * Consider making this an option or configure option + * Consider making this an option (but this has bootstrap problems) or configure option */ #define DATASTORE_TOP_SYMBOL "config" -/* Name of default netns for clixon-restconf.yang socket/namespace field +/*! Name of default netns for clixon-restconf.yang socket/namespace field + * Restconf allows opening sockets in different network namespaces. This is teh name of + * "host"/"default" namespace. Unsure what to really label this but seems like there is differing + * consensus on how to label it. + * Either find that proper label, or move it to a option */ #define RESTCONF_NETNS_DEFAULT "default" + +/*! Set a temporary parent for use in special case "when" xpath calls + * Problem is when changing an existing (candidate) in-memory datastore that yang "when" conditionals + * should be changed in clixon_datastore_write.c:text_modify(). + * Problem is that the tree is in an intermediate state so that a when condition may not see the + * full context. + * More specifically, new nodes (x0) are created without hooking them into the existing parent (x0p) + * and thus an xpath on the form ".."/PARENT may not be evaluated as they should. x0 is eventually + * added to its parent but then it is more difficult to check trhe when condition. + * This fix add the parent x0p as a "candidate" so that the xpath-eval function can use it as + * an alernative if it exists. + * Note although this solves many usecases involving parents and absolute paths, itstill does not + * solve all usecases, such as absolute usecases where the added node is looked for + */ +#define XML_PARENT_CANDIDATE diff --git a/lib/clixon/clixon_xml.h b/lib/clixon/clixon_xml.h index 39c4a8d1..14576bdd 100644 --- a/lib/clixon/clixon_xml.h +++ b/lib/clixon/clixon_xml.h @@ -186,6 +186,10 @@ int nscache_replace(cxobj *x, cvec *ns); int xmlns_set(cxobj *x, char *prefix, char *ns); cxobj *xml_parent(cxobj *xn); int xml_parent_set(cxobj *xn, cxobj *parent); +#ifdef XML_PARENT_CANDIDATE +cxobj *xml_parent_candidate(cxobj *xn); +int xml_parent_candidate_set(cxobj *xn, cxobj *parent); +#endif /* XML_PARENT_CANDIDATE */ uint16_t xml_flag(cxobj *xn, uint16_t flag); int xml_flag_set(cxobj *xn, uint16_t flag); diff --git a/lib/src/clixon_datastore_write.c b/lib/src/clixon_datastore_write.c index dd632471..4c874782 100644 --- a/lib/src/clixon_datastore_write.c +++ b/lib/src/clixon_datastore_write.c @@ -205,7 +205,7 @@ check_body_namespace(cxobj *x0, /*! Check yang when condition between a new xml x1 and old x0 * - * check if there is a when condition. First try it on the new request (x1), then on the + * Check if there is a when condition. First try it on the new request (x1), then on the * existing (x0). * @param[in] x0p Parent of x0 * @param[in] x1 XML tree which modifies base @@ -226,9 +226,9 @@ check_when_condition(cxobj *x0p, char *xpath = NULL; cvec *nsc = NULL; int nr; - cxobj *x1p; yang_stmt *y = NULL; cbuf *cberr = NULL; + cxobj *x1p; if ((y = y0) != NULL || (y = (yang_stmt*)xml_spec(x1)) != NULL){ @@ -272,7 +272,7 @@ check_when_condition(cxobj *x0p, * @param[in] h Clicon handle * @param[in] x0 Base xml tree (can be NULL in add scenarios) * @param[in] x0p Parent of x0 - * @param[in] x0t + * @param[in] x0t Top level of existing tree, eg needed for NACM rules * @param[in] x1 XML tree which modifies base * @param[in] x1t Request root node (nacm needs this) * @param[in] y0 Yang spec corresponding to xml-node x0. NULL if x0 is NULL @@ -662,7 +662,9 @@ text_modify(clicon_handle h, if ((x0 = xml_new(x1name, NULL, CX_ELMNT)) == NULL) goto done; xml_spec_set(x0, y0); - +#ifdef XML_PARENT_CANDIDATE + xml_parent_candidate_set(x0, x0p); +#endif changed++; /* Get namespace from x1 * Check if namespace exists in x0 parent @@ -730,8 +732,8 @@ text_modify(clicon_handle h, x1c = NULL; i = 0; while ((x1c = xml_child_each(x1, x1c, CX_ELMNT)) != NULL) { - x1cname = xml_name(x1c); x0c = x0vec[i++]; + x1cname = xml_name(x1c); yc = yang_find_datanode(y0, x1cname); if ((ret = text_modify(h, x0c, x0, x0t, x1c, x1t, yc, op, @@ -742,6 +744,9 @@ text_modify(clicon_handle h, goto fail; } if (changed){ +#ifdef XML_PARENT_CANDIDATE + xml_parent_candidate_set(x0, NULL); +#endif if (xml_insert(x0p, x0, insert, keystr, nscx1) < 0) goto done; } @@ -786,7 +791,7 @@ text_modify(clicon_handle h, /*! Modify a top-level base tree x0 with modification tree x1 * @param[in] h Clicon handle * @param[in] x0 Base xml tree (can be NULL in add scenarios) - * @param[in] x0t + * @param[in] x0t Top level of existing tree, eg needed for NACM rules * @param[in] x1 XML tree which modifies base * @param[in] x1t Request root node (nacm needs this) * @param[in] yspec Top-level yang spec (if y is NULL) diff --git a/lib/src/clixon_xml.c b/lib/src/clixon_xml.c index c07333d0..f019c1a5 100644 --- a/lib/src/clixon_xml.c +++ b/lib/src/clixon_xml.c @@ -166,6 +166,9 @@ struct xml{ char *x_prefix; /* namespace localname N, called prefix */ uint16_t x_flags; /* Flags according to XML_FLAG_* */ struct xml *x_up; /* parent node in hierarchy if any */ +#ifdef XML_PARENT_CANDIDATE + struct xml *x_up_candidate; /* Candidate parent node for special cases (when+xpath) */ +#endif int _x_vector_i; /* internal use: xml_child_each */ int _x_i; /* internal use for sorting: see xml_enumerate and xml_cmp */ @@ -196,6 +199,9 @@ struct xmlbody{ char *xb_prefix; /* namespace localname N, called prefix */ uint16_t xb_flags; /* Flags according to XML_FLAG_* */ struct xml *xb_up; /* parent node in hierarchy if any */ +#ifdef XML_PARENT_CANDIDATE + struct xml *xb_up_candidate; /* Candidate parent node for special cases (when+xpath) */ +#endif int _xb_vector_i; /* internal use: xml_child_each */ int _xb_i; /* internal use for sorting: see xml_enumerate and xml_cmp */ @@ -251,6 +257,7 @@ xml_stats_one(cxobj *x, { size_t sz = 0; + if (x->x_name) sz += strlen(x->x_name) + 1; if (x->x_prefix) @@ -587,6 +594,34 @@ xml_parent_set(cxobj *xn, return 0; } +#ifdef XML_PARENT_CANDIDATE +/*! Get candidate parent of xnode + * @param[in] xn xml node + * @retval parent xml node + */ +cxobj* +xml_parent_candidate(cxobj *xn) +{ + if (xn == NULL) { + return NULL; + } + return xn->x_up_candidate; +} + +/*! Set candidate parent of xml node + * @param[in] xn xml node + * @param[in] parent pointer to candidate parent xml node + * @retval 0 OK + */ +int +xml_parent_candidate_set(cxobj *xn, + cxobj *parent) +{ + xn->x_up_candidate = parent; + return 0; +} +#endif /* XML_PARENT_CANDIDATE */ + /*! Get xml node flags, used for internal algorithms * @param[in] xn xml node * @retval flag Flag value(s), see XML_FLAG_MARK et al @@ -1420,8 +1455,7 @@ xml_child_rm(cxobj *xp, * @param[in] xc xml child node to be removed * @retval 0 OK * @retval -1 Error - * @note you should not remove xchild in loop (unless yoy keep track of xprev) - * + * @note you should not remove xchild in loop (unless you keep track of xprev) * @see xml_child_rm Remove a child of a node */ int diff --git a/lib/src/clixon_xpath_eval.c b/lib/src/clixon_xpath_eval.c index 62a5c032..6c6e9449 100644 --- a/lib/src/clixon_xpath_eval.c +++ b/lib/src/clixon_xpath_eval.c @@ -392,7 +392,12 @@ xp_eval_step(xp_ctx *xc0, xc->xc_nodeset = NULL; for (i=0; ixc_nodeset, &xc->xc_size) < 0) goto done; } @@ -979,8 +984,13 @@ xp_eval(xp_ctx *xc, case XP_ABSPATH: /* Set context node to top node, and nodeset to that node only */ x = xc->xc_node; +#ifdef XML_PARENT_CANDIDATE + while (xml_parent(x) != NULL || xml_parent_candidate(x) != NULL) + x = xml_parent(x)?xml_parent(x):xml_parent_candidate(x); +#else while (xml_parent(x) != NULL) x = xml_parent(x); +#endif xc->xc_node = x; xc->xc_nodeset[0] = x; xc->xc_size=1; diff --git a/test/test_yang_when.sh b/test/test_yang_when.sh index 884d9b30..4eb35388 100755 --- a/test/test_yang_when.sh +++ b/test/test_yang_when.sh @@ -4,7 +4,6 @@ # 1. set/get/validate matching and non-matching when statement # 2 RFC 7950 7.21.5: If a key leaf is defined in a grouping that is used in a list, the # "uses" statement MUST NOT have a "when" statement. -# XXX:Originally for cli tests but not specific to cli (clean when done?) # Magic line must be first in script (see README.md) s="$_" ; . ./lib.sh || if [ "$s" = $0 ]; then exit 0; else return 0; fi @@ -69,7 +68,7 @@ commit("Commit the changes"), cli_commit(); quit("Quit"), cli_quit(); EOF - +# Start backend, add entry with matching name / non-matrching and check get/validation function testrun() { new "test params: -f $cfg" @@ -81,8 +80,6 @@ function testrun() fi new "start backend -s init -f $cfg" start_backend -s init -f $cfg - - fi new "wait backend" wait_backend @@ -129,14 +126,16 @@ function testrun() new "netconf validate default set" expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^]]>]]>$" - new "Kill backend" - # Check if premature kill - pid=$(pgrep -u root -f clixon_backend) - if [ -z "$pid" ]; then - err "backend already dead" + if [ $BE -ne 0 ]; then + new "Kill backend" + # Check if premature kill + pid=$(pgrep -u root -f clixon_backend) + if [ -z "$pid" ]; then + err "backend already dead" + fi + # kill backend + stop_backend -f $cfg fi - # kill backend - stop_backend -f $cfg } cat < $fyang @@ -198,9 +197,6 @@ EOF new "2. augment/uses/when test edit,default (same but with augment)" testrun -new "3. RFC 7950 7.21.5: If a key leaf is defined in a grouping that is used in a list" -# the "uses" statement MUST NOT have a "when" statement. - cat < $fyang module example { namespace "urn:example:clixon"; @@ -221,14 +217,91 @@ module example { } } EOF +new "3. RFC 7950 7.21.5: If a key leaf is defined in a grouping that is used in a list" +# the "uses" statement MUST NOT have a "when" statement. if [ ${valgrindtest} -eq 0 ]; then # Error dont cleanup mem OK new "start backend -s init -f $cfg, expect yang when fail" expectpart "$(sudo $clixon_backend -F -D $DBG -s init -f $cfg 2>&1)" 255 "Yang error: Key leaf 'name' defined in grouping 'value-top' is used in a 'uses' statement, This is not allowed according to RFC 7950 Sec 7.21.5" fi -# kill backend -stop_backend -f $cfg +cat < $fyang +module example { + namespace "urn:example:clixon"; + prefix ex; + + leaf allow { + type boolean; + default false; + } + grouping value-top { + leaf value{ + type string; + default foo; + } + } + container table{ + list parameter{ + key name; + leaf name{ + type string; + } + uses value-top { + when "../../ex:allow = 'true'"; + } + } + } +} +EOF + + +new "3. Check a top-level value as allowed value" + +new "test params: -f $cfg" +if [ $BE -ne 0 ]; then + new "kill old backend" + sudo clixon_backend -z -f $cfg + if [ $? -ne 0 ]; then + err + fi + new "start backend -s init -f $cfg" + start_backend -s init -f $cfg +fi + +new "wait backend" +wait_backend + +new "netconf set allow true" +expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLOtruemerge]]>]]>" "^]]>]]>$" + +new "netconf commit" +expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^]]>]]>$" + +new "netconf set value expect OK" +expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLOkalle42
merge]]>]]>" "^]]>]]>$" + +new "netconf discard-changes" +expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^]]>]]>$" + +new "netconf set allow false" +expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLOfalsemerge]]>]]>" "^]]>]]>$" + +new "netconf commit" +expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^]]>]]>$" + +new "netconf set value expect fail" +expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLOkalle42
merge]]>]]>" "^applicationunknown-elementvalueerrorNode 'value' tagged with 'when' condition '../../ex:allow = 'true'' in module 'example' evaluates to false in edit-config operation (see RFC 7950 Sec 8.3.2)]]>]]>$" + +if [ $BE -ne 0 ]; then + new "Kill backend" + # Check if premature kill + pid=$(pgrep -u root -f clixon_backend) + if [ -z "$pid" ]; then + err "backend already dead" + fi + # kill backend + stop_backend -f $cfg +fi rm -rf $dir diff --git a/yang/clixon/clixon-config@2021-05-20.yang b/yang/clixon/clixon-config@2021-05-20.yang index 750272fe..947880ac 100644 --- a/yang/clixon/clixon-config@2021-05-20.yang +++ b/yang/clixon/clixon-config@2021-05-20.yang @@ -484,7 +484,7 @@ module clixon-config { type string; default "/usr/local/sbin"; description - "Path to clixon-restconf daemon binary as used by backend if started internally + "Path to dir of clixon-restconf daemon binary as used by backend if started internally Discussion: Somewhat problematic to have it as run time option. It may think it should be known at configure or install time, but for example the main docker installation moves the binaries, and this may be true elsewehere too. @@ -520,7 +520,8 @@ module clixon-config { leaf CLICON_RESTCONF_USER { type string; description - "User name for restconf. + "Run clixon_daemon as this user + When drop privileges is used, the daemon will drop privileges to this user. In pre-5.2 code this was configured as compile-time constant WWWUSER with default value www-data See also CLICON_PRIVILEGES setting";