Patch seems fine; though VPNs are would now inconsistent with devices, which don't send the config on "down".  Note that NM doesn't block on the script so everything might be torn down by the time the script is run.  What we'd like to do here instead is create a "pre-down" event that guarantees that the config is not yet torn down and does block for a few seconds waiting for scripts.

Ah, I thought I commented on this one but it must have been a one-on-one conversation. I'm not particularly for or against the patch itself, I just don't see the need for it and we might just as well want to maintain the consistency instead.

Generally, the best way to maintain lists of records over IPC is to:

a) Send the whole list whenever it's modified. This is especially useful for small lists and/or lists that are used as a whole.

b) Send the whole list at first (beginning of communication, explicit request) and send change notifications. This is especially good for large lists and/or situations when the reciever often reacts to the sole fact that something changed.

A typical example for #b is the list of interfaces in nm-platform accessed by NetworkManager core. Or the list of connections accessed through NetworkManager API.

In the context of dispatcher scripts, the lists of nameservers/domains are usually conveyed once as part of the 'up' action. But some VPNs or devices could possibly change the lists without going down (devices do that e.g. upon a DHCP request). So while it sounds like the easiest way is to pass the former configuration with 'down' action, it isn't necessarily a good way.

I think it should be enough to just use the identifying information (e.g. the device name) to clean up *all* records to that device name, or, if you really need to know which exact records are to be removed (which would indicate bad design on the side of the unbound API), you can easily store them e.g. in /run and then you can cope with any of the possible action (pre-up/up/modify/down/pre-down/whatever).

I should also mention my motivation for this change. Currently dnssec-trigger
together with unbound is used for DNSSEC validation (on workstations).
dnssec-trigger has NM dispatcher script which uses nmcli to obtain current
DNS servers. Unfortunately dnssec-trigger relies on VPN clients to configure
unbound if user connects to VPN.

I would like to change the dnssec-trigger NM dispatcher script so it configures
unbound itself. When connecting to the VPN that provides internal domain and DNS
servers, someone/something has to add a forward zone into unbound so it is able
to resolve internal domain names. When disconnecting from VPN one has to remove
the forward zone from unbound and to do this you need to specify the zone.

Right now there is no way how to obtain the zone when disconnecting from VPN,
unless it was stored somewhere and can be looked up (by connection UUID or so).

(In reply to comment #2)
> Ah, I thought I commented on this one but it must have been a one-on-one
> conversation. I'm not particularly for or against the patch itself, I just
> don't see the need for it and we might just as well want to maintain the
> consistency instead.
> 
> Generally, the best way to maintain lists of records over IPC is to:
> 
> a) Send the whole list whenever it's modified. This is especially useful for
> small lists and/or lists that are used as a whole.
> 
> b) Send the whole list at first (beginning of communication, explicit request)
> and send change notifications. This is especially good for large lists and/or
> situations when the reciever often reacts to the sole fact that something
> changed.
> 
> A typical example for #b is the list of interfaces in nm-platform accessed by
> NetworkManager core. Or the list of connections accessed through NetworkManager
> API.
> 
> In the context of dispatcher scripts, the lists of nameservers/domains are
> usually conveyed once as part of the 'up' action. But some VPNs or devices
> could possibly change the lists without going down (devices do that e.g. upon a
> DHCP request). So while it sounds like the easiest way is to pass the former
> configuration with 'down' action, it isn't necessarily a good way.

I little bit agree, but I don't see how providing configuration of the
connection that is going down could be harmful if you know it's going down.

> I think it should be enough to just use the identifying information (e.g. the
> device name) to clean up *all* records to that device name, or, if you really
> need to know which exact records are to be removed (which would indicate bad
> design on the side of the unbound API), you can easily store them e.g. in /run
> and then you can cope with any of the possible action
> (pre-up/up/modify/down/pre-down/whatever).

I know the problem of obtaining necessary information could be solved by storing
them somewhere on the filesystem. I just find the idea of not storing any information better. If you don't agree and think that things should stay as
they are now I'll be completely fine with it and solve my problem other way.

Please let me know what will be your final decision so I can proceed with
changes in dnssec-trigger NM dispatcher script.

Thanks!

(In reply to comment #3)
> I would like to change the dnssec-trigger NM dispatcher script so it configures
> unbound itself.

I don't say it is wrong, I would just like to understand why should dnssec-trigger install a dispatcher script to configure unbound's list of DNS subtrees and their respective nameservers.

Currently, NetworkManager supports dnsmasq directly via a plugin which is part of NetworkManager. The reversed logic makes better sense to me and I'd say it is a good idea to instead package a dispatcher script with unbound.

But what exactly makes you think it would be best to package the script with dnssec-trigger. If dnssec-trigger needs to configure unbound itself, then yes, but I would still prefer to have both scripts...

1) A dispatcher script installed with unbound making NM push information directly to unbound.

2) If needed, a dispatcher script installed with dnssec-trigger which talks to dnssec-trigger, not unbound.

If you think those two scripts would be in conflict, you can make the first one check for the existence of the second one and/or a running dnssec-trigger instance and exit conditionally.

> (In reply to comment #2)
> > Ah, I thought I commented on this one but it must have been a one-on-one
> > conversation. I'm not particularly for or against the patch itself, I just
> > don't see the need for it and we might just as well want to maintain the
> > consistency instead.
> > 
> > Generally, the best way to maintain lists of records over IPC is to:
> > 
> > a) Send the whole list whenever it's modified. This is especially useful for
> > small lists and/or lists that are used as a whole.
> > 
> > b) Send the whole list at first (beginning of communication, explicit request)
> > and send change notifications. This is especially good for large lists and/or
> > situations when the reciever often reacts to the sole fact that something
> > changed.
> > 
> > A typical example for #b is the list of interfaces in nm-platform accessed by
> > NetworkManager core. Or the list of connections accessed through NetworkManager
> > API.
> > 
> > In the context of dispatcher scripts, the lists of nameservers/domains are
> > usually conveyed once as part of the 'up' action. But some VPNs or devices
> > could possibly change the lists without going down (devices do that e.g. upon a
> > DHCP request). So while it sounds like the easiest way is to pass the former
> > configuration with 'down' action, it isn't necessarily a good way.
> 
> I little bit agree, but I don't see how providing configuration of the
> connection that is going down could be harmful if you know it's going down.

It wouldn't be harmful, just inconsistent. If you use the logic I'm proposing, you can actually ignore the action [unless there's another inconsistency], and just always synchronize the list of DNS configurations for a particular interface. The link between DNS information and the interface is important anyway and we will need to store it somewhere for proper connection's DNS configuration assumption.

> > I think it should be enough to just use the identifying information (e.g. the
> > device name) to clean up *all* records to that device name, or, if you really
> > need to know which exact records are to be removed (which would indicate bad
> > design on the side of the unbound API), you can easily store them e.g. in /run
> > and then you can cope with any of the possible action
> > (pre-up/up/modify/down/pre-down/whatever).
> 
> I know the problem of obtaining necessary information could be solved by
> storing
> them somewhere on the filesystem. I just find the idea of not storing any
> information better.

In my opinion, unbound should keep this information, ideally together with the interface name (or index). For each interface name it should know the list of domains and the list of servers. I don't know whether it currently does that, but I'm sure we will want to be able to restore this information anyway.

What we need is a way to query the list of domains and the list of servers for an interface from unbound (e.g. when NetworkManager doesn't have that information. It might be also good to be able to store lifetimes for individual DNS servers and domain names.

My opinion is that a future version of unbound should be able to store and retrieve that information, as kernel does for IP configuration. Before (if at all) that is done, storing it by the unbound dispatcher script sounds like a good idea. It doesn't necessarily be stored on disk, it can be a tmpfs.

> If you don't agree and think that things should stay as
> they are now I'll be completely fine with it and solve my problem other way.

Yep, that is my personal opinion. You can still check with other developers, though.

> Please let me know what will be your final decision so I can proceed with
> changes in dnssec-trigger NM dispatcher script.

Please consult Dan Williams about the overall logic of the dispatcher scripts and DNS information. I'm also adding Dan Winship as he recently worked on DNS-related code.

I just realized that Tomáš struggles with fundamental lack of API (nmcli) regarding active configuration or active connections. It might be just as well a broader problem with NetworkManager which will complicate other things as well. Let me explain.

All DNS configuration is bound to something which I would naturally call connections. But this doesn't match the NMConnection objects in NetworkManager, as those are static. Instead, it's a set of runtime information which determine the DNS information that's actually used, including static and dynamic configuration.

In NetworkManager, this is normally consumed by a NMDevice object and its attributes and therefore doesn't apply to VPN, which is rather unfortunate, as then we will be forced to do the same for VPN. The dispatcher scripts treat VPN as something magical, so while unbound/dnssec-trigger doesn't treat VPNs differently at all, the scripts have to translate the data from VPNs and devices and then process them in the exactly same way.

In my opinion, this bug report itself is a non-issue, as it's very easy to just store the configuration somewhere so you can later unconfigure it. But there are real issues with the API (and nmcli in this case).

For each interface, on each change, the scripts will need to know:

1) a handle (interface name, uuid, connection name, whatever)
2) domain list
3) server list
4) whether the connection should be used as default DNS or not

AFAIK #1, #2 and #3 are already possible, it's just unfortunately different for VPN connections than for other connections. For VPN connections, it's AFAIK not accessible using nmcli, which is awkward. The #4 should be accessible using the connection API through the $UUID in dispatcher scripts.

The question is, which actions the scripts need to catch to always know about changes of all four attributes above and whether I forgot about something. Also, I'm curious whether nmcli and/or script variables could present the information in a more sane way so that the scripts don't need to do so much magic including differentiation between VPNs and non-VPNs, etc. Currently it looks like in many cases (DNS being an example), the differentiation is there just to make things more difficult.

In an ideal world, Tomáš's dispatcher script would look like:

#!/bin/sh
if [ "$DNS_CHANGED" ]; then
    old=/run/NetworkManager/unbound-$UUID
    new=$old.$pid

    >$new
    echo "dns-domains $DNS_DOMAINS" >>$new
    echo "dns-servers $DNS_SERVERS" >>$new
    if [ "$DEVICE"] then
        echo "device $DEVICE" >>$new
    fi
    if [ "$DNS_DEFAULT" == "yes" ]; then
        echo "default" >>$new
    fi

    process_dns_change $old $new
    mv $new $old
fi

Note that I'm ignoring $1 and $2 and just looking up a variable that tells me there's a change in DNS configuration for a connection. I don't care whether it's VPN or not, whether it's tied to some device or not, etc.

Moving to IP and DNS configuration, as it turned out to more of a general problem than just VPN.

*** Bug 603321 has been marked as a duplicate of this bug. ***

This has been AFAIK solved and dnssec-trigger python dispatcher.d script now uses the feature. Please confirm.

Yes, this is already fixed in master and dnssec-trigger script uses the feature.