Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support using swarm Configs as CredentialSpecs in Services. #1781

Merged
merged 4 commits into from
Apr 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 46 additions & 8 deletions cli/command/service/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -95,14 +97,8 @@ 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
if err := setConfigs(apiClient, &service, opts); err != nil {
return err
}

if err := resolveServiceImageDigestContentTrust(dockerCli, &service); err != nil {
Expand Down Expand Up @@ -141,3 +137,45 @@ 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
thaJeztah marked this conversation as resolved.
Show resolved Hide resolved
// we've found the right config, no need to keep iterating
// through the rest of them.
break
}
}
}
}

return nil
}
271 changes: 271 additions & 0 deletions cli/command/service/create_test.go
Original file line number Diff line number Diff line change
@@ -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, "")
}
17 changes: 15 additions & 2 deletions cli/command/service/opts.go
Original file line number Diff line number Diff line change
Expand Up @@ -331,12 +331,25 @@ func (c *credentialSpecOpt) Set(value string) error {
c.source = value
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
thaJeztah marked this conversation as resolved.
Show resolved Hide resolved
// during a service update.
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
Expand Down Expand Up @@ -663,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(),
}
Expand Down
Loading