refactor parsing restart-policies

- opts.ParseRestartPolicy: use a struct-literal and clarify that this
  function only parses, but does not validate invalid combinations.
- cli/compose/convert: convertRestartPolicy: update doc to be more
  clear on the order of preference and intent.
- cli/compose/convert: convertRestartPolicy: use a switch based on
  known values to allow the exhaustive linter to catch missing options.
- cli/compose/convert: convertRestartPolicy: add validation for negative
  values when converting the legacy service.restart policy.
- cli/compose/convert: convertRestartPolicy: always set MaxAttempts
  and leave validation to the daemon.

opts: ParseRestartPolicy: improve validation of max restart-counts

Use the new container.ValidateRestartPolicy utility to verify if a max-restart-count
is allowed for the given restart-policy.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Author: thaJeztah

cli/command/container/opts_test.go

@@ -829,13 +829,6 @@ func TestParseRestartPolicy(t *testing.T) {
 				Name: container.RestartPolicyAlways,
 			},
 		},
-		{
-			input: "always:1",
-			expected: container.RestartPolicy{
-				Name:              container.RestartPolicyAlways,
-				MaximumRetryCount: 1,
-			},
-		},
 		{
 			input:       "always:2:3",
 			expectedErr: "invalid restart policy format: maximum retry count must be an integer",
@@ -861,6 +854,16 @@ func TestParseRestartPolicy(t *testing.T) {
 			input:       "unless-stopped:invalid",
 			expectedErr: "invalid restart policy format: maximum retry count must be an integer",
 		},
+
+		// Unknown / invalid combinations: validation is handled by the daemon>
+		{
+			input:    "anything:123",
+			expected: container.RestartPolicy{Name: "anything", MaximumRetryCount: 123},
+		},
+		{
+			input:    "negative:-123",
+			expected: container.RestartPolicy{Name: "negative", MaximumRetryCount: -123},
+		},
 	}
 	for _, tc := range tests {
 		t.Run(tc.input, func(t *testing.T) {

cli/compose/convert/service.go

@@ -85,8 +85,7 @@ func Service(
 		return swarm.ServiceSpec{}, err
 	}
 
-	restartPolicy, err := convertRestartPolicy(
-		service.Restart, service.Deploy.RestartPolicy)
+	restartPolicy, err := convertRestartPolicy(service.Restart, service.Deploy.RestartPolicy)
 	if err != nil {
 		return swarm.ServiceSpec{}, err
 	}
@@ -472,37 +471,58 @@ func convertHealthcheck(healthcheck *composetypes.HealthCheckConfig) (*container
 	}, nil
 }
 
-func convertRestartPolicy(restart string, source *composetypes.RestartPolicy) (*swarm.RestartPolicy, error) {
-	// TODO: log if restart is being ignored
-	if source == nil {
-		policy, err := opts.ParseRestartPolicy(restart)
-		if err != nil {
-			return nil, err
-		}
-		switch {
-		case policy.IsNone():
-			return nil, nil
-		case policy.IsAlways(), policy.IsUnlessStopped():
-			return &swarm.RestartPolicy{
-				Condition: swarm.RestartPolicyConditionAny,
-			}, nil
-		case policy.IsOnFailure():
-			attempts := uint64(policy.MaximumRetryCount)
-			return &swarm.RestartPolicy{
-				Condition:   swarm.RestartPolicyConditionOnFailure,
-				MaxAttempts: &attempts,
-			}, nil
-		default:
-			return nil, fmt.Errorf("unknown restart policy: %s", restart)
+// convertRestartPolicy converts the service's restart-policy. It prefers
+// service.deploy.restart_policy, but falls back to parsing the legacy
+// service.restart if service.deploy.restart_policy is not set.
+func convertRestartPolicy(restart string, restartPolicy *composetypes.RestartPolicy) (*swarm.RestartPolicy, error) {
+	// Use service.deploy.restart_policy, if set.
+	if restartPolicy != nil {
+		// TODO: log or error if both "service.restart" and "service.deploy.restartpolicy" are set.
+		return &swarm.RestartPolicy{
+			Condition:   swarm.RestartPolicyCondition(restartPolicy.Condition),
+			Delay:       composetypes.ConvertDurationPtr(restartPolicy.Delay),
+			MaxAttempts: restartPolicy.MaxAttempts,
+			Window:      composetypes.ConvertDurationPtr(restartPolicy.Window),
+		}, nil
+	}
+	if restart == "" {
+		return nil, nil
+	}
+
+	// Fall back to the legacy service.restart restart-policy.
+	policy, err := opts.ParseRestartPolicy(restart)
+	if err != nil {
+		return nil, err
+	}
+	if policy.MaximumRetryCount < 0 {
+		return nil, errors.New("invalid restart policy: maximum retry count cannot be negative")
+	}
+	uint64Ptr := func(i int) *uint64 {
+		if i <= 0 {
+			return nil
 		}
+		p := uint64(i)
+		return &p
 	}
 
-	return &swarm.RestartPolicy{
-		Condition:   swarm.RestartPolicyCondition(source.Condition),
-		Delay:       composetypes.ConvertDurationPtr(source.Delay),
-		MaxAttempts: source.MaxAttempts,
-		Window:      composetypes.ConvertDurationPtr(source.Window),
-	}, nil
+	switch policy.Name {
+	case container.RestartPolicyDisabled, "":
+		return nil, nil
+	case container.RestartPolicyAlways, container.RestartPolicyUnlessStopped:
+		return &swarm.RestartPolicy{
+			Condition:   swarm.RestartPolicyConditionAny,
+			MaxAttempts: uint64Ptr(policy.MaximumRetryCount),
+		}, nil
+	case container.RestartPolicyOnFailure:
+		return &swarm.RestartPolicy{
+			Condition:   swarm.RestartPolicyConditionOnFailure,
+			MaxAttempts: uint64Ptr(policy.MaximumRetryCount),
+		}, nil
+	default:
+		return nil, fmt.Errorf("invalid restart policy: unknown policy '%s' (must be one of '%s', '%s', '%s', or '%s')",
+			policy.Name, container.RestartPolicyDisabled, container.RestartPolicyAlways, container.RestartPolicyOnFailure, container.RestartPolicyUnlessStopped,
+		)
+	}
 }
 
 func convertUpdateConfig(source *composetypes.UpdateConfig) *swarm.UpdateConfig {

cli/compose/convert/service_test.go

@@ -10,6 +10,7 @@ import (
 	"time"
 
 	composetypes "github.com/docker/cli/cli/compose/types"
+	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/moby/moby/api/types/container"
 	"github.com/moby/moby/api/types/swarm"
@@ -18,35 +19,52 @@ import (
 	is "gotest.tools/v3/assert/cmp"
 )
 
-func TestConvertRestartPolicyFromNone(t *testing.T) {
-	policy, err := convertRestartPolicy("no", nil)
-	assert.NilError(t, err)
-	assert.Check(t, is.DeepEqual((*swarm.RestartPolicy)(nil), policy))
-}
-
-func TestConvertRestartPolicyFromUnknown(t *testing.T) {
-	_, err := convertRestartPolicy("unknown", nil)
-	assert.Error(t, err, "unknown restart policy: unknown")
-}
-
-func TestConvertRestartPolicyFromAlways(t *testing.T) {
-	policy, err := convertRestartPolicy("always", nil)
-	expected := &swarm.RestartPolicy{
-		Condition: swarm.RestartPolicyConditionAny,
+func TestConvertRestartPolicy(t *testing.T) {
+	attempts := uint64(4)
+	tests := []struct {
+		input    string
+		expected *swarm.RestartPolicy
+		expError string
+	}{
+		{},
+		{
+			input: "no",
+		},
+		{
+			input:    "unknown",
+			expError: "invalid restart policy: unknown policy 'unknown'",
+		},
+		{
+			input: "always",
+			expected: &swarm.RestartPolicy{
+				Condition: swarm.RestartPolicyConditionAny,
+			},
+		},
+		{
+			input: "on-failure:4",
+			expected: &swarm.RestartPolicy{
+				Condition:   swarm.RestartPolicyConditionOnFailure,
+				MaxAttempts: &attempts,
+			},
+		},
 	}
-	assert.NilError(t, err)
-	assert.Check(t, is.DeepEqual(expected, policy))
-}
 
-func TestConvertRestartPolicyFromFailure(t *testing.T) {
-	policy, err := convertRestartPolicy("on-failure:4", nil)
-	attempts := uint64(4)
-	expected := &swarm.RestartPolicy{
-		Condition:   swarm.RestartPolicyConditionOnFailure,
-		MaxAttempts: &attempts,
+	for _, tc := range tests {
+		name := tc.input
+		if name == "" {
+			name = "empty"
+		}
+		t.Run(name, func(t *testing.T) {
+			policy, err := convertRestartPolicy(tc.input, nil)
+			if tc.expError != "" {
+				assert.Check(t, is.ErrorContains(err, tc.expError))
+				assert.Check(t, is.Nil(policy))
+				return
+			}
+			assert.NilError(t, err)
+			assert.Check(t, cmp.Equal(policy, tc.expected))
+		})
 	}
-	assert.NilError(t, err)
-	assert.Check(t, is.DeepEqual(expected, policy))
 }
 
 func strPtr(val string) *string {

opts/parse.go

@@ -70,28 +70,36 @@ func ConvertKVStringsToMapWithNil(values []string) map[string]*string {
 	return result
 }
 
-// ParseRestartPolicy returns the parsed policy or an error indicating what is incorrect
+// ParseRestartPolicy parses a restart policy string ("name[:max-retries]")
+// into a [container.RestartPolicy].
+//
+// Parsing is syntactic only; semantic validation is deferred to the daemon/API.
+// An empty input returns a zero-value policy for backward compatibility. The
+// retry count, if set, must be an integer (negative values are allowed here
+// but may be rejected by the daemon).
 func ParseRestartPolicy(policy string) (container.RestartPolicy, error) {
 	if policy == "" {
-		// for backward-compatibility, we don't set the default ("no")
-		// policy here, because older versions of the engine may not
-		// support it.
+		// For backward compatibility, do not set an explicit default ("no"),
+		// as older daemons may not support it.
 		return container.RestartPolicy{}, nil
 	}
 
-	p := container.RestartPolicy{}
-	k, v, ok := strings.Cut(policy, ":")
-	if ok && k == "" {
+	name, count, ok := strings.Cut(policy, ":")
+	if ok && name == "" {
 		return container.RestartPolicy{}, errors.New("invalid restart policy format: no policy provided before colon")
 	}
-	if v != "" {
-		count, err := strconv.Atoi(v)
+
+	var retryCount int
+	if count != "" {
+		c, err := strconv.Atoi(count)
 		if err != nil {
 			return container.RestartPolicy{}, errors.New("invalid restart policy format: maximum retry count must be an integer")
 		}
-		p.MaximumRetryCount = count
+		retryCount = c
 	}
 
-	p.Name = container.RestartPolicyMode(k)
-	return p, nil
+	return container.RestartPolicy{
+		Name:              container.RestartPolicyMode(name),
+		MaximumRetryCount: retryCount,
+	}, nil
 }