Improve error reporting for sending metadata to Secondaries

src/libaktualizr/primary/aktualizr_test.cc

@@ -111,7 +111,8 @@ TEST(Aktualizr, FullNoUpdates) {
 }
 
 /*
- * Compute device installation failure code as concatenation of ECU failure codes.
+ * Compute device installation failure code as concatenation of ECU failure
+ * codes during installation.
  */
 TEST(Aktualizr, DeviceInstallationResult) {
   TemporaryDirectory temp_dir;
@@ -151,6 +152,56 @@ TEST(Aktualizr, DeviceInstallationResult) {
   EXPECT_EQ(res_json["success"], false);
 }
 
+#ifdef FIU_ENABLE
+
+/*
+ * Compute device installation failure code as concatenation of ECU failure
+ * codes from sending metadata to Secondaries.
+ */
+TEST(Aktualizr, DeviceInstallationResultMetadata) {
+  TemporaryDirectory temp_dir;
+  // Use "hasupdates" to make sure Image repo Root gets fetched, despite that we
+  // won't use the default update.
+  auto http = std::make_shared<HttpFake>(temp_dir.Path(), "hasupdates", fake_meta_dir);
+  Config conf = UptaneTestCommon::makeTestConfig(temp_dir, http->tls_server);
+  UptaneTestCommon::addDefaultSecondary(conf, temp_dir, "sec_serial1", "sec_hw1");
+  UptaneTestCommon::addDefaultSecondary(conf, temp_dir, "sec_serial2", "sec_hw2");
+  UptaneTestCommon::addDefaultSecondary(conf, temp_dir, "sec_serial3", "sec_hw3");
+
+  auto storage = INvStorage::newStorage(conf.storage);
+  UptaneTestCommon::TestAktualizr aktualizr(conf, storage, http);
+  aktualizr.Initialize();
+  auto update_result = aktualizr.CheckUpdates().get();
+  EXPECT_EQ(update_result.status, result::UpdateStatus::kUpdatesAvailable);
+
+  fault_injection_init();
+  fiu_enable("secondary_putmetadata", 1, nullptr, 0);
+
+  // Try updating two Secondaries; leave the third one alone.
+  std::vector<Uptane::Target> targets;
+  Json::Value target_json;
+  target_json["custom"]["targetFormat"] = "BINARY";
+  target_json["custom"]["ecuIdentifiers"]["sec_serial1"]["hardwareId"] = "sec_hw1";
+  target_json["custom"]["ecuIdentifiers"]["sec_serial3"]["hardwareId"] = "sec_hw3";
+  targets.emplace_back(Uptane::Target("test", target_json));
+
+  data::InstallationResult result;
+  aktualizr.uptane_client()->sendMetadataToEcus(targets, &result, nullptr);
+  auto res_json = result.toJson();
+  EXPECT_EQ(res_json["code"].asString(), "sec_hw1:VERIFICATION_FAILED|sec_hw3:VERIFICATION_FAILED");
+  EXPECT_EQ(res_json["success"], false);
+
+  fiu_disable("secondary_putmetadata");
+
+  // Retry after disabling fault injection to verify the test.
+  aktualizr.uptane_client()->sendMetadataToEcus(targets, &result, nullptr);
+  res_json = result.toJson();
+  EXPECT_EQ(res_json["code"].asString(), "OK");
+  EXPECT_EQ(res_json["success"], true);
+}
+
+#endif  // FIU_ENABLE
+
 class HttpFakeEventCounter : public HttpFake {
  public:
   HttpFakeEventCounter(const boost::filesystem::path& test_dir_in, const boost::filesystem::path& meta_dir_in)

src/libaktualizr/primary/sotauptaneclient.cc

@@ -412,7 +412,6 @@ void SotaUptaneClient::computeDeviceInstallationResult(data::InstallationResult
       device_installation_result = data::InstallationResult(data::ResultCode::Numeric::kInternalError,
                                                             "Unable to get installation results from ECUs");
       raw_ir = "Failed to load ECU installation results";
-
       break;
     }
 
@@ -430,7 +429,6 @@ void SotaUptaneClient::computeDeviceInstallationResult(data::InstallationResult
                                                               "Unable to get installation results from ECUs");
 
         raw_ir = "Failed to find an ECU with the given serial: " + ecu_serial.ToString();
-
         break;
       }
 
@@ -440,14 +438,13 @@ void SotaUptaneClient::computeDeviceInstallationResult(data::InstallationResult
             data::InstallationResult(data::ResultCode::Numeric::kNeedCompletion,
                                      "ECU needs completion/finalization to be installed: " + ecu_serial.ToString());
         raw_ir = "ECU needs completion/finalization to be installed: " + ecu_serial.ToString();
-
         break;
       }
 
       // format:
       // ecu1_hwid:failure1|ecu2_hwid:failure2
       if (!installation_res.isSuccess()) {
-        std::string ecu_code_str = (*hw_id).ToString() + ":" + installation_res.result_code.toString();
+        const std::string ecu_code_str = (*hw_id).ToString() + ":" + installation_res.result_code.toString();
         result_code_err_str += (result_code_err_str != "" ? "|" : "") + ecu_code_str;
       }
     }
@@ -456,8 +453,8 @@ void SotaUptaneClient::computeDeviceInstallationResult(data::InstallationResult
       // installation on at least one of the ECUs has failed
       device_installation_result =
           data::InstallationResult(data::ResultCode(data::ResultCode::Numeric::kInstallFailed, result_code_err_str),
-                                   "Installation failed on at least one of ECUs");
-      raw_ir = "Installation failed on at least one of ECUs";
+                                   "Installation failed on one or more ECUs");
+      raw_ir = "Installation failed on one or more ECUs";
 
       break;
     }
@@ -993,12 +990,12 @@ result::Install SotaUptaneClient::uptaneInstall(const std::vector<Uptane::Target
     std::vector<Uptane::Target> primary_updates = findForEcu(updates, primary_ecu_serial);
 
     //   6 - send metadata to all the ECUs
-    // TODO: Handle individual ECU metadata failures and combine output into
-    // the device report as is done for installation.
-    data::InstallationResult metadata_res = sendMetadataToEcus(updates);
+    data::InstallationResult metadata_res;
+    std::string rr;
+    sendMetadataToEcus(updates, &metadata_res, &rr);
     if (!metadata_res.isSuccess()) {
       result.dev_report = std::move(metadata_res);
-      return std::make_tuple(result, "Secondary metadata verification failed");
+      return std::make_tuple(result, rr);
     }
 
     //   7 - send images to ECUs (deploy for OSTree)
@@ -1038,7 +1035,6 @@ result::Install SotaUptaneClient::uptaneInstall(const std::vector<Uptane::Target
 
     auto sec_reports = sendImagesToEcus(updates);
     result.ecu_reports.insert(result.ecu_reports.end(), sec_reports.begin(), sec_reports.end());
-    std::string rr;
     computeDeviceInstallationResult(&result.dev_report, &rr);
 
     return std::make_tuple(result, rr);
@@ -1234,37 +1230,56 @@ data::InstallationResult SotaUptaneClient::rotateSecondaryRoot(Uptane::Repositor
 }
 
 // TODO: the function blocks until it updates all the Secondaries. Consider non-blocking operation.
-data::InstallationResult SotaUptaneClient::sendMetadataToEcus(const std::vector<Uptane::Target> &targets) {
+void SotaUptaneClient::sendMetadataToEcus(const std::vector<Uptane::Target> &targets, data::InstallationResult *result,
+                                          std::string *raw_installation_report) {
   data::InstallationResult final_result{data::ResultCode::Numeric::kOk, ""};
+  std::string result_code_err_str;
   for (const auto &target : targets) {
     for (const auto &ecu : target.ecus()) {
       const Uptane::EcuSerial ecu_serial = ecu.first;
+      const Uptane::HardwareIdentifier hw_id = ecu.second;
       auto sec = secondaries.find(ecu_serial);
       if (sec == secondaries.end()) {
         continue;
       }
 
-      /* Root rotation if necessary */
-      auto result = rotateSecondaryRoot(Uptane::RepositoryType::Director(), *(sec->second));
-      if (!result.isSuccess()) {
-        final_result = result;
-        continue;
-      }
-      result = rotateSecondaryRoot(Uptane::RepositoryType::Image(), *(sec->second));
-      if (!result.isSuccess()) {
-        final_result = result;
-        continue;
-      }
-      result = sec->second->putMetadata(target);
-      if (!result.isSuccess()) {
-        LOG_ERROR << "Sending metadata to " << sec->first << " failed: " << result.result_code << " "
-                  << result.description;
-        final_result = result;
+      data::InstallationResult local_result{data::ResultCode::Numeric::kOk, ""};
+      do {
+        /* Root rotation if necessary */
+        local_result = rotateSecondaryRoot(Uptane::RepositoryType::Director(), *(sec->second));
+        if (!local_result.isSuccess()) {
+          final_result = local_result;
+          break;
+        }
+        local_result = rotateSecondaryRoot(Uptane::RepositoryType::Image(), *(sec->second));
+        if (!local_result.isSuccess()) {
+          final_result = local_result;
+          break;
+        }
+        local_result = sec->second->putMetadata(target);
+      } while (false);
+      if (!local_result.isSuccess()) {
+        LOG_ERROR << "Sending metadata to " << sec->first << " failed: " << local_result.result_code << " "
+                  << local_result.description;
+        const std::string ecu_code_str = hw_id.ToString() + ":" + local_result.result_code.toString();
+        result_code_err_str += (result_code_err_str != "" ? "|" : "") + ecu_code_str;
       }
     }
   }
 
-  return final_result;
+  if (!result_code_err_str.empty()) {
+    // Sending the metadata to at least one of the ECUs has failed.
+    final_result =
+        data::InstallationResult(data::ResultCode(data::ResultCode::Numeric::kVerificationFailed, result_code_err_str),
+                                 "Sending metadata to one or more ECUs failed");
+    if (raw_installation_report != nullptr) {
+      *raw_installation_report = "Sending metadata to one or more ECUs failed";
+    }
+  }
+
+  if (result != nullptr) {
+    *result = final_result;
+  }
 }
 
 std::future<data::InstallationResult> SotaUptaneClient::sendFirmwareAsync(SecondaryInterface &secondary,

src/libaktualizr/primary/sotauptaneclient.h

@@ -97,6 +97,7 @@ class SotaUptaneClient {
  private:
   FRIEND_TEST(Aktualizr, FullNoUpdates);
   FRIEND_TEST(Aktualizr, DeviceInstallationResult);
+  FRIEND_TEST(Aktualizr, DeviceInstallationResultMetadata);
   FRIEND_TEST(Aktualizr, FullMultipleSecondaries);
   FRIEND_TEST(Aktualizr, CheckNoUpdates);
   FRIEND_TEST(Aktualizr, DownloadWithUpdates);
@@ -142,7 +143,8 @@ class SotaUptaneClient {
   bool waitSecondariesReachable(const std::vector<Uptane::Target> &updates);
   void storeInstallationFailure(const data::InstallationResult &result);
   data::InstallationResult rotateSecondaryRoot(Uptane::RepositoryType repo, SecondaryInterface &secondary);
-  data::InstallationResult sendMetadataToEcus(const std::vector<Uptane::Target> &targets);
+  void sendMetadataToEcus(const std::vector<Uptane::Target> &targets, data::InstallationResult *result,
+                          std::string *raw_installation_report);
   std::future<data::InstallationResult> sendFirmwareAsync(SecondaryInterface &secondary, const Uptane::Target &target);
   std::vector<result::Install::EcuReport> sendImagesToEcus(const std::vector<Uptane::Target> &targets);
 

src/libaktualizr/uptane/uptane_delegation_test.cc

@@ -90,12 +90,13 @@ TEST(Delegation, Basic) {
 TEST(Delegation, RevokeAfterCheckUpdates) {
   for (auto generate_fun : {delegation_basic, delegation_nested}) {
     TemporaryDirectory temp_dir;
+    auto http = std::make_shared<HttpFakeDelegation>(temp_dir.Path());
+    Config conf = UptaneTestCommon::makeTestConfig(temp_dir, http->tls_server);
+    auto storage = INvStorage::newStorage(conf.storage);
+
     auto delegation_path = temp_dir.Path() / "delegation_test";
-    generate_fun(delegation_path, false);
     {
-      auto http = std::make_shared<HttpFakeDelegation>(temp_dir.Path());
-      Config conf = UptaneTestCommon::makeTestConfig(temp_dir, http->tls_server);
-      auto storage = INvStorage::newStorage(conf.storage);
+      generate_fun(delegation_path, false);
       UptaneTestCommon::TestAktualizr aktualizr(conf, storage, http);
       aktualizr.Initialize();
 
@@ -106,9 +107,6 @@ TEST(Delegation, RevokeAfterCheckUpdates) {
     // Revoke delegation after CheckUpdates() and test if we can properly handle it.
     {
       generate_fun(delegation_path, true);
-      auto http = std::make_shared<HttpFakeDelegation>(temp_dir.Path());
-      Config conf = UptaneTestCommon::makeTestConfig(temp_dir, http->tls_server);
-      auto storage = INvStorage::newStorage(conf.storage);
       UptaneTestCommon::TestAktualizr aktualizr(conf, storage, http);
       aktualizr.Initialize();
 
@@ -130,12 +128,13 @@ TEST(Delegation, RevokeAfterCheckUpdates) {
 TEST(Delegation, RevokeAfterDownload) {
   for (auto generate_fun : {delegation_basic, delegation_nested}) {
     TemporaryDirectory temp_dir;
+    auto http = std::make_shared<HttpFakeDelegation>(temp_dir.Path());
+    Config conf = UptaneTestCommon::makeTestConfig(temp_dir, http->tls_server);
+    auto storage = INvStorage::newStorage(conf.storage);
+
     auto delegation_path = temp_dir.Path() / "delegation_test";
-    generate_fun(delegation_path, false);
     {
-      auto http = std::make_shared<HttpFakeDelegation>(temp_dir.Path());
-      Config conf = UptaneTestCommon::makeTestConfig(temp_dir, http->tls_server);
-      auto storage = INvStorage::newStorage(conf.storage);
+      generate_fun(delegation_path, false);
       UptaneTestCommon::TestAktualizr aktualizr(conf, storage, http);
       aktualizr.Initialize();
 
@@ -148,10 +147,6 @@ TEST(Delegation, RevokeAfterDownload) {
     // Revoke delegation after Download() and test if we can properly handle it
     {
       generate_fun(delegation_path, true);
-
-      auto http = std::make_shared<HttpFakeDelegation>(temp_dir.Path());
-      Config conf = UptaneTestCommon::makeTestConfig(temp_dir, http->tls_server);
-      auto storage = INvStorage::newStorage(conf.storage);
       UptaneTestCommon::TestAktualizr aktualizr(conf, storage, http);
       aktualizr.Initialize();
 
@@ -173,12 +168,13 @@ TEST(Delegation, RevokeAfterDownload) {
 TEST(Delegation, RevokeAfterInstall) {
   for (auto generate_fun : {delegation_basic, delegation_nested}) {
     TemporaryDirectory temp_dir;
+    auto http = std::make_shared<HttpFakeDelegation>(temp_dir.Path());
+    Config conf = UptaneTestCommon::makeTestConfig(temp_dir, http->tls_server);
+    auto storage = INvStorage::newStorage(conf.storage);
+
     auto delegation_path = temp_dir.Path() / "delegation_test";
-    generate_fun(delegation_path, false);
     {
-      auto http = std::make_shared<HttpFakeDelegation>(temp_dir.Path());
-      Config conf = UptaneTestCommon::makeTestConfig(temp_dir, http->tls_server);
-      auto storage = INvStorage::newStorage(conf.storage);
+      generate_fun(delegation_path, false);
       UptaneTestCommon::TestAktualizr aktualizr(conf, storage, http);
       aktualizr.Initialize();
 
@@ -196,10 +192,6 @@ TEST(Delegation, RevokeAfterInstall) {
     // Revoke delegation after Install() and test if can properly CheckUpdates again
     {
       generate_fun(delegation_path, true);
-
-      auto http = std::make_shared<HttpFakeDelegation>(temp_dir.Path());
-      Config conf = UptaneTestCommon::makeTestConfig(temp_dir, http->tls_server);
-      auto storage = INvStorage::newStorage(conf.storage);
       UptaneTestCommon::TestAktualizr aktualizr(conf, storage, http);
       aktualizr.Initialize();
 

src/libaktualizr/uptane/uptane_network_test.cc

@@ -187,7 +187,6 @@ TEST(UptaneNetwork, LogConnectivityRestored) {
   config.provision.primary_ecu_hardware_id = "primary_hw";
   config.storage.path = temp_dir.Path();
   config.tls.server = http->tls_server;
-  UptaneTestCommon::addDefaultSecondary(config, temp_dir, "secondary_ecu_serial", "secondary_hw");
 
   auto storage = INvStorage::newStorage(config.storage);
   auto up = std_::make_unique<UptaneTestCommon::TestUptaneClient>(config, storage, http);

src/libaktualizr/uptane/uptane_serial_test.cc

@@ -80,16 +80,15 @@ TEST(Uptane, ReloadSerial) {
   EcuSerials ecu_serials_1;
   EcuSerials ecu_serials_2;
 
+  Config conf("tests/config/basic.toml");
+  conf.storage.path = temp_dir.Path();
+  conf.provision.primary_ecu_serial = "";
+  UptaneTestCommon::addDefaultSecondary(conf, temp_dir, "", "secondary_hardware", false);
+
   // Initialize. Should store new serials.
   {
-    Config conf("tests/config/basic.toml");
-    conf.storage.path = temp_dir.Path();
-    conf.provision.primary_ecu_serial = "";
-
     auto storage = INvStorage::newStorage(conf.storage);
     auto http = std::make_shared<HttpFake>(temp_dir.Path());
-
-    UptaneTestCommon::addDefaultSecondary(conf, temp_dir, "", "secondary_hardware", false);
     auto uptane_client = std_::make_unique<UptaneTestCommon::TestUptaneClient>(conf, storage, http);
 
     ASSERT_NO_THROW(uptane_client->initialize());
@@ -102,13 +101,8 @@ TEST(Uptane, ReloadSerial) {
   // Keep storage directory, but initialize new objects. Should load existing
   // serials.
   {
-    Config conf("tests/config/basic.toml");
-    conf.storage.path = temp_dir.Path();
-    conf.provision.primary_ecu_serial = "";
-
     auto storage = INvStorage::newStorage(conf.storage);
     auto http = std::make_shared<HttpFake>(temp_dir.Path());
-    UptaneTestCommon::addDefaultSecondary(conf, temp_dir, "", "secondary_hardware", false);
     auto uptane_client = std_::make_unique<UptaneTestCommon::TestUptaneClient>(conf, storage, http);
 
     ASSERT_NO_THROW(uptane_client->initialize());

src/libaktualizr/uptane/uptane_test.cc

@@ -210,15 +210,16 @@ TEST(Uptane, PutManifest) {
   Config config = config_common();
   config.storage.path = temp_dir.Path();
   boost::filesystem::copy_file("tests/test_data/cred.zip", (temp_dir / "cred.zip").string());
-  boost::filesystem::copy_file("tests/test_data/firmware.txt", (temp_dir / "firmware.txt").string());
-  boost::filesystem::copy_file("tests/test_data/firmware_name.txt", (temp_dir / "firmware_name.txt").string());
   config.provision.provision_path = temp_dir / "cred.zip";
   config.provision.mode = ProvisionMode::kSharedCred;
   config.uptane.director_server = http->tls_server + "/director";
   config.uptane.repo_server = http->tls_server + "/repo";
   config.provision.primary_ecu_serial = "testecuserial";
   config.pacman.type = PACKAGE_MANAGER_NONE;
-  UptaneTestCommon::addDefaultSecondary(config, temp_dir, "secondary_ecu_serial", "secondary_hardware");
+  Primary::VirtualSecondaryConfig sec_config =
+      UptaneTestCommon::addDefaultSecondary(config, temp_dir, "secondary_ecu_serial", "secondary_hardware");
+  boost::filesystem::copy_file("tests/test_data/firmware.txt", sec_config.firmware_path);
+  boost::filesystem::copy_file("tests/test_data/firmware_name.txt", sec_config.target_name_path);
 
   auto storage = INvStorage::newStorage(config.storage);
 

src/virtual_secondary/virtualsecondary.cc

@@ -53,6 +53,10 @@ void VirtualSecondaryConfig::dump(const boost::filesystem::path& file_full_path)
   json_config["metadata_path"] = metadata_path.string();
 
   Json::Value root;
+  // Append to the config file if it already exists.
+  if (boost::filesystem::exists(file_full_path)) {
+    root = Utils::parseJSONFile(file_full_path);
+  }
   root[Type].append(json_config);
 
   Json::StreamWriterBuilder json_bwriter;