Repository: code-ready/crc
Pull request #2350 (2021-05-18 15:11:14)

Always validate pull secret before storing it in keyring
Better to do the validation once before saving the pull secret, rather
than expecting all callers of StoreInKeyring to do the validation
themselves.



## Proposed changes

This should not have any user visible change, except for a changed status code in the HTTP API when the pull secret is invalid.

openshift-ci[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/code-ready/crc/pull/2350#" title="Author self-approved">cfergeau</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=code-ready%2Fcrc).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/code-ready/crc/blob/master/OWNERS)~~ [cfergeau]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/packaging/windows/split.go b/packaging/windows/split.go
index 1733b2d5d..e780bc93e 100755
--- a/packaging/windows/split.go
+++ b/packaging/windows/split.go
@@ -43,5 +43,8 @@ func split(filePath string) error {
 			}
 			return err
 		}
+		if err = partFile.Close(); err != nil {
+			return err
+		}
 	}
 }
diff --git a/pkg/crc/api/adapter.go b/pkg/crc/api/adapter.go
index c48d19ca1..5d3ed231f 100644
--- a/pkg/crc/api/adapter.go
+++ b/pkg/crc/api/adapter.go
@@ -7,7 +7,6 @@ import (
 	"github.com/code-ready/crc/pkg/crc/logging"
 	"github.com/code-ready/crc/pkg/crc/machine"
 	"github.com/code-ready/crc/pkg/crc/machine/types"
-	"github.com/code-ready/machine/libmachine/state"
 )
 
 type AdaptedClient interface {
@@ -16,6 +15,7 @@ type AdaptedClient interface {
 	Start(ctx context.Context, startConfig types.StartConfig) client.StartResult
 	Status() client.ClusterStatusResult
 	Stop() client.Result
+	PowerOff() client.Result
 }
 
 type Adapter struct {
@@ -88,18 +88,25 @@ func (a *Adapter) Status() client.ClusterStatusResult {
 }
 
 func (a *Adapter) Stop() client.Result {
-	vmState, err := a.Underlying.Stop()
+	_, err := a.Underlying.Stop()
 	if err != nil {
 		logging.Error(err)
-		if vmState == state.Running {
-			err := a.Underlying.PowerOff()
-			if err != nil {
-				logging.Error(err)
-				return client.Result{
-					Success: false,
-					Error:   err.Error(),
-				}
-			}
+		return client.Result{
+			Success: false,
+			Error:   err.Error(),
+		}
+	}
+	return client.Result{
+		Success: true,
+	}
+}
+
+func (a *Adapter) PowerOff() client.Result {
+	if err := a.Underlying.PowerOff(); err != nil {
+		logging.Error(err)
+		return client.Result{
+			Success: false,
+			Error:   err.Error(),
 		}
 	}
 	return client.Result{
diff --git a/pkg/crc/api/api.go b/pkg/crc/api/api.go
index f56f56846..575f0104d 100644
--- a/pkg/crc/api/api.go
+++ b/pkg/crc/api/api.go
@@ -51,6 +51,8 @@ func (api Server) handleRequest(req commandRequest, conn net.Conn) {
 		result = api.handler.Start(req.Args)
 	case "stop":
 		result = api.handler.Stop()
+	case "poweroff":
+		result = api.handler.PowerOff()
 	case "status":
 		result = api.handler.Status()
 	case "delete":
diff --git a/pkg/crc/api/api_http.go b/pkg/crc/api/api_http.go
index a41218d83..1c39504b9 100644
--- a/pkg/crc/api/api_http.go
+++ b/pkg/crc/api/api_http.go
@@ -13,7 +13,6 @@ import (
 	crcConfig "github.com/code-ready/crc/pkg/crc/config"
 	"github.com/code-ready/crc/pkg/crc/logging"
 	"github.com/code-ready/crc/pkg/crc/machine"
-	"github.com/code-ready/crc/pkg/crc/validation"
 )
 
 func NewMux(config crcConfig.Storage, machine machine.Client, logger Logger, telemetry Telemetry) http.Handler {
@@ -42,6 +41,14 @@ func NewMux(config crcConfig.Storage, machine machine.Client, logger Logger, tel
 		sendResponse(w, stopResult)
 	})
 
+	mux.HandleFunc("/poweroff", func(w http.ResponseWriter, r *http.Request) {
+		if wrongHTTPMethodUsed(r, w, http.MethodPost) {
+			return
+		}
+		stopResult := handler.PowerOff()
+		sendResponse(w, stopResult)
+	})
+
 	mux.HandleFunc("/status", func(w http.ResponseWriter, r *http.Request) {
 		if wrongHTTPMethodUsed(r, w, http.MethodGet) {
 			return
@@ -101,10 +108,6 @@ func pullSecretHandler(config crcConfig.Storage) func(w http.ResponseWriter, r *
 				http.Error(w, err.Error(), http.StatusBadRequest)
 				return
 			}
-			if err := validation.ImagePullSecret(string(bin)); err != nil {
-				http.Error(w, err.Error(), http.StatusBadRequest)
-				return
-			}
 			if err := cluster.StoreInKeyring(string(bin)); err != nil {
 				http.Error(w, err.Error(), http.StatusInternalServerError)
 				return
diff --git a/pkg/crc/api/handlers.go b/pkg/crc/api/handlers.go
index 0e9ecc588..139a01f1f 100644
--- a/pkg/crc/api/handlers.go
+++ b/pkg/crc/api/handlers.go
@@ -59,6 +59,11 @@ func (h *Handler) Stop() string {
 	return encodeStructToJSON(commandResult)
 }
 
+func (h *Handler) PowerOff() string {
+	commandResult := h.MachineClient.PowerOff()
+	return encodeStructToJSON(commandResult)
+}
+
 func (h *Handler) Start(args json.RawMessage) string {
 	var parsedArgs client.StartConfig
 	if args != nil {
diff --git a/pkg/crc/cluster/pullsecret.go b/pkg/crc/cluster/pullsecret.go
index 051396604..da0e5210d 100644
--- a/pkg/crc/cluster/pullsecret.go
+++ b/pkg/crc/cluster/pullsecret.go
@@ -131,6 +131,11 @@ func loadFromKeyring() (string, error) {
 
 func StoreInKeyring(pullSecret string) error {
 	var b bytes.Buffer
+
+	if err := validation.ImagePullSecret(pullSecret); err != nil {
+		return err
+	}
+
 	compressor := gzip.NewWriter(&b)
 	if _, err := compressor.Write([]byte(pullSecret)); err != nil {
 		return err
diff --git a/pkg/crc/machine/sync.go b/pkg/crc/machine/sync.go
index 874e7b8b6..e55ebbbeb 100644
--- a/pkg/crc/machine/sync.go
+++ b/pkg/crc/machine/sync.go
@@ -163,7 +163,20 @@ func (s *Synchronized) PowerOff() error {
 }
 
 func (s *Synchronized) Status() (*types.ClusterStatusResult, error) {
-	return s.underlying.Status()
+	switch s.CurrentState() {
+	case Starting:
+		return &types.ClusterStatusResult{
+			CrcStatus:       state.Starting,
+			OpenshiftStatus: types.OpenshiftStarting,
+		}, nil
+	case Stopping, Deleting:
+		return &types.ClusterStatusResult{
+			CrcStatus:       state.Stopping,
+			OpenshiftStatus: types.OpenshiftStopping,
+		}, nil
+	default:
+		return s.underlying.Status()
+	}
 }
 
 func (s *Synchronized) IsRunning() (bool, error) {
diff --git a/pkg/crc/machine/types/types.go b/pkg/crc/machine/types/types.go
index 8447705ff..9fb85ef2d 100644
--- a/pkg/crc/machine/types/types.go
+++ b/pkg/crc/machine/types/types.go
@@ -60,6 +60,7 @@ const (
 	OpenshiftRunning     OpenshiftStatus = "Running"
 	OpenshiftDegraded    OpenshiftStatus = "Degraded"
 	OpenshiftStopped     OpenshiftStatus = "Stopped"
+	OpenshiftStopping    OpenshiftStatus = "Stopping"
 )
 
 type ConsoleResult struct {
diff --git a/pkg/drivers/hyperv/copy.go b/pkg/drivers/hyperv/copy.go
index 798870845..bb139db7d 100644
--- a/pkg/drivers/hyperv/copy.go
+++ b/pkg/drivers/hyperv/copy.go
@@ -29,5 +29,9 @@ func copyFile(src, dst string) error {
 		return err
 	}
 
-	return os.Chmod(dst, fi.Mode())
+	if err = os.Chmod(dst, fi.Mode()); err != nil {
+		return err
+	}
+
+	return out.Close()
 }
diff --git a/pkg/embed/embed.go b/pkg/embed/embed.go
index ba5bc4890..bf6f087c4 100644
--- a/pkg/embed/embed.go
+++ b/pkg/embed/embed.go
@@ -65,5 +65,5 @@ func ExtractFromExecutable(executablePath, embedName, destFile string) error {
 	if err != nil {
 		return fmt.Errorf("Failed to copy embedded '%s' from %s to %s: %v", embedName, executablePath, destFile, err)
 	}
-	return nil
+	return writer.Close()
 }
diff --git a/pkg/extract/extract.go b/pkg/extract/extract.go
index 0d9df28c5..74535bebe 100644
--- a/pkg/extract/extract.go
+++ b/pkg/extract/extract.go
@@ -133,7 +133,7 @@ func untar(reader io.Reader, targetDir string, fileFilter func(string) bool, sho
 		// if it's a file create it
 		case tar.TypeReg, tar.TypeGNUSparse:
 			// tar.Next() will externally only iterate files, so we might have to create intermediate directories here
-			if err := untarFile(tarReader, header, path, showProgress); err != nil {
+			if err := uncompressFile(tarReader, header.FileInfo(), path, showProgress); err != nil {
 				return nil, err
 			}
 			extractedFiles = append(extractedFiles, path)
@@ -141,24 +141,28 @@ func untar(reader io.Reader, targetDir string, fileFilter func(string) bool, sho
 	}
 }
 
-func untarFile(tarReader *tar.Reader, header *tar.Header, path string, showProgress bool) error {
+func uncompressFile(tarReader io.Reader, fileInfo os.FileInfo, path string, showProgress bool) error {
+	// with a file filter, we may have skipped the intermediate directories, make sure they exist
 	if err := os.MkdirAll(filepath.Dir(path), 0750); err != nil {
 		return err
 	}
 
-	file, err := os.OpenFile(path, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, header.FileInfo().Mode())
+	file, err := os.OpenFile(path, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, fileInfo.Mode())
 	if err != nil {
 		return err
 	}
 	defer file.Close()
 
-	reader, cleanup := progressBarReader(tarReader, header.FileInfo(), showProgress)
+	reader, cleanup := progressBarReader(tarReader, fileInfo, showProgress)
 	defer cleanup()
 
 	// copy over contents
 	// #nosec G110
 	_, err = io.Copy(file, reader)
-	return err
+	if err != nil {
+		return err
+	}
+	return file.Close()
 }
 
 func buildPath(baseDir, filename string) (string, error) {
@@ -210,29 +214,13 @@ func unzip(archive, target string, fileFilter func(string) bool, showProgress bo
 }
 
 func unzipFile(file *zip.File, path string, showProgress bool) error {
-	// with a file filter, we may have skipped the intermediate directories, make sure they exist
-	if err := os.MkdirAll(filepath.Dir(path), 0750); err != nil {
-		return err
-	}
-
 	fileReader, err := file.Open()
 	if err != nil {
 		return err
 	}
 	defer fileReader.Close()
 
-	reader, cleanup := progressBarReader(fileReader, file.FileInfo(), showProgress)
-	defer cleanup()
-
-	targetFile, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, file.Mode())
-	if err != nil {
-		return err
-	}
-	defer targetFile.Close()
-
-	// #nosec G110
-	_, err = io.Copy(targetFile, reader)
-	return err
+	return uncompressFile(fileReader, file.FileInfo(), path, showProgress)
 }
 
 func progressBarReader(reader io.Reader, info os.FileInfo, showProgress bool) (io.Reader, func()) {
diff --git a/pkg/os/util.go b/pkg/os/util.go
index 0098fbf7a..5f3ba86d6 100644
--- a/pkg/os/util.go
+++ b/pkg/os/util.go
@@ -60,7 +60,7 @@ func CopyFileContents(src string, dst string, permission os.FileMode) error {
 		return fmt.Errorf("[%v] Cannot sync '%s' to '%s'", err, src, dst)
 	}
 
-	return nil
+	return destFile.Close()
 }
 
 func FileContentMatches(path string, expectedContent []byte) error {


Test patch:
diff --git a/pkg/crc/cluster/pullsecret_test.go b/pkg/crc/cluster/pullsecret_test.go
index 4de16ed30..2d2e6d9ad 100644
--- a/pkg/crc/cluster/pullsecret_test.go
+++ b/pkg/crc/cluster/pullsecret_test.go
@@ -15,6 +15,7 @@ const (
 	secret1 = `{"auths":{"quay.io":{"auth":"secret1"}}}` // #nosec G101
 	secret2 = `{"auths":{"quay.io":{"auth":"secret2"}}}` // #nosec G101
 	secret3 = `{"auths":{"quay.io":{"auth":"secret3"}}}` // #nosec G101
+	secret4 = `{`
 )
 
 func TestLoadPullSecret(t *testing.T) {
@@ -54,4 +55,6 @@ func TestLoadPullSecret(t *testing.T) {
 	val, err = loader.Value()
 	assert.NoError(t, err)
 	assert.Equal(t, secret1, val)
+
+	assert.Error(t, StoreInKeyring(secret4))
 }
diff --git a/test/extended/util/util.go b/test/extended/util/util.go
index bb1114601..02929fc37 100644
--- a/test/extended/util/util.go
+++ b/test/extended/util/util.go
@@ -72,6 +72,11 @@ func CopyResourcesFromPath(resourcesPath string) error {
 			fmt.Printf("Error occurred syncing file: %s", err)
 			return err
 		}
+		err = dFile.Close()
+		if err != nil {
+			fmt.Printf("Error closing file: %s", err)
+			return err
+		}
 	}
 	return nil
 }