Merge ~tiago.pasqualini/maas:lp1807725 into maas:master

[Tiago Pasqualini da Silva]

Fix incorrect hostname from interface name

    MAAS currently creates a DNS record for each interface in a host
    by simply using its interface name. Whenever an interface has the
    '_' character, the code uses it anyway, which is currently breaking
    bind as this character is not allowed on domain names.
    This patch fixes that by verifying it and replacing the incorrect
    character.

    LP: #1807725

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1807725 lp:~tiago.pasqualini/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas-tester/1547/consoleText
COMMIT: d5b537062408b0f05dbbe98bc95f21e4d542ca41

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1807725 lp:~tiago.pasqualini/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas-tester/1549/consoleText
COMMIT: a3337f142376a14d3b1b300033e176fbadfbbbff

review: Needs Fixing

[Alexsander de Souza (alexsander-souza)]

jenkins: !test

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1807725 lp:~tiago.pasqualini/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: a3337f142376a14d3b1b300033e176fbadfbbbff

review: Approve

[Thorsten Merten (thorsten-merten)]

Thanks for the MP! This would fix the DNS resolution.

While we are at it, I have the feeling that a dot in the interface name (.) is also a bit weird to be taken over to a hostname as a dot has its own semantics in DNS.

Finally, it might be confusing to the user if we replace characters "at random" without a warning in the UI or the CLI in case that the interface name will be used as the host name.

We will need to check with UX before merging.

[Björn Tillenius (bjornt)]

I think the patch looks fine as it is. Could you please add a test for this, though?

review: Needs Information

[Tiago Pasqualini da Silva (tiago.pasqualini)]

Added the tests, please let me know if that looks fine.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1807725 lp:~tiago.pasqualini/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas-tester/2161/consoleText
COMMIT: f2d97c69435a5968daf6ab84d0e6e39f80ac1c50

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1807725 lp:~tiago.pasqualini/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: d3af9741abe27a7d675ce530f6f1dccb3a604ee6

review: Approve

[Mauricio Faria de Oliveira (mfo)]

Hi Björn,

Friendly follow-up, if you have a chance to review the tests added.

Thanks!

[Björn Tillenius (bjornt)]

Thanks, it looks good now, except for Alberto's comment, which should be trivial to fix.

review: Approve

[Tiago Pasqualini da Silva (tiago.pasqualini)]

Hi Björn, you approved it but left a comment. Do you want me to fix that or it is good as is?

I'm fine either way, it's a simple method replace.

[Adam Collard (adam-collard)]

> Hi Björn, you approved it but left a comment. Do you want me to fix that or it
> is good as is?
>
> I'm fine either way, it's a simple method replace.

Please fix it - we do not want to add any more assertThat()s to MAAS

[Tiago Pasqualini da Silva (tiago.pasqualini)]

Done

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1807725 lp:~tiago.pasqualini/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: daf5370ba095eb80ca10b44c53d14bfac0bb83ef

review: Approve

[Tiago Pasqualini da Silva (tiago.pasqualini)]

Hi Björn and Adam,

Friendly follow-up. Please review the changes.

Thank you,
Tiago

[Björn Tillenius (bjornt)]

Sorry for the delay. Thanks for the fixes, it's good to go now!

review: Approve

[MAAS Lander (maas-lander)]

UNABLE TO START LANDING

STATUS: MISSING COMMIT MESSAGE

diff --git a/src/maasserver/models/staticipaddress.py b/src/maasserver/models/staticipaddress.py
index b5794a1..81c7a82 100644
--- a/src/maasserver/models/staticipaddress.py
+++ b/src/maasserver/models/staticipaddress.py
@@ -52,7 +52,10 @@ from maasserver.models.domain import Domain
 from maasserver.models.subnet import Subnet
 from maasserver.models.timestampedmodel import TimestampedModel
 from maasserver.utils import orm
-from maasserver.utils.dns import get_ip_based_hostname
+from maasserver.utils.dns import (
+    get_iface_name_based_hostname,
+    get_ip_based_hostname,
+)
 from provisioningserver.utils.enum import map_enum_reverse
 
 StaticIPAddress = TypeVar("StaticIPAddress")
@@ -841,7 +844,11 @@ class StaticIPAddressManager(Manager):
             # node, then consider adding the IP.
             if result.assigned or not assigned_ips[result.fqdn]:
                 if result.ip not in mapping[result.fqdn].ips:
-                    entry = mapping[f"{result.iface_name}.{result.fqdn}"]
+                    fqdn = "{}.{}".format(
+                        get_iface_name_based_hostname(result.iface_name),
+                        result.fqdn,
+                    )
+                    entry = mapping[fqdn]
                     entry.node_type = result.node_type
                     entry.system_id = result.system_id
                     if result.user_id is not None:
diff --git a/src/maasserver/models/tests/test_staticipaddress.py b/src/maasserver/models/tests/test_staticipaddress.py
index ee9c08a..08cdff1 100644
--- a/src/maasserver/models/tests/test_staticipaddress.py
+++ b/src/maasserver/models/tests/test_staticipaddress.py
@@ -50,7 +50,10 @@ from maasserver.testing.testcase import (
     MAASTransactionServerTestCase,
 )
 from maasserver.utils import orm
-from maasserver.utils.dns import get_ip_based_hostname
+from maasserver.utils.dns import (
+    get_iface_name_based_hostname,
+    get_ip_based_hostname,
+)
 from maasserver.utils.orm import reload_object, transactional
 from maasserver.websockets.base import dehydrate_datetime
 
@@ -483,6 +486,47 @@ class TestStaticIPAddressManagerMapping(MAASServerTestCase):
         }
         self.assertEqual(expected, mapping)
 
+    def test_get_hostname_ip_mapping_sanitized_iface_name(self):
+        hostname = factory.make_name("hostname")
+        domainname = factory.make_name("domain")
+        factory.make_Domain(name=domainname)
+        full_hostname = f"{hostname}.{domainname}"
+        subnet = factory.make_Subnet()
+        node = factory.make_Node_with_Interface_on_Subnet(
+            extra_ifnames=["eth_1"],
+            hostname=full_hostname,
+            interface_count=2,
+            subnet=subnet,
+        )
+        boot_interface = node.get_boot_interface()
+        staticip = factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.STICKY,
+            ip=factory.pick_ip_in_Subnet(subnet),
+            subnet=subnet,
+            interface=boot_interface,
+        )
+        iface2 = node.current_config.interface_set.exclude(
+            id=boot_interface.id
+        ).first()
+        sip2 = factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.STICKY,
+            ip=factory.pick_ip_in_Subnet(subnet),
+            subnet=subnet,
+            interface=iface2,
+        )
+        mapping = StaticIPAddress.objects.get_hostname_ip_mapping(subnet)
+        sanitized_if2name = get_iface_name_based_hostname(iface2.name)
+        expected = {
+            full_hostname: HostnameIPMapping(
+                node.system_id, 30, {staticip.ip}, node.node_type
+            ),
+            "%s.%s"
+            % (sanitized_if2name, full_hostname): HostnameIPMapping(
+                node.system_id, 30, {sip2.ip}, node.node_type
+            ),
+        }
+        self.assertEqual(expected, mapping)
+
     def make_mapping(self, node, raw_ttl=False):
         if raw_ttl or node.address_ttl is not None:
             ttl = node.address_ttl
diff --git a/src/maasserver/utils/dns.py b/src/maasserver/utils/dns.py
index 0c8cde5..76f553a 100644
--- a/src/maasserver/utils/dns.py
+++ b/src/maasserver/utils/dns.py
@@ -91,6 +91,16 @@ def get_ip_based_hostname(ip):
     return hostname
 
 
+def get_iface_name_based_hostname(iface_name):
+    """Given the specified interface name, creates an automatically generated
+    hostname by converting the '_' characters in it to '-' characters.
+
+    :param iface_name: Input value for the interface name.
+    """
+    hostname = iface_name.replace("_", "-")
+    return hostname
+
+
 def validate_url(url, schemes=("http", "https")):
     """Validator for URLs.
 
diff --git a/src/maasserver/utils/tests/test_dns.py b/src/maasserver/utils/tests/test_dns.py
index e6be8ab..41cfe80 100644
--- a/src/maasserver/utils/tests/test_dns.py
+++ b/src/maasserver/utils/tests/test_dns.py
@@ -8,6 +8,7 @@ from django.core.validators import URLValidator
 from testtools.matchers import Equals, HasLength
 
 from maasserver.utils.dns import (
+    get_iface_name_based_hostname,
     get_ip_based_hostname,
     validate_domain_name,
     validate_hostname,
@@ -244,3 +245,11 @@ class TestIpBasedHostnameGenerator(MAASTestCase):
             get_ip_based_hostname("2001:67c:1562::15"),
             Equals("2001-67c-1562--15"),
         )
+
+
+class TestIfaceBasedHostnameGenerator(MAASTestCase):
+    def test_interface_name_changed(self):
+        self.assertEqual(get_iface_name_based_hostname("eth_0"), "eth-0")
+
+    def test_interface_name_unchanged(self):
+        self.assertEqual(get_iface_name_based_hostname("eth0"), "eth0")