From 6f0bd01a6a359a1873b21f7200e3c54648446242 Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Tue, 18 Oct 2022 09:36:11 +0200 Subject: [PATCH] Confirm-commit RESTCONF support --- CHANGELOG.md | 3 ++ apps/backend/backend_client.c | 41 ++++++++++----- apps/backend/backend_commit.c | 40 ++++++++------ apps/backend/clixon_backend_commit.h | 5 +- test/test_confirmed_commit.sh | 64 +++++++++++++++++------ yang/clixon/clixon-config@2022-03-21.yang | 2 + 6 files changed, 108 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 43f98a8b..e9d23ee7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,9 @@ Expected: End of 2022 * 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) + * Known issues + * Backend privileges drop + * Lock check, see RFC 6241 7.5 ### API changes on existing protocol/config features diff --git a/apps/backend/backend_client.c b/apps/backend/backend_client.c index 94866a9b..95cfdda3 100644 --- a/apps/backend/backend_client.c +++ b/apps/backend/backend_client.c @@ -455,19 +455,34 @@ from_client_edit_config(clicon_handle h, autocommit = 1; /* If autocommit option is set or requested by client */ if (clicon_autocommit(h) || autocommit) { - // TODO: if this is from a restconf client ... - // and, if there is an existing ephemeral commit, set is_valid_confirming_commit=1 such that - // candidate_commit will apply the configuration per RFC 8040 1.4: - // If a confirmed commit procedure is - // in progress by any NETCONF client, then any new commit will act as - // the confirming commit. - // and, if there is an existing persistent commit, netconf_operation_failed with "in-use", so - // that the restconf server will return "409 Conflict" per RFC 8040 1.4: - // If the NETCONF server is expecting a - // "persist-id" parameter to complete the confirmed commit procedure, - // then the RESTCONF edit operation MUST fail with a "409 Conflict" - // status-line. The error-tag "in-use" is used in this case. - + /* if this is from a restconf client ... + * and, if there is an existing ephemeral commit, set is_valid_confirming_commit=1 such that + * candidate_commit will apply the configuration per RFC 8040 1.4: + * If a confirmed commit procedure is + * in progress by any NETCONF client, then any new commit will act as + * the confirming commit. + * and, if there is an existing persistent commit, netconf_operation_failed with "in-use", so + * that the restconf server will return "409 Conflict" per RFC 8040 1.4: + * If the NETCONF server is expecting a + * "persist-id" parameter to complete the confirmed commit procedure, + * then the RESTCONF edit operation MUST fail with a "409 Conflict" + * status-line. The error-tag "in-use" is used in this case. + */ + if (if_feature(yspec, "ietf-netconf", "confirmed-commit")) { + switch (confirmed_commit.state){ + case INACTIVE: + break; + case PERSISTENT: + if (netconf_in_use(cbret, "application", "Persistent commit is ongoing")< 0) + goto done; + goto ok; + break; + case EPHEMERAL: + case ROLLBACK: + cancel_confirmed_commit(h); + break; + } + } if ((ret = candidate_commit(h, "candidate", cbret)) < 0){ /* Assume validation fail, nofatal */ if (netconf_operation_failed(cbret, "application", clicon_err_reason)< 0) goto done; diff --git a/apps/backend/backend_commit.c b/apps/backend/backend_commit.c index 8e689620..58af5c3c 100644 --- a/apps/backend/backend_commit.c +++ b/apps/backend/backend_commit.c @@ -648,7 +648,7 @@ candidate_validate(clicon_handle h, * @retval -1 No Rollback event was found */ int -cancel_rollback_event() +cancel_rollback_event(void) { int retval; @@ -723,6 +723,28 @@ schedule_rollback_event(clicon_handle h, return retval; } +/*! Cancel a confirming commit by removing rollback, and free state + * @param[in] h + * @param[out] cbret + * @retval 0 OK + */ +int +cancel_confirmed_commit(clicon_handle h) +{ + cancel_rollback_event(); + + if (confirmed_commit.state == PERSISTENT && confirmed_commit.persist_id != NULL) { + free(confirmed_commit.persist_id); + confirmed_commit.persist_id = NULL; + } + + confirmed_commit.state = INACTIVE; + + if (xmldb_delete(h, "rollback") < 0) + clicon_err(OE_DB, 0, "Error deleting the rollback configuration"); + return 0; +} + /*! Handle the second phase of confirmed-commit processing. * * In the first phase, the proper action was taken in the case of a valid confirming-commit, but no subsequent @@ -1211,20 +1233,8 @@ from_client_commit(clicon_handle h, /* If is *not* present, this will conclude the confirmed-commit, so cancel the rollback. */ if (xml_find_type(confirmed_commit.xe, NULL, "confirmed", CX_ELMNT) == NULL && is_valid_confirming_commit) { - cancel_rollback_event(); - - if (confirmed_commit.state == PERSISTENT && confirmed_commit.persist_id != NULL) { - free(confirmed_commit.persist_id); - confirmed_commit.persist_id = NULL; - } - - confirmed_commit.state = INACTIVE; - - if (xmldb_delete(h, "rollback") < 0) - clicon_err(OE_DB, 0, "Error deleting the rollback configuration"); - - cprintf(cbret, "", NETCONF_BASE_NAMESPACE); - + cancel_confirmed_commit(h); + cprintf(cbret, "", NETCONF_BASE_NAMESPACE); goto ok; } } diff --git a/apps/backend/clixon_backend_commit.h b/apps/backend/clixon_backend_commit.h index 62785eef..46a00a64 100644 --- a/apps/backend/clixon_backend_commit.h +++ b/apps/backend/clixon_backend_commit.h @@ -65,13 +65,14 @@ struct confirmed_commit { void *arg; // the clicon_handle that will be passed to rollback_fn() }; -extern struct confirmed_commit confirmed_commit; +extern struct confirmed_commit confirmed_commit; // XXX global /* * Prototypes */ int do_rollback(clicon_handle h, uint8_t *errs); -int cancel_rollback_event(); +int cancel_rollback_event(void); +int cancel_confirmed_commit(clicon_handle h); int startup_validate(clicon_handle h, char *db, cxobj **xtr, cbuf *cbret); int startup_commit(clicon_handle h, char *db, cbuf *cbret); diff --git a/test/test_confirmed_commit.sh b/test/test_confirmed_commit.sh index 9d109328..a805ce27 100755 --- a/test/test_confirmed_commit.sh +++ b/test/test_confirmed_commit.sh @@ -1,10 +1,8 @@ #!/usr/bin/env bash # Netconf confirm commit capability -# See RFC 6241 Section 8.4 -# Test uses privileges drop +# See RFC 6241 Section 8.4 and RFC 8040 Section 1.4 # TODO: # - privileges drop -# - restconf # - lock check # Magic line must be first in script (see README.md) @@ -24,19 +22,12 @@ RESTCONFIG=$(restconf_config none false) cat < $cfg - $cfg ietf-netconf:confirmed-commit clixon-restconf:allow-auth-none - 42 + $cfg ${YANG_INSTALLDIR} - $IETFRFC $fyang /usr/local/lib/$APPNAME/clispec - /usr/local/lib/$APPNAME/backend - example_backend.so$ - /usr/local/lib/$APPNAME/netconf - false - /usr/local/lib/$APPNAME/restconf /usr/local/lib/$APPNAME/cli $APPNAME $dir/$APPNAME.sock @@ -90,6 +81,7 @@ function rpc() { } function commit() { + new "commit $1" if [[ "$1" == "" ]] then rpc "" "" @@ -101,6 +93,7 @@ function commit() { function edit_config() { TARGET="$1" CONFIG="$2" + new "edit-config $1 $2" rpc "<$TARGET/>$CONFIG" "" } @@ -111,6 +104,7 @@ function assert_config_equals() { rpc "<$TARGET/>" "$(data "$EXPECTED")" } +# delete all function reset() { rpc "none" "" commit @@ -123,8 +117,10 @@ RUNNING_PATH="/usr/local/var/$APPNAME/running_db" ROLLBACK_PATH="/usr/local/var/$APPNAME/rollback_db" FAILSAFE_PATH="/usr/local/var/$APPNAME/failsafe_db" + CONFIGB="eth0
" CONFIGC="eth1
" +CONFIGCONLY="eth1" # restcpnf CONFIGBPLUSC="eth0eth1
" FAILSAFE_CFG="eth99
" @@ -144,8 +140,6 @@ fi new "wait backend" wait_backend - - new "Hello check confirm-commit capability" expecteof "$clixon_netconf -f $cfg" 0 "urn:ietf:params:netconf:base:1.1]]>]]>" "urn:ietf:params:netconf:capability:confirmed-commit:1.1" '^$' @@ -225,7 +219,6 @@ commit "" assert_config_equals "running" "$CONFIGB" ################################################################################ - # 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() @@ -247,14 +240,16 @@ stop_backend -f $cfg start_backend -s init -f $cfg ################################################################################ - new "backend loads failsafe at startup if rollback present but cannot be loaded" + +if [ ${valgrindtest} -eq 2 ]; then # backend valgrind + sleep 3 +fi reset sudo tee "$FAILSAFE_PATH" > /dev/null << EOF # create a failsafe database $FAILSAFE_CFG EOF - edit_config "candidate" "$CONFIGC" commit "foobar" assert_config_equals "running" "$CONFIGC" @@ -382,7 +377,6 @@ assert_config_equals "running" "$CONFIGBPLUSC" sleep 5 assert_config_equals "running" "" - # TODO test restconf receives "409 conflict" when there is a persistent confirmed-commit active # TODO test restconf causes confirming-commit for ephemeral confirmed-commit if [ $RC -ne 0 ]; then @@ -396,6 +390,42 @@ fi new "wait restconf" wait_restconf +new "restconf as confirmed commit" +reset +edit_config "candidate" "$CONFIGB" +assert_config_equals "candidate" "$CONFIGB" +# 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 + +new "restconf POST" +expectpart "$(curl $CURLOPTS -X POST -H "Content-Type: application/yang-data+xml" -d "$CONFIGCONLY" $RCPROTO://localhost/restconf/data/clixon-example:table)" 0 "HTTP/$HVER 201" "location:" + +assert_config_equals "running" "$CONFIGBPLUSC" + +new "soft kill" +kill ${PIDS[0]} # kill the while loop above to close STDIN on 1st + +assert_config_equals "running" "$CONFIGBPLUSC" + +kill -9 ${PIDS[0]} 2> /dev/null # kill the while loop above to close STDIN on 1st + +assert_config_equals "running" "$CONFIGBPLUSC" + +################################################################################ + +new "restconf persistid expect fail" +reset +edit_config "candidate" "$CONFIGB" +commit "a" +assert_config_equals "running" "$CONFIGB" + +new "restconf POST" +expectpart "$(curl $CURLOPTS -X POST -H "Content-Type: application/yang-data+xml" -d "$CONFIGCONLY" $RCPROTO://localhost/restconf/data/clixon-example:table)" 0 # "HTTP/$HVER 409" + +assert_config_equals "running" "$CONFIGB" + if [ $RC -ne 0 ]; then new "Kill restconf daemon" stop_restconf diff --git a/yang/clixon/clixon-config@2022-03-21.yang b/yang/clixon/clixon-config@2022-03-21.yang index 00441a8b..693b3be1 100644 --- a/yang/clixon/clixon-config@2022-03-21.yang +++ b/yang/clixon/clixon-config@2022-03-21.yang @@ -862,6 +862,8 @@ module clixon-config { description "Set if all configuration changes are committed automatically on every edit change. Explicit commit commands unnecessary + If confirm-commit, follow RESTCONF semantics: commit ephemeral but fail on + persistent confirming commit. (consider boolean)"; } leaf CLICON_XMLDB_DIR {