From 6511da877f00e8b4f583ca5f1473a8b59cb37523 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 2 Feb 2019 16:35:26 +0100 Subject: [PATCH 1/4] Add support for using Configs as CredentialSpecs in services Signed-off-by: Sebastiaan van Stijn --- cli/command/service/opts.go | 4 +- cli/command/service/opts_test.go | 55 +++++++++++++ cli/compose/convert/service.go | 23 +++++- cli/compose/convert/service_test.go | 79 +++++++++++++------ cli/compose/loader/loader_test.go | 14 ++++ cli/compose/schema/bindata.go | 72 ++++++++--------- .../schema/data/config_schema_v3.8.json | 1 + cli/compose/schema/schema_test.go | 4 +- cli/compose/types/types.go | 3 +- 9 files changed, 188 insertions(+), 67 deletions(-) diff --git a/cli/command/service/opts.go b/cli/command/service/opts.go index c51d37c1de8f..ce9f39c8420a 100644 --- a/cli/command/service/opts.go +++ b/cli/command/service/opts.go @@ -331,12 +331,14 @@ func (c *credentialSpecOpt) Set(value string) error { c.source = value c.value = &swarm.CredentialSpec{} switch { + case strings.HasPrefix(value, "config://"): + c.value.Config = strings.TrimPrefix(value, "config://") case strings.HasPrefix(value, "file://"): c.value.File = strings.TrimPrefix(value, "file://") case strings.HasPrefix(value, "registry://"): c.value.Registry = strings.TrimPrefix(value, "registry://") default: - return errors.New("Invalid credential spec - value must be prefixed file:// or registry:// followed by a value") + return errors.New(`invalid credential spec: value must be prefixed with "config://", "file://", or "registry://"`) } return nil diff --git a/cli/command/service/opts_test.go b/cli/command/service/opts_test.go index 9493a8230349..9f26daa3bf33 100644 --- a/cli/command/service/opts_test.go +++ b/cli/command/service/opts_test.go @@ -14,6 +14,61 @@ import ( is "gotest.tools/assert/cmp" ) +func TestCredentialSpecOpt(t *testing.T) { + tests := []struct { + name string + in string + value swarm.CredentialSpec + expectedErr string + }{ + { + name: "empty", + in: "", + value: swarm.CredentialSpec{}, + expectedErr: `invalid credential spec: value must be prefixed with "config://", "file://", or "registry://"`, + }, + { + name: "no-prefix", + in: "noprefix", + value: swarm.CredentialSpec{}, + expectedErr: `invalid credential spec: value must be prefixed with "config://", "file://", or "registry://"`, + }, + { + name: "config", + in: "config://0bt9dmxjvjiqermk6xrop3ekq", + value: swarm.CredentialSpec{Config: "0bt9dmxjvjiqermk6xrop3ekq"}, + }, + { + name: "file", + in: "file://somefile.json", + value: swarm.CredentialSpec{File: "somefile.json"}, + }, + { + name: "registry", + in: "registry://testing", + value: swarm.CredentialSpec{Registry: "testing"}, + }, + } + + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + var cs credentialSpecOpt + + err := cs.Set(tc.in) + + if tc.expectedErr != "" { + assert.Error(t, err, tc.expectedErr) + } else { + assert.NilError(t, err) + } + + assert.Equal(t, cs.String(), tc.in) + assert.DeepEqual(t, cs.Value(), &tc.value) + }) + } +} + func TestMemBytesString(t *testing.T) { var mem opts.MemBytes = 1048576 assert.Check(t, is.Equal("1MiB", mem.String())) diff --git a/cli/compose/convert/service.go b/cli/compose/convert/service.go index 21f4850c8792..0fe40bf3a7bc 100644 --- a/cli/compose/convert/service.go +++ b/cli/compose/convert/service.go @@ -600,11 +600,26 @@ func convertDNSConfig(DNS []string, DNSSearch []string) (*swarm.DNSConfig, error } func convertCredentialSpec(spec composetypes.CredentialSpecConfig) (*swarm.CredentialSpec, error) { - if spec.File == "" && spec.Registry == "" { - return nil, nil + var o []string + + // Config was added in API v1.40 + if spec.Config != "" { + o = append(o, `"Config"`) + } + if spec.File != "" { + o = append(o, `"File"`) } - if spec.File != "" && spec.Registry != "" { - return nil, errors.New("Invalid credential spec - must provide one of `File` or `Registry`") + if spec.Registry != "" { + o = append(o, `"Registry"`) + } + l := len(o) + switch { + case l == 0: + return nil, nil + case l == 2: + return nil, errors.Errorf("invalid credential spec: cannot specify both %s and %s", o[0], o[1]) + case l > 2: + return nil, errors.Errorf("invalid credential spec: cannot specify both %s, and %s", strings.Join(o[:l-1], ", "), o[l-1]) } swarmCredSpec := swarm.CredentialSpec(spec) return &swarmCredSpec, nil diff --git a/cli/compose/convert/service_test.go b/cli/compose/convert/service_test.go index d281e0e4529b..f275fb874cad 100644 --- a/cli/compose/convert/service_test.go +++ b/cli/compose/convert/service_test.go @@ -314,30 +314,65 @@ func TestConvertDNSConfigSearch(t *testing.T) { } func TestConvertCredentialSpec(t *testing.T) { - swarmSpec, err := convertCredentialSpec(composetypes.CredentialSpecConfig{}) - assert.NilError(t, err) - assert.Check(t, is.Nil(swarmSpec)) - - swarmSpec, err = convertCredentialSpec(composetypes.CredentialSpecConfig{ - File: "/foo", - }) - assert.NilError(t, err) - assert.Check(t, is.Equal(swarmSpec.File, "/foo")) - assert.Check(t, is.Equal(swarmSpec.Registry, "")) + tests := []struct { + name string + in composetypes.CredentialSpecConfig + out *swarm.CredentialSpec + expectedErr string + }{ + { + name: "empty", + }, + { + name: "config-and-file", + in: composetypes.CredentialSpecConfig{Config: "0bt9dmxjvjiqermk6xrop3ekq", File: "somefile.json"}, + expectedErr: `invalid credential spec: cannot specify both "Config" and "File"`, + }, + { + name: "config-and-registry", + in: composetypes.CredentialSpecConfig{Config: "0bt9dmxjvjiqermk6xrop3ekq", Registry: "testing"}, + expectedErr: `invalid credential spec: cannot specify both "Config" and "Registry"`, + }, + { + name: "file-and-registry", + in: composetypes.CredentialSpecConfig{File: "somefile.json", Registry: "testing"}, + expectedErr: `invalid credential spec: cannot specify both "File" and "Registry"`, + }, + { + name: "config-and-file-and-registry", + in: composetypes.CredentialSpecConfig{Config: "0bt9dmxjvjiqermk6xrop3ekq", File: "somefile.json", Registry: "testing"}, + expectedErr: `invalid credential spec: cannot specify both "Config", "File", and "Registry"`, + }, + { + name: "config", + in: composetypes.CredentialSpecConfig{Config: "0bt9dmxjvjiqermk6xrop3ekq"}, + out: &swarm.CredentialSpec{Config: "0bt9dmxjvjiqermk6xrop3ekq"}, + }, + { + name: "file", + in: composetypes.CredentialSpecConfig{File: "somefile.json"}, + out: &swarm.CredentialSpec{File: "somefile.json"}, + }, + { + name: "registry", + in: composetypes.CredentialSpecConfig{Registry: "testing"}, + out: &swarm.CredentialSpec{Registry: "testing"}, + }, + } - swarmSpec, err = convertCredentialSpec(composetypes.CredentialSpecConfig{ - Registry: "foo", - }) - assert.NilError(t, err) - assert.Check(t, is.Equal(swarmSpec.File, "")) - assert.Check(t, is.Equal(swarmSpec.Registry, "foo")) + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + swarmSpec, err := convertCredentialSpec(tc.in) - swarmSpec, err = convertCredentialSpec(composetypes.CredentialSpecConfig{ - File: "/asdf", - Registry: "foo", - }) - assert.Check(t, is.ErrorContains(err, "")) - assert.Check(t, is.Nil(swarmSpec)) + if tc.expectedErr != "" { + assert.Error(t, err, tc.expectedErr) + } else { + assert.NilError(t, err) + } + assert.DeepEqual(t, swarmSpec, tc.out) + }) + } } func TestConvertUpdateConfigOrder(t *testing.T) { diff --git a/cli/compose/loader/loader_test.go b/cli/compose/loader/loader_test.go index f78eb2f1feb4..a5933cc31c4b 100644 --- a/cli/compose/loader/loader_test.go +++ b/cli/compose/loader/loader_test.go @@ -295,6 +295,20 @@ configs: assert.Assert(t, is.Len(actual.Configs, 1)) } +func TestLoadV38(t *testing.T) { + actual, err := loadYAML(` +version: "3.8" +services: + foo: + image: busybox + credential_spec: + config: "0bt9dmxjvjiqermk6xrop3ekq" +`) + assert.NilError(t, err) + assert.Assert(t, is.Len(actual.Services, 1)) + assert.Check(t, is.Equal(actual.Services[0].CredentialSpec.Config, "0bt9dmxjvjiqermk6xrop3ekq")) +} + func TestParseAndLoad(t *testing.T) { actual, err := loadYAML(sampleYAML) assert.NilError(t, err) diff --git a/cli/compose/schema/bindata.go b/cli/compose/schema/bindata.go index ccecb08cbcc7..581e9d502df2 100644 --- a/cli/compose/schema/bindata.go +++ b/cli/compose/schema/bindata.go @@ -510,45 +510,45 @@ bnBpPlHfjORjkTRf1wyAwiYqMXd9/G6313QfoXs6/sbZ66r6e179PwAA//8ZL3SpvkUAAA== "/data/config_schema_v3.8.json": { local: "data/config_schema_v3.8.json", - size: 18204, + size: 18246, modtime: 1518458244, compressed: ` H4sIAAAAAAAC/+xcS4/juBG++1cI2r1tPwbIIkjmlmNOyTkNj0BTZZvbFMktUp72DvzfAz1bokiRtuXu -3qQDBDstFR9FVn38qljyj1WSpD9ruoeCpF+TdG+M+vr4+JuW4r55+iBx95gj2Zr7L78+Ns9+Su+qdiyv -mlAptmyXNW+yw18e/vZQNW9EzFFBJSQ3vwE1zTOE30uGUDV+Sg+AmkmRru9W1TuFUgEaBjr9mlSTS5Je -pHsw6FYbZGKX1o9PdQ9JkmrAA6ODHvqp/vT42v9jL3Zn9zqYbP1cEWMAxb+nc6tff3si93/84/4/X+7/ -/pDdr3/5efS6Wl+EbTN8DlsmmGFS9OOnveSp/depH5jkeS1M+GjsLeEaxjoLMN8lPod07sXeSed2fIfO -Y3UOkpdFcAc7qXdSphl+mf3TQBFM2GQbqXez2Gr4ZRRuUCOkcCf1Tgo3w1+n8KpT2j3H9NvLffXfU93n -bH9NL4P51UqMMM+1nC7M8a9nv6CelcxBcXmsZ+5es0agAGHSfpmSJN2UjOf2qksB/6q6eBo8TJIfNrwP -+qnfj/7yG0X/3qNL/55KYeDF1ErND90sgaTPgFvGIbYFwcbSPUvGmTaZxCxn1Djbc7IBflUPlNA9ZFuU -RbCXbdZoop0ddQgeqbkhuIPoldX7ItPsj9G6PqVMGNgBpnd92/XJajvpLOyYtk9X/1uvHB2mlKiM5PlI -CYJIjtWMmIFCu/VL0lKw30v4ZytisAS73xylWr7jHcpSZYpg5YXza59SWRRELOWa5+gRsfKTQ2Lk7+0Y -w1f9aKNpebRJIqzSARcBuAkDTmXpskQaix/n+lGSpCXL44V35wgXMh/PW5TFBjA9TYQnTjr6e71yvbF2 -3xAmADNBCgjaMUIOwjDCM62A+mzGsWlz25VGwnyKsGPa4NEpu/IgVRxKDbXMQYHIddaEQ+fjeJpDHxst -ijm5mDufmm6qE6qaW2o1zDQQpPsL28uCMBFjISAMHpVkDSZ+OLADcch6azt7GUAcGEpRdIgfxxMG7V+U -1HA90vandqv4XQ8Qa8tjthILUk22G9vrJVPLGy7gUIeKXxOecSaelzdxeDFIsr3U5hIqlu6BcLOne6DP -M82HUqPWUpsYI2cF2YWFBBufJRspORAxFlI02I+WnJg2NzMneDGBTRfdykG3crerRH32OwmIIkOJHNkB -MJbvSvUax7kO/RDRCAa+I9FvD03cO+Oj9b84nxJs13luP7GPxNjD7XVXCkIrpo2gdcii2jgkm9CRV9mJ -sI7F/YvCo/PD0qitC+YugiTXR2TjrSyO1HbbzhnRoK+LMwcodPg10iZcbf8629bT1NtnfFQZ6GrInjl3 -TmQd5tO3DHrVOCYYY0WNEEMHUxLNm4Rprzj1Sh+awaeRm73dUY1uE+7NoFRcsNflQNwNVLnhTO8hP6cN -SiOp5HGO4cxqxTvDTOh3EdNTyA6Mw87S2EVjEEieScGPEZLaEAwmTDTQEpk5ZlKZxTmmOwP2avV9Amw8 -Ievu4DNL8v+TJdFHTc1l3FqbnIlMKhBB39BGqmyHhEKmAJl0LsUIYPMSm9Bg0o1mO0F4yM1MobYXphSM -CTt7yVnB/E7jTBMF+VrD1dwUbYaeRUH2TIQwHyBERAZ7gmccHbVjbj3n0yqSA42rAOr+7tqJrJ3yZ1Ev -exprL/txO1Wpg0FcLSN0FnG0O66z/xwIPdqjWnx9EY63I0Vi561RP5oRjK8INdMGBD3GD7Rhk3uVc+Ou -uKirliI7fyrGHZtE+2pb6fAmqghJpfJszZVq9EfK7bXoOJw/OLWRcyaOLZhgRVmkX5Mvvog1fmVuTO2t -HNAMofdh73eJz9XJnjOcs+XTfO3HuK7izOIUK1U7V1ExFA1WqcxXd4QqL5gmG+syypm3FQbw4CZYYYaG -YJBZ90Mddx1SLNAf8xbFsAJkaS6lpwTN+QTXrmEbFMp09zFzJjSQtC3oqTehLu0SNJMYPgIir+/BosgL -guKMEh0iiFck+VFyviH0OWsLrha6u1UECefAmS5i2G2aAyfHiyynudAijJcIGaERVyLtXglmJF4+ZEFe -sm7YWiTgt42fYg6+MUHU54zNLxvPuN8y1KZJQ0jV/jWG/wWvukuVEwOfJvFpEsMMXR0b6KXMwZkEWKam -UJWx9xVpAYUMV45cm/KfFKzoiib4LiA/ygI4pHcgABnNRtbgOXKmsje6RbneshvuITlrQswlzJtK0cwj -BnmuhLoKdyoiXiijo6D1OxO5/H4+zVpgtRUnFCxqdu1Ca4OECXN2rYK9LAphCwiCwqxbTnNGM3mj5RLy -CoHk73Bl5LK2jphWhD0TNpN1ZSQvMZsrvnFwAtVcJDBtMAkpx/vu2G//Pvv3t4otKYKBfmRXDWXIhubt -J31us2FBiE8PhJcRtycX1Zv4sg4RjU/OT65Ce9qJLRDaxdR/RRUgtVKZVMvfgISLjNbh/DtTpFgKm6NL -slJnqPERULfcCE+C+8aou9yR29Vmenb1qU9l3fVrtY7eYq9jLDf/OqtmX1u60m/EGEL3UZm6MxMmb5D4 -nCT6nZDWSn0i2hmI9me3/49nq+3XqMEvHmup8AekV1hoxDciH2D/l9jW/zm3rOJVTgxkM+q8gS1PmIfT -llupT1te2pY/iBVYJU0Da5herc1tUHTd9Wp4k9ZPwxZz/O6GLwr1Tsp3EWwN2u7NvOYLgsjDLzNsf+77 -iBvR5AWKSd17aiWoVn3pqP2zAX7o6dpPfkSg0lMcJ1e/P8blQ80PAKxH62OJNN8uDVB7HZW8cP20gF28 -1H3i76mnHEf4q+r/p9V/AwAA//9tFzTmHEcAAA== +3qQDBDstFR/15FfFkn+skiT9WdM9FCT9mqR7Y9TXx8fftBT3zdMHibvHHMnW3H/59bF59lN6V41jeTWE +SrFlu6x5kx3+8vC3h2p4Q2KOCioiufkNqGmeIfxeMoRq8FN6ANRMinR9t6reKZQK0DDQ6dek2lyS9CTd +g8G02iATu7R+fKpnSJJUAx4YHczQb/Wnx9f5H3uyO3vWwWbr54oYAyj+Pd1b/frbE7n/4x/3//ly//eH +7H79y8+j15V8EbbN8jlsmWCGSdGvn/aUp/Zfp35hkuc1MeGjtbeEaxjzLMB8l/gc4rkneyee2/UdPI/Z +OUheFkENdlTvxEyz/DL600ARTNhkG6p3s9hq+WUYbqJGiOGO6p0Ybpa/juFVx7R7j+m3l/vqv6d6ztn5 +mlkG+6uZGMU8lzhdMccvz16gHknmoLg81jt3y6whKECYtBdTkqSbkvHclroU8K9qiqfBwyT5YYf3wTz1 ++9FffqPo33t46d9TKQy8mJqp+aUbEUj6DLhlHGJHEGws3SMyzrTJJGY5o8Y5npMN8KtmoITuIduiLIKz +bLOGE+2cqIvgkZwbgjuIlqzeF5lmf4zk+pQyYWAHmN71Y9cna+xksrBj2j5d/W+9ckyYUqIykucjJggi +OVY7YgYK7eYvSUvBfi/hny2JwRLseXOUavmJdyhLlSmClRfOyz6lsiiIWMo1z+EjQvKTQ2Lk7+0aw1f9 +aqNtebhJIqzSES4C4SYccCpLlyXS2Phxrh8lSVqyPJ54dw5xIfPxvkVZbADT04R44qSjv9cr1xtL+4Yw +AZgJUkDQjhFyEIYRnmkF1GczDqXNqas1wQjxpJEHQoqwY9rg0Um78sS0uHg2lEcOCkSusyZxOj/ipzn0 +WdSi0SkXcydZM011llV7S62BmQaCdH/heFkQJmJsCYTBo5KsiZ4fLiyCOGS9tZ0tBhAHhlIU3dkQhygG +41+U1HB9TO7P95bxuz6UrG3PkliQarPd2l4vmVreUIBDHiokTnjGmXhe3sThxSDJ9lKbS0BbugfCzZ7u +gT7PDB9SjUZLbWKMnBVkFyYSbHzqbKTkQMSYSNHgPFpyYtoqzhzhxVA3XVSVg2nlbleR+ux3kjpFJh05 +sgNgLDKW6jXjc8GDECQJpsgj0m8PTYY846P1vzifQnHXyW8/sY/E2MPtVSsFoRUmR9A6ZFFtxpJNgMsr +7YRYx8b9ixKp8xPYKNUFqxxBOOyDvPFWFgd/O7VzRjTo6zLSQRQ6/BppE66xf50d6xnqnTM+/wxMNcTZ +nDs3sg4j71umx2qcPYxjRR0hhg6mJJo3Sehe49QrfGgWn+Z4trqjBt0mMZyJUnFpYVctcQ9Q5YYzvYf8 +nDEojaSSxzmGs/4V7wwzSeJFSE8hOzAOO4tjF4xBIHkmBT9GUGpDMFha0UBLZOaYSWUWx5juWtmr1fel +svGGrFuGz3rK/089RR81NZdha21yJjKpQAR9Qxupsh0SCpkCZNIpilGAzUtsUoPJNJrtBOEhNzOF2l5Y +UjAm7OwlZwXzO42zoBTEaw1Wc0O0GXgWFbJnMoT5BCEiM9gTPOPoqB1z6zmfVpEYaNwvUM93125k7aQ/ +C3rZ21h70Y/bqUodTOJqGqGziKPdcfH954jQIx3V5OuL4ni7UmTsvHXUj0YE44KxZtqAoMf4hTZscgNz +bt4Vl3XVVGTnL8W4c5NoX217It6EFSGpVB7VXMlGf6TcnosOw/mTUztyzuSxBROsKIv0a/LFl7HGS+bG +0N6qAc0Ael/s/S7xuTrZc4Zztnya7xIZd2Cc2cZilWrnei+GpMF+lvk+kFCPBtNkY11GOeu2wgAe3AAr +jNAQDDLrfqjDrkOIBfpj3qIYVoAszaXwlKA5H+Da3W6DlpruPmbOhAaUtgU99SbUlV2CZhKDR0Dk9T1Y +FHhBUJxRokMA8YoiP0rON4Q+Z6/3skvc8iqChHPgTBcx6DbNgZPjRZbTXGgRxkuEjNCIK5FWV4IZiZcv +WZCXrFu2Jgn4beOnmINvTRD1OWPjy8Yz7rcMtWnKEFK1f43D/4JX3aXKiYFPk/g0iWGFrs4N9FLm4CwC +LNN9qMrY+4q0gEKGO0euLflPGlZ0BRN8F5AfRQAO6h0IQEazkTV4jpwp7Y1uUa637AZ7SM6aFHOhNqdm +HzGR58pQV8WdCogXyuio0PqdiVx+Px9mLSBtxQkFC5pdK2htkDBhzu5VsMWiELaAICjMuuW0ZjRTN1qu +IK8QSP4OV0Yua+uAaQXYM2EjWVdF8hKzueJrCGegmssEpgMmKeVY7w59+/Xs12+VW1IEA/3Krm7LkA3N +20/63FbDgiE+PRBeRtyeXNRv4qs6RAw+OT/OCum0I1sgtYvp/4pqQGqpMqmWvwEJNxmtw/V3pkixVGyO +bslKnanGR4i65UZ4Ctw3jrrLHbldb6ZHq099Keuul9U6WsVex1hu/3VVzb62dJXfiDGE7qMqdWcWTN6g +8Dkp9DtDWkv1GdHOiGh/dvv/eLbafrca/Daypgp/anqFhUZ8I/IB9L+EWv/n3LLKVzkxkM2w8wa2PEEe +TltuqT5teWlb/iBWYLU0DaxherU2p6DovuvV8Cat34ZN5viFDl8W6t2U7yLYWrTVzTznCwaRh19m0P7c +9xE3gskLNJO6dWoVqFZ966j9AwP+0NONn/zcQMWnOE6ufn+M24eanwpYj+RjkTTfLg2i9jqqeOH6EQK7 +ean7MQBPP+U4w19V/z+t/hsAAP//Fd/bF0ZHAAA= `, }, diff --git a/cli/compose/schema/data/config_schema_v3.8.json b/cli/compose/schema/data/config_schema_v3.8.json index 670780905b13..059c0bcf76d8 100644 --- a/cli/compose/schema/data/config_schema_v3.8.json +++ b/cli/compose/schema/data/config_schema_v3.8.json @@ -125,6 +125,7 @@ "credential_spec": { "type": "object", "properties": { + "config": {"type": "string"}, "file": {"type": "string"}, "registry": {"type": "string"} }, diff --git a/cli/compose/schema/schema_test.go b/cli/compose/schema/schema_test.go index 9b29b138b3a9..10c40bba72c6 100644 --- a/cli/compose/schema/schema_test.go +++ b/cli/compose/schema/schema_test.go @@ -92,7 +92,7 @@ func TestValidateCredentialSpecs(t *testing.T) { {version: "3.5", expectedErr: "config"}, {version: "3.6", expectedErr: "config"}, {version: "3.7", expectedErr: "config"}, - {version: "3.8", expectedErr: "something"}, + {version: "3.8"}, } for _, tc := range tests { @@ -104,7 +104,7 @@ func TestValidateCredentialSpecs(t *testing.T) { "foo": dict{ "image": "busybox", "credential_spec": dict{ - tc.expectedErr: "foobar", + "config": "foobar", }, }, }, diff --git a/cli/compose/types/types.go b/cli/compose/types/types.go index 0895a4d04d56..d77c1b63dc4e 100644 --- a/cli/compose/types/types.go +++ b/cli/compose/types/types.go @@ -500,8 +500,7 @@ func (e External) MarshalJSON() ([]byte, error) { // CredentialSpecConfig for credential spec on Windows type CredentialSpecConfig struct { - // @TODO Config is not yet in use - Config string `yaml:"-" json:"-"` // Config was added in API v1.40 + Config string `yaml:",omitempty" json:"config,omitempty"` // Config was added in API v1.40 File string `yaml:",omitempty" json:"file,omitempty"` Registry string `yaml:",omitempty" json:"registry,omitempty"` } From 01f4f2e80a2dc1709bc576d005d9181696adfbd7 Mon Sep 17 00:00:00 2001 From: Drew Erny Date: Wed, 27 Mar 2019 15:44:32 -0500 Subject: [PATCH 2/4] Update CredentialSpec code to allow using configs Updates the CredentialSpec handling code for services to allow using swarm Configs. Additionally, fixes a bug where the `--credential-spec` flag would not be respected on service updates. Signed-off-by: Drew Erny --- cli/command/service/create.go | 51 ++++++++++++++--- cli/command/service/opts.go | 13 ++++- cli/command/service/opts_test.go | 7 +-- cli/command/service/parse.go | 35 ++++++++++++ cli/command/service/update.go | 98 +++++++++++++++++++++++++++++++- 5 files changed, 188 insertions(+), 16 deletions(-) diff --git a/cli/command/service/create.go b/cli/command/service/create.go index fb8f26c995a4..aff8d46eb089 100644 --- a/cli/command/service/create.go +++ b/cli/command/service/create.go @@ -8,7 +8,9 @@ import ( "github.com/docker/cli/cli/command" cliopts "github.com/docker/cli/opts" "github.com/docker/docker/api/types" + "github.com/docker/docker/api/types/swarm" "github.com/docker/docker/api/types/versions" + "github.com/docker/docker/client" "github.com/spf13/cobra" "github.com/spf13/pflag" ) @@ -95,15 +97,7 @@ func runCreate(dockerCli command.Cli, flags *pflag.FlagSet, opts *serviceOptions service.TaskTemplate.ContainerSpec.Secrets = secrets } - specifiedConfigs := opts.configs.Value() - if len(specifiedConfigs) > 0 { - // parse and validate configs - configs, err := ParseConfigs(apiClient, specifiedConfigs) - if err != nil { - return err - } - service.TaskTemplate.ContainerSpec.Configs = configs - } + setConfigs(apiClient, &service, opts) if err := resolveServiceImageDigestContentTrust(dockerCli, &service); err != nil { return err @@ -141,3 +135,42 @@ func runCreate(dockerCli command.Cli, flags *pflag.FlagSet, opts *serviceOptions return waitOnService(ctx, dockerCli, response.ID, opts.quiet) } + +// setConfigs does double duty: it both sets the ConfigReferences of the +// service, and it sets the service CredentialSpec. This is because there is an +// interplay between the CredentialSpec and the Config it depends on. +func setConfigs(apiClient client.ConfigAPIClient, service *swarm.ServiceSpec, opts *serviceOptions) error { + specifiedConfigs := opts.configs.Value() + // if the user has requested to use a Config, for the CredentialSpec add it + // to the specifiedConfigs as a RuntimeTarget. + if cs := opts.credentialSpec.Value(); cs != nil && cs.Config != "" { + specifiedConfigs = append(specifiedConfigs, &swarm.ConfigReference{ + ConfigName: cs.Config, + Runtime: &swarm.ConfigReferenceRuntimeTarget{}, + }) + } + if len(specifiedConfigs) > 0 { + // parse and validate configs + configs, err := ParseConfigs(apiClient, specifiedConfigs) + if err != nil { + return err + } + service.TaskTemplate.ContainerSpec.Configs = configs + // if we have a CredentialSpec Config, find its ID and rewrite the + // field on the spec + // + // we check the opts instead of the service directly because there are + // a few layers of nullable objects in the service, which is a PITA + // to traverse, but the existence of the option implies that those are + // non-null. + if cs := opts.credentialSpec.Value(); cs != nil && cs.Config != "" { + for _, config := range configs { + if config.ConfigName == cs.Config { + service.TaskTemplate.ContainerSpec.Privileges.CredentialSpec.Config = config.ConfigID + } + } + } + } + + return nil +} diff --git a/cli/command/service/opts.go b/cli/command/service/opts.go index ce9f39c8420a..d76687337ad2 100644 --- a/cli/command/service/opts.go +++ b/cli/command/service/opts.go @@ -332,11 +332,22 @@ func (c *credentialSpecOpt) Set(value string) error { c.value = &swarm.CredentialSpec{} switch { case strings.HasPrefix(value, "config://"): + // NOTE(dperny): we allow the user to specify the value of + // CredentialSpec Config using the Name of the config, but the API + // requires the ID of the config. For simplicity, we will parse + // whatever value is provided into the "Config" field, but before + // making API calls, we may need to swap the Config Name for the ID. + // Therefore, this isn't the definitive location for the value of + // Config that is passed to the API. c.value.Config = strings.TrimPrefix(value, "config://") case strings.HasPrefix(value, "file://"): c.value.File = strings.TrimPrefix(value, "file://") case strings.HasPrefix(value, "registry://"): c.value.Registry = strings.TrimPrefix(value, "registry://") + case value == "": + // if the value of the flag is an empty string, that means there is no + // CredentialSpec needed. This is useful for removing a CredentialSpec + // during a service update. default: return errors.New(`invalid credential spec: value must be prefixed with "config://", "file://", or "registry://"`) } @@ -665,7 +676,7 @@ func (options *serviceOptions) ToService(ctx context.Context, apiClient client.N EndpointSpec: options.endpoint.ToEndpointSpec(), } - if options.credentialSpec.Value() != nil { + if options.credentialSpec.String() != "" && options.credentialSpec.Value() != nil { service.TaskTemplate.ContainerSpec.Privileges = &swarm.Privileges{ CredentialSpec: options.credentialSpec.Value(), } diff --git a/cli/command/service/opts_test.go b/cli/command/service/opts_test.go index 9f26daa3bf33..6b9b83d18626 100644 --- a/cli/command/service/opts_test.go +++ b/cli/command/service/opts_test.go @@ -22,10 +22,9 @@ func TestCredentialSpecOpt(t *testing.T) { expectedErr string }{ { - name: "empty", - in: "", - value: swarm.CredentialSpec{}, - expectedErr: `invalid credential spec: value must be prefixed with "config://", "file://", or "registry://"`, + name: "empty", + in: "", + value: swarm.CredentialSpec{}, }, { name: "no-prefix", diff --git a/cli/command/service/parse.go b/cli/command/service/parse.go index 254107071fd1..b3d35f551ac3 100644 --- a/cli/command/service/parse.go +++ b/cli/command/service/parse.go @@ -70,10 +70,29 @@ func ParseConfigs(client client.ConfigAPIClient, requestedConfigs []*swarmtypes. return []*swarmtypes.ConfigReference{}, nil } + // the configRefs map has two purposes: it prevents duplication of config + // target filenames, and it it used to get all configs so we can resolve + // their IDs. unfortunately, there are other targets for ConfigReferences, + // besides just a File; specifically, the Runtime target, which is used for + // CredentialSpecs. Therefore, we need to have a list of ConfigReferences + // that are not File targets as well. at this time of writing, the only use + // for Runtime targets is CredentialSpecs. However, to future-proof this + // functionality, we should handle the case where multiple Runtime targets + // are in use for the same Config, and we should deduplicate + // such ConfigReferences, as no matter how many times the Config is used, + // it is only needed to be referenced once. configRefs := make(map[string]*swarmtypes.ConfigReference) + runtimeRefs := make(map[string]*swarmtypes.ConfigReference) ctx := context.Background() for _, config := range requestedConfigs { + if config.Runtime != nil { + // by assigning to a map based on ConfigName, if the same Config + // is required as a Runtime target for multiple purposes, we only + // include it once in the final set of configs. + runtimeRefs[config.ConfigName] = config + } + if _, exists := configRefs[config.File.Name]; exists { return nil, errors.Errorf("duplicate config target for %s not allowed", config.ConfigName) } @@ -87,6 +106,9 @@ func ParseConfigs(client client.ConfigAPIClient, requestedConfigs []*swarmtypes. for _, s := range configRefs { args.Add("name", s.ConfigName) } + for _, s := range runtimeRefs { + args.Add("name", s.ConfigName) + } configs, err := client.ConfigList(ctx, types.ConfigListOptions{ Filters: args, @@ -114,5 +136,18 @@ func ParseConfigs(client client.ConfigAPIClient, requestedConfigs []*swarmtypes. addedConfigs = append(addedConfigs, ref) } + // unfortunately, because the key of configRefs and runtimeRefs is different + // values that may collide, we can't just do some fancy trickery to + // concat maps, we need to do two separate loops + for _, ref := range runtimeRefs { + id, ok := foundConfigs[ref.ConfigName] + if !ok { + return nil, errors.Errorf("config not found: %s", ref.ConfigName) + } + + ref.ConfigID = id + addedConfigs = append(addedConfigs, ref) + } + return addedConfigs, nil } diff --git a/cli/command/service/update.go b/cli/command/service/update.go index 8269adcd0011..7371a28bc1a0 100644 --- a/cli/command/service/update.go +++ b/cli/command/service/update.go @@ -201,6 +201,48 @@ func runUpdate(dockerCli command.Cli, flags *pflag.FlagSet, options *serviceOpti spec.TaskTemplate.ContainerSpec.Configs = updatedConfigs + // set the credential spec value after get the updated configs, because we + // might need the updated configs to set the correct value of the + // CredentialSpec. + if flags.Changed(flagCredentialSpec) { + containerSpec := spec.TaskTemplate.ContainerSpec + credSpecOpt := flags.Lookup(flagCredentialSpec) + // if the flag has changed, and the value is empty string, then we + // should remove any credential spec that might be present + if credSpecOpt.Value.String() == "" { + if containerSpec.Privileges != nil { + containerSpec.Privileges.CredentialSpec = nil + } + return nil + } + + // otherwise, set the credential spec to be the parsed value + credSpec := credSpecOpt.Value.(*credentialSpecOpt).Value() + + // if this is a Config credential spec, we we still need to replace the + // value of credSpec.Config with the config ID instead of Name. + if credSpec.Config != "" { + for _, config := range updatedConfigs { + // if the config name matches, then set the config ID. we do + // not need to worry about if this is a Runtime target or not. + // even if it is not a Runtime target, getUpdatedConfigs + // ensures that a Runtime target for this config exists, and + // the Name is unique so the ID is correct no matter the + // target. + if config.ConfigName == credSpec.Config { + credSpec.Config = config.ConfigID + break + } + } + } + + if containerSpec.Privileges == nil { + containerSpec.Privileges = &swarm.Privileges{} + } + + containerSpec.Privileges.CredentialSpec = credSpec + } + // only send auth if flag was set sendAuth, err := flags.GetBool(flagRegistryAuth) if err != nil { @@ -734,17 +776,69 @@ func getUpdatedSecrets(apiClient client.SecretAPIClient, flags *pflag.FlagSet, s func getUpdatedConfigs(apiClient client.ConfigAPIClient, flags *pflag.FlagSet, configs []*swarm.ConfigReference) ([]*swarm.ConfigReference, error) { newConfigs := []*swarm.ConfigReference{} + // resolveConfigs is a slice of any new configs that need to have the ID + // resolved + resolveConfigs := []*swarm.ConfigReference{} + + // credSpecConfig is used for two things, and may be a bit confusing. + // First, it's used to store the temporary value of the ConfigReference for + // a credential spec config. This signals to the loop that removes configs + // that a credential spec config is needed. If a Runtime target Config with + // this name is found in the existing configs, then that loop will nil this + // variable. then, before we go looking up all the new configs, if this + // variable is non-nil, we'll add it to resolveConfigs and fill in its ID. + var credSpecConfig *swarm.ConfigReference + + if flags.Changed(flagCredentialSpec) { + credSpec := flags.Lookup(flagCredentialSpec).Value.(*credentialSpecOpt).Value() + if credSpec.Config != "" { + credSpecConfig = &swarm.ConfigReference{ + ConfigName: credSpec.Config, + Runtime: &swarm.ConfigReferenceRuntimeTarget{}, + } + } + } + toRemove := buildToRemoveSet(flags, flagConfigRemove) for _, config := range configs { + // if the config is a Runtime target, make sure it's still in use right + // now, the only use for Runtime target is credential specs. if, in + // the future, more uses are added, then this check will need to be + // made more intelligent. + if config.Runtime != nil { + // for now, just check if the credential spec flag has changed, and + // if it has, that the credSpecConfig exists and has the same name. + // if the credential spec flag has changed, and it's either no + // longer a config or its a different config, then we do not add + // this config to newConfigs + if flags.Changed(flagCredentialSpec) && credSpecConfig != nil { + if credSpecConfig.ConfigName == config.ConfigName { + newConfigs = append(newConfigs, config) + credSpecConfig = nil + } + } + // continue the loop, to skip the part where we check if the config + // is in toRemove. + continue + } + if _, exists := toRemove[config.ConfigName]; !exists { newConfigs = append(newConfigs, config) } } if flags.Changed(flagConfigAdd) { - values := flags.Lookup(flagConfigAdd).Value.(*opts.ConfigOpt).Value() + resolveConfigs = append(resolveConfigs, flags.Lookup(flagConfigAdd).Value.(*opts.ConfigOpt).Value()...) + } + + // if credSpecConfig is non-nil at this point, it means its a new config, + // and we need to resolve its ID accordingly. + if credSpecConfig != nil { + resolveConfigs = append(resolveConfigs, credSpecConfig) + } - addConfigs, err := ParseConfigs(apiClient, values) + if len(resolveConfigs) > 0 { + addConfigs, err := ParseConfigs(apiClient, resolveConfigs) if err != nil { return nil, err } From 4cacd1304ae7b3ec0170cfcc10e84f69b300c6d7 Mon Sep 17 00:00:00 2001 From: Drew Erny Date: Thu, 28 Mar 2019 14:06:06 -0500 Subject: [PATCH 3/4] Add CredentialSpec tests Adds tests for setting and updating swarm service CredentialSpecs, especially when using a Config as a credential spec. Signed-off-by: Drew Erny --- cli/command/service/create.go | 4 +- cli/command/service/create_test.go | 271 ++++++++++++++++++++++++ cli/command/service/parse.go | 9 +- cli/command/service/update.go | 202 ++++++++++-------- cli/command/service/update_test.go | 323 +++++++++++++++++++++++++++++ 5 files changed, 719 insertions(+), 90 deletions(-) create mode 100644 cli/command/service/create_test.go diff --git a/cli/command/service/create.go b/cli/command/service/create.go index aff8d46eb089..ae359bd68717 100644 --- a/cli/command/service/create.go +++ b/cli/command/service/create.go @@ -97,7 +97,9 @@ func runCreate(dockerCli command.Cli, flags *pflag.FlagSet, opts *serviceOptions service.TaskTemplate.ContainerSpec.Secrets = secrets } - setConfigs(apiClient, &service, opts) + if err := setConfigs(apiClient, &service, opts); err != nil { + return err + } if err := resolveServiceImageDigestContentTrust(dockerCli, &service); err != nil { return err diff --git a/cli/command/service/create_test.go b/cli/command/service/create_test.go new file mode 100644 index 000000000000..cd675c7995c4 --- /dev/null +++ b/cli/command/service/create_test.go @@ -0,0 +1,271 @@ +package service + +import ( + "context" + "testing" + + "github.com/docker/docker/api/types" + "github.com/docker/docker/api/types/swarm" + "gotest.tools/assert" + is "gotest.tools/assert/cmp" + + cliopts "github.com/docker/cli/opts" +) + +// fakeConfigAPIClientList is used to let us pass a closure as a +// ConfigAPIClient, to use as ConfigList. for all the other methods in the +// interface, it does nothing, not even return an error, so don't use them +type fakeConfigAPIClientList func(context.Context, types.ConfigListOptions) ([]swarm.Config, error) + +func (f fakeConfigAPIClientList) ConfigList(ctx context.Context, opts types.ConfigListOptions) ([]swarm.Config, error) { + return f(ctx, opts) +} + +func (f fakeConfigAPIClientList) ConfigCreate(_ context.Context, _ swarm.ConfigSpec) (types.ConfigCreateResponse, error) { + return types.ConfigCreateResponse{}, nil +} + +func (f fakeConfigAPIClientList) ConfigRemove(_ context.Context, _ string) error { + return nil +} + +func (f fakeConfigAPIClientList) ConfigInspectWithRaw(_ context.Context, _ string) (swarm.Config, []byte, error) { + return swarm.Config{}, nil, nil +} + +func (f fakeConfigAPIClientList) ConfigUpdate(_ context.Context, _ string, _ swarm.Version, _ swarm.ConfigSpec) error { + return nil +} + +// TestSetConfigsWithCredSpecAndConfigs tests that the setConfigs function for +// create correctly looks up the right configs, and correctly handles the +// credentialSpec +func TestSetConfigsWithCredSpecAndConfigs(t *testing.T) { + // we can't directly access the internal fields of the ConfigOpt struct, so + // we need to let it do the parsing + configOpt := &cliopts.ConfigOpt{} + configOpt.Set("bar") + opts := &serviceOptions{ + credentialSpec: credentialSpecOpt{ + value: &swarm.CredentialSpec{ + Config: "foo", + }, + source: "config://foo", + }, + configs: *configOpt, + } + + // create a service spec. we need to be sure to fill in the nullable + // fields, like the code expects + service := &swarm.ServiceSpec{ + TaskTemplate: swarm.TaskSpec{ + ContainerSpec: &swarm.ContainerSpec{ + Privileges: &swarm.Privileges{ + CredentialSpec: opts.credentialSpec.value, + }, + }, + }, + } + + // set up a function to use as the list function + var fakeClient fakeConfigAPIClientList = func(_ context.Context, opts types.ConfigListOptions) ([]swarm.Config, error) { + f := opts.Filters + + // we're expecting the filter to have names "foo" and "bar" + names := f.Get("name") + assert.Equal(t, len(names), 2) + assert.Assert(t, is.Contains(names, "foo")) + assert.Assert(t, is.Contains(names, "bar")) + + return []swarm.Config{ + { + ID: "fooID", + Spec: swarm.ConfigSpec{ + Annotations: swarm.Annotations{ + Name: "foo", + }, + }, + }, { + ID: "barID", + Spec: swarm.ConfigSpec{ + Annotations: swarm.Annotations{ + Name: "bar", + }, + }, + }, + }, nil + } + + // now call setConfigs + err := setConfigs(fakeClient, service, opts) + // verify no error is returned + assert.NilError(t, err) + + credSpecConfigValue := service.TaskTemplate.ContainerSpec.Privileges.CredentialSpec.Config + assert.Equal(t, credSpecConfigValue, "fooID") + + configRefs := service.TaskTemplate.ContainerSpec.Configs + assert.Assert(t, is.Contains(configRefs, &swarm.ConfigReference{ + ConfigID: "fooID", + ConfigName: "foo", + Runtime: &swarm.ConfigReferenceRuntimeTarget{}, + }), "expected configRefs to contain foo config") + assert.Assert(t, is.Contains(configRefs, &swarm.ConfigReference{ + ConfigID: "barID", + ConfigName: "bar", + File: &swarm.ConfigReferenceFileTarget{ + Name: "bar", + // these are the default field values + UID: "0", + GID: "0", + Mode: 0444, + }, + }), "expected configRefs to contain bar config") +} + +// TestSetConfigsOnlyCredSpec tests that even if a CredentialSpec is the only +// config needed, setConfigs still works +func TestSetConfigsOnlyCredSpec(t *testing.T) { + opts := &serviceOptions{ + credentialSpec: credentialSpecOpt{ + value: &swarm.CredentialSpec{ + Config: "foo", + }, + source: "config://foo", + }, + } + + service := &swarm.ServiceSpec{ + TaskTemplate: swarm.TaskSpec{ + ContainerSpec: &swarm.ContainerSpec{ + Privileges: &swarm.Privileges{ + CredentialSpec: opts.credentialSpec.value, + }, + }, + }, + } + + // set up a function to use as the list function + var fakeClient fakeConfigAPIClientList = func(_ context.Context, opts types.ConfigListOptions) ([]swarm.Config, error) { + f := opts.Filters + + names := f.Get("name") + assert.Equal(t, len(names), 1) + assert.Assert(t, is.Contains(names, "foo")) + + return []swarm.Config{ + { + ID: "fooID", + Spec: swarm.ConfigSpec{ + Annotations: swarm.Annotations{ + Name: "foo", + }, + }, + }, + }, nil + } + + // now call setConfigs + err := setConfigs(fakeClient, service, opts) + // verify no error is returned + assert.NilError(t, err) + + credSpecConfigValue := service.TaskTemplate.ContainerSpec.Privileges.CredentialSpec.Config + assert.Equal(t, credSpecConfigValue, "fooID") + + configRefs := service.TaskTemplate.ContainerSpec.Configs + assert.Assert(t, is.Contains(configRefs, &swarm.ConfigReference{ + ConfigID: "fooID", + ConfigName: "foo", + Runtime: &swarm.ConfigReferenceRuntimeTarget{}, + })) +} + +// TestSetConfigsOnlyConfigs verifies setConfigs when only configs (and not a +// CredentialSpec) is needed. +func TestSetConfigsOnlyConfigs(t *testing.T) { + configOpt := &cliopts.ConfigOpt{} + configOpt.Set("bar") + opts := &serviceOptions{ + configs: *configOpt, + } + + service := &swarm.ServiceSpec{ + TaskTemplate: swarm.TaskSpec{ + ContainerSpec: &swarm.ContainerSpec{}, + }, + } + + var fakeClient fakeConfigAPIClientList = func(_ context.Context, opts types.ConfigListOptions) ([]swarm.Config, error) { + f := opts.Filters + + names := f.Get("name") + assert.Equal(t, len(names), 1) + assert.Assert(t, is.Contains(names, "bar")) + + return []swarm.Config{ + { + ID: "barID", + Spec: swarm.ConfigSpec{ + Annotations: swarm.Annotations{ + Name: "bar", + }, + }, + }, + }, nil + } + + // now call setConfigs + err := setConfigs(fakeClient, service, opts) + // verify no error is returned + assert.NilError(t, err) + + configRefs := service.TaskTemplate.ContainerSpec.Configs + assert.Assert(t, is.Contains(configRefs, &swarm.ConfigReference{ + ConfigID: "barID", + ConfigName: "bar", + File: &swarm.ConfigReferenceFileTarget{ + Name: "bar", + // these are the default field values + UID: "0", + GID: "0", + Mode: 0444, + }, + })) +} + +// TestSetConfigsNoConfigs checks that setConfigs works when there are no +// configs of any kind needed +func TestSetConfigsNoConfigs(t *testing.T) { + // add a credentialSpec that isn't a config + opts := &serviceOptions{ + credentialSpec: credentialSpecOpt{ + value: &swarm.CredentialSpec{ + File: "foo", + }, + source: "file://foo", + }, + } + service := &swarm.ServiceSpec{ + TaskTemplate: swarm.TaskSpec{ + ContainerSpec: &swarm.ContainerSpec{ + Privileges: &swarm.Privileges{ + CredentialSpec: opts.credentialSpec.value, + }, + }, + }, + } + + var fakeClient fakeConfigAPIClientList = func(_ context.Context, opts types.ConfigListOptions) ([]swarm.Config, error) { + // assert false -- we should never call this function + assert.Assert(t, false, "we should not be listing configs") + return nil, nil + } + + err := setConfigs(fakeClient, service, opts) + assert.NilError(t, err) + + // ensure that the value of the credentialspec has not changed + assert.Equal(t, service.TaskTemplate.ContainerSpec.Privileges.CredentialSpec.File, "foo") + assert.Equal(t, service.TaskTemplate.ContainerSpec.Privileges.CredentialSpec.Config, "") +} diff --git a/cli/command/service/parse.go b/cli/command/service/parse.go index b3d35f551ac3..25677d3841d6 100644 --- a/cli/command/service/parse.go +++ b/cli/command/service/parse.go @@ -86,19 +86,24 @@ func ParseConfigs(client client.ConfigAPIClient, requestedConfigs []*swarmtypes. ctx := context.Background() for _, config := range requestedConfigs { + // copy the config, so we don't mutate the args + configRef := new(swarmtypes.ConfigReference) + *configRef = *config + if config.Runtime != nil { // by assigning to a map based on ConfigName, if the same Config // is required as a Runtime target for multiple purposes, we only // include it once in the final set of configs. runtimeRefs[config.ConfigName] = config + // continue, so we skip the logic below for handling file-type + // configs + continue } if _, exists := configRefs[config.File.Name]; exists { return nil, errors.Errorf("duplicate config target for %s not allowed", config.ConfigName) } - configRef := new(swarmtypes.ConfigReference) - *configRef = *config configRefs[config.File.Name] = configRef } diff --git a/cli/command/service/update.go b/cli/command/service/update.go index 7371a28bc1a0..c95dcf50d3e9 100644 --- a/cli/command/service/update.go +++ b/cli/command/service/update.go @@ -194,7 +194,7 @@ func runUpdate(dockerCli command.Cli, flags *pflag.FlagSet, options *serviceOpti spec.TaskTemplate.ContainerSpec.Secrets = updatedSecrets - updatedConfigs, err := getUpdatedConfigs(apiClient, flags, spec.TaskTemplate.ContainerSpec.Configs) + updatedConfigs, err := getUpdatedConfigs(apiClient, flags, spec.TaskTemplate.ContainerSpec) if err != nil { return err } @@ -204,44 +204,7 @@ func runUpdate(dockerCli command.Cli, flags *pflag.FlagSet, options *serviceOpti // set the credential spec value after get the updated configs, because we // might need the updated configs to set the correct value of the // CredentialSpec. - if flags.Changed(flagCredentialSpec) { - containerSpec := spec.TaskTemplate.ContainerSpec - credSpecOpt := flags.Lookup(flagCredentialSpec) - // if the flag has changed, and the value is empty string, then we - // should remove any credential spec that might be present - if credSpecOpt.Value.String() == "" { - if containerSpec.Privileges != nil { - containerSpec.Privileges.CredentialSpec = nil - } - return nil - } - - // otherwise, set the credential spec to be the parsed value - credSpec := credSpecOpt.Value.(*credentialSpecOpt).Value() - - // if this is a Config credential spec, we we still need to replace the - // value of credSpec.Config with the config ID instead of Name. - if credSpec.Config != "" { - for _, config := range updatedConfigs { - // if the config name matches, then set the config ID. we do - // not need to worry about if this is a Runtime target or not. - // even if it is not a Runtime target, getUpdatedConfigs - // ensures that a Runtime target for this config exists, and - // the Name is unique so the ID is correct no matter the - // target. - if config.ConfigName == credSpec.Config { - credSpec.Config = config.ConfigID - break - } - } - } - - if containerSpec.Privileges == nil { - containerSpec.Privileges = &swarm.Privileges{} - } - - containerSpec.Privileges.CredentialSpec = credSpec - } + updateCredSpecConfig(flags, spec.TaskTemplate.ContainerSpec) // only send auth if flag was set sendAuth, err := flags.GetBool(flagRegistryAuth) @@ -773,49 +736,87 @@ func getUpdatedSecrets(apiClient client.SecretAPIClient, flags *pflag.FlagSet, s return newSecrets, nil } -func getUpdatedConfigs(apiClient client.ConfigAPIClient, flags *pflag.FlagSet, configs []*swarm.ConfigReference) ([]*swarm.ConfigReference, error) { - newConfigs := []*swarm.ConfigReference{} +func getUpdatedConfigs(apiClient client.ConfigAPIClient, flags *pflag.FlagSet, spec *swarm.ContainerSpec) ([]*swarm.ConfigReference, error) { + var ( + // credSpecConfigName stores the name of the config specified by the + // credential-spec flag. if a Runtime target Config with this name is + // already in the containerSpec, then this value will be set to + // emptystring in the removeConfigs stage. otherwise, a ConfigReference + // will be created to pass to ParseConfigs to get the ConfigID. + credSpecConfigName string + // credSpecConfigID stores the ID of the credential spec config if that + // config is being carried over from the old set of references + credSpecConfigID string + ) + + if flags.Changed(flagCredentialSpec) { + credSpec := flags.Lookup(flagCredentialSpec).Value.(*credentialSpecOpt).Value() + credSpecConfigName = credSpec.Config + } else { + // if the credential spec flag has not changed, then check if there + // already is a credentialSpec. if there is one, and it's for a Config, + // then it's from the old object, and its value is the config ID. we + // need this so we don't remove the config if the credential spec is + // not being updated. + if spec.Privileges != nil && spec.Privileges.CredentialSpec != nil { + if config := spec.Privileges.CredentialSpec.Config; config != "" { + credSpecConfigID = config + } + } + } + + newConfigs := removeConfigs(flags, spec, credSpecConfigName, credSpecConfigID) // resolveConfigs is a slice of any new configs that need to have the ID // resolved resolveConfigs := []*swarm.ConfigReference{} - // credSpecConfig is used for two things, and may be a bit confusing. - // First, it's used to store the temporary value of the ConfigReference for - // a credential spec config. This signals to the loop that removes configs - // that a credential spec config is needed. If a Runtime target Config with - // this name is found in the existing configs, then that loop will nil this - // variable. then, before we go looking up all the new configs, if this - // variable is non-nil, we'll add it to resolveConfigs and fill in its ID. - var credSpecConfig *swarm.ConfigReference + if flags.Changed(flagConfigAdd) { + resolveConfigs = append(resolveConfigs, flags.Lookup(flagConfigAdd).Value.(*opts.ConfigOpt).Value()...) + } - if flags.Changed(flagCredentialSpec) { - credSpec := flags.Lookup(flagCredentialSpec).Value.(*credentialSpecOpt).Value() - if credSpec.Config != "" { - credSpecConfig = &swarm.ConfigReference{ - ConfigName: credSpec.Config, - Runtime: &swarm.ConfigReferenceRuntimeTarget{}, - } + // if credSpecConfigNameis non-empty at this point, it means its a new + // config, and we need to resolve its ID accordingly. + if credSpecConfigName != "" { + resolveConfigs = append(resolveConfigs, &swarm.ConfigReference{ + ConfigName: credSpecConfigName, + Runtime: &swarm.ConfigReferenceRuntimeTarget{}, + }) + } + + if len(resolveConfigs) > 0 { + addConfigs, err := ParseConfigs(apiClient, resolveConfigs) + if err != nil { + return nil, err } + newConfigs = append(newConfigs, addConfigs...) } + return newConfigs, nil +} + +// removeConfigs figures out which configs in the existing spec should be kept +// after the update. +func removeConfigs(flags *pflag.FlagSet, spec *swarm.ContainerSpec, credSpecName, credSpecID string) []*swarm.ConfigReference { + keepConfigs := []*swarm.ConfigReference{} + toRemove := buildToRemoveSet(flags, flagConfigRemove) - for _, config := range configs { + // all configs in spec.Configs should have both a Name and ID, because + // they come from an already-accepted spec. + for _, config := range spec.Configs { // if the config is a Runtime target, make sure it's still in use right // now, the only use for Runtime target is credential specs. if, in // the future, more uses are added, then this check will need to be // made more intelligent. if config.Runtime != nil { - // for now, just check if the credential spec flag has changed, and - // if it has, that the credSpecConfig exists and has the same name. - // if the credential spec flag has changed, and it's either no - // longer a config or its a different config, then we do not add - // this config to newConfigs - if flags.Changed(flagCredentialSpec) && credSpecConfig != nil { - if credSpecConfig.ConfigName == config.ConfigName { - newConfigs = append(newConfigs, config) - credSpecConfig = nil - } + // if we're carrying over a credential spec explicitly (because the + // user passed --credential-spec with the same config name) then we + // should match on credSpecName. if we're carrying over a + // credential spec implicitly (because the user did not pass any + // --credential-spec flag) then we should match on credSpecID. in + // either case, we're keeping the config that already exists. + if config.ConfigName == credSpecName || config.ConfigID == credSpecID { + keepConfigs = append(keepConfigs, config) } // continue the loop, to skip the part where we check if the config // is in toRemove. @@ -823,29 +824,11 @@ func getUpdatedConfigs(apiClient client.ConfigAPIClient, flags *pflag.FlagSet, c } if _, exists := toRemove[config.ConfigName]; !exists { - newConfigs = append(newConfigs, config) - } - } - - if flags.Changed(flagConfigAdd) { - resolveConfigs = append(resolveConfigs, flags.Lookup(flagConfigAdd).Value.(*opts.ConfigOpt).Value()...) - } - - // if credSpecConfig is non-nil at this point, it means its a new config, - // and we need to resolve its ID accordingly. - if credSpecConfig != nil { - resolveConfigs = append(resolveConfigs, credSpecConfig) - } - - if len(resolveConfigs) > 0 { - addConfigs, err := ParseConfigs(apiClient, resolveConfigs) - if err != nil { - return nil, err + keepConfigs = append(keepConfigs, config) } - newConfigs = append(newConfigs, addConfigs...) } - return newConfigs, nil + return keepConfigs } func envKey(value string) string { @@ -1314,3 +1297,48 @@ func updateNetworks(ctx context.Context, apiClient client.NetworkAPIClient, flag spec.TaskTemplate.Networks = newNetworks return nil } + +// updateCredSpecConfig updates the value of the credential spec Config field +// to the config ID if the credential spec has changed. it mutates the passed +// spec. it does not handle the case where the credential spec specifies a +// config that does not exist -- that case is handled as part of +// getUpdatedConfigs +func updateCredSpecConfig(flags *pflag.FlagSet, containerSpec *swarm.ContainerSpec) { + if flags.Changed(flagCredentialSpec) { + credSpecOpt := flags.Lookup(flagCredentialSpec) + // if the flag has changed, and the value is empty string, then we + // should remove any credential spec that might be present + if credSpecOpt.Value.String() == "" { + if containerSpec.Privileges != nil { + containerSpec.Privileges.CredentialSpec = nil + } + return + } + + // otherwise, set the credential spec to be the parsed value + credSpec := credSpecOpt.Value.(*credentialSpecOpt).Value() + + // if this is a Config credential spec, we we still need to replace the + // value of credSpec.Config with the config ID instead of Name. + if credSpec.Config != "" { + for _, config := range containerSpec.Configs { + // if the config name matches, then set the config ID. we do + // not need to worry about if this is a Runtime target or not. + // even if it is not a Runtime target, getUpdatedConfigs + // ensures that a Runtime target for this config exists, and + // the Name is unique so the ID is correct no matter the + // target. + if config.ConfigName == credSpec.Config { + credSpec.Config = config.ConfigID + break + } + } + } + + if containerSpec.Privileges == nil { + containerSpec.Privileges = &swarm.Privileges{} + } + + containerSpec.Privileges.CredentialSpec = credSpec + } +} diff --git a/cli/command/service/update_test.go b/cli/command/service/update_test.go index bd35750217d2..6eeea55a1fcf 100644 --- a/cli/command/service/update_test.go +++ b/cli/command/service/update_test.go @@ -925,3 +925,326 @@ func TestUpdateSysCtls(t *testing.T) { }) } } + +func TestUpdateGetUpdatedConfigs(t *testing.T) { + // cannedConfigs is a set of configs that we'll use over and over in the + // tests. it's a map of Name to Config + cannedConfigs := map[string]*swarm.Config{ + "bar": { + ID: "barID", + Spec: swarm.ConfigSpec{ + Annotations: swarm.Annotations{ + Name: "bar", + }, + }, + }, + "cred": { + ID: "credID", + Spec: swarm.ConfigSpec{ + Annotations: swarm.Annotations{ + Name: "cred", + }, + }, + }, + "newCred": { + ID: "newCredID", + Spec: swarm.ConfigSpec{ + Annotations: swarm.Annotations{ + Name: "newCred", + }, + }, + }, + } + // cannedConfigRefs is the same thing, but with config references instead + // instead of ID, however, it just maps an arbitrary string value. this is + // so we could have multiple config refs using the same config + cannedConfigRefs := map[string]*swarm.ConfigReference{ + "fooRef": { + ConfigID: "fooID", + ConfigName: "foo", + File: &swarm.ConfigReferenceFileTarget{ + Name: "foo", + UID: "0", + GID: "0", + Mode: 0444, + }, + }, + "barRef": { + ConfigID: "barID", + ConfigName: "bar", + File: &swarm.ConfigReferenceFileTarget{ + Name: "bar", + UID: "0", + GID: "0", + Mode: 0444, + }, + }, + "bazRef": { + ConfigID: "bazID", + ConfigName: "baz", + File: &swarm.ConfigReferenceFileTarget{ + Name: "baz", + UID: "0", + GID: "0", + Mode: 0444, + }, + }, + "credRef": { + ConfigID: "credID", + ConfigName: "cred", + Runtime: &swarm.ConfigReferenceRuntimeTarget{}, + }, + "newCredRef": { + ConfigID: "newCredID", + ConfigName: "newCred", + Runtime: &swarm.ConfigReferenceRuntimeTarget{}, + }, + } + + type flagVal [2]string + type test struct { + // the name of the subtest + name string + // flags are the flags we'll be setting + flags []flagVal + // oldConfigs are the configs that would already be on the service + // it is a slice of strings corresponding to the the key of + // cannedConfigRefs + oldConfigs []string + // oldCredSpec is the credentialSpec being carried over from the old + // object + oldCredSpec *swarm.CredentialSpec + // lookupConfigs are the configs we're expecting to be listed. it is a + // slice of strings corresponding to the key of cannedConfigs + lookupConfigs []string + // expected is the configs we should get as a result. it is a slice of + // strings corresponding to the key in cannedConfigRefs + expected []string + } + + testCases := []test{ + { + name: "no configs added or removed", + oldConfigs: []string{"fooRef"}, + expected: []string{"fooRef"}, + }, { + name: "add a config", + flags: []flagVal{{"config-add", "bar"}}, + oldConfigs: []string{"fooRef"}, + lookupConfigs: []string{"bar"}, + expected: []string{"fooRef", "barRef"}, + }, { + name: "remove a config", + flags: []flagVal{{"config-rm", "bar"}}, + oldConfigs: []string{"fooRef", "barRef"}, + expected: []string{"fooRef"}, + }, { + name: "include an old credential spec", + oldConfigs: []string{"credRef"}, + oldCredSpec: &swarm.CredentialSpec{Config: "credID"}, + expected: []string{"credRef"}, + }, { + name: "add a credential spec", + oldConfigs: []string{"fooRef"}, + flags: []flagVal{{"credential-spec", "config://cred"}}, + lookupConfigs: []string{"cred"}, + expected: []string{"fooRef", "credRef"}, + }, { + name: "change a credential spec", + oldConfigs: []string{"fooRef", "credRef"}, + oldCredSpec: &swarm.CredentialSpec{Config: "credID"}, + flags: []flagVal{{"credential-spec", "config://newCred"}}, + lookupConfigs: []string{"newCred"}, + expected: []string{"fooRef", "newCredRef"}, + }, { + name: "credential spec no longer config", + oldConfigs: []string{"fooRef", "credRef"}, + oldCredSpec: &swarm.CredentialSpec{Config: "credID"}, + flags: []flagVal{{"credential-spec", "file://someFile"}}, + lookupConfigs: []string{}, + expected: []string{"fooRef"}, + }, { + name: "credential spec becomes config", + oldConfigs: []string{"fooRef"}, + oldCredSpec: &swarm.CredentialSpec{File: "someFile"}, + flags: []flagVal{{"credential-spec", "config://cred"}}, + lookupConfigs: []string{"cred"}, + expected: []string{"fooRef", "credRef"}, + }, { + name: "remove credential spec", + oldConfigs: []string{"fooRef", "credRef"}, + oldCredSpec: &swarm.CredentialSpec{Config: "credID"}, + flags: []flagVal{{"credential-spec", ""}}, + lookupConfigs: []string{}, + expected: []string{"fooRef"}, + }, { + name: "just frick my stuff up", + // a more complicated test. add barRef, remove bazRef, keep fooRef, + // change credentialSpec from credRef to newCredRef + oldConfigs: []string{"fooRef", "bazRef", "credRef"}, + oldCredSpec: &swarm.CredentialSpec{Config: "cred"}, + flags: []flagVal{ + {"config-add", "bar"}, + {"config-rm", "baz"}, + {"credential-spec", "config://newCred"}, + }, + lookupConfigs: []string{"bar", "newCred"}, + expected: []string{"fooRef", "barRef", "newCredRef"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + for _, f := range tc.flags { + flags.Set(f[0], f[1]) + } + + // fakeConfigAPIClientList is actually defined in create_test.go, + // but we'll use it here as well + var fakeClient fakeConfigAPIClientList = func(_ context.Context, opts types.ConfigListOptions) ([]swarm.Config, error) { + names := opts.Filters.Get("name") + assert.Equal(t, len(names), len(tc.lookupConfigs)) + + configs := []swarm.Config{} + for _, lookup := range tc.lookupConfigs { + assert.Assert(t, is.Contains(names, lookup)) + cfg, ok := cannedConfigs[lookup] + assert.Assert(t, ok) + configs = append(configs, *cfg) + } + return configs, nil + } + + // build the actual set of old configs and the container spec + oldConfigs := []*swarm.ConfigReference{} + for _, config := range tc.oldConfigs { + cfg, ok := cannedConfigRefs[config] + assert.Assert(t, ok) + oldConfigs = append(oldConfigs, cfg) + } + + containerSpec := &swarm.ContainerSpec{ + Configs: oldConfigs, + Privileges: &swarm.Privileges{ + CredentialSpec: tc.oldCredSpec, + }, + } + + finalConfigs, err := getUpdatedConfigs(fakeClient, flags, containerSpec) + assert.NilError(t, err) + + // ensure that the finalConfigs consists of all of the expected + // configs + assert.Equal(t, len(finalConfigs), len(tc.expected), + "%v final configs, %v expected", + len(finalConfigs), len(tc.expected), + ) + for _, expected := range tc.expected { + assert.Assert(t, is.Contains(finalConfigs, cannedConfigRefs[expected])) + } + }) + } +} + +func TestUpdateCredSpec(t *testing.T) { + type testCase struct { + // name is the name of the subtest + name string + // flagVal is the value we're setting flagCredentialSpec to + flagVal string + // spec is the existing serviceSpec with its configs + spec *swarm.ContainerSpec + // expected is the expected value of the credential spec after the + // function. it may be nil + expected *swarm.CredentialSpec + } + + testCases := []testCase{ + { + name: "add file credential spec", + flagVal: "file://somefile", + spec: &swarm.ContainerSpec{}, + expected: &swarm.CredentialSpec{File: "somefile"}, + }, { + name: "remove a file credential spec", + flagVal: "", + spec: &swarm.ContainerSpec{ + Privileges: &swarm.Privileges{ + CredentialSpec: &swarm.CredentialSpec{ + File: "someFile", + }, + }, + }, + expected: nil, + }, { + name: "remove when no CredentialSpec exists", + flagVal: "", + spec: &swarm.ContainerSpec{}, + expected: nil, + }, { + name: "add a config credenital spec", + flagVal: "config://someConfigName", + spec: &swarm.ContainerSpec{ + Configs: []*swarm.ConfigReference{ + { + ConfigName: "someConfigName", + ConfigID: "someConfigID", + Runtime: &swarm.ConfigReferenceRuntimeTarget{}, + }, + }, + }, + expected: &swarm.CredentialSpec{ + Config: "someConfigID", + }, + }, { + name: "remove a config credential spec", + flagVal: "", + spec: &swarm.ContainerSpec{ + Privileges: &swarm.Privileges{ + CredentialSpec: &swarm.CredentialSpec{ + Config: "someConfigID", + }, + }, + }, + expected: nil, + }, { + name: "update a config credential spec", + flagVal: "config://someConfigName", + spec: &swarm.ContainerSpec{ + Configs: []*swarm.ConfigReference{ + { + ConfigName: "someConfigName", + ConfigID: "someConfigID", + Runtime: &swarm.ConfigReferenceRuntimeTarget{}, + }, + }, + Privileges: &swarm.Privileges{ + CredentialSpec: &swarm.CredentialSpec{ + Config: "someDifferentConfigID", + }, + }, + }, + expected: &swarm.CredentialSpec{ + Config: "someConfigID", + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + flags.Set(flagCredentialSpec, tc.flagVal) + + updateCredSpecConfig(flags, tc.spec) + // handle the case where tc.spec.Privileges is nil + if tc.expected == nil { + assert.Assert(t, tc.spec.Privileges == nil || tc.spec.Privileges.CredentialSpec == nil) + return + } + + assert.Assert(t, tc.spec.Privileges != nil) + assert.DeepEqual(t, tc.spec.Privileges.CredentialSpec, tc.expected) + }) + } +} From 42ec51e1ae5e0f0a84f28cbdd95cc57090651d93 Mon Sep 17 00:00:00 2001 From: Drew Erny Date: Mon, 1 Apr 2019 13:38:11 -0500 Subject: [PATCH 4/4] add support for config credentialspecs to compose Signed-off-by: Drew Erny --- cli/command/service/create.go | 3 ++ cli/compose/convert/service.go | 78 +++++++++++++++++++++++++---- cli/compose/convert/service_test.go | 61 +++++++++++++++++++--- 3 files changed, 125 insertions(+), 17 deletions(-) diff --git a/cli/command/service/create.go b/cli/command/service/create.go index ae359bd68717..4c709eb3a6f9 100644 --- a/cli/command/service/create.go +++ b/cli/command/service/create.go @@ -169,6 +169,9 @@ func setConfigs(apiClient client.ConfigAPIClient, service *swarm.ServiceSpec, op for _, config := range configs { if config.ConfigName == cs.Config { service.TaskTemplate.ContainerSpec.Privileges.CredentialSpec.Config = config.ConfigID + // we've found the right config, no need to keep iterating + // through the rest of them. + break } } } diff --git a/cli/compose/convert/service.go b/cli/compose/convert/service.go index 0fe40bf3a7bc..2da8c6dc5545 100644 --- a/cli/compose/convert/service.go +++ b/cli/compose/convert/service.go @@ -40,7 +40,7 @@ func Services( if err != nil { return nil, errors.Wrapf(err, "service %s", service.Name) } - configs, err := convertServiceConfigObjs(client, namespace, service.Configs, config.Configs) + configs, err := convertServiceConfigObjs(client, namespace, service, config.Configs) if err != nil { return nil, errors.Wrapf(err, "service %s", service.Name) } @@ -109,7 +109,9 @@ func Service( } var privileges swarm.Privileges - privileges.CredentialSpec, err = convertCredentialSpec(service.CredentialSpec) + privileges.CredentialSpec, err = convertCredentialSpec( + namespace, service.CredentialSpec, configs, + ) if err != nil { return swarm.ServiceSpec{}, err } @@ -286,11 +288,17 @@ func convertServiceSecrets( return secrs, err } +// convertServiceConfigObjs takes an API client, a namespace, a ServiceConfig, +// and a set of compose Config specs, and creates the swarm ConfigReferences +// required by the serivce. Unlike convertServiceSecrets, this takes the whole +// ServiceConfig, because some Configs may be needed as a result of other +// fields (like CredentialSpecs). +// // TODO: fix configs API so that ConfigsAPIClient is not required here func convertServiceConfigObjs( client client.ConfigAPIClient, namespace Namespace, - configs []composetypes.ServiceConfigObjConfig, + service composetypes.ServiceConfig, configSpecs map[string]composetypes.ConfigObjConfig, ) ([]*swarm.ConfigReference, error) { refs := []*swarm.ConfigReference{} @@ -302,7 +310,7 @@ func convertServiceConfigObjs( } return composetypes.FileObjectConfig(configSpec), nil } - for _, config := range configs { + for _, config := range service.Configs { obj, err := convertFileObject(namespace, composetypes.FileReferenceConfig(config), lookup) if err != nil { return nil, err @@ -315,6 +323,38 @@ func convertServiceConfigObjs( }) } + // finally, after converting all of the file objects, create any + // Runtime-type configs that are needed. these are configs that are not + // mounted into the container, but are used in some other way by the + // container runtime. Currently, this only means CredentialSpecs, but in + // the future it may be used for other fields + + // grab the CredentialSpec out of the Service + credSpec := service.CredentialSpec + // if the credSpec uses a config, then we should grab the config name, and + // create a config reference for it. A File or Registry-type CredentialSpec + // does not need this operation. + if credSpec.Config != "" { + // look up the config in the configSpecs. + obj, err := lookup(credSpec.Config) + if err != nil { + return nil, err + } + + // get the actual correct name. + name := namespace.Scope(credSpec.Config) + if obj.Name != "" { + name = obj.Name + } + + // now append a Runtime-type config. + refs = append(refs, &swarm.ConfigReference{ + ConfigName: name, + Runtime: &swarm.ConfigReferenceRuntimeTarget{}, + }) + + } + confs, err := servicecli.ParseConfigs(client, refs) if err != nil { return nil, err @@ -342,11 +382,6 @@ func convertFileObject( config composetypes.FileReferenceConfig, lookup func(key string) (composetypes.FileObjectConfig, error), ) (swarmReferenceObject, error) { - target := config.Target - if target == "" { - target = config.Source - } - obj, err := lookup(config.Source) if err != nil { return swarmReferenceObject{}, err @@ -357,6 +392,11 @@ func convertFileObject( source = obj.Name } + target := config.Target + if target == "" { + target = config.Source + } + uid := config.UID gid := config.GID if uid == "" { @@ -599,7 +639,7 @@ func convertDNSConfig(DNS []string, DNSSearch []string) (*swarm.DNSConfig, error return nil, nil } -func convertCredentialSpec(spec composetypes.CredentialSpecConfig) (*swarm.CredentialSpec, error) { +func convertCredentialSpec(namespace Namespace, spec composetypes.CredentialSpecConfig, refs []*swarm.ConfigReference) (*swarm.CredentialSpec, error) { var o []string // Config was added in API v1.40 @@ -622,5 +662,23 @@ func convertCredentialSpec(spec composetypes.CredentialSpecConfig) (*swarm.Crede return nil, errors.Errorf("invalid credential spec: cannot specify both %s, and %s", strings.Join(o[:l-1], ", "), o[l-1]) } swarmCredSpec := swarm.CredentialSpec(spec) + // if we're using a swarm Config for the credential spec, over-write it + // here with the config ID + if swarmCredSpec.Config != "" { + for _, config := range refs { + if swarmCredSpec.Config == config.ConfigName { + swarmCredSpec.Config = config.ConfigID + return &swarmCredSpec, nil + } + } + // if none of the configs match, try namespacing + for _, config := range refs { + if namespace.Scope(swarmCredSpec.Config) == config.ConfigName { + swarmCredSpec.Config = config.ConfigID + return &swarmCredSpec, nil + } + } + return nil, errors.Errorf("invalid credential spec: spec specifies config %v, but no such config can be found", swarmCredSpec.Config) + } return &swarmCredSpec, nil } diff --git a/cli/compose/convert/service_test.go b/cli/compose/convert/service_test.go index f275fb874cad..42050386672d 100644 --- a/cli/compose/convert/service_test.go +++ b/cli/compose/convert/service_test.go @@ -318,6 +318,7 @@ func TestConvertCredentialSpec(t *testing.T) { name string in composetypes.CredentialSpecConfig out *swarm.CredentialSpec + configs []*swarm.ConfigReference expectedErr string }{ { @@ -343,10 +344,41 @@ func TestConvertCredentialSpec(t *testing.T) { in: composetypes.CredentialSpecConfig{Config: "0bt9dmxjvjiqermk6xrop3ekq", File: "somefile.json", Registry: "testing"}, expectedErr: `invalid credential spec: cannot specify both "Config", "File", and "Registry"`, }, + { + name: "missing-config-reference", + in: composetypes.CredentialSpecConfig{Config: "missing"}, + expectedErr: "invalid credential spec: spec specifies config missing, but no such config can be found", + configs: []*swarm.ConfigReference{ + { + ConfigName: "someName", + ConfigID: "missing", + }, + }, + }, + { + name: "namespaced-config", + in: composetypes.CredentialSpecConfig{Config: "name"}, + configs: []*swarm.ConfigReference{ + { + ConfigName: "namespaced-config_name", + ConfigID: "someID", + }, + }, + out: &swarm.CredentialSpec{Config: "someID"}, + }, { name: "config", - in: composetypes.CredentialSpecConfig{Config: "0bt9dmxjvjiqermk6xrop3ekq"}, - out: &swarm.CredentialSpec{Config: "0bt9dmxjvjiqermk6xrop3ekq"}, + in: composetypes.CredentialSpecConfig{Config: "someName"}, + configs: []*swarm.ConfigReference{ + { + ConfigName: "someOtherName", + ConfigID: "someOtherID", + }, { + ConfigName: "someName", + ConfigID: "someID", + }, + }, + out: &swarm.CredentialSpec{Config: "someID"}, }, { name: "file", @@ -363,7 +395,8 @@ func TestConvertCredentialSpec(t *testing.T) { for _, tc := range tests { tc := tc t.Run(tc.name, func(t *testing.T) { - swarmSpec, err := convertCredentialSpec(tc.in) + namespace := NewNamespace(tc.name) + swarmSpec, err := convertCredentialSpec(namespace, tc.in, tc.configs) if tc.expectedErr != "" { assert.Error(t, err, tc.expectedErr) @@ -502,9 +535,14 @@ func TestConvertServiceSecrets(t *testing.T) { func TestConvertServiceConfigs(t *testing.T) { namespace := Namespace{name: "foo"} - configs := []composetypes.ServiceConfigObjConfig{ - {Source: "foo_config"}, - {Source: "bar_config"}, + service := composetypes.ServiceConfig{ + Configs: []composetypes.ServiceConfigObjConfig{ + {Source: "foo_config"}, + {Source: "bar_config"}, + }, + CredentialSpec: composetypes.CredentialSpecConfig{ + Config: "baz_config", + }, } configSpecs := map[string]composetypes.ConfigObjConfig{ "foo_config": { @@ -513,18 +551,23 @@ func TestConvertServiceConfigs(t *testing.T) { "bar_config": { Name: "bar_config", }, + "baz_config": { + Name: "baz_config", + }, } client := &fakeClient{ configListFunc: func(opts types.ConfigListOptions) ([]swarm.Config, error) { assert.Check(t, is.Contains(opts.Filters.Get("name"), "foo_config")) assert.Check(t, is.Contains(opts.Filters.Get("name"), "bar_config")) + assert.Check(t, is.Contains(opts.Filters.Get("name"), "baz_config")) return []swarm.Config{ {Spec: swarm.ConfigSpec{Annotations: swarm.Annotations{Name: "foo_config"}}}, {Spec: swarm.ConfigSpec{Annotations: swarm.Annotations{Name: "bar_config"}}}, + {Spec: swarm.ConfigSpec{Annotations: swarm.Annotations{Name: "baz_config"}}}, }, nil }, } - refs, err := convertServiceConfigObjs(client, namespace, configs, configSpecs) + refs, err := convertServiceConfigObjs(client, namespace, service, configSpecs) assert.NilError(t, err) expected := []*swarm.ConfigReference{ { @@ -536,6 +579,10 @@ func TestConvertServiceConfigs(t *testing.T) { Mode: 0444, }, }, + { + ConfigName: "baz_config", + Runtime: &swarm.ConfigReferenceRuntimeTarget{}, + }, { ConfigName: "foo_config", File: &swarm.ConfigReferenceFileTarget{