[libvirt] [PATCH] virNetworkDefUpdateDNSHost: Require both IP and a hostname to match
Laine Stump
laine at laine.org
Wed Nov 7 16:17:51 UTC 2018
On 11/6/18 12:10 PM, W. Trevor King wrote:
> Since fc19a0059 (network: backend functions for updating network dns
> host/srv/txt, 2012-11-12), the matching logic for various network
> components has been:
>
> 1) for HOST records, it's considered a match if the IP address or any
> of the hostnames of an existing record matches.
>
> 2) for SRV records, it's a match if all of
> domain+service+protocol+target *which have been specified* are
> matched.
>
> 3) for TXT records, there is only a single field to match - name
> (value can be the same for multiple records, and isn't considered a
> search term), so by definition there can be no ambiguous matches.
>
> But HOST records can have the same hostname for multiple records
> (similar to TXT records with the same value). The value that needs to
> be distinct for HOST records is the IP address.
You're going to force me to go dig out the decades-old DNS RFCs, aren't
you? :-P (Seriously, I think we need to go back to the original source
and make sure we're interpreting it correctly before making any changes.
I can't do that right now without losing a ton of context on other
issues in my brain though...)
(but in the meantime, if something was working before (e.g. removing an
entry by hostname alone) then that needs to continue working, otherwise
some application depending on that behavior will now be broken, and
we've made promises about that not happening with libvirt.
> This commit updates
> the matching logic to only consider the IP address. Compared to the
> previous HOST logic:
>
> 1. You can now delete entries from an existing network like:
>
> <dns>
> <host ip="192.168.1.1">
> <hostname>example</hostname>
> </host>
> <host ip="192.168.1.2">
> <hostname>example</hostname>
> </host>
> </dns>
>
> with input like:
>
> <host ip="192.168.1.1">
> </host>
>
> or:
>
> <host ip="192.168.1.1">
> <hostname>example</hostname>
> </host>
>
> Previously, only the former would work (the latter used to raise
> "multiple matching DNS HOST records were found in network").
Without thinking too much about it, that (the "multiple matching ...."
error) sounds like a bug that can/should be fixed - since both hostname
and ip find only a single record, there shouldn't be an error.
>
> 2. You can no longer remove entries by hostname alone. Previously,
> you may have been able to remove an entry from an existing network
> like:
>
> <dns>
> <host ip="192.168.1.1">
> <hostname>example-1</hostname>
> </host>
> <host ip="192.168.1.2">
> <hostname>example-2</hostname>
> </host>
> </dns>
>
> with input like:
>
> <host name="example">
> <hostname>example-1</hostname>
> </host>
>
> using the 'name' property to get through the partialOkay check in
> virNetworkDHCPHostDefParseXML. Now that input will raise "Missing
> IP address in network '%s' DNS HOST record".
That is a change in the behavior of the API where the previous behavior
was desired (i.e. not a bug), so not acceptable.
>
> 3. You can now add multiple entries with a common hostname (as long as
> they have distinct IP addresses). Previously, adding:
>
> <host ip="192.168.1.1">
> <hostname>example</hostname>
> </host>
>
> to an existing:
>
> <host ip="192.168.1.2">
> <hostname>example</hostname>
> </host>
>
> would have raised "there is already at least one DNS HOST record
> with a matching field in network".
> ---
>
> I'm actually not clear on whether the 'ip' attribute is required to be
> unique or not.
Well, *something* needs to be unique. Either one of the fields, or the
combination of some fields. If possible, decisions about that should be
based on the RFCs, and then if the backend implementation (dnsmasq in
this case) is any different, that should be treated as a different kind
of error.
> If not, maybe the logic should be:
>
> * Deletes with just an IP remove all <host> entries that match that
> IP.
> * Deletes with just a hostname remove all <hostname> entries that
> match that hostname.
> * Deletes with an IP and a hostname remove matching <hostname> entries
> from <host> entries which match the IP.
> * If <hostname> removal completely empties a <host>, the <host> is
> also removed.
>
> Thoughts?
>
> src/conf/network_conf.c | 31 ++++++++++---------------------
> 1 file changed, 10 insertions(+), 21 deletions(-)
>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 39a13b4..8ed62ac 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -587,14 +587,14 @@ virNetworkDNSHostDefParseXML(const char *networkName,
> xmlNodePtr cur;
> char *ip;
>
> - if (!(ip = virXMLPropString(node, "ip")) && !partialOkay) {
> + if (!(ip = virXMLPropString(node, "ip"))) {
> virReportError(VIR_ERR_XML_DETAIL,
> _("Missing IP address in network '%s' DNS HOST record"),
> networkName);
> goto error;
> }
>
> - if (ip && (virSocketAddrParse(&def->ip, ip, AF_UNSPEC) < 0)) {
> + if (virSocketAddrParse(&def->ip, ip, AF_UNSPEC) < 0) {
> virReportError(VIR_ERR_XML_DETAIL,
> _("Invalid IP address in network '%s' DNS HOST record"),
> networkName);
> @@ -603,6 +603,13 @@ virNetworkDNSHostDefParseXML(const char *networkName,
> }
> VIR_FREE(ip);
>
> + if (!VIR_SOCKET_ADDR_VALID(&def->ip)) {
> + virReportError(VIR_ERR_XML_DETAIL,
> + _("Invalid IP address in network '%s' DNS HOST record"),
> + networkName);
> + goto error;
> + }
> +
> cur = node->children;
> while (cur != NULL) {
> if (cur->type == XML_ELEMENT_NODE &&
> @@ -631,13 +638,6 @@ virNetworkDNSHostDefParseXML(const char *networkName,
> goto error;
> }
>
> - if (!VIR_SOCKET_ADDR_VALID(&def->ip) && def->nnames == 0) {
> - virReportError(VIR_ERR_XML_DETAIL,
> - _("Missing ip and hostname in network '%s' DNS HOST record"),
> - networkName);
> - goto error;
> - }
> -
> return 0;
>
> error:
> @@ -3334,18 +3334,7 @@ virNetworkDefUpdateDNSHost(virNetworkDefPtr def,
> goto cleanup;
>
> for (i = 0; i < dns->nhosts; i++) {
> - bool foundThisTime = false;
> -
> - if (virSocketAddrEqual(&host.ip, &dns->hosts[i].ip))
> - foundThisTime = true;
> -
> - for (j = 0; j < host.nnames && !foundThisTime; j++) {
> - for (k = 0; k < dns->hosts[i].nnames && !foundThisTime; k++) {
> - if (STREQ(host.names[j], dns->hosts[i].names[k]))
> - foundThisTime = true;
> - }
> - }
> - if (foundThisTime) {
> + if virSocketAddrEqual(&host.ip, &dns->hosts[i].ip) {
> foundCt++;
> foundIdx = i;
> }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pEpkey.asc
Type: application/pgp-keys
Size: 1757 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20181107/2a744d64/attachment-0001.bin>
More information about the libvir-list
mailing list