diff --git a/overlord/managers_test.go b/overlord/managers_test.go index ae2e50bd931..4ef4d20fa1d 100644 --- a/overlord/managers_test.go +++ b/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. diff --git a/overlord/snapstate/backend_test.go b/overlord/snapstate/backend_test.go index edf4ec5f988..651e09c65da 100644 --- a/overlord/snapstate/backend_test.go +++ b/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") diff --git a/overlord/snapstate/handlers.go b/overlord/snapstate/handlers.go index 5c42d8de50f..a65a0a16c19 100644 --- a/overlord/snapstate/handlers.go +++ b/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 diff --git a/overlord/snapstate/snapstate_install_test.go b/overlord/snapstate/snapstate_install_test.go index b503313cef5..2e140e478a9 100644 --- a/overlord/snapstate/snapstate_install_test.go +++ b/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) {