From 88464687487673247a163885b3379e67c2cd88f8 Mon Sep 17 00:00:00 2001 From: Pieter Lexis Date: Mon, 3 Jun 2024 10:54:38 +0200 Subject: [PATCH] SVCB: Make SvcParams a dict --- octodns/record/svcb.py | 54 ++++++++++++++-------- tests/test_octodns_record_svcb.py | 74 ++++++++++++++++++------------- 2 files changed, 77 insertions(+), 51 deletions(-) diff --git a/octodns/record/svcb.py b/octodns/record/svcb.py index 8ff40af..3ec3a39 100644 --- a/octodns/record/svcb.py +++ b/octodns/record/svcb.py @@ -118,10 +118,19 @@ class SvcbValue(EqualityTupleMixin, dict): except ValueError: pass targetname = unquote(targetname) + params = dict() + for svcparam in svcparams: + paramkey, *paramvalue = svcparam.split('=') + if paramkey in params.keys(): + raise RrParseError(f'{paramkey} is specified twice') + if len(paramvalue) != 0: + params[paramkey] = paramvalue[0] + continue + params[paramkey] = None return { 'svcpriority': svcpriority, 'targetname': targetname, - 'svcparams': svcparams, + 'svcparams': params, } @classmethod @@ -154,31 +163,30 @@ class SvcbValue(EqualityTupleMixin, dict): ) if 'svcparams' in value: - svcparams = value.get('svcparams', list()) + svcparams = value.get('svcparams', dict()) if svcpriority == 0 and len(svcparams) != 0: reasons.append('svcparams set on AliasMode SVCB record') - for param in svcparams: + for svcparamkey, svcparamvalue in svcparams.items(): # XXX: Should we test for keys existing when set in 'mandatory'? - paramkey, *paramvalue = param.split('=') - if paramkey.startswith('key'): - reasons += validate_svckey_number(paramkey) + if svcparamkey.startswith('key'): + reasons += validate_svckey_number(svcparamkey) continue if ( - paramkey not in SUPPORTED_PARAMS.keys() - and not paramkey.startswith('key') + svcparamkey not in SUPPORTED_PARAMS.keys() + and not svcparamkey.startswith('key') ): - reasons.append(f'Unknown SvcParam {paramkey}') + reasons.append(f'Unknown SvcParam {svcparamkey}') continue - if SUPPORTED_PARAMS[paramkey].get('has_arg', True): - reasons += SUPPORTED_PARAMS[paramkey]['validate']( - paramvalue[0] + if SUPPORTED_PARAMS[svcparamkey].get('has_arg', True): + reasons += SUPPORTED_PARAMS[svcparamkey]['validate']( + svcparamvalue ) if ( - not SUPPORTED_PARAMS[paramkey].get('has_arg', True) - and len(paramvalue) != 0 + not SUPPORTED_PARAMS[svcparamkey].get('has_arg', True) + and svcparamvalue is not None ): reasons.append( - f'SvcParam {paramkey} has value when it should not' + f'SvcParam {svcparamkey} has value when it should not' ) return reasons @@ -192,7 +200,7 @@ class SvcbValue(EqualityTupleMixin, dict): { 'svcpriority': int(value['svcpriority']), 'targetname': idna_encode(value['targetname']), - 'svcparams': value.get('svcparams', list()), + 'svcparams': value.get('svcparams', dict()), } ) @@ -223,15 +231,23 @@ class SvcbValue(EqualityTupleMixin, dict): @property def rdata_text(self): params = '' - if len(self.svcparams) != 0: - params = f' {" ".join(self.svcparams)}' + for svcparamkey, svcparamvalue in self.svcparams.items(): + params += f' {svcparamkey}' + if svcparamvalue is not None: + params += f'={svcparamvalue}' return f'{self.svcpriority} {self.targetname}{params}' def __hash__(self): return hash(self.__repr__()) def _equality_tuple(self): - return (self.svcpriority, self.targetname, self.svcparams) + params = set() + for svcparamkey, svcparamvalue in self.svcparams.items(): + if svcparamvalue is not None: + params.add(f'{svcparamkey}={svcparamvalue}') + else: + params.add(f'{svcparamkey}') + return (self.svcpriority, self.targetname, params) def __repr__(self): return f"'{self.rdata_text}'" diff --git a/tests/test_octodns_record_svcb.py b/tests/test_octodns_record_svcb.py index 729b977..767d3f0 100644 --- a/tests/test_octodns_record_svcb.py +++ b/tests/test_octodns_record_svcb.py @@ -37,14 +37,14 @@ class TestRecordSvcb(TestCase): { 'svcpriority': 1, 'targetname': 'foo.example.com.', - 'svcparams': 'port=8002', + 'svcparams': {'port': 8002}, } ), SvcbValue( { 'svcpriority': 2, 'targetname': 'foo.example.net.', - 'svcparams': 'port=8080', + 'svcparams': {'port': 8080}, } ), ] @@ -86,13 +86,13 @@ class TestRecordSvcb(TestCase): self.assertEqual(change.new, other) # Diff in target causes change other.values[0].svcpriority = b.values[0].svcpriority - other.values[0].targetname = 'blabla.example.com' + other.values[0].targetname = 'blabla.example.com.' change = b.changes(other, target) self.assertEqual(change.existing, b) self.assertEqual(change.new, other) # Diff in params causes change other.values[0].targetname = b.values[0].targetname - other.values[0].svcparams = 'port=8888' + other.values[0].svcparams = {'port': '8888'} change = b.changes(other, target) self.assertEqual(change.existing, b) self.assertEqual(change.new, other) @@ -110,12 +110,16 @@ class TestRecordSvcb(TestCase): with self.assertRaises(RrParseError): SvcbValue.parse_rdata_text('nope') + # Double keys are not allowed + with self.assertRaises(RrParseError): + SvcbValue.parse_rdata_text('1 foo.example.com port=8080 port=8084') + # priority not int self.assertEqual( { 'svcpriority': 'one', 'targetname': 'foo.example.com', - 'svcparams': list(), + 'svcparams': dict(), }, SvcbValue.parse_rdata_text('one foo.example.com'), ) @@ -125,9 +129,11 @@ class TestRecordSvcb(TestCase): { 'svcpriority': 1, 'targetname': 'svcb.unit.tests.', - 'svcparams': ['port=8080'], + 'svcparams': {'port': '8080', 'no-default-alpn': None}, }, - SvcbValue.parse_rdata_text('1 svcb.unit.tests. port=8080'), + SvcbValue.parse_rdata_text( + '1 svcb.unit.tests. port=8080 no-default-alpn' + ), ) # quoted target @@ -135,7 +141,7 @@ class TestRecordSvcb(TestCase): { 'svcpriority': 1, 'targetname': 'svcb.unit.tests.', - 'svcparams': list(), + 'svcparams': dict(), }, SvcbValue.parse_rdata_text('1 "svcb.unit.tests."'), ) @@ -149,16 +155,16 @@ class TestRecordSvcb(TestCase): 'value': { 'svcpriority': 1, 'targetname': 'svcb.unit.tests.', - 'svcparams': ['port=8080'], + 'svcparams': {'port': '8080'}, }, }, ) self.assertEqual(1, a.values[0].svcpriority) self.assertEqual('svcb.unit.tests.', a.values[0].targetname) - self.assertEqual(['port=8080'], a.values[0].svcparams) + self.assertEqual({'port': '8080'}, a.values[0].svcparams) # both directions should match - rdata = '1 svcb.unit.tests. port=8080' + rdata = '1 svcb.unit.tests. port=8080 no-default-alpn' record = SvcbRecord( zone, 'svc', {'ttl': 32, 'value': SvcbValue.parse_rdata_text(rdata)} ) @@ -173,26 +179,30 @@ class TestRecordSvcb(TestCase): def test_svcb_value(self): a = SvcbValue( - {'svcpriority': 0, 'targetname': 'foo.', 'svcparams': list()} + {'svcpriority': 0, 'targetname': 'foo.', 'svcparams': dict()} ) b = SvcbValue( - {'svcpriority': 1, 'targetname': 'foo.', 'svcparams': list()} + {'svcpriority': 1, 'targetname': 'foo.', 'svcparams': dict()} ) c = SvcbValue( - {'svcpriority': 0, 'targetname': 'foo.', 'svcparams': ['port=8080']} + { + 'svcpriority': 0, + 'targetname': 'foo.', + 'svcparams': {'port': 8080, 'no-default-alpn': None}, + } ) d = SvcbValue( { 'svcpriority': 0, 'targetname': 'foo.', - 'svcparams': ['alpn=h2,h3', 'port=8080'], + 'svcparams': {'alpn': 'h2,h3', 'port': 8080}, } ) e = SvcbValue( { 'svcpriority': 0, 'targetname': 'mmm.', - 'svcparams': ['ipv4hint=192.0.2.1'], + 'svcparams': {'ipv4hint': '192.0.2.1'}, } ) @@ -402,7 +412,7 @@ class TestRecordSvcb(TestCase): 'value': { 'svcpriority': 0, 'targetname': 'foo.bar.com.', - 'svcparams': ['port=8000'], + 'svcparams': {'port': '8000'}, }, }, ) @@ -421,7 +431,7 @@ class TestRecordSvcb(TestCase): 'value': { 'svcpriority': 1, 'targetname': 'foo.bar.com.', - 'svcparams': ['blablabla=222'], + 'svcparams': {'blablabla': '222'}, }, }, ) @@ -438,7 +448,7 @@ class TestRecordSvcb(TestCase): 'value': { 'svcpriority': 1, 'targetname': 'foo.bar.com.', - 'svcparams': ['port=100000'], + 'svcparams': {'port': 100000}, }, }, ) @@ -457,7 +467,7 @@ class TestRecordSvcb(TestCase): 'value': { 'svcpriority': 1, 'targetname': 'foo.bar.com.', - 'svcparams': ['port=foo'], + 'svcparams': {'port': 'foo'}, }, }, ) @@ -473,7 +483,7 @@ class TestRecordSvcb(TestCase): 'value': { 'svcpriority': 1, 'targetname': 'foo.bar.com.', - 'svcparams': ['no-default-alpn'], + 'svcparams': {'no-default-alpn': None}, }, }, ) @@ -489,7 +499,7 @@ class TestRecordSvcb(TestCase): 'value': { 'svcpriority': 1, 'targetname': 'foo.bar.com.', - 'svcparams': ['no-default-alpn=foobar'], + 'svcparams': {'no-default-alpn': 'foobar'}, }, }, ) @@ -509,7 +519,7 @@ class TestRecordSvcb(TestCase): 'value': { 'svcpriority': 1, 'targetname': 'foo.bar.com.', - 'svcparams': ['alpn=h2,😅'], + 'svcparams': {'alpn': 'h2,😅'}, }, }, ) @@ -526,7 +536,7 @@ class TestRecordSvcb(TestCase): 'value': { 'svcpriority': 1, 'targetname': 'foo.bar.com.', - 'svcparams': ['ipv4hint=192.0.2.0,500.500.30.30'], + 'svcparams': {'ipv4hint': '192.0.2.0,500.500.30.30'}, }, }, ) @@ -546,7 +556,7 @@ class TestRecordSvcb(TestCase): 'value': { 'svcpriority': 1, 'targetname': 'foo.bar.com.', - 'svcparams': ['ipv6hint=2001:db8:43::1,notanip'], + 'svcparams': {'ipv6hint': '2001:db8:43::1,notanip'}, }, }, ) @@ -566,7 +576,7 @@ class TestRecordSvcb(TestCase): 'value': { 'svcpriority': 1, 'targetname': 'foo.bar.com.', - 'svcparams': ['mandatory=ipv4hint,unknown,key4444'], + 'svcparams': {'mandatory': 'ipv4hint,unknown,key4444'}, }, }, ) @@ -586,7 +596,7 @@ class TestRecordSvcb(TestCase): 'value': { 'svcpriority': 1, 'targetname': 'foo.bar.com.', - 'svcparams': ['ech=dG90YWxseUZha2VFQ0hPcHRpb24'], + 'svcparams': {'ech': ' dG90YWxseUZha2VFQ0hPcHRpb24'}, }, }, ) @@ -605,11 +615,11 @@ class TestRecordSvcb(TestCase): 'value': { 'svcpriority': 1, 'targetname': 'foo.bar.com.', - 'svcparams': [ - 'key100000=foo', - 'key3333=bar', - 'keyXXX=foo', - ], + 'svcparams': { + 'key100000': 'foo', + 'key3333': 'bar', + 'keyXXX': 'foo', + }, }, }, )