Looks like MREG allows name collisions between host objects (A records) and CNAMEs
Closed this issue · 3 comments
mikaeld
virker som vi har funnet en bug: vortex-sync01 er et host-object som har et cname knyttet til seg: vortex-sync. i går fikk
virtprov lov å lage et fullverdig host-object med navn vortex-sync
resultatet er at vi har både "vortex-sync cname vortex-sync01" og "vortex-sync A 129.240.113.119"
det går ikke
odberg
Jøss! Betyr det at denne sjekken gjøres i mreg-cli, men ikke i API'et?
mreg> host cname_add fangorn floffa
OK: : Added cname alias floffa.uio.no for fangorn.uio.no
mreg> host add -ip 129.240.202.0/ floffa
WARNING: : the name is already in use by a cname
mreg>
(Forresten, dette burde vært en ERROR, ikke WARNING. Ting som ikke blir utført bør gi ERROR).
mikaeld
enig, warning gir en antydning om at det ble utført selv om det ikke er optimalt å gjøre det
amedov
Må ærlig innrømme at det aldri falt meg inn å gjøre en slik sjekk i virtprov, da jeg trodde man alltid kunne stole på at det der ikke kunne skje. Men det gir mening med tanke på interne logikken
amedov
For det gir jo mening at mreg tillater noe sånt i APIet, felt i A-records-tabellen er jo helt separat fra C-records-tabellen
Men tydeligvis burde de to ha kobling i navnfelta så det aldri kan eksistere to felt som er like
odberg
Hm, nei, jeg synes ikke at det gir mening at mreg tillater at man gjør operasjoner via API'et som vil gi ugyldig sonefil.
amedov
Jeg forstår jo at det ikke gir mening for DNS, og tydeligvis er dette enten en bug eller feil implementasjon
amedov
Ser jo ut som sjekken blir gjort:
mreg/mreg/api/v1/serializers.py
Line 49 in 59d658d
Here is a test that reproduces the issue:
diff --git a/mreg/api/v1/tests/tests.py b/mreg/api/v1/tests/tests.py
index 8e277b7..0a2f9d2 100644
--- a/mreg/api/v1/tests/tests.py
+++ b/mreg/api/v1/tests/tests.py
@@ -453,6 +453,14 @@ class APIHostsTestCase(MregAPITestCase):
""""Posting a new host with a name already in use should return 409"""
self.assert_post_and_409('/hosts/', self.post_data_name)
+ def test_hosts_post_409_conflict_cname(self):
+ """"Posting a new host with a CNAME already in use should return 409"""
+ post_cname = {'name': 'new-name2.example.org',
+ 'host': self.host_two.id,
+ 'ttl': 5000}
+ self.assert_post('/cnames/', post_cname)
+ self.assert_post_and_409('/hosts/', self.post_data)
+
def test_hosts_patch_204_no_content(self):
"""Patching an existing and valid entry should return 204 and Location"""
response = self.assert_patch_and_204('/hosts/%s' % self.host_one.name, self.patch_data)
I tried naively patching HostSerializer to check for a CNAME conflict:
def validate(self, data):
data = super().validate(data)
name = data.get('name')
if name and Cname.objects.filter(name=name).exists():
raise serializers.ValidationError(
"CNAME record already exists with this name")
return data
...which kind of works, but gives 400 Bad Request instead of 409 Conflict as demanded by API_hosts.md.
Not sure how to entice a 409 response here. Thoughts?
Embarrassing bug. Though API_hosts.md status 409 for Conflicts, it is severly outdated...
DRF returns 400 when validation fails, as there can be multiple errors and therefore the generic 400 is chosen.
A simple un-tested fix for the 400 -> 409 error code is:
err = serializers.ValidationError("blah")
err.status_code = status.HTTP_409_CONFLICT
raise err