Merge pull request #696 from jimmidyson/atomic-file-cache

Use temp file for cached downloads & rename to handle interrupted downloads gracefully

pkg/minikube/cluster/cluster.go

@@ -23,6 +23,7 @@ import (
 	"encoding/json"
 	"flag"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"net"
 	"net/http"
@@ -43,6 +44,7 @@ import (
 	kubeApi "k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/client/unversioned"
 	"k8s.io/kubernetes/pkg/client/unversioned/clientcmd"
+
 	"k8s.io/minikube/pkg/minikube/assets"
 	"k8s.io/minikube/pkg/minikube/constants"
 	"k8s.io/minikube/pkg/minikube/sshutil"
@@ -320,16 +322,14 @@ func createVirtualboxHost(config MachineConfig) drivers.Driver {
 	return d
 }
 
-func isIsoChecksumValid(isoData *[]byte, shaURL string) bool {
+func isIsoChecksumValid(actualSum []byte, shaURL string) bool {
 	expectedSum, err := util.ParseSHAFromURL(shaURL)
 	if err != nil {
 		glog.Errorf("Error retrieving SHA from URL: %s. Error: %s.", shaURL, err)
 		return false
 	}
 
-	b := sha256.Sum256(*isoData)
-	actualSum := hex.EncodeToString(b[:])
-	if string(expectedSum) != actualSum {
+	if string(expectedSum) != hex.EncodeToString(actualSum) {
 		glog.Errorf("Downloaded ISO checksum does not match expected value. Actual: %s. Expected: %s", actualSum, expectedSum)
 		return false
 	}
@@ -344,31 +344,41 @@ func (m *MachineConfig) CacheMinikubeISOFromURL() error {
 	}
 
 	defer response.Body.Close()
-	isoData, err := ioutil.ReadAll(response.Body)
+
+	if response.StatusCode != http.StatusOK {
+		return errors.Errorf("Received %d response from %s while trying to download minikube.iso", response.StatusCode, m.MinikubeISO)
+	}
+
+	hasher := sha256.New()
+	r := io.TeeReader(response.Body, hasher)
+
+	destDir := filepath.Dir(m.GetISOCacheFilepath())
+	destFilename := filepath.Base(m.GetISOCacheFilepath())
+	tempFile, err := ioutil.TempFile(destDir, ".tmp-"+destFilename)
+	if err != nil {
+		os.Remove(tempFile.Name())
+		return errors.Wrap(err, "Failed to create temp cache ISO file")
+	}
+
+	_, err = io.Copy(tempFile, r)
 	if err != nil {
-		return errors.Wrap(err, "Error reading minikubeISO url response")
+		os.Remove(tempFile.Name())
+		return errors.Wrap(err, "Failed to write to temp cache ISO file")
 	}
 
 	// Validate the ISO if it was the default URL, before writing it to disk.
 	if m.MinikubeISO == constants.DefaultIsoUrl {
-		if !isIsoChecksumValid(&isoData, constants.DefaultIsoShaUrl) {
+		if !isIsoChecksumValid(hasher.Sum(nil), constants.DefaultIsoShaUrl) {
+			os.Remove(tempFile.Name())
 			return errors.New("Error validating ISO checksum.")
 		}
 	}
 
-	if response.StatusCode != http.StatusOK {
-		return errors.Errorf("Received %d response from %s while trying to download minikube.iso", response.StatusCode, m.MinikubeISO)
-	}
-
-	out, err := os.Create(m.GetISOCacheFilepath())
-	if err != nil {
-		return errors.Wrap(err, "Error creating minikube iso cache filepath")
+	if err = os.Rename(tempFile.Name(), m.GetISOCacheFilepath()); err != nil {
+		os.Remove(tempFile.Name())
+		return errors.Wrap(err, "Error copying temp iso file to destination")
 	}
-	defer out.Close()
 
-	if _, err = out.Write(isoData); err != nil {
-		return errors.Wrap(err, "Error writing iso data to file")
-	}
 	return nil
 }
 

pkg/minikube/cluster/cluster_test.go

@@ -725,7 +725,7 @@ func TestIsIsoChecksumValid(t *testing.T) {
 	}
 
 	isoData := []byte("myIsoData")
-	isoCheckSum := sha256.Sum256(isoData)
+	isoCheckSum := sha256.New().Sum(isoData)
 	for _, tc := range tests {
 		ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			if tc.httpError != 0 {
@@ -738,7 +738,7 @@ func TestIsIsoChecksumValid(t *testing.T) {
 			}
 		}))
 		defer ts.Close()
-		valid := isIsoChecksumValid(&isoData, ts.URL)
+		valid := isIsoChecksumValid(isoCheckSum, ts.URL)
 		if valid != tc.expected {
 			t.Errorf("Expected isIsoChecksumValid to be %v, was %v", tc.expected, valid)
 		}

pkg/minikube/cluster/localkube_caching.go

@@ -63,17 +63,26 @@ func (l *localkubeCacher) isLocalkubeCached() bool {
 	return true
 }
 
+// cacheLocalKube downloads the stream to a temporary file & then atomically
+// renames the file to the destination file name. This is to gracefully handle
+// any errors in downloading the file.
 func (l *localkubeCacher) cacheLocalkube(body io.ReadCloser) error {
 	// store localkube inside the .minikube dir
-	out, err := os.Create(l.getLocalkubeCacheFilepath())
+	dir := filepath.Dir(l.getLocalkubeCacheFilepath())
+	filename := filepath.Base(l.getLocalkubeCacheFilepath())
+	out, err := ioutil.TempFile(dir, ".tmp-"+filename)
 	if err != nil {
-		return errors.Wrap(err, "Error creating localkube local file")
+		os.Remove(out.Name())
+		return errors.Wrap(err, "Error creating temporary localkube local file")
 	}
-	defer out.Close()
-	defer body.Close()
 	if _, err = io.Copy(out, body); err != nil {
+		os.Remove(out.Name())
 		return errors.Wrap(err, "Error writing localkube to file")
 	}
+	if err := os.Rename(out.Name(), l.getLocalkubeCacheFilepath()); err != nil {
+		os.Remove(out.Name())
+		return errors.Wrap(err, "Error renaming temporary localkube to destination")
+	}
 	return nil
 }
 
@@ -98,6 +107,7 @@ func (l *localkubeCacher) downloadAndCacheLocalkube() error {
 	if err = util.Retry(5, downloader); err != nil {
 		return errors.Wrap(err, "Max error attempts retrying localkube downloader")
 	}
+	defer resp.Body.Close()
 	if err = l.cacheLocalkube(resp.Body); err != nil {
 		return errors.Wrap(err, "Error caching localkube to local directory")
 	}