store: add functions to download snap icons

store/download_test.go

@@ -172,14 +172,13 @@ func (s *downloadSuite) TestActualDownloadLessDetailedCloudInfoFromAuthContext(c
 }
 
 func (s *downloadSuite) TestDownloadCancellation(c *C) {
-	// the channel used by mock server to request cancellation from the test
-	syncCh := make(chan struct{})
+	ctx, cancel := context.WithCancel(context.Background())
 
 	n := 0
 	mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		n++
 		io.WriteString(w, "foo")
-		syncCh <- struct{}{}
+		cancel()
 		io.WriteString(w, "bar")
 		time.Sleep(10 * time.Millisecond)
 	}))
@@ -188,23 +187,12 @@ func (s *downloadSuite) TestDownloadCancellation(c *C) {
 
 	theStore := store.New(&store.Config{}, nil)
 
-	ctx, cancel := context.WithCancel(context.Background())
-
-	result := make(chan string)
-	go func() {
-		sha3 := ""
-		var buf SillyBuffer
-		err := store.Download(ctx, "foo", sha3, mockServer.URL, nil, theStore, &buf, 0, nil, nil)
-		result <- err.Error()
-		close(result)
-	}()
-
-	<-syncCh
-	cancel()
+	sha3 := ""
+	var buf SillyBuffer
+	err := store.Download(ctx, "foo", sha3, mockServer.URL, nil, theStore, &buf, 0, nil, nil)
 
-	err := <-result
 	c.Check(n, Equals, 1)
-	c.Assert(err, Equals, "the download has been cancelled: context canceled")
+	c.Assert(err, ErrorMatches, "the download has been cancelled: context canceled")
 }
 
 type nopeSeeker struct{ io.ReadWriter }
@@ -291,8 +279,8 @@ func (s *downloadSuite) TestActualDownload500Once(c *C) {
 	c.Check(n, Equals, 2)
 }
 
-// SillyBuffer is a ReadWriteSeeker buffer with a limited size for the tests
-// (bytes does not implement an ReadWriteSeeker)
+// SillyBuffer is a ReadWriteSeekTruncater buffer with a limited size for the tests
+// (bytes does not implement an ReadWriteSeekTruncater)
 type SillyBuffer struct {
 	buf [1024]byte
 	pos int64
@@ -333,6 +321,13 @@ func (sb *SillyBuffer) Write(p []byte) (n int, err error) {
 	}
 	return n, nil
 }
+func (sb *SillyBuffer) Truncate(size int64) error {
+	if size < 0 || size > int64(len(sb.buf)) {
+		return fmt.Errorf("truncate out of bounds: %d", size)
+	}
+	sb.end = size
+	return nil
+}
 func (sb *SillyBuffer) String() string {
 	return string(sb.buf[0:sb.pos])
 }
@@ -708,3 +703,117 @@ func (s *downloadSuite) TestActualDownloadRateLimited(c *C) {
 	c.Check(buf.String(), Equals, canary)
 	c.Check(ratelimitReaderUsed, Equals, true)
 }
+
+func (s *downloadSuite) TestActualDownloadIcon(c *C) {
+	n := 0
+	mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		c.Check(r.Header.Get("Snap-CDN"), Equals, "")
+		c.Check(r.Header.Get("Snap-Device-Location"), Equals, "")
+		c.Check(r.Header.Get("Snap-Refresh-Reason"), Equals, "")
+		n++
+		io.WriteString(w, "response-data")
+	}))
+	c.Assert(mockServer, NotNil)
+	defer mockServer.Close()
+
+	var buf SillyBuffer
+	err := store.DownloadIconImpl(context.TODO(), "foo", mockServer.URL, &buf)
+	c.Assert(err, IsNil)
+	c.Check(buf.String(), Equals, "response-data")
+	c.Check(n, Equals, 1)
+}
+
+func (s *downloadSuite) TestActualDownloadIconTooLarge(c *C) {
+	var maxSize int64 = 1000 // Must be less than size of SillyBuffer, so we can write enough to exceed the limit
+	restore := store.MockMaxIconFilesize(maxSize)
+	defer restore()
+
+	n := 0
+	mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		n++
+		response := make([]byte, maxSize+1)
+		w.Write(response)
+	}))
+	c.Assert(mockServer, NotNil)
+	defer mockServer.Close()
+
+	var buf SillyBuffer
+	err := store.DownloadIconImpl(context.TODO(), "foo", mockServer.URL, &buf)
+	c.Assert(err, ErrorMatches, "unsupported Content-Length .*")
+	c.Check(n, Equals, 1)
+}
+
+func (s *downloadSuite) TestDownloadIconCancellation(c *C) {
+	ctx, cancel := context.WithCancel(context.Background())
+
+	n := 0
+	mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		n++
+		io.WriteString(w, "foo")
+		cancel()
+		io.WriteString(w, "bar")
+		time.Sleep(10 * time.Millisecond)
+	}))
+	c.Assert(mockServer, NotNil)
+	defer mockServer.Close()
+
+	var buf SillyBuffer
+	err := store.DownloadIconImpl(ctx, "foo", mockServer.URL, &buf)
+
+	c.Check(n, Equals, 1)
+	c.Assert(err, ErrorMatches, ".*: context canceled")
+}
+
+func (s *downloadSuite) TestActualDownloadIcon404(c *C) {
+	n := 0
+	mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		n++
+		w.WriteHeader(404)
+	}))
+	c.Assert(mockServer, NotNil)
+	defer mockServer.Close()
+
+	var buf SillyBuffer
+	err := store.DownloadIconImpl(context.TODO(), "foo", mockServer.URL, &buf)
+	c.Assert(err, NotNil)
+	c.Assert(err, FitsTypeOf, &store.DownloadError{})
+	c.Check(err.(*store.DownloadError).Code, Equals, 404)
+	c.Check(n, Equals, 1)
+}
+
+func (s *downloadSuite) TestActualDownloadIcon500(c *C) {
+	n := 0
+	mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		n++
+		w.WriteHeader(500)
+	}))
+	c.Assert(mockServer, NotNil)
+	defer mockServer.Close()
+
+	var buf SillyBuffer
+	err := store.DownloadIconImpl(context.TODO(), "foo", mockServer.URL, &buf)
+	c.Assert(err, NotNil)
+	c.Assert(err, FitsTypeOf, &store.DownloadError{})
+	c.Check(err.(*store.DownloadError).Code, Equals, 500)
+	c.Check(n, Equals, 5)
+}
+
+func (s *downloadSuite) TestActualDownloadIcon500Once(c *C) {
+	n := 0
+	mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		n++
+		if n == 1 {
+			w.WriteHeader(500)
+		} else {
+			io.WriteString(w, "response-data")
+		}
+	}))
+	c.Assert(mockServer, NotNil)
+	defer mockServer.Close()
+
+	var buf SillyBuffer
+	err := store.DownloadIconImpl(context.TODO(), "foo", mockServer.URL, &buf)
+	c.Assert(err, IsNil)
+	c.Check(buf.String(), Equals, "response-data")
+	c.Check(n, Equals, 2)
+}

store/export_test.go

@@ -42,6 +42,8 @@ var (
 	ApiURL        = apiURL
 	Download      = download
 
+	DownloadIconImpl = downloadIcon
+
 	ApplyDelta = applyDelta
 
 	AuthLocation      = authLocation
@@ -149,6 +151,14 @@ func MockDownload(f func(ctx context.Context, name, sha3_384, downloadURL string
 	}
 }
 
+func MockMaxIconFilesize(maxSize int64) (restore func()) {
+	return testutil.Mock(&maxIconFilesize, maxSize)
+}
+
+func MockDownloadIcon(f func(ctx context.Context, name, downloadURL string, w ReadWriteSeekTruncater) error) (restore func()) {
+	return testutil.Mock(&downloadIcon, f)
+}
+
 func MockDoDownloadReq(f func(ctx context.Context, storeURL *url.URL, cdnHeader string, resume int64, s *Store, user *auth.UserState) (*http.Response, error)) (restore func()) {
 	orig := doDownloadReq
 	doDownloadReq = f

store/store.go

@@ -766,6 +766,22 @@
 	}
 }
 
+// doIconRequest does an unauthenticated GET request to the given URL.
+func doIconRequest(ctx context.Context, client *http.Client, iconURL *url.URL) (*http.Response, error) {
+	var body io.Reader // empty body
+	req, err := http.NewRequestWithContext(ctx, "GET", iconURL.String(), body)
+	if err != nil {
+		return nil, err
+	}
+
+	resp, err := client.Do(req)
+	if err != nil {
+		return nil, err
+	}
+
+	return resp, err
+}
+
 func (s *Store) buildLocationString() (string, error) {
 	if s.dauthCtx == nil {
 		return "", nil

store/store_download.go

@@ -308,6 +308,42 @@
 	return s.cacher.Put(downloadInfo.Sha3_384, targetPath)
 }
 
+// DownloadIcon downloads the icon for the snap from the given download URL to
+// the given target path. Snap icons are small (<256kB) files served from an
+// ordinary unauthenticated file server, so this does not require store
+// authentication or user state, nor a progress bar. They are also not revision-
+// specific, and do not use a download cache.
+func DownloadIcon(ctx context.Context, name string, targetPath string, downloadURL string) error {
+	if err := os.MkdirAll(filepath.Dir(targetPath), 0o755); err != nil {
+		return err
+	}
+
+	// Download icon to a temporary file location, since there may be an active
+	// snap icon hard-linked to targetPath. If download fails, don't want to
+	// corrupt the existing icon file contents. Instead, commit the new file
+	// once the download succeeds, thus leaving any previous icon linked to the
+	// original unchanged icon contents, until a future task re-links to the
+	// new contents.
+	aw, err := osutil.NewAtomicFile(targetPath, 0o644, 0, osutil.NoChown, osutil.NoChown)
+	if err != nil {
+		return fmt.Errorf("cannot create file for snap icon for snap %s: %v", name, err)
+	}
+	// on success, Cancel becomes a no-op
+	defer aw.Cancel()
+
+	logger.Debugf("Starting download of %q to %q.", downloadURL, targetPath)
+
+	if err = downloadIcon(ctx, name, downloadURL, aw); err != nil {
+		logger.Debugf("download of %q failed: %#v", downloadURL, err)
+		return err
+	}
+
+	if err = aw.Commit(); err != nil {
+		return fmt.Errorf("cannot commit snap icon file for snap %s: %v", name, err)
+	}
+	return nil
+}
+
 func downloadReqOpts(storeURL *url.URL, cdnHeader string, opts *DownloadOptions) *requestOptions {
 	reqOptions := requestOptions{
 		Method:       "GET",
@@ -485,7 +521,7 @@
 			return fmt.Errorf("the download has been cancelled: %s", downloadCtx.Err())
 		}
 		var resp *http.Response
-		cli := s.newHTTPClient(nil)
+		cli := s.newHTTPClient(nil) // XXX: there's no timeout defined for this client, and the context is context.TODO(), so it won't be cancelled
 		oldCheckRedirect := cli.CheckRedirect
 		if oldCheckRedirect == nil {
 			panic("internal error: the httputil.NewHTTPClient-produced http.Client must have CheckRedirect defined")
@@ -600,6 +636,90 @@
 	return finalErr
 }
 
+type ReadWriteSeekTruncater interface {
+	io.Reader
+	io.Writer
+	io.Seeker
+
+	Truncate(size int64) error
+}
+
+var (
+	maxIconFilesize int64 = 300000
+	downloadIcon          = downloadIconImpl
+)
+
+// downloadIconImpl writes an http.Request which does not require authentication
+// or a progress.Meter.
+func downloadIconImpl(ctx context.Context, name, downloadURL string, w ReadWriteSeekTruncater) error {
+	iconURL, err := url.Parse(downloadURL)
+	if err != nil {
+		return err
+	}
+
+	startTime := time.Now()
+	errRetry := errors.New("retry")
+	for attempt := retry.Start(downloadRetryStrategy, nil); attempt.Next(); {
+		httputil.MaybeLogRetryAttempt(iconURL.String(), attempt, startTime)
+
+		err = func() error {
+			clientOpts := &httputil.ClientOptions{} // XXX: should this have a timeout?
+			cli := httputil.NewHTTPClient(clientOpts)
+			var resp *http.Response
+			resp, err = doIconRequest(ctx, cli, iconURL)
+			if err != nil {
+				if httputil.ShouldRetryAttempt(attempt, err) {
+					return errRetry
+				}
+				return err
+			}
+
+			defer resp.Body.Close()
+
+			if httputil.ShouldRetryHttpResponse(attempt, resp) {
+				return errRetry
+			}
+
+			if resp.StatusCode != 200 {
+				return &DownloadError{Code: resp.StatusCode, URL: resp.Request.URL}
+			}
+
+			if resp.ContentLength == 0 || resp.ContentLength > maxIconFilesize {
+				return fmt.Errorf("unsupported Content-Length for %s (must be nonzero and <%dB): %d", downloadURL, maxIconFilesize, resp.ContentLength)
+			}
+			logger.Debugf("download size for %s: %d", downloadURL, resp.ContentLength)
+
+			_, err = io.CopyN(w, resp.Body, resp.ContentLength)
+
+			if err != nil {
+				if httputil.ShouldRetryAttempt(attempt, err) {
+					if _, err := w.Seek(0, 0); err != nil {
+						return err
+					}
+					if err := w.Truncate(0); err != nil {
+						return err
+					}
+					return errRetry
+				}
+				return err
+			}
+
+			return nil
+		}()
+
+		if err == errRetry {
+			continue
+		}
+
+		if err == nil {
+			logger.Debugf("icon download succeeded for %s", name)
+		}
+
+		return err
+	}
+	return nil
+}
+
 // DownloadStream will copy the snap from the request to the io.Reader
 func (s *Store) DownloadStream(ctx context.Context, name string, downloadInfo *snap.DownloadInfo, resume int64, user *auth.UserState) (io.ReadCloser, int, error) {
 	// most other store network operations use s.endpointURL, which returns an