From 6f1d7ddfa47bc6aae85ac997cf21b2f2a3b03d69 Mon Sep 17 00:00:00 2001 From: Drew Erny Date: Thu, 7 Feb 2019 14:27:08 -0600 Subject: [PATCH] Use Runtime target The Swarmkit api specifies a target for configs called called "Runtime" which indicates that the config is not mounted into the container but has some other use. This commit updates the Docker api to reflect this. Signed-off-by: Drew Erny --- api/server/router/swarm/cluster_routes.go | 38 +----- api/server/router/swarm/helpers.go | 32 +++++ api/server/router/swarm/helpers_test.go | 87 +++++++++++++ api/swagger.yaml | 16 ++- api/types/swarm/config.go | 7 +- daemon/cluster/convert/container.go | 53 +++++--- daemon/cluster/convert/service_test.go | 144 ++++++++++++++++++++++ daemon/container_operations_unix.go | 9 +- daemon/container_operations_windows.go | 9 +- daemon/oci_windows.go | 17 ++- docs/api/version-history.md | 2 + 11 files changed, 353 insertions(+), 61 deletions(-) create mode 100644 api/server/router/swarm/helpers_test.go diff --git a/api/server/router/swarm/cluster_routes.go b/api/server/router/swarm/cluster_routes.go index 0966d3a5a4..ef4157bd8a 100644 --- a/api/server/router/swarm/cluster_routes.go +++ b/api/server/router/swarm/cluster_routes.go @@ -213,24 +213,7 @@ func (sr *swarmRouter) createService(ctx context.Context, w http.ResponseWriter, if versions.LessThan(cliVersion, "1.30") { queryRegistry = true } - if versions.LessThan(cliVersion, "1.40") { - if service.TaskTemplate.ContainerSpec != nil { - // Sysctls for docker swarm services weren't supported before - // API version 1.40 - service.TaskTemplate.ContainerSpec.Sysctls = nil - - if service.TaskTemplate.ContainerSpec.Privileges != nil && service.TaskTemplate.ContainerSpec.Privileges.CredentialSpec != nil { - // Support for setting credential-spec through configs was added in API 1.40 - service.TaskTemplate.ContainerSpec.Privileges.CredentialSpec.Config = "" - } - } - - if service.TaskTemplate.Placement != nil { - // MaxReplicas for docker swarm services weren't supported before - // API version 1.40 - service.TaskTemplate.Placement.MaxReplicas = 0 - } - } + adjustForAPIVersion(cliVersion, &service) } resp, err := sr.backend.CreateService(service, encodedAuth, queryRegistry) @@ -270,24 +253,7 @@ func (sr *swarmRouter) updateService(ctx context.Context, w http.ResponseWriter, if versions.LessThan(cliVersion, "1.30") { queryRegistry = true } - if versions.LessThan(cliVersion, "1.40") { - if service.TaskTemplate.ContainerSpec != nil { - // Sysctls for docker swarm services weren't supported before - // API version 1.40 - service.TaskTemplate.ContainerSpec.Sysctls = nil - - if service.TaskTemplate.ContainerSpec.Privileges != nil && service.TaskTemplate.ContainerSpec.Privileges.CredentialSpec != nil { - // Support for setting credential-spec through configs was added in API 1.40 - service.TaskTemplate.ContainerSpec.Privileges.CredentialSpec.Config = "" - } - } - - if service.TaskTemplate.Placement != nil { - // MaxReplicas for docker swarm services weren't supported before - // API version 1.40 - service.TaskTemplate.Placement.MaxReplicas = 0 - } - } + adjustForAPIVersion(cliVersion, &service) } resp, err := sr.backend.UpdateService(vars["id"], version, service, flags, queryRegistry) diff --git a/api/server/router/swarm/helpers.go b/api/server/router/swarm/helpers.go index 1f57074f92..3120e8f8f2 100644 --- a/api/server/router/swarm/helpers.go +++ b/api/server/router/swarm/helpers.go @@ -9,6 +9,8 @@ import ( "github.com/docker/docker/api/server/httputils" basictypes "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/backend" + "github.com/docker/docker/api/types/swarm" + "github.com/docker/docker/api/types/versions" ) // swarmLogs takes an http response, request, and selector, and writes the logs @@ -64,3 +66,33 @@ func (sr *swarmRouter) swarmLogs(ctx context.Context, w io.Writer, r *http.Reque httputils.WriteLogStream(ctx, w, msgs, logsConfig, !tty) return nil } + +// adjustForAPIVersion takes a version and service spec and removes fields to +// make the spec compatible with the specified version. +func adjustForAPIVersion(cliVersion string, service *swarm.ServiceSpec) { + if cliVersion == "" { + return + } + if versions.LessThan(cliVersion, "1.40") { + if service.TaskTemplate.ContainerSpec != nil { + // Sysctls for docker swarm services weren't supported before + // API version 1.40 + service.TaskTemplate.ContainerSpec.Sysctls = nil + + if service.TaskTemplate.ContainerSpec.Privileges != nil && service.TaskTemplate.ContainerSpec.Privileges.CredentialSpec != nil { + // Support for setting credential-spec through configs was added in API 1.40 + service.TaskTemplate.ContainerSpec.Privileges.CredentialSpec.Config = "" + } + for _, config := range service.TaskTemplate.ContainerSpec.Configs { + // support for the Runtime target was added in API 1.40 + config.Runtime = nil + } + } + + if service.TaskTemplate.Placement != nil { + // MaxReplicas for docker swarm services weren't supported before + // API version 1.40 + service.TaskTemplate.Placement.MaxReplicas = 0 + } + } +} diff --git a/api/server/router/swarm/helpers_test.go b/api/server/router/swarm/helpers_test.go new file mode 100644 index 0000000000..08764e5a17 --- /dev/null +++ b/api/server/router/swarm/helpers_test.go @@ -0,0 +1,87 @@ +package swarm // import "github.com/docker/docker/api/server/router/swarm" + +import ( + "reflect" + "testing" + + "github.com/docker/docker/api/types/swarm" +) + +func TestAdjustForAPIVersion(t *testing.T) { + var ( + expectedSysctls = map[string]string{"foo": "bar"} + ) + // testing the negative -- does this leave everything else alone? -- is + // prohibitively time-consuming to write, because it would need an object + // with literally every field filled in. + spec := &swarm.ServiceSpec{ + TaskTemplate: swarm.TaskSpec{ + ContainerSpec: &swarm.ContainerSpec{ + Sysctls: expectedSysctls, + Privileges: &swarm.Privileges{ + CredentialSpec: &swarm.CredentialSpec{ + Config: "someconfig", + }, + }, + Configs: []*swarm.ConfigReference{ + { + File: &swarm.ConfigReferenceFileTarget{ + Name: "foo", + UID: "bar", + GID: "baz", + }, + ConfigID: "configFile", + ConfigName: "configFile", + }, + { + Runtime: &swarm.ConfigReferenceRuntimeTarget{}, + ConfigID: "configRuntime", + ConfigName: "configRuntime", + }, + }, + }, + Placement: &swarm.Placement{ + MaxReplicas: 222, + }, + }, + } + + // first, does calling this with a later version correctly NOT strip + // fields? do the later version first, so we can reuse this spec in the + // next test. + adjustForAPIVersion("1.40", spec) + if !reflect.DeepEqual(spec.TaskTemplate.ContainerSpec.Sysctls, expectedSysctls) { + t.Error("Sysctls was stripped from spec") + } + + if spec.TaskTemplate.ContainerSpec.Privileges.CredentialSpec.Config != "someconfig" { + t.Error("CredentialSpec.Config field was stripped from spec") + } + + if spec.TaskTemplate.ContainerSpec.Configs[1].Runtime == nil { + t.Error("ConfigReferenceRuntimeTarget was stripped from spec") + } + + if spec.TaskTemplate.Placement.MaxReplicas != 222 { + t.Error("MaxReplicas was stripped from spec") + } + + // next, does calling this with an earlier version correctly strip fields? + adjustForAPIVersion("1.29", spec) + if spec.TaskTemplate.ContainerSpec.Sysctls != nil { + t.Error("Sysctls was not stripped from spec") + } + + if spec.TaskTemplate.ContainerSpec.Privileges.CredentialSpec.Config != "" { + t.Error("CredentialSpec.Config field was not stripped from spec") + } + + if spec.TaskTemplate.ContainerSpec.Configs[1].Runtime != nil { + t.Error("ConfigReferenceRuntimeTarget was not stripped from spec") + } + + if spec.TaskTemplate.Placement.MaxReplicas != 0 { + t.Error("MaxReplicas was not stripped from spec") + } + +} diff --git a/api/swagger.yaml b/api/swagger.yaml index 124494b364..514077ff7f 100644 --- a/api/swagger.yaml +++ b/api/swagger.yaml @@ -2628,6 +2628,7 @@ definitions: example: "0bt9dmxjvjiqermk6xrop3ekq" description: | Load credential spec from a Swarm Config with the given ID. + The specified config must also be present in the Configs field with the Runtime property set.


@@ -2768,7 +2769,12 @@ definitions: type: "object" properties: File: - description: "File represents a specific target that is backed by a file." + description: | + File represents a specific target that is backed by a file. + +


+ + > **Note**: `Configs.File` and `Configs.Runtime` are mutually exclusive type: "object" properties: Name: @@ -2784,6 +2790,14 @@ definitions: description: "Mode represents the FileMode of the file." type: "integer" format: "uint32" + Runtime: + description: | + Runtime represents a target that is not mounted into the container but is used by the task + +


+ + > **Note**: `Configs.File` and `Configs.Runtime` are mutually exclusive + type: "object" ConfigID: description: "ConfigID represents the ID of the specific config that we're referencing." type: "string" diff --git a/api/types/swarm/config.go b/api/types/swarm/config.go index a1555cf43e..16202ccce6 100644 --- a/api/types/swarm/config.go +++ b/api/types/swarm/config.go @@ -27,9 +27,14 @@ type ConfigReferenceFileTarget struct { Mode os.FileMode } +// ConfigReferenceRuntimeTarget is a target for a config specifying that it +// isn't mounted into the container but instead has some other purpose. +type ConfigReferenceRuntimeTarget struct{} + // ConfigReference is a reference to a config in swarm type ConfigReference struct { - File *ConfigReferenceFileTarget + File *ConfigReferenceFileTarget `json:",omitempty"` + Runtime *ConfigReferenceRuntimeTarget `json:",omitempty"` ConfigID string ConfigName string } diff --git a/daemon/cluster/convert/container.go b/daemon/cluster/convert/container.go index 52e2d0ec61..c3eb28df54 100644 --- a/daemon/cluster/convert/container.go +++ b/daemon/cluster/convert/container.go @@ -178,14 +178,26 @@ func secretReferencesFromGRPC(sr []*swarmapi.SecretReference) []*types.SecretRef return refs } -func configReferencesToGRPC(sr []*types.ConfigReference) []*swarmapi.ConfigReference { +func configReferencesToGRPC(sr []*types.ConfigReference) ([]*swarmapi.ConfigReference, error) { refs := make([]*swarmapi.ConfigReference, 0, len(sr)) for _, s := range sr { ref := &swarmapi.ConfigReference{ ConfigID: s.ConfigID, ConfigName: s.ConfigName, } - if s.File != nil { + switch { + case s.Runtime == nil && s.File == nil: + return nil, errors.New("either File or Runtime should be set") + case s.Runtime != nil && s.File != nil: + return nil, errors.New("cannot specify both File and Runtime") + case s.Runtime != nil: + // Runtime target was added in API v1.40 and takes precedence over + // File target. However, File and Runtime targets are mutually exclusive, + // so we should never have both. + ref.Target = &swarmapi.ConfigReference_Runtime{ + Runtime: &swarmapi.RuntimeTarget{}, + } + case s.File != nil: ref.Target = &swarmapi.ConfigReference_File{ File: &swarmapi.FileTarget{ Name: s.File.Name, @@ -199,28 +211,32 @@ func configReferencesToGRPC(sr []*types.ConfigReference) []*swarmapi.ConfigRefer refs = append(refs, ref) } - return refs + return refs, nil } func configReferencesFromGRPC(sr []*swarmapi.ConfigReference) []*types.ConfigReference { refs := make([]*types.ConfigReference, 0, len(sr)) for _, s := range sr { - target := s.GetFile() - if target == nil { - // not a file target - logrus.Warnf("config target not a file: config=%s", s.ConfigID) - continue + + r := &types.ConfigReference{ + ConfigID: s.ConfigID, + ConfigName: s.ConfigName, } - refs = append(refs, &types.ConfigReference{ - File: &types.ConfigReferenceFileTarget{ + if target := s.GetRuntime(); target != nil { + r.Runtime = &types.ConfigReferenceRuntimeTarget{} + } else if target := s.GetFile(); target != nil { + r.File = &types.ConfigReferenceFileTarget{ Name: target.Name, UID: target.UID, GID: target.GID, Mode: target.Mode, - }, - ConfigID: s.ConfigID, - ConfigName: s.ConfigName, - }) + } + } else { + // not a file target + logrus.Warnf("config target not known: config=%s", s.ConfigID) + continue + } + refs = append(refs, r) } return refs @@ -243,7 +259,6 @@ func containerToGRPC(c *types.ContainerSpec) (*swarmapi.ContainerSpec, error) { ReadOnly: c.ReadOnly, Hosts: c.Hosts, Secrets: secretReferencesToGRPC(c.Secrets), - Configs: configReferencesToGRPC(c.Configs), Isolation: isolationToGRPC(c.Isolation), Init: initToGRPC(c.Init), Sysctls: c.Sysctls, @@ -284,6 +299,14 @@ func containerToGRPC(c *types.ContainerSpec) (*swarmapi.ContainerSpec, error) { } } + if c.Configs != nil { + configs, err := configReferencesToGRPC(c.Configs) + if err != nil { + return nil, errors.Wrap(err, "invalid Config") + } + containerSpec.Configs = configs + } + // Mounts for _, m := range c.Mounts { mount := swarmapi.Mount{ diff --git a/daemon/cluster/convert/service_test.go b/daemon/cluster/convert/service_test.go index a989f391b9..9c4284bd71 100644 --- a/daemon/cluster/convert/service_test.go +++ b/daemon/cluster/convert/service_test.go @@ -467,3 +467,147 @@ func TestTaskConvertFromGRPCNetworkAttachment(t *testing.T) { t.Fatalf("expected Runtime to be %v", swarmtypes.RuntimeNetworkAttachment) } } + +// TestServiceConvertFromGRPCConfigs tests that converting config references +// from GRPC is correct +func TestServiceConvertFromGRPCConfigs(t *testing.T) { + cases := []struct { + name string + from *swarmapi.ConfigReference + to *swarmtypes.ConfigReference + }{ + { + name: "file", + from: &swarmapi.ConfigReference{ + ConfigID: "configFile", + ConfigName: "configFile", + Target: &swarmapi.ConfigReference_File{ + // skip mode, if everything else here works mode will too. otherwise we'd need to import os. + File: &swarmapi.FileTarget{Name: "foo", UID: "bar", GID: "baz"}, + }, + }, + to: &swarmtypes.ConfigReference{ + ConfigID: "configFile", + ConfigName: "configFile", + File: &swarmtypes.ConfigReferenceFileTarget{Name: "foo", UID: "bar", GID: "baz"}, + }, + }, + { + name: "runtime", + from: &swarmapi.ConfigReference{ + ConfigID: "configRuntime", + ConfigName: "configRuntime", + Target: &swarmapi.ConfigReference_Runtime{Runtime: &swarmapi.RuntimeTarget{}}, + }, + to: &swarmtypes.ConfigReference{ + ConfigID: "configRuntime", + ConfigName: "configRuntime", + Runtime: &swarmtypes.ConfigReferenceRuntimeTarget{}, + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + grpcService := swarmapi.Service{ + Spec: swarmapi.ServiceSpec{ + Task: swarmapi.TaskSpec{ + Runtime: &swarmapi.TaskSpec_Container{ + Container: &swarmapi.ContainerSpec{ + Configs: []*swarmapi.ConfigReference{tc.from}, + }, + }, + }, + }, + } + + engineService, err := ServiceFromGRPC(grpcService) + assert.NilError(t, err) + assert.DeepEqual(t, + engineService.Spec.TaskTemplate.ContainerSpec.Configs[0], + tc.to, + ) + }) + } +} + +// TestServiceConvertToGRPCConfigs tests that converting config references to +// GRPC is correct +func TestServiceConvertToGRPCConfigs(t *testing.T) { + cases := []struct { + name string + from *swarmtypes.ConfigReference + to *swarmapi.ConfigReference + expectedErr string + }{ + { + name: "file", + from: &swarmtypes.ConfigReference{ + ConfigID: "configFile", + ConfigName: "configFile", + File: &swarmtypes.ConfigReferenceFileTarget{Name: "foo", UID: "bar", GID: "baz"}, + }, + to: &swarmapi.ConfigReference{ + ConfigID: "configFile", + ConfigName: "configFile", + Target: &swarmapi.ConfigReference_File{ + // skip mode, if everything else here works mode will too. otherwise we'd need to import os. + File: &swarmapi.FileTarget{Name: "foo", UID: "bar", GID: "baz"}, + }, + }, + }, + { + name: "runtime", + from: &swarmtypes.ConfigReference{ + ConfigID: "configRuntime", + ConfigName: "configRuntime", + Runtime: &swarmtypes.ConfigReferenceRuntimeTarget{}, + }, + to: &swarmapi.ConfigReference{ + ConfigID: "configRuntime", + ConfigName: "configRuntime", + Target: &swarmapi.ConfigReference_Runtime{Runtime: &swarmapi.RuntimeTarget{}}, + }, + }, + { + name: "file and runtime", + from: &swarmtypes.ConfigReference{ + ConfigID: "fileAndRuntime", + ConfigName: "fileAndRuntime", + File: &swarmtypes.ConfigReferenceFileTarget{}, + Runtime: &swarmtypes.ConfigReferenceRuntimeTarget{}, + }, + expectedErr: "invalid Config: cannot specify both File and Runtime", + }, + { + name: "none", + from: &swarmtypes.ConfigReference{ + ConfigID: "none", + ConfigName: "none", + }, + expectedErr: "invalid Config: either File or Runtime should be set", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + engineServiceSpec := swarmtypes.ServiceSpec{ + TaskTemplate: swarmtypes.TaskSpec{ + ContainerSpec: &swarmtypes.ContainerSpec{ + Configs: []*swarmtypes.ConfigReference{tc.from}, + }, + }, + } + + grpcServiceSpec, err := ServiceSpecToGRPC(engineServiceSpec) + if tc.expectedErr != "" { + assert.Error(t, err, tc.expectedErr) + return + } + + assert.NilError(t, err) + taskRuntime := grpcServiceSpec.Task.Runtime.(*swarmapi.TaskSpec_Container) + assert.DeepEqual(t, taskRuntime.Container.Configs[0], tc.to) + }) + } +} diff --git a/daemon/container_operations_unix.go b/daemon/container_operations_unix.go index 5552d09df3..c7984f7a07 100644 --- a/daemon/container_operations_unix.go +++ b/daemon/container_operations_unix.go @@ -230,7 +230,14 @@ func (daemon *Daemon) setupSecretDir(c *container.Container) (setupErr error) { for _, ref := range c.ConfigReferences { // TODO (ehazlett): use type switch when more are supported if ref.File == nil { - logrus.Error("config target type is not a file target") + // Runtime configs are not mounted into the container, but they're + // a valid type of config so we should not error when we encounter + // one. + if ref.Runtime == nil { + logrus.Error("config target type is not a file or runtime target") + } + // However, in any case, this isn't a file config, so we have no + // further work to do continue } diff --git a/daemon/container_operations_windows.go b/daemon/container_operations_windows.go index 10bfd53d6e..c2381e6431 100644 --- a/daemon/container_operations_windows.go +++ b/daemon/container_operations_windows.go @@ -44,7 +44,14 @@ func (daemon *Daemon) setupConfigDir(c *container.Container) (setupErr error) { for _, configRef := range c.ConfigReferences { // TODO (ehazlett): use type switch when more are supported if configRef.File == nil { - logrus.Error("config target type is not a file target") + // Runtime configs are not mounted into the container, but they're + // a valid type of config so we should not error when we encounter + // one. + if configRef.Runtime == nil { + logrus.Error("config target type is not a file or runtime target") + } + // However, in any case, this isn't a file config, so we have no + // further work to do continue } diff --git a/daemon/oci_windows.go b/daemon/oci_windows.go index 11868ba3a0..215d5b6d3a 100644 --- a/daemon/oci_windows.go +++ b/daemon/oci_windows.go @@ -289,18 +289,23 @@ func (daemon *Daemon) createSpecWindowsFields(c *container.Container, s *specs.S return err } } else if match, csValue = getCredentialSpec("config://", splitsOpt[1]); match { + // if the container does not have a DependencyStore, then it + // isn't swarmkit managed. In order to avoid creating any + // impression that `config://` is a valid API, return the same + // error as if you'd passed any other random word. + if c.DependencyStore == nil { + return fmt.Errorf("invalid credential spec security option - value must be prefixed file:// or registry:// followed by a value") + } + + // after this point, we can return regular swarmkit-relevant + // errors, because we'll know this container is managed. if csValue == "" { return fmt.Errorf("no value supplied for config:// credential spec security option") } - // if the container does not have a DependencyStore, then we - // return an error - if c.DependencyStore == nil { - return fmt.Errorf("cannot use config:// credential spec security option if not swarmkit managed") - } csConfig, err := c.DependencyStore.Configs().Get(csValue) if err != nil { - return fmt.Errorf("error getting value from config store: %v", err) + return errors.Wrap(err, "error getting value from config store") } // stuff the resulting secret data into a string to use as the // CredentialSpec diff --git a/docs/api/version-history.md b/docs/api/version-history.md index b7bb466f12..409b94c8c4 100644 --- a/docs/api/version-history.md +++ b/docs/api/version-history.md @@ -31,6 +31,8 @@ keywords: "API, Docker, rcli, REST, documentation" * `POST /services/{id}/update` now accepts `Sysctls` as part of the `ContainerSpec`. * `POST /services/create` now accepts `Config` as part of `ContainerSpec.Privileges.CredentialSpec`. * `POST /services/{id}/update` now accepts `Config` as part of `ContainerSpec.Privileges.CredentialSpec`. +* `POST /services/create` now includes `Runtime` as an option in `ContainerSpec.Configs` +* `POST /services/{id}/update` now includes `Runtime` as an option in `ContainerSpec.Configs` * `GET /tasks` now returns `Sysctls` as part of the `ContainerSpec`. * `GET /tasks/{id}` now returns `Sysctls` as part of the `ContainerSpec`. * `GET /nodes` now supports a filter type `node.label` filter to filter nodes based