Skip to content

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Nov 15, 2017

The /info endpoint currently doesn't show what DNS options are configured on the daemon. This information can be useful, for example in situations where a "local" DNS server is configured (which may not work if the IP address of the local DNS overlaps with the container's IP range).

This patch adds the DNS configuration that is set on the daemon to the output of the /info endpoint.

@fcrisciani I could use some input on this one; the problem I was trying to solve here is situations where a local DNS is used (which could be localhost / 127.0.0.1, or a DNS server in a local address range).

However this patch would only make this discoverable if that DNS is explicitly set as a daemon configuration; if no --dns option is set on the daemon, the default behaviour for containers is to bind-mount a copy of the hosts's /etc/resolv.conf into the container, so if that file contains the actual (e.g.) 127.0.0.1 DNS server, things still don't work (and are not visible in the docker info output).

Trying to parse the host's /etc/resolv.conf in order to extract the configuration sounds like a bad idea for this purpose, but perhaps you have other suggestions.

- Description for the changelog

* Add DNS information to the `/info` endpoint [moby/moby#35506](https://github.com/moby/moby/pull/35506)

- A picture of a cute animal (not mandatory but encouraged)

MemTotal int64
GenericResources []swarm.GenericResource
DockerRootDir string
DNSDefaults DNSInfo
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming this xxDefaults as I've seen there's confusion about the --dns configuration in general (i.e., this is what's used as a default for containers that are started, but it's not the DNS configuration for the daemon process itself.

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Desgin SGTM

@mlaventure
Copy link
Contributor

Design SGTM too, so does the code. Moving to code review

@mlaventure
Copy link
Contributor

ping @fcrisciani


// DNSInfo contains the default DNS configuration to use for containers.
// These values can be overridden per container.
// TODO use a "ContainerDefaults" wrapper for this instead, and deprecate top-level defaults????
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😝

Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should just have a Config: section in Info that prints the entire config instead of picking individual values out and creating new types?

We could make this a new info format for the next API version, and remove the old fields that are coming from config.

@cpuguy83
Copy link
Member

cpuguy83 commented Feb 1, 2019

Agree with @dnephin
Only maybe we can add an API endpoint to fetch the daemon config instead of bloating /info with all of the daemon config.

@cpuguy83
Copy link
Member

cpuguy83 commented Mar 3, 2020

Should we close this?

The `/info` endpoint currently doesn't show what DNS options
are configured on the daemon. This information can be
useful, for example in situations where a "local" DNS server
is configured (which may not work if the IP address of the
local DNS overlaps with the container's IP range).

This patch adds the DNS configuration that is set on the daemon
to the output of the `/info` endpoint.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the add-dns-to-docker-info branch from 07e39c0 to f744640 Compare February 22, 2022 09:13
Comment on lines +66 to +70
DNSDefaults: types.DNSInfo{
DNS: daemon.configStore.DNS,
DNSOptions: daemon.configStore.DNSOptions,
DNSSearch: daemon.configStore.DNSSearch,
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the DNS prefix on all these a little redundant? 🙈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants