diff --git a/CHANGELOG.md b/CHANGELOG.md index e60386e1..7581d1ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,8 @@ Expected: May 2020 ### Minor changes +* Added decriptive error message when plugins produce invalid state XML. + * Example: `operation-failedmystateNo such yang module. Internal error, state callback returned invalid XML: example_backend` * Added option `CLICON_YANG_UNKNOWN_ANYDATA` to treat unknown XML (wrt YANG) as anydata. * This is a way to loosen sanity checks if you need to accept eg unsynchronized YANG and XML * Compile-time option: `USE_CLIGEN44` for running clixon-45 with cligen-44. diff --git a/apps/backend/backend_client.c b/apps/backend/backend_client.c index a935be2b..5bcc0e14 100644 --- a/apps/backend/backend_client.c +++ b/apps/backend/backend_client.c @@ -284,7 +284,7 @@ clixon_stats_get_db(clicon_handle h, if (xml_stats(xt, &nr, &sz) < 0) goto done; cprintf(cb, "%s%" PRIu64 "" - "%" PRIu64 "", + "%zu", dbname, nr, sz); } retval = 0; @@ -977,8 +977,6 @@ from_client_get(clicon_handle h, yang_stmt *yspec; int i; cxobj *xerr = NULL; - cxobj *xr; - cxobj *xb; int ret; clicon_debug(1, "%s", __FUNCTION__); @@ -1064,21 +1062,16 @@ from_client_get(clicon_handle h, */ if ((ret = xml_yang_validate_all_top(h, xret, &xerr)) < 0) goto done; - if (ret > 0 && (ret = xml_yang_validate_add(h, xret, &xerr)) < 0) + if (ret > 0 && + (ret = xml_yang_validate_add(h, xret, &xerr)) < 0) goto done; if (ret == 0){ if (debug) clicon_log_xml(LOG_DEBUG, xret, "VALIDATE_STATE"); - if ((xr = xpath_first(xerr, NULL, "//error-tag")) != NULL && - (xb = xml_body_get(xr))){ - if (xml_value_set(xb, "operation-failed") < 0) - goto done; - } - if ((xr = xpath_first(xerr, NULL, "//error-message")) != NULL && - (xb = xml_body_get(xr))){ - if (xml_value_append(xb, " Internal error, state callback returned invalid XML") < 0) - goto done; - } + if (clixon_netconf_internal_error(xerr, + ". Internal error, state callback returned invalid XML", + NULL) < 0) + goto done; if (clicon_xml2cbuf(cbret, xerr, 0, 0, -1) < 0) goto done; goto ok; diff --git a/apps/backend/backend_plugin.c b/apps/backend/backend_plugin.c index 0e6eb281..1eaceee1 100644 --- a/apps/backend/backend_plugin.c +++ b/apps/backend/backend_plugin.c @@ -230,6 +230,7 @@ clixon_plugin_statedata_all(clicon_handle h, cxobj *x = NULL; clixon_plugin *cp = NULL; cbuf *cberr = NULL; + cxobj *xerr = NULL; clicon_debug(1, "%s", __FUNCTION__); while ((cp = clixon_plugin_each(h, cp)) != NULL) { @@ -249,8 +250,18 @@ clixon_plugin_statedata_all(clicon_handle h, clicon_log_xml(LOG_DEBUG, x, "%s STATE:", __FUNCTION__); #endif /* XXX: ret == 0 invalid yang binding should be handled as internal error */ - if (xml_bind_yang(x, YB_MODULE, yspec, NULL) < 0) + if ((ret = xml_bind_yang(x, YB_MODULE, yspec, &xerr)) < 0) goto done; + if (ret == 0){ + if (clixon_netconf_internal_error(xerr, + ". Internal error, state callback returned invalid XML: ", + cp->cp_name) < 0) + goto done; + xml_free(*xret); + *xret = xerr; + xerr = NULL; + goto fail; + } if (xml_sort_recurse(x) < 0) goto done; if (xml_default_recurse(x) < 0) @@ -266,6 +277,8 @@ clixon_plugin_statedata_all(clicon_handle h, } /* while plugin */ retval = 1; done: + if (xerr) + xml_free(xerr); if (cberr) cbuf_free(cberr); if (x) diff --git a/example/main/example_backend.c b/example/main/example_backend.c index fbf6f448..5bbe0e22 100644 --- a/example/main/example_backend.c +++ b/example/main/example_backend.c @@ -364,13 +364,23 @@ example_statedata(clicon_handle h, #endif } else{ + cxobj *x1; if ((fd = open(_state_file, O_RDONLY)) < 0){ clicon_err(OE_UNIX, errno, "open(%s)", _state_file); goto done; } - if (clixon_xml_parse_file(fd, YB_MODULE, yspec, NULL, &xstate, NULL) < 0) + if (clixon_xml_parse_file(fd, YB_MODULE, yspec, NULL, &xt, NULL) < 0) goto done; close(fd); + if ((x1 = xpath_first(xt, nsc, "%s", xpath)) != NULL){ + xml_flag_set(x1, XML_FLAG_MARK); + /* Remove everything that is not marked */ + if (xml_tree_prune_flagged_sub(xt, XML_FLAG_MARK, 1, NULL) < 0) + goto done; + xml_flag_reset(x1, XML_FLAG_MARK); + if (xml_copy(xt, xstate) < 0) + goto done; + } } } else { diff --git a/lib/clixon/clixon_netconf_lib.h b/lib/clixon/clixon_netconf_lib.h index 4cc5a5cb..4871d51f 100644 --- a/lib/clixon/clixon_netconf_lib.h +++ b/lib/clixon/clixon_netconf_lib.h @@ -107,7 +107,7 @@ const netconf_content netconf_content_str2int(char *str); const char *netconf_content_int2str(netconf_content nr); int netconf_hello_server(clicon_handle h, cbuf *cb, uint32_t session_id); int netconf_hello_req(clicon_handle h, cbuf *cb); - int clixon_netconf_error_fn(const char *fn, const int line, cxobj *xerr, const char *fmt, const char *arg); +int clixon_netconf_internal_error(cxobj *xerr, char *msg, char *arg); #endif /* _CLIXON_NETCONF_LIB_H */ diff --git a/lib/src/clixon_netconf_lib.c b/lib/src/clixon_netconf_lib.c index 633b5c8e..e6d175a1 100644 --- a/lib/src/clixon_netconf_lib.c +++ b/lib/src/clixon_netconf_lib.c @@ -1546,3 +1546,42 @@ clixon_netconf_error_fn(const char *fn, cbuf_free(cb); return retval; } + +/*! Add internal error info to existing netconf error message by rewriting + * + * If a eg sanity check detects errors in internal messages passing, such as from a plugin or + * in backend communication, an error is generated. However, it does not make sense to send this + * error message as-is to the requestor. Instead this function transforms the error to a more + * generic "operation-failed" error and adds info in its error message to say it is an internal error. + * If a requestor receives such an error, it will be clear that the error is internal. + * @param[in] xerr Netconf error xml tree on the form: + * @param[in] msg Error message + * @param[in] arg Extra error message (consider stdarg?) + * @retval 0 OK + * @retval -1 Error + */ +int +clixon_netconf_internal_error(cxobj *xerr, + char *msg, + char *arg) +{ + int retval = -1; + cxobj *xr; + cxobj *xb; + + if ((xr = xpath_first(xerr, NULL, "//error-tag")) != NULL && + (xb = xml_body_get(xr))){ + if (xml_value_set(xb, "operation-failed") < 0) + goto done; + } + if ((xr = xpath_first(xerr, NULL, "//error-message")) != NULL && + (xb = xml_body_get(xr))){ + if (xml_value_append(xb, msg) < 0) + goto done; + if (arg &&xml_value_append(xb, arg) < 0) + goto done; + } + retval = 0; + done: + return retval; +} diff --git a/lib/src/clixon_proto_client.c b/lib/src/clixon_proto_client.c index 6f154beb..aae2b758 100644 --- a/lib/src/clixon_proto_client.c +++ b/lib/src/clixon_proto_client.c @@ -374,6 +374,10 @@ clicon_rpc_get_config(clicon_handle h, if ((ret = xml_bind_yang(xd, YB_MODULE, yspec, &xerr)) < 0) goto done; if (ret == 0){ + if (clixon_netconf_internal_error(xerr, + ". Internal error, backend returned invalid XML.", + NULL) < 0) + goto done; if ((xd = xpath_first(xerr, NULL, "rpc-error")) == NULL){ clicon_err(OE_XML, ENOENT, "Expected rpc-error tag but none found(internal)"); goto done; @@ -730,7 +734,10 @@ clicon_rpc_get(clicon_handle h, if ((ret = xml_bind_yang(xd, YB_MODULE, yspec, &xerr)) < 0) goto done; if (ret == 0){ - assert(xerr != NULL); + if (clixon_netconf_internal_error(xerr, + ". Internal error, backend returned invalid XML.", + NULL) < 0) + goto done; if ((xd = xpath_first(xerr, NULL, "rpc-error")) == NULL){ clicon_err(OE_XML, ENOENT, "Expected rpc-error tag but none found(internal)"); goto done; diff --git a/test/lib.sh b/test/lib.sh index d4d2277b..cf5e93b4 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -345,20 +345,23 @@ expecteq(){ # Evaluate and return # like expecteq but partial match is OK -# Example: expectpart $(fn arg) 0 "my return" +# Example: expectpart $(fn arg) 0 "my return" -- "foo" # - evaluated expression # - expected command return value (0 if OK) # - expected stdout outcome* +# - the token "--not--" +# - not expected stdout outcome* # @note need to escape \[\] expectpart(){ r=$? ret=$1 retval=$2 expect=$3 + # echo "r:$r" # echo "ret:\"$ret\"" # echo "retval:$retval" -# echo "expect:$expect" +# echo "expect:\"$expect\"" if [ $r != $retval ]; then echo -e "\e[31m\nError ($r != $retval) in Test$testnr [$testname]:" echo -e "\e[0m:" @@ -367,15 +370,26 @@ expectpart(){ if [ -z "$ret" -a -z "$expect" ]; then return fi - # Loop over all variable args expect strings + # Loop over all variable args expect strings (skip first two args) + # note that "expect" var is never actually used + # Then test positive for strings, if the token --neg-- is detected, then test negative for the rest + positive=true; let i=0; for exp in "$@"; do - if [ $i -gt 1 ]; then + if [ "$exp" == "--not--" ]; then + positive=false; + elif [ $i -gt 1 ]; then # echo "echo \"$ret\" | grep --null -o \"$exp"\" match=$(echo "$ret" | grep --null -o "$exp") # XXX -EZo: -E cant handle {} r=$? - if [ $r != 0 ]; then - err "$exp" "$ret" + if $positive; then + if [ $r != 0 ]; then + err "$exp" "$ret" + fi + else + if [ $r == 0 ]; then + err "not $exp" "$ret" + fi fi fi let i++; @@ -385,7 +399,6 @@ expectpart(){ # fi } - # Pipe stdin to command # Arguments: # - Command diff --git a/test/test_leafref_state.sh b/test/test_leafref_state.sh index b4b3adcd..249656b9 100755 --- a/test/test_leafref_state.sh +++ b/test/test_leafref_state.sh @@ -167,10 +167,10 @@ expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "^]]>]]>' '^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 ']]>]]>' '^applicationoperation-failedxerrorLeafref validation failed: No leaf x matching path /ex:sender-config/ex:name. Internal error, state callback returned invalid XML]]>]]>$' new "netconf get / state-only should fail" -expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' '^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 ']]>]]>' '^applicationoperation-failedxerrorLeafref validation failed: No leaf x matching path /ex:sender-config/ex:name. Internal error, state callback returned invalid XML]]>]]>$' new "netconf get / config-only ok" expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' '^y]]>]]>$' diff --git a/test/test_perf_state.sh b/test/test_perf_state.sh index bb4f82c8..ab7a8419 100755 --- a/test/test_perf_state.sh +++ b/test/test_perf_state.sh @@ -83,6 +83,7 @@ module $APPNAME{ } EOF +# Mixed config + state XML new "generate state file with $perfnr list entries" echo -n "foo" > $fstate for (( i=0; i<$perfnr; i++ )); do diff --git a/test/test_restconf_err.sh b/test/test_restconf_err.sh index 4367ad79..59fb0ecd 100755 --- a/test/test_restconf_err.sh +++ b/test/test_restconf_err.sh @@ -15,6 +15,9 @@ # Note this is different from an api-path that is invalid from a yang point # of view, this is interpreted as 400 Bad Request invalid-value/unknown-element # XXX: complete non-existent yang with unknown-element for all PUT/POST/GET api-paths +# +# Also generate an invalid state XML. This should generate an "Internal" error and the name of the +# plugin should be visible in the error message. # Magic line must be first in script (see README.md) s="$_" ; . ./lib.sh || if [ "$s" = $0 ]; then exit 0; else return 0; fi @@ -25,6 +28,7 @@ cfg=$dir/conf.xml fyang=$dir/example.yang fyang2=$dir/augment.yang fxml=$dir/initial.xml +fstate=$dir/state.xml # example cat < $cfg @@ -36,6 +40,8 @@ cat < $cfg false /usr/local/var/$APPNAME/$APPNAME.sock $dir/restconf.pidfile + /usr/local/lib/$APPNAME/backend + example_backend.so$ /usr/local/var/$APPNAME /usr/local/lib/$APPNAME/clispec /usr/local/lib/$APPNAME/cli @@ -103,9 +109,32 @@ module example{ } } } + container mystate{ + config false; + description "Just for generating a invalid XML"; + list parameter{ + key name; + leaf name{ + type string; + } + leaf value{ + type string; + } + } + } } EOF +# State file with error: wrong namespace +cat < $fstate + + + x + x + + +EOF + # Initial tree XML=$(cat <0No leaf b, No container c, No leaf d @@ -121,28 +150,29 @@ if [ $BE -ne 0 ]; then err fi sudo pkill -f clixon_backend # to be sure - new "start backend -s init -f $cfg" - start_backend -s init -f $cfg + new "start backend -s init -f $cfg -- -sS $fstate" + start_backend -s init -f $cfg -- -sS $fstate fi new "waiting" wait_backend -new "kill old restconf daemon" -sudo pkill -u $wwwuser -f clixon_restconf +if [ $RC -ne 0 ]; then + new "kill old restconf daemon" + sudo pkill -u $wwwuser -f clixon_restconf -new "start restconf daemon" -start_restconf -f $cfg + new "start restconf daemon" + start_restconf -f $cfg -new "waiting" -wait_restconf + new "waiting" + wait_restconf +fi new "restconf POST initial tree" -expecteq "$(curl -s -X POST -H 'Content-Type: application/yang-data+xml' -d "$XML" http://localhost/restconf/data)" 0 '' +expectpart "$(curl -si -X POST -H 'Content-Type: application/yang-data+xml' -d "$XML" http://localhost/restconf/data)" 0 'HTTP/1.1 201 Created' new "restconf GET initial datastore" -expecteq "$(curl -s -X GET -H 'Accept: application/yang-data+xml' http://localhost/restconf/data/example:a=0)" 0 "$XML - " +expectpart "$(curl -si -X GET -H 'Accept: application/yang-data+xml' http://localhost/restconf/data/example:a=0)" 0 'HTTP/1.1 200 OK' "$XML" new "restconf GET non-qualified list" expectpart "$(curl -si -X GET http://localhost/restconf/data/example:a)" 0 'HTTP/1.1 400 Bad Request' "{\"ietf-restconf:errors\":{\"error\":{\"error-type\":\"rpc\",\"error-tag\":\"malformed-message\",\"error-severity\":\"error\",\"error-message\":\"malformed key =example:a, expected '=restval'\"}}}" @@ -165,7 +195,7 @@ expectpart "$(curl -is -X POST -H 'Content-Type: application/yang-data+xml' -d " # Test for multi-module path where an augment stretches across modules new "restconf POST augment multi-namespace path" -expecteq "$(curl -s -X POST -H 'Content-Type: application/yang-data+xml' -d '23' http://localhost/restconf/data)" 0 '' +expectpart "$(curl -si -X POST -H 'Content-Type: application/yang-data+xml' -d '23' http://localhost/restconf/data)" 0 'HTTP/1.1 201 Created' new "restconf GET augment multi-namespace top" expectpart "$(curl -si -X GET http://localhost/restconf/data/augment:route-config)" 0 'HTTP/1.1 200 OK' '{"augment:route-config":{"dynamic":{"example:ospf":{"reference-bandwidth":23}}}}' @@ -183,8 +213,15 @@ expectpart "$(curl -si -X GET http://localhost/restconf/data/augment:route-confi new "restconf GET augment multi-namespace, no 2nd module in api-path, fail" expectpart "$(curl -si -X GET http://localhost/restconf/data/augment:route-config/dynamic/ospf)" 0 'HTTP/1.1 404 Not Found' '{"ietf-restconf:errors":{"error":{"error-type":"application","error-tag":"invalid-value","error-severity":"error","error-message":"Instance does not exist"}}}' -new "Kill restconf daemon" -stop_restconf +#---------------------------------------------- +# Also generate an invalid state XML. This should generate an "Internal" error and the name of the +new "restconf GET failed state" +expectpart "$(curl -si -X GET -H 'Accept: application/yang-data+xml' http://localhost/restconf/data?content=nonconfig)" 0 '412 Precondition Failed' 'applicationoperation-failedmystateerrorNo such yang module. Internal error, state callback returned invalid XML: example_backend' + +if [ $RC -ne 0 ]; then + new "Kill restconf daemon" + stop_restconf +fi if [ $BE -eq 0 ]; then exit # BE