600cec6ccc
Round-1 review found the floor was DEFEATED for the main #34 target (daemon unreachable at construction): the SDK keeps ClientVersion() == "1.51" on a ping failure (not empty), so the old empty-string guard passed, the floor branch was skipped, lazy negotiation stayed armed, and the client could downgrade below 1.24 on the first real request. - Detect unreachable EXPLICITLY via cli.Ping(ctx) instead of inspecting ClientVersion(): ping error -> rebuildAtFloor; otherwise NegotiateAPIVersionPing(ping) applies the advertised version AND marks the client negotiated (disarms the lazy re-ping); if the result is empty or < 1.24 -> rebuildAtFloor. rebuildAtFloor closes and rebuilds with WithVersion(minSupportedAPIVersion) — which sets manualOverride, making both the lazy checkVersion and NegotiateAPIVersion no-ops, so the pinned client can NEVER re-negotiate below the floor. The rebuild reuses the same opts, so agent-signature headers / TLS / custom transport are preserved. - Pin the floor at minSupportedAPIVersion "1.24", not "1.44": 1.44 is "too new" for a genuinely old daemon (400) and would make container.go's `< 1.44` MAC-cleanup wrongly skip. 1.24 satisfies the invariant and every supported daemon accepts it. Dropped the 1.44 const. - resolveNegotiateTimeout(timeout): use the caller's timeout when set (deployer's 3600s, autoupdate/autoheal), else a modest 10s default. The proxy hotpath and snapshot loop pass nil -> bounded 10s, so a daemon that accepts TCP but hangs on /_ping can't stall the whole snapshot loop for a huge fixed time. - Test TestNegotiateWithFloor_UnreachableThenBelowFloor: unreachable at construction, then the daemon advertises 1.20; asserts the client pins to 1.24, ContainerStart goes /v1.24/ (not /v1.20/), and /_ping isn't hit again after construction (no lazy re-negotiation). Mutation-verified: reverting to the old logic reds it. go build / vet / gofmt clean; go test ./docker/client/ -race 4/4 pass. Still framed as a hardening (part of #34) — the vendored SDK's ContainerStart sends no body regardless, so confirm the failing deployment's build before treating this as the complete #34 fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
203 lines
6.4 KiB
Go
203 lines
6.4 KiB
Go
package client
|
|
|
|
import (
|
|
"context"
|
|
"io"
|
|
"net/http"
|
|
"net/http/httptest"
|
|
"strings"
|
|
"sync/atomic"
|
|
"testing"
|
|
|
|
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, defaultNegotiateTimeout)
|
|
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
|
|
// minSupportedAPIVersion 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, defaultNegotiateTimeout)
|
|
require.NoError(t, err)
|
|
t.Cleanup(func() { _ = cli.Close() })
|
|
|
|
// The client is pinned at the supported floor, not the legacy 1.20.
|
|
require.Equal(t, minSupportedAPIVersion, cli.ClientVersion())
|
|
}
|
|
|
|
// TestNegotiateWithFloor_UnreachableThenBelowFloor covers the exact issue #34
|
|
// scenario the reviewer reproduced: the daemon is UNREACHABLE at construction
|
|
// time, then comes back up advertising a legacy version (1.20). The client must
|
|
// pin at the floor during construction AND must not lazily re-negotiate down to
|
|
// the legacy version on the first real request.
|
|
func TestNegotiateWithFloor_UnreachableThenBelowFloor(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
var (
|
|
reachable atomic.Bool // false during construction, true afterwards
|
|
pingHits atomic.Int32
|
|
rec startRecord
|
|
)
|
|
|
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
|
switch {
|
|
case strings.HasSuffix(r.URL.Path, "/_ping"):
|
|
pingHits.Add(1)
|
|
if !reachable.Load() {
|
|
// Unreachable: make Ping fail (both HEAD and its GET fallback).
|
|
w.WriteHeader(http.StatusServiceUnavailable)
|
|
|
|
return
|
|
}
|
|
|
|
// Daemon is now up, but advertises a legacy, below-floor version.
|
|
w.Header().Set("Api-Version", "1.20")
|
|
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)
|
|
|
|
opts := []client.Opt{
|
|
client.WithHost(srv.URL),
|
|
client.WithAPIVersionNegotiation(),
|
|
}
|
|
|
|
// Construction happens while the daemon is unreachable.
|
|
cli, err := negotiateWithFloor(opts, defaultNegotiateTimeout)
|
|
require.NoError(t, err)
|
|
t.Cleanup(func() { _ = cli.Close() })
|
|
|
|
require.Equal(t, minSupportedAPIVersion, cli.ClientVersion(),
|
|
"unreachable daemon must pin the client at the floor")
|
|
|
|
pingsAfterConstruction := pingHits.Load()
|
|
|
|
// The daemon comes back up advertising a legacy version.
|
|
reachable.Store(true)
|
|
|
|
err = cli.ContainerStart(context.Background(), "abc123", dockercontainer.StartOptions{})
|
|
require.NoError(t, err)
|
|
|
|
// The floor must hold: the request is issued at the pinned version, not the
|
|
// daemon's legacy 1.20, and the client did not re-ping to re-negotiate.
|
|
require.Contains(t, rec.path, "/v"+minSupportedAPIVersion+"/",
|
|
"start must be issued at the pinned floor version, not the legacy 1.20")
|
|
require.NotContains(t, rec.path, "/v1.20/")
|
|
require.Equal(t, pingsAfterConstruction, pingHits.Load(),
|
|
"client must not lazily re-negotiate after being pinned at the floor")
|
|
}
|