dbd2d06659
Issue #34 reports container Recreate / native auto-update (#28, #10) failing on modern Docker Engines via the agent with "starting container with non-empty request body ... removed in v1.24". The proposed fix: make API-version resolution robust in all three docker client constructors. - api/docker/client/client.go: add a shared negotiateWithFloor() used by createLocalClient, createTCPClient, createAgentClient. It eagerly NegotiateAPIVersion (10s bound) instead of relying on lazy first-call negotiation, and enforces a modern floor: if the negotiated version is empty or < 1.24 (versions.LessThan), rebuild pinned at 1.44 (WithVersion, which the SDK requires INSTEAD of negotiation — WithVersion sets manualOverride, disabling NegotiateAPIVersion). Negotiation stays primary; the floor is only a safety net. Effective API version is now guaranteed >= 1.24. Custom transport / TLS / agent headers untouched. - Tests (api/docker/client/client_test.go): modern daemon -> empty start body; below-floor daemon -> pinned to floor; unreachable daemon -> stays >= 1.24. IMPORTANT FINDING (needs maintainer confirmation before this is called a complete fix for #34): on the pinned SDK (github.com/docker/docker v28.5.2), ContainerStart posts a nil body UNCONDITIONALLY (client/container_start.go: cli.post(ctx, ".../start", query, nil, nil) — no version branch), and NewClientWithOpts defaults to version 1.51 (negotiation only downgrades). So on develop, the server-side path the issue names (autoupdate.go -> Recreate -> cli.ContainerStart) cannot emit a request body regardless of API version — this change hardens the client path but is unlikely to be the actual source of the reported symptom. The "non-empty body" more likely originates from a DIFFERENT layer (an OLDER docker SDK in the failing deployment, where ContainerStart did attach HostConfig at API < 1.24; or the docker PROXY forwarding a raw browser request). Recommend confirming the failing environment's build before merging as the #34 fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
156 lines
5.0 KiB
Go
156 lines
5.0 KiB
Go
package client
|
|
|
|
import (
|
|
"context"
|
|
"io"
|
|
"net/http"
|
|
"net/http/httptest"
|
|
"strings"
|
|
"testing"
|
|
"time"
|
|
|
|
portainer "github.com/portainer/portainer/api"
|
|
"github.com/portainer/portainer/pkg/fips"
|
|
|
|
dockercontainer "github.com/docker/docker/api/types/container"
|
|
"github.com/docker/docker/api/types/versions"
|
|
"github.com/docker/docker/client"
|
|
"github.com/stretchr/testify/require"
|
|
)
|
|
|
|
func TestHttpClient(t *testing.T) {
|
|
t.Parallel()
|
|
fips.InitFIPS(false)
|
|
|
|
// Valid TLS configuration
|
|
endpoint := &portainer.Endpoint{}
|
|
endpoint.TLSConfig = portainer.TLSConfiguration{TLS: true}
|
|
|
|
cli, err := httpClient(endpoint, nil)
|
|
require.NoError(t, err)
|
|
require.NotNil(t, cli)
|
|
|
|
// Invalid TLS configuration
|
|
endpoint.TLSConfig.TLSCertPath = "/invalid/path/client.crt"
|
|
endpoint.TLSConfig.TLSKeyPath = "/invalid/path/client.key"
|
|
|
|
cli, err = httpClient(endpoint, nil)
|
|
require.Error(t, err)
|
|
require.Nil(t, cli)
|
|
}
|
|
|
|
// startRecord captures the request made to POST /containers/{id}/start.
|
|
type startRecord struct {
|
|
path string
|
|
body []byte
|
|
}
|
|
|
|
// newDockerStub returns an httptest server that mimics a Docker daemon
|
|
// advertising apiVersion, and records the request made to the container start
|
|
// endpoint into rec.
|
|
func newDockerStub(t *testing.T, apiVersion string, rec *startRecord) *httptest.Server {
|
|
t.Helper()
|
|
|
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
|
switch {
|
|
case strings.HasSuffix(r.URL.Path, "/_ping"):
|
|
w.Header().Set("Api-Version", apiVersion)
|
|
w.Header().Set("Ostype", "linux")
|
|
w.WriteHeader(http.StatusOK)
|
|
case strings.HasSuffix(r.URL.Path, "/start"):
|
|
body, _ := io.ReadAll(r.Body)
|
|
rec.path = r.URL.Path
|
|
rec.body = body
|
|
w.WriteHeader(http.StatusNoContent)
|
|
default:
|
|
w.WriteHeader(http.StatusOK)
|
|
_, _ = w.Write([]byte("{}"))
|
|
}
|
|
}))
|
|
t.Cleanup(srv.Close)
|
|
|
|
return srv
|
|
}
|
|
|
|
// TestNegotiateWithFloor_ModernDaemon verifies that against a daemon
|
|
// advertising a modern API version (>= 1.24) the client negotiates a modern
|
|
// version and issues POST /containers/{id}/start with an EMPTY body — the
|
|
// behavior required by API >= 1.24 (regression test for issue #34).
|
|
func TestNegotiateWithFloor_ModernDaemon(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
var rec startRecord
|
|
srv := newDockerStub(t, "1.54", &rec)
|
|
|
|
opts := []client.Opt{
|
|
client.WithHost(srv.URL),
|
|
client.WithAPIVersionNegotiation(),
|
|
}
|
|
|
|
cli, err := negotiateWithFloor(opts, negotiateTimeout)
|
|
require.NoError(t, err)
|
|
t.Cleanup(func() { _ = cli.Close() })
|
|
|
|
// Negotiation resolved a concrete, modern version (capped at the client's
|
|
// maximum supported version).
|
|
require.NotEmpty(t, cli.ClientVersion())
|
|
require.False(t, versions.LessThan(cli.ClientVersion(), minSupportedAPIVersion),
|
|
"effective API version %q must be >= %q", cli.ClientVersion(), minSupportedAPIVersion)
|
|
|
|
err = cli.ContainerStart(context.Background(), "abc123", dockercontainer.StartOptions{})
|
|
require.NoError(t, err)
|
|
|
|
// The request must be versioned and carry no HostConfig body.
|
|
require.Contains(t, rec.path, "/v"+cli.ClientVersion()+"/")
|
|
require.Empty(t, rec.body, "start request body must be empty on API >= 1.24")
|
|
}
|
|
|
|
// TestNegotiateWithFloor_BelowFloorDaemon verifies the floor branch: a daemon
|
|
// that advertises an API version below the supported floor causes negotiation
|
|
// to resolve a legacy version, and the client must then be pinned at
|
|
// modernAPIVersionFloor instead of issuing requests below the supported floor.
|
|
func TestNegotiateWithFloor_BelowFloorDaemon(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
var rec startRecord
|
|
// 1.20 is below minSupportedAPIVersion (1.24); negotiation would downgrade
|
|
// the client to it, which is exactly what must NOT be used for requests.
|
|
srv := newDockerStub(t, "1.20", &rec)
|
|
|
|
opts := []client.Opt{
|
|
client.WithHost(srv.URL),
|
|
client.WithAPIVersionNegotiation(),
|
|
}
|
|
|
|
cli, err := negotiateWithFloor(opts, negotiateTimeout)
|
|
require.NoError(t, err)
|
|
t.Cleanup(func() { _ = cli.Close() })
|
|
|
|
// The client is pinned at the modern floor, not the legacy 1.20.
|
|
require.Equal(t, modernAPIVersionFloor, cli.ClientVersion())
|
|
require.False(t, versions.LessThan(cli.ClientVersion(), minSupportedAPIVersion),
|
|
"floor version %q must be >= %q", cli.ClientVersion(), minSupportedAPIVersion)
|
|
}
|
|
|
|
// TestNegotiateWithFloor_UnreachableDaemon verifies that when the daemon is
|
|
// unreachable at construction (negotiation cannot ping it) the client still
|
|
// resolves to an effective API version >= the supported floor, never a legacy
|
|
// default.
|
|
func TestNegotiateWithFloor_UnreachableDaemon(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
opts := []client.Opt{
|
|
// Unreachable daemon: connection is refused immediately.
|
|
client.WithHost("tcp://127.0.0.1:1"),
|
|
client.WithAPIVersionNegotiation(),
|
|
}
|
|
|
|
cli, err := negotiateWithFloor(opts, 2*time.Second)
|
|
require.NoError(t, err)
|
|
t.Cleanup(func() { _ = cli.Close() })
|
|
|
|
require.NotEmpty(t, cli.ClientVersion())
|
|
require.False(t, versions.LessThan(cli.ClientVersion(), minSupportedAPIVersion),
|
|
"effective API version %q must be >= %q", cli.ClientVersion(), minSupportedAPIVersion)
|
|
}
|