From e4a49e1094a5253194b3b56c63a78c92b5e70b5c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 28 Aug 2023 12:09:06 +0200 Subject: [PATCH] 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 --- cli/command/container/opts_test.go | 17 +++--- cli/compose/convert/service.go | 80 ++++++++++++++++++----------- cli/compose/convert/service_test.go | 70 +++++++++++++++---------- opts/parse.go | 32 +++++++----- 4 files changed, 124 insertions(+), 75 deletions(-) diff --git a/cli/command/container/opts_test.go b/cli/command/container/opts_test.go index 51a0e72b4943..76ee182377a2 100644 --- a/cli/command/container/opts_test.go +++ b/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) { diff --git a/cli/compose/convert/service.go b/cli/compose/convert/service.go index b67d6ae61029..cc04bc357a42 100644 --- a/cli/compose/convert/service.go +++ b/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 { diff --git a/cli/compose/convert/service_test.go b/cli/compose/convert/service_test.go index b5999fe6ea8e..8c8e8838c251 100644 --- a/cli/compose/convert/service_test.go +++ b/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 { diff --git a/opts/parse.go b/opts/parse.go index c04fc7d4b8bb..cb817760e743 100644 --- a/opts/parse.go +++ b/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 }