From 3cb1f5c66c78160f353e3ee320474a646fa9cfbd Mon Sep 17 00:00:00 2001
From: Philippe Martin <phmartin@redhat.com>
Date: Tue, 1 Aug 2023 16:07:41 +0200
Subject: [PATCH] Fix podman version check (#7010)

* Fix podman version check

* Try to decode podman version output even if command not finished

* Return timeout error if any and decode error
---
 pkg/podman/version.go | 96 ++++++++++++++-----------------------------
 1 file changed, 31 insertions(+), 65 deletions(-)

diff --git a/pkg/podman/version.go b/pkg/podman/version.go
index ba7c135847f..3d72f123cb4 100644
--- a/pkg/podman/version.go
+++ b/pkg/podman/version.go
@@ -4,10 +4,8 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"os/exec"
-	"sync"
 	"time"
 
 	"k8s.io/klog"
@@ -34,85 +32,53 @@ func (o *PodmanCli) Version(ctx context.Context) (SystemVersionReport, error) {
 	// it is expected to return in a timely manner (hence this configurable timeout).
 	// This is to avoid situations like the one described in https://github.com/redhat-developer/odo/issues/6575
 	// (where a podman CLI that takes too long to respond affects the "odo dev" command, even if the user did not intend to use the Podman platform).
-	ctxWithTimeout, cancel := context.WithTimeout(ctx, o.podmanCmdInitTimeout)
-	defer cancel()
 
-	cmd := exec.CommandContext(ctxWithTimeout, o.podmanCmd, "version", "--format", "json")
+	cmd := exec.CommandContext(ctx, o.podmanCmd, "version", "--format", "json")
 	klog.V(3).Infof("executing %v", cmd.Args)
 
-	// Because cmd.Output() does not respect the context timeout (see https://github.com/golang/go/issues/57129),
-	// we are reading from the connected pipes instead.
-	stdoutPipe, err := cmd.StdoutPipe()
-	if err != nil {
-		return SystemVersionReport{}, err
-	}
-	stderrPipe, err := cmd.StderrPipe()
-	if err != nil {
-		return SystemVersionReport{}, err
-	}
+	outbuf, errbuf := new(bytes.Buffer), new(bytes.Buffer)
+	cmd.Stdout, cmd.Stderr = outbuf, errbuf
 
-	err = cmd.Start()
+	err := cmd.Start()
 	if err != nil {
 		return SystemVersionReport{}, err
 	}
 
-	var wg sync.WaitGroup
-	wg.Add(3)
+	// Use a channel to signal completion so we can use a select statement
+	done := make(chan error)
+	go func() { done <- cmd.Wait() }()
 
-	var result SystemVersionReport
-	go func() {
-		defer wg.Done()
-		// Reading from the pipe is a blocking call, hence this goroutine.
-		// The goroutine will exit after the pipe is closed or the command exits;
-		// these will be triggered by cmd.Wait() either after the timeout expires or the command finished.
-		err = json.NewDecoder(stdoutPipe).Decode(&result)
+	var timeoutErr error
+	select {
+	case <-time.After(o.podmanCmdInitTimeout):
+		err = cmd.Process.Kill()
 		if err != nil {
-			klog.V(3).Infof("unable to decode output: %v", err)
+			klog.V(3).Infof("unable to kill podman version process: %s", err)
 		}
-	}()
-
-	var stderr string
-	go func() {
-		defer wg.Done()
-		var buf bytes.Buffer
-		_, rErr := buf.ReadFrom(stderrPipe)
-		if rErr != nil {
-			klog.V(7).Infof("unable to read from stderr pipe: %v", rErr)
-		}
-		stderr = buf.String()
-	}()
-
-	var wErr error
-	go func() {
-		defer wg.Done()
-		// cmd.Wait() will block until the timeout expires or the program started by cmd exits.
-		// It will then close all resources associated with cmd,
-		// including the stdout and stderr pipes above, which will in turn terminate the goroutines spawned above.
-		wErr = cmd.Wait()
-	}()
+		timeoutErr = fmt.Errorf("timeout (%s) while waiting for Podman version", o.podmanCmdInitTimeout.Round(time.Second).String())
+		klog.V(3).Infof(timeoutErr.Error())
 
-	// Wait until all we are sure all previous goroutines are done
-	wg.Wait()
+	case err = <-done:
+		if err != nil {
+			klog.V(3).Infof("Non-zero exit code for podman version: %v", err)
 
-	if wErr != nil {
-		ctxErr := ctxWithTimeout.Err()
-		if ctxErr != nil {
-			msg := "error"
-			if errors.Is(ctxErr, context.DeadlineExceeded) {
-				msg = fmt.Sprintf("timeout (%s)", o.podmanCmdInitTimeout.Round(time.Second).String())
+			stderr := errbuf.String()
+			if len(stderr) > 0 {
+				klog.V(3).Infof("podman version stderr: %v", stderr)
 			}
-			wErr = fmt.Errorf("%s while waiting for Podman version: %s: %w", msg, ctxErr, wErr)
-		}
-		if exitErr, ok := wErr.(*exec.ExitError); ok {
-			wErr = fmt.Errorf("%s: %s", wErr, string(exitErr.Stderr))
-		}
-		if err != nil {
-			wErr = fmt.Errorf("%s: (%w)", wErr, err)
+
+			return SystemVersionReport{}, err
 		}
-		if stderr != "" {
-			wErr = fmt.Errorf("%w: %s", wErr, stderr)
+	}
+
+	var result SystemVersionReport
+	err = json.NewDecoder(outbuf).Decode(&result)
+	if err != nil {
+		klog.V(3).Infof("unable to decode output: %v", err)
+		if timeoutErr != nil {
+			return SystemVersionReport{}, timeoutErr
 		}
-		return SystemVersionReport{}, fmt.Errorf("%v. Please check the output of the following command: %v", wErr, cmd.Args)
+		return SystemVersionReport{}, err
 	}
 
 	return result, nil