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 <[email protected]>
containers · Nov 2, 2021 · f211547 · f211547
1 parent 3147ff8
commit f211547
Show file tree

Hide file tree

Showing 11 changed files with 94 additions and 23 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
+}