store: add functions to download snap icons #14990

olivercalder · 2025-01-28T02:22:54Z

These new functions, downloadIconImpl and DownloadIcon, mirror their non-icon counterparts downloadImpl and Download, except that because snap icons are small, simple assets on a fileserver, there is no need for snap-specific headers, authentication, partial downloads, or caching.

These will be used in the future in order to download a snap's icon whenever the new .snap blob is downloaded for that snap and linked to a snap icons directory so that the snapd API can serve snap icons from local icon files, rather than returning an error if the snap does not ship an icon at meta/gui/icon.*.

This work is tracked internally by https://warthogs.atlassian.net/browse/SNAPDENG-34095

These new functions, `downloadIconImpl` and `DownloadIcon`, mirror their non-icon counterparts `downloadImpl` and `Download`, except that because snap icons are small, simple assets on a fileserver, there is no need for snap-specific headers, authentication, partial downloads, or caching. These will be used in the future in order to download a snap's icon whenever the new `.snap` blob is downloaded for that snap and linked to a snap icons directory so that the snapd API can serve snap icons from local icon files, rather than returning an error if the snap does not ship an icon at `meta/gui/icon.*`. Signed-off-by: Oliver Calder <[email protected]>

codecov · 2025-01-28T02:41:21Z

Codecov Report

Attention: Patch coverage is 69.41176% with 26 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@103b4eb). Learn more about missing BASE report.
Report is 69 commits behind head on master.

Files with missing lines	Patch %	Lines
store/store_download.go	68.91%	18 Missing and 5 partials ⚠️
store/store.go	72.72%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #14990   +/-   ##
=========================================
  Coverage          ?   78.21%           
=========================================
  Files             ?     1164           
  Lines             ?   154433           
  Branches          ?        0           
=========================================
  Hits              ?   120791           
  Misses            ?    26194           
  Partials          ?     7448

Flag	Coverage Δ
unittests	`78.21% <69.41%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2025-01-28T04:11:59Z

Wed Feb 5 01:05:14 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/13146270163

Failures:

Executing:

google-distro-1:debian-11-64:tests/main/auto-refresh-gating
openstack:fedora-41-64:tests/main/auto-refresh-gating
google:ubuntu-20.04-64:tests/main/lxd:snapd_cgroup_just_inside
google:ubuntu-20.04-64:tests/main/snapd-state
google:ubuntu-20.04-64:tests/main/auto-refresh-gating
google:ubuntu-24.10-64:tests/main/auto-refresh-gating
google:ubuntu-24.04-64:tests/main/snapd-state

Restoring:

google-core:ubuntu-core-18-64:tests/core/kernel-base-gadget-single-reboot-failover
google-core:ubuntu-core-18-64:tests/core/auto-refresh-backoff-after-reboot:kernel

ZeyadYasser

Looks good, small nitpicks and a question.

Is the icon file validation (max size, format ..etc) intentionally ignored because snapd is guaranteed to get the URL from the store which should be trusted?

store/store.go

Signed-off-by: Oliver Calder <[email protected]>

andrewphelpsj

Some questions about the cleanup code, and maybe consider making that retry loop body into a function?

store/store.go

store/store_download.go

andrewphelpsj · 2025-01-28T21:25:03Z

store/store_download.go

+	var finalErr error
+	startTime := time.Now()
+	for attempt := retry.Start(downloadRetryStrategy, nil); attempt.Next(); {
+		httputil.MaybeLogRetryAttempt(iconURL.String(), attempt, startTime)


Would it be possible to make this inner loop a function? I understand it might be awkward because you need to hold on to finalErr.

I've made it a function now

andrewphelpsj · 2025-01-28T21:33:29Z

store/store_download_test.go

+	// ... and ensure that the tempfile is removed
+	c.Assert(osutil.FileExists(tmpfile.Name()), Equals, false)
+	// ... because it's been renamed to the target path already
+	c.Assert(osutil.FileExists(path), Equals, true)


It seems this test is expecting that behavior from my other comment? Seems like this could be avoided? If we fail to download the file and attempt cleanup, I feel like we should properly do the cleanup, unless there is some other use case I'm missing.

My guess is that if the file is renamed, its contents are good, so for large .snap files we want to keep it around. But that doesn't hold up since an error would suggest it's not in the cache, so it wouldn't be used on subsequent download anyway. So I'm not really sure why. Some git spelunking shows the code this is originally based on was written by John Lenton to support preserving partial download files after error: ebbb57e

I really don't know why the sync occurs after the rename. On unix, the rename should be atomic. Maybe it's an optimization to avoid committing the write to disk until we're sure the rename will work? But that seems silly, since a sync may occur anyway in the background during this time. Either way, it seems like an optimization for dealing with large .snap files, not applicable here.

All of this was now replaced with usage of osutil.AtomicFile

ZeyadYasser

Thank you!

Other than addressing comments from @andrewphelpsj, I think this looks good.

pedronis

remark about being careful with the download length/icon size

store/store_download.go

pedronis · 2025-01-30T17:23:25Z

@olivercalder I added @bboozzoo to also look at this

pedronis

actual vote

Signed-off-by: Oliver Calder <[email protected]>

pedronis

further comment

store/store_download.go

this will need a re-review

Signed-off-by: Oliver Calder <[email protected]>

store/store_download.go

bboozzoo · 2025-02-04T12:14:29Z

store/store_download.go

+			logger.Debugf("Download size for %s: %d", downloadURL, resp.ContentLength)
+		}
+
+		_, finalErr = io.CopyN(w, resp.Body, resp.ContentLength)


btw. no integrity check?

I think we need a test to verify the size limit

We don't have any information about what the icon contents are supposed to be, so there's no way to check them. We could leverage the etag to see if the server reports that the file has changed, but I don't think there's a way to use the etag to check for file integrity.

I'll add a test to verify the size limit though, thanks.

Signed-off-by: Oliver Calder <[email protected]>

Signed-off-by: Oliver Calder <[email protected]> Co-authored-by: Maciej Borzecki <[email protected]>

Signed-off-by: Oliver Calder <[email protected]>

andrewphelpsj

Thanks, some small comments.

store/store_download_test.go

store/store_download.go

Also add comments noting that the http clients for both snap downloads and snap icon downloads have no timeout defined. Signed-off-by: Oliver Calder <[email protected]>

Signed-off-by: Oliver Calder <[email protected]>

olivercalder added the Needs Samuele review Needs a review from Samuele before it can land label Jan 28, 2025

olivercalder requested review from ZeyadYasser and andrewphelpsj January 28, 2025 02:22

olivercalder requested a review from pedronis January 28, 2025 04:12

ZeyadYasser reviewed Jan 28, 2025

View reviewed changes

store/store.go Outdated Show resolved Hide resolved

store/store.go Outdated Show resolved Hide resolved

store: simplify doIconRequest to remove unused request options

fb306be

Signed-off-by: Oliver Calder <[email protected]>

olivercalder requested a review from ZeyadYasser January 28, 2025 16:33

andrewphelpsj reviewed Jan 28, 2025

View reviewed changes

olivercalder mentioned this pull request Jan 30, 2025

o/snapstate: download/link/unlink/discard snap icons in snapstate handlers #15003

Open

ZeyadYasser previously approved these changes Jan 30, 2025

View reviewed changes

pedronis reviewed Jan 30, 2025

View reviewed changes

store/store_download.go Outdated Show resolved Hide resolved

pedronis requested a review from bboozzoo January 30, 2025 17:22

pedronis requested changes Jan 30, 2025

View reviewed changes

olivercalder added 3 commits January 30, 2025 12:48

store: enforce snap icon size limit during download

774ae5d

Signed-off-by: Oliver Calder <[email protected]>

store: clarify sync/close/rename behavior when downloading snap icon

5658e58

Signed-off-by: Oliver Calder <[email protected]>

store: simplify doIconRequest further

7b20bc2

Signed-off-by: Oliver Calder <[email protected]>

olivercalder requested review from andrewphelpsj and pedronis January 30, 2025 21:52

pedronis reviewed Feb 3, 2025

View reviewed changes

store/store_download.go Outdated Show resolved Hide resolved

store: use CopyN when writing icon file contents

9972463

Signed-off-by: Oliver Calder <[email protected]>

olivercalder requested review from pedronis and ZeyadYasser February 4, 2025 04:26

bboozzoo reviewed Feb 4, 2025

View reviewed changes

olivercalder and others added 2 commits February 4, 2025 11:53

store: use AtomicFile when downloading snap icons

995ca51

Signed-off-by: Oliver Calder <[email protected]>

store: improve downloadIcon following review

6c6b08f

Signed-off-by: Oliver Calder <[email protected]> Co-authored-by: Maciej Borzecki <[email protected]>

olivercalder added 2 commits February 4, 2025 14:00

store: truncate icon file when retrying download

0c5ba59

Signed-off-by: Oliver Calder <[email protected]>

store: add test that max icon filesize is obeyed

dfaa1bb

Signed-off-by: Oliver Calder <[email protected]>

andrewphelpsj reviewed Feb 4, 2025

View reviewed changes

store/store_download_test.go Outdated Show resolved Hide resolved

store/store_download.go Outdated Show resolved Hide resolved

store/store_download.go Outdated Show resolved Hide resolved

store/store_download.go Outdated Show resolved Hide resolved

olivercalder added 3 commits February 4, 2025 16:32

store: remove explicit cancellation checks and simplify tests

97ce6c4

Also add comments noting that the http clients for both snap downloads and snap icon downloads have no timeout defined. Signed-off-by: Oliver Calder <[email protected]>

store: simplify icon download status code handling

462c669

Signed-off-by: Oliver Calder <[email protected]>

store: improve consistency of icon download tests

215774e

Signed-off-by: Oliver Calder <[email protected]>

olivercalder requested review from andrewphelpsj and bboozzoo February 4, 2025 22:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

store: add functions to download snap icons #14990

store: add functions to download snap icons #14990

olivercalder commented Jan 28, 2025

codecov bot commented Jan 28, 2025 •

edited

Loading

github-actions bot commented Jan 28, 2025 •

edited

Loading

ZeyadYasser left a comment

andrewphelpsj left a comment

andrewphelpsj Jan 28, 2025

olivercalder Feb 4, 2025

andrewphelpsj Jan 28, 2025

olivercalder Jan 30, 2025

olivercalder Feb 4, 2025

ZeyadYasser left a comment

pedronis left a comment

pedronis commented Jan 30, 2025

pedronis left a comment

pedronis left a comment

bboozzoo Feb 4, 2025

bboozzoo Feb 4, 2025 •

edited

Loading

olivercalder Feb 4, 2025

andrewphelpsj left a comment

store: add functions to download snap icons #14990

Are you sure you want to change the base?

store: add functions to download snap icons #14990

Conversation

olivercalder commented Jan 28, 2025

codecov bot commented Jan 28, 2025 • edited Loading

Codecov Report

github-actions bot commented Jan 28, 2025 • edited Loading

Failures:

Executing:

Restoring:

ZeyadYasser left a comment

Choose a reason for hiding this comment

andrewphelpsj left a comment

Choose a reason for hiding this comment

andrewphelpsj Jan 28, 2025

Choose a reason for hiding this comment

olivercalder Feb 4, 2025

Choose a reason for hiding this comment

andrewphelpsj Jan 28, 2025

Choose a reason for hiding this comment

olivercalder Jan 30, 2025

Choose a reason for hiding this comment

olivercalder Feb 4, 2025

Choose a reason for hiding this comment

ZeyadYasser left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

pedronis commented Jan 30, 2025

pedronis left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

bboozzoo Feb 4, 2025

Choose a reason for hiding this comment

bboozzoo Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

olivercalder Feb 4, 2025

Choose a reason for hiding this comment

andrewphelpsj left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 28, 2025 •

edited

Loading

github-actions bot commented Jan 28, 2025 •

edited

Loading

bboozzoo Feb 4, 2025 •

edited

Loading