From c35593a6d9ade5c549edfcfa1bd56499ce7941e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Pecka?= Date: Wed, 5 Jun 2024 13:10:11 +0200 Subject: [PATCH] restconf: better error message for possible missing action node There is a possible data race in sysrepo when calling actions. First, we check if the action node is present and then we executed the action. There is a possibility that between those two sysrepo calls the action node can be created or deleted. Fortunately, sysrepo raises and error when the action node is not present when performing the rpc call. Unfortunately, it is the same error as if the input data tree for the rpc was invalid. I was thinking about removing the check for the action node existence and rely solely on the sysrepo's exception. This would indeed remove the data race but the disadvantage is, that the error message is now very broad (in case of actions) because we can't differentiate between invalid input data and missing action node. I don't think this would bring a better behaviour from rousette. There are two possible scenarios with the current behaviour: 1) The data node gets created between the check and the rpc call. From the client's point of view the node does not exist in the moment the request is created and I *think* it is ok to reject it. 2) The data node gets deleted between the check and the rpc call. From the client's point of view the node was existing and it is expected to return something. However, sysrepo (and rousette) reports the validation failure error which is (at least) confusing. Therefore I have edited the error message to report this possibility as well. Bug: https://github.com/CESNET/rousette/issues/2 Change-Id: Iae1a70160a8c5d32ed041ef90c4b67e18cd964c1 --- src/restconf/Server.cpp | 19 ++++++++++++++----- tests/restconf-rpc.cpp | 4 ++-- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/restconf/Server.cpp b/src/restconf/Server.cpp index 8632bad5..31eb6f4e 100644 --- a/src/restconf/Server.cpp +++ b/src/restconf/Server.cpp @@ -163,6 +163,7 @@ void processActionOrRPC(std::shared_ptr requestCtx) { requestCtx->sess.switchDatastore(sysrepo::Datastore::Operational); auto ctx = requestCtx->sess.getContext(); + bool isAction = false; try { auto rpcSchemaNode = ctx.findPath(requestCtx->restconfRequest.path); @@ -170,15 +171,22 @@ void processActionOrRPC(std::shared_ptr requestCtx) throw ErrorResponse(400, "protocol", "invalid-value", "Content-type header missing."); } - // validate if action node's parent is present - if (rpcSchemaNode.nodeType() == libyang::NodeType::Action) { - // FIXME: This is race-prone: we check for existing action data node but before we send the RPC the node may be gone + isAction = rpcSchemaNode.nodeType() == libyang::NodeType::Action; + + // check if action node's parent is present + if (isAction) { + /* + * This is race-prone: + * - The data node exists but might get deleted right after this check: Sysrepo throws an error when this happens. + * - The data node does not exist but might get created right after this check: The node was not there when the request was issues so it should not be a problem + */ auto [pathToParent, pathSegment] = asLibyangPathSplit(ctx, requestCtx->req.uri().path); if (!requestCtx->sess.getData(pathToParent)) { throw ErrorResponse(400, "application", "operation-failed", "Action data node '" + requestCtx->restconfRequest.path + "' does not exist."); } } + auto [parent, rpcNode] = ctx.newPath2(requestCtx->restconfRequest.path); if (!requestCtx->payload.empty()) { @@ -219,9 +227,10 @@ void processActionOrRPC(std::shared_ptr requestCtx) /* * FIXME: This happens on invalid input data (e.g., missing mandatory nodes) or missing action data node. * The former (invalid input data) should probably be validated by libyang's parseOp but it only parses. Is there better way? At least somehow extract logs? - * We check on the missing action data node, but it is racy. + * We can check if the action node exists before sending the RPC but that is racy because two sysrepo operations must be done (query + rpc) and operational DS cannot be locked. */ - rejectWithError(requestCtx->sess.getContext(), requestCtx->dataFormat.response, requestCtx->req, requestCtx->res, 400, "application", "operation-failed", "Input data validation failed"); + rejectWithError(requestCtx->sess.getContext(), requestCtx->dataFormat.response, requestCtx->req, requestCtx->res, 400, "application", "operation-failed", + "Validation failed. Invalid input data"s + (isAction ? " or the action node is not present" : "") + "."); } else { rejectWithError(requestCtx->sess.getContext(), requestCtx->dataFormat.response, requestCtx->req, requestCtx->res, 500, "application", "operation-failed", "Internal server error due to sysrepo exception: "s + e.what()); } diff --git a/tests/restconf-rpc.cpp b/tests/restconf-rpc.cpp index 48e86829..0f43aa0a 100644 --- a/tests/restconf-rpc.cpp +++ b/tests/restconf-rpc.cpp @@ -217,7 +217,7 @@ TEST_CASE("invoking actions and rpcs") { "error-type": "application", "error-tag": "operation-failed", - "error-message": "Input data validation failed" + "error-message": "Validation failed. Invalid input data." } ] } @@ -233,7 +233,7 @@ TEST_CASE("invoking actions and rpcs") { "error-type": "application", "error-tag": "operation-failed", - "error-message": "Input data validation failed" + "error-message": "Validation failed. Invalid input data." } ] }