Handle HTTP 409 error messages properly for Pod actions

This PR fixes the case when the API return HTTP 409 response. Where the
API return the body format different then for other HTTP error codes.

Signed-off-by: Ondra Machacek <omachace@redhat.com>

11 files changed, 94 insertions, 23 deletions

diff --git a/pkg/bindings/connection.go b/pkg/bindings/connection.go
index e2c46e481..2fe86ca39 100644
--- a/pkg/bindings/connection.go
+++ b/pkg/bindings/connection.go
@@ -379,6 +379,11 @@ func (h *APIResponse) IsClientError() bool {
 	return h.Response.StatusCode/100 == 4
 }
 
+// IsConflictError returns true if the response code is 409
+func (h *APIResponse) IsConflictError() bool {
+	return h.Response.StatusCode == 409
+}
+
 // IsServerError returns true if the response code is 5xx
 func (h *APIResponse) IsServerError() bool {
 	return h.Response.StatusCode/100 == 5
diff --git a/pkg/bindings/errors.go b/pkg/bindings/errors.go
index 9c311d912..ec837b39c 100644
--- a/pkg/bindings/errors.go
+++ b/pkg/bindings/errors.go
@@ -12,17 +12,22 @@ var (
 	ErrNotImplemented = errors.New("function not implemented")
 )
 
-func handleError(data []byte) error {
-	e := errorhandling.ErrorModel{}
-	if err := json.Unmarshal(data, &e); err != nil {
+func handleError(data []byte, unmarshalErrorInto interface{}) error {
+	if err := json.Unmarshal(data, unmarshalErrorInto); err != nil {
 		return err
 	}
-	return e
+	return unmarshalErrorInto.(error)
 }
 
 // Process drains the response body, and processes the HTTP status code
 // Note: Closing the response.Body is left to the caller
 func (h APIResponse) Process(unmarshalInto interface{}) error {
+	return h.ProcessWithError(unmarshalInto, &errorhandling.ErrorModel{})
+}
+
+// Process drains the response body, and processes the HTTP status code
+// Note: Closing the response.Body is left to the caller
+func (h APIResponse) ProcessWithError(unmarshalInto interface{}, unmarshalErrorInto interface{}) error {
 	data, err := ioutil.ReadAll(h.Response.Body)
 	if err != nil {
 		return errors.Wrap(err, "unable to process API response")
@@ -33,14 +38,22 @@ func (h APIResponse) Process(unmarshalInto interface{}) error {
 		}
 		return nil
 	}
+
+	if h.IsConflictError() {
+		return handleError(data, unmarshalErrorInto)
+	}
+
 	// TODO should we add a debug here with the response code?
-	return handleError(data)
+	return handleError(data, &errorhandling.ErrorModel{})
 }
 
 func CheckResponseCode(inError error) (int, error) {
-	e, ok := inError.(errorhandling.ErrorModel)
-	if !ok {
+	switch e := inError.(type) {
+	case *errorhandling.ErrorModel:
+		return e.Code(), nil
+	case *errorhandling.PodConflictErrorModel:
+		return e.Code(), nil
+	default:
 		return -1, errors.New("error is not type ErrorModel")
 	}
-	return e.Code(), nil
 }
diff --git a/pkg/bindings/pods/pods.go b/pkg/bindings/pods/pods.go
index a1a431a3b..3b5832373 100644
--- a/pkg/bindings/pods/pods.go
+++ b/pkg/bindings/pods/pods.go
@@ -9,6 +9,7 @@ import (
 	"github.com/containers/podman/v3/pkg/api/handlers"
 	"github.com/containers/podman/v3/pkg/bindings"
 	"github.com/containers/podman/v3/pkg/domain/entities"
+	"github.com/containers/podman/v3/pkg/errorhandling"
 	jsoniter "github.com/json-iterator/go"
 )
 
@@ -97,7 +98,7 @@ func Kill(ctx context.Context, nameOrID string, options *KillOptions) (*entities
 	}
 	defer response.Body.Close()
 
-	return &report, response.Process(&report)
+	return &report, response.ProcessWithError(&report, &errorhandling.PodConflictErrorModel{})
 }
 
 // Pause pauses all running containers in a given pod.
@@ -117,7 +118,7 @@ func Pause(ctx context.Context, nameOrID string, options *PauseOptions) (*entiti
 	}
 	defer response.Body.Close()
 
-	return &report, response.Process(&report)
+	return &report, response.ProcessWithError(&report, &errorhandling.PodConflictErrorModel{})
 }
 
 // Prune by default removes all non-running pods in local storage.
@@ -184,7 +185,7 @@ func Restart(ctx context.Context, nameOrID string, options *RestartOptions) (*en
 	}
 	defer response.Body.Close()
 
-	return &report, response.Process(&report)
+	return &report, response.ProcessWithError(&report, &errorhandling.PodConflictErrorModel{})
 }
 
 // Remove deletes a Pod from from local storage. The optional force parameter denotes
@@ -232,7 +233,8 @@ func Start(ctx context.Context, nameOrID string, options *StartOptions) (*entiti
 		report.Id = nameOrID
 		return &report, nil
 	}
-	return &report, response.Process(&report)
+
+	return &report, response.ProcessWithError(&report, &errorhandling.PodConflictErrorModel{})
 }
 
 // Stop stops all containers in a Pod. The optional timeout parameter can be
@@ -260,7 +262,7 @@ func Stop(ctx context.Context, nameOrID string, options *StopOptions) (*entities
 		report.Id = nameOrID
 		return &report, nil
 	}
-	return &report, response.Process(&report)
+	return &report, response.ProcessWithError(&report, &errorhandling.PodConflictErrorModel{})
 }
 
 // Top gathers statistics about the running processes in a pod. The nameOrID can be a pod name
@@ -316,7 +318,7 @@ func Unpause(ctx context.Context, nameOrID string, options *UnpauseOptions) (*en
 	}
 	defer response.Body.Close()
 
-	return &report, response.Process(&report)
+	return &report, response.ProcessWithError(&report, &errorhandling.PodConflictErrorModel{})
 }
 
 // Stats display resource-usage statistics of one or more pods.
diff --git a/pkg/bindings/test/common_test.go b/pkg/bindings/test/common_test.go
index 91ebe21fc..d996595bf 100644
--- a/pkg/bindings/test/common_test.go
+++ b/pkg/bindings/test/common_test.go
@@ -225,12 +225,23 @@ func (b *bindingTest) RunTopContainer(containerName *string, podName *string) (s
 // This method creates a pod with the given pod name.
 // Podname is an optional parameter
 func (b *bindingTest) Podcreate(name *string) {
+	b.PodcreateAndExpose(name, nil)
+}
+
+// This method creates a pod with the given pod name and publish port.
+// Podname is an optional parameter
+// port is an optional parameter
+func (b *bindingTest) PodcreateAndExpose(name *string, port *string) {
+	command := []string{"pod", "create"}
 	if name != nil {
 		podname := *name
-		b.runPodman([]string{"pod", "create", "--name", podname}).Wait(45)
-	} else {
-		b.runPodman([]string{"pod", "create"}).Wait(45)
+		command = append(command, "--name", podname)
+	}
+	if port != nil {
+		podport := *port
+		command = append(command, "--publish", podport)
 	}
+	b.runPodman(command).Wait(45)
 }
 
 //  StringInSlice returns a boolean based on whether a given
diff --git a/pkg/bindings/test/pods_test.go b/pkg/bindings/test/pods_test.go
index 5331cf439..879d4d00d 100644
--- a/pkg/bindings/test/pods_test.go
+++ b/pkg/bindings/test/pods_test.go
@@ -1,6 +1,7 @@
 package test_bindings
 
 import (
+	"fmt"
 	"net/http"
 	"strings"
 	"time"
@@ -9,7 +10,9 @@ import (
 	"github.com/containers/podman/v3/pkg/bindings"
 	"github.com/containers/podman/v3/pkg/bindings/pods"
 	"github.com/containers/podman/v3/pkg/domain/entities"
+	"github.com/containers/podman/v3/pkg/errorhandling"
 	"github.com/containers/podman/v3/pkg/specgen"
+	"github.com/containers/podman/v3/utils"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	"github.com/onsi/gomega/gexec"
@@ -208,6 +211,29 @@ var _ = Describe("Podman pods", func() {
 		}
 	})
 
+	It("start pod with port conflict", func() {
+		randomport, err := utils.GetRandomPort()
+		Expect(err).To(BeNil())
+
+		portPublish := fmt.Sprintf("%d:%d", randomport, randomport)
+		var podwithport string = "newpodwithport"
+		bt.PodcreateAndExpose(&podwithport, &portPublish)
+
+		// Start pod and expose port 12345
+		_, err = pods.Start(bt.conn, podwithport, nil)
+		Expect(err).To(BeNil())
+
+		// Start another pod and expose same port 12345
+		var podwithport2 string = "newpodwithport2"
+		bt.PodcreateAndExpose(&podwithport2, &portPublish)
+
+		_, err = pods.Start(bt.conn, podwithport2, nil)
+		Expect(err).ToNot(BeNil())
+		code, _ := bindings.CheckResponseCode(err)
+		Expect(code).To(BeNumerically("==", http.StatusConflict))
+		Expect(err).To(BeAssignableToTypeOf(&errorhandling.PodConflictErrorModel{}))
+	})
+
 	It("start stop restart pod", func() {
 		// Start an invalid pod
 		_, err = pods.Start(bt.conn, "dummyName", nil)
diff --git a/pkg/domain/infra/tunnel/containers.go b/pkg/domain/infra/tunnel/containers.go
index b638bfe24..c4d62bd89 100644
--- a/pkg/domain/infra/tunnel/containers.go
+++ b/pkg/domain/infra/tunnel/containers.go
@@ -226,7 +226,7 @@ func (ic *ContainerEngine) ContainerInspect(ctx context.Context, namesOrIds []st
 	for _, name := range namesOrIds {
 		inspect, err := containers.Inspect(ic.ClientCtx, name, options)
 		if err != nil {
-			errModel, ok := err.(errorhandling.ErrorModel)
+			errModel, ok := err.(*errorhandling.ErrorModel)
 			if !ok {
 				return nil, nil, err
 			}
diff --git a/pkg/domain/infra/tunnel/images.go b/pkg/domain/infra/tunnel/images.go
index 9a746d68c..ab2a93c75 100644
--- a/pkg/domain/infra/tunnel/images.go
+++ b/pkg/domain/infra/tunnel/images.go
@@ -188,7 +188,7 @@ func (ir *ImageEngine) Inspect(ctx context.Context, namesOrIDs []string, opts en
 	for _, i := range namesOrIDs {
 		r, err := images.GetImage(ir.ClientCtx, i, options)
 		if err != nil {
-			errModel, ok := err.(errorhandling.ErrorModel)
+			errModel, ok := err.(*errorhandling.ErrorModel)
 			if !ok {
 				return nil, nil, err
 			}
diff --git a/pkg/domain/infra/tunnel/network.go b/pkg/domain/infra/tunnel/network.go
index 7e59e44c2..7c183b1ac 100644
--- a/pkg/domain/infra/tunnel/network.go
+++ b/pkg/domain/infra/tunnel/network.go
@@ -23,7 +23,7 @@ func (ic *ContainerEngine) NetworkInspect(ctx context.Context, namesOrIds []stri
 	for _, name := range namesOrIds {
 		report, err := network.Inspect(ic.ClientCtx, name, options)
 		if err != nil {
-			errModel, ok := err.(errorhandling.ErrorModel)
+			errModel, ok := err.(*errorhandling.ErrorModel)
 			if !ok {
 				return nil, nil, err
 			}
diff --git a/pkg/domain/infra/tunnel/secrets.go b/pkg/domain/infra/tunnel/secrets.go
index 6337c7fbe..e5fa200bd 100644
--- a/pkg/domain/infra/tunnel/secrets.go
+++ b/pkg/domain/infra/tunnel/secrets.go
@@ -28,7 +28,7 @@ func (ic *ContainerEngine) SecretInspect(ctx context.Context, nameOrIDs []string
 	for _, name := range nameOrIDs {
 		inspected, err := secrets.Inspect(ic.ClientCtx, name, nil)
 		if err != nil {
-			errModel, ok := err.(errorhandling.ErrorModel)
+			errModel, ok := err.(*errorhandling.ErrorModel)
 			if !ok {
 				return nil, nil, err
 			}
@@ -67,7 +67,7 @@ func (ic *ContainerEngine) SecretRm(ctx context.Context, nameOrIDs []string, opt
 	for _, name := range nameOrIDs {
 		secret, err := secrets.Inspect(ic.ClientCtx, name, nil)
 		if err != nil {
-			errModel, ok := err.(errorhandling.ErrorModel)
+			errModel, ok := err.(*errorhandling.ErrorModel)
 			if !ok {
 				return nil, err
 			}
diff --git a/pkg/domain/infra/tunnel/volumes.go b/pkg/domain/infra/tunnel/volumes.go
index 2b2b2c2a1..a3d9e1b6d 100644
--- a/pkg/domain/infra/tunnel/volumes.go
+++ b/pkg/domain/infra/tunnel/volumes.go
@@ -56,7 +56,7 @@ func (ic *ContainerEngine) VolumeInspect(ctx context.Context, namesOrIds []strin
 	for _, id := range namesOrIds {
 		data, err := volumes.Inspect(ic.ClientCtx, id, nil)
 		if err != nil {
-			errModel, ok := err.(errorhandling.ErrorModel)
+			errModel, ok := err.(*errorhandling.ErrorModel)
 			if !ok {
 				return nil, nil, err
 			}
diff --git a/pkg/errorhandling/errorhandling.go b/pkg/errorhandling/errorhandling.go
index 6adbc9f34..fcb0f7881 100644
--- a/pkg/errorhandling/errorhandling.go
+++ b/pkg/errorhandling/errorhandling.go
@@ -83,6 +83,12 @@ func Contains(err error, sub error) bool {
 	return strings.Contains(err.Error(), sub.Error())
 }
 
+// PodConflictErrorModel is used in remote connections with podman
+type PodConflictErrorModel struct {
+	Errs []string
+	Id   string //nolint
+}
+
 // ErrorModel is used in remote connections with podman
 type ErrorModel struct {
 	// API root cause formatted for automated parsing
@@ -106,3 +112,11 @@ func (e ErrorModel) Cause() error {
 func (e ErrorModel) Code() int {
 	return e.ResponseCode
 }
+
+func (e PodConflictErrorModel) Error() string {
+	return strings.Join(e.Errs, ",")
+}
+
+func (e PodConflictErrorModel) Code() int {
+	return 409
+}