o/snapstate: download snap icon after downloading snap file

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

overlord/managers_test.go

@@ -1075,7 +1075,7 @@ func (s *baseMgrsSuite) mockStore(c *C) *httptest.Server {
 		hit := strings.Replace(hitTemplate, "@URL@", baseURL.String()+"/api/v1/snaps/download/"+name+"/"+revno, -1)
 		hit = strings.Replace(hit, "@NAME@", name, -1)
 		hit = strings.Replace(hit, "@SNAPID@", fakeSnapID(name), -1)
-		hit = strings.Replace(hit, "@ICON@", baseURL.String()+"/icon", -1)
+		hit = strings.Replace(hit, "@ICON@", "http://example.com/icon.svg", -1)
 		hit = strings.Replace(hit, "@VERSION@", info.Version, -1)
 		hit = strings.Replace(hit, "@REVISION@", revno, -1)
 		hit = strings.Replace(hit, `@TYPE@`, string(info.Type()), -1)
@@ -1094,6 +1094,14 @@ func (s *baseMgrsSuite) mockStore(c *C) *httptest.Server {
 			s.storeObserver(r)
 		}
 
+		if r.URL.Path == "http://example.com/icon.svg" {
+			// the http server was hit while requesting the snap icon, so just
+			// write some stand-in data
+			iconContents := fmt.Sprintf("icon contents")
+			w.Write([]byte(iconContents))
+			return
+		}
+
 		// all URLS are /api/v1/snaps/... or /v2/snaps/ or /v2/assertions/... so
 		// check the url is sane and discard the common prefix
 		// to simplify indexing into the comps slice.

overlord/snapstate/backend_test.go

@@ -154,6 +154,12 @@ type fakeDownload struct {
 	opts     *store.DownloadOptions
 }
 
+type fakeIconDownload struct {
+	name   string
+	target string
+	url    string
+}
+
 type byName []store.CurrentSnap
 
 func (bna byName) Len() int      { return len(bna) }
@@ -183,6 +189,7 @@ type fakeStore struct {
 	mu sync.Mutex
 
 	downloads           []fakeDownload
+	iconDownloads       []fakeIconDownload
 	refreshRevnos       map[string]snap.Revision
 	fakeBackend         *fakeSnappyBackend
 	fakeCurrentProgress int
@@ -196,7 +203,8 @@ type fakeStore struct {
 	// it should return the resources that the snap should have.
 	snapResourcesFn func(*snap.Info) []store.SnapResourceResult
 
-	downloadCallback func()
+	downloadCallback     func()
+	downloadIconCallback func(targetPath string)
 
 	namesToAssertedIDs map[string]string
 	idsToNames         map[string]string
@@ -250,6 +258,12 @@ func (f *fakeStore) appendDownload(dl *fakeDownload) {
 	f.downloads = append(f.downloads, *dl)
 }
 
+func (f *fakeStore) appendIconDownload(dl *fakeIconDownload) {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	f.iconDownloads = append(f.iconDownloads, *dl)
+}
+
 type snapSpec struct {
 	Name     string
 	Channel  string
@@ -931,6 +945,20 @@ func (f *fakeStore) Download(ctx context.Context, name, targetFn string, snapInf
 	return nil
 }
 
+func (f *fakeStore) DownloadIcon(ctx context.Context, name string, targetPath string, downloadURL string) error {
+	if f.downloadIconCallback != nil {
+		f.downloadIconCallback(targetPath)
+	}
+	f.appendIconDownload(&fakeIconDownload{
+		name:   name,
+		target: targetPath,
+		url:    downloadURL,
+	})
+	f.fakeBackend.appendOp(&fakeOp{op: "storesvc-download-icon", name: name})
+
+	return nil
+}
+
 func (f *fakeStore) WriteCatalogs(ctx context.Context, _ io.Writer, _ store.SnapAdder) error {
 	if ctx == nil {
 		panic("context required")

overlord/snapstate/handlers.go

@@ -726,6 +726,8 @@ func (m *SnapManager) doDownloadSnap(t *state.Task, tomb *tomb.Tomb) error {
 
 	meter := NewTaskProgressAdapterUnlocked(t)
 	targetFn := snapsup.BlobPath()
+	targetIconFn := backend.IconDownloadFilename(snapsup.SideInfo.SnapID)
+	iconURL := snapsup.Media.IconURL()
 
 	dlOpts := &store.DownloadOptions{
 		Scheduled: snapsup.IsAutoRefresh,
@@ -753,17 +755,53 @@ func (m *SnapManager) doDownloadSnap(t *state.Task, tomb *tomb.Tomb) error {
 		if err != nil {
 			return err
 		}
+
+		// Re-compute icon download filepath and URL if the result info has the
+		// necessary information
+		if result.SnapID != "" {
+			targetIconFn = backend.IconDownloadFilename(result.SnapID)
+		}
+		if resultIconURL := result.Media.IconURL(); resultIconURL != "" {
+			iconURL = resultIconURL
+		}
+
+		ctx := tomb.Context(nil) // XXX: should this be a real context?
+
 		timings.Run(perfTimings, "download", fmt.Sprintf("download snap %q", snapsup.SnapName()), func(timings.Measurer) {
-			err = theStore.Download(tomb.Context(nil), snapsup.SnapName(), targetFn, &result.DownloadInfo, meter, user, dlOpts)
+			err = theStore.Download(ctx, snapsup.SnapName(), targetFn, &result.DownloadInfo, meter, user, dlOpts)
 		})
 		snapsup.SideInfo = &result.SideInfo
+		if err != nil {
+			return err
+		}
+		// Snap download succeeded, now try to download the snap icon
+		if iconURL == "" {
+			logger.Debugf("cannot download snap icon for %q: no icon URL", snapsup.SnapName())
+		} else {
+			timings.Run(perfTimings, "download-icon", fmt.Sprintf("download snap icon for %q", snapsup.SnapName()), func(timings.Measurer) {
+				if iconErr := theStore.DownloadIcon(ctx, snapsup.SnapName(), targetIconFn, iconURL); iconErr != nil {
+					logger.Debugf("cannot download snap icon for %q: %#v", snapsup.SnapName(), iconErr)
+				}
+			})
+		}
 	} else {
+		ctx := tomb.Context(nil) // XXX: should this be a real context?
 		timings.Run(perfTimings, "download", fmt.Sprintf("download snap %q", snapsup.SnapName()), func(timings.Measurer) {
-			err = theStore.Download(tomb.Context(nil), snapsup.SnapName(), targetFn, snapsup.DownloadInfo, meter, user, dlOpts)
+			err = theStore.Download(ctx, snapsup.SnapName(), targetFn, snapsup.DownloadInfo, meter, user, dlOpts)
 		})
-	}
-	if err != nil {
-		return err
+		if err != nil {
+			return err
+		}
+		// Snap download succeeded, now try to download the snap icon
+		if iconURL == "" {
+			logger.Debugf("cannot download snap icon for %q: no icon URL", snapsup.SnapName())
+		} else {
+			timings.Run(perfTimings, "download-icon", fmt.Sprintf("download snap icon for %q", snapsup.SnapName()), func(timings.Measurer) {
+				if iconErr := theStore.DownloadIcon(ctx, snapsup.SnapName(), targetIconFn, iconURL); iconErr != nil {
+					logger.Debugf("cannot download snap icon for %q: %#v", snapsup.SnapName(), iconErr)
+				}
+			})
+		}
 	}
 
 	snapsup.SnapPath = targetFn

overlord/snapstate/snapstate_install_test.go

@@ -1320,6 +1320,17 @@ func (s *snapmgrTestSuite) TestInstallRunThrough(c *C) {
 	// we start without the auxiliary store info
 	c.Check(backend.AuxStoreInfoFilename("some-snap-id"), testutil.FileAbsent)
 
+	iconContents := []byte("I'm an svg")
+	// set up the downloadIcon callback to write the fake icon to the icons pool
+	var downloadIconCalls int
+	s.fakeStore.downloadIconCallback = func(targetPath string) {
+		downloadIconCalls++
+		c.Assert(downloadIconCalls, Equals, 1)
+
+		c.Assert(os.MkdirAll(filepath.Dir(targetPath), 0o755), IsNil)
+		c.Assert(os.WriteFile(targetPath, iconContents, 0o644), IsNil)
+	}
+
 	chg := s.state.NewChange("install", "install a snap")
 	opts := &snapstate.RevisionOptions{Channel: "channel-for-media"}
 	ts, err := snapstate.Install(context.Background(), s.state, "some-snap", opts, s.user.ID, snapstate.Flags{})
@@ -1337,6 +1348,11 @@ func (s *snapmgrTestSuite) TestInstallRunThrough(c *C) {
 		name:     "some-snap",
 		target:   filepath.Join(dirs.SnapBlobDir, "some-snap_11.snap"),
 	}})
+	c.Check(s.fakeStore.iconDownloads, DeepEquals, []fakeIconDownload{{
+		name:   "some-snap",
+		target: backend.IconDownloadFilename("some-snap-id"),
+		url:    "http://example.com/icon.png",
+	}})
 	c.Check(s.fakeStore.seenPrivacyKeys["privacy-key"], Equals, true, Commentf("salts seen: %v", s.fakeStore.seenPrivacyKeys))
 	expected := fakeOps{
 		{
@@ -1357,6 +1373,10 @@ func (s *snapmgrTestSuite) TestInstallRunThrough(c *C) {
 			op:   "storesvc-download",
 			name: "some-snap",
 		},
+		{
+			op:   "storesvc-download-icon",
+			name: "some-snap",
+		},
 		{
 			op:    "validate-snap:Doing",
 			name:  "some-snap",
@@ -1514,6 +1534,8 @@ func (s *snapmgrTestSuite) TestInstallRunThrough(c *C) {
 		},
 	})
 	c.Check(info.StoreURL, Equals, "https://snapcraft.io/example-snap")
+
+	c.Check(backend.IconInstallFilename("some-snap-id"), testutil.FileEquals, iconContents)
 }
 
 func (s *snapmgrTestSuite) testParallelInstanceInstallRunThrough(c *C, inputFlags, expectedFlags snapstate.Flags) {