Fix/ota 3057/url determination #1267

pattivacek · 2019-07-25T12:48:20Z

The most interesting part is in sotauptaneclient.cc, where if the custom URL is not set in the Director target metadata, we check if it is set in the Images repo target metadata and use that if it is.

A lot of the changes relate to aktualizr-repo, where I added parameters to set the custom URL.

codecov-io · 2019-07-25T14:11:43Z

Codecov Report

Merging #1267 into master will decrease coverage by 0.12%.
The diff coverage is 96.96%.

@@            Coverage Diff             @@
##           master    #1267      +/-   ##
==========================================
- Coverage    79.7%   79.57%   -0.13%     
==========================================
  Files         173      173              
  Lines       10301    10320      +19     
==========================================
+ Hits         8210     8212       +2     
- Misses       2091     2108      +17

Impacted Files	Coverage Δ
src/aktualizr_repo/image_repo.h	`100% <ø> (ø)`	⬆️
src/aktualizr_repo/uptane_repo.h	`100% <ø> (ø)`	⬆️
src/libaktualizr/uptane/tuf.cc	`89.04% <ø> (ø)`	⬆️
src/libaktualizr/primary/sotauptaneclient.h	`100% <ø> (ø)`	⬆️
src/aktualizr_repo/director_repo.h	`100% <ø> (ø)`	⬆️
src/libaktualizr/primary/sotauptaneclient.cc	`91.35% <100%> (-0.56%)`	⬇️
src/aktualizr_repo/main.cc	`82.45% <100%> (+0.74%)`	⬆️
src/libaktualizr/uptane/tuf.h	`93.69% <100%> (+0.05%)`	⬆️
src/aktualizr_repo/director_repo.cc	`91.54% <100%> (+0.37%)`	⬆️
src/aktualizr_repo/uptane_repo.cc	`96.55% <100%> (ø)`	⬆️
... and 10 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 8c8877f...7b0c026. Read the comment docs.

mike-sul · 2019-07-26T07:46:04Z

src/aktualizr_repo/director_repo.cc

+  if (!url.empty()) {
+    director_targets["targets"][target_name]["custom"]["uri"] = url;
+  } else {
+    director_targets["targets"][target_name]["custom"].removeMember("uri");


Why a nested structure is required for the custom URI, not just a single attribute of target_name, to paraphrase why ["custom"]["uri"] not just ["custom-uri"] or there is/are other attribute(s) of ["custom"] ?
Perhaps, just the whole "custom" can be removed ?
Oh, I see another attribute of custom, it's "targetFormat", but still not clear why not remove the whole "custom" element.

there is/are other attribute(s) of ["custom"] ?

Precisely. It's following the recommendation of Uptane to keep implementation-specific custom metadata in a custom section.

mike-sul · 2019-07-26T07:53:41Z

src/aktualizr_repo/image_repo.cc

@@ -37,11 +37,14 @@ void ImageRepo::addBinaryImage(const boost::filesystem::path &image_path, const
  target["hashes"]["sha256"] = boost::algorithm::to_lower_copy(boost::algorithm::hex(Crypto::sha256digest(image)));


I know that it's not related to this specific PR, but loading the whole file/image into memory doesn't look like optimal way to calculate file hash, unless aktualizr-repo is used just for testing purposes.

Valid point. This is not what we do in libaktualizr itself, but aktualizr-repo is really just for testing, so it's not so critical.

mike-sul · 2019-07-26T08:19:56Z

src/libaktualizr/primary/sotauptaneclient.cc

-  for (auto it = targets.cbegin(); it != targets.cend(); ++it) {
-    auto res = downloadImage(*it, token);
+  for (const auto &it : targets) {
+    auto res = downloadImage(it, token);


for some reason downloadImage() passes a target object by value, maybe it makes sense to pass it by reference ?

This was changed because the targets inside the vectors are modified with setUri() and we probably don't want to change it in the vector that lives in the caller.

That's a bit annoying though, I would maybe suggest passing by const reference and making a copy inside the function so that it's more explicit. (+ a comment about that)

I would maybe suggest passing by const reference and making a copy inside the function so that it's more explicit. (+ a comment about that)

Agreed, done.

mike-sul · 2019-07-26T08:22:16Z

src/libaktualizr/primary/sotauptaneclient.cc

-  for (auto it = targets.cbegin(); it != targets.cend(); ++it) {
-    auto res = downloadImage(*it, token);
+  for (const auto &it : targets) {
+    auto res = downloadImage(it, token);
    if (res.first) {
      downloaded_targets.push_back(res.second);
    }


Why do we need to continue downloading if a current target image download fails, don't we consider a singe multi-target update as an atomic operation ?

Not necessarily. That logic is still being worked out as far as I understand. That should be up to the user to decide what to do. For now, we attempt to download everything regardless of failures.

Maybe, makes sense to make it configurable.

Agreed. That requires some integration with the backend, though, and we haven't sorted out the details yet.

mike-sul · 2019-07-26T08:22:38Z

src/libaktualizr/primary/sotauptaneclient.cc

@@ -582,10 +587,9 @@ void SotaUptaneClient::reportResume() {
  report_queue->enqueue(std_::make_unique<DeviceResumedReport>(correlation_id));
 }

-std::pair<bool, Uptane::Target> SotaUptaneClient::downloadImage(Uptane::Target target,
+std::pair<bool, Uptane::Target> SotaUptaneClient::downloadImage(const Uptane::Target &target,


Oh, you did it here :)

mike-sul · 2019-07-26T08:25:32Z

src/libaktualizr/primary/sotauptaneclient.h

@@ -41,8 +41,7 @@ class SotaUptaneClient {

  void initialize();
  void addNewSecondary(const std::shared_ptr<Uptane::SecondaryInterface> &sec);
-  result::Download downloadImages(const std::vector<Uptane::Target> &targets,
-                                  const api::FlowControlToken *token = nullptr);
+  result::Download downloadImages(std::vector<Uptane::Target> targets, const api::FlowControlToken *token = nullptr);


Why not by reference, is it because of the move semantics ?

lbonn · 2019-07-26T08:33:05Z

src/libaktualizr/primary/sotauptaneclient.cc

                                                  const api::FlowControlToken *token) {
  // Uptane step 4 - download all the images and verify them against the metadata (for OSTree - pull without
  // deploying)
  std::lock_guard<std::mutex> guard(download_mutex);
  result::Download result;
  std::vector<Uptane::Target> downloaded_targets;
-  for (auto it = targets.cbegin(); it != targets.cend(); ++it) {
-    auto images_target = findTargetInDelegationTree(*it);
+  for (auto &it : targets) {


When using this style of for, it makes it clearer to rename the variable to something other than it, as it's not an iterator anymore.

Good call. Done.

URLs are not matched. If the Director provides one, we use that. If not, but the Image repository does, use that. Otherwise, leave it empty and use the default. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Necessary for creating custom URLs. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

pattivacek requested review from lbonn, eu-siemann and mike-sul July 25, 2019 12:48

mike-sul reviewed Jul 26, 2019

View reviewed changes

lbonn reviewed Jul 26, 2019

View reviewed changes

pattivacek added 4 commits July 26, 2019 10:48

Use URL from Images repo if it is set and Director did not provide one.

fe6d68f

URLs are not matched. If the Director provides one, we use that. If not, but the Image repository does, use that. Otherwise, leave it empty and use the default. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Add --url parameter to aktualizr-repo image and addtarget commands.

38b2081

Necessary for creating custom URLs. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Tests for custom URL logic.

a97c13d

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Improve clarity of target copying logic.

7b0c026

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

pattivacek force-pushed the fix/OTA-3057/url-determination branch from 8236d1b to 7b0c026 Compare July 26, 2019 08:55

lbonn approved these changes Jul 29, 2019

View reviewed changes

pattivacek merged commit 7645037 into master Jul 29, 2019

pattivacek deleted the fix/OTA-3057/url-determination branch July 29, 2019 07:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/ota 3057/url determination #1267

Fix/ota 3057/url determination #1267

pattivacek commented Jul 25, 2019

codecov-io commented Jul 25, 2019 •

edited

Loading

mike-sul Jul 26, 2019 •

edited

Loading

pattivacek Jul 26, 2019

mike-sul Jul 26, 2019

pattivacek Jul 26, 2019

mike-sul Jul 26, 2019

lbonn Jul 26, 2019 •

edited

Loading

pattivacek Jul 26, 2019

mike-sul Jul 26, 2019

pattivacek Jul 26, 2019

mike-sul Jul 26, 2019

pattivacek Jul 26, 2019

mike-sul Jul 26, 2019

mike-sul Jul 26, 2019

lbonn Jul 26, 2019

pattivacek Jul 26, 2019

		@@ -37,11 +37,14 @@ void ImageRepo::addBinaryImage(const boost::filesystem::path &image_path, const
		target["hashes"]["sha256"] = boost::algorithm::to_lower_copy(boost::algorithm::hex(Crypto::sha256digest(image)));

Fix/ota 3057/url determination #1267

Fix/ota 3057/url determination #1267

Conversation

pattivacek commented Jul 25, 2019

codecov-io commented Jul 25, 2019 • edited Loading

Codecov Report

mike-sul Jul 26, 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

lbonn Jul 26, 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

codecov-io commented Jul 25, 2019 •

edited

Loading

mike-sul Jul 26, 2019 •

edited

Loading

lbonn Jul 26, 2019 •

edited

Loading