szaszm commented on code in PR #2069:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2069#discussion_r2598939062
##########
extensions/standard-processors/tests/unit/ConfigurationTests.cpp:
##########
@@ -15,12 +15,27 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+#include <unordered_set>
+
#include "unit/TestBase.h"
#include "unit/Catch.h"
-
#include "properties/Configuration.h"
#include "utils/Environment.h"
+namespace {
+bool fileContentsMatch(const std::filesystem::path& file_name, const
std::unordered_set<std::string>& expected_contents) {
Review Comment:
(thread reply) I think move is valid: the moved-from state is messy in
general, some people say invalid, the standard says unspecified but valid
state. But if it's reassigned (by getfile), then it probably doesn't matter. I
would be a proponent of a destructive move / relocate, which would prevent
reuse like this, but we don't have it yet.
I think it's fine either way.
##########
libminifi/src/properties/Properties.cpp:
##########
@@ -157,8 +158,41 @@ void fixValidatedProperty(const std::string& property_name,
needs_to_persist_new_value = false;
}
}
+
+auto getExtraPropertiesFileNames(const std::filesystem::path&
extra_properties_files_dir, const std::shared_ptr<core::logging::Logger>&
logger) {
+ std::vector<std::filesystem::path> extra_properties_file_names;
+ if (utils::file::exists(extra_properties_files_dir) &&
utils::file::is_directory(extra_properties_files_dir)) {
+ utils::file::list_dir(extra_properties_files_dir, [&](const
std::filesystem::path&, const std::filesystem::path& file_name) {
+ if (!file_name.string().ends_with(".bak")) {
+ extra_properties_file_names.push_back(file_name);
Review Comment:
I've changed my mind about the pattern matching: I think I'd rather have a
whitelist by matching extensions, rather than a blacklist by `*.bak`. It's more
explicit about what we want to read, and it leaves room for things like a
README.txt like in `/etc/sysctl.d` on my system. By the way, another pattern I
was thinking about following was `/etc/X11/xorg.conf.d`
##########
libminifi/src/core/FlowConfiguration.cpp:
##########
@@ -29,6 +29,18 @@
#include "minifi-cpp/SwapManager.h"
#include "Connection.h"
+namespace {
+void createDefaultFlowConfigFile(const std::filesystem::path& path) {
+ std::ofstream ostream(path);
Review Comment:
just a bit of error reporting
```suggestion
std::ofstream ostream(path);
ostream.exceptions(std::ofstream::failbit | std::ofstream::badbit);
```
##########
README.md:
##########
@@ -457,8 +457,22 @@ The performance tests can similarly be enabled. To execute
them and see their ou
$ ctest --verbose -L performance
```
+
### Configuring
-The 'conf' directory in the installation root contains a template config.yml
document, minifi.properties, and minifi-log.properties. Please see our
[Configuration document](CONFIGURE.md) for details on how to configure agents.
+The 'conf' directory in the installation root contains all configuration files.
+
+The files conf/minifi.properties, conf/minifi-log.properties and
conf/minifi-uid.properties contain key-value pair configuration settings;
+these are the default settings supplied by the latest MiNiFi version. If you
would like to modify these, you should create a corresponding
+.d directory (e.g. conf/minifi.properties.d) and put your settings in a new
file inside this directory. These files are read and applied
Review Comment:
I'm thinking it would be more user-friendly if we shipped the directory. One
option could be to ship a symlink in it to minifi.properties like
/etc/sysctl.d. Alternatively we should ship a README or an empty file just to
keep the directory.
##########
libminifi/src/utils/ChecksumCalculator.cpp:
##########
@@ -21,59 +21,66 @@
#include <fstream>
#include "sodium/crypto_hash_sha256.h"
-#include "utils/file/FileUtils.h"
#include "utils/StringUtils.h"
#include "properties/Configuration.h"
namespace {
const std::string AGENT_IDENTIFIER_KEY =
std::string(org::apache::nifi::minifi::Configuration::nifi_c2_agent_identifier)
+ "=";
+namespace utils = org::apache::nifi::minifi::utils;
+
+void addFileToChecksum(const std::filesystem::path& file_path,
crypto_hash_sha256_state& state) {
+ std::ifstream input_file{file_path, std::ios::in | std::ios::binary};
+ if (!input_file.is_open()) {
+ throw std::runtime_error(utils::string::join_pack("Could not open config
file '", file_path.string(), "' to compute the checksum: ",
std::strerror(errno)));
+ }
+
+ std::string line;
+ while (std::getline(input_file, line)) {
+ // skip lines containing the agent identifier, so agents in the same class
will have the same checksum
+ if (line.starts_with(AGENT_IDENTIFIER_KEY)) {
+ continue;
+ }
+ if (!input_file.eof()) { // eof() means we have just read the last line,
which was not terminated by a newline
+ line.append("\n");
+ }
+ crypto_hash_sha256_update(&state, reinterpret_cast<const unsigned
char*>(line.data()), line.size());
+ }
+ if (input_file.bad()) {
+ throw std::runtime_error(utils::string::join_pack("Error reading config
file '", file_path.string(), "' while computing the checksum: ",
std::strerror(errno)));
+ }
+}
+
} // namespace
namespace org::apache::nifi::minifi::utils {
-void ChecksumCalculator::setFileLocation(const std::filesystem::path&
file_location) {
- file_location_ = file_location;
- file_name_ = file_location.filename();
+void ChecksumCalculator::setFileLocations(std::vector<std::filesystem::path>
file_locations) {
+ gsl_Expects(!file_locations.empty());
+ file_locations_ = std::move(file_locations);
invalidateChecksum();
}
std::filesystem::path ChecksumCalculator::getFileName() const {
- gsl_Expects(file_name_);
- return *file_name_;
+ gsl_Expects(!file_locations_.empty());
+ return file_locations_.front().filename();
Review Comment:
Does this function still make sense? If it's using a list of files now, and
exposes a single file name, then I'd rather change it to not expose a single
file name instead of just taking the first one in the list.
##########
packaging/msi/WixWin.wsi.in:
##########
@@ -263,16 +262,16 @@ ${WIX_EXTRA_FEATURES}
</UI>
- <Property Id="AGENT_CLASS" Value="Your Agent Class" />
- <Property Id="AGENT_IDENTIFIER" />
- <Property Id="AGENT_HEARTBEAT" Value="30 sec" />
- <Property Id="SERVER_PATH_BASE" Value="http://localhost:8181/api"; />
- <Property Id="SERVER_HEARTBEAT" Value="/c2-protocol/heartbeat" />
- <Property Id="SERVER_ACK" Value="/c2-protocol/acknowledge" />
- <Property Id="ENABLEC2" />
- <Property Id="AUTOSTART" Value="1" />
- <Property Id="SERVICEACCOUNT" Value="LocalSystem" />
- <Property Id="SERVICEACCOUNTPASSWORD" />
+ <Property Id="AGENT_CLASS" Secure="yes" Value="Your Agent Class" />
+ <Property Id="AGENT_IDENTIFIER" Secure="yes" />
+ <Property Id="AGENT_HEARTBEAT" Secure="yes" Value="30 sec" />
+ <Property Id="SERVER_PATH_BASE" Secure="yes"
Value="http://localhost:8181/api"; />
+ <Property Id="SERVER_HEARTBEAT" Secure="yes"
Value="/c2-protocol/heartbeat" />
+ <Property Id="SERVER_ACK" Secure="yes" Value="/c2-protocol/acknowledge" />
+ <Property Id="ENABLEC2" Secure="yes" />
+ <Property Id="AUTOSTART" Secure="yes" Value="1" />
+ <Property Id="SERVICEACCOUNT" Secure="yes" Value="LocalSystem" />
+ <Property Id="SERVICEACCOUNTPASSWORD" Secure="yes" />
Review Comment:
what does this do? I found some docs saying "Denotes that the Property can
be passed to the server side when doing a managed installation with elevated
privileges.", but I don't understand what the server side is and what exactly a
managed installation means.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
