908 - Adding configs/secrets to service inspect pretty by essamhassan · Pull Request #1006 · docker/cli

essamhassan · 2018-04-15T23:46:05Z

Signed-off-by: Essam A. Hassan es.hassan187@gmail.com

- What I did
Resolving #908 showing info about configs/secrets in docker service inspect --pretty version

- How I did it
1- methods to get configs and secrets
2- adding configs/secrets to the template if they exists

- How to verify it

docker config create configtest.conf

docker secret create secrettest.conf

    docker service create -d \
    --config source=foo.conf,target=/foo.conf,uid=123,gid=456 \
    --secret source=secret.conf,target=/secret.conf,uid=234,gid=567 \
    --replicas 1 \
    --name myservice \
    nginx:alpine

Output should be

ID:		v6hlue5d5g0yhethbsxwsuvgp #other id
Name:		myservice
Configs:
 Target:	/foo.conf
  Source:	foo.conf
Secrets:
 Target:	/secret.conf
  Source:	secret.conf
Service Mode:	Replicated
 Replicas:	1
Placement:
UpdateConfig:
 Parallelism:	1
 On failure:	pause
 Monitoring Period: 5s
 Max failure ratio: 0
 Update order:      stop-first
RollbackConfig:
 Parallelism:	1
 On failure:	pause
 Monitoring Period: 5s
 Max failure ratio: 0
 Rollback order:    stop-first
ContainerSpec:
 Image:		nginx:alpine@sha256:89f15d9cbad18d54fe49acc79b24199a05b4b1c48294ce4516393bdd2bc714b2
Resources:
Endpoint Mode:	vip

- Description for the changelog

adding configs/secrets to service inspect pretty
- A picture of a cute animal (not mandatory but encouraged)

Very important cat!

thaJeztah · 2018-04-20T00:05:45Z

Thank you for working on this!

I gave this a quick try, and think we can make some changes; here's what I tried:

I created a service that uses the same secret (or config) twice, but using different target-directories in the container (also added two mounts using the same volume, and a network);

$ docker service create \
  --label foo=bar \
  --label bar=baz \
  --config src=my_config,target=/foo/config \
  --config src=my_config,target=/bar/config \
  --secret src=my_secret,target=/foo/secret \
  --secret src=my_secret,target=/bar/secret \
  --mount type=volume,src=foobar,target=/bax \
  --mount type=volume,src=foobar,target=/baz \
  --network testnet \
  --name testie \
  nginx:alpine

Inspecting that service with --pretty with this PR prints:

Secrets:
 ID:	w6is539aq4tscf2dbxnb56ui0
 Name:	my_secret
 ID:	w6is539aq4tscf2dbxnb56ui0
 Name:	my_secret

Which accurate, that probably doesn't show all the information that's useful.

In JSON format, it becomes clear that the same secret is used in different locations inside the container (and possibly other differences, such as UID/GID and Mode):

{
    "Secrets": [
    {
        "File": {
            "Name": "/foo/secret",
            "UID": "0",
            "GID": "0",
            "Mode": 292
        },
        "SecretID": "w6is539aq4tscf2dbxnb56ui0",
        "SecretName": "my_secret"
    },
    {
        "File": {
            "Name": "/bar/secret",
            "UID": "0",
            "GID": "0",
            "Mode": 292
        },
        "SecretID": "w6is539aq4tscf2dbxnb56ui0",
        "SecretName": "my_secret"
    }
    ]
}

Looking at how we present mounts (which can have the same situation: bind-mount a single host location to multiple locations inside the container);

cli/cli/command/formatter/service.go

Lines 94 to 99 in e98e95e

    
           {{- range $mount := .ContainerMounts }} 
        
             Target = {{ $mount.Target }} 
        
              Source = {{ $mount.Source }} 
        
              ReadOnly = {{ $mount.ReadOnly }} 
        
              Type = {{ $mount.Type }} 
        
           {{- end -}}

. For mounts, we use the "target" as reference, and "source" under that (indented);

The mounts of the service above would output;

Mounts:
  Target = /bax
   Source = foobar
   ReadOnly = false
   Type = volume
  Target = /baz
   Source = foobar
   ReadOnly = false
   Type = volume

For networks (in "pretty" mode), I see we're not printing the ID. So based on those, perhaps we should take a similar approach, e.g.

Secrets:
  Target:   /foo/secret
   Source:  my_secret
  Target:   /bar/secret
   Source:  my_secret

What do you think?

thaJeztah · 2018-04-20T00:06:52Z

I also noticed that Mounts is using = between field and value (but all other parts use a colon (:); we should change that (but in a separate PR)

essamhassan · 2018-04-20T21:48:00Z

thank you for reviewing the pr

concerning the format of displayed data, I actually agree that --pretty should match the json output however I noticed that there are so many trimmed data from the json output that's why I decided to go for the simpler output. If we agreed on changing this, I can push the changes and update the branch
For the mounts, its a simple change I guess I can open another pr for it
I also want to note that I think the testing for service inspect does not cover all cases, my proposal is build a factory for service at formatServiceInspect so that future tests can be flexible and not work on a static well defined state of the service

silvin-lubecki · 2018-04-24T12:12:44Z

cli/command/service/inspect_test.go

+func TestPrettyPrintWithConfigsAndSecrets(t *testing.T) {
+	s := formatServiceInspect(t, formatter.NewServiceFormat("pretty"), time.Now())
+
+	if !strings.Contains(s, "Configs:") {


You can use gotestyourself Contains assertion here:

assert.Check(t, is.Contains(s, "Configs:"), "Pretty print missing configs") assert.Check(t, is.Contains(s, "Secrets:"), "Pretty print missing secrets")

Wondering if we should use the .golden files for this @vdemeester ?

Yes, most likely we could use golden package for it 👼
Can be done in a follow-up though 😉

thaJeztah · 2018-04-24T15:22:01Z

concerning the format of displayed data, I actually agree that --pretty should match the json output however

Could you elaborate a bit on this? Open to suggestions, just not sure what you mean exactly with matching the json output.

My interpretation is that (so far) the --pretty output is to provide the most important information in a human-readable format. This may mean that lower-level details (e.g. network ID) can be omitted; that information can be obtained using the full JSON output, or using a custom template to get specific information.

essamhassan · 2018-04-24T15:42:35Z

@thaJeztah I got you and I agree with that. I'm working on the changes and will push them asap.

thaJeztah · 2018-04-24T15:49:00Z

Thanks!

essamhassan · 2018-04-24T16:19:21Z

@thaJeztah changes pushed

thaJeztah · 2018-04-25T14:27:23Z

Looking good:

ID:		x2l4mdjijqncfeebbq7zmz5tr
Name:		testie
Labels:
 bar=baz
 foo=bar
Configs:
 Target:	/bar/config
  Source:	my_config
 Target:	/foo/config
  Source:	my_config
Secrets:
 Target:	/foo/secret
  Source:	my_secret
 Target:	/bar/secret
  Source:	my_secret
Service Mode:	Replicated
 Replicas:	1
Placement:
UpdateConfig:
 Parallelism:	1
 On failure:	pause
 Monitoring Period: 5s
 Max failure ratio: 0
 Update order:      stop-first
RollbackConfig:
 Parallelism:	1
 On failure:	pause
 Monitoring Period: 5s
 Max failure ratio: 0
 Rollback order:    stop-first
ContainerSpec:
 Image:		nginx:alpine@sha256:3a44395131c5a9704417d19ab4c8d6cb104013659f5babb2f1c632e789588196
Mounts:
  Target = /bax
   Source = foobar
   ReadOnly = false
   Type = volume
  Target = /baz
   Source = foobar
   ReadOnly = false
   Type = volume
Resources:
Networks: testnet 
Endpoint Mode:	vip

thaJeztah · 2018-04-25T14:32:33Z

So, one thing (bear with me) I'm thinking while looking at the output: should we group "attached" objects, and move configs and secrets lower (near Mounts and Networks)? Perhaps Labels should be moved as well (not sure about that one though, and that should be separate from this PR); would look something like;

ID:		x2l4mdjijqncfeebbq7zmz5tr
Name:		testie
Service Mode:	Replicated
 Replicas:	1
Placement:
UpdateConfig:
 Parallelism:	1
 On failure:	pause
 Monitoring Period: 5s
 Max failure ratio: 0
 Update order:      stop-first
RollbackConfig:
 Parallelism:	1
 On failure:	pause
 Monitoring Period: 5s
 Max failure ratio: 0
 Rollback order:    stop-first
ContainerSpec:
 Image:		nginx:alpine@sha256:3a44395131c5a9704417d19ab4c8d6cb104013659f5babb2f1c632e789588196
Labels:
 bar=baz
 foo=bar
Configs:
 Target:	/bar/config
  Source:	my_config
 Target:	/foo/config
  Source:	my_config
Secrets:
 Target:	/foo/secret
  Source:	my_secret
 Target:	/bar/secret
  Source:	my_secret
Mounts:
  Target = /bax
   Source = foobar
   ReadOnly = false
   Type = volume
  Target = /baz
   Source = foobar
   ReadOnly = false
   Type = volume
Resources:
Networks: testnet 
Endpoint Mode:	vip

essamhassan · 2018-04-25T15:03:18Z

Makes sense but I'd say if we are going to follow the same order as in the json output they should be after Mounts

ID:		x2l4mdjijqncfeebbq7zmz5tr
Name:		testie
Service Mode:	Replicated
 Replicas:	1
Placement:
UpdateConfig:
 Parallelism:	1
 On failure:	pause
 Monitoring Period: 5s
 Max failure ratio: 0
 Update order:      stop-first
RollbackConfig:
 Parallelism:	1
 On failure:	pause
 Monitoring Period: 5s
 Max failure ratio: 0
 Rollback order:    stop-first
ContainerSpec:
 Image:		nginx:alpine@sha256:3a44395131c5a9704417d19ab4c8d6cb104013659f5babb2f1c632e789588196
Labels:
 bar=baz
 foo=bar
Mounts:
  Target = /bax
   Source = foobar
   ReadOnly = false
   Type = volume
  Target = /baz
   Source = foobar
   ReadOnly = false
   Type = volume
Configs:
 Target:	/bar/config
  Source:	my_config
 Target:	/foo/config
  Source:	my_config
Secrets:
 Target:	/foo/secret
  Source:	my_secret
 Target:	/bar/secret
  Source:	my_secret
Resources:
Networks: testnet 
Endpoint Mode:	vip

thaJeztah · 2018-04-25T15:09:03Z

Ah! Definitely ok with me to move after mounts

Signed-off-by: Essam A. Hassan <es.hassan187@gmail.com>

essamhassan · 2018-04-25T15:22:51Z

@thaJeztah changes pushed

thaJeztah

LGTM, thank you!

ping @vdemeester @silvin-lubecki PTAL

vdemeester

LGTM 🐯

GordonTheTurtle added the status/0-triage label Apr 15, 2018

essamhassan force-pushed the 908_show_secrets_configs_srv_inspect branch from 055fdf1 to 3fbaa46 Compare April 15, 2018 23:53

essamhassan changed the title ~~#908 adding configs/secrets to service inspect pretty~~ 908- Adding configs/secrets to service inspect pretty Apr 16, 2018

essamhassan changed the title ~~908- Adding configs/secrets to service inspect pretty~~ 908 - Adding configs/secrets to service inspect pretty Apr 16, 2018

thaJeztah added status/2-code-review and removed status/0-triage labels Apr 19, 2018

silvin-lubecki reviewed Apr 24, 2018

View reviewed changes

essamhassan force-pushed the 908_show_secrets_configs_srv_inspect branch 2 times, most recently from aba4952 to 48f4e2f Compare April 24, 2018 16:12

essamhassan force-pushed the 908_show_secrets_configs_srv_inspect branch from 48f4e2f to 8238644 Compare April 25, 2018 15:16

essamhassan added 3 commits April 25, 2018 17:21

adding configs/secrets to service inspect pretty

3ae2d4b

Signed-off-by: Essam A. Hassan <es.hassan187@gmail.com>

add config and secret to service in inspect test

2b1f27e

Signed-off-by: Essam A. Hassan <es.hassan187@gmail.com>

testing config/secrets are showing in pretty print

8238644

Signed-off-by: Essam A. Hassan <es.hassan187@gmail.com>

thaJeztah approved these changes Apr 25, 2018

View reviewed changes

thaJeztah added kind/enhancement impact/changelog labels Apr 30, 2018

vdemeester approved these changes May 3, 2018

View reviewed changes

vdemeester merged commit e9731e9 into docker:master May 3, 2018

GordonTheTurtle added this to the 18.06.0 milestone May 3, 2018

thaJeztah mentioned this pull request Dec 19, 2018

--health-start-period value isn't shown when pretty inspecting service #117

Closed

Conversation

essamhassan commented Apr 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Apr 20, 2018

Uh oh!

essamhassan commented Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silvin-lubecki Apr 24, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 24, 2018

Choose a reason for hiding this comment

Uh oh!

vdemeester Apr 24, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Apr 24, 2018

Uh oh!

essamhassan commented Apr 24, 2018

Uh oh!

thaJeztah commented Apr 24, 2018

Uh oh!

essamhassan commented Apr 24, 2018

Uh oh!

thaJeztah commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Apr 25, 2018

Uh oh!

essamhassan commented Apr 25, 2018

Uh oh!

thaJeztah commented Apr 25, 2018

Uh oh!

essamhassan commented Apr 25, 2018

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

essamhassan commented Apr 15, 2018 •

edited

Loading

thaJeztah commented Apr 20, 2018 •

edited

Loading

essamhassan commented Apr 20, 2018 •

edited

Loading

thaJeztah commented Apr 25, 2018 •

edited

Loading