OTA-2481/2488: Get rid of SecondaryFactory and VirtualSecondary… (#1241)

OTA-2481/2488: Get rid of SecondaryFactory and VirtualSecondary refactoring

src/CMakeLists.txt

@@ -3,6 +3,7 @@ if(BUILD_WITH_CODE_COVERAGE)
 endif()
 
 add_subdirectory("libaktualizr")
+add_subdirectory("virtual_secondary")
 add_subdirectory("sota_tools")
 add_subdirectory("aktualizr_primary")
 add_subdirectory("libaktualizr-posix")

src/aktualizr_secondary/aktualizr_secondary.cc

@@ -9,13 +9,11 @@
 #endif
 #include "socket_activation/socket_activation.h"
 #include "socket_server.h"
-#include "uptane/secondaryfactory.h"
 #include "utilities/utils.h"
 
 class SecondaryAdapter : public Uptane::SecondaryInterface {
  public:
-  SecondaryAdapter(const Uptane::SecondaryConfig& sconfig_in, AktualizrSecondary& sec)
-      : SecondaryInterface(sconfig_in), secondary(sec) {}
+  SecondaryAdapter(AktualizrSecondary& sec) : secondary(sec) {}
   ~SecondaryAdapter() override = default;
 
   Uptane::EcuSerial getSerial() override { return secondary.getSerialResp(); }
@@ -36,8 +34,7 @@ class SecondaryAdapter : public Uptane::SecondaryInterface {
 AktualizrSecondary::AktualizrSecondary(const AktualizrSecondaryConfig& config,
                                        const std::shared_ptr<INvStorage>& storage)
     : AktualizrSecondaryCommon(config, storage),
-      socket_server_(std_::make_unique<SecondaryAdapter>(Uptane::SecondaryConfig(), *this),
-                     SocketFromSystemdOrPort(config.network.port)) {
+      socket_server_(std_::make_unique<SecondaryAdapter>(*this), SocketFromSystemdOrPort(config.network.port)) {
   // note: we don't use TlsConfig here and supply the default to
   // KeyManagerConf. Maybe we should figure a cleaner way to do that
   // (split KeyManager?)

src/aktualizr_secondary/update_test.cc

@@ -13,8 +13,7 @@ std::string sysroot;
 
 class ShortCircuitSecondary : public Uptane::SecondaryInterface {
  public:
-  ShortCircuitSecondary(const Uptane::SecondaryConfig& sconfig_in, AktualizrSecondary& sec)
-      : SecondaryInterface(sconfig_in), secondary(sec) {}
+  ShortCircuitSecondary(AktualizrSecondary& sec) : secondary(sec) {}
   ~ShortCircuitSecondary() override = default;
 
   Uptane::EcuSerial getSerial() override { return secondary.getSerialResp(); }
@@ -42,8 +41,7 @@ TEST(aktualizr_secondary_protocol, DISABLED_manual_update) {
   AktualizrSecondary as(config, storage);
 
   // secondary interface
-  Uptane::SecondaryConfig config_iface;
-  ShortCircuitSecondary sec_iface{config_iface, as};
+  ShortCircuitSecondary sec_iface{as};
 
   // storage
   TemporaryDirectory temp_dir;

src/hmi_stub/main.cc

@@ -14,7 +14,6 @@
 #include "config/config.h"
 #include "logging/logging.h"
 #include "primary/aktualizr.h"
-#include "uptane/secondaryfactory.h"
 #include "utilities/utils.h"
 
 namespace bpo = boost::program_options;

src/libaktualizr-posix/ipuptanesecondary.h

@@ -19,9 +19,6 @@ class IpUptaneSecondary : public SecondaryInterface {
   explicit IpUptaneSecondary(const std::string& address, unsigned short port, EcuSerial serial,
                              HardwareIdentifier hw_id, PublicKey pub_key);
 
-  // what this method for ? Looks like should be removed out of SecondaryInterface
-  void Initialize() override{};
-
   // It looks more natural to return const EcuSerial& and const Uptane::HardwareIdentifier&
   // and they should be 'const' methods
   EcuSerial getSerial() /*const*/ override { return serial_; };

src/libaktualizr/CMakeLists.txt

@@ -41,3 +41,6 @@ endif (BUILD_ISOTP)
 
 target_include_directories(aktualizr_static_lib PUBLIC
     $<TARGET_PROPERTY:package_manager,INCLUDE_DIRECTORIES>)
+
+# To be removed once the refactoring is completed
+target_link_libraries(aktualizr_static_lib virtual_secondary)

src/libaktualizr/primary/CMakeLists.txt

@@ -12,9 +12,11 @@ set(HEADERS aktualizr.h
 
 
 add_library(primary OBJECT ${SOURCES})
+target_include_directories(primary PUBLIC ${PROJECT_SOURCE_DIR}/src/virtual_secondary)
 
 add_aktualizr_test(NAME aktualizr SOURCES aktualizr_test.cc PROJECT_WORKING_DIRECTORY ARGS ${PROJECT_BINARY_DIR}/uptane_repos)
 add_dependencies(t_aktualizr uptane_repo_full_no_correlation_id)
+target_link_libraries(t_aktualizr virtual_secondary)
 
 if (BUILD_OSTREE)
     add_aktualizr_test(NAME aktualizr_fullostree SOURCES aktualizr_fullostree_test.cc PROJECT_WORKING_DIRECTORY ARGS $<TARGET_FILE:aktualizr-repo> ${PROJECT_BINARY_DIR}/ostree_repo)
@@ -31,5 +33,9 @@ add_aktualizr_test(NAME emptytargets SOURCES empty_targets_test.cc PROJECT_WORKI
 add_aktualizr_test(NAME device_cred_prov SOURCES device_cred_prov_test.cc PROJECT_WORKING_DIRECTORY)
 set_tests_properties(test_device_cred_prov PROPERTIES LABELS "crypto")
 
+add_aktualizr_test(NAME uptane_key SOURCES uptane_key_test.cc PROJECT_WORKING_DIRECTORY)
+set_tests_properties(test_uptane_key PROPERTIES LABELS "crypto")
+target_include_directories(t_uptane_key PUBLIC ${PROJECT_SOURCE_DIR}/src/virtual_secondary)
+target_link_libraries(t_uptane_key virtual_secondary)
 
 aktualizr_source_file_checks(${SOURCES} ${HEADERS} ${TEST_SOURCES})

src/libaktualizr/primary/aktualizr_test.cc

@@ -14,9 +14,9 @@
 #include "primary/aktualizr.h"
 #include "primary/events.h"
 #include "primary/sotauptaneclient.h"
-#include "uptane/secondaryfactory.h"
 #include "uptane_test_common.h"
 #include "utilities/utils.h"
+#include "virtualsecondary.h"
 
 #include "utilities/fault_injection.h"
 
@@ -136,7 +136,7 @@ TEST(Aktualizr, AddSecondary) {
 
   Uptane::SecondaryConfig ecu_config = virtual_configuration(temp_dir.Path());
 
-  aktualizr.AddSecondary(Uptane::SecondaryFactory::makeSecondary(ecu_config));
+  aktualizr.AddSecondary(std::make_shared<Uptane::VirtualSecondary>(ecu_config));
 
   aktualizr.Initialize();
 
@@ -156,7 +156,7 @@ TEST(Aktualizr, AddSecondary) {
   EXPECT_EQ(expected_ecus.size(), 0);
 
   ecu_config.ecu_serial = "ecuserial4";
-  auto sec4 = Uptane::SecondaryFactory::makeSecondary(ecu_config);
+  auto sec4 = std::make_shared<Uptane::VirtualSecondary>(ecu_config);
   EXPECT_THROW(aktualizr.AddSecondary(sec4), std::logic_error);
 }
 
@@ -180,7 +180,7 @@ TEST(Aktualizr, DeviceInstallationResult) {
 
   Uptane::SecondaryConfig ecu_config = virtual_configuration(temp_dir.Path());
 
-  aktualizr.AddSecondary(Uptane::SecondaryFactory::makeSecondary(ecu_config));
+  aktualizr.AddSecondary(std::make_shared<Uptane::VirtualSecondary>(ecu_config));
 
   aktualizr.Initialize();
 

src/libaktualizr/primary/initializer.cc

@@ -252,10 +252,6 @@ Initializer::Initializer(
       return;
     }
 
-    for (auto it = secondary_info_.begin(); it != secondary_info_.end(); ++it) {
-      it->second->Initialize();
-    }
-
     // TODO: acknowledge on server _before_ setting the flag
     storage_->storeEcuRegistered();
     success_ = true;

src/libaktualizr/primary/sotauptaneclient.cc

@@ -13,9 +13,9 @@
 #include "package_manager/packagemanagerfactory.h"
 #include "uptane/exceptions.h"
 #include "uptane/secondaryconfig.h"
-#include "uptane/secondaryfactory.h"
 #include "utilities/fault_injection.h"
 #include "utilities/utils.h"
+#include "virtualsecondary.h"
 
 static void report_progress_cb(event::Channel *channel, const Uptane::Target &target, const std::string &description,
                                unsigned int progress) {
@@ -32,8 +32,17 @@ std::shared_ptr<SotaUptaneClient> SotaUptaneClient::newDefaultClient(
   std::shared_ptr<Bootloader> bootloader_in = std::make_shared<Bootloader>(config_in.bootloader, *storage_in);
   std::shared_ptr<ReportQueue> report_queue_in = std::make_shared<ReportQueue>(config_in, http_client_in);
 
-  return std::make_shared<SotaUptaneClient>(config_in, storage_in, http_client_in, bootloader_in, report_queue_in,
-                                            events_channel_in);
+  auto sota_uptane_client = std::make_shared<SotaUptaneClient>(config_in, storage_in, http_client_in, bootloader_in,
+                                                               report_queue_in, events_channel_in);
+
+  // This factory method is used just for manual testing of VirtualSecondaries, so calling VirtualSecondary ctor
+  // directly here is fine for now until the virtual secondary implementation is moved out of libaktualizr
+  std::vector<Uptane::SecondaryConfig>::const_iterator it;
+  for (it = config_in.uptane.secondary_configs.begin(); it != config_in.uptane.secondary_configs.end(); ++it) {
+    sota_uptane_client->addSecondary(std::make_shared<Uptane::VirtualSecondary>(*it));
+  }
+
+  return sota_uptane_client;
 }
 
 std::shared_ptr<SotaUptaneClient> SotaUptaneClient::newTestClient(Config &config_in,
@@ -42,8 +51,17 @@ std::shared_ptr<SotaUptaneClient> SotaUptaneClient::newTestClient(Config &config
                                                                   std::shared_ptr<event::Channel> events_channel_in) {
   std::shared_ptr<Bootloader> bootloader_in = std::make_shared<Bootloader>(config_in.bootloader, *storage_in);
   std::shared_ptr<ReportQueue> report_queue_in = std::make_shared<ReportQueue>(config_in, http_client_in);
-  return std::make_shared<SotaUptaneClient>(config_in, storage_in, http_client_in, bootloader_in, report_queue_in,
-                                            events_channel_in);
+  auto sota_uptane_client = std::make_shared<SotaUptaneClient>(config_in, storage_in, http_client_in, bootloader_in,
+                                                               report_queue_in, events_channel_in);
+
+  // This factory method is used just for automated tests, so calling VirtualSecondary ctor directly here is fine
+  // for now until the virtual secondary implementation is moved out of libaktualizr
+  std::vector<Uptane::SecondaryConfig>::const_iterator it;
+  for (it = config_in.uptane.secondary_configs.begin(); it != config_in.uptane.secondary_configs.end(); ++it) {
+    sota_uptane_client->addSecondary(std::make_shared<Uptane::VirtualSecondary>(*it));
+  }
+
+  return sota_uptane_client;
 }
 
 SotaUptaneClient::SotaUptaneClient(Config &config_in, std::shared_ptr<INvStorage> storage_in,
@@ -66,12 +84,6 @@ SotaUptaneClient::SotaUptaneClient(Config &config_in, std::shared_ptr<INvStorage
   if (package_manager_->imageUpdated()) {
     bootloader->setBootOK();
   }
-
-  std::vector<Uptane::SecondaryConfig>::const_iterator it;
-  for (it = config.uptane.secondary_configs.begin(); it != config.uptane.secondary_configs.end(); ++it) {
-    auto sec = Uptane::SecondaryFactory::makeSecondary(*it);
-    addSecondary(sec);
-  }
 }
 
 SotaUptaneClient::~SotaUptaneClient() { conn.disconnect(); }

src/libaktualizr/uptane/uptane_key_test.cc → src/libaktualizr/primary/uptane_key_test.cc

@@ -10,10 +10,10 @@
 
 #include "httpfake.h"
 #include "logging/logging.h"
+#include "managedsecondary.h"
 #include "primary/reportqueue.h"
 #include "primary/sotauptaneclient.h"
 #include "storage/invstorage.h"
-#include "uptane/managedsecondary.h"
 #include "uptane/secondaryconfig.h"
 #include "uptane/uptanerepository.h"
 
@@ -87,7 +87,6 @@ class UptaneKey_Check_Test {
     // Verify that each secondary has valid keys.
     std::map<Uptane::EcuSerial, std::shared_ptr<Uptane::SecondaryInterface> >::iterator it;
     for (it = sota_client->secondaries.begin(); it != sota_client->secondaries.end(); it++) {
-      EXPECT_TRUE(it->second->sconfig.secondary_type == Uptane::SecondaryType::kVirtual);
       std::shared_ptr<Uptane::ManagedSecondary> managed =
           boost::polymorphic_pointer_downcast<Uptane::ManagedSecondary>(it->second);
       std::string public_key;

src/libaktualizr/uptane/CMakeLists.txt

@@ -1,32 +1,26 @@
 set(SOURCES
     fetcher.cc
     iterator.cc
-    managedsecondary.cc
     metawithkeys.cc
     partialverificationsecondary.cc
     role.cc
     root.cc
-    secondaryfactory.cc
     tuf.cc
     uptanerepository.cc
     directorrepository.cc
-    imagesrepository.cc
-    virtualsecondary.cc)
+    imagesrepository.cc)
 
 set(HEADERS
     exceptions.h
     fetcher.h
     iterator.h
-    managedsecondary.h
     partialverificationsecondary.h
     secondaryconfig.h
-    secondaryfactory.h
     secondaryinterface.h
     tuf.h
     uptanerepository.h
     directorrepository.h
-    imagesrepository.h
-    virtualsecondary.h)
+    imagesrepository.h)
 
 
 add_library(uptane OBJECT ${SOURCES})
@@ -57,14 +51,9 @@ add_aktualizr_test(NAME uptane_delegation SOURCES uptane_delegation_test.cc PROJ
 add_dependencies(t_uptane_delegation aktualizr-repo)
 set_tests_properties(test_uptane_delegation PROPERTIES LABELS "crypto")
 
-add_aktualizr_test(NAME uptane_key SOURCES uptane_key_test.cc PROJECT_WORKING_DIRECTORY)
-set_tests_properties(test_uptane_key PROPERTIES LABELS "crypto")
-
 add_aktualizr_test(NAME uptane_network SOURCES uptane_network_test.cc PROJECT_WORKING_DIRECTORY)
 set_tests_properties(test_uptane_network PROPERTIES LABELS "crypto")
 
-add_aktualizr_test(NAME uptane_secondary SOURCES uptane_secondary_test.cc PROJECT_WORKING_DIRECTORY)
-
 add_aktualizr_test(NAME uptane_serial SOURCES uptane_serial_test.cc ARGS ${PROJECT_BINARY_DIR}
                    PROJECT_WORKING_DIRECTORY)
 

src/libaktualizr/uptane/isotpsecondary.cc

@@ -33,7 +33,7 @@ enum class IsoTpUptaneMesType {
 namespace Uptane {
 
 IsoTpSecondary::IsoTpSecondary(const SecondaryConfig& sconfig_in)
-    : SecondaryInterface(sconfig_in), conn(sconfig.can_iface, LIBUPTINY_ISOTP_PRIMARY_CANID, sconfig_in.can_id) {}
+    : conn(sconfig_in.can_iface, LIBUPTINY_ISOTP_PRIMARY_CANID, sconfig_in.can_id) {}
 
 EcuSerial IsoTpSecondary::getSerial() {
   std::string out;

src/libaktualizr/uptane/isotpsecondary.h

@@ -2,6 +2,7 @@
 #define UPTANE_ISOTPSECONDARY_H_
 
 #include "isotp_conn/isotp_conn.h"
+#include "secondaryconfig.h"
 #include "secondaryinterface.h"
 
 namespace Uptane {

src/libaktualizr/uptane/partialverificationsecondary.cc

@@ -14,8 +14,8 @@
 
 namespace Uptane {
 
-PartialVerificationSecondary::PartialVerificationSecondary(const SecondaryConfig &sconfig_in)
-    : SecondaryInterface(sconfig_in), root_(Root::Policy::kAcceptAll) {
+PartialVerificationSecondary::PartialVerificationSecondary(SecondaryConfig sconfig_in)
+    : sconfig(std::move(sconfig_in)), root_(Root::Policy::kAcceptAll) {
   boost::filesystem::create_directories(sconfig.metadata_path);
 
   // FIXME Probably we need to generate keys on the secondary

src/libaktualizr/uptane/partialverificationsecondary.h

@@ -15,14 +15,15 @@ namespace Uptane {
 
 class PartialVerificationSecondary : public SecondaryInterface {
  public:
-  explicit PartialVerificationSecondary(const SecondaryConfig& sconfig_in);
+  explicit PartialVerificationSecondary(SecondaryConfig sconfig_in);
 
   EcuSerial getSerial() override {
     if (!sconfig.ecu_serial.empty()) {
       return Uptane::EcuSerial(sconfig.ecu_serial);
     }
     return Uptane::EcuSerial(public_key_.KeyId());
   }
+  Uptane::HardwareIdentifier getHwId() override { return Uptane::HardwareIdentifier(sconfig.ecu_hardware_id); }
   PublicKey getPublicKey() override { return public_key_; }
 
   bool putMetadata(const RawMetaPack& meta) override;
@@ -36,6 +37,7 @@ class PartialVerificationSecondary : public SecondaryInterface {
   void storeKeys(const std::string& public_key, const std::string& private_key);
   bool loadKeys(std::string* public_key, std::string* private_key);
 
+  SecondaryConfig sconfig;
   Uptane::Root root_;
   PublicKey public_key_;
   std::string private_key_;

src/libaktualizr/uptane/secondaryconfig.h

@@ -16,8 +16,6 @@ enum class SecondaryType {
 
   kVirtual,  // Virtual secondary (in-process fake implementation).
 
-  kLegacy,  // Deprecated. Do not use.
-
   kIpUptane,  // Custom Uptane protocol over TCP/IP network
 
   kVirtualUptane,  // Partial UPTANE secondary implemented inside primary