diff --git a/CHANGELOG.md b/CHANGELOG.md index c8ba6d08..3b8abc5d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,15 @@ Expected: September 2022 Users may have to change how they access the system +* NETCONF error handling for `data-not-unique` and `missing-choice` follows standard more closely + * data-not-unique: + * Added YANG namespace to `` tag + * Changed `yang-unique` value to path insytead of single name + * See RFC 7950 Sec 15.1 + * missing-choice: + * Added `` + * Added YANG namespace to `` tag + * See RFC 7950 Sec 15.6 * Backend transaction plugins: edit of choice node will always result in a "del/add" event for all edits of change nodes, never a "change" event. * Before, some cases were using a "change" event if the "yang ordering" happended to be the same. * See more details in: [Clixon backend transactions for choice/case is not logical](https://github.com/clicon/clixon/issues/361) @@ -110,6 +119,8 @@ e` ### Corrected Bugs +* Fixed: [When multiple lists have same key name, need more elaborate error message in case of configuration having duplicate keys](https://github.com/clicon/clixon/issues/362) + * Solved by implementing RFC7950 Sec 5.1 correctly * Fixed: [All values in list don't appear when writing "show " in cli](https://github.com/clicon/clixon/issues/359) * Fixed: [yang regular char \w not include underline char](https://github.com/clicon/clixon/issues/357) * Fixed: [Clixon backend transactions for choice/case is not logical](https://github.com/clicon/clixon/issues/361) diff --git a/lib/clixon/clixon_netconf_lib.h b/lib/clixon/clixon_netconf_lib.h index fed507cf..6b0b0935 100644 --- a/lib/clixon/clixon_netconf_lib.h +++ b/lib/clixon/clixon_netconf_lib.h @@ -143,8 +143,9 @@ int netconf_lock_denied(cbuf *cb, char *info, char *message); int netconf_resource_denied(cbuf *cb, char *type, char *message); int netconf_rollback_failed(cbuf *cb, char *type, char *message); int netconf_data_exists(cbuf *cb, char *message); -int netconf_data_missing(cbuf *cb, char *missing_choice, char *message); -int netconf_data_missing_xml(cxobj **xret, char *missing_choice, char *message); +int netconf_data_missing(cbuf *cb, char *message); +int netconf_data_missing_xml(cxobj **xret, char *message); +int netconf_missing_choice_xml(cxobj **xret, cxobj *x, char *missing_choice, char *message); int netconf_operation_not_supported_xml(cxobj **xret, char *type, char *message); int netconf_operation_not_supported(cbuf *cb, char *type, char *message); int netconf_operation_failed(cbuf *cb, char *type, char *message); diff --git a/lib/src/clixon_datastore_write.c b/lib/src/clixon_datastore_write.c index 15c5bcb8..760bab19 100644 --- a/lib/src/clixon_datastore_write.c +++ b/lib/src/clixon_datastore_write.c @@ -540,7 +540,7 @@ text_modify(clicon_handle h, /* RFC 8040 4.6 PATCH: * If the target resource instance does not exist, the server MUST NOT create it. */ - if (netconf_data_missing(cbret, NULL, + if (netconf_data_missing(cbret, "RFC 8040 4.6. PATCH: If the target resource instance does not exist, the server MUST NOT create it") < 0) goto done; goto fail; @@ -714,7 +714,7 @@ text_modify(clicon_handle h, break; case OP_DELETE: if (x0==NULL){ - if (netconf_data_missing(cbret, NULL, "Data does not exist; cannot delete resource") < 0) + if (netconf_data_missing(cbret, "Data does not exist; cannot delete resource") < 0) goto done; goto fail; } @@ -735,7 +735,7 @@ text_modify(clicon_handle h, } else { if (op == OP_DELETE){ - if (netconf_data_missing(cbret, NULL, "Data does not exist; cannot delete resource") < 0) + if (netconf_data_missing(cbret, "Data does not exist; cannot delete resource") < 0) goto done; goto fail; } @@ -941,7 +941,7 @@ text_modify(clicon_handle h, break; case OP_DELETE: if (x0==NULL){ - if (netconf_data_missing(cbret, NULL, "Data does not exist; cannot delete resource") < 0) + if (netconf_data_missing(cbret, "Data does not exist; cannot delete resource") < 0) goto done; goto fail; } @@ -1060,7 +1060,7 @@ text_modify_top(clicon_handle h, I.e., curl -u andy:bar -sS -X DELETE http://localhost/restconf/data */ case OP_DELETE: - if (netconf_data_missing(cbret, NULL, "Data does not exist; cannot delete resource") < 0) + if (netconf_data_missing(cbret, "Data does not exist; cannot delete resource") < 0) goto done; goto fail; break; diff --git a/lib/src/clixon_netconf_lib.c b/lib/src/clixon_netconf_lib.c index 26f2f990..bf26e5cc 100644 --- a/lib/src/clixon_netconf_lib.c +++ b/lib/src/clixon_netconf_lib.c @@ -923,18 +923,16 @@ netconf_data_exists(cbuf *cb, * does not exist. For example, a "delete" operation was attempted on * data that does not exist. * @param[out] cb CLIgen buf. Error XML is written in this buffer - * @param[in] missing_choice If set, see RFC7950: 15.6 violates mandatory choice * @param[in] message Error message */ int netconf_data_missing(cbuf *cb, - char *missing_choice, char *message) { int retval = -1; cxobj *xret = NULL; - if (netconf_data_missing_xml(&xret, missing_choice, message) < 0) + if (netconf_data_missing_xml(&xret, message) < 0) goto done; if (clixon_xml2cbuf(cb, xret, 0, 0, -1, 0) < 0) goto done; @@ -945,18 +943,16 @@ netconf_data_missing(cbuf *cb, return retval; } -/*! Create Netconf data-missing error XML tree according to RFC 6241 App A +/*! Create Netconf data-missing error XML tree according to RFC 6241 App A / RFC 7950 15.6(choice) * * Request could not be completed because the relevant data model content * does not exist. For example, a "delete" operation was attempted on * data that does not exist. * @param[out] xret Error XML tree. Free with xml_free after use - * @param[in] missing_choice If set, see RFC7950: 15.6 violates mandatiry choice * @param[in] message Error message */ int netconf_data_missing_xml(cxobj **xret, - char *missing_choice, char *message) { int retval = -1; @@ -984,12 +980,6 @@ netconf_data_missing_xml(cxobj **xret, "application" "data-missing") < 0) goto done; - if (missing_choice) /* NYI: RFC7950: 15.6 */ - if (clixon_xml_parse_va(YB_NONE, NULL, &xerr, NULL, - "missing-choice" - "%s", - missing_choice) < 0) - goto done; if (clixon_xml_parse_va(YB_NONE, NULL, &xerr, NULL, "error") < 0) goto done; @@ -1006,7 +996,80 @@ netconf_data_missing_xml(cxobj **xret, free(encstr); return retval; } - + +/*! Create Netconf data-missing / missing-choice as defeind in RFC 7950 15.6 + * + * If a NETCONF operation would result in configuration data where no + * nodes exists in a mandatory choice, the following error MUST be + * returned: + * @param[out] xret Error XML tree. Free with xml_free after use + * @param[in] x Element with missing choice + * @param[in] name Name of missing mandatory choice + * @param[in] message Error message + */ +int +netconf_missing_choice_xml(cxobj **xret, + cxobj *x, + char *name, + char *message) +{ + int retval = -1; + char *encstr = NULL; + cxobj *xerr; + cxobj *xa; + char *path = NULL; + char *encpath = NULL; + + if (xret == NULL || name == NULL){ + clicon_err(OE_NETCONF, EINVAL, "xret or name is NULL"); + goto done; + } + if (*xret == NULL){ + if ((*xret = xml_new("rpc-reply", NULL, CX_ELMNT)) == NULL) + goto done; + } + else if (xml_name_set(*xret, "rpc-reply") < 0) + goto done; + if ((xa = xml_new("xmlns", *xret, CX_ATTR)) == NULL) + goto done; + if (xml_value_set(xa, NETCONF_BASE_NAMESPACE) < 0) + goto done; + if ((xerr = xml_new("rpc-error", *xret, CX_ELMNT)) == NULL) + goto done; + /* error-path: Path to the element with the missing choice. */ + if (xml2xpath(x, NULL, &path) < 0) + goto done; + if (xml_chardata_encode(&encpath, "%s", path) < 0) + goto done; + if (clixon_xml_parse_va(YB_NONE, NULL, &xerr, NULL, + "application" + "data-missing" + "missing-choice" + "%s" + "%s" + "error", + encpath, + YANG_XML_NAMESPACE, + name) < 0) + goto done; + if (message){ + if (xml_chardata_encode(&encstr, "%s", message) < 0) + goto done; + if (clixon_xml_parse_va(YB_NONE, NULL, &xerr, NULL, + "%s", encstr) < 0) + goto done; + } + retval = 0; + done: + if (path) + free(path); + if (encstr) + free(encstr); + if (encpath) + free(encpath); + return retval; +} + /*! Create Netconf operation-not-supported error XML according to RFC 6241 App A * * Request could not be completed because the requested operation is not @@ -1272,7 +1335,7 @@ netconf_malformed_message_xml(cxobj **xret, /*! Create Netconf data-not-unique error message according to RFC 7950 15.1 * * A NETCONF operation would result in configuration data where a - * "unique" constraint is invalidated. + * "unique" constraint is invalidated. * @param[out] xret Error XML tree. Free with xml_free after use * @param[in] x List element containing duplicate * @param[in] cvk List of components in x that are non-unique @@ -1287,9 +1350,10 @@ netconf_data_not_unique_xml(cxobj **xret, cg_var *cvi = NULL; cxobj *xerr; cxobj *xinfo; - cbuf *cb = NULL; cxobj *xa; - + char *path = NULL; + char *encpath = NULL; + if (xret == NULL){ clicon_err(OE_NETCONF, EINVAL, "xret is NULL"); goto done; @@ -1312,23 +1376,31 @@ netconf_data_not_unique_xml(cxobj **xret, "data-not-unique" "error") < 0) goto done; + /* 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 ((xinfo = xml_new("error-info", xerr, CX_ELMNT)) == NULL) goto done; - if ((cb = cbuf_new()) == NULL){ - clicon_err(OE_UNIX, errno, "cbuf_new"); + if (xml2xpath(x, NULL, &path) < 0) + goto done; + if (xml_chardata_encode(&encpath, "%s", path) < 0) goto done; - } while ((cvi = cvec_each(cvk, cvi)) != NULL){ if (clixon_xml_parse_va(YB_NONE, NULL, &xinfo, NULL, - "%s", cv_string_get(cvi)) < 0) + "%s/%s", + YANG_XML_NAMESPACE, + encpath, + cv_string_get(cvi)) < 0) goto done; } } retval = 0; done: - if (cb) - cbuf_free(cb); + if (path) + free(path); + if (encpath) + free(encpath); return retval; } @@ -1351,7 +1423,7 @@ netconf_minmax_elements_xml(cxobj **xret, int retval = -1; cxobj *xerr; char *path = NULL; - cbuf *cb = NULL; + char *encpath = NULL; cxobj *xa; if (xret == NULL){ @@ -1370,31 +1442,27 @@ netconf_minmax_elements_xml(cxobj **xret, goto done; if ((xerr = xml_new("rpc-error", *xret, CX_ELMNT)) == NULL) goto done; - if ((cb = cbuf_new()) == NULL){ - clicon_err(OE_UNIX, errno, "cbuf_new"); - goto done; - } if (xml_parent(xp)){ /* Dont include root, eg */ - if (xml2xpath(xp, NULL, &path) < 0) + if (xml2xpath(xp, NULL, &path) < 0) + goto done; + if (xml_chardata_encode(&encpath, "%s", path) < 0) goto done; - if (path) - cprintf(cb, "%s", path); } - cprintf(cb, "/%s", name); if (clixon_xml_parse_va(YB_NONE, NULL, &xerr, NULL, "protocol" "operation-failed" "too-%s-elements" "error" - "%s", + "%s/%s", max?"many":"few", - cbuf_get(cb)) < 0) + encpath?encpath:"", + name) < 0) goto done; retval = 0; done: if (path) free(path); - if (cb) - cbuf_free(cb); + if (encpath) + free(encpath); return retval; } diff --git a/lib/src/clixon_validate.c b/lib/src/clixon_validate.c index 12ca5879..7c45390a 100644 --- a/lib/src/clixon_validate.c +++ b/lib/src/clixon_validate.c @@ -818,7 +818,7 @@ check_mandatory(cxobj *xt, if (x == NULL){ /* @see RFC7950: 15.6 Error Message for Data That Violates * a Mandatory "choice" Statement */ - if (xret && netconf_data_missing_xml(xret, yang_argument_get(yc), NULL) < 0) + if (xret && netconf_missing_choice_xml(xret, xt, yang_argument_get(yc), NULL) < 0) goto done; goto fail; } diff --git a/lib/src/clixon_validate_minmax.c b/lib/src/clixon_validate_minmax.c index 2fe718bf..47c9a4f9 100644 --- a/lib/src/clixon_validate_minmax.c +++ b/lib/src/clixon_validate_minmax.c @@ -138,17 +138,15 @@ unique_search_xpath(cxobj *x, goto done; } -/*! New element last in list, check if already exists if sp return -1 +/*! New element last in list, return error if already exists + * * @param[in] vec Vector of existing entries (new is last) * @param[in] i1 The new entry is placed at vec[i1] - * @param[in] vlen Lenght of entry + * @param[in] vlen Length of vec * @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 */ static int check_insert_duplicate(char **vec, @@ -684,9 +682,9 @@ xml_yang_minmax_recurse(cxobj *xt, nr=1; /* new list check */ if (ret && - keyw == Y_LIST && - (ret = xml_yang_minmax_newlist(x, xt, y, xret)) < 0) - goto done; + keyw == Y_LIST) + if ((ret = xml_yang_minmax_newlist(x, xt, y, xret)) < 0) + goto done; if (ret == 0) goto fail; yprev = y; diff --git a/test/test_choice.sh b/test/test_choice.sh index 040da440..d96665e4 100755 --- a/test/test_choice.sh +++ b/test/test_choice.sh @@ -340,7 +340,7 @@ new "netconf get mandatory empty" expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "" new "netconf validate mandatory" -expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "applicationdata-missingmissing-choicenameerror" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "applicationdata-missingmissing-choice/system/mandatorynameerror" new "netconf set mandatory udp" expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "" diff --git a/test/test_restconf_op.sh b/test/test_restconf_op.sh index 2256cb26..bf9eeac7 100755 --- a/test/test_restconf_op.sh +++ b/test/test_restconf_op.sh @@ -151,7 +151,7 @@ new "restconf DELETE" expectpart "$(curl $CURLOPTS -X DELETE $RCPROTO://localhost/restconf/data/example:cont1)" 0 "HTTP/$HVER 204" new "restconf POST from top containing duplicate keys expect error" -expectpart "$(curl $CURLOPTS -X POST -H "Content-Type: application/yang-data+json" $RCPROTO://localhost/restconf/data -d '{"example:cont1":{"interface":[{"name":"TEST","type":"eth0"},{"name":"TEST","type":"eth0"}]}}')" 0 "HTTP/$HVER 412" '{"ietf-restconf:errors":{"error":{"error-type":"application","error-tag":"operation-failed","error-app-tag":"data-not-unique","error-severity":"error","error-info":{"non-unique":"name"}}}}' +expectpart "$(curl $CURLOPTS -X POST -H "Content-Type: application/yang-data+json" $RCPROTO://localhost/restconf/data -d '{"example:cont1":{"interface":[{"name":"TEST","type":"eth0"},{"name":"TEST","type":"eth0"}]}}')" 0 "HTTP/$HVER 412" '{"ietf-restconf:errors":{"error":{"error-type":"application","error-tag":"operation-failed","error-app-tag":"data-not-unique","error-severity":"error","error-info":{"non-unique":"/rpc/edit-config/config/cont1/interface\[name=\\"TEST\\"\]/name"}}}}' new "restconf GET null datastore" expectpart "$(curl $CURLOPTS -X GET $RCPROTO://localhost/restconf/data/example:cont1)" 0 "HTTP/$HVER 404" '{"ietf-restconf:errors":{"error":{"error-type":"application","error-tag":"invalid-value","error-severity":"error","error-message":"Instance does not exist"}}}' diff --git a/test/test_rpc.sh b/test/test_rpc.sh index ecf52211..ab7dd0a4 100755 --- a/test/test_rpc.sh +++ b/test/test_rpc.sh @@ -301,13 +301,13 @@ expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "" "" "applicationdata-missingmissing-choiceconfig-targeterror" +expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "" "" "applicationdata-missingmissing-choice/rpc/edit-config/targetconfig-targeterror" new "netconf edit-config missing target: should fail" expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "" "" "applicationmissing-elementtargeterrorMandatory variable of edit-config in module ietf-netconf" new "netconf edit-config missing config: should fail" -expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "" "" "applicationdata-missingmissing-choiceedit-contenterror" +expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "" "" "applicationdata-missingmissing-choice/rpc/edit-configedit-contenterror" # Negative errors (namespace/module missing) new "netconf wrong rpc namespace: should fail" @@ -320,7 +320,7 @@ expectpart "$(curl $CURLOPTS -X POST -H "Content-Type: application/yang-data+jso LIST='foobarbar' # On docker /fcgi the return is bar,foo,bar -new "netconf example rpc input list without key with non-unique entriesxxx" +new "netconf example rpc input list without key with non-unique entries" rpc=$(chunked_framing "mandatory$LIST") ret=$($clixon_netconf -qef $cfg <mandatory$LIST" "" "applicationoperation-faileddata-not-uniqueerroruk" +expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "mandatory$LIST" "" "applicationoperation-faileddata-not-uniqueerror/rpc/example/u1[uk=\"bar\"]/uk" new "netconf choice ok" expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "42xxx" "" "4242xxx" diff --git a/test/test_unique.sh b/test/test_unique.sh index 0714e77d..176f061f 100755 --- a/test/test_unique.sh +++ b/test/test_unique.sh @@ -106,7 +106,7 @@ expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" " 192.0.2.1 25 -" "" "applicationoperation-faileddata-not-uniqueerroripport" +" "" "applicationoperation-faileddata-not-uniqueerror/rpc/edit-config/config/c/server[name=\"http\"]/ip/rpc/edit-config/config/c/server[name=\"http\"]/port" new "Add valid example" expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "replace @@ -132,7 +132,7 @@ expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" " " "" "" new "netconf validate (should fail)" -expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "applicationoperation-faileddata-not-uniqueerroripport" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "applicationoperation-faileddata-not-uniqueerror/c/server[name=\"smtp\"]/ip/c/server[name=\"smtp\"]/port" new "make it valid by deleting port from smtp entry" expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "nonesmtp25 @@ -160,8 +160,8 @@ expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" " " "" "" -new "netconf validate (should fail)" -expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "applicationoperation-faileddata-not-uniqueerrorip" +new "netconf validate (should fail) 2nd" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "applicationoperation-faileddata-not-uniqueerror/c/single[name=\"smtp\"]/ip" new "make valid by replacing IP of http entry" expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "nonehttp178.23.34.1 @@ -201,8 +201,8 @@ expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" " " "" "" -new "netconf validate (should fail)" -expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "applicationoperation-faileddata-not-uniqueerroripport" +new "netconf validate (should fail) example3" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "applicationoperation-faileddata-not-uniqueerror/c/server[name=\"smtp\"]/ip/c/server[name=\"smtp\"]/port" new "netconf discard-changes" expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "" diff --git a/test/test_unique_descendant.sh b/test/test_unique_descendant.sh index e7f5fdd4..b82ef446 100755 --- a/test/test_unique_descendant.sh +++ b/test/test_unique_descendant.sh @@ -98,12 +98,12 @@ expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "xafoobfooyafiebfum" new "Add invalid example 1" -expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "${rpc}" "" "applicationoperation-faileddata-not-uniqueerrorc/inner/value" +expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "${rpc}" "" "applicationoperation-faileddata-not-uniqueerror/rpc/edit-config/config/outer[name=\"x\"]/c/inner/value" rpc="replacexafoobbaryafiebbar" new "Add invalid example 2" -expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "${rpc}" "" "applicationoperation-faileddata-not-uniqueerrorc/inner/value" +expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "${rpc}" "" "applicationoperation-faileddata-not-uniqueerror/rpc/edit-config/config/outer[name=\"y\"]/c/inner/value" if [ $BE -ne 0 ]; then new "Kill backend"