sts-sponsors team mailing list archive

[Alberto Donato <mp+438041@xxxxxxxxxxxxxxxxxx>]

Alberto Donato has proposed merging ~ack/maas:macaddress-field-rework into maas:master.

Commit message:
rework MACAddresField to use text as storage type


Requested reviews:
  Björn Tillenius (bjornt)
  MAAS Lander (maas-lander)
Related bugs:
  Bug #1696108 in MAAS: "Interface model validates the MAC address twice"
  https://bugs.launchpad.net/maas/+bug/1696108

For more details, see:
https://code.launchpad.net/~ack/maas/+git/maas/+merge/438041
-- 
Your team MAAS Committers is subscribed to branch maas:master.

diff --git a/src/maasserver/api/machines.py b/src/maasserver/api/machines.py
index 766dc68..a5374f0 100644
--- a/src/maasserver/api/machines.py
+++ b/src/maasserver/api/machines.py
@@ -13,7 +13,7 @@ import json
 import re
 
 from django.conf import settings
-from django.core.exceptions import PermissionDenied, ValidationError
+from django.core.exceptions import PermissionDenied
 from django.db.models import Q
 from django.http import (
     HttpResponse,
@@ -62,7 +62,7 @@ from maasserver.exceptions import (
     NodeStateViolation,
     Unauthorized,
 )
-from maasserver.fields import MAC_VALIDATOR
+from maasserver.fields import MAC_RE
 from maasserver.forms import (
     AdminMachineForm,
     get_machine_create_form,
@@ -1914,9 +1914,7 @@ class AnonMachinesHandler(AnonNodesHandler):
             # caught.
             macs_valid = True
             for mac_address in mac_addresses:
-                try:
-                    MAC_VALIDATOR(mac_address)
-                except ValidationError:
+                if not MAC_RE.match(mac_address):
                     macs_valid = False
                     break
             if macs_valid:
diff --git a/src/maasserver/api/nodes.py b/src/maasserver/api/nodes.py
index f6d58e6..5754853 100644
--- a/src/maasserver/api/nodes.py
+++ b/src/maasserver/api/nodes.py
@@ -41,7 +41,7 @@ from maasserver.exceptions import (
     NoScriptsFound,
     StaticIPAddressExhaustion,
 )
-from maasserver.fields import MAC_RE
+from maasserver.fields import MAC_RE, normalise_macaddress
 from maasserver.forms import BulkNodeSetZoneForm
 from maasserver.forms.ephemeral import TestForm
 from maasserver.models import Filesystem, Interface, Node, OwnerData
@@ -209,13 +209,17 @@ def filtered_nodes_list_from_request(request, model=None):
 
 def is_registered(request, ignore_statuses=None):
     """Used by both `NodesHandler` and `AnonNodesHandler`."""
-    mac_address = get_mandatory_param(request.GET, "mac_address")
-    interfaces = Interface.objects.filter(mac_address=mac_address)
-    interfaces = interfaces.exclude(node_config__node__isnull=True)
     if ignore_statuses is None:
         ignore_statuses = [NODE_STATUS.RETIRED]
-    interfaces = interfaces.exclude(
-        node_config__node__status__in=ignore_statuses
+
+    mac_address = normalise_macaddress(
+        get_mandatory_param(request.GET, "mac_address")
+    )
+
+    interfaces = (
+        Interface.objects.filter(mac_address=mac_address)
+        .exclude(node_config__node__isnull=True)
+        .exclude(node_config__node__status__in=ignore_statuses)
     )
     return interfaces.exists()
 
diff --git a/src/maasserver/fields.py b/src/maasserver/fields.py
index 276d4d4..c3cfb46 100644
--- a/src/maasserver/fields.py
+++ b/src/maasserver/fields.py
@@ -3,6 +3,7 @@
 
 """Custom model and form fields."""
 
+from itertools import chain
 import re
 
 from django import forms
@@ -15,6 +16,7 @@ from django.db.models import (
     GenericIPAddressField,
     IntegerField,
     Q,
+    TextField,
     URLField,
 )
 from django.utils.deconstruct import deconstructible
@@ -36,8 +38,30 @@ MAC_RE = re.compile(
     r"([0-9a-fA-F]{3,4}.){2}[0-9a-fA-F]{3,4}"  # aabb.ccdd.eeff
     r"$"
 )
-MAC_ERROR_MSG = "'%(value)s' is not a valid MAC address."
-MAC_VALIDATOR = RegexValidator(regex=MAC_RE, message=MAC_ERROR_MSG)
+MAC_VALIDATOR = RegexValidator(
+    regex=MAC_RE, message="'%(value)s' is not a valid MAC address."
+)
+
+MAC_SPLIT_RE = re.compile(r"[-:.]")
+
+
+def normalise_macaddress(mac: str) -> str:
+    """Return a colon-separated format for the specified MAC.
+
+    This supports converting from input formats matching the MAC_RE regexp.
+    """
+
+    tokens = MAC_SPLIT_RE.split(mac.lower())
+    match len(tokens):
+        case 1:  # no separator
+            tokens = re.findall("..", tokens[0])
+        case 3:  # each token is two bytes
+            tokens = chain(
+                *(re.findall("..", token.zfill(4)) for token in tokens)
+            )
+        case _:  # single-byte tokens
+            tokens = (token.zfill(2) for token in tokens)
+    return ":".join(tokens)
 
 
 class MACAddressFormField(forms.CharField):
@@ -45,24 +69,23 @@ class MACAddressFormField(forms.CharField):
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
-        self.validators.append(
-            RegexValidator(regex=MAC_RE, message=MAC_ERROR_MSG)
-        )
+        self.validators.append(MAC_VALIDATOR)
 
 
-class MACAddressField(Field):
+class MACAddressField(TextField):
     """Model field type: MAC address."""
 
     description = "MAC address"
 
-    default_validators = [MAC_VALIDATOR]
-
-    def db_type(self, *args, **kwargs):
-        return "macaddr"
-
     def get_prep_value(self, value):
-        # Convert empty string to None.
-        return super().get_prep_value(value) or None
+        value = super().get_prep_value(value)
+        if not value:
+            return value
+        return normalise_macaddress(value)
+
+    def validate(self, value, model_instance):
+        if value:
+            MAC_VALIDATOR(value)
 
 
 class XMLField(Field):
diff --git a/src/maasserver/migrations/maasserver/0295_macaddress_text_field.py b/src/maasserver/migrations/maasserver/0295_macaddress_text_field.py
new file mode 100644
index 0000000..071b63a
--- /dev/null
+++ b/src/maasserver/migrations/maasserver/0295_macaddress_text_field.py
@@ -0,0 +1,16 @@
+# Generated by Django 3.2.12 on 2023-02-28 16:03
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("maasserver", "0294_keyring_data_binary_field"),
+    ]
+
+    operations = [
+        migrations.RunSQL(
+            f"ALTER TABLE maasserver_{table} ALTER COLUMN mac_address TYPE TEXT"
+        )
+        for table in ("interface", "neighbour", "virtualmachineinterface")
+    ]
diff --git a/src/maasserver/models/interface.py b/src/maasserver/models/interface.py
index 1b89447..c9b3980 100644
--- a/src/maasserver/models/interface.py
+++ b/src/maasserver/models/interface.py
@@ -49,7 +49,7 @@ from maasserver.exceptions import (
     StaticIPAddressOutOfRange,
     StaticIPAddressUnavailable,
 )
-from maasserver.fields import MAC_VALIDATOR, MACAddressField
+from maasserver.fields import MACAddressField
 from maasserver.models.cleansave import CleanSave
 from maasserver.models.staticipaddress import StaticIPAddress
 from maasserver.models.timestampedmodel import TimestampedModel
@@ -1482,13 +1482,6 @@ class Interface(CleanSave, TimestampedModel):
             all_related |= ancestor.get_successors()
         return all_related
 
-    def clean(self):
-        super().clean()
-
-        # Verify that the MAC address is legal if it is not empty.
-        if self.mac_address:
-            MAC_VALIDATOR(self.mac_address)
-
     def delete(self, remove_ip_address=True):
         # We set the _skip_ip_address_removal so the signal can use it to
         # skip removing the IP addresses. This is normally only done by the
diff --git a/src/maasserver/models/tests/test_interface.py b/src/maasserver/models/tests/test_interface.py
index 919074b..aca2a9e 100644
--- a/src/maasserver/models/tests/test_interface.py
+++ b/src/maasserver/models/tests/test_interface.py
@@ -1004,23 +1004,11 @@ class TestInterface(MAASServerTestCase):
             INTERFACE_TYPE.PHYSICAL,
             mac_address="invalid",
         )
-        # XXX Danilo 2017-05-26 bug #1696108: we validate the MAC address
-        # twice: once as part of Interface.clean() resulting in the __all__
-        # error, and once as part of field validation that happens after a
-        # few queries are done, so we cannot easily get rid of
-        # MAC_VALIDATOR() in clean().
-        self.assertThat(
+        self.assertEqual(
             exception.message_dict,
-            MatchesDict(
-                {
-                    "__all__": MatchesListwise(
-                        [Equals("'invalid' is not a valid MAC address.")]
-                    ),
-                    "mac_address": MatchesListwise(
-                        [Equals("'invalid' is not a valid MAC address.")]
-                    ),
-                }
-            ),
+            {
+                "mac_address": ["'invalid' is not a valid MAC address."],
+            },
         )
 
     def test_allows_blank_mac_address(self):
diff --git a/src/maasserver/pytest_tests/test_fields.py b/src/maasserver/pytest_tests/test_fields.py
new file mode 100644
index 0000000..87ad823
--- /dev/null
+++ b/src/maasserver/pytest_tests/test_fields.py
@@ -0,0 +1,19 @@
+import pytest
+
+from maasserver.fields import normalise_macaddress
+
+
+@pytest.mark.parametrize(
+    ("value", "expected"),
+    [
+        ("aabbccddeeff", "aa:bb:cc:dd:ee:ff"),
+        ("aa:bb:cc:dd:ee:ff", "aa:bb:cc:dd:ee:ff"),
+        ("aa:b:cc:d:ee:f", "aa:0b:cc:0d:ee:0f"),
+        ("aa-bb-cc-dd-ee-ff", "aa:bb:cc:dd:ee:ff"),
+        ("aa-b-cc-d-ee-f", "aa:0b:cc:0d:ee:0f"),
+        ("aabb.ccdd.eeff", "aa:bb:cc:dd:ee:ff"),
+        ("abb.cdd.eeff", "0a:bb:0c:dd:ee:ff"),
+    ],
+)
+def test_normalise_mac_address(value, expected):
+    assert normalise_macaddress(value) == expected
diff --git a/src/maasserver/rpc/tests/test_rackcontrollers.py b/src/maasserver/rpc/tests/test_rackcontrollers.py
index 88b0591..05de7b4 100644
--- a/src/maasserver/rpc/tests/test_rackcontrollers.py
+++ b/src/maasserver/rpc/tests/test_rackcontrollers.py
@@ -212,7 +212,7 @@ class TestRegisterRackController(MAASServerTestCase):
 
     def test_creates_new_rackcontroller(self):
         existing_machine = factory.make_Machine()
-        rack_mac = (factory.make_mac_address(),)
+        rack_mac = factory.make_mac_address()
         interfaces = {
             factory.make_name("eth0"): {
                 "type": "physical",
diff --git a/src/maasserver/tests/test_fields.py b/src/maasserver/tests/test_fields.py
index 24014da..99cc72d 100644
--- a/src/maasserver/tests/test_fields.py
+++ b/src/maasserver/tests/test_fields.py
@@ -64,7 +64,7 @@ class TestModelNameValidator(MAASServerTestCase):
 class TestMACAddressField(MAASServerTestCase):
     def test_mac_address_is_stored_normalized_and_loaded(self):
         interface = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, mac_address=" AA-bb-CC-dd-EE-Ff "
+            INTERFACE_TYPE.PHYSICAL, mac_address="AA-bb-CC-dd-EE-Ff"
         )
         loaded_mac = Interface.objects.get(id=interface.id)
         self.assertEqual("aa:bb:cc:dd:ee:ff", loaded_mac.mac_address)