Merge pull request #7896 from tstromberg/download-tests-suck

download: unify handling, refuse while under test

Author: tstromberg

pkg/minikube/download/binary.go

@@ -24,7 +24,6 @@ import (
 
 	"github.com/blang/semver"
 	"github.com/golang/glog"
-	"github.com/hashicorp/go-getter"
 	"github.com/pkg/errors"
 	"k8s.io/minikube/pkg/minikube/localpath"
 )
@@ -58,28 +57,14 @@ func Binary(binary, version, osName, archName string) (string, error) {
 		return targetFilepath, nil
 	}
 
-	if err = os.MkdirAll(targetDir, 0777); err != nil {
-		return "", errors.Wrapf(err, "mkdir %s", targetDir)
-	}
-
-	tmpDst := targetFilepath + ".download"
-
-	client := &getter.Client{
-		Src:     url,
-		Dst:     tmpDst,
-		Mode:    getter.ClientModeFile,
-		Options: []getter.ClientOption{getter.WithProgress(DefaultProgressBar)},
-	}
-
-	glog.Infof("Downloading: %+v", client)
-	if err := client.Get(); err != nil {
+	if err := download(url, targetFilepath); err != nil {
 		return "", errors.Wrapf(err, "download failed: %s", url)
 	}
 
 	if osName == runtime.GOOS && archName == runtime.GOARCH {
-		if err = os.Chmod(tmpDst, 0755); err != nil {
+		if err = os.Chmod(targetFilepath, 0755); err != nil {
 			return "", errors.Wrapf(err, "chmod +x %s", targetFilepath)
 		}
 	}
-	return targetFilepath, os.Rename(tmpDst, targetFilepath)
+	return targetFilepath, nil
 }

pkg/minikube/download/binary_test.go

@@ -0,0 +1,88 @@
+/*
+Copyright 2020 The Kubernetes Authors All rights reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package download
+
+import (
+	"flag"
+	"fmt"
+	"os"
+	"path/filepath"
+	"strings"
+
+	"github.com/golang/glog"
+	"github.com/hashicorp/go-getter"
+	"github.com/pkg/errors"
+)
+
+var (
+	mockMode = false
+)
+
+// EnableMock allows tests to selectively enable if downloads are mocked
+func EnableMock(b bool) {
+	mockMode = b
+}
+
+// download is a well-configured atomic download function
+func download(src string, dst string) error {
+	tmpDst := dst + ".download"
+	client := &getter.Client{
+		Src:     src,
+		Dst:     tmpDst,
+		Dir:     false,
+		Mode:    getter.ClientModeFile,
+		Options: []getter.ClientOption{getter.WithProgress(DefaultProgressBar)},
+		Getters: map[string]getter.Getter{
+			"file":  &getter.FileGetter{Copy: false},
+			"http":  &getter.HttpGetter{Netrc: false},
+			"https": &getter.HttpGetter{Netrc: false},
+		},
+	}
+
+	if err := os.MkdirAll(filepath.Dir(dst), 0755); err != nil {
+		return errors.Wrap(err, "mkdir")
+	}
+
+	// Don't bother with getter.MockGetter, as we don't provide a way to inspect the outcome
+	if mockMode {
+		glog.Infof("Mock download: %s -> %s", src, dst)
+		// Callers expect the file to exist
+		_, err := os.Create(dst)
+		return err
+	}
+
+	// Politely prevent tests from shooting themselves in the foot
+	if withinUnitTest() {
+		return fmt.Errorf("unmocked download under test")
+	}
+
+	glog.Infof("Downloading: %s -> %s", src, dst)
+	if err := client.Get(); err != nil {
+		return errors.Wrapf(err, "getter: %+v", client)
+	}
+	return os.Rename(tmpDst, dst)
+}
+
+// withinUnitTset detects if we are in running within a unit-test
+func withinUnitTest() bool {
+	// Nope, it's the integration test
+	if flag.Lookup("minikube-start-args") != nil || strings.HasPrefix(filepath.Base(os.Args[0]), "e2e-") {
+		return false
+	}
+
+	return flag.Lookup("test.v") != nil || strings.HasSuffix(os.Args[0], "test")
+}

pkg/minikube/download/download.go

@@ -21,8 +21,6 @@ import (
 	"os"
 
 	"github.com/blang/semver"
-	"github.com/golang/glog"
-	"github.com/hashicorp/go-getter"
 	"github.com/pkg/errors"
 	"k8s.io/minikube/pkg/minikube/out"
 )
@@ -35,25 +33,10 @@ func driverWithChecksumURL(name string, v semver.Version) string {
 // Driver downloads an arbitrary driver
 func Driver(name string, destination string, v semver.Version) error {
 	out.T(out.FileDownload, "Downloading driver {{.driver}}:", out.V{"driver": name})
-
-	tmpDst := destination + ".download"
-
-	url := driverWithChecksumURL(name, v)
-	client := &getter.Client{
-		Src:     url,
-		Dst:     tmpDst,
-		Mode:    getter.ClientModeFile,
-		Options: []getter.ClientOption{getter.WithProgress(DefaultProgressBar)},
+	if err := download(driverWithChecksumURL(name, v), destination); err != nil {
+		return errors.Wrap(err, "download")
 	}
 
-	glog.Infof("Downloading: %+v", client)
-	if err := client.Get(); err != nil {
-		return errors.Wrapf(err, "download failed: %s", url)
-	}
 	// Give downloaded drivers a baseline decent file permission
-	err := os.Chmod(tmpDst, 0755)
-	if err != nil {
-		return err
-	}
-	return os.Rename(tmpDst, destination)
+	return os.Chmod(destination, 0755)
 }

pkg/minikube/download/driver.go

@@ -26,7 +26,6 @@ import (
 	"time"
 
 	"github.com/golang/glog"
-	"github.com/hashicorp/go-getter"
 	"github.com/juju/mutex"
 	"github.com/pkg/errors"
 	"k8s.io/minikube/pkg/minikube/localpath"
@@ -134,19 +133,5 @@ func downloadISO(isoURL string, skipChecksum bool) error {
 		urlWithChecksum = isoURL
 	}
 
-	tmpDst := dst + ".download"
-
-	opts := []getter.ClientOption{getter.WithProgress(DefaultProgressBar)}
-	client := &getter.Client{
-		Src:     urlWithChecksum,
-		Dst:     tmpDst,
-		Mode:    getter.ClientModeFile,
-		Options: opts,
-	}
-
-	glog.Infof("full url: %s", urlWithChecksum)
-	if err := client.Get(); err != nil {
-		return errors.Wrap(err, isoURL)
-	}
-	return os.Rename(tmpDst, dst)
+	return download(urlWithChecksum, dst)
 }

pkg/minikube/download/iso.go

@@ -30,7 +30,6 @@ import (
 	"google.golang.org/api/option"
 
 	"github.com/golang/glog"
-	"github.com/hashicorp/go-getter"
 	"github.com/pkg/errors"
 	"github.com/spf13/viper"
 	"k8s.io/minikube/pkg/minikube/localpath"
@@ -133,27 +132,19 @@ func Preload(k8sVersion, containerRuntime string) error {
 	out.T(out.FileDownload, "Downloading Kubernetes {{.version}} preload ...", out.V{"version": k8sVersion})
 	url := remoteTarballURL(k8sVersion, containerRuntime)
 
-	tmpDst := targetPath + ".download"
-	client := &getter.Client{
-		Src:     url,
-		Dst:     tmpDst,
-		Mode:    getter.ClientModeFile,
-		Options: []getter.ClientOption{getter.WithProgress(DefaultProgressBar)},
-	}
-
-	glog.Infof("Downloading: %+v", client)
-	if err := client.Get(); err != nil {
+	if err := download(url, targetPath); err != nil {
 		return errors.Wrapf(err, "download failed: %s", url)
 	}
 
 	if err := saveChecksumFile(k8sVersion, containerRuntime); err != nil {
 		return errors.Wrap(err, "saving checksum file")
 	}
 
-	if err := verifyChecksum(k8sVersion, containerRuntime, tmpDst); err != nil {
+	if err := verifyChecksum(k8sVersion, containerRuntime, targetPath); err != nil {
 		return errors.Wrap(err, "verify")
 	}
-	return os.Rename(tmpDst, targetPath)
+
+	return nil
 }
 
 func saveChecksumFile(k8sVersion, containerRuntime string) error {

pkg/minikube/download/preload.go

@@ -25,6 +25,7 @@ import (
 	"k8s.io/minikube/pkg/minikube/assets"
 	"k8s.io/minikube/pkg/minikube/bootstrapper"
 	"k8s.io/minikube/pkg/minikube/command"
+	"k8s.io/minikube/pkg/minikube/download"
 )
 
 type copyFailRunner struct {
@@ -82,6 +83,8 @@ func TestCopyBinary(t *testing.T) {
 }
 
 func TestCacheBinariesForBootstrapper(t *testing.T) {
+	download.EnableMock(true)
+
 	oldMinikubeHome := os.Getenv("MINIKUBE_HOME")
 	defer os.Setenv("MINIKUBE_HOME", oldMinikubeHome)