Add insecure registries to docker info

[tkopczynski]

Adding additional section to docker info: Insecure registries.
I think we don't need to make any additional changes in engine-api but let me know if I missed something.

Also, I haven't written any test because I haven't found any for docker info command. If you think it should be covered somehow, I will add it to this PR.

Fixes #20236

Signed-off-by: Tomasz Kopczynski tomek@kopczynski.net.pl

[aaronlehmann]

I think this needs to check for Secure == false.

[calavera]

I think it would make more sense to have a "Registries" section in general with information of all registries, not only insecure ones.

Please, add tests to https://github.com/docker/docker/blob/master/integration-cli/docker_cli_info_test.go

[tkopczynski]

@aaronlehmann Added additional Secure == false condition checking

@calavera What exactly do you mean by all registries? Those defined in config.json as well?

[tkopczynski]

I've added a test for that. This way the base issue for this PR is covered.

But what about this proposition of extending docker info to all registries? Personally, I think it would be useful and I will be happy to extend this PR with it but could you first guide me a little bit with this idea? Are we talking about those registries defined in config.json?

ping @calavera @thaJeztah

[thaJeztah]

Could be useful, not entirely sure what @calavera had in mind how to present it

ping @calavera can you show an example how you think it should be shown?

[calavera]

Yes, I'm talking about showing all registries configured, doing something like this (or similar):

+       fmt.Fprintln(cli.out, "Registries:")
+       for _, registry := range info.RegistryConfig.IndexConfigs {
+                              label := "secure registry"
+                              if !registry.Secure {
+                                  label = "insecure registry"
+                              }
+               fmt.Fprintf(cli.out, " %s: %s\n", registry.Name, label)
+       }

[tkopczynski]

Thanks @calavera , now I get it.
I agree that this would make it more useful, so I'll update this PR tomorrow.

[tkopczynski]

@calavera @thaJeztah
I've just updated the PR with @calavera's suggestions. wdyt?

[thaJeztah]

Design LGTM. Can you also update the documentation, and update the example output in https://github.com/docker/docker/blob/master/docs/reference/commandline/info.md and https://github.com/docker/docker/blob/master/man/docker-info.1.md ?

Moving this to code review

[thaJeztah]

ping @calavera @aaronlehmann for review

[tkopczynski]

@thaJeztah I've added example output to those files you mentioned - hope that's all the related docs ;)

[aaronlehmann]

I'm not sure I like the concept of exposing this as a list of configured registries instead of a list of insecure registries. I prefer the original approach.

The problem is that the only way to add something to the list is to use --insecure-registry. So in practice, you'll see docker.io (secure) and zero or more "insecure" entries. If anyone has a TLS-capable private registry, it won't show up under "Registries", and I think that would be confusing.

IndexConfigs is essentially a list of insecure registries plus docker.io, and I don't think it should be exposed directly to the user, especially under the label "Registries".

[thaJeztah]

hm good point 😢 @calavera ^^

[tkopczynski]

Hey @calavera @thaJeztah
Any updates on which version do you prefer?

[thaJeztah]

Based on @aaronlehmann's comment I think we need to change back to the original version; showing an incomplete list of "secure" registries isn't useful, so if we cannot show all registries, then I think only showing the insecure ones is probably best.

At that point, I'm wondering if we should have a feature that just shows the active configuration, e.g. docker (daemon) show-config which may be more useful.

@calavera wdyt?

[aaronlehmann]

ping @calavera

[tkopczynski]

Hey @calavera @thaJeztah @aaronlehmann
anything new on this PR ?

[calavera]

showing an incomplete list of "secure" registries isn't useful, so if we cannot show all registries, then I think only showing the insecure ones is probably best

sounds good to me

At that point, I'm wondering if we should have a feature that just shows the active configuration

We can make it part of the Info output, we already mix client info with server info.

[tkopczynski]

OK, so I've updated this PR to the version that seems to be acceptable (only insecure registries).

As of listing the active configuration of the daemon (and #21559): I can do it as well but we would need to talk about what exactly needs to be printed and under which command.

ping @thaJeztah @calavera @aaronlehmann

[aaronlehmann]

Minor cosmetic nit: I think this would be simpler as fmt.Fprintln(cli.out, " "+registry.Name). This is mostly personal preference - feel free to ignore.

[tkopczynski]

If it's not a must, I'd prefer my version.

[aaronlehmann]

Thanks for your patience. The latest version of the PR looks correct to me. I noted a few minor cosmetic issues about the use of printf formatting.

[thaJeztah]

@aaronlehmann PR was updated, ptal

[tkopczynski]

Hey @aaronlehmann

Thanks for your patience.

No problem, I'm glad it's going to be merged eventually.

The latest version of the PR looks correct to me. I noted a few minor cosmetic issues about the use of printf formatting.

I've corrected those with the exception of the first one which (as I understand) is not a blocker.

[aaronlehmann]

LGTM

[tkopczynski]

@thaJeztah that was really fast :)

[thaJeztah]

LGTM

[tonistiigi]

LGTM

[thaJeztah]

merging. thanks so much @tkopczynski, also for being patient during the bike-shedding 👍

[tkopczynski]

@thaJeztah It's fine, great that it's in master finally 😄

[jmMeessen]

@tkopczynski Thank you for the PR. (I was the original requester)