From 4c2d7b7f7036ae47bbfbb2210e99a7888ec2dd41 Mon Sep 17 00:00:00 2001 From: Nick Adcock Date: Tue, 7 Jan 2020 11:28:28 +0000 Subject: [PATCH 1/3] Detect single value advanced config/secret syntax Allow the use of the advanced source=x syntax for config and secret values when there is no comma Before this change the following would fail with config not found: docker service create --name hello1 --config source=myconfig nginx:alpine And the following would fail with secret not found: docker service create --name hello2 --secret source=mysecret nginx:alpine Signed-off-by: Nick Adcock --- opts/config.go | 2 +- opts/config_test.go | 92 +++++++++++++++++++++++++++++++++++++++++++++ opts/secret.go | 2 +- opts/secret_test.go | 12 ++++++ 4 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 opts/config_test.go diff --git a/opts/config.go b/opts/config.go index 82fd2bce4e..3e56d5de61 100644 --- a/opts/config.go +++ b/opts/config.go @@ -32,7 +32,7 @@ func (o *ConfigOpt) Set(value string) error { } // support a simple syntax of --config foo - if len(fields) == 1 { + if len(fields) == 1 && !strings.Contains(fields[0], "=") { options.File.Name = fields[0] options.ConfigName = fields[0] o.values = append(o.values, options) diff --git a/opts/config_test.go b/opts/config_test.go new file mode 100644 index 0000000000..4e9b707267 --- /dev/null +++ b/opts/config_test.go @@ -0,0 +1,92 @@ +package opts + +import ( + "os" + "testing" + + "gotest.tools/assert" + is "gotest.tools/assert/cmp" +) + +func TestConfigOptionsSimple(t *testing.T) { + var opt ConfigOpt + + testCase := "app-config" + assert.NilError(t, opt.Set(testCase)) + + reqs := opt.Value() + assert.Assert(t, is.Len(reqs, 1)) + req := reqs[0] + assert.Check(t, is.Equal("app-config", req.ConfigName)) + assert.Check(t, is.Equal("app-config", req.File.Name)) + assert.Check(t, is.Equal("0", req.File.UID)) + assert.Check(t, is.Equal("0", req.File.GID)) +} + +func TestConfigOptionsSource(t *testing.T) { + var opt ConfigOpt + + testCase := "source=foo" + assert.NilError(t, opt.Set(testCase)) + + reqs := opt.Value() + assert.Assert(t, is.Len(reqs, 1)) + req := reqs[0] + assert.Check(t, is.Equal("foo", req.ConfigName)) +} + +func TestConfigOptionsSourceTarget(t *testing.T) { + var opt ConfigOpt + + testCase := "source=foo,target=testing" + assert.NilError(t, opt.Set(testCase)) + + reqs := opt.Value() + assert.Assert(t, is.Len(reqs, 1)) + req := reqs[0] + assert.Check(t, is.Equal("foo", req.ConfigName)) + assert.Check(t, is.Equal("testing", req.File.Name)) +} + +func TestConfigOptionsShorthand(t *testing.T) { + var opt ConfigOpt + + testCase := "src=foo,target=testing" + assert.NilError(t, opt.Set(testCase)) + + reqs := opt.Value() + assert.Assert(t, is.Len(reqs, 1)) + req := reqs[0] + assert.Check(t, is.Equal("foo", req.ConfigName)) +} + +func TestConfigOptionsCustomUidGid(t *testing.T) { + var opt ConfigOpt + + testCase := "source=foo,target=testing,uid=1000,gid=1001" + assert.NilError(t, opt.Set(testCase)) + + reqs := opt.Value() + assert.Assert(t, is.Len(reqs, 1)) + req := reqs[0] + assert.Check(t, is.Equal("foo", req.ConfigName)) + assert.Check(t, is.Equal("testing", req.File.Name)) + assert.Check(t, is.Equal("1000", req.File.UID)) + assert.Check(t, is.Equal("1001", req.File.GID)) +} + +func TestConfigOptionsCustomMode(t *testing.T) { + var opt ConfigOpt + + testCase := "source=foo,target=testing,uid=1000,gid=1001,mode=0444" + assert.NilError(t, opt.Set(testCase)) + + reqs := opt.Value() + assert.Assert(t, is.Len(reqs, 1)) + req := reqs[0] + assert.Check(t, is.Equal("foo", req.ConfigName)) + assert.Check(t, is.Equal("testing", req.File.Name)) + assert.Check(t, is.Equal("1000", req.File.UID)) + assert.Check(t, is.Equal("1001", req.File.GID)) + assert.Check(t, is.Equal(os.FileMode(0444), req.File.Mode)) +} diff --git a/opts/secret.go b/opts/secret.go index a1fde54d91..6a66dbdfd5 100644 --- a/opts/secret.go +++ b/opts/secret.go @@ -32,7 +32,7 @@ func (o *SecretOpt) Set(value string) error { } // support a simple syntax of --secret foo - if len(fields) == 1 { + if len(fields) == 1 && !strings.Contains(fields[0], "=") { options.File.Name = fields[0] options.SecretName = fields[0] o.values = append(o.values, options) diff --git a/opts/secret_test.go b/opts/secret_test.go index 94dd80d19a..539f0167f2 100644 --- a/opts/secret_test.go +++ b/opts/secret_test.go @@ -23,6 +23,18 @@ func TestSecretOptionsSimple(t *testing.T) { assert.Check(t, is.Equal("0", req.File.GID)) } +func TestSecretOptionsSource(t *testing.T) { + var opt SecretOpt + + testCase := "source=foo" + assert.NilError(t, opt.Set(testCase)) + + reqs := opt.Value() + assert.Assert(t, is.Len(reqs, 1)) + req := reqs[0] + assert.Check(t, is.Equal("foo", req.SecretName)) +} + func TestSecretOptionsSourceTarget(t *testing.T) { var opt SecretOpt From 9698b7a37466473b9166656c05943a809829854b Mon Sep 17 00:00:00 2001 From: Nick Adcock Date: Tue, 7 Jan 2020 13:11:53 +0000 Subject: [PATCH 2/3] Default config/secret target to source name When using advanced syntax for setting config and secret values, default the target value to the source value when the user does not specify a target. Signed-off-by: Nick Adcock --- opts/config.go | 3 +++ opts/config_test.go | 1 + opts/secret.go | 3 +++ opts/secret_test.go | 1 + 4 files changed, 8 insertions(+) diff --git a/opts/config.go b/opts/config.go index 3e56d5de61..b110439056 100644 --- a/opts/config.go +++ b/opts/config.go @@ -72,6 +72,9 @@ func (o *ConfigOpt) Set(value string) error { if options.ConfigName == "" { return fmt.Errorf("source is required") } + if options.File.Name == "" { + options.File.Name = options.ConfigName + } o.values = append(o.values, options) return nil diff --git a/opts/config_test.go b/opts/config_test.go index 4e9b707267..7ddf7ce3f2 100644 --- a/opts/config_test.go +++ b/opts/config_test.go @@ -33,6 +33,7 @@ func TestConfigOptionsSource(t *testing.T) { assert.Assert(t, is.Len(reqs, 1)) req := reqs[0] assert.Check(t, is.Equal("foo", req.ConfigName)) + assert.Check(t, is.Equal("foo", req.File.Name)) } func TestConfigOptionsSourceTarget(t *testing.T) { diff --git a/opts/secret.go b/opts/secret.go index 6a66dbdfd5..c527ad3be6 100644 --- a/opts/secret.go +++ b/opts/secret.go @@ -72,6 +72,9 @@ func (o *SecretOpt) Set(value string) error { if options.SecretName == "" { return fmt.Errorf("source is required") } + if options.File.Name == "" { + options.File.Name = options.SecretName + } o.values = append(o.values, options) return nil diff --git a/opts/secret_test.go b/opts/secret_test.go index 539f0167f2..610157a076 100644 --- a/opts/secret_test.go +++ b/opts/secret_test.go @@ -33,6 +33,7 @@ func TestSecretOptionsSource(t *testing.T) { assert.Assert(t, is.Len(reqs, 1)) req := reqs[0] assert.Check(t, is.Equal("foo", req.SecretName)) + assert.Check(t, is.Equal("foo", req.File.Name)) } func TestSecretOptionsSourceTarget(t *testing.T) { From 3baa6d57fabb387492fee01a4cf3e94894584b82 Mon Sep 17 00:00:00 2001 From: Nick Adcock Date: Thu, 9 Jan 2020 10:15:05 +0000 Subject: [PATCH 3/3] Refactor config and secret tests to table-driven Refactors the config and secret unit tests to be table driven to remove duplication Signed-off-by: Nick Adcock --- opts/config_test.go | 156 +++++++++++++++++++++----------------------- opts/secret_test.go | 156 +++++++++++++++++++++----------------------- 2 files changed, 150 insertions(+), 162 deletions(-) diff --git a/opts/config_test.go b/opts/config_test.go index 7ddf7ce3f2..e0dc7feade 100644 --- a/opts/config_test.go +++ b/opts/config_test.go @@ -8,86 +8,80 @@ import ( is "gotest.tools/assert/cmp" ) -func TestConfigOptionsSimple(t *testing.T) { - var opt ConfigOpt +func TestConfigOptions(t *testing.T) { + testCases := []struct { + name string + input string + configName string + fileName string + uid string + gid string + fileMode uint + }{ + { + name: "Simple", + input: "app-config", + configName: "app-config", + fileName: "app-config", + uid: "0", + gid: "0", + }, + { + name: "Source", + input: "source=foo", + configName: "foo", + fileName: "foo", + }, + { + name: "SourceTarget", + input: "source=foo,target=testing", + configName: "foo", + fileName: "testing", + }, + { + name: "Shorthand", + input: "src=foo,target=testing", + configName: "foo", + fileName: "testing", + }, + { + name: "CustomUidGid", + input: "source=foo,target=testing,uid=1000,gid=1001", + configName: "foo", + fileName: "testing", + uid: "1000", + gid: "1001", + }, + { + name: "CustomMode", + input: "source=foo,target=testing,uid=1000,gid=1001,mode=0444", + configName: "foo", + fileName: "testing", + uid: "1000", + gid: "1001", + fileMode: 0444, + }, + } - testCase := "app-config" - assert.NilError(t, opt.Set(testCase)) - - reqs := opt.Value() - assert.Assert(t, is.Len(reqs, 1)) - req := reqs[0] - assert.Check(t, is.Equal("app-config", req.ConfigName)) - assert.Check(t, is.Equal("app-config", req.File.Name)) - assert.Check(t, is.Equal("0", req.File.UID)) - assert.Check(t, is.Equal("0", req.File.GID)) -} - -func TestConfigOptionsSource(t *testing.T) { - var opt ConfigOpt - - testCase := "source=foo" - assert.NilError(t, opt.Set(testCase)) - - reqs := opt.Value() - assert.Assert(t, is.Len(reqs, 1)) - req := reqs[0] - assert.Check(t, is.Equal("foo", req.ConfigName)) - assert.Check(t, is.Equal("foo", req.File.Name)) -} - -func TestConfigOptionsSourceTarget(t *testing.T) { - var opt ConfigOpt - - testCase := "source=foo,target=testing" - assert.NilError(t, opt.Set(testCase)) - - reqs := opt.Value() - assert.Assert(t, is.Len(reqs, 1)) - req := reqs[0] - assert.Check(t, is.Equal("foo", req.ConfigName)) - assert.Check(t, is.Equal("testing", req.File.Name)) -} - -func TestConfigOptionsShorthand(t *testing.T) { - var opt ConfigOpt - - testCase := "src=foo,target=testing" - assert.NilError(t, opt.Set(testCase)) - - reqs := opt.Value() - assert.Assert(t, is.Len(reqs, 1)) - req := reqs[0] - assert.Check(t, is.Equal("foo", req.ConfigName)) -} - -func TestConfigOptionsCustomUidGid(t *testing.T) { - var opt ConfigOpt - - testCase := "source=foo,target=testing,uid=1000,gid=1001" - assert.NilError(t, opt.Set(testCase)) - - reqs := opt.Value() - assert.Assert(t, is.Len(reqs, 1)) - req := reqs[0] - assert.Check(t, is.Equal("foo", req.ConfigName)) - assert.Check(t, is.Equal("testing", req.File.Name)) - assert.Check(t, is.Equal("1000", req.File.UID)) - assert.Check(t, is.Equal("1001", req.File.GID)) -} - -func TestConfigOptionsCustomMode(t *testing.T) { - var opt ConfigOpt - - testCase := "source=foo,target=testing,uid=1000,gid=1001,mode=0444" - assert.NilError(t, opt.Set(testCase)) - - reqs := opt.Value() - assert.Assert(t, is.Len(reqs, 1)) - req := reqs[0] - assert.Check(t, is.Equal("foo", req.ConfigName)) - assert.Check(t, is.Equal("testing", req.File.Name)) - assert.Check(t, is.Equal("1000", req.File.UID)) - assert.Check(t, is.Equal("1001", req.File.GID)) - assert.Check(t, is.Equal(os.FileMode(0444), req.File.Mode)) + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + var opt ConfigOpt + assert.NilError(t, opt.Set(tc.input)) + reqs := opt.Value() + assert.Assert(t, is.Len(reqs, 1)) + req := reqs[0] + assert.Check(t, is.Equal(tc.configName, req.ConfigName)) + assert.Check(t, is.Equal(tc.fileName, req.File.Name)) + if tc.uid != "" { + assert.Check(t, is.Equal(tc.uid, req.File.UID)) + } + if tc.gid != "" { + assert.Check(t, is.Equal(tc.gid, req.File.GID)) + } + if tc.fileMode != 0 { + assert.Check(t, is.Equal(os.FileMode(tc.fileMode), req.File.Mode)) + } + }) + } } diff --git a/opts/secret_test.go b/opts/secret_test.go index 610157a076..aeab48b8c4 100644 --- a/opts/secret_test.go +++ b/opts/secret_test.go @@ -8,86 +8,80 @@ import ( is "gotest.tools/assert/cmp" ) -func TestSecretOptionsSimple(t *testing.T) { - var opt SecretOpt +func TestSecretOptions(t *testing.T) { + testCases := []struct { + name string + input string + secretName string + fileName string + uid string + gid string + fileMode uint + }{ + { + name: "Simple", + input: "app-secret", + secretName: "app-secret", + fileName: "app-secret", + uid: "0", + gid: "0", + }, + { + name: "Source", + input: "source=foo", + secretName: "foo", + fileName: "foo", + }, + { + name: "SourceTarget", + input: "source=foo,target=testing", + secretName: "foo", + fileName: "testing", + }, + { + name: "Shorthand", + input: "src=foo,target=testing", + secretName: "foo", + fileName: "testing", + }, + { + name: "CustomUidGid", + input: "source=foo,target=testing,uid=1000,gid=1001", + secretName: "foo", + fileName: "testing", + uid: "1000", + gid: "1001", + }, + { + name: "CustomMode", + input: "source=foo,target=testing,uid=1000,gid=1001,mode=0444", + secretName: "foo", + fileName: "testing", + uid: "1000", + gid: "1001", + fileMode: 0444, + }, + } - testCase := "app-secret" - assert.NilError(t, opt.Set(testCase)) - - reqs := opt.Value() - assert.Assert(t, is.Len(reqs, 1)) - req := reqs[0] - assert.Check(t, is.Equal("app-secret", req.SecretName)) - assert.Check(t, is.Equal("app-secret", req.File.Name)) - assert.Check(t, is.Equal("0", req.File.UID)) - assert.Check(t, is.Equal("0", req.File.GID)) -} - -func TestSecretOptionsSource(t *testing.T) { - var opt SecretOpt - - testCase := "source=foo" - assert.NilError(t, opt.Set(testCase)) - - reqs := opt.Value() - assert.Assert(t, is.Len(reqs, 1)) - req := reqs[0] - assert.Check(t, is.Equal("foo", req.SecretName)) - assert.Check(t, is.Equal("foo", req.File.Name)) -} - -func TestSecretOptionsSourceTarget(t *testing.T) { - var opt SecretOpt - - testCase := "source=foo,target=testing" - assert.NilError(t, opt.Set(testCase)) - - reqs := opt.Value() - assert.Assert(t, is.Len(reqs, 1)) - req := reqs[0] - assert.Check(t, is.Equal("foo", req.SecretName)) - assert.Check(t, is.Equal("testing", req.File.Name)) -} - -func TestSecretOptionsShorthand(t *testing.T) { - var opt SecretOpt - - testCase := "src=foo,target=testing" - assert.NilError(t, opt.Set(testCase)) - - reqs := opt.Value() - assert.Assert(t, is.Len(reqs, 1)) - req := reqs[0] - assert.Check(t, is.Equal("foo", req.SecretName)) -} - -func TestSecretOptionsCustomUidGid(t *testing.T) { - var opt SecretOpt - - testCase := "source=foo,target=testing,uid=1000,gid=1001" - assert.NilError(t, opt.Set(testCase)) - - reqs := opt.Value() - assert.Assert(t, is.Len(reqs, 1)) - req := reqs[0] - assert.Check(t, is.Equal("foo", req.SecretName)) - assert.Check(t, is.Equal("testing", req.File.Name)) - assert.Check(t, is.Equal("1000", req.File.UID)) - assert.Check(t, is.Equal("1001", req.File.GID)) -} - -func TestSecretOptionsCustomMode(t *testing.T) { - var opt SecretOpt - - testCase := "source=foo,target=testing,uid=1000,gid=1001,mode=0444" - assert.NilError(t, opt.Set(testCase)) - - reqs := opt.Value() - assert.Assert(t, is.Len(reqs, 1)) - req := reqs[0] - assert.Check(t, is.Equal("foo", req.SecretName)) - assert.Check(t, is.Equal("testing", req.File.Name)) - assert.Check(t, is.Equal("1000", req.File.UID)) - assert.Check(t, is.Equal("1001", req.File.GID)) - assert.Check(t, is.Equal(os.FileMode(0444), req.File.Mode)) + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + var opt SecretOpt + assert.NilError(t, opt.Set(tc.input)) + reqs := opt.Value() + assert.Assert(t, is.Len(reqs, 1)) + req := reqs[0] + assert.Check(t, is.Equal(tc.secretName, req.SecretName)) + assert.Check(t, is.Equal(tc.fileName, req.File.Name)) + if tc.uid != "" { + assert.Check(t, is.Equal(tc.uid, req.File.UID)) + } + if tc.gid != "" { + assert.Check(t, is.Equal(tc.gid, req.File.GID)) + } + if tc.fileMode != 0 { + assert.Check(t, is.Equal(os.FileMode(tc.fileMode), req.File.Mode)) + } + }) + } }