plugin/bind: Bind by interface name

[m-yosefpor]

1. Why is this pull request needed and what does it do?

Bind by interface name

e.g.

.:53 {
  bind eth0
  .. whatever config here ...
}

It generally helps that Corefile to be more inclusive as the host IP does not need to be hardcoded in Corefile, or be different for each separte host (Also when host IP is changed, there is no need to update Corefile this way).

A use-case is when CoreDNS runs as a DaemonSet in host network namespace, the Corefile ConfigMap is identical for all pods running on any node. We don't have the option to listen on all interfaces. So we need to listen on a specific interface by its name, as IP varies between hosts.

2. Which issues (if any) are related?

#4219

3. Which documentation changes (if any) need to be made?

The plugin README.md has been changed.

4. Does this introduce a backward incompatible change or deprecation?

Yes it has backward compatibility.

[m-yosefpor]

Hmm I followed the instructions on https://github.com/coredns/coredns/pull/4522/checks?check_run_id=2116110526 , It seems something went wrong.

[miekg]

lgtm
can you revert the man/* updates and a little test? Bind to lo or something?

[SuperQ]

I'm not sure this is going to work as expected. What happens if the address list on an interface changes? If there are any DHCP, SLAAC, or other interface changes, they will not be picked up.

[miekg]

yep true, but if your're DNS server runs dhcp and changes the iface address you have other problems

[SuperQ]

No, that's not true, it's completely normal to allow for address changes on an interface. Even in the case of DNS servers.

As-is, this is going to cause unexpected behavior.

In order for this to be effective, there needs to be a goroutine that monitors the interface(s) for changes and updates the list.

[SuperQ]

For example, what about the case of keepalived? If you have a VIP that floats between systems, it may not exist at the time of CoreDNS startup on a keepalived managed replica. When the failover happens, CoreDNS will not pick up the change with this feature.

[miekg]

you could simple restart your nameserver when this happens. This doesn't need a fancy detect changes in process machinery. If you use the setup you described you are in 'i know what i do land', I think you will be fine then.

Documenting that this is not done is a good idea however.

[SuperQ]

"Just restart" seems like not a great idea either. Drops caches, could fail to startup due to a bad config, etc.

This kind of behavior isn't trivial.

[matthiasr]

Can the daemonset use the downward API to pass the node IP to CoreDNS?

[miekg]

[ Quoting ***@***.***> in "Re: [coredns/coredns] plugin/bind: ..." ]

"Just restart" seems like not a great idea either. Drops caches, could fail to startup due to a bad config, etc. This kind of behavior isn't trivial.

exactly that's why we should not even attempt it, binding to the interface makes your Corefile much more portable. This is a useful addition, invoking a complex setup than maintaining it doesn't work, doesn't help at all. And if you run such a setup I trust you know what you are doing anyway, so you will bind to IP and not use this.

[johnbelamaric]

To address @SuperQ 's concerns, please document that this binds to the IPs on that interface at the time the time of startup or reload (reload will happen with a SIGHUP or if the config file changes).

Also verify that this is true; i.e., that reload results in binding to the new addresses, if the addresses have changed since the initial load.

[m-yosefpor]

Thanks. I've added these to README. I will also test and verify this behavior so please don't merge this until I verified this.

[m-yosefpor]

@johnbelamaric BTW I also verified this (both on SIGHUP and with reload plugin).

[m-yosefpor]

Can the daemonset use the downward API to pass the node IP to CoreDNS?

For the mentioned daemonset use-case, in some cases (including ours) the node IP might be different than the interface which CoreDNS needs to listen on (when node have multiple interfaces)

[m-yosefpor]

No, that's not true, it's completely normal to allow for address changes on an interface. Even in the case of DNS servers.

As-is, this is going to cause unexpected behavior.

In order for this to be effective, there needs to be a goroutine that monitors the interface(s) for changes and updates the list.

@SuperQ FYI this behavior is common in other dns servers which support bind by name. E.g. Dnsmasq has this bind by interface name feature, and it has two flags --bind-interface and --bind-dynamic. The first one only takes IPs in the startup (same as here) and it cannot follow interface/ip changes overtime. The second one which added in newer versions, hooks OS events that trigger on interfaces changes, and rescans the interfaces and addresses.

So I think both cases might be useful in different scenarios. However, implementing the dynamic configuration is way more problematic and might be way harder than just watch an interface with a separate goroutine and reload (As @miekg also mentioned some unwanted behaviors of reload).

So I think it's enough to elaborate the behavior in docs, and expect users to know what they are doing. (e.g. for use-cases you mentioned). In future we can also implement dynamic bind and give the option for users to choose between these two behaviors with a parameter.

[m-yosefpor]

Oops. I really have no idea how to fix this DCO check, I signed off the commit, also signed the commit with my GPG key, reverted man/* and did the rebase stuff mentioned in the DCO check instructions, but it's still there. It seems it's because CI (coredns-auto-go-mod-tidy[bot]) commits are not signed off. hmm.

[m-yosefpor]

There are two points that needs attention:

1. The bind plugin does not work with IPv6?!

. {
    bind fe80::f816:3eff:fe04:f673
    forward . 8.8.8.8:53
}

This results in following error:

Listen: listen tcp [fe80::f816:3eff:fe04:f673]:53: bind: invalid argument

Loopback ::1 works, but other link local IPv6 addresses does not work. So now that we add all IPs (v4 and v6) of the interfaces, we receive an error for interfaces except lo.

I can change the code to only listen on IPv4 addresses of an interface name, but should I?
Why does it have this behavior? Shouldn't we fix the listener to be able to listen on these valid link local IPv6 addresses?

2. If a user specifies an interface IP and an interface name which also includes that IP, it will result in duplication address error:

. {
    bind 127.0.0.1 lo
    forward . 8.8.8.8:53
}

results in:

cannot serve dns://.:53 on 127.0.0.1 - it is already defined

Shall I change the code to remove duplication? or should be expected behavior and we shouldn't change this and only need to mention in documents?

[miekg]

cannot serve dns://.:53 on 127.0.0.1 - it is already defined

looks WAI to me. I'll check on the IPv6 thing, but that needs to be a different PR anyway (if there is something to fix)

[miekg]

works for me:

:5053 on fd7a:115c:a1e0:ab12:4843:cd96:6243:c074
CoreDNS-1.8.3
linux/amd64, go1.16.2, 
^C[INFO] SIGINT: Shutting down
% more Corefile                                                                                              ~coredns master
.:5053 {
    bind fd7a:115c:a1e0:ab12:4843:cd96:6243:c074
    forward . /etc/resolv.conf {
       max_concurrent 1000
    }
    cache 1
    log . "{type} {class} {name} {proto} {size} do={>do} buf={>bufsize} || {rcode} {>rflags} {rsize} {duration}"
}

[miekg]

[ Quoting ***@***.***> in "Re: [coredns/coredns] plugin/bind: ..." ] ***@***.*** commented on this pull request.

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ In man/coredns-sign.7: > @@ -8,9 +8,9 @@ .SH "DESCRIPTION" .PP The \fIsign\fP plugin is used to sign (see RFC 6781) zones. In this process DNSSEC resource records are -added to the zone. The signatures that sign the resource records sets have an expiration date. This -means the signing process must be repeated before this expiration data is reached. Otherwise the -zone's data will go BAD (RFC 4035, Section 5.5). The \fIsign\fP plugin takes care of this. +added. The signatures that sign the resource records sets have an expiration date, this means the I didn't change this file. man/* files are added by github-ci. The only generated man file should be man/coredns-bind file which I have changed the corresponding README. I don't know why ci changes these files for me. Maybe these are changes happened on master after I created PR and I need to rebase to HEAD of master branch.

really? The workflow runs there as well. Hmmm, that explains the non-signed commits. nmv then

[m-yosefpor]

really? The workflow runs there as well. Hmmm, that explains the non-signed commits. nmv then

Yeah. I also reverted them multiple times, but as soon as I push changes to github, the CI will add all those changes on man/* files anyway and commits them on my fork.

[m-yosefpor]

works for me:

@miekg I tested with a global unicast IPv6 address and it worked for me as well. But it doesn't work on link local IPv6 addresses (fe80::/10). I found multiple stackoverflow/github issues related to binding on IPv6 link-local (e.g. this or this ) explaining why this does not work. It seems we need to also specify scope_id in binding. So it needs changes in coredns/core and caddy parts if we want to be able to bind on link local addresses as well.

So I suggest, we add a IsLinkLocalUnicast() check and discard link-local IPv6 addresses, and specify a except for IPv6 link-local addresses in readme. WDYT?

[m-yosefpor]

works for me:

@miekg I tested with a global unicast IPv6 address and it worked for me as well. But it doesn't work on link local IPv6 addresses (fe80::/10). I found multiple stackoverflow/github issues related to binding on IPv6 link-local (e.g. this or this ) explaining why this does not work. It seems we need to also specify scope_id in binding. So it needs changes in coredns/core and caddy parts if we want to be able to bind on link local addresses as well.

So I suggest, we add a IsLinkLocalUnicast() check and discard link-local IPv6 addresses, and specify a except for IPv6 link-local addresses in readme. WDYT?

@miekg but what about link local thing? This feature does not work with interfaces with link local address (which in some distros it has by default), so using those interface names will not work. We should either discard them, or change cady code to make it work.

Shall I open a new PR to discard link-local addresses from appending to list?

[miekg]

[ Quoting ***@***.***> in "Re: [coredns/coredns] plugin/bind: ..." ]

works for me: @miekg I tested with a global unicast IPv6 address and it worked for me as well. But it doesn't work on link local IPv6 addresses (fe80::/10). I found multiple stackoverflow/github issues related to binding on IPv6 link-local (e.g. this or this ) explaining why this does not work. It seems we need to also specify scope_id in binding. So it needs changes in coredns/core and caddy parts if we want to be able to bind on link local addresses as well. So I suggest, we add a IsLinkLocalUnicast() check and discard link-local IPv6 addresses, and specify a except for IPv6 link-local addresses in readme. WDYT?

***@***.*** but what about link local thing? This feature does not work with

interfaces with link local address (which in some distros it has by default), so using those interface names will not work. We should either discard them, or change cady code to make it work.

making them work is best I guess. Let's see how that diff looks?