From 8abcda6f85658cae96bb01174385f07687aa4c50 Mon Sep 17 00:00:00 2001 From: Phil Heller Date: Mon, 3 Oct 2022 21:42:01 -0600 Subject: [PATCH] confirmed-commit fixes, tests - fixed typo preventing evaluation of confirmed-commit logic in backend_startup - fixed uninitialized variable warnings - added details to CHANGELOG.MD - added capabilities advertisement for confirmed-commit 1.0 and 1.1 - added xml hello message that uses only eom framing, for simplicity in asynch tests - add stty restore after wait_restconf to fix console corruption in tests - adjust test_confirmed_commit to drop perms and run as the invoking user. This will require running user to have permissions on /usr/local/var/example - added CLI tests --- CHANGELOG.md | 15 ++++ apps/backend/backend_commit.c | 6 +- apps/backend/backend_main.c | 1 - apps/backend/backend_startup.c | 4 +- lib/src/clixon_datastore.c | 5 +- lib/src/clixon_netconf_lib.c | 8 ++ test/lib.sh | 6 ++ test/site.sh | 1 + test/test_confirmed_commit.sh | 130 +++++++++++++++++++++++++++++++-- 9 files changed, 160 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2075914..9886f4f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,19 @@ ## 6.0.0 Expected: End of 2022 +### New features + +* Confirmed-commit capability + * Standards + * RFC 4741 "NETCONF Configuration Protocol": Section 8.4 + * RFC 6241 "Network Configuration Protocol (NETCONF)": Section 8.4 + * Features + * `:confirmed-commit:1.0` capability + * `:confirmed-commit:1.1` capability + * Added support for relevant arguments to CLI commit + * See [example_cli.cli](https://github.com/clicon/clixon/blob/master/example/main/example_cli.cli) + * See [Netconf Confirmed Commit Capability](https://github.com/clicon/clixon/issues/255) + ### API changes on existing protocol/config features Users may have to change how they access the system @@ -53,6 +66,8 @@ Developers may need to change their code * C API changes * Added `defaults` parameter to `clicon_rpc_get_pageable_list()` + * Added `confirmed`, `cancel`, `timeout`, `persist-id`, and `persist-id-val` parameters to + `cli_commit(...)` ### Minor features diff --git a/apps/backend/backend_commit.c b/apps/backend/backend_commit.c index 658e5f72..4ce995d8 100644 --- a/apps/backend/backend_commit.c +++ b/apps/backend/backend_commit.c @@ -676,7 +676,7 @@ rollback_fn(int fd, { clicon_handle h = arg; - clicon_log(LOG_EMERG, "a confirming-commit was not received before the confirm-timeout expired; rolling back"); + clicon_log(LOG_CRIT, "a confirming-commit was not received before the confirm-timeout expired; rolling back"); return do_rollback(h, NULL); } @@ -1017,7 +1017,7 @@ do_rollback(clicon_handle h, uint8_t *errs) * restart the backend, which will try to load the rollback_db, and delete it if successful * (otherwise it will load the failsafe) */ - clicon_log(LOG_EMERG, "An error occurred during rollback and the rollback_db wasn't deleted."); + clicon_log(LOG_CRIT, "An error occurred during rollback and the rollback_db wasn't deleted."); errstate |= ROLLBACK_NOT_APPLIED | ROLLBACK_DB_NOT_DELETED; goto done; } @@ -1044,7 +1044,7 @@ do_rollback(clicon_handle h, uint8_t *errs) /* Attempt to load the failsafe config */ if (load_failsafe(h, "Rollback") < 0) { - clicon_log(LOG_EMERG, "An error occurred committing the failsafe database. Exiting."); + clicon_log(LOG_CRIT, "An error occurred committing the failsafe database. Exiting."); /* Invoke our own signal handler to exit */ raise(SIGINT); diff --git a/apps/backend/backend_main.c b/apps/backend/backend_main.c index 727a73c8..fa0ac8ef 100644 --- a/apps/backend/backend_main.c +++ b/apps/backend/backend_main.c @@ -888,7 +888,6 @@ main(int argc, switch (startup_mode){ case SM_INIT: /* Scratch running and start from empty */ /* Delete any rollback database, if it exists */ - // TODO: xmldb_delete doesn't actually unlink; need to look at this xmldb_delete(h, "rollback"); /* [Delete and] create running db */ if (xmldb_db_reset(h, "running") < 0) diff --git a/apps/backend/backend_startup.c b/apps/backend/backend_startup.c index 7f7a02c8..0602d64f 100644 --- a/apps/backend/backend_startup.c +++ b/apps/backend/backend_startup.c @@ -133,7 +133,7 @@ startup_mode_startup(clicon_handle h, cbuf *cbret) { int retval = -1; - int ret; + int ret = 0; int db_exists; if (strcmp(db, "running")==0){ @@ -154,7 +154,7 @@ startup_mode_startup(clicon_handle h, * rebooted. */ yang_stmt *yspec = clicon_dbspec_yang(h); - if (if_feature(yspec, "ietf-netconf", "configmed-commit")) { + if (if_feature(yspec, "ietf-netconf", "confirmed-commit")) { db_exists = xmldb_exists(h, "rollback"); if (db_exists < 0) { clicon_err(OE_DAEMON, 0, "Error checking for the existence of the rollback database"); diff --git a/lib/src/clixon_datastore.c b/lib/src/clixon_datastore.c index 8048f4b3..bc5f09a4 100644 --- a/lib/src/clixon_datastore.c +++ b/lib/src/clixon_datastore.c @@ -418,7 +418,8 @@ xmldb_delete(clicon_handle h, if (xmldb_db2file(h, db, &filename) < 0) goto done; if (lstat(filename, &sb) == 0) - // TODO this had been changed from unlink to truncate some time ago. It was changed back for confirmed-commit + // TODO this had been changed from unlink to truncate some time ago, likely related to dropping privileges. + // It was changed back for confirmed-commit, and test_confirmed_commit.sh drops privileges. // as the presence of the rollback_db at startup triggers loading of the rollback rather than the startup // configuration. It might not be sufficient to check for a truncated file. Needs more review, switching back // to unlink temporarily. @@ -619,7 +620,7 @@ xmldb_rename(clicon_handle h, const char *suffix) { char *old; - char *fname; + char *fname = NULL; int retval = -1; if ((xmldb_db2file(h, db, &old)) < 0) { diff --git a/lib/src/clixon_netconf_lib.c b/lib/src/clixon_netconf_lib.c index 6d7d795e..e0e92d6d 100644 --- a/lib/src/clixon_netconf_lib.c +++ b/lib/src/clixon_netconf_lib.c @@ -1765,6 +1765,7 @@ netconf_hello_server(clicon_handle h, char *module_set_id; char *ietf_yang_library_revision; char *encstr = NULL; + yang_stmt *yspec = clicon_dbspec_yang(h); module_set_id = clicon_option_str(h, "CLICON_MODULE_SET_ID"); cprintf(cb, "", NETCONF_BASE_NAMESPACE); @@ -1799,6 +1800,13 @@ netconf_hello_server(clicon_handle h, cprintf(cb, ""); xml_chardata_cbuf_append(cb, "urn:ietf:params:netconf:capability:with-defaults:1.0?basic-mode=explicit&also-supported=report-all,trim,report-all-tagged"); cprintf(cb, ""); + + /* rfc 4741 and 6241 confirmed-commit capabilities */ + if (if_feature(yspec, "ietf-netconf", "confirmed-commit")) { + cprintf(cb, "urn:ietf:params:netconf:capability:confirmed-commit:1.0"); + cprintf(cb, "urn:ietf:params:netconf:capability:confirmed-commit:1.1"); + } + cprintf(cb, ""); if (session_id) cprintf(cb, "%lu", (long unsigned int)session_id); diff --git a/test/lib.sh b/test/lib.sh index ac39a21d..cb5e8f98 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -98,6 +98,9 @@ DEFAULTNS="$DEFAULTONLY message-id=\"42\"" # Minimal hello message as a prelude to netconf rpcs DEFAULTHELLO="urn:ietf:params:netconf:base:1.0urn:ietf:params:netconf:base:1.1]]>]]>" +# Minimal hello message that excludes 1.1 capability, in the case EOM style framing is needed +HELLONO11="urn:ietf:params:netconf:base:1.0]]>]]>" + # XXX cannot get this to work for all combinations of nc/netcat fcgi/native # But leave it here for debugging where netcat works properly if [ -n "$(type netcat 2> /dev/null)" ]; then @@ -569,6 +572,7 @@ function wait_backend(){ # Start restconf daemon # @see wait_restconf function start_restconf(){ + STTYSETTINGS=`stty -g` # Start in background echo "sudo -u $wwwstartuser -s $clixon_restconf $RCLOG -D $DBG $*" sudo -u $wwwstartuser -s $clixon_restconf $RCLOG -D $DBG $* /dev/null & @@ -625,6 +629,8 @@ function wait_restconf(){ if [ $valgrindtest -eq 3 ]; then sleep 2 # some problems with valgrind fi + + stty $STTYSETTINGS } # Wait for restconf to stop diff --git a/test/site.sh b/test/site.sh index c01c7f26..480ef3bf 100644 --- a/test/site.sh +++ b/test/site.sh @@ -10,6 +10,7 @@ # The SKIPLIST has precedence over the 'pattern' variable that you can use to # specify included file when running the various test scripts such as "all.sh". #SKIPLIST="test_[a-t]*\.sh test_openconfig.sh test_yangmodels.sh" +SKIPLIST="test_http_data.sh test_netconf_ssh_callhome.sh test_privileges.sh test_restconf.sh test_yang_models_ieee.sh" # # Parse yang openconfig models from https://github.com/openconfig/public OPENCONFIG=/usr/local/share/openconfig/public diff --git a/test/test_confirmed_commit.sh b/test/test_confirmed_commit.sh index b5ee69f1..3f18fc86 100755 --- a/test/test_confirmed_commit.sh +++ b/test/test_confirmed_commit.sh @@ -35,6 +35,8 @@ cat < $cfg $dir/$APPNAME.sock /usr/local/var/$APPNAME/$APPNAME.pidfile /usr/local/var/$APPNAME + $USER + drop_perm EOF @@ -213,6 +215,8 @@ fi new "wait backend" wait_backend +################################################################################ + new "netconf ephemeral confirmed-commit rolls back after disconnect" reset edit_config "candidate" "$CONFIGB" @@ -220,6 +224,8 @@ assert_config_equals "candidate" "$CONFIGB" commit "30" assert_config_equals "running" "" +################################################################################ + new "netconf persistent confirmed-commit" reset edit_config "candidate" "$CONFIGB" @@ -229,12 +235,18 @@ edit_config "candidate" "$CONFIGC" commit "aba" assert_config_equals "running" "$CONFIGBPLUSC" +################################################################################ + new "netconf cancel-commit with invalid persist-id" rpc "abc" "applicationinvalid-valueerrora confirmed-commit with the given persist-id was not found" +################################################################################ + new "netconf cancel-commit with valid persist-id" rpc "ab" "" +################################################################################ + new "netconf persistent confirmed-commit with timeout" reset edit_config "candidate" "$CONFIGB" @@ -243,6 +255,8 @@ assert_config_equals "running" "$CONFIGB" sleep 2 assert_config_equals "running" "" +################################################################################ + new "netconf persistent confirmed-commit with reset timeout" reset edit_config "candidate" "$CONFIGB" @@ -257,6 +271,8 @@ assert_config_equals "running" "$CONFIGBPLUSC" sleep 5 assert_config_equals "running" "" +################################################################################ + new "netconf persistent confirming-commit to epehemeral confirmed-commit should rollback" reset edit_config "candidate" "$CONFIGB" @@ -265,6 +281,8 @@ assert_config_equals "running" "$CONFIGB" commit "" assert_config_equals "running" "" +################################################################################ + new "netconf confirming-commit for persistent confirmed-commit with empty persist value" reset edit_config "candidate" "$CONFIGB" @@ -273,8 +291,10 @@ assert_config_equals "running" "$CONFIGB" commit "" assert_config_equals "running" "$CONFIGB" -# TODO the next two tests are broken. The whole idea of presence or absence of rollback_db indicating something might -# need reconsideration. see clixon_datastore.c#xmldb_delete() and backend_startup.c#startup_mode_startup() +################################################################################ + +# TODO reconsider logic around presence/absence of rollback_db as a signal as dropping permissions may impact ability +# to unlink and/or create that file. see clixon_datastore.c#xmldb_delete() and backend_startup.c#startup_mode_startup() new "backend loads rollback if present at startup" reset @@ -293,6 +313,8 @@ assert_config_equals "running" "$CONFIGB" stop_backend -f $cfg start_backend -s init -f $cfg -- -s +################################################################################ + new "backend loads failsafe at startup if rollback present but cannot be loaded" reset @@ -316,26 +338,118 @@ start_backend -s running -f $cfg -- -s wait_backend assert_config_equals "running" "$FAILSAFE_CFG" - -# TODO this test is now broken too, but not sure why; suspicion that the initial confirmed-commit session is not kept alive as intended stop_backend -f $cfg start_backend -s init -f $cfg -lf/tmp/clixon.log -D1 -- -s wait_backend + +################################################################################ + new "ephemeral confirmed-commit survives unrelated ephemeral session disconnect" reset edit_config "candidate" "$CONFIGB" +assert_config_equals "candidate" "$CONFIGB" # start a new ephemeral confirmed commit, but keep the confirmed-commit session alive (need to put it in the background) -sleep 60 | cat <(echo "$DEFAULTHELLO60]]>]]>") -| $clixon_netconf -qf $cfg >> /dev/null & +# use HELLONO11 which uses older EOM framing +sleep 60 | cat <(echo "$HELLONO1160]]>]]>") -| $clixon_netconf -qf $cfg >> /dev/null & PIDS=($(jobs -l % | cut -c 6- | awk '{print $1}')) assert_config_equals "running" "$CONFIGB" # assert config twice to prove it surives disconnect assert_config_equals "running" "$CONFIGB" # of ephemeral sessions kill -9 ${PIDS[0]} # kill the while loop above to close STDIN on 1st - # ephemeral session and cause rollback + +################################################################################ + +new "cli ephemeral confirmed-commit rolls back after disconnect" +reset + +tmppipe=$(mktemp -u) +mkfifo -m 600 "$tmppipe" +cat << EOF | clixon_cli -f $cfg >> /dev/null & +set interfaces interface eth0 type ex:eth +set interfaces interface eth0 enabled true +commit confirmed 60 +shell echo >> $tmppipe +shell cat $tmppipe +quit +EOF +cat $tmppipe >> /dev/null +assert_config_equals "running" "$CONFIGB" +echo >> $tmppipe +sleep 1 +assert_config_equals "running" "" +rm $tmppipe + +################################################################################ + +new "cli persistent confirmed-commit" +reset +cat << EOF | clixon_cli -f $cfg >> /dev/null +set interfaces interface eth0 type ex:eth +set interfaces interface eth0 enabled true +commit confirmed persist a +quit +EOF +assert_config_equals "running" "$CONFIGB" + +cat << EOF | clixon_cli -f $cfg >> /dev/null +set interfaces interface eth1 type ex:eth +set interfaces interface eth1 enabled true +commit persist-id a confirmed persist ab +quit +EOF +assert_config_equals "running" "$CONFIGBPLUSC" + +################################################################################ + +new "cli cancel-commit with invalid persist-id" +expectpart "$($clixon_cli -lo -1 -f $cfg commit persist-id abc cancel)" 255 "a confirmed-commit with the given persist-id was not found" + +################################################################################ + +new "cli cancel-commit with valid persist-id" +expectpart "$($clixon_cli -lo -1 -f $cfg commit persist-id ab cancel)" 0 "^$" +assert_config_equals "running" "" + +################################################################################ + +new "cli cancel-commit with no confirmed-commit in progress" +expectpart "$($clixon_cli -lo -1 -f $cfg commit persist-id ab cancel)" 255 "no confirmed-commit is in progress" + +################################################################################ + +new "cli persistent confirmed-commit with timeout" +reset +cat << EOF | clixon_cli -f $cfg >> /dev/null +set interfaces interface eth0 type ex:eth +set interfaces interface eth0 enabled true +commit confirmed persist abcd 2 +EOF +assert_config_equals "running" "$CONFIGB" +sleep 2 +assert_config_equals "running" "" + +################################################################################ + +new "cli persistent confirmed-commit with reset timeout" +reset +cat << EOF | clixon_cli -f $cfg >> /dev/null +set interfaces interface eth0 type ex:eth +set interfaces interface eth0 enabled true +commit confirmed persist abcd 5 +EOF +assert_config_equals "running" "$CONFIGB" +cat << EOF | clixon_cli -f $cfg >> /dev/null +set interfaces interface eth1 type ex:eth +set interfaces interface eth1 enabled true +commit persist-id abcd confirmed persist abcdef 10 +EOF +sleep 6 +assert_config_equals "running" "$CONFIGBPLUSC" +# now sleep long enough for rollback to happen; get config, assert == A +sleep 5 assert_config_equals "running" "" -# TODO test same cli methods as tested for netconf # TODO test restconf receives "409 conflict" when there is a persistent confirmed-commit active # TODO test restconf causes confirming-commit for ephemeral confirmed-commit @@ -343,7 +457,7 @@ assert_config_equals "running" "" if [ $BE -ne 0 ]; then new "Kill backend" # Check if premature kill - pid=$(pgrep -u root -f clixon_backend) + pid=$(pgrep -u ${USER} -f clixon_backend) if [ -z "$pid" ]; then err "backend already dead" fi