Propagate TLS Info in swarm info and node info REST endpoints by cyli · Pull Request #32875 · moby/moby

cyli · 2017-04-27T02:49:51Z

Propagate the swarm cluster and node TLS info provided by the swarm
objects into the REST API responses. In the CLI, display only
whether the nodes' TLS info matches the cluster's TLS info, or
whether the node needs cert rotation.

Signed-off-by: Ying Li ying.li@docker.com

This is useful to see whether a root rotation is in progress, and which nodes have yet to have their certs rotated.

cc @diogomonica @aaronlehmann

If there is no TLS info for a node, for instance if it's an old node, docker node inspect looks something like:

[
    {
        "ID": "vec1ht0gf5vkp26iqff2u0d8u",
        ...
        "Description": {
            ....
            "TLSInfo": {}
        },
        ...
    }
]

But will have the TLS info if it's been reported back on a newer node.

docker node ls will look something like:

$  bundles/17.06.0-dev/dynbinary-client/docker node ls
ID                            HOSTNAME            STATUS              AVAILABILITY        MANAGER STATUS      TLS STATUS
vec1ht0gf5vkp26iqff2u0d8u *   moby                Ready               Active              Leader              Ready

If the TLS certificate is signed by the current root. If it isn't, it'd way "Needs Rotation"

aaronlehmann · 2017-04-27T22:38:13Z

Can we hide the TLS STATUS column behind a flag? Or only display it if at least one node has a TLS status other than "ready"? There's a lot of information already shown, and the certificate rotation state will seldom be relevant.

diogomonica · 2017-04-28T08:23:14Z

@aaronlehmann @cyli I'm fine with not adding it to the output of node ls, and just have it on node inspect and node inspect --pretty.

@cyli to see if a rotation is in progress, maybe we could add something to docker info?

cyli · 2017-04-28T17:19:42Z

@diogomonica It seems more useful on node ls if there is a root rotation outstanding, because you can see which nodes are causing the issues. I was thinking about outputting this information under the rotation CLI progress bar itself, but if there are a lot of nodes that could be problematic to display.

Can totally hide it with a flag or only display if a root rotation was in progress, though.

@diogomonica Yes, I can add the boolean to docker info

diogomonica · 2017-04-29T17:52:10Z

@cyli totally fine with just showing that column if there is a rotation currently in progress. I assume that the json output of the node inspect will show what the current status of the node is w.r.t root rotation?

cyli · 2017-05-01T19:07:03Z

@diogomonica The JSON output shows the current CA cert trusted by the node, and the base64-encoded raw public key and raw subject of the issuer. Example in YAML format: https://github.com/moby/moby/pull/32875/files#diff-6822c0fe0233dff6af0bbadb1300458cR1756

cyli · 2017-05-02T02:19:33Z

Updated: here are some sample outputs (this is in the case of no root rotation, since we can't enable root rotation yet)

$ docker system info
...
Swarm: active
 NodeID: 6rv29rh4lg4bflx5tqq3sr99p
 Is Manager: true
 ClusterID: vsm0xtdcetezj9xbxs0iad04b
 Managers: 1
 Nodes: 1
 Orchestration:
  Task History Retention Limit: 5
 Raft:
  Snapshot Interval: 10000
  Number of Old Snapshots to Retain: 0
  Heartbeat Tick: 1
  Election Tick: 3
 Dispatcher:
  Heartbeat Period: 5 seconds
 CA Configuration:
  Expiry Duration: 3 months
 Root Rotation In Progress: false
 Node Address: 192.168.126.146
 Manager Addresses:
  192.168.126.146:2377
...

$ docker node ls
ID                            HOSTNAME            STATUS              AVAILABILITY        MANAGER STATUS
6rv29rh4lg4bflx5tqq3sr99p *   ubuntu              Ready               Active              Leader

$ docker node ls --format="{{json .}}"
{"Availability":"Active","Hostname":"ubuntu","ID":"6rv29rh4lg4bflx5tqq3sr99p","ManagerStatus":"Leader","Self":true,"Status":"Ready","TLSStatus":""}

$ docker node inspect self
[
    {
        "ID": "6rv29rh4lg4bflx5tqq3sr99p",
        ...
        "Description": {
            ...
            "TLSInfo": {
                "TrustRoot": "-----BEGIN CERTIFICATE-----\nMIIBazCCARCgAwIBAgIUOzgqU4tA2q5Yv1HnkzhSIwGyIBswCgYIKoZIzj0EAwIw\nEzERMA8GA1UEAxMIc3dhcm0tY2EwHhcNMTcwNTAyMDAyNDAwWhcNMzcwNDI3MDAy\nNDAwWjATMREwDwYDVQQDEwhzd2FybS1jYTBZMBMGByqGSM49AgEGCCqGSM49AwEH\nA0IABMbiAmET+HZyve35ujrnL2kOLBEQhFDZ5MhxAuYs96n796sFlfxTxC1lM/2g\nAh8DI34pm3JmHgZxeBPKUURJHKWjQjBAMA4GA1UdDwEB/wQEAwIBBjAPBgNVHRMB\nAf8EBTADAQH/MB0GA1UdDgQWBBS3sjTJOcXdkls6WSY2rTx1KIJueTAKBggqhkjO\nPQQDAgNJADBGAiEAoeVWkaXgSUAucQmZ3Yhmx22N/cq1EPBgYHOBZmHt0NkCIQC3\nzONcJ/+WA21OXtb+vcijpUOXtNjyHfcox0N8wsLDqQ==\n-----END CERTIFICATE-----\n",
                "CertIssuerSubject": "MBMxETAPBgNVBAMTCHN3YXJtLWNh",
                "CertIssuerPublicKey": "MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExuICYRP4dnK97fm6OucvaQ4sERCEUNnkyHEC5iz3qfv3qwWV/FPELWUz/aACHwMjfimbcmYeBnF4E8pRREkcpQ=="
            }
        },
     ...
    }
]

$ docker node inspect self --pretty
...
TLS Info:
 TrustRoot:
-----BEGIN CERTIFICATE-----
MIIBazCCARCgAwIBAgIUOzgqU4tA2q5Yv1HnkzhSIwGyIBswCgYIKoZIzj0EAwIw
EzERMA8GA1UEAxMIc3dhcm0tY2EwHhcNMTcwNTAyMDAyNDAwWhcNMzcwNDI3MDAy
NDAwWjATMREwDwYDVQQDEwhzd2FybS1jYTBZMBMGByqGSM49AgEGCCqGSM49AwEH
A0IABMbiAmET+HZyve35ujrnL2kOLBEQhFDZ5MhxAuYs96n796sFlfxTxC1lM/2g
Ah8DI34pm3JmHgZxeBPKUURJHKWjQjBAMA4GA1UdDwEB/wQEAwIBBjAPBgNVHRMB
Af8EBTADAQH/MB0GA1UdDgQWBBS3sjTJOcXdkls6WSY2rTx1KIJueTAKBggqhkjO
PQQDAgNJADBGAiEAoeVWkaXgSUAucQmZ3Yhmx22N/cq1EPBgYHOBZmHt0NkCIQC3
zONcJ/+WA21OXtb+vcijpUOXtNjyHfcox0N8wsLDqQ==
-----END CERTIFICATE-----

 Issuer Public Key:	MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExuICYRP4dnK97fm6OucvaQ4sERCEUNnkyHEC5iz3qfv3qwWV/FPELWUz/aACHwMjfimbcmYeBnF4E8pRREkcpQ==
 Issuer Subject:	MBMxETAPBgNVBAMTCHN3YXJtLWNh

aaronlehmann · 2017-05-02T02:29:09Z

Design LGTM. pinging @thaJeztah for a sanity check on the design, particularly showing the TLS STATUS column only when it is applicable (which I like, but don't think has precedent).

diogomonica · 2017-05-03T18:17:09Z

LGTM

thaJeztah · 2017-05-04T18:39:53Z

cli/command/formatter/node.go

Do we want this to be shown by default in docker node ls? Or only in docker node inspect? Discussing with @ehazlett and @cpuguy83 and we think it may not be needed; but happy to hear about that

I was thinking to show it in docker node ls, so if the root rotation is hanging, we can see which node(s) may be problematic and not able to renew. (#32993 is the root rotation CLI changes)

Should we have a more generic ERROR column for that? (thinking out loud)

Possibly - I'm not sure what other errors we should include, though. There's already an availability column, and likely the ones that are failing to renew are the down or unavailable ones, but it's possible that they went down after renewing.

The other option is to not include the TLS status here, but to include a list of nodes that need certificate renewals in the CLI in #32993 - I was just worried this list would be too long if the user had like 1000 nodes, and providing it in node ls allows the user to look at only the ones that need renewal.

@cpuguy83 @ehazlett WDYT?

(heh, didn't read the entire discussion above, and see the same point came up above)

thaJeztah · 2017-05-05T02:42:44Z

Ok, I overlooked that it's only shown in node ls if a rotation is actually in progress, so not "always"

design LGTM

cpuguy83 · 2017-05-05T10:21:56Z

I really don't like a conditional column. It makes scripting really difficult.
If we can provide the column via the formatted I think it's likely enough.

cyli · 2017-05-05T19:32:17Z

Updated:

# docker node ls
ID                            HOSTNAME            STATUS              AVAILABILITY        MANAGER STATUS
6rv29rh4lg4bflx5tqq3sr99p *   ubuntu              Ready               Active              Leader
umall6tifty5ttyx9sg6axi7b     moby  

# docker node ls --format "table {{.ID}}\t{{.Hostname}}\t{{.TLSStatus}}"
ID                          HOSTNAME            TLS STATUS
6rv29rh4lg4bflx5tqq3sr99p   ubuntu              Ready
umall6tifty5ttyx9sg6axi7b   moby                Ready

# docker node ls --format "{{json .}}"
{"Availability":"Active","Hostname":"ubuntu","ID":"6rv29rh4lg4bflx5tqq3sr99p","ManagerStatus":"Leader","Self":true,"Status":"Ready","TLSStatus":"Ready"}
{"Availability":"Active","Hostname":"moby","ID":"umall6tifty5ttyx9sg6axi7b","ManagerStatus":"","Self":false,"Status":"Ready","TLSStatus":"Ready"}

No more conditional column, even if a root rotation is in progress.

aaronlehmann · 2017-05-05T19:35:38Z

I hate to bikeshed, but IMHO it's unfortunate that a user can't see which nodes are holding up the root rotation without passing a magic incantation to node ls. I can see the issue with a variable set of columns being inconvenient for scripting, but shouldn't scripts be passing --format to avoid this problem? It seems better than requiring humans to come up with a --format argument.

cpuguy83 · 2017-05-05T19:40:13Z

@aaronlehmann How frequently do you think this would be used?
The list views should be important information at a glance, and the root rotation field would be only useful some of the time.

It's also not clear exactly how one would disable the column with --format when it's conditionally applied.

We can also always add it if people start asking for it, but we can't really take it away.

cyli · 2017-05-05T20:47:16Z

@cpuguy83 The TLS status info is only useful some of the time, but it would only be displayed during those times that it's useful (when a root rotation is in progress). You'd only need to exclude it if you really didn't want to see the TLS status even if a root rotation is going - otherwise it wouldn't appear.

Alternately, maybe we should provide a filter to filter by nodes whose TLS status is not "Ready", and we can display the info in the filtered view?

If there are nodes holding up the root rotation, it may be useful to see only the nodes that are holding things up, and not the nodes that are in good shape.

cyli · 2017-05-05T22:10:01Z

Also I assume this should be split into 2 PRs now

cyli · 2017-05-08T17:26:27Z

Hm... I think the filtering change would have to come in swarmkit because we'd have to tell whether the TLS status matches the desired cluster TLS status.

@cpuguy83 Is the concern that the user would see TLS status information during status information during a root rotation, but not want to see it?

cyli · 2017-05-08T18:47:30Z

Ok, have removed the CLI bits and put them in docker/cli#44. Does the API design look ok? If so, if this could be merged, would love to get #32993 in for 17.06 :)

cpuguy83

LGTM

cyli · 2017-05-09T20:46:23Z

Sorry to keep bothering folks :) Tests are all green - is this ok to merge or are there more API issues to address?

aaronlehmann · 2017-05-09T20:56:19Z

daemon/cluster/convert/swarm.go

Please check the error value. It's not necessary to expose it, but I think we should avoid relying on the contents of issuerInfo if the function returns an error.

aaronlehmann · 2017-05-09T20:56:53Z

Noted one thing, otherwise this looks fine. @thaJeztah: Does this PR need to go through docs-review?

thaJeztah · 2017-05-10T17:25:41Z

Looking now 👍

thaJeztah · 2017-05-10T17:40:49Z

Swagger output looks good to me;

Can you;

Update the API history to mention the new fields? https://github.com/moby/moby/blob/master/docs/api/version-history.md#v130-api-changes

After docker/cli#44 is merged, we should update some example outputs for

docker info
docker node ls (explain what the column does, and add the new option to the formatting section
same for docker node inspect

objects into the REST API responses. In the CLI, display only whether the nodes' TLS info matches the cluster's TLS info, or whether the node needs cert rotation. Signed-off-by: Ying Li <ying.li@docker.com>

cyli · 2017-05-10T21:09:31Z

@thaJeztah Updated and also opened #33148 in preparation for the CLI PR to be merged :) Thanks

thaJeztah

docs LGTM, thanks!

thaJeztah · 2017-05-10T22:35:39Z

I see @aaronlehmann's comment was addressed as well; all green, so I'll go ahead and merge 👍

cyli · 2017-05-10T22:36:35Z

@thaJeztah Thanks!!

GordonTheTurtle added the status/0-triage label Apr 27, 2017

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

cyli mentioned this pull request Apr 27, 2017

[feature] CA Root rotation moby/swarmkit#1990

Closed

10 tasks

cyli changed the title ~~WIP - Propagate TLS Info in swarm info and node info REST endpoints~~ Propagate TLS Info in swarm info and node info REST endpoints Apr 27, 2017

cyli mentioned this pull request May 3, 2017

API changes to rotate swarm root CA #32993

Merged

thaJeztah added this to the 17.06.0 milestone May 4, 2017

thaJeztah requested changes May 4, 2017

View reviewed changes

thaJeztah added status/2-code-review and removed status/1-design-review labels May 5, 2017

thaJeztah added the impact/cli label May 5, 2017

cyli mentioned this pull request May 8, 2017

Swarm cluster and node TLS info in CLI docker/cli#44

Merged

cpuguy83 approved these changes May 8, 2017

View reviewed changes

aaronlehmann reviewed May 9, 2017

View reviewed changes

thaJeztah added the impact/api label May 10, 2017

thaJeztah added the impact/changelog label May 10, 2017

Propagate the swarm cluster and node TLS info provided by the swarm

64ccced

objects into the REST API responses. In the CLI, display only whether the nodes' TLS info matches the cluster's TLS info, or whether the node needs cert rotation. Signed-off-by: Ying Li <ying.li@docker.com>

cyli mentioned this pull request May 10, 2017

Update the CLI docs to display whether a root rotation is in progress #33148

Merged

thaJeztah approved these changes May 10, 2017

View reviewed changes

thaJeztah merged commit f02a5b5 into moby:master May 10, 2017

cyli deleted the root-ca-info-in-api branch May 10, 2017 22:36

Conversation

cyli commented Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronlehmann commented Apr 27, 2017

Uh oh!

diogomonica commented Apr 28, 2017

Uh oh!

cyli commented Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

diogomonica commented Apr 29, 2017

Uh oh!

cyli commented May 1, 2017

Uh oh!

cyli commented May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronlehmann commented May 2, 2017

Uh oh!

diogomonica commented May 3, 2017

Uh oh!

thaJeztah May 4, 2017

Choose a reason for hiding this comment

Uh oh!

cyli May 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah May 5, 2017

Choose a reason for hiding this comment

Uh oh!

cyli May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah May 5, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah May 5, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented May 5, 2017

Uh oh!

cpuguy83 commented May 5, 2017

Uh oh!

cyli commented May 5, 2017

Uh oh!

aaronlehmann commented May 5, 2017

Uh oh!

cpuguy83 commented May 5, 2017

Uh oh!

cyli commented May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyli commented May 5, 2017

Uh oh!

cyli commented May 8, 2017

Uh oh!

cyli commented May 8, 2017

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

cyli commented May 9, 2017

Uh oh!

aaronlehmann May 9, 2017

Choose a reason for hiding this comment

Uh oh!

cyli May 9, 2017

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented May 9, 2017

Uh oh!

thaJeztah commented May 10, 2017

Uh oh!

thaJeztah commented May 10, 2017

Uh oh!

cyli commented May 10, 2017

Uh oh!

cyli commented Apr 27, 2017 •

edited

Loading

cyli commented Apr 28, 2017 •

edited

Loading

cyli commented May 2, 2017 •

edited

Loading

cyli May 4, 2017 •

edited

Loading

cyli May 5, 2017 •

edited

Loading

cyli commented May 5, 2017 •

edited

Loading