Skip to content

Commit cc669bb

Browse files
fadiromdhan3fadiromdhan3
committed
volume create: detect --secret and --topology-* as cluster options
hasClusterVolumeOptionSet decides whether a ClusterVolumeSpec is sent with the request. It looked up a flag named "secrets", but the flag is registered as "secret", so pflag's Changed() always returned false for it. The "topology-required" and "topology-preferred" flags were not checked at all. As a result, running "docker volume create" with only one of those options (for example "--secret key=val" or "--topology-required region=R1") silently created a plain, non-cluster volume and dropped the option without any warning. Also swap the help text of --limit-bytes and --required-bytes, which were reversed: RequiredBytes is the minimum capacity and LimitBytes is the maximum capacity, as documented on volume.CapacityRange. Add a test that sets each cluster-only flag on its own and verifies the resulting ClusterVolumeSpec. Signed-off-by: Fadi Romdhan <fadiromdhan3@gmail.com>
1 parent 7fc2dff commit cc669bb

3 files changed

Lines changed: 151 additions & 6 deletions

File tree

‎cli/command/volume/create.go‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -92,10 +92,10 @@ func newCreateCommand(dockerCLI command.Cli) *cobra.Command {
9292
flags.Var(&options.secrets, "secret", "Cluster Volume secrets")
9393
flags.SetAnnotation("secret", "version", []string{"1.42"})
9494
flags.SetAnnotation("secret", "swarm", []string{"manager"})
95-
flags.Var(&options.limitBytes, "limit-bytes", "Minimum size of the Cluster Volume in bytes")
95+
flags.Var(&options.limitBytes, "limit-bytes", "Maximum size of the Cluster Volume in bytes")
9696
flags.SetAnnotation("limit-bytes", "version", []string{"1.42"})
9797
flags.SetAnnotation("limit-bytes", "swarm", []string{"manager"})
98-
flags.Var(&options.requiredBytes, "required-bytes", "Maximum size of the Cluster Volume in bytes")
98+
flags.Var(&options.requiredBytes, "required-bytes", "Minimum size of the Cluster Volume in bytes")
9999
flags.SetAnnotation("required-bytes", "version", []string{"1.42"})
100100
flags.SetAnnotation("required-bytes", "swarm", []string{"manager"})
101101
flags.Var(&options.requisiteTopology, "topology-required", "A topology that the Cluster Volume must be accessible from")
@@ -113,8 +113,9 @@ func newCreateCommand(dockerCLI command.Cli) *cobra.Command {
113113
func hasClusterVolumeOptionSet(flags *pflag.FlagSet) bool {
114114
return flags.Changed("group") || flags.Changed("scope") ||
115115
flags.Changed("sharing") || flags.Changed("availability") ||
116-
flags.Changed("type") || flags.Changed("secrets") ||
117-
flags.Changed("limit-bytes") || flags.Changed("required-bytes")
116+
flags.Changed("type") || flags.Changed("secret") ||
117+
flags.Changed("limit-bytes") || flags.Changed("required-bytes") ||
118+
flags.Changed("topology-required") || flags.Changed("topology-preferred")
118119
}
119120

120121
func runCreate(ctx context.Context, dockerCli command.Cli, options createOptions) error {

‎cli/command/volume/create_test.go‎

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -262,3 +262,147 @@ func TestVolumeCreateClusterOpts(t *testing.T) {
262262

263263
assert.NilError(t, cmd.Execute())
264264
}
265+
266+
// TestVolumeCreateClusterOptionDetection verifies that each cluster-specific
267+
// flag on its own is enough to send a ClusterVolumeSpec, and that its value
268+
// is propagated.
269+
func TestVolumeCreateClusterOptionDetection(t *testing.T) {
270+
testCases := []struct {
271+
flag string
272+
value string
273+
expected *volume.ClusterVolumeSpec
274+
}{
275+
{
276+
flag: "secret",
277+
value: "key1=secret1",
278+
expected: &volume.ClusterVolumeSpec{
279+
Secrets: []volume.Secret{{Key: "key1", Secret: "secret1"}},
280+
},
281+
},
282+
{
283+
flag: "topology-required",
284+
value: "region=R1,zone=Z1",
285+
expected: &volume.ClusterVolumeSpec{
286+
AccessibilityRequirements: &volume.TopologyRequirement{
287+
Requisite: []volume.Topology{
288+
{Segments: map[string]string{"region": "R1", "zone": "Z1"}},
289+
},
290+
Preferred: []volume.Topology{},
291+
},
292+
},
293+
},
294+
{
295+
flag: "topology-preferred",
296+
value: "region=R1,zone=Z2",
297+
expected: &volume.ClusterVolumeSpec{
298+
AccessibilityRequirements: &volume.TopologyRequirement{
299+
Requisite: []volume.Topology{},
300+
Preferred: []volume.Topology{
301+
{Segments: map[string]string{"region": "R1", "zone": "Z2"}},
302+
},
303+
},
304+
},
305+
},
306+
{
307+
flag: "limit-bytes",
308+
value: "567890",
309+
expected: &volume.ClusterVolumeSpec{
310+
CapacityRange: &volume.CapacityRange{LimitBytes: 567890},
311+
},
312+
},
313+
{
314+
flag: "required-bytes",
315+
value: "1234",
316+
expected: &volume.ClusterVolumeSpec{
317+
CapacityRange: &volume.CapacityRange{RequiredBytes: 1234},
318+
},
319+
},
320+
{
321+
flag: "scope",
322+
value: "multi",
323+
expected: &volume.ClusterVolumeSpec{
324+
AccessMode: &volume.AccessMode{
325+
Scope: volume.ScopeMultiNode,
326+
Sharing: volume.SharingNone,
327+
BlockVolume: &volume.TypeBlock{},
328+
},
329+
},
330+
},
331+
{
332+
flag: "sharing",
333+
value: "all",
334+
expected: &volume.ClusterVolumeSpec{
335+
AccessMode: &volume.AccessMode{
336+
Scope: volume.ScopeSingleNode,
337+
Sharing: volume.SharingAll,
338+
BlockVolume: &volume.TypeBlock{},
339+
},
340+
},
341+
},
342+
{
343+
flag: "type",
344+
value: "mount",
345+
expected: &volume.ClusterVolumeSpec{
346+
AccessMode: &volume.AccessMode{
347+
Scope: volume.ScopeSingleNode,
348+
Sharing: volume.SharingNone,
349+
MountVolume: &volume.TypeMount{},
350+
},
351+
},
352+
},
353+
{
354+
flag: "group",
355+
value: "gronp",
356+
expected: &volume.ClusterVolumeSpec{
357+
Group: "gronp",
358+
},
359+
},
360+
{
361+
flag: "availability",
362+
value: "drain",
363+
expected: &volume.ClusterVolumeSpec{
364+
Availability: volume.AvailabilityDrain,
365+
},
366+
},
367+
}
368+
369+
for _, tc := range testCases {
370+
t.Run(tc.flag, func(t *testing.T) {
371+
var actual *volume.ClusterVolumeSpec
372+
cli := test.NewFakeCli(&fakeClient{
373+
volumeCreateFunc: func(options client.VolumeCreateOptions) (client.VolumeCreateResult, error) {
374+
actual = options.ClusterVolumeSpec
375+
return client.VolumeCreateResult{}, nil
376+
},
377+
})
378+
379+
cmd := newCreateCommand(cli)
380+
cmd.SetOut(io.Discard)
381+
cmd.SetErr(io.Discard)
382+
cmd.SetArgs([]string{"my-csi-volume"})
383+
assert.Check(t, cmd.Flags().Set("driver", "csi"))
384+
assert.Check(t, cmd.Flags().Set(tc.flag, tc.value))
385+
assert.NilError(t, cmd.Execute())
386+
387+
assert.Assert(t, actual != nil, "expected a ClusterVolumeSpec when only --%s is set", tc.flag)
388+
if tc.expected.Secrets != nil {
389+
assert.Check(t, is.DeepEqual(actual.Secrets, tc.expected.Secrets))
390+
}
391+
if tc.expected.AccessMode != nil {
392+
assert.Check(t, is.DeepEqual(actual.AccessMode, tc.expected.AccessMode))
393+
}
394+
if tc.expected.AccessibilityRequirements != nil {
395+
assert.Check(t, is.DeepEqual(actual.AccessibilityRequirements, tc.expected.AccessibilityRequirements))
396+
}
397+
if tc.expected.CapacityRange != nil {
398+
assert.Check(t, is.DeepEqual(actual.CapacityRange, tc.expected.CapacityRange))
399+
}
400+
if tc.expected.Group != "" {
401+
assert.Check(t, is.Equal(actual.Group, tc.expected.Group))
402+
}
403+
if tc.expected.Availability != "" {
404+
assert.Check(t, is.Equal(actual.Availability, tc.expected.Availability))
405+
}
406+
})
407+
}
408+
}

‎docs/reference/commandline/volume_create.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,9 @@ Create a volume
1111
| `-d`, `--driver` | `string` | `local` | Specify volume driver name |
1212
| `--group` | `string` | | Cluster Volume group (cluster volumes) |
1313
| `--label` | `list` | | Set metadata for a volume |
14-
| `--limit-bytes` | `bytes` | `0` | Minimum size of the Cluster Volume in bytes |
14+
| `--limit-bytes` | `bytes` | `0` | Maximum size of the Cluster Volume in bytes |
1515
| [`-o`](#opt), [`--opt`](#opt) | `map` | `map[]` | Set driver specific options |
16-
| `--required-bytes` | `bytes` | `0` | Maximum size of the Cluster Volume in bytes |
16+
| `--required-bytes` | `bytes` | `0` | Minimum size of the Cluster Volume in bytes |
1717
| `--scope` | `string` | `single` | Cluster Volume access scope (`single`, `multi`) |
1818
| `--secret` | `map` | `map[]` | Cluster Volume secrets |
1919
| `--sharing` | `string` | `none` | Cluster Volume access sharing (`none`, `readonly`, `onewriter`, `all`) |

0 commit comments

Comments
 (0)