podman-registry: simpler, safer invocations

First: fix podman-registry script so it preserves the initial $PODMAN,
so all subsequent invocations of ps, logs, and stop will use the
same binary and arguments. Until now we've handled this by requiring
that our caller manage $PODMAN (and keep it the same), but that's
just wrong.

Next, simplify the golang interface: move the $PODMAN setting into
registry.go, instead of requiring e2e callers to set it. (This
could use some work: the local/remote conditional is icky).

IMPORTANT: To prevent registry.go from using the wrong podman binary,
the Start() call is gone. Only StartWithOptions() is valid now.

And, minor cleanup: comments, and add an actual error-message check

Reason for this PR is a recurring flake, #18355, whose multiple
failure modes I truly can't understand. I don't think this PR
is going to fix it, but this is still necessary work.

Signed-off-by: Ed Santiago <santiago@redhat.com>

hack/podman-registry

@@ -99,6 +99,9 @@ function podman() {
         fi
     fi
 
+    # Reset $PODMAN, so ps/logs/stop use same args as the initial start
+    PODMAN="$(<${PODMAN_REGISTRY_WORKDIR}/PODMAN)"
+
     ${PODMAN} --root    ${PODMAN_REGISTRY_WORKDIR}/root        \
               --runroot ${PODMAN_REGISTRY_WORKDIR}/runroot     \
               "$@"
@@ -174,6 +177,10 @@ function do_start() {
 
     mkdir -p ${PODMAN_REGISTRY_WORKDIR}
 
+    # Preserve initial podman path & args, so all subsequent invocations
+    # of this script are consistent with the first one.
+    echo "$PODMAN" >${PODMAN_REGISTRY_WORKDIR}/PODMAN
+
     local AUTHDIR=${PODMAN_REGISTRY_WORKDIR}/auth
     mkdir -p $AUTHDIR
 

hack/podman-registry-go/registry.go

@@ -2,6 +2,7 @@ package registry
 
 import (
 	"fmt"
+	"os"
 	"strings"
 
 	"github.com/containers/podman/v4/utils"
@@ -33,16 +34,15 @@ type Registry struct {
 
 // Options allows for customizing a registry.
 type Options struct {
+	// PodmanPath - path to podman executable
+	PodmanPath string
+	// PodmanArgs - array of podman options
+	PodmanArgs []string
 	// Image - custom registry image.
 	Image string
 }
 
-// Start a new registry and return it along with it's image, user, password, and port.
-func Start() (*Registry, error) {
-	return StartWithOptions(nil)
-}
-
-// StartWithOptions a new registry and return it along with it's image, user, password, and port.
+// StartWithOptions a new registry and return it along with its image, user, password, and port.
 func StartWithOptions(options *Options) (*Registry, error) {
 	if options == nil {
 		options = &Options{}
@@ -54,8 +54,18 @@ func StartWithOptions(options *Options) (*Registry, error) {
 	}
 	args = append(args, "start")
 
+	podmanCmd := []string{"podman"}
+	if options.PodmanPath != "" {
+		podmanCmd[0] = options.PodmanPath
+	}
+	if len(options.PodmanArgs) != 0 {
+		podmanCmd = append(podmanCmd, options.PodmanArgs...)
+	}
+
 	// Start a registry.
+	os.Setenv("PODMAN", strings.Join(podmanCmd, " "))
 	out, err := utils.ExecCmd(binary, args...)
+	os.Unsetenv("PODMAN")
 	if err != nil {
 		return nil, fmt.Errorf("running %q: %s: %w", binary, out, err)
 	}

hack/podman-registry-go/registry_test.go

@@ -13,10 +13,14 @@ func TestStartAndStopMultipleRegistries(t *testing.T) {
 
 	registries := []*Registry{}
 
+	registryOptions := &Options{
+		PodmanPath: "../../bin/podman",
+	}
+
 	// Start registries.
 	var errors *multierror.Error
 	for i := 0; i < 3; i++ {
-		reg, err := Start()
+		reg, err := StartWithOptions(registryOptions)
 		if err != nil {
 			errors = multierror.Append(errors, err)
 			continue

pkg/bindings/test/auth_test.go

@@ -22,10 +22,14 @@ var _ = Describe("Podman images", func() {
 	)
 
 	BeforeEach(func() {
+		registryOptions := &podmanRegistry.Options{
+			PodmanPath: getPodmanBinary(),
+		}
+
 		// Note: we need to start the registry **before** setting up
 		// the test. Otherwise, the registry is not reachable for
 		// currently unknown reasons.
-		registry, err = podmanRegistry.Start()
+		registry, err = podmanRegistry.StartWithOptions(registryOptions)
 		Expect(err).ToNot(HaveOccurred())
 
 		bt = newBindingTest()

pkg/bindings/test/images_test.go

@@ -388,7 +388,10 @@ var _ = Describe("Podman images", func() {
 	})
 
 	It("Image Push", func() {
-		registry, err := podmanRegistry.Start()
+		registryOptions := &podmanRegistry.Options{
+			PodmanPath: getPodmanBinary(),
+		}
+		registry, err := podmanRegistry.StartWithOptions(registryOptions)
 		Expect(err).ToNot(HaveOccurred())
 
 		var writer bytes.Buffer

pkg/bindings/test/manifests_test.go

@@ -176,7 +176,10 @@ var _ = Describe("Podman manifests", func() {
 	})
 
 	It("Manifest Push", func() {
-		registry, err := podmanRegistry.Start()
+		registryOptions := &podmanRegistry.Options{
+			PodmanPath: getPodmanBinary(),
+		}
+		registry, err := podmanRegistry.StartWithOptions(registryOptions)
 		Expect(err).ToNot(HaveOccurred())
 
 		name := "quay.io/libpod/foobar:latest"

test/e2e/libpod_suite_remote_test.go

@@ -115,7 +115,7 @@ func (p *PodmanTestIntegration) StopRemoteService() {
 	}
 }
 
-// MakeOptions assembles all the podman main options
+// getRemoteOptions assembles all the podman main options
 func getRemoteOptions(p *PodmanTestIntegration, args []string) []string {
 	networkDir := p.NetworkConfigDir
 	podmanOptions := strings.Split(fmt.Sprintf("--root %s --runroot %s --runtime %s --conmon %s --network-config-dir %s --network-backend %s --cgroup-manager %s --tmpdir %s --events-backend %s --db-backend %s",

test/e2e/manifest_test.go

@@ -388,22 +388,21 @@ var _ = Describe("Podman manifest", func() {
 
 	It("authenticated push", func() {
 		registryOptions := &podmanRegistry.Options{
-			Image: "docker-archive:" + imageTarPath(REGISTRY_IMAGE),
+			PodmanPath: podmanTest.PodmanBinary,
+			PodmanArgs: podmanTest.MakeOptions(nil, false, false),
+			Image:      "docker-archive:" + imageTarPath(REGISTRY_IMAGE),
 		}
 
-		// registry script invokes $PODMAN; make sure we define that
-		// so it can use our same networking options.
-		opts := strings.Join(podmanTest.MakeOptions(nil, false, false), " ")
+		// Special case for remote: invoke local podman, with all
+		// network/storage/other args
 		if IsRemote() {
-			opts = strings.Join(getRemoteOptions(podmanTest, nil), " ")
+			registryOptions.PodmanArgs = getRemoteOptions(podmanTest, nil)
 		}
-		os.Setenv("PODMAN", podmanTest.PodmanBinary+" "+opts)
 		registry, err := podmanRegistry.StartWithOptions(registryOptions)
 		Expect(err).ToNot(HaveOccurred())
 		defer func() {
 			err := registry.Stop()
 			Expect(err).ToNot(HaveOccurred())
-			os.Unsetenv("PODMAN")
 		}()
 
 		session := podmanTest.Podman([]string{"manifest", "create", "foo"})
@@ -438,6 +437,7 @@ var _ = Describe("Podman manifest", func() {
 		push = podmanTest.Podman([]string{"manifest", "push", "--tls-verify=false", "--creds=podmantest:wrongpasswd", "foo", "localhost:" + registry.Port + "/credstest"})
 		push.WaitWithDefaultTimeout()
 		Expect(push).To(ExitWithError())
+		Expect(push.ErrorToString()).To(ContainSubstring(": authentication required"))
 
 		// push --rm after pull image (#15033)
 		push = podmanTest.Podman([]string{"manifest", "push", "--rm", "--tls-verify=false", "--creds=" + registry.User + ":" + registry.Password, "foo", "localhost:" + registry.Port + "/rmtest"})