Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions common/pkg/json-proxy/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@ import (
"os"
"sync"
"syscall"
"time"

"github.com/opencontainers/go-digest"
imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1"
"github.com/sirupsen/logrus"
"go.podman.io/common/pkg/config"
"go.podman.io/image/v5/image"
"go.podman.io/image/v5/manifest"
"go.podman.io/image/v5/pkg/blobinfocache"
Expand Down Expand Up @@ -106,6 +108,54 @@ func (h *handler) Initialize(ctx context.Context, args []any) (replyBuf, error)
return r, nil
}

// GetEngineConfig returns a curated subset of the host containers.conf
// [engine] settings (new in 0.2.9). Today this is the retry policy; clients
// use it to drive retries with the same defaults as `podman pull` without
// having to read containers.conf themselves.
//
// Note the proxy only advertises the configuration; it does not perform
// retries itself, since a partially streamed blob cannot be transparently
// resumed.
func (h *handler) GetEngineConfig(_ context.Context, args []any) (replyBuf, error) {
h.lock.Lock()
defer h.lock.Unlock()

var ret replyBuf

if h.sysctx == nil {
return ret, errors.New("client error: must invoke Initialize")
}
if len(args) != 0 {
return ret, errors.New("invalid request, expecting zero arguments")
}

// config.Default() memoizes internally, so this is cheap to call here.
cfg, err := config.Default()
Comment on lines +132 to +133

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know where little about that proxy but since it is used by skopeo never depended on containers.conf in any way.

This adds a large dependency chain thus to skopeo and makes it care about something it never did before, I do not maintain skopeo so I do not have a strong opinion but it is a design choice that likely should be looked at carefully.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, yes. That’s pretty likely to be a big issue.

if err != nil {
return ret, fmt.Errorf("loading containers.conf: %w", err)
}

ec := engineConfig{
Retry: cfg.Engine.Retry,
}
// An empty retry_delay means "use exponential backoff"; only advertise a
// concrete value when one is configured. We normalize to milliseconds so
// clients don't have to parse Go's duration syntax.
if cfg.Engine.RetryDelay != "" {
d, err := time.ParseDuration(cfg.Engine.RetryDelay)
if err != nil {
return ret, fmt.Errorf("parsing containers.conf retry_delay %q: %w", cfg.Engine.RetryDelay, err)
}
// A negative delay is meaningless; treat it as unset (exponential
// backoff) rather than advertising a nonsensical value to clients.
Comment on lines +149 to +150

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • AFAICS the actual behavior of c/common/pkg/retry is to treat this as “no delay” (= wait for $delay immediately complete), not as exponential backoff
  • I’m aesthetically worried about silently replacing invalid data with some other data. It will not be trivial to find out how the value is “lost” if anyone had to dig into it.

if ms := d.Milliseconds(); ms > 0 {
ec.RetryDelayMS = &ms
}
}

return replyBuf{value: ec}, nil
}

// OpenImage accepts a string image reference i.e. TRANSPORT:REF - like `skopeo copy`.
// The return value is an opaque integer handle.
func (h *handler) OpenImage(ctx context.Context, args []any) (replyBuf, error) {
Expand Down Expand Up @@ -706,6 +756,8 @@ func (h *handler) processRequest(ctx context.Context, readBytes []byte) (rb repl
switch req.Method {
case "Initialize":
rb, err = h.Initialize(ctx, req.Args)
case "GetEngineConfig":
rb, err = h.GetEngineConfig(ctx, req.Args)
case "OpenImage":
rb, err = h.OpenImage(ctx, req.Args)
case "OpenImageOptional":
Expand Down
26 changes: 26 additions & 0 deletions common/pkg/json-proxy/proxy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,32 @@ func TestProxyGetBlob(t *testing.T) {
assert.NoError(t, err)
}

func TestProxyGetEngineConfig(t *testing.T) {
p := newProxy(t)

v, err := p.callNoFd("GetEngineConfig", nil)
require.NoError(t, err)

// The reply value is a JSON object; it arrives as a map here.
m, ok := v.(map[string]any)
require.True(t, ok, "GetEngineConfig return value is %T", v)

// retry is always present. We don't assert a specific number since it
// comes from the host containers.conf, but it must be a non-negative
// integer (default 3).
retry, ok := m["retry"].(float64)
require.True(t, ok, "retry is %T", m["retry"])
assert.GreaterOrEqual(t, retry, float64(0))

// retry_delay_ms is optional (omitted when unset => exponential backoff).
// When present it must be a non-negative integer count of milliseconds.
if raw, present := m["retry_delay_ms"]; present {
delay, ok := raw.(float64)
require.True(t, ok, "retry_delay_ms is %T", raw)
assert.GreaterOrEqual(t, delay, float64(0))
}
}

func TestProxyPolicyVerification(t *testing.T) {
for _, tc := range []struct {
name string
Expand Down
23 changes: 22 additions & 1 deletion common/pkg/json-proxy/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,11 @@ import (
// departure from the original code which used HTTP.
//
// When bumping this, please also update the man page.
const protocolVersion = "0.2.8"
//
// 0.2.9: Added the GetEngineConfig method, which exposes the host
// containers.conf engine settings (e.g. retry policy) so clients can apply
// them without re-reading the config themselves.
const protocolVersion = "0.2.9"

// maxMsgSize is the current limit on a packet size.
// Note that all non-metadata (i.e. payload data) is sent over a pipe.
Expand Down Expand Up @@ -71,6 +75,23 @@ type reply struct {
Error string `json:"error"`
}

// engineConfig is the value returned by GetEngineConfig (new in 0.2.9). It
// exposes a curated subset of the host containers.conf [engine] settings that
// are relevant to a client driving image pulls through the proxy.
//
// This is intentionally extensible: fields are additive and optional, so
// clients using serde(default)-style decoding remain forward compatible as new
// settings are surfaced here.
type engineConfig struct {
// Retry is the number of times a failed pull should be retried
// (containers.conf [engine] retry; default 3).
Retry uint `json:"retry"`
// RetryDelayMS is the fixed delay between retries in milliseconds
// (containers.conf [engine] retry_delay). It is omitted when unset, in
// which case the client should use exponential backoff.
RetryDelayMS *int64 `json:"retry_delay_ms,omitempty"`
}

// replyBuf is our internal deserialization of reply plus optional fd.
type replyBuf struct {
// value will be converted to a reply Value.
Expand Down
Loading