-
Notifications
You must be signed in to change notification settings - Fork 18.9k
[RFC] Add DNS configuration to /info endpoint #35506
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
api/types/types.go
Outdated
| MemTotal int64 | ||
| GenericResources []swarm.GenericResource | ||
| DockerRootDir string | ||
| DNSDefaults DNSInfo |
There was a problem hiding this comment.
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.
vdemeester
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Desgin SGTM
|
Design SGTM too, so does the code. Moving to code review |
|
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???? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😝
There was a problem hiding this 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.
|
Agree with @dnephin |
|
Should we close this? |
48ec7ee to
ac61431
Compare
ac61431 to
07e39c0
Compare
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>
07e39c0 to
f744640
Compare
| DNSDefaults: types.DNSInfo{ | ||
| DNS: daemon.configStore.DNS, | ||
| DNSOptions: daemon.configStore.DNSOptions, | ||
| DNSSearch: daemon.configStore.DNSSearch, | ||
| }, |
There was a problem hiding this comment.
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? 🙈
The
/infoendpoint 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
/infoendpoint.@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
--dnsoption is set on the daemon, the default behaviour for containers is to bind-mount a copy of the hosts's/etc/resolv.confinto the container, so if that file contains the actual (e.g.)127.0.0.1DNS server, things still don't work (and are not visible in thedocker infooutput).Trying to parse the host's
/etc/resolv.confin order to extract the configuration sounds like a bad idea for this purpose, but perhaps you have other suggestions.- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)