On Wed, Jul 10, 2024 at 10:30:31AM +0200, Michal Prívozník wrote:
On 7/9/24 17:23, Adam Julis wrote:The "modify" command allows to replace an existing Srv record (some of its elements respectively: port, priority and weight). The primary key used to choose the modify record is the remaining parameters, only one of them is required. Not using some of these parameters may cause duplicate records and error message. This logic is there because of the previous implementation (Add and Delete options) in the function. Tests in networkxml2xmlupdatetest.c contain replacements of an existing DNS-Srv record and failure due to non-existing record. Resolves: https://gitlab.com/libvirt/libvirt/-/issues/639 Signed-off-by: Adam Julis <ajulis@xxxxxxxxxx> --- src/conf/network_conf.c | 27 ++++++++++++++----- .../srv-not-existing.xml | 1 + .../srv-record-modify-few.xml | 1 + .../nat-network-dns-srv-modify-few.xml | 26 ++++++++++++++++++ tests/networkxml2xmlupdatetest.c | 10 ++++++- 5 files changed, 58 insertions(+), 7 deletions(-) create mode 100644 tests/networkxml2xmlupdatein/srv-not-existing.xml create mode 100644 tests/networkxml2xmlupdatein/srv-record-modify-few.xml create mode 100644 tests/networkxml2xmlupdateout/nat-network-dns-srv-modify-few.xmlReviewed-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
I would urge against this for a few reasons. The already-existing logic for finding records (for delete or duplicates during add) is already flawed, and by adding "modify" this makes it even more complicated. The explanation might be a bit long, so bear with me. Consider this: # virsh net-update default add dns-srv '<srv service="asdf" protocol="tcp"/>' Updated network default live state # virsh net-update default add dns-srv '<srv service="asdf" protocol="tcp" domain="libvirt.org"/>' Updated network default live state The above works (it's the same without `target`), because the SRV record is being added for two different domains. One for the default domain of the network and one for libvirt.org. Let's try another service, but in different order: # virsh net-update default add dns-srv '<srv service="aaa" protocol="tcp" domain="libvirt.org"/>' Updated network default live state # virsh net-update default add dns-srv '<srv service="aaa" protocol="tcp"/>' error: Failed to update network default error: Requested operation is not valid: there is already at least one DNS SRV record matching all specified fields in network default This does not work, even though it should have the same exact effect as the previous example. The issue is we do some heuristic guesswork to figure out what maybe makes sense and what does not. And the same (and more) will happen with the modify command. It's not that difficult to see how the above will mess up "modify", but let's see something more. We currently support what dnsmasq supports, multiple SRV records for the same domain/service (see dnsmasq(8) man page). That means this is perfectly valid configuration, even though it is heavily constructed to show the exact issue I have with it, so does not make *that* much sense: <srv service="a" protocol="tcp" target="asdf.com" domain="libvirt.org" priority="0" weight="10"/> <srv service="a" protocol="tcp" target="asdf.com" domain="libvirt.org" priority="0" weight="10" port="2"/> <srv service="a" protocol="tcp" target="asdf.com" domain="libvirt.org" priority="0" weight="10" port="3"/> That is, three SRV records with the only difference being they are pointing to different ports, 1, 2 and 3 (the first one defaults to 1). With net-update modify dns-srv how do you change the port for any of them? How do you remove the weight and priority from the second one? And on top of that, how do you delete the first one (or any for that matter)? The last is still an issue even without this patch, but we'll get to that. The only way to do this properly would be to have an API that accepts two strings, one that is the exact equivalent of the current record to modify and second which is the new "look" of the to-be-modified record. At that point it is clear to see these should just be two commands, "add" and "delete" one after each other. Since I cannot think of any reason why we should provide an atomic modify operation, the add->delete ought to be enough without requiring us to document the specificities of "modify" and the added support burden thereof. Having said that, "add" and "delete" operations should work in a more straightforward fashion. Add should only complain about adding something that is already there, but with different priority/weight (for that one I am willing to accept that there's probably no use for) and delete should similarly only look for the exact replicas of the records passed as input and not trying to be helpful. This is IMHO the best way to "fix" the current situation. I just hope we will revert this patch before the release so that we do not have to support `net-update modify dns-srv` till the end of times =D @Adam would you be willing to send a revert for this, please? @Jiri can you wait a little bit with the release for this? Thank you all. My way more than 2 cents, Martin
Attachment:
signature.asc
Description: PGP signature
