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 dc75dac5a..a2be44ab4 100644
--- a/pkg/bindings/connection.go
+++ b/pkg/bindings/connection.go
@@ -390,6 +390,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 3f78ba7bc..5b5a1912c 100644
--- a/pkg/domain/infra/tunnel/containers.go
+++ b/pkg/domain/infra/tunnel/containers.go
@@ -228,7 +228,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 b8af2de68..e17f746a5 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 79fba1943..069982d30 100644
--- a/pkg/domain/infra/tunnel/network.go
+++ b/pkg/domain/infra/tunnel/network.go
@@ -25,7 +25,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 cfd1574c3..ccb363935 100644
--- a/pkg/domain/infra/tunnel/volumes.go
+++ b/pkg/domain/infra/tunnel/volumes.go
@@ -59,7 +59,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 44a0c3efd..04110b62a 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
+}