diff --git a/CHANGELOG.md b/CHANGELOG.md index b451d560..4bd0a867 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,13 +57,13 @@ Users may have to change how they access the system ### C/CLI-API changes on existing features Developers may need to change their code +* Rename function `xml_yang_minmax_recurse()` -> `xml_yang_validate_minmax()` * Modified msg functions for clearer NETCONF 1.0 vs 1.1 API: * `clicon_rpc1` --> `clixon_rpc10` * `clicon_msg_send1` --> `clixon_msg_send10` * `clicon_msg_rcv` and `clicon_msg_decode` --> `clixon_msg_rcv11` * Rewrite by calling `clixon_msg_rcv11` and explicit xml parsing * `clicon_msg_rcv1` --> `clixon_msg_rcv10` - * Added `yspec` parameter to `api_path_fmt2api_path()`: * `api_path_fmt2api_path(af, c, a, c)` --> `api_path_fmt2api_path(af, c, yspec, a, c)` * Added flags parameter to default functions: @@ -109,6 +109,7 @@ Developers may need to change their code ### Corrected Bugs +* Fixed: [If services add duplicate entries, controller does not detect this](https://github.com/clicon/clixon-controller/issues/107) * Fixed: [Problems with diff of YANG lists ordered-by user](https://github.com/clicon/clixon/issues/496) * Fixed: [show compare does not show correct diff while load merge xml](https://github.com/clicon/clixon-controller/issues/101) * Fixed: [commit goes 2 times](https://github.com/clicon/clixon/issues/488) diff --git a/apps/backend/backend_client.c b/apps/backend/backend_client.c index 1caff528..579c666f 100644 --- a/apps/backend/backend_client.c +++ b/apps/backend/backend_client.c @@ -555,10 +555,12 @@ from_client_edit_config(clixon_handle h, } /* Limited validation of incoming payload */ - if ((ret = xml_yang_minmax_recurse(xc, 1, &xret)) < 0) + if ((ret = xml_yang_validate_minmax(xc, 1, &xret)) < 0) + goto done; + if (ret == 1 && (ret = xml_yang_validate_unique_recurse(xc, &xret)) < 0) goto done; /* xmldb_put (difflist handling) requires list keys */ - if (ret==1 && (ret = xml_yang_validate_list_key_only(xc, &xret)) < 0) + if (ret == 1 && (ret = xml_yang_validate_list_key_only(xc, &xret)) < 0) goto done; if (ret == 0){ if (clixon_xml2cbuf(cbret, xret, 0, 0, NULL, -1, 0) < 0) diff --git a/lib/clixon/clixon_validate_minmax.h b/lib/clixon/clixon_validate_minmax.h index 1cc1b403..fe754911 100644 --- a/lib/clixon/clixon_validate_minmax.h +++ b/lib/clixon/clixon_validate_minmax.h @@ -43,6 +43,9 @@ /* * Prototypes */ -int xml_yang_minmax_recurse(cxobj *xt, int recurse, cxobj **xret); +int xml_yang_validate_minmax(cxobj *xt, int presence, cxobj **xret); +int xml_yang_validate_minmax_recurse(cxobj *xt, cxobj **xret); +int xml_yang_validate_unique(cxobj *xt, cxobj **xret); +int xml_yang_validate_unique_recurse(cxobj *xt, cxobj **xret); #endif /* _CLIXON_VALIDATE_MINMAX_H_ */ diff --git a/lib/src/clixon_netconf_lib.c b/lib/src/clixon_netconf_lib.c index 8d385232..0ad5d8af 100644 --- a/lib/src/clixon_netconf_lib.c +++ b/lib/src/clixon_netconf_lib.c @@ -1474,7 +1474,7 @@ netconf_data_not_unique_xml(cxobj **xret, /* error-info: Contains an instance identifier that points to a leaf * that invalidates the "unique" constraint. This element is present once for each * non-unique leaf. */ - if (cvec_len(cvk)){ + if (cvk && cvec_len(cvk)){ if ((xinfo = xml_new("error-info", xerr, CX_ELMNT)) == NULL) goto done; if (xml2xpath(x, NULL, 0, 0, &path) < 0) diff --git a/lib/src/clixon_plugin.c b/lib/src/clixon_plugin.c index 7d11957f..4213baaa 100644 --- a/lib/src/clixon_plugin.c +++ b/lib/src/clixon_plugin.c @@ -1288,6 +1288,9 @@ rpc_callback_call(clixon_handle h, nr++; if (clixon_resource_check(h, &wh, rc->rc_name, __FUNCTION__) < 0) goto done; + /* Ensure only one reply: first wins */ + if (cbuf_len(cbret) > 0) + break; } rc = NEXTQ(rpc_callback_t *, rc); } while (rc != ms->ms_rpc_callbacks); diff --git a/lib/src/clixon_validate.c b/lib/src/clixon_validate.c index 3f276ae6..239471ce 100644 --- a/lib/src/clixon_validate.c +++ b/lib/src/clixon_validate.c @@ -1383,7 +1383,7 @@ xml_yang_validate_all(clixon_handle h, /* Check unique and min-max after choice test for example*/ if (yang_config(yt) != 0){ /* Checks if next level contains any unique list constraints */ - if ((ret = xml_yang_minmax_recurse(xt, 1, xret)) < 0) + if ((ret = xml_yang_validate_minmax(xt, 1, xret)) < 0) goto done; if (ret == 0) goto fail; @@ -1422,7 +1422,7 @@ xml_yang_validate_all_top(clixon_handle h, if ((ret = xml_yang_validate_all(h, x, xret)) < 1) return ret; } - if ((ret = xml_yang_minmax_recurse(xt, 0, xret)) < 1) + if ((ret = xml_yang_validate_minmax(xt, 0, xret)) < 1) return ret; return 1; } diff --git a/lib/src/clixon_validate_minmax.c b/lib/src/clixon_validate_minmax.c index 367de9b5..f67af7fd 100644 --- a/lib/src/clixon_validate_minmax.c +++ b/lib/src/clixon_validate_minmax.c @@ -85,12 +85,10 @@ * @param[in] i1 The new entry is placed at vec[i1] * @param[in] vlen Length of entry * @param[in] sorted Sorted by system, ie sorted by key, otherwise no assumption - * @retval 0 OK, entry is unique - * @retval -1 Duplicate detected - * @note This is currently quadratic complexity. It could be improved by inserting new element sorted and binary search. * @retval 1 Validation OK * @retval 0 Validation failed (cbret set) * @retval -1 Error + * @note This is currently quadratic complexity. It could be improved by inserting new element sorted and binary search. */ static int unique_search_xpath(cxobj *x, @@ -218,16 +216,16 @@ check_unique_list_direct(cxobj *x, cxobj **xret) { int retval = -1; - cg_var *cvi; /* unique node name */ - cxobj *xi; - char **vec = NULL; /* 2xmatrix */ - int clen; - int i; - int v; - char *bi; - int sorted; - char *str; - cvec *cvk; + cg_var *cvi; /* unique node name */ + cxobj *xi; + char **vec = NULL; /* 2xmatrix */ + int clen; + int i; + int v; + char *bi; + int sorted; + char *str; + cvec *cvk; /* If list and is sorted by system, then it is assumed elements are in key-order which is optimized * Other cases are "unique" constraint or list sorted by user which is quadratic in complexity @@ -385,7 +383,7 @@ check_unique_list(cxobj *x, goto done; } -/*! Given a list, check if any min/max-elemants constraints apply +/*! Given a list or leaf-list, check if any min/max-elemants constraints apply * * @param[in] xp Parent of the xml list there are too few/many (for error) * @param[in] y Yang spec of the failing list @@ -395,7 +393,7 @@ check_unique_list(cxobj *x, * @retval 0 Validation failed (cbret set) * @retval -1 Error * @see RFC7950 7.7.5 - * @note No recurse for non-presence container is made, see eg xml_yang_minmax_recurse + * @note No recurse for non-presence container is made, see eg xml_yang_minmax */ static int check_minmax(cxobj *xp, @@ -480,11 +478,21 @@ check_empty_list_minmax(cxobj *xt, goto done; } +/*! Check duplicates/unique in list + * + * @param[in] x XML LIST node + * @param[in] xt XML parent + * @param[in] y YANG of x + * @param[out] xret Error as XML if ret = 0 + * @retval 1 Validation OK + * @retval 0 Validation failed (xret set) + * @retval -1 Error + */ static int -xml_yang_minmax_newlist(cxobj *x, - cxobj *xt, - yang_stmt *y, - cxobj **xret) +xml_yang_minmax_new_list(cxobj *x, + cxobj *xt, + yang_stmt *y, + cxobj **xret) { int retval = -1; yang_stmt *yu; @@ -523,6 +531,55 @@ xml_yang_minmax_newlist(cxobj *x, goto done; } +/*! Check duplicates in leaf-list + * + * @param[in] x0 XML LIST node + * @param[in] xt XML parent + * @param[in] y0 YANG of x0 + * @param[out] xret Error as XML if ret = 0 + * @retval 1 Validation OK + * @retval 0 Validation failed (xret set) + * @retval -1 Error + * @note works for both ordered-by user and system. But worst case quadratic + */ +static int +xml_yang_minmax_new_leaf_list(cxobj *x0, + cxobj *xt, + yang_stmt *y0, + cxobj **xret) +{ + int retval = -1; + cxobj *xi; + cxobj *xj; + char *bi; + char *bj; + + xi = x0; + do { + if ((bi = xml_body(xi)) == NULL) + continue; + xj = xi; + while ((xj = xml_child_each(xt, xj, CX_ELMNT)) != NULL && + xml_spec(xj) == y0) { + if ((bj = xml_body(xj)) == NULL) + continue; + if (bi && bj && strcmp(bi, bj) == 0){ + if (xret && netconf_data_not_unique_xml(xret, xi, NULL) < 0) + goto done; + goto fail; + } + } + } + while ((xi = xml_child_each(xt, xi, CX_ELMNT)) != NULL && + xml_spec(xi) == y0); + retval = 1; + done: + return retval; + fail: + retval = 0; + goto done; +} + /*! Perform gap analysis in a child-vector interval [ye,y] * * Gap analysis here meaning if there is a list x with min-element constraint but there are no @@ -581,7 +638,7 @@ xml_yang_minmax_gap_analysis(cxobj *xt, goto done; } -/*! Recursive minmax check +/*! YANG Minmax check, no recursion * * Assume xt:s children are sorted and yang populated. * The function does two different things of the children of an XML node: @@ -637,7 +694,7 @@ xml_yang_minmax_gap_analysis(cxobj *xt, * 7 null nolist neq gap analysis; nopresence-check; * 8 list nolist neq gap analysis; yprev: check-minmax; nopresence-check; * @param[in] xt XML parent (may have lists w unique constraints as child) - * @param[in] recurse Set if called in a recursive loop (will recurse anyway), + * @param[in] presence Set if called in a recursive loop (the caller will recurse anyway), * otherwise non-presence containers will be traversed * @param[out] xret Error XML tree. Free with xml_free after use * @retval 1 Validation OK @@ -646,11 +703,12 @@ xml_yang_minmax_gap_analysis(cxobj *xt, * Note that many checks, ie all except gap analysis, may be unnecessary if this fn * is called in a recursive environment, since the recursion being made here will * be made in that environment anyway and thus leading to double checks. + * @see xml_yang_validate_unique Sunset of unique tests */ int -xml_yang_minmax_recurse(cxobj *xt, - int recurse, - cxobj **xret) +xml_yang_validate_minmax(cxobj *xt, + int presence, + cxobj **xret) { int retval = -1; cxobj *x = NULL; @@ -686,10 +744,18 @@ xml_yang_minmax_recurse(cxobj *xt, } nr=1; /* new list check */ - if (ret && - keyw == Y_LIST) - if ((ret = xml_yang_minmax_newlist(x, xt, y, xret)) < 0) - goto done; + if (ret){ + if (keyw == Y_LIST){ + if ((ret = xml_yang_minmax_new_list(x, xt, y, xret)) < 0) + goto done; + } +#ifdef NOTYET /* XXX This is enforced in xml_yang_validate_unique instead */ + else if (keyw == Y_LEAF_LIST){ + if ((ret = xml_yang_minmax_new_leaf_list(x, xt, y, xret)) < 0) + goto done; + } +#endif + } if (ret == 0) goto fail; yprev = y; @@ -716,11 +782,11 @@ xml_yang_minmax_recurse(cxobj *xt, } if (ret == 0) goto fail; - if (recurse && keyw == Y_CONTAINER && + if (presence && keyw == Y_CONTAINER && yang_find(y, Y_PRESENCE, NULL) == NULL){ yang_stmt *yc = NULL; while ((yc = yn_each(y, yc)) != NULL) { - if ((ret = xml_yang_minmax_recurse(x, recurse, xret)) < 0) + if ((ret = xml_yang_validate_minmax(x, presence, xret)) < 0) goto done; if (ret == 0) goto fail; @@ -765,3 +831,168 @@ xml_yang_minmax_recurse(cxobj *xt, retval = 0; goto done; } + +/*! Recursive minmax check + * + * Callback function type for xml_apply + * @param[in] x XML node + * @param[in] arg General-purpose argument + * @retval 2 Locally abort this subtree, continue with others + * @retval 1 Abort, dont continue with others, return 1 to end user + * @retval 0 OK, continue + * @retval -1 Error, aborted at first error encounter, return -1 to end user + */ +static int +xml_yang_minmax_apply(cxobj *x, + void *arg) +{ + int ret; + cxobj **xret = (cxobj **)arg; + + if ((ret = xml_yang_validate_minmax(x, 1, xret)) < 0) + return -1; + if (ret == 0){ /* Validation failed (xret set) */ + return 1; /* Abort dont continue */ + } + return 0; +} + +/*! Recursive YANG Minmax check + * + * @param[in] xt XML parent (may have lists w unique constraints as child) + * @param[out] xret Error XML tree. Free with xml_free after use + * @retval 1 Validation OK + * @retval 0 Validation failed (xret set) + * @retval -1 Error + */ +int +xml_yang_validate_minmax_recurse(cxobj *xt, + cxobj **xret) +{ + int retval = -1; + int ret; + + if ((ret = xml_apply0(xt, CX_ELMNT, xml_yang_minmax_apply, xret)) < 0) + goto done; + if (ret == 1) + goto fail; + retval = 1; + done: + return retval; + fail: + retval = 0; + goto done; +} + +/*! YANG unique check, no recursion + * + * Assume xt:s children are sorted and yang populated. + * @param[in] xt XML parent (may have lists w unique constraints as child) + * @param[out] xret Error XML tree. Free with xml_free after use + * @retval 1 Validation OK + * @retval 0 Validation failed (xret set) + * @retval -1 Error + * @see xml_yang_validate_minmax which include these unique tests + */ +int +xml_yang_validate_unique(cxobj *xt, + cxobj **xret) +{ + int retval = -1; + cxobj *x = NULL; + yang_stmt *y; + yang_stmt *yprev = NULL; + enum rfc_6020 keyw; + int nr = 0; + int ret; + + while ((x = xml_child_each(xt, x, CX_ELMNT)) != NULL){ + if ((y = xml_spec(x)) == NULL) + continue; + keyw = yang_keyword_get(y); + if (keyw == Y_LIST || keyw == Y_LEAF_LIST){ + /* equal: just continue*/ + if (y == yprev){ + nr++; + continue; + } + nr=1; + /* new list check */ + switch (keyw){ + case Y_LIST: + if ((ret = xml_yang_minmax_new_list(x, xt, y, xret)) < 0) + goto done; + if (ret == 0) + goto fail; + break; + case Y_LEAF_LIST: + if ((ret = xml_yang_minmax_new_leaf_list(x, xt, y, xret)) < 0) + goto done; + if (ret == 0) + goto fail; + break; + default: + break; + } + yprev = y; + } + } + retval = 1; + done: + return retval; + fail: + retval = 0; + goto done; +} + +/*! Recursive unique check + * + * Callback function type for xml_apply + * @param[in] x XML node + * @param[in] arg General-purpose argument + * @retval 2 Locally abort this subtree, continue with others + * @retval 1 Abort, dont continue with others, return 1 to end user + * @retval 0 OK, continue + * @retval -1 Error, aborted at first error encounter, return -1 to end user + */ +static int +xml_yang_unique_apply(cxobj *x, + void *arg) +{ + int ret; + cxobj **xret = (cxobj **)arg; + + if ((ret = xml_yang_validate_unique(x, xret)) < 0) + return -1; + if (ret == 0){ /* Validation failed (xret set) */ + return 1; /* Abort dont continue */ + } + return 0; +} + +/*! Recursive YANG unique check + * + * @param[in] xt XML parent (may have lists w unique constraints as child) + * @param[out] xret Error XML tree. Free with xml_free after use + * @retval 1 Validation OK + * @retval 0 Validation failed (xret set) + * @retval -1 Error + */ +int +xml_yang_validate_unique_recurse(cxobj *xt, + cxobj **xret) +{ + int retval = -1; + int ret; + + if ((ret = xml_apply0(xt, CX_ELMNT, xml_yang_unique_apply, xret)) < 0) + goto done; + if (ret == 1) + goto fail; + retval = 1; + done: + return retval; + fail: + retval = 0; + goto done; +} diff --git a/test/test_netconf_duplicate.sh b/test/test_netconf_duplicate.sh new file mode 100755 index 00000000..c786e56d --- /dev/null +++ b/test/test_netconf_duplicate.sh @@ -0,0 +1,104 @@ +#!/usr/bin/env bash +# Detect duplicates in incoming edit-configs (not top-level) +# Both list and leaf-list +# See https://github.com/clicon/clixon-controller/issues/107 + +# Magic line must be first in script (see README.md) +s="$_" ; . ./lib.sh || if [ "$s" = $0 ]; then exit 0; else return 0; fi + +APPNAME=example + +cfg=$dir/conf_yang.xml +fyang=$dir/unique.yang + +cat < $cfg + + $cfg + ${YANG_INSTALLDIR} + $fyang + /usr/local/lib/$APPNAME/clispec + /usr/local/lib/$APPNAME/cli + $APPNAME + /usr/local/var/run/$APPNAME.sock + /usr/local/var/run/$APPNAME.pidfile + /usr/local/var/$APPNAME + +EOF + +# Example (the list server part) from RFC7950 Sec 7.8.3.1 w changed types +cat < $fyang +module unique{ + yang-version 1.1; + namespace "urn:example:clixon"; + prefix un; + container c{ + presence "trigger"; // force presence container to trigger error + list server { + description ""; + key "name"; + leaf name { + type string; + } + leaf value { + type string; + } + } + leaf-list b{ + type string; + } + } +} +EOF + +new "test params: -f $cfg" + +if [ $BE -ne 0 ]; then + new "kill old backend" + sudo clixon_backend -zf $cfg + if [ $? -ne 0 ]; then + err + fi + new "start backend -s init -f $cfg" + # start new backend + start_backend -s init -f $cfg +fi + +new "wait backend" +wait_backend + +if false; then +new "Add list with duplicate" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "replace + + one + foo + + + one + foo + +" "" "applicationoperation-faileddata-not-uniqueerror/rpc/edit-config/config/c/server[name=\"one\"]/name" +fi + +new "Add leaf-list with duplicate" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "replace + one + two + one +" "" "applicationoperation-faileddata-not-uniqueerror" + +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 + +new "endtest" +endtest diff --git a/test/test_unique.sh b/test/test_unique.sh index 7355846a..ab45662f 100755 --- a/test/test_unique.sh +++ b/test/test_unique.sh @@ -94,7 +94,7 @@ fi new "wait backend" wait_backend -# RFC test two-field caes +# RFC test two-field case new "Add not valid example" expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "replace smtp