You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nifi.apache.org by sz...@apache.org on 2022/05/13 14:37:19 UTC
[nifi-minifi-cpp] 02/03: MINIFICPP-1749 Validate C2 property update values
This is an automated email from the ASF dual-hosted git repository.
szaszm pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/nifi-minifi-cpp.git
commit c3906edfa830f65738c3562f5f288bff76911dc6
Author: Gabor Gyimesi <ga...@gmail.com>
AuthorDate: Fri May 13 16:32:27 2022 +0200
MINIFICPP-1749 Validate C2 property update values
Added verification of C2 property values while updating them, also
changed the C2 response value in case no value could be updated due to
validation failure or if the update is denied by the
UpdatePolicyController to NOT_APPLIED instead of PARTIALLY_APPLIED. The
PARTIALLY_APPLIED response is still sent if any of the requested updates
could be finished.
Closes #1324
Signed-off-by: Marton Szasz <sz...@apache.org>
---
.../http-curl/tests/C2PropertiesUpdateTests.cpp | 31 +++++++++++++++++++---
.../tests/unit/ConfigurationTests.cpp | 11 ++++++++
libminifi/include/properties/Configuration.h | 11 +++-----
libminifi/src/Configuration.cpp | 9 +++++++
libminifi/src/c2/C2Agent.cpp | 13 +++++----
5 files changed, 58 insertions(+), 17 deletions(-)
diff --git a/extensions/http-curl/tests/C2PropertiesUpdateTests.cpp b/extensions/http-curl/tests/C2PropertiesUpdateTests.cpp
index 27773ddd6..870604bcc 100644
--- a/extensions/http-curl/tests/C2PropertiesUpdateTests.cpp
+++ b/extensions/http-curl/tests/C2PropertiesUpdateTests.cpp
@@ -17,6 +17,8 @@
*/
#undef NDEBUG
+#include <mutex>
+
#include "HTTPIntegrationBase.h"
#include "HTTPHandlers.h"
#include "utils/gsl.h"
@@ -39,6 +41,7 @@ struct PropertyChange {
class C2HeartbeatHandler : public ServerAwareHandler {
public:
bool handlePost(CivetServer* /*server*/, struct mg_connection *conn) override {
+ std::lock_guard<std::mutex> lock(mutex_);
if (response_) {
mg_printf(conn, "HTTP/1.1 200 OK\r\nContent-Type: "
"text/plain\r\nContent-Length: %lu\r\nConnection: close\r\n\r\n",
@@ -61,6 +64,8 @@ class C2HeartbeatHandler : public ServerAwareHandler {
fields.push_back(fmt::format(R"("{}": "{}")", change.name, change.value));
}
}
+
+ std::lock_guard<std::mutex> lock(mutex_);
response_ =
R"({
"operation" : "heartbeat",
@@ -76,6 +81,7 @@ class C2HeartbeatHandler : public ServerAwareHandler {
}
private:
+ std::mutex mutex_;
std::optional<std::string> response_;
};
@@ -158,13 +164,28 @@ int main() {
harness = VerifyPropertyUpdate([&] {
assert(utils::verifyEventHappenedInPollTime(3s, [&] {return ack_handler.isAcknowledged("79");}));
assert(utils::verifyEventHappenedInPollTime(3s, [&] {
- return ack_handler.getApplyCount("FULLY_APPLIED") == 1
- && harness.getRestartRequestedCount() == 1;
+ return ack_handler.getApplyCount("FULLY_APPLIED") == 1;
}));
+
+ // Updating the same property will result in a no operation response
assert(utils::verifyEventHappenedInPollTime(3s, [&] {
- return ack_handler.getApplyCount("NO_OPERATION") > 0
- && harness.getRestartRequestedCount() == 1; // only one, i.e. no additional restart requests compared to the previous update.
+ return ack_handler.getApplyCount("NO_OPERATION") > 0;
}));
+
+ // Change the update response to 1 invalid and 1 valid value update
+ hb_handler.setProperties({{minifi::Configuration::nifi_c2_rest_heartbeat_minimize_updates, "banana", true}, {minifi::Configuration::minifi_disk_space_watchdog_enable, "true", true}});
+
+ // Due to 1 invalid value the result will be partially applied
+ assert(utils::verifyEventHappenedInPollTime(3s, [&] {
+ return ack_handler.getApplyCount("PARTIALLY_APPLIED") == 1;
+ }));
+
+ // Repeating the previous update request results in 1 no operation and 1 failure which results in not applied response
+ assert(utils::verifyEventHappenedInPollTime(3s, [&] {
+ return ack_handler.getApplyCount("NOT_APPLIED") > 0
+ && harness.getRestartRequestedCount() == 2;
+ }));
+
// update operation acknowledged
{
// verify final log levels
@@ -181,6 +202,8 @@ int main() {
assert(!minifi_properties.hasValue("nifi.dummy.property"));
assert(minifi_properties.getValue("nifi.property.one") == "bush");
assert(minifi_properties.getValue("nifi.property.two") == "ring");
+ assert(!minifi_properties.hasValue(minifi::Configuration::nifi_c2_rest_heartbeat_minimize_updates));
+ assert(minifi_properties.getValue(minifi::Configuration::minifi_disk_space_watchdog_enable) == "true");
}
{
diff --git a/extensions/standard-processors/tests/unit/ConfigurationTests.cpp b/extensions/standard-processors/tests/unit/ConfigurationTests.cpp
index eb8d95dc9..d395905c5 100644
--- a/extensions/standard-processors/tests/unit/ConfigurationTests.cpp
+++ b/extensions/standard-processors/tests/unit/ConfigurationTests.cpp
@@ -22,6 +22,8 @@
using org::apache::nifi::minifi::Configuration;
+namespace org::apache::nifi::minifi::test {
+
TEST_CASE("Configuration can merge lists of property names", "[mergeProperties]") {
using vector = std::vector<std::string>;
@@ -45,3 +47,12 @@ TEST_CASE("Configuration can merge lists of property names", "[mergeProperties]"
REQUIRE(Configuration::mergeProperties(vector{"a", "b"}, vector{"a", "c\r\n"}) == (vector{"a", "b", "c"}));
REQUIRE(Configuration::mergeProperties(vector{"a", "b"}, vector{"b\n", "\t c"}) == (vector{"a", "b", "c"}));
}
+
+TEST_CASE("Configuration can validate values to be assigned to specific properties", "[validatePropertyValue]") {
+ REQUIRE(Configuration::validatePropertyValue(Configuration::nifi_server_name, "anything is valid"));
+ REQUIRE_FALSE(Configuration::validatePropertyValue(Configuration::nifi_flow_configuration_encrypt, "invalid.value"));
+ REQUIRE(Configuration::validatePropertyValue(Configuration::nifi_flow_configuration_encrypt, "true"));
+ REQUIRE(Configuration::validatePropertyValue("random.property", "random_value"));
+}
+
+} // namespace org::apache::nifi::minifi::test
diff --git a/libminifi/include/properties/Configuration.h b/libminifi/include/properties/Configuration.h
index f7db69cd4..301bdafd2 100644
--- a/libminifi/include/properties/Configuration.h
+++ b/libminifi/include/properties/Configuration.h
@@ -24,10 +24,7 @@
#include "utils/OptionalUtils.h"
#include "utils/Export.h"
-namespace org {
-namespace apache {
-namespace nifi {
-namespace minifi {
+namespace org::apache::nifi::minifi {
namespace core {
struct ConfigurationProperty;
@@ -162,9 +159,7 @@ class Configuration : public Properties {
static std::vector<std::string> mergeProperties(std::vector<std::string> properties,
const std::vector<std::string>& additional_properties);
static std::vector<std::string> getSensitiveProperties(std::function<std::optional<std::string>(const std::string&)> reader);
+ static bool validatePropertyValue(const std::string& property_name, const std::string& property_value);
};
-} // namespace minifi
-} // namespace nifi
-} // namespace apache
-} // namespace org
+} // namespace org::apache::nifi::minifi
diff --git a/libminifi/src/Configuration.cpp b/libminifi/src/Configuration.cpp
index 4e50b2447..c8bc7e445 100644
--- a/libminifi/src/Configuration.cpp
+++ b/libminifi/src/Configuration.cpp
@@ -155,4 +155,13 @@ std::vector<std::string> Configuration::getSensitiveProperties(std::function<std
return sensitive_properties;
}
+bool Configuration::validatePropertyValue(const std::string& property_name, const std::string& property_value) {
+ for (const auto& config_property: Configuration::CONFIGURATION_PROPERTIES) {
+ if (config_property.name == property_name) {
+ return config_property.validator->validate(property_name, property_value).valid();
+ }
+ }
+ return true;
+}
+
} // namespace org::apache::nifi::minifi
diff --git a/libminifi/src/c2/C2Agent.cpp b/libminifi/src/c2/C2Agent.cpp
index d3c635a1d..3e6fc92a3 100644
--- a/libminifi/src/c2/C2Agent.cpp
+++ b/libminifi/src/c2/C2Agent.cpp
@@ -611,9 +611,10 @@ void C2Agent::handlePropertyUpdate(const C2ContentResponse &resp) {
if (update_result == UpdateResult::UPDATE_SUCCESSFUL) {
result = state::UpdateState::FULLY_APPLIED;
} else if (update_result == UpdateResult::UPDATE_FAILED) {
- result = state::UpdateState::PARTIALLY_APPLIED;
+ result = state::UpdateState::NOT_APPLIED;
}
- } else if (result == state::UpdateState::FULLY_APPLIED && update_result == UpdateResult::UPDATE_FAILED) {
+ } else if ((result == state::UpdateState::FULLY_APPLIED && update_result == UpdateResult::UPDATE_FAILED) ||
+ (result == state::UpdateState::NOT_APPLIED && update_result == UpdateResult::UPDATE_SUCCESSFUL)) {
result = state::UpdateState::PARTIALLY_APPLIED;
}
};
@@ -627,19 +628,21 @@ void C2Agent::handlePropertyUpdate(const C2ContentResponse &resp) {
changeUpdateState(update_property(entry.first, entry.second.to_string(), lifetime));
}
// apply changes and persist properties requested to be persisted
- if (result != state::UpdateState::NO_OPERATION && !configuration_->commitChanges()) {
+ const bool propertyWasUpdated = result == state::UpdateState::FULLY_APPLIED || result == state::UpdateState::PARTIALLY_APPLIED;
+ if (propertyWasUpdated && !configuration_->commitChanges()) {
result = state::UpdateState::PARTIALLY_APPLIED;
}
C2Payload response(Operation::ACKNOWLEDGE, result, resp.ident, true);
enqueue_c2_response(std::move(response));
- if (result != state::UpdateState::NO_OPERATION) { restart_needed_ = true; }
+ if (propertyWasUpdated) { restart_needed_ = true; }
}
/**
* Updates a property
*/
C2Agent::UpdateResult C2Agent::update_property(const std::string &property_name, const std::string &property_value, PropertyChangeLifetime lifetime) {
- if (update_service_ && !update_service_->canUpdate(property_name)) {
+ if (!Configuration::validatePropertyValue(property_name, property_value) ||
+ (update_service_ && !update_service_->canUpdate(property_name))) {
return UpdateResult::UPDATE_FAILED;
}