Add credspec to compose #32393

friism · 2017-04-06T02:19:21Z

This is a follow-up PR to #32339

aluzzardi · 2017-04-06T02:40:15Z

cli/compose/types/types.go

@@ -80,6 +80,7 @@ type ServiceConfig struct {
 	CgroupParent    string   `mapstructure:"cgroup_parent"`
 	Command         ShellCommand
 	ContainerName   string   `mapstructure:"container_name"`
+	CredentialSpec  string     `mapstructure:"credential_spec"`


@friism @dnephin @diogomonica Do we want to have the raw CredentialSpec here or should this be embedded in Privileges, like in the API?

@aluzzardi we could embed, but you're never gonna use credential_spec with the selinux stuff anyway, so I don't see why.

friism · 2017-04-06T04:49:40Z

I've done a cursory test and this seems to work correctly.

I'll test end-to-end Thursday.

friism · 2017-04-06T04:59:11Z

rebased

friism · 2017-04-06T14:08:04Z

@dnephin can you take a look too? Should this go into 3.3 or 3.2?

I'm trying to fix the build.

friism · 2017-04-06T21:17:34Z

I tested this end-to-end as in #32339, and it checks out

aaronlehmann · 2017-04-07T17:44:45Z

If we are going to bump the compose file version, shouldn't we ideally add all the SELinux options that are part of #32339 instead of just CredentialSpec?

friism · 2017-04-07T17:47:20Z

I don't know what's customary. I can put it in v3.2 too.

aaronlehmann · 2017-04-07T17:47:21Z

cli/compose/convert/service.go

@@ -123,6 +131,7 @@ func convertService(
 				TTY:             service.Tty,
 				OpenStdin:       service.StdinOpen,
 				Secrets:         secrets,
+				Privileges:      &privileges,


I think it's better to leave Privileges as nil if there is nothing related to it specified in the compose file.

dnephin · 2017-04-07T17:47:48Z

cc @shin- I guess we'd need to handle this in docker-compose as well

aaronlehmann · 2017-04-07T18:10:55Z

Code LGTM. I'm still wondering if we should support the other options that are being introduced simultaneously, but I'll leave that up to @dnephin.

dnephin · 2017-04-07T18:35:26Z

cli/compose/schema/data/config_schema_v3.3.json

+          ]
+        },
+        "container_name": {"type": "string"},
+        "credential_spec": {"type": "string"},


I don't think this should be immediately in the service struct. We just went and removed a bunch of fields from the Compose format with the argument that we wanted things to be platform agnostic, so we can't go adding new ones back in.

I think we need to group these, something like this:

services: foo: ... platform: windows: credential_spec: ...

See the discussion on the other PR; although this only is used by tasks/containers running on Windows, the manager you're connected to doesn't make this distinction. Similar to it being possible to set SELinux or AppArmor options, but they won't be consumed if the node it lands on doesn't have it enabled

Yes, I have serious concerns with that design.

It's "not as bad" in the API, but from the Compose file is it s a problem.

If you specify an option in the compose file you expect it to do something. This has been a common issue reported by users. This is why we have the build and deploy sections, so it's clear when something might be ignored.

The same applies to these options. It needs to be clear from the config that the options only apply in some cases.

I believe the daemon ignores unsupported fields, so the user would never be informed that their settings are being ignored.

I think that's a separate thing (and I agree - meant to open an issue for the, e.g. if "memory-limit" is not supported by the node, you'll never be aware of that.)

In a way, it's not non-functional, as in you're able to pass the definition; we can't explain everything in the file format itself

We don't have to explain everything, just the important pieces.

Same answer as with the API:

#32339 (comment)

I hear you but I don't think that's the right move. Rather than grouping flags by functionality (e.g. Privileges), we'll end up with highly unrelated groups (e.g. tty next to selinux next to apparmor next to other stuff).

It's fine to have options that only apply to a platform, as long as it's documented.

Ultimately, flags will be supported on platforms where they are supported. You can set SELinux under Linux all you want, it's not because you are running Linux that it's going to work anyway (do you have selinux enabled in your system? the tooling installed? the daemon started with --selinux-enabled)?

Signed-off-by: Michael Friis <friism@gmail.com>

aluzzardi · 2017-04-25T17:57:50Z

Reviving this up

/cc @diogomonica

Should privileges (credentialspec, selinux) be top-level or nested under privileges?

As I explained in #32393 (comment), I don't believe in platform nesting

diogomonica · 2017-05-03T16:28:35Z

I think selinux should be under privileges, but credential spec shouldn't.

friism · 2017-05-03T16:38:52Z

@diogomonica ok, so since selinux is not part of this PR anyway, we should be good right?

diogomonica · 2017-05-03T18:21:58Z

You know my feelings about credential spec ;)

But I don't see a better way forward, so LGTM.

cpuguy83 · 2017-05-03T18:27:25Z

cli/compose/convert/service.go

+
+	credentialSpec := swarm.CredentialSpec{}
+	switch {
+	case strings.HasPrefix(source, "file://"):


Can we just follow the swarm type for this instead of string parsing?

I think it's fine to leave it as-is:

it makes compose and cli more similar

it's simpler to add additional schemes (I'm looking forward to config, for example)

I think it's (mostly) fine on the command line because having lots of flags that are basically the same is gross... however providing a strongly typed object which we are representing in the yaml format is much more desirable.

Otherwise we have to rely on the daemon to parse these strings.

thaJeztah · 2017-05-03T18:33:05Z

Looks like we're ok on design, so moving to code review, but let me know if there are strong objections

aluzzardi · 2017-05-05T23:48:29Z

LGTM

Unrelated to this PR:
@diogomonica

I think selinux should be under privileges, but credential spec shouldn't.

Do you think that's true for the API as well? I nested them under Privileges :/

diogomonica · 2017-05-08T18:25:26Z

@aluzzardi I think it's fine, but overall I would rather have credspec as a distinct thing from privileges everywhere.

mavenugo · 2017-05-10T06:03:21Z

@friism needs rebase.

dnephin · 2017-05-10T14:16:07Z

rebase wont help. This needs to be re-opened against docker/cli

thaJeztah · 2017-05-11T12:34:41Z

Carrying in docker/cli#71

GordonTheTurtle added dco/no Automatically set by a bot when one of the commits lacks proper signature status/0-triage labels Apr 6, 2017

friism force-pushed the add-credspec-to-compose branch from 0aaece9 to 9bb3685 Compare April 6, 2017 02:20

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Apr 6, 2017

friism force-pushed the add-credspec-to-compose branch from 9bb3685 to 9a99888 Compare April 6, 2017 02:21

aluzzardi reviewed Apr 6, 2017

View reviewed changes

friism force-pushed the add-credspec-to-compose branch 2 times, most recently from 240af03 to c1634bf Compare April 6, 2017 04:45

friism force-pushed the add-credspec-to-compose branch from c1634bf to 8bfc673 Compare April 6, 2017 04:58

friism force-pushed the add-credspec-to-compose branch 2 times, most recently from d9c95c6 to 6972326 Compare April 6, 2017 14:06

friism force-pushed the add-credspec-to-compose branch from 6972326 to cd12fe1 Compare April 6, 2017 18:34

friism changed the title ~~[WIP] Add credspec to compose~~ Add credspec to compose Apr 6, 2017

friism force-pushed the add-credspec-to-compose branch 2 times, most recently from c995d33 to 307fee5 Compare April 6, 2017 22:43

vdemeester added status/1-design-review and removed status/0-triage labels Apr 7, 2017

vdemeester requested review from dnephin, thaJeztah and aaronlehmann April 7, 2017 14:08

friism force-pushed the add-credspec-to-compose branch from 307fee5 to b80d3d5 Compare April 7, 2017 17:08

aaronlehmann reviewed Apr 7, 2017

View reviewed changes

friism force-pushed the add-credspec-to-compose branch from b80d3d5 to 74a9ecf Compare April 7, 2017 18:07

dnephin reviewed Apr 7, 2017

View reviewed changes

friism force-pushed the add-credspec-to-compose branch from 74a9ecf to 2df1239 Compare April 7, 2017 18:47

vieux added this to the 17.06 milestone Apr 7, 2017

add credential-spec to compose

1cd0575

Signed-off-by: Michael Friis <friism@gmail.com>

friism force-pushed the add-credspec-to-compose branch from 2df1239 to 1cd0575 Compare April 7, 2017 23:44

GordonTheTurtle assigned aluzzardi Apr 20, 2017

cpuguy83 mentioned this pull request Apr 28, 2017

Support --credential-spec in docker compose / docker stack #32053

Closed

cpuguy83 reviewed May 3, 2017

View reviewed changes

thaJeztah added status/2-code-review impact/changelog impact/cli impact/documentation and removed status/1-design-review labels May 3, 2017

thaJeztah mentioned this pull request May 11, 2017

add credential-spec to compose docker/cli#71

Merged

thaJeztah closed this May 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add credspec to compose #32393

Add credspec to compose #32393

friism commented Apr 6, 2017 •

edited

aluzzardi Apr 6, 2017

friism Apr 6, 2017

friism commented Apr 6, 2017

friism commented Apr 6, 2017

friism commented Apr 6, 2017

friism commented Apr 6, 2017

aaronlehmann commented Apr 7, 2017

friism commented Apr 7, 2017 via email •

edited

aaronlehmann Apr 7, 2017

friism Apr 7, 2017

dnephin commented Apr 7, 2017

aaronlehmann commented Apr 7, 2017

dnephin Apr 7, 2017

thaJeztah Apr 7, 2017

dnephin Apr 7, 2017

thaJeztah Apr 7, 2017

dnephin Apr 7, 2017

aluzzardi Apr 7, 2017

aluzzardi commented Apr 25, 2017

diogomonica commented May 3, 2017

friism commented May 3, 2017

diogomonica commented May 3, 2017

cpuguy83 May 3, 2017

friism May 8, 2017

cpuguy83 May 8, 2017

thaJeztah commented May 3, 2017

aluzzardi commented May 5, 2017

diogomonica commented May 8, 2017

mavenugo commented May 10, 2017

dnephin commented May 10, 2017

thaJeztah commented May 11, 2017

Add credspec to compose #32393

Add credspec to compose #32393

Conversation

friism commented Apr 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

friism commented Apr 6, 2017

friism commented Apr 6, 2017

friism commented Apr 6, 2017

friism commented Apr 6, 2017

aaronlehmann commented Apr 7, 2017

friism commented Apr 7, 2017 via email • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin commented Apr 7, 2017

aaronlehmann commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aluzzardi commented Apr 25, 2017

diogomonica commented May 3, 2017

friism commented May 3, 2017

diogomonica commented May 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented May 3, 2017

aluzzardi commented May 5, 2017

diogomonica commented May 8, 2017

mavenugo commented May 10, 2017

dnephin commented May 10, 2017

thaJeztah commented May 11, 2017

friism commented Apr 6, 2017 •

edited

friism commented Apr 7, 2017 via email •

edited