Fix/ota 3057/metadata matching #1258

pattivacek · 2019-07-17T09:02:10Z

Looks like a lot of changes, but almost all of it is just adapting existing tests to handle one big new change: hardware IDs must now match between Director and Image repo Target objects. This can be seen in tuf.cc.

Details: The Director sends a map of serials to HWIDs, but the Image repo sends us a vector of HWIDs. If the map and vector match, the comparison succeeds. Otherwise, we expect that every HWID in the map can be found in the vector. Added tests for all of these cases. Added explanations for the rest of the fields we use for why we don't check them. Added a requirement to aktualizr-repo that the "image" command must have a hardware ID so that we can properly populate the metadata.

~~This PR depends on advancedtelematic/tuf-test-vectors#56.~~

codecov-io · 2019-07-17T12:22:32Z

Codecov Report

❗ No coverage uploaded for pull request base (master@383dfc3). Click here to learn what that means.
The diff coverage is 94.04%.

@@            Coverage Diff            @@
##             master    #1258   +/-   ##
=========================================
  Coverage          ?   79.38%           
=========================================
  Files             ?      171           
  Lines             ?    10211           
  Branches          ?        0           
=========================================
  Hits              ?     8106           
  Misses            ?     2105           
  Partials          ?        0

Impacted Files	Coverage Δ
src/aktualizr_repo/image_repo.h	`100% <ø> (ø)`
src/aktualizr_repo/uptane_repo.h	`100% <ø> (ø)`
src/libaktualizr/primary/sotauptaneclient.h	`100% <ø> (ø)`
src/aktualizr_repo/director_repo.cc	`91.17% <100%> (ø)`
...libaktualizr/package_manager/packagemanagerfake.cc	`89.79% <100%> (ø)`
src/aktualizr_repo/uptane_repo.cc	`96.55% <100%> (ø)`
...tualizr/package_manager/packagemanagerinterface.cc	`89.87% <100%> (ø)`
src/aktualizr_repo/main.cc	`81.7% <100%> (ø)`
src/aktualizr_repo/image_repo.cc	`93.69% <100%> (ø)`
src/libaktualizr/storage/sqlstorage.cc	`74.74% <100%> (ø)`
... and 4 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 383dfc3...5320005. Read the comment docs.

lbonn · 2019-07-17T13:17:20Z

src/libaktualizr/uptane/tuf.cc

-
-  for (auto it = t.ecus_.begin(); it != t.ecus_.end(); ++it) {
-    os << it->first;
+  for (auto it = t.ecus_.cbegin(); it != t.ecus_.cend(); ++it) {


All these loops could use simpler for range loops:

for (const auto &ecu : t.ecus) { ... }

lbonn · 2019-07-17T13:28:05Z

src/libaktualizr/uptane/tuf.h

    if (filename_ != t2.filename_) {
      return false;
    }
    if (length_ != t2.length_) {
      return false;
    }

+    // If the HWID vector and ECU->HWID map match, we're good. Otherwise, assume


It's maybe a naive question, but maybe it's the time where it's too much to have all this logic in an overloaded comparison operator and instead have a dedicated function? The potential for misuse and confusion seems big.

Would you prefer to have operator== call that function, or to reserve operator== only for exact matches and use the new match function for Uptane purposes? I'm hoping the former, because otherwise I'd worry about accidentally doing the wrong thing (using equality instead of the matching function).

I imagined that == would not call that but then you're probably right. In this case it would be better to delete the comparison operator and only keep the method.
But I'm ready to debate that.

Deleting the comparison operator is an interesting idea, but at that point, aren't we back to just having the same thing, just named differently? What's the benefit?

As a matter of style it can be surprising to have this complicated business logic inside this operator with a benign name. And the objects are not really "equal", they "match" in some application sense, so I think it's stretching the sense a bit. Also, it makes it hard to grep for usage of this comparison.

It's why I would prefer this to have a scary name but maybe it's not really feasible or we can argue that we contain the usage effectively.

I see your points. I think if we delete the == operator then it's probably a reasonable way to go.

lbonn · 2019-07-17T13:30:35Z

src/libaktualizr/uptane/tuf_test.cc

+  ecu_map.insert({Uptane::EcuSerial("serial"), hwid});
+  Uptane::Target target1("abc", generateDirectorTarget("hash_good", 739, ecu_map));
+  Uptane::Target target2("abc", generateImagesTarget("hash_good", 739, hardwareIds));
+  EXPECT_TRUE(target1 == target2);


Should we also test target2 == target1?

Probably not a bad idea. They should be identical, but it would help prevent potential mistakes to future edits to the Target class.

Yes I would be more comfortable if all the tests checked the reflexivity.

lbonn · 2019-07-17T13:58:49Z

src/libaktualizr/storage/invstorage.cc

@@ -216,7 +216,8 @@ void INvStorage::FSSToSQLS(FSStorageRead& fs_storage, SQLStorage& sql_storage) {
 }

 bool INvStorage::fsReadInstalledVersions(const boost::filesystem::path& filename,
-                                         std::vector<Uptane::Target>* installed_versions, size_t* current_version) {
+                                         std::vector<Uptane::Target>* installed_versions, size_t* current_version,


Is it intentional that importInstalledVersions was not changed to adopt these changes? It's a code part that should get more hits than FSStorageRead::loadInstalledVersions which is only used for legacy migrations.

Sorta. This change was a bit of a hack to get tests to work. importInstalledVersions should only run on a primary, and there is already logic to assume it's for the primary when reading the installation versions in the function that converts FS to SQL storage. However, the tests had to do some fakery to test that effectively, so I had to force the serial/hwid. Would you prefer that importInstalledVersions also used it? In that case, I probably wouldn't need the parameters to be optional; I think I could make them a requirement.

That's a bit annoying to have this almost dead code to make the tests work indeed. Then it should maybe explained somewhere with a good comment (maybe I've missed it?), as I found it a bit confusing in this state.

There is no comment at present. The other option is to do some more hackery in the test itself, which is maybe safer, but makes the test slightly less meaningful. It's kind of ugly no matter how you slice it, but I'm open to advice.

mike-sul · 2019-07-18T06:51:33Z

src/aktualizr_repo/image_repo.cc

  boost::filesystem::path repo_dir(path_ / "repo/image");

  boost::filesystem::path targets_path =
      delegation ? ((repo_dir / "delegations") / delegation.name).string() + ".json" : repo_dir / "targets.json";
  Json::Value targets = Utils::parseJSONFile(targets_path)["signed"];
+  // TODO: support multiple hardware IDs.
+  target["custom"]["hardwareIds"][0] = hardware_id;


I am just wondering why it's in "custom" section which sounds like a bucket for optional params while hardware-ID looks like a mandatory param.

I couldn't really tell you, but that's how Uptane specifies it. See https://uptane.github.io/uptane-standard/uptane-standard.html#custom-metadata-about-images. I think the point is that it is not a hard requirement, but rather a SHOULD. If we were to want to change it, first we'd have to convince the backend team, but I think it's fine as is, even though it is admittedly confusing.

mike-sul · 2019-07-18T06:57:27Z

src/aktualizr_repo/main.cc

-        std::string hwid = vm["hwid"].as<std::string>();
-        std::string serial = vm["serial"].as<std::string>();
+        const std::string targetname = vm["targetname"].as<std::string>();
+        const std::string hwid = vm["hwid"].as<std::string>();


Looks like line #154 and #107 are identical

Indeed. However, they are two different subcommands of aktualizr-repo, and other subcommands don't require it, so the redundancy is probably the cleanest solution.

mike-sul · 2019-07-18T07:05:48Z

src/libaktualizr/package_manager/packagemanagerfake_test.cc

@@ -26,7 +26,8 @@ TEST(PackageManagerFake, FinalizeAfterReboot) {

  PackageManagerFake fakepm(config.pacman, storage, bootloader, nullptr);

-  Uptane::Target target("pkg", {Uptane::Hash(Uptane::Hash::Type::kSha256, "hash")}, 1, "");
+  std::map<Uptane::EcuSerial, Uptane::HardwareIdentifier> primary_ecu;


It's not exactly related to this specific PR rather to the code base in general - this map can be found in many places of the code and effectively represents an ID of ECU, just thinking if it makes sense to define corresponding types, e.g. class ECU { class/using/typedef ID ..... so it can be referenced as Uptane::ECU::ID ...

Not a bad thought. I'll look into it.

mike-sul · 2019-07-18T07:20:31Z

src/libaktualizr/uptane/tuf.cc

@@ -209,6 +215,61 @@ bool Target::IsOstree() const {
  }
 }

+bool Target::operator==(const Target &t2) const {


At first glance, it's confusing that a single class represents two types of Targets that require different context/class members, as well as their comparison.

That is an interesting point. @lbonn what would you think of having two children classes, one for DirectorTarget and one for ImagesTarget? Almost all of the logic would still be in the parent class, but the ecu map and hwid vectors would be in the respective children, and the match function would probably have to be a static function outside of the classes. I think that would help make things much, much more explicit.

On second thought, this is a pretty huge change and will require changing the API. I'm deep enough in this task already that I think I'd rather wait on that work and come back to it another time.

mike-sul · 2019-07-18T07:28:27Z

src/libaktualizr/uptane/tuf.cc

+    } else {
+      return false;
+    }
+    for (auto map_it = ecu_map->cbegin(); map_it != ecu_map->cend(); ++map_it) {


I am rather trying to understand a design & control flow here than comment the code. What's the point to specify ECU serial at all if the solution will update all ECUs that have HW-ID specified in the target metadata. At least it's how it works for secondaries, I ask to update just one ECU but the solution updates all ECUs of the given type/HW-ID.

That logic (of selecting all secondaries with the same HWID) is on the server-side. I'd prefer to not rely on that in libaktualizr. Besides, we still have to distinguish the primary. It's also a safety check: we need to confirm that the ECU has the HWID we expect it to (which is checked elsewhere).

That makes sense, policies are defined at the backend and can be even account/user specific, a a client/aktualizr does exactly what Director says without applying any policies.

mike-sul · 2019-07-18T07:37:39Z

src/libaktualizr/uptane/tuf.h

  Json::Value toDebugJson() const;
  friend std::ostream &operator<<(std::ostream &os, const Target &t);

 private:
  bool valid{true};
  std::string filename_;
  std::string type_;
-  std::map<EcuSerial, HardwareIdentifier> ecus_;
+  std::map<EcuSerial, HardwareIdentifier> ecus_;  // Director only


That's what I meant in one of the earlier comment, a field is used just for a specific type of Target instances.

Signed-off-by: Patrick Vacek <[email protected]>

Added a check for HW IDs. The Director sends a map of serials to HWIDs, but the Image repo sends us a vector of HWIDs. If the map and vector match, the comparison succeeds. Otherwise, we expect that every HWID in the map can be found in the vector. Added tests for all of these cases. Also added explanations for the rest of the fields we use for why we don't check them. Signed-off-by: Patrick Vacek <[email protected]>

Mostly just better error handling and messaging for Target mismatches. Signed-off-by: Patrick Vacek <[email protected]>

This required adding functionality to aktualizr-repo to add HWIDs to the image repo. Signed-off-by: Patrick Vacek <[email protected]>

Signed-off-by: Patrick Vacek <[email protected]>

Fixes OTA-2706 (Remove tests/test_data/fake_root). Also fixes the last broken test due to the Target matching changes. Signed-off-by: Patrick Vacek <[email protected]>

This should make Target matching more explicit (and easier to grep for). The == operator is not usually meaningful or desirable for Target objects. Signed-off-by: Patrick Vacek <[email protected]>

Signed-off-by: Patrick Vacek <[email protected]>

pattivacek · 2019-07-19T12:42:43Z

I believe I've finally addressed all review comments except for making Director/Image-specific children of the Target class. I looked into it and it is way too much work to do as part of this PR. It's not a bad idea, but that's for some other time.

pattivacek added the do-not-merge-until-ready label Jul 17, 2019

pattivacek requested review from zabbal, lbonn, eu-siemann and mike-sul July 17, 2019 09:02

pattivacek force-pushed the fix/OTA-3057/metadata-matching branch 2 times, most recently from 644abf9 to 69c9791 Compare July 17, 2019 12:22

pattivacek removed the do-not-merge-until-ready label Jul 17, 2019

pattivacek force-pushed the fix/OTA-3057/metadata-matching branch from 69c9791 to 5a1b54f Compare July 17, 2019 13:04

lbonn reviewed Jul 17, 2019

View reviewed changes

pattivacek mentioned this pull request Jul 17, 2019

Added tests cases for covering metadata type mismatching advancedtelematic/tuf-test-vectors#54

Merged

mike-sul reviewed Jul 18, 2019

View reviewed changes

mike-sul previously approved these changes Jul 18, 2019

View reviewed changes

pattivacek dismissed mike-sul’s stale review via 1935f07 July 18, 2019 12:13

pattivacek force-pushed the fix/OTA-3057/metadata-matching branch 5 times, most recently from 907e488 to 3c3c64c Compare July 18, 2019 14:41

pattivacek added 2 commits July 19, 2019 10:49

Use the types we have defined to make things easier.

c4ca53b

Signed-off-by: Patrick Vacek <[email protected]>

Document net-tools as a requirement for running the tests.

fb4c3ef

Signed-off-by: Patrick Vacek <[email protected]>

pattivacek added 2 commits July 19, 2019 10:49

Update tuf-test-vectors and fix related issues.

a38e24a

Mostly just better error handling and messaging for Target mismatches. Signed-off-by: Patrick Vacek <[email protected]>

pattivacek force-pushed the fix/OTA-3057/metadata-matching branch from 3c3c64c to 4620480 Compare July 19, 2019 09:01

pattivacek added 5 commits July 19, 2019 12:23

Fix tests for matching Target metadata.

6594d41

This required adding functionality to aktualizr-repo to add HWIDs to the image repo. Signed-off-by: Patrick Vacek <[email protected]>

Minor refactor to move Target equality function out of header file.

9d8bb97

Signed-off-by: Patrick Vacek <[email protected]>

Update changelog.

76e842d

Signed-off-by: Patrick Vacek <[email protected]>

Create EcuMap type to improve readability.

a4a8fee

Signed-off-by: Patrick Vacek <[email protected]>

Generate fake_root for selfupdate test dynamically.

38537a4

Fixes OTA-2706 (Remove tests/test_data/fake_root). Also fixes the last broken test due to the Target matching changes. Signed-off-by: Patrick Vacek <[email protected]>

pattivacek force-pushed the fix/OTA-3057/metadata-matching branch from 4620480 to 38537a4 Compare July 19, 2019 10:23

pattivacek added 2 commits July 19, 2019 14:35

Remove Target's comparison operator and replace with MatchTarget.

dc2d3e5

This should make Target matching more explicit (and easier to grep for). The == operator is not usually meaningful or desirable for Target objects. Signed-off-by: Patrick Vacek <[email protected]>

Rename MatchWith to MatchHash for clarity.

5320005

Signed-off-by: Patrick Vacek <[email protected]>

lbonn approved these changes Jul 23, 2019

View reviewed changes

pattivacek merged commit 4348855 into master Jul 23, 2019

pattivacek deleted the fix/OTA-3057/metadata-matching branch July 23, 2019 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/ota 3057/metadata matching #1258

Fix/ota 3057/metadata matching #1258

pattivacek commented Jul 17, 2019 •

edited

Loading

codecov-io commented Jul 17, 2019 •

edited

Loading

lbonn Jul 17, 2019

lbonn Jul 17, 2019

pattivacek Jul 17, 2019

lbonn Jul 17, 2019

pattivacek Jul 17, 2019

lbonn Jul 17, 2019

pattivacek Jul 18, 2019

lbonn Jul 17, 2019

pattivacek Jul 17, 2019

lbonn Jul 17, 2019

lbonn Jul 17, 2019

pattivacek Jul 17, 2019

lbonn Jul 17, 2019

pattivacek Jul 17, 2019

mike-sul Jul 18, 2019

pattivacek Jul 18, 2019

mike-sul Jul 18, 2019

pattivacek Jul 18, 2019

mike-sul Jul 18, 2019

pattivacek Jul 18, 2019

mike-sul Jul 18, 2019

pattivacek Jul 18, 2019

pattivacek Jul 18, 2019

mike-sul Jul 18, 2019

pattivacek Jul 18, 2019

mike-sul Jul 18, 2019

mike-sul Jul 18, 2019

pattivacek commented Jul 19, 2019

Fix/ota 3057/metadata matching #1258

Fix/ota 3057/metadata matching #1258

Conversation

pattivacek commented Jul 17, 2019 • edited Loading

codecov-io commented Jul 17, 2019 • edited Loading

Codecov Report

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

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 commented Jul 19, 2019

pattivacek commented Jul 17, 2019 •

edited

Loading

codecov-io commented Jul 17, 2019 •

edited

Loading