OTA-2536: Preconfigure IpUptaneSecondary with IP address #1183

mike-sul · 2019-04-17T13:59:59Z

Tiny IpUptaneSecondary refactoring

to make it working with a fake package manager;
to get serial & hw-id from Secondary during initialization;

Pre-configure Secondary with IP addresses by using a json configuration file;
Initialize Aktualizr app with Secondaries.

Signed-off-by: Mike Sul [email protected]

OYTIS · 2019-04-17T14:44:25Z

This pull request introduces 1 alert when merging f5b19d3 into 99bfcf9 - view on LGTM.com

new alerts:

1 for FIXME comment

Comment posted by LGTM.com

codecov-io · 2019-04-17T15:25:55Z

Codecov Report

Merging #1183 into master will decrease coverage by 0.79%.
The diff coverage is 6.52%.

@@            Coverage Diff            @@
##           master    #1183     +/-   ##
=========================================
- Coverage   77.91%   77.12%   -0.8%     
=========================================
  Files         170      173      +3     
  Lines       10003    10114    +111     
=========================================
+ Hits         7794     7800      +6     
- Misses       2209     2314    +105

Impacted Files	Coverage Δ
src/libaktualizr-posix/ipsecondarydiscovery.cc	`78.57% <ø> (-0.74%)`	⬇️
src/libaktualizr/uptane/secondaryconfig.h	`100% <ø> (ø)`	⬆️
src/libaktualizr/config/config.h	`100% <ø> (ø)`	⬆️
.../aktualizr_secondary/aktualizr_secondary_common.cc	`88.23% <ø> (ø)`	⬆️
src/libaktualizr-posix/asn1/asn1_message.cc	`20.4% <0%> (ø)`	⬆️
src/libaktualizr/uptane/secondaryinterface.h	`83.33% <0%> (-16.67%)`	⬇️
src/aktualizr_secondary/aktualizr_secondary.cc	`21.87% <0%> (-0.47%)`	⬇️
src/libaktualizr-posix/asn1/asn1_message.h	`74.07% <0%> (-2.85%)`	⬇️
src/libaktualizr-posix/ipuptanesecondary.cc	`1.21% <0%> (+1.21%)`	⬆️
src/libaktualizr-posix/ipuptanesecondary.h	`0% <0%> (ø)`	⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3f0242...e37947f. Read the comment docs.

OYTIS · 2019-04-17T16:07:26Z

This pull request introduces 1 alert when merging 87325ea into 99bfcf9 - view on LGTM.com

new alerts:

1 for FIXME comment

Comment posted by LGTM.com

pattivacek

Looks pretty good so far. I haven't reviewed everything carefully yet, and the ASN1 stuff I'm too unfamiliar with to offer any substantial advice on.

pattivacek · 2019-04-17T16:24:44Z

src/aktualizr_primary/secondary.cc

+using SecondaryFactoryRegistry =
+    std::unordered_map<std::string, std::function<std::shared_ptr<Uptane::SecondaryInterface>(const SecondaryConfig&)>>;
+
+static SecondaryFactoryRegistry sec_factory_regisrty = {


Typo: sec_factory_registry.

pattivacek · 2019-04-17T16:28:19Z

src/libaktualizr-posix/asn1/messages/ipuptane_message.asn1

+  AKDiscoveryReqMes ::= SEQUENCE {
+    port INTEGER(0..65535),
+    ...
+  }


Not critical at all, but the indenting here is inconsistent.

I am not sure if I see it.

If you look at the surrounding lines on github, it is more apparent. I think it's a tab vs. spaces thing.

Yeah, a tab is used for indentation here while my editor inserts spaces on tap pressing.

pattivacek · 2019-04-17T16:32:16Z

src/libaktualizr-posix/ipuptanesecondary.h

+  explicit IpUptaneSecondary(const sockaddr_in& sock_addr, EcuSerial serial, HardwareIdentifier hw_id,
+                             PublicKey pub_key);
+
+  // what this method for ? Looks like should be removed out of SecondaryInterface


It's used by ManagedSecondary. It's possible that it could be merged into the constructor, but I think there used to be a good reason it was necessary before. It may not be necessary now. Not priority, though. We can look into that further once more of the refactoring is done.

pattivacek · 2019-04-17T16:33:48Z

src/libaktualizr-posix/ipuptanesecondary.h

+  void Initialize() override{};
+
+  // It looks more natural to return const EcuSerial& and const Uptane::HardwareIdentifier&
+  // and they should be 'const' methods


You might be right. In the past we didn't do that in case system calls or other non-const-friendly operations were required to implement these. We can re-evaluate that again soon.

pattivacek · 2019-04-17T16:35:12Z

src/libaktualizr/uptane/secondaryconfig.h

@@ -37,16 +37,18 @@ class SecondaryConfig {
  std::string ecu_public_key;
  KeyType key_type{KeyType::kRSA2048};

+  // FIXME: secondary config contains secondary specific params
+  // so introduction of any new type of secondary will require changes here/libaktualizr
+  // what we would like to avoid


Yep, this is not ideal. It might be a necessary evil for now, but we should try to remove them if possible. Not the top priority, though.

pattivacek · 2019-04-17T16:36:52Z

src/libaktualizr/uptane/secondaryinterface.h

@@ -20,21 +20,32 @@ namespace Uptane {

 class SecondaryInterface {
 public:
+  // This ctor should be removed as the secondary configuration SecondaryConfig
+  // is the secondaries's specific, see SecondaryConfig declaration


You are probably right. I think a lot of this was done to make SecondaryFactory work smoothly, and since we are removing that, we can get rid of some of the associated cruft along with it.

OK. I can try it in the next PRs since it leads to to many changes across the source code.

pattivacek · 2019-04-17T16:37:18Z

src/libaktualizr/uptane/secondaryinterface.h

  virtual void Initialize(){};  // optional step, called after device registration
+  // should be pure virtual, since the current implementation reads from the secondaries specific config


I think that makes sense.

I'll try in the next PR.

pattivacek · 2019-04-17T16:37:46Z

src/libaktualizr/uptane/secondaryinterface.h

  virtual Uptane::HardwareIdentifier getHwId() { return Uptane::HardwareIdentifier(sconfig.ecu_hardware_id); }
  virtual PublicKey getPublicKey() = 0;

+  // getSerial(), getHwId() and getPublicKey() can be moved to seperate interface
+  // since their usage pattern differ from the following methods' one


Separate interface? What do you mean?

Well, serial, hw_id and pub_key are constant properties of a secondary and can be determined/obtained before or during a secondary object instantiation, while all other methods represent RPC calls that actually transfer messages between Primary and Secondary. I don't think, that there is any practical need/benefit in breaking the current interface into two parts because of the aforementioned, but it might appear in the future.

pattivacek

A few more thoughts after I had more time to review. Still looking pretty good on the whole, I think.

pattivacek · 2019-04-18T07:28:08Z

src/aktualizr_primary/main.cc

@@ -42,6 +43,7 @@ bpo::variables_map parse_options(int argc, char *argv[]) {
      ("primary-ecu-serial", bpo::value<std::string>(), "serial number of primary ecu")
      ("primary-ecu-hardware-id", bpo::value<std::string>(), "hardware ID of primary ecu")
      ("secondary-configs-dir", bpo::value<boost::filesystem::path>(), "directory containing secondary ECU configuration files")
+      ("secondary-config-file", bpo::value<boost::filesystem::path>(), "secondary ECUs configuration file")


Just noting that we should get rid of secondary-configs-dir once we are able to. I think we have to leave both for now, but we should eventually convert virtual secondaries (and anything else still depending on the old factory mechanism) to use the new secondary-config-file.

Yeah, fully agree.

Could be helpful to have more different naming and avoid confusion in the meantime (haven't had a great idea though). Would it also make sense to disallow using both options at once?

I'll remove secondary-configs-dir soon (in one of the following PRs)

Ok. I haven't had time to look the details in depth but is the new format really providing the old functionality?
In particular, can virtual secondaries be still used? Some docs refer to them and they are used by qa and more for testing.

The plan is to adjust virtual secondaries implementation to the new configuration approach.

pattivacek · 2019-04-18T07:33:40Z

src/aktualizr_primary/secondary_config.cc

+
+    if (sec_cfg_factory_registry_.find(secondary_type) == sec_cfg_factory_registry_.end()) {
+      LOG_ERROR << "Unsupported type of sescondary config was found: `" << secondary_type
+                << "`. Ingoring it and continue with parsing of other secondary configs";


Typo: "Ignoring". And actually "continuing" would be better than "continue".

pattivacek · 2019-04-18T07:35:54Z

src/libaktualizr-posix/asn1/asn1_message.cc

  if (socket_fd < 0) {
    throw std::system_error(errno, std::system_category(), "socket");
  }
+  // what's the point in usage of SocketHandle


I have no idea.

I'll get rid of it in the next PR.

I guess it automatically closes the socket, once it's out of scope.

Yeah, your're right. Normally, such RAII objects imply object initialization/creation and deinitialization/deletion at ctor and dtor correspondingly. Also, the dtor's part should include shutdown and preferably verification if a socket was actually opened.

pattivacek · 2019-04-18T07:36:03Z

src/libaktualizr-posix/asn1/asn1_message.cc

-Asn1Message::Ptr Asn1Rpc(const Asn1Message::Ptr& tx, const struct sockaddr_storage& client) {
-  int socket_fd = socket(AF_INET6, SOCK_STREAM, 0);
+Asn1Message::Ptr Asn1Rpc(const Asn1Message::Ptr& tx, const struct sockaddr_in& server_sock_addr) {
+  int socket_fd = socket(AF_INET, SOCK_STREAM, 0);


AF_INET6 is no good?

Benefit of ipv6 usage is questionable in this case (local network within a car).

What are the drawbacks?

Well,

people get used to IPv4 addresses, so easier to edit a configuration file (::1 instead of 127.0.0.1 and IPv6 slightly longer

At the kernel level (IP stack implementation) slightly more work for packets processing

We consider IPUptaneSecondary as a reference implementation, so I tend to make it simple, unless there is a requirement to support IPv6

I actually have no strong feelings here. I'm fine with keeping it simple for now.

I don't really buy the performance considerations if not tied to a specific platform/system architecture. Disabling v6 support also seems easier than adding it after the fact to a v4 only code base.

That said, I would have hoped that we wouldn't have to worry about either v4 or v6 by using dual stack sockets. If that's not the case and makes development harder, then ok for dropping it.

It's worth keeping in mind this whole functionality is just meant to be a reference and demonstration; so it is perfectly acceptable to make a semi-arbitrary choice and just roll with it.

Fair point, I forgot that it will actually support both AF_INET and AF_INET6. Then I'm for leaving it as it was before :)

@patrickvacek sure, sounds reasonable

The previous version supported only AF_INET6 and it's used the 'discovery' mechanism to get IPv6 addresses. Since, we are going to remove the 'discovery' mechanism and input IP addresses in the config file then there is no need to stay with IPv6.
I prefer to choose either IPv4 or IPv6, adding support of both will require additional effort, but I am fine with either choices.

src/libaktualizr-posix/ipsecondarydiscovery.cc

pattivacek · 2019-04-18T07:38:35Z

src/libaktualizr-posix/ipuptanesecondary.cc


-  req->present(AKIpUptaneMes_PR_publicKeyReq);
+  EcuSerial serial{"serial"};
+  HardwareIdentifier hw_id{"hw_id"};


I'd suggest initializing these to empty strings. Otherwise it might be confusing in error conditions. Especially for testing, we often use dummy values that might be easily confused with these defaults.

I tried, but there is an input string length verification in EcuSerial ctor what doesn't allow me to specify an empty string.
Can I remove it as someone removed it for HardwareIdentifier ?

I think I remember running into that before, and somehow I was able to work around it. That seems less possible here. We'd need to do a quick check that removing that restriction won't open up any holes, but it's probably fine.

Looks like remove it is fine, at least tests passes on my local env.

I was fairly sure the tests would pass, but I mean we should look at where EcuSerials are instantiated and check if we need to add any new checks for non-emptiness in those places.

eu-siemann · 2019-04-18T09:35:04Z

src/aktualizr_primary/secondary.cc

+
+  for (auto& config : secondary_configs) {
+    try {
+      LOG_INFO << "Creating Secondary of type: " << config->type();


Actually LOG_XXX adds "\n" to the end of a message, so I am removing "\n" I added in other places.

huh, good to know

eu-siemann · 2019-04-18T10:07:46Z

src/aktualizr_secondary/socket_server.cc

@@ -61,6 +61,15 @@ void SocketServer::HandleOneConnection(int socket) {
    // Figure out what to do with the message
    Asn1Message::Ptr resp = Asn1Message::Empty();
    switch (msg->present()) {
+      case AKIpUptaneMes_PR_getInfoReq: {
+        std::cout << ">>>>>>>>>>>> Got Info request";


Debug leftover?

Signed-off-by: Mike Sul <[email protected]>

pattivacek

I think this looks fine unless there's still something you wanted to clean up in this PR.

mike-sul · 2019-04-18T14:15:35Z

I think this looks fine unless there's still something you wanted to clean up in this PR.

No, I am fine with it.

mike-sul requested review from pattivacek, eu-siemann and lbonn April 17, 2019 14:00

mike-sul force-pushed the feat/OTA-2536/preconfigure-ip-secondaries branch from f5b19d3 to 87325ea Compare April 17, 2019 15:25

pattivacek reviewed Apr 17, 2019

View reviewed changes

mike-sul force-pushed the feat/OTA-2536/preconfigure-ip-secondaries branch from 87325ea to 6aa63eb Compare April 17, 2019 18:18

pattivacek reviewed Apr 18, 2019

View reviewed changes

eu-siemann reviewed Apr 18, 2019

View reviewed changes

OTA-2536: Preconfigure IpUptaneSecondary with IP address

e37947f

Signed-off-by: Mike Sul <[email protected]>

mike-sul force-pushed the feat/OTA-2536/preconfigure-ip-secondaries branch from 6aa63eb to e37947f Compare April 18, 2019 11:15

pattivacek approved these changes Apr 18, 2019

View reviewed changes

pattivacek merged commit f5bb1cc into master Apr 18, 2019

pattivacek deleted the feat/OTA-2536/preconfigure-ip-secondaries branch April 18, 2019 14:21

		virtual void Initialize(){}; // optional step, called after device registration
		// should be pure virtual, since the current implementation reads from the secondaries specific config

OTA-2536: Preconfigure IpUptaneSecondary with IP address #1183

OTA-2536: Preconfigure IpUptaneSecondary with IP address #1183

Conversation

mike-sul commented Apr 17, 2019

OYTIS commented Apr 17, 2019

codecov-io commented Apr 17, 2019 • edited Loading

Codecov Report

OYTIS commented Apr 17, 2019

pattivacek left a comment

Choose a reason for hiding this comment

pattivacek Apr 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pattivacek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-sul Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eu-siemann Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-sul Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pattivacek left a comment

Choose a reason for hiding this comment

mike-sul commented Apr 18, 2019

codecov-io commented Apr 17, 2019 •

edited

Loading

pattivacek Apr 17, 2019 •

edited

Loading

mike-sul Apr 18, 2019 •

edited

Loading

eu-siemann Apr 18, 2019 •

edited

Loading

mike-sul Apr 18, 2019 •

edited

Loading