Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

Support custom URI for downloading targets. #1147

Merged
merged 2 commits into from
Mar 22, 2019
Merged

Conversation

pattivacek
Copy link
Collaborator

No description provided.

@doanac
Copy link
Collaborator

doanac commented Mar 20, 2019

Thanks @patrickvacek! This could be a handy feature for some stuff I've been looking at.

@@ -116,6 +116,12 @@ bool Fetcher::fetchVerifyTarget(const Target& target) {
} else {
ds.fhandle = storage->allocateTargetFile(false, target);
}

std::string target_url = target.uri();
if (target_url.empty() || target_url.find("example.com") != std::string::npos) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "example.com" do here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If "example.com" is part of the custom URL, we ignore it and fall back to the default fileserver based on the autoprov URL in the credentials. This is to support backwards compatibility with the server, which often fills that field with "https://example.com/" despite that that is obviously not where we should go looking for files. It's a hack, but I'm working around something out of my control.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the server really do that? Maybe it's just in a tutorial or something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the server really do that? Maybe it's just in a tutorial or something?

Yes, last I checked, which was yesterday.

Where is this being used? https://github.com/advancedtelematic/aktualizr/blob/master/src/sota_tools/deploy.cc#L91

garage-deploy when credentials indicate offline signing is required.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC we added the "example.com" bit there because there was a problem with that field being null. That was a long time ago and I'd forgotten about it. Perhaps it can be removed now?

Copy link
Collaborator Author

@pattivacek pattivacek Mar 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it's been there for as long as meta-updater has supported garage-sign: advancedtelematic/meta-updater@e7d4fbf

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to change the URL check here to match exactly what we've been using ("https://example.com/") to reduce the likelihood of accidental matches, and I will open a ticket to look into removing that URL as the default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@codecov-io
Copy link

codecov-io commented Mar 20, 2019

Codecov Report

Merging #1147 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1147      +/-   ##
==========================================
- Coverage   77.77%   77.75%   -0.02%     
==========================================
  Files         167      167              
  Lines        9947     9954       +7     
==========================================
+ Hits         7736     7740       +4     
- Misses       2211     2214       +3
Impacted Files Coverage Δ
src/libaktualizr/uptane/fetcher.cc 96.96% <100%> (+0.06%) ⬆️
src/libaktualizr/uptane/tuf.cc 87.55% <100%> (+0.2%) ⬆️
src/libaktualizr/uptane/tuf.h 93.85% <100%> (+0.05%) ⬆️
src/libaktualizr/storage/sqlstorage_base.cc 75.78% <0%> (-2.35%) ⬇️

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 e0398ee...3e758cd. Read the comment docs.

Only check for exact matches with the default URL, and do the check
earlier to reduce the space of URLs that get checked.

Signed-off-by: Patrick Vacek <[email protected]>
@pattivacek pattivacek force-pushed the feat/OTA-2376/use-custom-uri branch from 0688dd4 to 3e758cd Compare March 21, 2019 07:54
@lbonn lbonn merged commit f6202e4 into master Mar 22, 2019
@lbonn lbonn deleted the feat/OTA-2376/use-custom-uri branch March 22, 2019 09:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants