From 7e25ebb91ccfabc50869b8f5c85541e002b107af Mon Sep 17 00:00:00 2001
From: Mauro Ezequiel Moltrasio <mmoltras@redhat.com>
Date: Mon, 3 Feb 2025 17:23:52 +0100
Subject: [PATCH 01/10] Use protobuf reflection for loading runtime
 configuration

This change is based upon a toy project I have at
https://github.com/Molter73/config-much.

The basic idea is to use the reflection API from protobuf to populate
the runtime configuration structures, which should allow us to add new
fields to our protobuf definitions and have them magically show up in
our code to use.
---
 collector/collector.cpp                |   2 +-
 collector/lib/ConfigLoader.cpp         | 525 +++++++++++++++++++++----
 collector/lib/ConfigLoader.h           |  99 ++++-
 collector/test/CMakeLists.txt          |   8 +
 collector/test/ConfigLoaderTest.cpp    | 250 +++++++++++-
 collector/test/proto/test-config.proto |  26 ++
 6 files changed, 805 insertions(+), 105 deletions(-)
 create mode 100644 collector/test/proto/test-config.proto

diff --git a/collector/collector.cpp b/collector/collector.cpp
index a4425b44e7..af1b828e72 100644
--- a/collector/collector.cpp
+++ b/collector/collector.cpp
@@ -140,7 +140,7 @@ int main(int argc, char** argv) {
 
   CollectorConfig config;
   config.InitCollectorConfig(args);
-  if (!ConfigLoader::LoadConfiguration(config)) {
+  if (!ConfigLoader(config).LoadConfiguration()) {
     CLOG(FATAL) << "Unable to parse configuration file";
   }
 
diff --git a/collector/lib/ConfigLoader.cpp b/collector/lib/ConfigLoader.cpp
index 797fa2a8a8..1f433f46aa 100644
--- a/collector/lib/ConfigLoader.cpp
+++ b/collector/lib/ConfigLoader.cpp
@@ -1,5 +1,9 @@
 #include "ConfigLoader.h"
 
+#include <google/protobuf/descriptor.h>
+
+#include "internalapi/sensor/collector.pb.h"
+
 #include "EnvVar.h"
 #include "Logging.h"
 
@@ -13,109 +17,485 @@ enum PathTags {
   LOADER_CONFIG_FILE,
   LOADER_CONFIG_REALPATH,
 };
-}  // namespace
+
+std::string NodeTypeToString(YAML::NodeType::value type) {
+  // Don't add the default case so linters can warn about a missing type
+  switch (type) {
+    case YAML::NodeType::Null:
+      return "Null";
+    case YAML::NodeType::Undefined:
+      return "Undefined";
+    case YAML::NodeType::Scalar:
+      return "Scalar";
+    case YAML::NodeType::Sequence:
+      return "Sequence";
+    case YAML::NodeType::Map:
+      return "Map";
+  }
+  return "";  // Unreachable
+}
+};  // namespace
 
 namespace stdf = std::filesystem;
 
-ConfigLoader::ConfigLoader(CollectorConfig& config)
-    : config_(config), file_(CONFIG_FILE.value()) {}
+ParserResult ParserYaml::Parse(google::protobuf::Message* msg) {
+  YAML::Node node = YAML::LoadFile(file_);
+  return Parse(msg, node);
+}
 
-void ConfigLoader::Start() {
-  thread_.Start([this] { WatchFile(); });
-  CLOG(INFO) << "Watching configuration file: " << file_;
+ParserResult ParserYaml::Parse(google::protobuf::Message* msg, const YAML::Node& node) {
+  using namespace google::protobuf;
+
+  if (node.IsScalar() || node.IsNull()) {
+    return {{"Invalid configuration"}};
+  }
+
+  ParserErrors errors;
+
+  const Descriptor* descriptor = msg->GetDescriptor();
+  for (int i = 0; i < descriptor->field_count(); i++) {
+    const FieldDescriptor* field = descriptor->field(i);
+
+    auto err = Parse(msg, node, field);
+    if (err) {
+      errors.insert(errors.end(), err->begin(), err->end());
+    }
+  }
+
+  auto res = FindUnkownFields(*msg, node);
+  if (res) {
+    errors.insert(errors.end(), res->begin(), res->end());
+  }
+
+  if (!errors.empty()) {
+    return errors;
+  }
+
+  return {};
 }
 
-void ConfigLoader::Stop() {
-  thread_.Stop();
-  CLOG(INFO) << "No longer watching configuration file: " << file_;
+template <typename T>
+ParserResult ParserYaml::ParseArrayInner(google::protobuf::Message* msg, const YAML::Node& node,
+                                         const google::protobuf::FieldDescriptor* field) {
+  ParserErrors errors;
+  auto f = msg->GetReflection()->GetMutableRepeatedFieldRef<T>(msg, field);
+  f.Clear();
+  for (const auto& n : node) {
+    auto value = TryConvert<T>(n);
+    if (!IsError(value)) {
+      f.Add(std::get<T>(value));
+    } else {
+      errors.emplace_back(std::get<ParserError>(value));
+    }
+  }
+
+  if (!errors.empty()) {
+    return errors;
+  }
+  return {};
 }
 
-bool ConfigLoader::LoadConfiguration(CollectorConfig& config) {
-  const auto& config_file = CONFIG_FILE.value();
-  YAML::Node node;
+ParserResult ParserYaml::ParseArrayEnum(google::protobuf::Message* msg, const YAML::Node& node,
+                                        const google::protobuf::FieldDescriptor* field) {
+  using namespace google::protobuf;
 
-  if (!stdf::exists(config_file)) {
-    CLOG(DEBUG) << "No configuration file found: " << config_file;
-    return true;
+  std::unique_ptr<std::string> name_ptr = nullptr;
+  const std::string* name = &field->name();
+
+  if (read_camelcase_) {
+    name_ptr = std::make_unique<std::string>(SnakeCaseToCamel(*name));
+    name = name_ptr.get();
   }
 
-  try {
-    node = YAML::LoadFile(config_file);
-  } catch (const YAML::BadFile& e) {
-    CLOG(ERROR) << "Failed to open the configuration file: " << config_file << ". Error: " << e.what();
-    return false;
-  } catch (const YAML::ParserException& e) {
-    CLOG(ERROR) << "Failed to parse the configuration file: " << config_file << ". Error: " << e.what();
-    return false;
+  ParserErrors errors;
+  auto f = msg->GetReflection()->GetMutableRepeatedFieldRef<int32>(msg, field);
+  f.Clear();
+
+  const EnumDescriptor* desc = field->enum_type();
+  for (const auto& n : node) {
+    auto v = TryConvert<std::string_view>(n);
+    if (IsError(v)) {
+      errors.emplace_back(std::get<ParserError>(v));
+      continue;
+    }
+    const auto enum_name = std::get<std::string_view>(v);
+
+    const EnumValueDescriptor* value = desc->FindValueByName(enum_name);
+    if (value == nullptr) {
+      ParserError err;
+      err << file_ << ": Invalid enum value '" << enum_name << "' for field " << *name;
+      errors.emplace_back(err);
+      continue;
+    }
+
+    f.Add(value->number());
   }
 
-  return LoadConfiguration(config, node);
+  if (!errors.empty()) {
+    return errors;
+  }
+  return {};
 }
 
-bool ConfigLoader::LoadConfiguration(CollectorConfig& config, const YAML::Node& node) {
-  const auto& config_file = CONFIG_FILE.value();
+ParserResult ParserYaml::FindUnkownFields(const google::protobuf::Message& msg, const YAML::Node& node) {
+  using namespace google::protobuf;
 
-  if (node.IsNull() || !node.IsDefined() || !node.IsMap()) {
-    CLOG(ERROR) << "Unable to read config from " << config_file;
-    return false;
+  const auto* descriptor = msg.GetDescriptor();
+  ParserErrors errors;
+
+  for (YAML::const_iterator it = node.begin(); it != node.end(); it++) {
+    auto name = it->first.as<std::string>();
+    if (read_camelcase_) {
+      name = CamelCaseToSnake(name);
+    }
+
+    const FieldDescriptor* field = descriptor->FindFieldByName(name);
+    if (field == nullptr) {
+      std::stringstream ss;
+      ss << "Unknown field '" << name << "'";
+      errors.emplace_back(ss.str());
+      continue;
+    }
+
+    if (it->second.IsMap()) {
+      if (field->type() != FieldDescriptor::TYPE_MESSAGE) {
+        std::stringstream ss;
+        ss << file_ << ": Invalid type '" << NodeTypeToString(it->second.Type()) << "' for field " << it->first.as<std::string_view>() << ", expected '" << field->type_name() << "'";
+        errors.emplace_back(ss.str());
+        continue;
+      }
+
+      const auto* reflection = msg.GetReflection();
+      auto res = FindUnkownFields(reflection->GetMessage(msg, field), it->second);
+
+      if (res) {
+        errors.insert(errors.end(), res->begin(), res->end());
+      }
+    }
   }
 
-  YAML::Node networking_node = node["networking"];
-  if (!networking_node || networking_node.IsNull()) {
-    CLOG(DEBUG) << "No networking in " << config_file;
-    return true;
+  if (!errors.empty()) {
+    return errors;
   }
+  return {};
+}
 
-  YAML::Node external_ips_node = networking_node["externalIps"];
-  if (!external_ips_node) {
-    CLOG(DEBUG) << "No external IPs in " << config_file;
-    return true;
+ParserResult ParserYaml::Parse(google::protobuf::Message* msg, const YAML::Node& node,
+                               const google::protobuf::FieldDescriptor* field) {
+  using namespace google::protobuf;
+
+  std::unique_ptr<std::string> name_ptr = nullptr;
+  const std::string* name = &field->name();
+  if (read_camelcase_) {
+    name_ptr = std::make_unique<std::string>(SnakeCaseToCamel(*name));
+    name = name_ptr.get();
+  }
+
+  if (!node[*name]) {
+    return {};
+  }
+
+  if (field->label() == FieldDescriptor::LABEL_REPEATED) {
+    if (!node[*name].IsSequence()) {
+      ParserError err;
+      YAML::NodeType::value type = node[*name].Type();
+      err << file_ << ": Type mismatch for '" << *name << "' - expected Sequence, got "
+          << NodeTypeToString(type);
+      return {{err}};
+    }
+    return ParseArray(msg, node[*name], field);
+  }
+
+  if (field->type() == FieldDescriptor::TYPE_MESSAGE) {
+    if (node[*name].IsNull()) {
+      // Ignore empty objects
+      return {};
+    }
+
+    if (!node[*name].IsMap()) {
+      ParserError err;
+      YAML::NodeType::value type = node[*name].Type();
+      err << file_ << ": Type mismatch for '" << *name << "' - expected Map, got "
+          << NodeTypeToString(type);
+      return {{err}};
+    }
+    ParserErrors errors;
+    const Reflection* reflection = msg->GetReflection();
+
+    Message* m = reflection->MutableMessage(msg, field);
+
+    const Descriptor* descriptor = m->GetDescriptor();
+    for (int i = 0; i < descriptor->field_count(); i++) {
+      const FieldDescriptor* f = descriptor->field(i);
+
+      auto err = Parse(m, node[*name], f);
+      if (err) {
+        errors.insert(errors.end(), err->begin(), err->end());
+      }
+    }
+
+    if (!errors.empty()) {
+      return errors;
+    }
+    return {};
+  }
+
+  if (!node[*name].IsScalar()) {
+    ParserError err;
+    err << file_ << ": Attempting to parse non-scalar field as scalar";
+    return {{err}};
+  }
+
+  switch (field->type()) {
+    case FieldDescriptor::TYPE_DOUBLE: {
+      auto value = TryConvert<double>(node[*name]);
+      if (IsError(value)) {
+        return {{std::get<ParserError>(value)}};
+      }
+
+      msg->GetReflection()->SetDouble(msg, field, std::get<double>(value));
+    } break;
+    case FieldDescriptor::TYPE_FLOAT: {
+      auto value = TryConvert<float>(node[*name]);
+      if (IsError(value)) {
+        return {{std::get<ParserError>(value)}};
+      }
+
+      msg->GetReflection()->SetFloat(msg, field, std::get<float>(value));
+    } break;
+    case FieldDescriptor::TYPE_SFIXED64:
+    case FieldDescriptor::TYPE_INT64: {
+      auto value = TryConvert<int64_t>(node[*name]);
+      if (IsError(value)) {
+        return {{std::get<ParserError>(value)}};
+      }
+
+      msg->GetReflection()->SetInt64(msg, field, std::get<int64_t>(value));
+    } break;
+    case FieldDescriptor::TYPE_SINT64:
+    case FieldDescriptor::TYPE_FIXED64:
+    case FieldDescriptor::TYPE_UINT64: {
+      auto value = TryConvert<uint64_t>(node[*name]);
+      if (IsError(value)) {
+        return {{std::get<ParserError>(value)}};
+      }
+
+      msg->GetReflection()->SetUInt64(msg, field, std::get<uint64_t>(value));
+    } break;
+    case FieldDescriptor::TYPE_FIXED32:
+    case FieldDescriptor::TYPE_UINT32: {
+      auto value = TryConvert<uint32_t>(node[*name]);
+      if (IsError(value)) {
+        return {{std::get<ParserError>(value)}};
+      }
+
+      msg->GetReflection()->SetUInt32(msg, field, std::get<uint32_t>(value));
+    } break;
+    case FieldDescriptor::TYPE_SINT32:
+    case FieldDescriptor::TYPE_SFIXED32:
+    case FieldDescriptor::TYPE_INT32: {
+      auto value = TryConvert<int32_t>(node[*name]);
+      if (IsError(value)) {
+        return {{std::get<ParserError>(value)}};
+      }
+
+      msg->GetReflection()->SetInt32(msg, field, std::get<int32_t>(value));
+    } break;
+    case FieldDescriptor::TYPE_BOOL: {
+      auto value = TryConvert<bool>(node[*name]);
+      if (IsError(value)) {
+        return {{std::get<ParserError>(value)}};
+      }
+
+      msg->GetReflection()->SetBool(msg, field, std::get<bool>(value));
+
+    } break;
+    case FieldDescriptor::TYPE_STRING: {
+      auto value = TryConvert<std::string>(node[*name]);
+      if (IsError(value)) {
+        return {{std::get<ParserError>(value)}};
+      }
+
+      msg->GetReflection()->SetString(msg, field, std::get<std::string>(value));
+
+    } break;
+    case FieldDescriptor::TYPE_BYTES:
+      std::cerr << "Unsupported type BYTES" << std::endl;
+      break;
+    case FieldDescriptor::TYPE_ENUM: {
+      const auto enum_name = node[*name].as<std::string_view>();
+
+      const EnumDescriptor* descriptor = field->enum_type();
+      const EnumValueDescriptor* value = descriptor->FindValueByName(enum_name);
+      if (value == nullptr) {
+        ParserError err;
+        err << file_ << ": Invalid enum value '" << enum_name << "' for field " << *name;
+        return {{err}};
+      }
+      msg->GetReflection()->SetEnumValue(msg, field, value->number());
+    } break;
+
+    case FieldDescriptor::TYPE_MESSAGE:
+    case FieldDescriptor::TYPE_GROUP: {
+      ParserError err;
+      err << "Unexpected type: " << field->type_name();
+      return {{err}};
+    }
+  }
+
+  return {};
+}
+
+ParserResult ParserYaml::ParseArray(google::protobuf::Message* msg, const YAML::Node& node,
+                                    const google::protobuf::FieldDescriptor* field) {
+  using namespace google::protobuf;
+
+  // mapping for repeated fields:
+  // https://protobuf.dev/reference/cpp/api-docs/google.protobuf.message/#Reflection.GetRepeatedFieldRef.details
+  switch (field->cpp_type()) {
+    case FieldDescriptor::CPPTYPE_INT32:
+      return ParseArrayInner<int32>(msg, node, field);
+    case FieldDescriptor::CPPTYPE_UINT32:
+      return ParseArrayInner<uint32_t>(msg, node, field);
+    case google::protobuf::FieldDescriptor::CPPTYPE_INT64:
+      return ParseArrayInner<int64_t>(msg, node, field);
+    case google::protobuf::FieldDescriptor::CPPTYPE_UINT64:
+      return ParseArrayInner<uint64_t>(msg, node, field);
+    case google::protobuf::FieldDescriptor::CPPTYPE_DOUBLE:
+      return ParseArrayInner<double>(msg, node, field);
+    case google::protobuf::FieldDescriptor::CPPTYPE_FLOAT:
+      return ParseArrayInner<float>(msg, node, field);
+    case google::protobuf::FieldDescriptor::CPPTYPE_BOOL:
+      return ParseArrayInner<bool>(msg, node, field);
+    case google::protobuf::FieldDescriptor::CPPTYPE_ENUM:
+      return ParseArrayEnum(msg, node, field);
+    case google::protobuf::FieldDescriptor::CPPTYPE_STRING:
+      return ParseArrayInner<std::string>(msg, node, field);
+    case google::protobuf::FieldDescriptor::CPPTYPE_MESSAGE: {
+      return {{"Unsupport repeated type MESSAGE"}};
+    } break;
+    default: {
+      std::stringstream ss;
+      ss << "Unknown type " << field->type_name();
+      return {{ss.str()}};
+    }
   }
+}
 
-  sensor::ExternalIpsEnabled enable_external_ips;
-  std::string enabled_value = external_ips_node["enabled"] ? external_ips_node["enabled"].as<std::string>() : "";
-  std::transform(enabled_value.begin(), enabled_value.end(), enabled_value.begin(), ::tolower);
+ParserError ParserYaml::WrapError(const std::exception& e) {
+  std::stringstream ss;
+  ss << file_ << ": " << e.what();
+  return ss.str();
+}
 
-  if (enabled_value == "enabled") {
-    enable_external_ips = sensor::ExternalIpsEnabled::ENABLED;
-  } else if (enabled_value == "disabled") {
-    enable_external_ips = sensor::ExternalIpsEnabled::DISABLED;
-  } else {
-    CLOG(WARNING) << "Unknown value for for networking.externalIps.enabled. Setting it to DISABLED";
-    enable_external_ips = sensor::ExternalIpsEnabled::DISABLED;
+template <typename T>
+std::variant<T, ParserError> ParserYaml::TryConvert(const YAML::Node& node) {
+  try {
+    return node.as<T>();
+  } catch (YAML::InvalidNode& e) {
+    return WrapError(e);
+  } catch (YAML::BadConversion& e) {
+    return WrapError(e);
   }
-  int64_t max_connections_per_minute = networking_node["maxConnectionsPerMinute"].as<int64_t>(CollectorConfig::kMaxConnectionsPerMinute);
+}
+
+std::string ParserYaml::SnakeCaseToCamel(const std::string& s) {
+  std::string out;
+  bool capitalize = false;
+
+  for (const auto& c : s) {
+    if (c == '_') {
+      capitalize = true;
+      continue;
+    }
 
+    if (capitalize) {
+      out += (char)std::toupper(c);
+    } else {
+      out += c;
+    }
+    capitalize = false;
+  }
+
+  return out;
+}
+
+std::string ParserYaml::CamelCaseToSnake(const std::string& s) {
+  std::string out;
+  bool first = true;
+
+  for (const auto& c : s) {
+    if (!first && std::isupper(c) != 0) {
+      out += '_';
+    }
+    out += (char)std::tolower(c);
+    first = false;
+  }
+
+  return out;
+}
+
+ConfigLoader::ConfigLoader(CollectorConfig& config)
+    : config_(config), parser_(CONFIG_FILE.value()) {}
+
+void ConfigLoader::Start() {
+  thread_.Start([this] { WatchFile(); });
+  CLOG(INFO) << "Watching configuration file: " << parser_.GetFile().string();
+}
+
+void ConfigLoader::Stop() {
+  thread_.Stop();
+  CLOG(INFO) << "No longer watching configuration file: " << parser_.GetFile().string();
+}
+
+bool ConfigLoader::LoadConfiguration() {
   sensor::CollectorConfig runtime_config;
-  auto* networking = runtime_config.mutable_networking();
-  networking
-      ->mutable_external_ips()
-      ->set_enabled(enable_external_ips);
-  networking
-      ->set_max_connections_per_minute(max_connections_per_minute);
+  auto errors = parser_.Parse(&runtime_config);
 
-  config.SetRuntimeConfig(std::move(runtime_config));
+  if (errors) {
+    CLOG(ERROR) << "Failed to parse " << parser_.GetFile();
+    for (const auto& err : *errors) {
+      CLOG(ERROR) << err;
+    }
+    return false;
+  }
 
-  CLOG(INFO) << "Runtime configuration:\n"
-             << config.GetRuntimeConfigStr();
+  config_.SetRuntimeConfig(std::move(runtime_config));
+  return true;
+}
+
+bool ConfigLoader::LoadConfiguration(const YAML::Node& node) {
+  sensor::CollectorConfig runtime_config;
+  auto errors = parser_.Parse(&runtime_config, node);
+  if (errors) {
+    CLOG(ERROR) << "Failed to parse " << parser_.GetFile();
+    for (const auto& err : *errors) {
+      CLOG(ERROR) << err;
+    }
+    return false;
+  }
+
+  config_.SetRuntimeConfig(std::move(runtime_config));
   return true;
 }
 
 void ConfigLoader::WatchFile() {
+  const auto& file = parser_.GetFile();
+
   if (!inotify_.IsValid()) {
-    CLOG(ERROR) << "Configuration reloading will not be used for " << file_;
+    CLOG(ERROR) << "Configuration reloading will not be used for " << file;
     return;
   }
 
-  if (inotify_.AddDirectoryWatcher(file_.parent_path(), LOADER_PARENT_PATH) < 0) {
+  if (inotify_.AddDirectoryWatcher(file.parent_path(), LOADER_PARENT_PATH) < 0) {
     return;
   }
 
-  if (stdf::exists(file_)) {
-    inotify_.AddFileWatcher(file_, LOADER_CONFIG_FILE);
+  if (stdf::exists(file)) {
+    inotify_.AddFileWatcher(file, LOADER_CONFIG_FILE);
 
-    if (stdf::is_symlink(file_)) {
-      inotify_.AddFileWatcher(stdf::canonical(file_), LOADER_CONFIG_REALPATH);
+    if (stdf::is_symlink(file)) {
+      inotify_.AddFileWatcher(stdf::canonical(file), LOADER_CONFIG_REALPATH);
     }
 
     // Reload configuration in case it has changed since startup
@@ -170,25 +550,27 @@ bool ConfigLoader::HandleEvent(const struct inotify_event* event) {
 }
 
 bool ConfigLoader::HandleConfigDirectoryEvent(const struct inotify_event* event) {
-  CLOG(DEBUG) << "Got directory event for " << file_.parent_path() / event->name << " - mask: [ " << Inotify::MaskToString(event->mask) << " ]";
+  const auto& file = parser_.GetFile();
+
+  CLOG(DEBUG) << "Got directory event for " << file.parent_path() / event->name << " - mask: [ " << Inotify::MaskToString(event->mask) << " ]";
 
   if ((event->mask & (IN_MOVE_SELF | IN_DELETE_SELF)) != 0) {
     CLOG(ERROR) << "Configuration directory was removed or renamed. Stopping runtime configuration";
     return false;
   }
 
-  if (file_.filename() != event->name) {
+  if (file.filename() != event->name) {
     return true;
   }
 
   if ((event->mask & (IN_CREATE | IN_MOVED_TO)) != 0) {
-    inotify_.AddFileWatcher(file_, LOADER_CONFIG_FILE);
+    inotify_.AddFileWatcher(file, LOADER_CONFIG_FILE);
     LoadConfiguration();
-    if (stdf::is_symlink(file_)) {
-      inotify_.AddFileWatcher(stdf::canonical(file_), LOADER_CONFIG_REALPATH);
+    if (stdf::is_symlink(file)) {
+      inotify_.AddFileWatcher(stdf::canonical(file), LOADER_CONFIG_REALPATH);
     }
   } else if ((event->mask & (IN_DELETE | IN_MOVED_FROM)) != 0) {
-    auto w = inotify_.FindWatcher(file_);
+    auto w = inotify_.FindWatcher(file);
     inotify_.RemoveWatcher(w);
     config_.ResetRuntimeConfig();
   }
@@ -227,11 +609,12 @@ void ConfigLoader::HandleConfigRealpathEvent(const struct inotify_event* event,
   if ((event->mask & IN_MODIFY) != 0) {
     LoadConfiguration();
   } else if ((event->mask & (IN_DELETE_SELF | IN_MOVE_SELF)) != 0) {
+    const auto& file = parser_.GetFile();
     // If the original file was a symlink pointing to this file and
     // it still exists, we need to add a new watcher to the newly
     // pointed configuration file and reload the configuration.
-    if (stdf::is_symlink(file_)) {
-      inotify_.AddFileWatcher(stdf::canonical(file_), LOADER_CONFIG_REALPATH);
+    if (stdf::is_symlink(file)) {
+      inotify_.AddFileWatcher(stdf::canonical(file), LOADER_CONFIG_REALPATH);
       LoadConfiguration();
     } else {
       inotify_.RemoveWatcher(w);
diff --git a/collector/lib/ConfigLoader.h b/collector/lib/ConfigLoader.h
index df04320495..0f3b52a2bb 100644
--- a/collector/lib/ConfigLoader.h
+++ b/collector/lib/ConfigLoader.h
@@ -1,6 +1,7 @@
 #ifndef _CONFIG_LOADER_H_
 #define _CONFIG_LOADER_H_
 
+#include <gtest/gtest_prod.h>
 #include <yaml-cpp/yaml.h>
 
 #include "CollectorConfig.h"
@@ -9,6 +10,78 @@
 
 namespace collector {
 
+class ParserError {
+ public:
+  ParserError() = default;
+  ParserError(ParserError&&) noexcept = default;
+  ParserError(const ParserError&) = default;
+  ParserError& operator=(const ParserError&) = default;
+  ParserError& operator=(ParserError&&) noexcept = default;
+  ~ParserError() = default;
+
+  ParserError(const char* msg) { msg_ += msg; }
+  ParserError(const std::string& msg) { msg_ += msg; }
+
+  const std::string& What() const { return msg_; }
+
+  template <typename T>
+  friend ParserError& operator<<(ParserError& e, const T msg) {
+    std::stringstream ss;
+    ss << msg;
+    e.msg_ += ss.str();
+    return e;
+  }
+
+  friend std::ostream& operator<<(std::ostream& os, const ParserError& err) {
+    os << err.What();
+    return os;
+  }
+
+  friend bool operator==(const ParserError& lhs, const ParserError& rhs) { return lhs.msg_ == rhs.msg_; }
+
+ private:
+  std::string msg_;
+};
+
+using ParserErrors = std::vector<ParserError>;
+using ParserResult = std::optional<ParserErrors>;
+
+class ParserYaml {
+ public:
+  ParserYaml(std::filesystem::path file, bool read_camelcase = true) : file_(std::move(file)), read_camelcase_(read_camelcase) {}
+
+  ParserResult Parse(google::protobuf::Message* msg);
+  ParserResult Parse(google::protobuf::Message* msg, const YAML::Node& node);
+
+  const std::filesystem::path& GetFile() { return file_; }
+
+ private:
+  ParserResult Parse(google::protobuf::Message* msg, const YAML::Node& node,
+                     const google::protobuf::FieldDescriptor* field);
+  ParserResult ParseArray(google::protobuf::Message* msg, const YAML::Node& node,
+                          const google::protobuf::FieldDescriptor* field);
+  template <typename T>
+  ParserResult ParseArrayInner(google::protobuf::Message* msg, const YAML::Node& node,
+                               const google::protobuf::FieldDescriptor* field);
+  ParserResult ParseArrayEnum(google::protobuf::Message* msg, const YAML::Node& node,
+                              const google::protobuf::FieldDescriptor* field);
+  ParserResult FindUnkownFields(const google::protobuf::Message& msg, const YAML::Node& node);
+
+  ParserError WrapError(const std::exception& e);
+
+  template <typename T>
+  std::variant<T, ParserError> TryConvert(const YAML::Node& node);
+  template <typename T>
+  static bool IsError(const std::variant<T, ParserError>& res) {
+    return std::holds_alternative<ParserError>(res);
+  }
+  static std::string SnakeCaseToCamel(const std::string& s);
+  static std::string CamelCaseToSnake(const std::string& s);
+
+  std::filesystem::path file_;
+  bool read_camelcase_;
+};
+
 /**
  * Reload configuration based on inotify events received on a
  * configuration file.
@@ -24,22 +97,26 @@ class ConfigLoader {
   /**
    * Load a configuration file into the supplied CollectorConfig object.
    *
-   * @param config The target object for the loaded configuration.
    * @returns true if configuration loading was successful.
    */
-  static bool LoadConfiguration(CollectorConfig& config);
+  bool LoadConfiguration();
+
+ private:
+  FRIEND_TEST(CollectorConfigTest, TestYamlConfigToConfigMultiple);
+  FRIEND_TEST(CollectorConfigTest, TestYamlConfigToConfigInvalid);
+  FRIEND_TEST(CollectorConfigTest, TestYamlConfigToConfigEmptyOrMalformed);
+  FRIEND_TEST(CollectorConfigTest, TestMaxConnectionsPerMinute);
 
   /**
-   * Load a configuration file into the supplied CollectorConfig object
-   * from the provided yaml node.
+   * Load configuration from a YAML string.
+   *
+   * This method is meant to be used for testing only.
    *
-   * @param config The target object for the loaded configuration.
-   * @param node a YAML::Node holding the new configuration.
+   * @param node a YAML::Node with the new configuration to be used.
    * @returns true if configuration loading was successful.
    */
-  static bool LoadConfiguration(CollectorConfig& config, const YAML::Node& node);
+  bool LoadConfiguration(const YAML::Node& node);
 
- private:
   /**
    * Wait for inotify events on a configuration file and reload it
    * accordingly.
@@ -87,14 +164,10 @@ class ConfigLoader {
    */
   void HandleConfigRealpathEvent(const struct inotify_event* event, Inotify::WatcherIterator w);
 
-  bool LoadConfiguration() {
-    return LoadConfiguration(config_);
-  }
-
   CollectorConfig& config_;
   Inotify inotify_;
   StoppableThread thread_;
-  const std::filesystem::path& file_;
+  ParserYaml parser_;
 };
 
 }  // namespace collector
diff --git a/collector/test/CMakeLists.txt b/collector/test/CMakeLists.txt
index 3f8dc250ea..47c70a73ec 100644
--- a/collector/test/CMakeLists.txt
+++ b/collector/test/CMakeLists.txt
@@ -3,6 +3,8 @@
 find_package(GTest CONFIG REQUIRED)
 enable_testing()
 
+set(CMAKE_INCLUDE_CURRENT_DIR ON)
+
 # Unit Tests
 file(GLOB TEST_SRC_FILES ${PROJECT_SOURCE_DIR}/test/*.cpp)
 foreach(test_file ${TEST_SRC_FILES})
@@ -12,6 +14,12 @@ foreach(test_file ${TEST_SRC_FILES})
     target_link_libraries(${test_name} collector_lib)
     target_link_libraries(${test_name} GTest::gtest GTest::gtest_main GTest::gmock GTest::gmock_main)
 
+    if(${test_name} STREQUAL "ConfigLoaderTest")
+        target_sources(${test_name} PRIVATE proto/test-config.proto)
+        target_include_directories(${test_name} PRIVATE proto)
+        protobuf_generate(TARGET ${test_name})
+    endif()
+
     add_test(${test_name} ${test_name})
 
     if(USE_VALGRIND)
diff --git a/collector/test/ConfigLoaderTest.cpp b/collector/test/ConfigLoaderTest.cpp
index 66af0df56f..f9413a0d28 100644
--- a/collector/test/ConfigLoaderTest.cpp
+++ b/collector/test/ConfigLoaderTest.cpp
@@ -1,8 +1,207 @@
+#include <sstream>
+
 #include <gtest/gtest.h>
 
+#include <google/protobuf/util/message_differencer.h>
+
+#include "internalapi/sensor/collector.pb.h"
+
 #include "ConfigLoader.h"
+#include "proto/test-config.pb.h"
 
 namespace collector {
+using namespace google::protobuf::util;
+
+std::string ErrorsToString(const ParserErrors& errors) {
+  std::stringstream ss;
+  for (size_t i = 0; i < errors.size(); i++) {
+    ss << i << ": " << errors.at(i).What() << std::endl;
+  }
+  return ss.str();
+}
+
+/*
+ * Generic yaml parser tests
+ */
+TEST(TestParserYaml, Parsing) {
+  struct TestCase {
+    std::string input;
+    test_config::Config expected;
+  };
+
+  test_config::Config all_fields;
+  all_fields.set_enabled(true);
+  all_fields.set_field_i32(-32);
+  all_fields.set_field_u32(32);
+  all_fields.set_field_i64(-64);
+  all_fields.set_field_u64(64);
+  all_fields.set_field_double(3.14);
+  all_fields.set_field_float(0.12345);
+  all_fields.set_field_string("Yes, this is some random string for testing");
+  all_fields.mutable_field_message()->set_enabled(true);
+  all_fields.mutable_field_repeated()->Add(1);
+  all_fields.mutable_field_repeated()->Add(2);
+  all_fields.mutable_field_repeated()->Add(3);
+  all_fields.set_field_enum(test_config::EnumField::TYPE2);
+  all_fields.mutable_field_repeated_enum()->Add(0);
+  all_fields.mutable_field_repeated_enum()->Add(1);
+
+  std::vector<TestCase> tests = {
+      {R"()", {}},
+      {R"(
+            enabled: true
+            fieldI32: -32
+            fieldU32: 32
+            fieldI64: -64
+            fieldU64: 64
+            fieldDouble: 3.14
+            fieldFloat: 0.12345
+            fieldString: Yes, this is some random string for testing
+            fieldMessage:
+                enabled: true
+            fieldRepeated:
+                - 1
+                - 2
+                - 3
+            fieldEnum: TYPE2
+            fieldRepeatedEnum:
+                - TYPE1
+                - TYPE2
+        )",
+       all_fields},
+  };
+
+  for (const auto& [input, expected] : tests) {
+    test_config::Config parsed;
+    ParserYaml parser("/test.yml");
+    parser.Parse(&parsed, YAML::Load(input));
+
+    bool equals = MessageDifferencer::Equals(parsed, expected);
+
+    ASSERT_TRUE(equals) << "### parsed: " << std::endl
+                        << parsed.DebugString() << std::endl
+                        << "### expected: " << std::endl
+                        << expected.DebugString();
+  }
+}
+
+TEST(TestParserYaml, OverwrittingFields) {
+  test_config::Config cfg;
+  std::string input = R"(
+        enabled: false
+        fieldI32: -1234
+        fieldU32: 4321
+        fieldRepeated:
+            - 15
+        fieldEnum: TYPE1
+        fieldRepeatedEnum:
+            - TYPE1
+    )";
+
+  test_config::Config expected;
+  expected.set_enabled(false);
+  expected.set_field_i32(-1234);
+  expected.set_field_u32(4321);
+  expected.mutable_field_repeated()->Add(15);
+  expected.set_field_enum(test_config::EnumField::TYPE1);
+  expected.mutable_field_repeated_enum()->Add(0);
+  ParserYaml parser("/test.yml");
+  parser.Parse(&cfg, YAML::Load(input));
+
+  bool equals = MessageDifferencer::Equals(cfg, expected);
+  ASSERT_TRUE(equals) << "### parsed: " << std::endl
+                      << cfg.DebugString() << std::endl
+                      << "### expected: " << std::endl
+                      << expected.DebugString();
+
+  input = R"(
+        enabled: true
+        fieldU32: 1234
+        fieldRepeated:
+            - 1
+            - 2
+            - 3
+        fieldEnum: TYPE2
+        fieldRepeatedEnum:
+            - TYPE2
+            - TYPE2
+    )";
+
+  expected.set_enabled(true);
+  expected.set_field_u32(1234);
+  expected.mutable_field_repeated()->Clear();
+  expected.mutable_field_repeated()->Add(1);
+  expected.mutable_field_repeated()->Add(2);
+  expected.mutable_field_repeated()->Add(3);
+  expected.set_field_enum(test_config::EnumField::TYPE2);
+  expected.mutable_field_repeated_enum()->Clear();
+  expected.mutable_field_repeated_enum()->Add(1);
+  expected.mutable_field_repeated_enum()->Add(1);
+
+  parser.Parse(&cfg, YAML::Load(input));
+
+  equals = MessageDifferencer::Equals(cfg, expected);
+  ASSERT_TRUE(equals) << "### parsed: " << std::endl
+                      << cfg.DebugString() << std::endl
+                      << "### expected: " << std::endl
+                      << expected.DebugString();
+}
+
+TEST(TestParserYaml, ParserErrors) {
+  test_config::Config cfg;
+  ParserYaml parser("/test.yml");
+  const std::string input = R"(
+        enabled: 1
+        fieldI32: wrong
+        fieldU32: {}
+        fieldI64: also_wrong
+        fieldU64: -64
+        fieldDouble: {}
+        fieldFloat: {}
+        fieldString: 123
+        fieldMessage: 1.2
+        fieldRepeated: 1
+        fieldEnum: NOT_REAL
+        fieldRepeatedEnum:
+            - NOT_REAL
+            - ALSO_INVALID
+            - TYPE2
+    )";
+
+  const ParserErrors expected = {
+      "\"/test.yml\": yaml-cpp: error at line 2, column 18: bad conversion",
+      "\"/test.yml\": yaml-cpp: error at line 3, column 19: bad conversion",
+      "\"/test.yml\": Attempting to parse non-scalar field as scalar",
+      "\"/test.yml\": yaml-cpp: error at line 5, column 19: bad conversion",
+      "\"/test.yml\": yaml-cpp: error at line 6, column 19: bad conversion",
+      "\"/test.yml\": Attempting to parse non-scalar field as scalar",
+      "\"/test.yml\": Attempting to parse non-scalar field as scalar",
+      "\"/test.yml\": Type mismatch for 'fieldMessage' - expected Map, got Scalar",
+      "\"/test.yml\": Type mismatch for 'fieldRepeated' - expected Sequence, got Scalar",
+      "\"/test.yml\": Invalid enum value 'NOT_REAL' for field fieldEnum",
+      "\"/test.yml\": Invalid enum value 'NOT_REAL' for field fieldRepeatedEnum",
+      "\"/test.yml\": Invalid enum value 'ALSO_INVALID' for field fieldRepeatedEnum",
+      "\"/test.yml\": Invalid type 'Map' for field fieldU32, expected 'uint32'",
+      "\"/test.yml\": Invalid type 'Map' for field fieldDouble, expected 'double'",
+      "\"/test.yml\": Invalid type 'Map' for field fieldFloat, expected 'float'",
+  };
+
+  auto errors = parser.Parse(&cfg, YAML::Load(input));
+  ASSERT_TRUE(errors);
+  ASSERT_EQ(errors->size(), expected.size()) << "#### parsed:" << std::endl
+                                             << ErrorsToString(*errors) << std::endl
+                                             << "#### expected" << std::endl
+                                             << ErrorsToString(expected);
+
+  for (unsigned int i = 0; i < expected.size(); i++) {
+    ASSERT_EQ(errors->at(i), expected.at(i));
+  }
+}
+
+/*
+ * Collector specific parsing tests
+ */
+
 TEST(CollectorConfigTest, TestYamlConfigToConfigMultiple) {
   std::vector<std::pair<std::string, bool>> tests = {
       {R"(
@@ -22,12 +221,17 @@ TEST(CollectorConfigTest, TestYamlConfigToConfigMultiple) {
                     externalIps:
                )",
        false},
+      {
+          R"(
+                  networking:
+               )",
+          false},
   };
 
   for (const auto& [yamlStr, expected] : tests) {
     YAML::Node yamlNode = YAML::Load(yamlStr);
     CollectorConfig config;
-    ASSERT_TRUE(ConfigLoader::LoadConfiguration(config, yamlNode));
+    ASSERT_TRUE(ConfigLoader(config).LoadConfiguration(yamlNode)) << "Input: " << yamlStr;
 
     auto runtime_config = config.GetRuntimeConfig();
 
@@ -46,10 +250,7 @@ TEST(CollectorConfigTest, TestYamlConfigToConfigInvalid) {
   std::vector<std::string> tests = {
       R"(
                   networking:
-               )",
-      R"(
-                  networking:
-                    unknownFiled: asdf
+                    unknownField: asdf
                )",
       R"(
                   unknownField: asdf
@@ -58,11 +259,11 @@ TEST(CollectorConfigTest, TestYamlConfigToConfigInvalid) {
   for (const auto& yamlStr : tests) {
     YAML::Node yamlNode = YAML::Load(yamlStr);
     CollectorConfig config;
-    ASSERT_TRUE(ConfigLoader::LoadConfiguration(config, yamlNode));
+    ASSERT_FALSE(ConfigLoader(config).LoadConfiguration(yamlNode)) << "Input: " << yamlStr;
 
     auto runtime_config = config.GetRuntimeConfig();
 
-    EXPECT_FALSE(runtime_config.has_value());
+    EXPECT_FALSE(runtime_config.has_value()) << "Input: " << yamlStr;
   }
 }
 
@@ -76,7 +277,7 @@ TEST(CollectorConfigTest, TestYamlConfigToConfigEmptyOrMalformed) {
   for (const auto& yamlStr : tests) {
     YAML::Node yamlNode = YAML::Load(yamlStr);
     CollectorConfig config;
-    ASSERT_FALSE(ConfigLoader::LoadConfiguration(config, yamlNode));
+    ASSERT_FALSE(ConfigLoader(config).LoadConfiguration(yamlNode));
 
     auto runtime_config = config.GetRuntimeConfig();
 
@@ -85,44 +286,53 @@ TEST(CollectorConfigTest, TestYamlConfigToConfigEmptyOrMalformed) {
 }
 
 TEST(CollectorConfigTest, TestMaxConnectionsPerMinute) {
-  std::vector<std::pair<std::string, int>> tests = {
+  struct TestCase {
+    std::string input;
+    int value;
+    bool valid;
+  };
+
+  std::vector<TestCase> tests = {
       {R"(
                   networking:
                     externalIps:
                       enabled: DISABLED
                     maxConnectionsPerMinute: 1234
                )",
-       1234},
+       1234, true},
       {R"(
                   networking:
                     externalIps:
                       enabled: DISABLED
                     maxConnectionsPerMinute: 1337
                )",
-       1337},
+       1337, true},
       {R"(
                   networking:
                     externalIps:
                       enabled: DISABLED
                     maxConnectionsPerMinute: invalid
                )",
-       2048},
+       2048, false},
   };
 
-  for (const auto& [yamlStr, expected] : tests) {
+  for (const auto& [yamlStr, expected, valid] : tests) {
     YAML::Node yamlNode = YAML::Load(yamlStr);
     CollectorConfig config;
-    ASSERT_TRUE(ConfigLoader::LoadConfiguration(config, yamlNode));
+    ASSERT_EQ(ConfigLoader(config).LoadConfiguration(yamlNode), valid);
 
     auto runtime_config = config.GetRuntimeConfig();
 
-    EXPECT_TRUE(runtime_config.has_value());
+    EXPECT_EQ(runtime_config.has_value(), valid);
 
-    int rate = runtime_config.value()
-                   .networking()
-                   .max_connections_per_minute();
-    EXPECT_EQ(rate, expected);
-    EXPECT_EQ(config.MaxConnectionsPerMinute(), expected);
+    if (valid) {
+      int rate = runtime_config.value()
+                     .networking()
+                     .max_connections_per_minute();
+      EXPECT_EQ(rate, expected);
+      EXPECT_EQ(config.MaxConnectionsPerMinute(), expected);
+    }
   }
 }
+
 }  // namespace collector
diff --git a/collector/test/proto/test-config.proto b/collector/test/proto/test-config.proto
new file mode 100644
index 0000000000..5c6f0c75f8
--- /dev/null
+++ b/collector/test/proto/test-config.proto
@@ -0,0 +1,26 @@
+syntax = "proto3";
+package test_config;
+
+message SubField {
+  bool enabled = 1;
+}
+
+enum EnumField {
+  TYPE1 = 0;
+  TYPE2 = 1;
+}
+
+message Config {
+  bool enabled = 1;
+  int32 field_i32 = 2;
+  uint32 field_u32 = 3;
+  int64 field_i64 = 4;
+  uint64 field_u64 = 5;
+  double field_double = 6;
+  float field_float = 7;
+  string field_string = 8;
+  SubField field_message = 9;
+  repeated uint64 field_repeated = 10;
+  EnumField field_enum = 11;
+  repeated EnumField field_repeated_enum = 12;
+}

From e01069894e46004e8924b3b2b20b255a6f562bda Mon Sep 17 00:00:00 2001
From: Mauro Ezequiel Moltrasio <mmoltras@redhat.com>
Date: Tue, 4 Feb 2025 11:25:23 +0100
Subject: [PATCH 02/10] Handle cases where the file doesn't exist

---
 collector/collector.cpp             |  8 +++++--
 collector/lib/ConfigLoader.cpp      | 37 ++++++++++++++++-------------
 collector/lib/ConfigLoader.h        | 24 ++++++++++---------
 collector/test/ConfigLoaderTest.cpp | 17 ++++++-------
 4 files changed, 48 insertions(+), 38 deletions(-)

diff --git a/collector/collector.cpp b/collector/collector.cpp
index af1b828e72..a503e45174 100644
--- a/collector/collector.cpp
+++ b/collector/collector.cpp
@@ -140,8 +140,12 @@ int main(int argc, char** argv) {
 
   CollectorConfig config;
   config.InitCollectorConfig(args);
-  if (!ConfigLoader(config).LoadConfiguration()) {
-    CLOG(FATAL) << "Unable to parse configuration file";
+  switch (ConfigLoader(config).LoadConfiguration()) {
+    case collector::ConfigLoader::SUCCESS:
+    case collector::ConfigLoader::FILE_NOT_FOUND:
+      break;
+    case collector::ConfigLoader::PARSE_ERROR:
+      CLOG(FATAL) << "Unable to parse configuration file";
   }
 
   setCoreDumpLimit(config.IsCoreDumpEnabled());
diff --git a/collector/lib/ConfigLoader.cpp b/collector/lib/ConfigLoader.cpp
index 1f433f46aa..8761415f88 100644
--- a/collector/lib/ConfigLoader.cpp
+++ b/collector/lib/ConfigLoader.cpp
@@ -39,7 +39,15 @@ std::string NodeTypeToString(YAML::NodeType::value type) {
 namespace stdf = std::filesystem;
 
 ParserResult ParserYaml::Parse(google::protobuf::Message* msg) {
-  YAML::Node node = YAML::LoadFile(file_);
+  YAML::Node node;
+  try {
+    node = YAML::LoadFile(file_);
+  } catch (const YAML::BadFile& e) {
+    return {{WrapError(e)}};
+  } catch (const YAML::ParserException& e) {
+    return {{WrapError(e)}};
+  }
+
   return Parse(msg, node);
 }
 
@@ -448,35 +456,30 @@ void ConfigLoader::Stop() {
   CLOG(INFO) << "No longer watching configuration file: " << parser_.GetFile().string();
 }
 
-bool ConfigLoader::LoadConfiguration() {
+ConfigLoader::Result ConfigLoader::LoadConfiguration(const std::optional<const YAML::Node>& node) {
   sensor::CollectorConfig runtime_config;
-  auto errors = parser_.Parse(&runtime_config);
+  ParserResult errors;
 
-  if (errors) {
-    CLOG(ERROR) << "Failed to parse " << parser_.GetFile();
-    for (const auto& err : *errors) {
-      CLOG(ERROR) << err;
+  if (!node.has_value()) {
+    if (!stdf::exists(parser_.GetFile())) {
+      return FILE_NOT_FOUND;
     }
-    return false;
-  }
+    errors = parser_.Parse(&runtime_config);
 
-  config_.SetRuntimeConfig(std::move(runtime_config));
-  return true;
-}
+  } else {
+    errors = parser_.Parse(&runtime_config, *node);
+  }
 
-bool ConfigLoader::LoadConfiguration(const YAML::Node& node) {
-  sensor::CollectorConfig runtime_config;
-  auto errors = parser_.Parse(&runtime_config, node);
   if (errors) {
     CLOG(ERROR) << "Failed to parse " << parser_.GetFile();
     for (const auto& err : *errors) {
       CLOG(ERROR) << err;
     }
-    return false;
+    return PARSE_ERROR;
   }
 
   config_.SetRuntimeConfig(std::move(runtime_config));
-  return true;
+  return SUCCESS;
 }
 
 void ConfigLoader::WatchFile() {
diff --git a/collector/lib/ConfigLoader.h b/collector/lib/ConfigLoader.h
index 0f3b52a2bb..f56ada0db1 100644
--- a/collector/lib/ConfigLoader.h
+++ b/collector/lib/ConfigLoader.h
@@ -1,6 +1,8 @@
 #ifndef _CONFIG_LOADER_H_
 #define _CONFIG_LOADER_H_
 
+#include <optional>
+
 #include <gtest/gtest_prod.h>
 #include <yaml-cpp/yaml.h>
 
@@ -94,12 +96,22 @@ class ConfigLoader {
   void Start();
   void Stop();
 
+  enum Result : uint8_t {
+    SUCCESS = 0,
+    PARSE_ERROR,
+    FILE_NOT_FOUND,
+  };
+
   /**
    * Load a configuration file into the supplied CollectorConfig object.
    *
+   * Alternatively, a YAML::Node can be supplied to load the
+   * configuration from it. This is mostly meant for testing pusposes.
+   *
+   * @param node a YAML::Node to be used as configuration
    * @returns true if configuration loading was successful.
    */
-  bool LoadConfiguration();
+  Result LoadConfiguration(const std::optional<const YAML::Node>& node = std::nullopt);
 
  private:
   FRIEND_TEST(CollectorConfigTest, TestYamlConfigToConfigMultiple);
@@ -107,16 +119,6 @@ class ConfigLoader {
   FRIEND_TEST(CollectorConfigTest, TestYamlConfigToConfigEmptyOrMalformed);
   FRIEND_TEST(CollectorConfigTest, TestMaxConnectionsPerMinute);
 
-  /**
-   * Load configuration from a YAML string.
-   *
-   * This method is meant to be used for testing only.
-   *
-   * @param node a YAML::Node with the new configuration to be used.
-   * @returns true if configuration loading was successful.
-   */
-  bool LoadConfiguration(const YAML::Node& node);
-
   /**
    * Wait for inotify events on a configuration file and reload it
    * accordingly.
diff --git a/collector/test/ConfigLoaderTest.cpp b/collector/test/ConfigLoaderTest.cpp
index f9413a0d28..c40ae86d0f 100644
--- a/collector/test/ConfigLoaderTest.cpp
+++ b/collector/test/ConfigLoaderTest.cpp
@@ -231,7 +231,7 @@ TEST(CollectorConfigTest, TestYamlConfigToConfigMultiple) {
   for (const auto& [yamlStr, expected] : tests) {
     YAML::Node yamlNode = YAML::Load(yamlStr);
     CollectorConfig config;
-    ASSERT_TRUE(ConfigLoader(config).LoadConfiguration(yamlNode)) << "Input: " << yamlStr;
+    ASSERT_EQ(ConfigLoader(config).LoadConfiguration(yamlNode), ConfigLoader::SUCCESS) << "Input: " << yamlStr;
 
     auto runtime_config = config.GetRuntimeConfig();
 
@@ -259,7 +259,7 @@ TEST(CollectorConfigTest, TestYamlConfigToConfigInvalid) {
   for (const auto& yamlStr : tests) {
     YAML::Node yamlNode = YAML::Load(yamlStr);
     CollectorConfig config;
-    ASSERT_FALSE(ConfigLoader(config).LoadConfiguration(yamlNode)) << "Input: " << yamlStr;
+    ASSERT_EQ(ConfigLoader(config).LoadConfiguration(yamlNode), ConfigLoader::PARSE_ERROR) << "Input: " << yamlStr;
 
     auto runtime_config = config.GetRuntimeConfig();
 
@@ -277,7 +277,7 @@ TEST(CollectorConfigTest, TestYamlConfigToConfigEmptyOrMalformed) {
   for (const auto& yamlStr : tests) {
     YAML::Node yamlNode = YAML::Load(yamlStr);
     CollectorConfig config;
-    ASSERT_FALSE(ConfigLoader(config).LoadConfiguration(yamlNode));
+    ASSERT_EQ(ConfigLoader(config).LoadConfiguration(yamlNode), ConfigLoader::PARSE_ERROR);
 
     auto runtime_config = config.GetRuntimeConfig();
 
@@ -290,6 +290,7 @@ TEST(CollectorConfigTest, TestMaxConnectionsPerMinute) {
     std::string input;
     int value;
     bool valid;
+    ConfigLoader::Result parse_result;
   };
 
   std::vector<TestCase> tests = {
@@ -299,27 +300,27 @@ TEST(CollectorConfigTest, TestMaxConnectionsPerMinute) {
                       enabled: DISABLED
                     maxConnectionsPerMinute: 1234
                )",
-       1234, true},
+       1234, true, ConfigLoader::SUCCESS},
       {R"(
                   networking:
                     externalIps:
                       enabled: DISABLED
                     maxConnectionsPerMinute: 1337
                )",
-       1337, true},
+       1337, true, ConfigLoader::SUCCESS},
       {R"(
                   networking:
                     externalIps:
                       enabled: DISABLED
                     maxConnectionsPerMinute: invalid
                )",
-       2048, false},
+       2048, false, ConfigLoader::PARSE_ERROR},
   };
 
-  for (const auto& [yamlStr, expected, valid] : tests) {
+  for (const auto& [yamlStr, expected, valid, parse_result] : tests) {
     YAML::Node yamlNode = YAML::Load(yamlStr);
     CollectorConfig config;
-    ASSERT_EQ(ConfigLoader(config).LoadConfiguration(yamlNode), valid);
+    ASSERT_EQ(ConfigLoader(config).LoadConfiguration(yamlNode), parse_result);
 
     auto runtime_config = config.GetRuntimeConfig();
 

From 46cca310266843462773cbe0d73266624a0f7739 Mon Sep 17 00:00:00 2001
From: Mauro Ezequiel Moltrasio <mmoltras@redhat.com>
Date: Tue, 4 Feb 2025 12:03:39 +0100
Subject: [PATCH 03/10] Minor cleanup + ignore asan false positive

---
 collector/lib/ConfigLoader.cpp      | 10 +++++-----
 collector/lib/ConfigLoader.h        |  7 +++----
 collector/test/CMakeLists.txt       |  5 +++++
 collector/test/ConfigLoaderTest.cpp |  4 ++--
 4 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/collector/lib/ConfigLoader.cpp b/collector/lib/ConfigLoader.cpp
index 8761415f88..11b2e54f52 100644
--- a/collector/lib/ConfigLoader.cpp
+++ b/collector/lib/ConfigLoader.cpp
@@ -58,7 +58,7 @@ ParserResult ParserYaml::Parse(google::protobuf::Message* msg, const YAML::Node&
     return {{"Invalid configuration"}};
   }
 
-  ParserErrors errors;
+  std::vector<ParserError> errors;
 
   const Descriptor* descriptor = msg->GetDescriptor();
   for (int i = 0; i < descriptor->field_count(); i++) {
@@ -85,7 +85,7 @@ ParserResult ParserYaml::Parse(google::protobuf::Message* msg, const YAML::Node&
 template <typename T>
 ParserResult ParserYaml::ParseArrayInner(google::protobuf::Message* msg, const YAML::Node& node,
                                          const google::protobuf::FieldDescriptor* field) {
-  ParserErrors errors;
+  std::vector<ParserError> errors;
   auto f = msg->GetReflection()->GetMutableRepeatedFieldRef<T>(msg, field);
   f.Clear();
   for (const auto& n : node) {
@@ -115,7 +115,7 @@ ParserResult ParserYaml::ParseArrayEnum(google::protobuf::Message* msg, const YA
     name = name_ptr.get();
   }
 
-  ParserErrors errors;
+  std::vector<ParserError> errors;
   auto f = msg->GetReflection()->GetMutableRepeatedFieldRef<int32>(msg, field);
   f.Clear();
 
@@ -149,7 +149,7 @@ ParserResult ParserYaml::FindUnkownFields(const google::protobuf::Message& msg,
   using namespace google::protobuf;
 
   const auto* descriptor = msg.GetDescriptor();
-  ParserErrors errors;
+  std::vector<ParserError> errors;
 
   for (YAML::const_iterator it = node.begin(); it != node.end(); it++) {
     auto name = it->first.as<std::string>();
@@ -227,7 +227,7 @@ ParserResult ParserYaml::Parse(google::protobuf::Message* msg, const YAML::Node&
           << NodeTypeToString(type);
       return {{err}};
     }
-    ParserErrors errors;
+    std::vector<ParserError> errors;
     const Reflection* reflection = msg->GetReflection();
 
     Message* m = reflection->MutableMessage(msg, field);
diff --git a/collector/lib/ConfigLoader.h b/collector/lib/ConfigLoader.h
index f56ada0db1..6c24940c09 100644
--- a/collector/lib/ConfigLoader.h
+++ b/collector/lib/ConfigLoader.h
@@ -21,8 +21,8 @@ class ParserError {
   ParserError& operator=(ParserError&&) noexcept = default;
   ~ParserError() = default;
 
-  ParserError(const char* msg) { msg_ += msg; }
-  ParserError(const std::string& msg) { msg_ += msg; }
+  ParserError(const char* msg) : msg_(msg) {}
+  ParserError(std::string msg) : msg_(std::move(msg)) {}
 
   const std::string& What() const { return msg_; }
 
@@ -45,8 +45,7 @@ class ParserError {
   std::string msg_;
 };
 
-using ParserErrors = std::vector<ParserError>;
-using ParserResult = std::optional<ParserErrors>;
+using ParserResult = std::optional<std::vector<ParserError>>;
 
 class ParserYaml {
  public:
diff --git a/collector/test/CMakeLists.txt b/collector/test/CMakeLists.txt
index 47c70a73ec..88bd295049 100644
--- a/collector/test/CMakeLists.txt
+++ b/collector/test/CMakeLists.txt
@@ -32,3 +32,8 @@ foreach(test_file ${TEST_SRC_FILES})
         add_test(NAME memcheck_${test_name} COMMAND valgrind -q --leak-check=full --trace-children=yes $<TARGET_FILE:${test_name}>)
     endif()
 endforeach()
+
+if (ADDRESS_SANITIZER)
+    # This test has a false positive when running under asan
+    set_property(TEST "ConfigLoaderTest" PROPERTY ENVIRONMENT "ASAN_OPTIONS=detect_container_overflow=0")
+endif()
diff --git a/collector/test/ConfigLoaderTest.cpp b/collector/test/ConfigLoaderTest.cpp
index c40ae86d0f..9ce2d90207 100644
--- a/collector/test/ConfigLoaderTest.cpp
+++ b/collector/test/ConfigLoaderTest.cpp
@@ -12,7 +12,7 @@
 namespace collector {
 using namespace google::protobuf::util;
 
-std::string ErrorsToString(const ParserErrors& errors) {
+std::string ErrorsToString(const std::vector<ParserError>& errors) {
   std::stringstream ss;
   for (size_t i = 0; i < errors.size(); i++) {
     ss << i << ": " << errors.at(i).What() << std::endl;
@@ -168,7 +168,7 @@ TEST(TestParserYaml, ParserErrors) {
             - TYPE2
     )";
 
-  const ParserErrors expected = {
+  const std::vector<ParserError> expected = {
       "\"/test.yml\": yaml-cpp: error at line 2, column 18: bad conversion",
       "\"/test.yml\": yaml-cpp: error at line 3, column 19: bad conversion",
       "\"/test.yml\": Attempting to parse non-scalar field as scalar",

From 56b45ff261e015899eed94162e2de24d9abcb04d Mon Sep 17 00:00:00 2001
From: Mauro Ezequiel Moltrasio <mmoltras@redhat.com>
Date: Tue, 4 Feb 2025 15:19:18 +0100
Subject: [PATCH 04/10] Add runtime configuration log statement + log events
 correctly on integration-tests

---
 collector/lib/ConfigLoader.cpp              | 2 ++
 integration-tests/pkg/mock_sensor/server.go | 4 ++++
 integration-tests/suites/base.go            | 2 ++
 3 files changed, 8 insertions(+)

diff --git a/collector/lib/ConfigLoader.cpp b/collector/lib/ConfigLoader.cpp
index 11b2e54f52..cd966d7aa0 100644
--- a/collector/lib/ConfigLoader.cpp
+++ b/collector/lib/ConfigLoader.cpp
@@ -479,6 +479,8 @@ ConfigLoader::Result ConfigLoader::LoadConfiguration(const std::optional<const Y
   }
 
   config_.SetRuntimeConfig(std::move(runtime_config));
+  CLOG(DEBUG) << "Runtime configuration:\n"
+              << config_.GetRuntimeConfigStr();
   return SUCCESS;
 }
 
diff --git a/integration-tests/pkg/mock_sensor/server.go b/integration-tests/pkg/mock_sensor/server.go
index f8e1e66be3..4bab19889f 100644
--- a/integration-tests/pkg/mock_sensor/server.go
+++ b/integration-tests/pkg/mock_sensor/server.go
@@ -514,3 +514,7 @@ func (m *MockSensor) translateAddress(addr *sensorAPI.NetworkAddress) string {
 	}
 	return peerId.String()
 }
+
+func (m *MockSensor) SetTestName(testName string) {
+	m.testName = testName
+}
diff --git a/integration-tests/suites/base.go b/integration-tests/suites/base.go
index 1430e22427..0195ced89e 100644
--- a/integration-tests/suites/base.go
+++ b/integration-tests/suites/base.go
@@ -151,6 +151,8 @@ func (s *IntegrationTestSuiteBase) Executor() executor.Executor {
 func (s *IntegrationTestSuiteBase) Sensor() *mock_sensor.MockSensor {
 	if s.sensor == nil {
 		s.sensor = mock_sensor.NewMockSensor(s.T().Name())
+	} else {
+		s.sensor.SetTestName(s.T().Name())
 	}
 	return s.sensor
 }

From 4fa145669bc769967e87a43ce37a970b797c5c60 Mon Sep 17 00:00:00 2001
From: Mauro Ezequiel Moltrasio <mmoltras@redhat.com>
Date: Tue, 4 Feb 2025 17:44:34 +0100
Subject: [PATCH 05/10] Provide defaults that are different from protobuf
 defaults

---
 collector/lib/ConfigLoader.cpp | 11 ++++++++++-
 collector/lib/ConfigLoader.h   |  4 ++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/collector/lib/ConfigLoader.cpp b/collector/lib/ConfigLoader.cpp
index cd966d7aa0..45be0198a3 100644
--- a/collector/lib/ConfigLoader.cpp
+++ b/collector/lib/ConfigLoader.cpp
@@ -457,7 +457,7 @@ void ConfigLoader::Stop() {
 }
 
 ConfigLoader::Result ConfigLoader::LoadConfiguration(const std::optional<const YAML::Node>& node) {
-  sensor::CollectorConfig runtime_config;
+  sensor::CollectorConfig runtime_config = NewRuntimeConfig();
   ParserResult errors;
 
   if (!node.has_value()) {
@@ -484,6 +484,15 @@ ConfigLoader::Result ConfigLoader::LoadConfiguration(const std::optional<const Y
   return SUCCESS;
 }
 
+sensor::CollectorConfig ConfigLoader::NewRuntimeConfig() {
+  sensor::CollectorConfig runtime_config;
+
+  // Set default values that are different from the protobuf defaults
+  runtime_config.mutable_networking()->set_max_connections_per_minute(CollectorConfig::kMaxConnectionsPerMinute);
+
+  return runtime_config;
+}
+
 void ConfigLoader::WatchFile() {
   const auto& file = parser_.GetFile();
 
diff --git a/collector/lib/ConfigLoader.h b/collector/lib/ConfigLoader.h
index 6c24940c09..c436ef8e2e 100644
--- a/collector/lib/ConfigLoader.h
+++ b/collector/lib/ConfigLoader.h
@@ -6,6 +6,8 @@
 #include <gtest/gtest_prod.h>
 #include <yaml-cpp/yaml.h>
 
+#include "internalapi/sensor/collector.pb.h"
+
 #include "CollectorConfig.h"
 #include "Inotify.h"
 #include "StoppableThread.h"
@@ -118,6 +120,8 @@ class ConfigLoader {
   FRIEND_TEST(CollectorConfigTest, TestYamlConfigToConfigEmptyOrMalformed);
   FRIEND_TEST(CollectorConfigTest, TestMaxConnectionsPerMinute);
 
+  static sensor::CollectorConfig NewRuntimeConfig();
+
   /**
    * Wait for inotify events on a configuration file and reload it
    * accordingly.

From 8ad639f2cdba43c9b1fe92d36ef28603724c2aa4 Mon Sep 17 00:00:00 2001
From: Mauro Ezequiel Moltrasio <mmoltras@redhat.com>
Date: Tue, 4 Feb 2025 17:52:46 +0100
Subject: [PATCH 06/10] Add case insensitive enum lookup

---
 collector/lib/ConfigLoader.cpp      | 16 +++++++++++++---
 collector/test/ConfigLoaderTest.cpp |  4 ++--
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/collector/lib/ConfigLoader.cpp b/collector/lib/ConfigLoader.cpp
index 45be0198a3..8284c8f2fd 100644
--- a/collector/lib/ConfigLoader.cpp
+++ b/collector/lib/ConfigLoader.cpp
@@ -1,5 +1,7 @@
 #include "ConfigLoader.h"
 
+#include <algorithm>
+
 #include <google/protobuf/descriptor.h>
 
 #include "internalapi/sensor/collector.pb.h"
@@ -121,12 +123,15 @@ ParserResult ParserYaml::ParseArrayEnum(google::protobuf::Message* msg, const YA
 
   const EnumDescriptor* desc = field->enum_type();
   for (const auto& n : node) {
-    auto v = TryConvert<std::string_view>(n);
+    auto v = TryConvert<std::string>(n);
     if (IsError(v)) {
       errors.emplace_back(std::get<ParserError>(v));
       continue;
     }
-    const auto enum_name = std::get<std::string_view>(v);
+    auto enum_name = std::get<std::string>(v);
+    std::transform(enum_name.begin(), enum_name.end(), enum_name.begin(), [](char c) {
+      return std::toupper(c);
+    });
 
     const EnumValueDescriptor* value = desc->FindValueByName(enum_name);
     if (value == nullptr) {
@@ -331,7 +336,12 @@ ParserResult ParserYaml::Parse(google::protobuf::Message* msg, const YAML::Node&
       std::cerr << "Unsupported type BYTES" << std::endl;
       break;
     case FieldDescriptor::TYPE_ENUM: {
-      const auto enum_name = node[*name].as<std::string_view>();
+      auto enum_name = node[*name].as<std::string>();
+
+      // We assume enum definitions use UPPER_CASE nomenclature, so
+      std::transform(enum_name.begin(), enum_name.end(), enum_name.begin(), [](char c) {
+        return std::toupper(c);
+      });
 
       const EnumDescriptor* descriptor = field->enum_type();
       const EnumValueDescriptor* value = descriptor->FindValueByName(enum_name);
diff --git a/collector/test/ConfigLoaderTest.cpp b/collector/test/ConfigLoaderTest.cpp
index 9ce2d90207..d980e6c155 100644
--- a/collector/test/ConfigLoaderTest.cpp
+++ b/collector/test/ConfigLoaderTest.cpp
@@ -63,9 +63,9 @@ TEST(TestParserYaml, Parsing) {
                 - 1
                 - 2
                 - 3
-            fieldEnum: TYPE2
+            fieldEnum: type2
             fieldRepeatedEnum:
-                - TYPE1
+                - type1
                 - TYPE2
         )",
        all_fields},

From bb08c357fb0ce9a39bc45cb9be3c03a4fceadb09 Mon Sep 17 00:00:00 2001
From: Mauro Ezequiel Moltrasio <mmoltras@redhat.com>
Date: Wed, 5 Feb 2025 10:55:28 +0100
Subject: [PATCH 07/10] Add some doxygen to the new classes

---
 collector/lib/ConfigLoader.h | 80 ++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/collector/lib/ConfigLoader.h b/collector/lib/ConfigLoader.h
index c436ef8e2e..0e363c72b5 100644
--- a/collector/lib/ConfigLoader.h
+++ b/collector/lib/ConfigLoader.h
@@ -53,31 +53,106 @@ class ParserYaml {
  public:
   ParserYaml(std::filesystem::path file, bool read_camelcase = true) : file_(std::move(file)), read_camelcase_(read_camelcase) {}
 
+  /**
+   * Populate a protobuf message from the configuration file assigned
+   * to this parser.
+   *
+   * @param msg The protobuf message to be populated.
+   * @return an optional vector of parser errors.
+   */
   ParserResult Parse(google::protobuf::Message* msg);
+
+  /**
+   * Populate a protobuf message from a provided YAML::Node.
+   *
+   * @param msg The protobuf message to be populated.
+   * @param node A YAML::Node used to populate the message.
+   * @return an optional vector of parser errors.
+   */
   ParserResult Parse(google::protobuf::Message* msg, const YAML::Node& node);
 
   const std::filesystem::path& GetFile() { return file_; }
 
  private:
+  /**
+   * Inner method that will parse the provided field into the protobuf
+   * message from the corresponding values in the YAML::Node.
+   *
+   * @param msg The protobuf message to be populated.
+   * @param node A YAML::Node used to populate the message.
+   * @param field The descriptor for the field being parsed.
+   * @return an optional vector of parser errors.
+   */
   ParserResult Parse(google::protobuf::Message* msg, const YAML::Node& node,
                      const google::protobuf::FieldDescriptor* field);
+
+  /**
+   * Populate a repeated protobuf message from an array.
+   *
+   * @param msg The protobuf message to be populated.
+   * @param node A YAML::Node used to populate the message.
+   * @param field The descriptor for the field being parsed.
+   * @return an optional vector of parser errors.
+   */
   ParserResult ParseArray(google::protobuf::Message* msg, const YAML::Node& node,
                           const google::protobuf::FieldDescriptor* field);
+
+  /**
+   * Populate a repeated protobuf message from an array.
+   *
+   * @param msg The protobuf message to be populated.
+   * @param node A YAML::Node used to populate the message.
+   * @param field The descriptor for the field being parsed.
+   * @return an optional vector of parser errors.
+   */
   template <typename T>
   ParserResult ParseArrayInner(google::protobuf::Message* msg, const YAML::Node& node,
                                const google::protobuf::FieldDescriptor* field);
+
+  /**
+   * Populate a repeated enum field from an array.
+   *
+   * @param msg The protobuf message to be populated.
+   * @param node A YAML::Node used to populate the message.
+   * @param field The descriptor for the field being parsed.
+   * @return an optional vector of parser errors.
+   */
   ParserResult ParseArrayEnum(google::protobuf::Message* msg, const YAML::Node& node,
                               const google::protobuf::FieldDescriptor* field);
+
+  /**
+   * Go through all nodes in the configuration and notify of any
+   * elements that have no corresponding field in the protobuf message.
+   *
+   * @param msg The protobuf message used for validation.
+   * @param node The YAML::Node to be walked.
+   * @return an optional vector of parser errors.
+   */
   ParserResult FindUnkownFields(const google::protobuf::Message& msg, const YAML::Node& node);
 
   ParserError WrapError(const std::exception& e);
 
+  /**
+   * Read a value from a YAML::Node, preventing exceptions from being
+   * thrown.
+   *
+   * @param node A YAML::Node to be read.
+   * @return Either the read value or a parser error.
+   */
   template <typename T>
   std::variant<T, ParserError> TryConvert(const YAML::Node& node);
+
+  /**
+   * Check if the result of TryConvert is an error.
+   *
+   * @param res The output from a call to TryConvert
+   * @return true if a parsing error occurred, false otherwise.
+   */
   template <typename T>
   static bool IsError(const std::variant<T, ParserError>& res) {
     return std::holds_alternative<ParserError>(res);
   }
+
   static std::string SnakeCaseToCamel(const std::string& s);
   static std::string CamelCaseToSnake(const std::string& s);
 
@@ -120,6 +195,11 @@ class ConfigLoader {
   FRIEND_TEST(CollectorConfigTest, TestYamlConfigToConfigEmptyOrMalformed);
   FRIEND_TEST(CollectorConfigTest, TestMaxConnectionsPerMinute);
 
+  /**
+   * Create a new runtime configuration object with correct defaults.
+   *
+   * @returns The new runtime configuration object.
+   */
   static sensor::CollectorConfig NewRuntimeConfig();
 
   /**

From 7d747d92d0e1dc94ea5123de4308664a7c1eed26 Mon Sep 17 00:00:00 2001
From: Mauro Ezequiel Moltrasio <mmoltras@redhat.com>
Date: Wed, 5 Feb 2025 11:05:14 +0100
Subject: [PATCH 08/10] Replace stringstream with ParserError where it makes
 sense

---
 collector/lib/ConfigLoader.cpp | 24 ++++++++++++------------
 collector/lib/ConfigLoader.h   | 18 +++++++++---------
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/collector/lib/ConfigLoader.cpp b/collector/lib/ConfigLoader.cpp
index 8284c8f2fd..4e58380029 100644
--- a/collector/lib/ConfigLoader.cpp
+++ b/collector/lib/ConfigLoader.cpp
@@ -164,17 +164,17 @@ ParserResult ParserYaml::FindUnkownFields(const google::protobuf::Message& msg,
 
     const FieldDescriptor* field = descriptor->FindFieldByName(name);
     if (field == nullptr) {
-      std::stringstream ss;
-      ss << "Unknown field '" << name << "'";
-      errors.emplace_back(ss.str());
+      ParserError err;
+      err << "Unknown field '" << name << "'";
+      errors.emplace_back(err);
       continue;
     }
 
     if (it->second.IsMap()) {
       if (field->type() != FieldDescriptor::TYPE_MESSAGE) {
-        std::stringstream ss;
-        ss << file_ << ": Invalid type '" << NodeTypeToString(it->second.Type()) << "' for field " << it->first.as<std::string_view>() << ", expected '" << field->type_name() << "'";
-        errors.emplace_back(ss.str());
+        ParserError err;
+        err << file_ << ": Invalid type '" << NodeTypeToString(it->second.Type()) << "' for field " << it->first.as<std::string_view>() << ", expected '" << field->type_name() << "'";
+        errors.emplace_back(err);
         continue;
       }
 
@@ -393,17 +393,17 @@ ParserResult ParserYaml::ParseArray(google::protobuf::Message* msg, const YAML::
       return {{"Unsupport repeated type MESSAGE"}};
     } break;
     default: {
-      std::stringstream ss;
-      ss << "Unknown type " << field->type_name();
-      return {{ss.str()}};
+      ParserError err;
+      err << "Unknown type " << field->type_name();
+      return {{err}};
     }
   }
 }
 
 ParserError ParserYaml::WrapError(const std::exception& e) {
-  std::stringstream ss;
-  ss << file_ << ": " << e.what();
-  return ss.str();
+  ParserError err;
+  err << file_ << ": " << e.what();
+  return err;
 }
 
 template <typename T>
diff --git a/collector/lib/ConfigLoader.h b/collector/lib/ConfigLoader.h
index 0e363c72b5..02c6aefc1c 100644
--- a/collector/lib/ConfigLoader.h
+++ b/collector/lib/ConfigLoader.h
@@ -58,7 +58,7 @@ class ParserYaml {
    * to this parser.
    *
    * @param msg The protobuf message to be populated.
-   * @return an optional vector of parser errors.
+   * @returns an optional vector of parser errors.
    */
   ParserResult Parse(google::protobuf::Message* msg);
 
@@ -67,7 +67,7 @@ class ParserYaml {
    *
    * @param msg The protobuf message to be populated.
    * @param node A YAML::Node used to populate the message.
-   * @return an optional vector of parser errors.
+   * @returns an optional vector of parser errors.
    */
   ParserResult Parse(google::protobuf::Message* msg, const YAML::Node& node);
 
@@ -81,7 +81,7 @@ class ParserYaml {
    * @param msg The protobuf message to be populated.
    * @param node A YAML::Node used to populate the message.
    * @param field The descriptor for the field being parsed.
-   * @return an optional vector of parser errors.
+   * @returns an optional vector of parser errors.
    */
   ParserResult Parse(google::protobuf::Message* msg, const YAML::Node& node,
                      const google::protobuf::FieldDescriptor* field);
@@ -92,7 +92,7 @@ class ParserYaml {
    * @param msg The protobuf message to be populated.
    * @param node A YAML::Node used to populate the message.
    * @param field The descriptor for the field being parsed.
-   * @return an optional vector of parser errors.
+   * @returns an optional vector of parser errors.
    */
   ParserResult ParseArray(google::protobuf::Message* msg, const YAML::Node& node,
                           const google::protobuf::FieldDescriptor* field);
@@ -103,7 +103,7 @@ class ParserYaml {
    * @param msg The protobuf message to be populated.
    * @param node A YAML::Node used to populate the message.
    * @param field The descriptor for the field being parsed.
-   * @return an optional vector of parser errors.
+   * @returns an optional vector of parser errors.
    */
   template <typename T>
   ParserResult ParseArrayInner(google::protobuf::Message* msg, const YAML::Node& node,
@@ -115,7 +115,7 @@ class ParserYaml {
    * @param msg The protobuf message to be populated.
    * @param node A YAML::Node used to populate the message.
    * @param field The descriptor for the field being parsed.
-   * @return an optional vector of parser errors.
+   * @returns an optional vector of parser errors.
    */
   ParserResult ParseArrayEnum(google::protobuf::Message* msg, const YAML::Node& node,
                               const google::protobuf::FieldDescriptor* field);
@@ -126,7 +126,7 @@ class ParserYaml {
    *
    * @param msg The protobuf message used for validation.
    * @param node The YAML::Node to be walked.
-   * @return an optional vector of parser errors.
+   * @returns an optional vector of parser errors.
    */
   ParserResult FindUnkownFields(const google::protobuf::Message& msg, const YAML::Node& node);
 
@@ -137,7 +137,7 @@ class ParserYaml {
    * thrown.
    *
    * @param node A YAML::Node to be read.
-   * @return Either the read value or a parser error.
+   * @returns Either the read value or a parser error.
    */
   template <typename T>
   std::variant<T, ParserError> TryConvert(const YAML::Node& node);
@@ -146,7 +146,7 @@ class ParserYaml {
    * Check if the result of TryConvert is an error.
    *
    * @param res The output from a call to TryConvert
-   * @return true if a parsing error occurred, false otherwise.
+   * @returns true if a parsing error occurred, false otherwise.
    */
   template <typename T>
   static bool IsError(const std::variant<T, ParserError>& res) {

From 09100de77a4d98464486531a944a9dd42756689b Mon Sep 17 00:00:00 2001
From: Mauro Ezequiel Moltrasio <mmoltras@redhat.com>
Date: Wed, 5 Feb 2025 14:32:42 +0100
Subject: [PATCH 09/10] Some more small cleanups

---
 collector/lib/ConfigLoader.cpp | 40 +++++++++++++++++++---------------
 collector/lib/ConfigLoader.h   |  2 ++
 2 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/collector/lib/ConfigLoader.cpp b/collector/lib/ConfigLoader.cpp
index 4e58380029..073c9b085e 100644
--- a/collector/lib/ConfigLoader.cpp
+++ b/collector/lib/ConfigLoader.cpp
@@ -232,11 +232,9 @@ ParserResult ParserYaml::Parse(google::protobuf::Message* msg, const YAML::Node&
           << NodeTypeToString(type);
       return {{err}};
     }
-    std::vector<ParserError> errors;
-    const Reflection* reflection = msg->GetReflection();
-
-    Message* m = reflection->MutableMessage(msg, field);
 
+    std::vector<ParserError> errors;
+    Message* m = msg->GetReflection()->MutableMessage(msg, field);
     const Descriptor* descriptor = m->GetDescriptor();
     for (int i = 0; i < descriptor->field_count(); i++) {
       const FieldDescriptor* f = descriptor->field(i);
@@ -259,9 +257,15 @@ ParserResult ParserYaml::Parse(google::protobuf::Message* msg, const YAML::Node&
     return {{err}};
   }
 
+  return ParseScalar(msg, node[*name], field, *name);
+}
+
+ParserResult ParserYaml::ParseScalar(google::protobuf::Message* msg, const YAML::Node& node, const google::protobuf::FieldDescriptor* field, const std::string& name) {
+  using namespace google::protobuf;
+
   switch (field->type()) {
     case FieldDescriptor::TYPE_DOUBLE: {
-      auto value = TryConvert<double>(node[*name]);
+      auto value = TryConvert<double>(node);
       if (IsError(value)) {
         return {{std::get<ParserError>(value)}};
       }
@@ -269,7 +273,7 @@ ParserResult ParserYaml::Parse(google::protobuf::Message* msg, const YAML::Node&
       msg->GetReflection()->SetDouble(msg, field, std::get<double>(value));
     } break;
     case FieldDescriptor::TYPE_FLOAT: {
-      auto value = TryConvert<float>(node[*name]);
+      auto value = TryConvert<float>(node);
       if (IsError(value)) {
         return {{std::get<ParserError>(value)}};
       }
@@ -278,7 +282,7 @@ ParserResult ParserYaml::Parse(google::protobuf::Message* msg, const YAML::Node&
     } break;
     case FieldDescriptor::TYPE_SFIXED64:
     case FieldDescriptor::TYPE_INT64: {
-      auto value = TryConvert<int64_t>(node[*name]);
+      auto value = TryConvert<int64_t>(node);
       if (IsError(value)) {
         return {{std::get<ParserError>(value)}};
       }
@@ -288,7 +292,7 @@ ParserResult ParserYaml::Parse(google::protobuf::Message* msg, const YAML::Node&
     case FieldDescriptor::TYPE_SINT64:
     case FieldDescriptor::TYPE_FIXED64:
     case FieldDescriptor::TYPE_UINT64: {
-      auto value = TryConvert<uint64_t>(node[*name]);
+      auto value = TryConvert<uint64_t>(node);
       if (IsError(value)) {
         return {{std::get<ParserError>(value)}};
       }
@@ -297,7 +301,7 @@ ParserResult ParserYaml::Parse(google::protobuf::Message* msg, const YAML::Node&
     } break;
     case FieldDescriptor::TYPE_FIXED32:
     case FieldDescriptor::TYPE_UINT32: {
-      auto value = TryConvert<uint32_t>(node[*name]);
+      auto value = TryConvert<uint32_t>(node);
       if (IsError(value)) {
         return {{std::get<ParserError>(value)}};
       }
@@ -307,7 +311,7 @@ ParserResult ParserYaml::Parse(google::protobuf::Message* msg, const YAML::Node&
     case FieldDescriptor::TYPE_SINT32:
     case FieldDescriptor::TYPE_SFIXED32:
     case FieldDescriptor::TYPE_INT32: {
-      auto value = TryConvert<int32_t>(node[*name]);
+      auto value = TryConvert<int32_t>(node);
       if (IsError(value)) {
         return {{std::get<ParserError>(value)}};
       }
@@ -315,7 +319,7 @@ ParserResult ParserYaml::Parse(google::protobuf::Message* msg, const YAML::Node&
       msg->GetReflection()->SetInt32(msg, field, std::get<int32_t>(value));
     } break;
     case FieldDescriptor::TYPE_BOOL: {
-      auto value = TryConvert<bool>(node[*name]);
+      auto value = TryConvert<bool>(node);
       if (IsError(value)) {
         return {{std::get<ParserError>(value)}};
       }
@@ -324,7 +328,7 @@ ParserResult ParserYaml::Parse(google::protobuf::Message* msg, const YAML::Node&
 
     } break;
     case FieldDescriptor::TYPE_STRING: {
-      auto value = TryConvert<std::string>(node[*name]);
+      auto value = TryConvert<std::string>(node);
       if (IsError(value)) {
         return {{std::get<ParserError>(value)}};
       }
@@ -332,11 +336,13 @@ ParserResult ParserYaml::Parse(google::protobuf::Message* msg, const YAML::Node&
       msg->GetReflection()->SetString(msg, field, std::get<std::string>(value));
 
     } break;
-    case FieldDescriptor::TYPE_BYTES:
-      std::cerr << "Unsupported type BYTES" << std::endl;
-      break;
+    case FieldDescriptor::TYPE_BYTES: {
+      ParserError err;
+      err << "Unexpected type BYTES";
+      return {{err}};
+    }
     case FieldDescriptor::TYPE_ENUM: {
-      auto enum_name = node[*name].as<std::string>();
+      auto enum_name = node.as<std::string>();
 
       // We assume enum definitions use UPPER_CASE nomenclature, so
       std::transform(enum_name.begin(), enum_name.end(), enum_name.begin(), [](char c) {
@@ -347,7 +353,7 @@ ParserResult ParserYaml::Parse(google::protobuf::Message* msg, const YAML::Node&
       const EnumValueDescriptor* value = descriptor->FindValueByName(enum_name);
       if (value == nullptr) {
         ParserError err;
-        err << file_ << ": Invalid enum value '" << enum_name << "' for field " << *name;
+        err << file_ << ": Invalid enum value '" << enum_name << "' for field " << name;
         return {{err}};
       }
       msg->GetReflection()->SetEnumValue(msg, field, value->number());
diff --git a/collector/lib/ConfigLoader.h b/collector/lib/ConfigLoader.h
index 02c6aefc1c..c1e259bcfa 100644
--- a/collector/lib/ConfigLoader.h
+++ b/collector/lib/ConfigLoader.h
@@ -120,6 +120,8 @@ class ParserYaml {
   ParserResult ParseArrayEnum(google::protobuf::Message* msg, const YAML::Node& node,
                               const google::protobuf::FieldDescriptor* field);
 
+  ParserResult ParseScalar(google::protobuf::Message* msg, const YAML::Node& node,
+                           const google::protobuf::FieldDescriptor* field, const std::string& name);
   /**
    * Go through all nodes in the configuration and notify of any
    * elements that have no corresponding field in the protobuf message.

From ff49a3461806ad7be6a349f3bc54f6c87cda2cca Mon Sep 17 00:00:00 2001
From: Mauro Ezequiel Moltrasio <mmoltras@redhat.com>
Date: Thu, 6 Feb 2025 11:03:18 +0100
Subject: [PATCH 10/10] Simplify switch statement to if

---
 collector/collector.cpp       | 8 ++------
 collector/test/CMakeLists.txt | 1 -
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/collector/collector.cpp b/collector/collector.cpp
index a503e45174..2c0b1df6ac 100644
--- a/collector/collector.cpp
+++ b/collector/collector.cpp
@@ -140,12 +140,8 @@ int main(int argc, char** argv) {
 
   CollectorConfig config;
   config.InitCollectorConfig(args);
-  switch (ConfigLoader(config).LoadConfiguration()) {
-    case collector::ConfigLoader::SUCCESS:
-    case collector::ConfigLoader::FILE_NOT_FOUND:
-      break;
-    case collector::ConfigLoader::PARSE_ERROR:
-      CLOG(FATAL) << "Unable to parse configuration file";
+  if (ConfigLoader(config).LoadConfiguration() == collector::ConfigLoader::PARSE_ERROR) {
+    CLOG(FATAL) << "Unable to parse configuration file";
   }
 
   setCoreDumpLimit(config.IsCoreDumpEnabled());
diff --git a/collector/test/CMakeLists.txt b/collector/test/CMakeLists.txt
index 88bd295049..015e2f8ffa 100644
--- a/collector/test/CMakeLists.txt
+++ b/collector/test/CMakeLists.txt
@@ -1,7 +1,6 @@
 # Setup testing
 
 find_package(GTest CONFIG REQUIRED)
-enable_testing()
 
 set(CMAKE_INCLUDE_CURRENT_DIR ON)