From 86bb91001a0540d58281d16ee2111c679a260506 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 29 Nov 2023 13:06:50 +0100 Subject: [PATCH 1/2] fix podman-remote exec regression with v4.8 Commit f48a706abc added a new API endpoint to remove exec session correctly. And the bindings try to call that endpoint for exec every time. Now since client and server must not be the same version this causes a problem if a new 4.8 client calls an older 4.7 server as it has no idea about such endpoint and throws an ugly error. This is a common scenario for podman machine setups. The client does know the server version so it should make sure to not call such endpoint if the server is older than 4.8. I added a exec test to the machine tests as this can be reproduced with podman machine as at the moment at least the VM image does not contain podman 4.8. And it should at least make sure podman exec keeps working for podman machine without regressions. Fixes #20821 Signed-off-by: Paul Holzinger --- pkg/bindings/containers/exec.go | 7 ++++++- pkg/machine/e2e/basic_test.go | 8 +++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/pkg/bindings/containers/exec.go b/pkg/bindings/containers/exec.go index 9a73a656ea..a536213a22 100644 --- a/pkg/bindings/containers/exec.go +++ b/pkg/bindings/containers/exec.go @@ -114,10 +114,15 @@ func ExecStart(ctx context.Context, sessionID string, options *ExecStartOptions) // ExecRemove removes a given exec session. func ExecRemove(ctx context.Context, sessionID string, options *ExecRemoveOptions) error { + v := bindings.ServiceVersion(ctx) + // The exec remove endpoint was added in 4.8. + if v.Major < 4 || (v.Major == 4 && v.Minor < 8) { + // Do no call this endpoint as it will not be supported on the server and throw an "NOT FOUND" error. + return nil + } if options == nil { options = new(ExecRemoveOptions) } - _ = options conn, err := bindings.GetClient(ctx) if err != nil { return err diff --git a/pkg/machine/e2e/basic_test.go b/pkg/machine/e2e/basic_test.go index a1cfb71ef5..ab5c5c6312 100644 --- a/pkg/machine/e2e/basic_test.go +++ b/pkg/machine/e2e/basic_test.go @@ -62,12 +62,18 @@ var _ = Describe("run basic podman commands", func() { Expect(err).ToNot(HaveOccurred()) Expect(session).To(Exit(0)) + ctrName := "test" bm := basicMachine{} - runAlp, err := mb.setCmd(bm.withPodmanCommand([]string{"run", "-dt", "-p", "62544:80", "quay.io/libpod/alpine_nginx"})).run() + runAlp, err := mb.setCmd(bm.withPodmanCommand([]string{"run", "-dt", "--name", ctrName, "-p", "62544:80", "quay.io/libpod/alpine_nginx"})).run() Expect(err).ToNot(HaveOccurred()) Expect(runAlp).To(Exit(0)) testHTTPServer("62544", false, "podman rulez") + // Test exec in machine scenario: https://github.com/containers/podman/issues/20821 + exec, err := mb.setCmd(bm.withPodmanCommand([]string{"exec", ctrName, "true"})).run() + Expect(err).ToNot(HaveOccurred()) + Expect(exec).To(Exit(0)) + out, err := pgrep("gvproxy") Expect(err).ToNot(HaveOccurred()) Expect(out).ToNot(BeEmpty()) From a8b8dc57069e48745481b72940c36bbdbcf2d7f9 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 29 Nov 2023 15:27:25 +0100 Subject: [PATCH 2/2] pkg/bindings: add new APIVersionError error type When a new API call is added to the bindings we should guard it based on the version and throw a useful error. Right now an old server that does not implement a given endpoint would throw a "NOT FOUND" error which is not good for callers. Instead implement a custom error type to give a usefule error instead. This allows bindings users to call errors.As() to know if they call and to old version. Signed-off-by: Paul Holzinger --- pkg/bindings/containers/exec.go | 2 +- pkg/bindings/errors.go | 24 ++++++++++++++++++++++++ pkg/domain/infra/tunnel/containers.go | 6 ++++++ 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/pkg/bindings/containers/exec.go b/pkg/bindings/containers/exec.go index a536213a22..395ef06dda 100644 --- a/pkg/bindings/containers/exec.go +++ b/pkg/bindings/containers/exec.go @@ -118,7 +118,7 @@ func ExecRemove(ctx context.Context, sessionID string, options *ExecRemoveOption // The exec remove endpoint was added in 4.8. if v.Major < 4 || (v.Major == 4 && v.Minor < 8) { // Do no call this endpoint as it will not be supported on the server and throw an "NOT FOUND" error. - return nil + return bindings.NewAPIVersionError("/exec/{id}/remove", v, "4.8.0") } if options == nil { options = new(ExecRemoveOptions) diff --git a/pkg/bindings/errors.go b/pkg/bindings/errors.go index 039d2187b4..a7cbeb30ae 100644 --- a/pkg/bindings/errors.go +++ b/pkg/bindings/errors.go @@ -6,6 +6,7 @@ import ( "fmt" "io" + "github.com/blang/semver/v4" "github.com/containers/podman/v4/pkg/errorhandling" ) @@ -61,3 +62,26 @@ func CheckResponseCode(inError error) (int, error) { return -1, errors.New("is not type ErrorModel") } } + +type APIVersionError struct { + endpoint string + serverVersion *semver.Version + requiredVersion string +} + +// NewAPIVersionError create bindings error when the endpoint on the server is not supported +// because the version is to old. +// - endpoint is the name fo the endpoint (e.g. /containers/json) +// - version is the server API version +// - requiredVersion is the server version need to use said endpoint +func NewAPIVersionError(endpoint string, version *semver.Version, requiredVersion string) *APIVersionError { + return &APIVersionError{ + endpoint: endpoint, + serverVersion: version, + requiredVersion: requiredVersion, + } +} + +func (e *APIVersionError) Error() string { + return fmt.Sprintf("API server version is %s, need at least %s to call %s", e.serverVersion.String(), e.requiredVersion, e.endpoint) +} diff --git a/pkg/domain/infra/tunnel/containers.go b/pkg/domain/infra/tunnel/containers.go index 55dc6a0dfb..d57d2cd669 100644 --- a/pkg/domain/infra/tunnel/containers.go +++ b/pkg/domain/infra/tunnel/containers.go @@ -17,6 +17,7 @@ import ( "github.com/containers/podman/v4/libpod/define" "github.com/containers/podman/v4/libpod/events" "github.com/containers/podman/v4/pkg/api/handlers" + "github.com/containers/podman/v4/pkg/bindings" "github.com/containers/podman/v4/pkg/bindings/containers" "github.com/containers/podman/v4/pkg/bindings/images" "github.com/containers/podman/v4/pkg/domain/entities" @@ -588,6 +589,11 @@ func (ic *ContainerEngine) ContainerExec(ctx context.Context, nameOrID string, o } defer func() { if err := containers.ExecRemove(ic.ClientCtx, sessionID, nil); err != nil { + apiErr := new(bindings.APIVersionError) + if errors.As(err, &apiErr) { + // if the API is to old do not throw an error + return + } if retErr == nil { exitCode = -1 retErr = err