Merge ~ack/maas:1982315-use-rack-ip-if-external-dns-3.2 into maas:3.2

[Alberto Donato]

LP:1982315 use rack IP instead of hostname in preseed if external DNS is used

This also makes handling of X-Forwarded-For a bit safer, since it ignores extra IPs that might come before the one from the rackd proxy (e.g. client-supplied values).

[Alberto Donato (ack)]

review: Approve

diff --git a/src/maasserver/compose_preseed.py b/src/maasserver/compose_preseed.py
index c814a01..145efc8 100644
--- a/src/maasserver/compose_preseed.py
+++ b/src/maasserver/compose_preseed.py
@@ -33,25 +33,36 @@ RSYSLOG_PORT = 5247
 RACK_CONTROLLER_PORT = 5248
 
 
+def _subnet_uses_dns(subnet):
+    return (
+        subnet is not None and not subnet.dns_servers and subnet.vlan.dhcp_on
+    )
+
+
+def _wrap_ipv6_address(ip):
+    return str(ip) if ip_address(ip).version == 4 else f"[{ip}]"
+
+
 def _get_anon_rack_host(request, rack_controller):
-    forwarded_for = request.META.get("HTTP_X_FORWARDED_FOR")
-    if forwarded_for:
-        client_ip = forwarded_for.split(",")[0]
-        subnet = Subnet.objects.get_best_subnet_for_ip(client_ip)
-        if subnet:
+    forwarded_ips = request.META.get("HTTP_X_FORWARDED_FOR", "").split(", ")
+    if len(forwarded_ips) >= 2:
+        # the reguest goes through the nginx proxy on the rack to the nginx
+        # proxy on the region
+        forwarded_ip = forwarded_ips[-2]
+        subnet = Subnet.objects.get_best_subnet_for_ip(forwarded_ip)
+        if _subnet_uses_dns(subnet):
             internal_domain = Config.objects.get_config("maas_internal_domain")
             return f"{get_resource_name_for_subnet(subnet)}.{internal_domain}"
+        else:
+            return _wrap_ipv6_address(forwarded_ip)
     return rack_controller.fqdn if rack_controller else ""
 
 
 def build_metadata_url(request, route, rack_controller, node=None, extra=""):
-    host = _get_anon_rack_host(request, rack_controller)
     if node and node.boot_cluster_ip:
-        host = (
-            str(node.boot_cluster_ip)
-            if ip_address(node.boot_cluster_ip).version == 4
-            else f"[{node.boot_cluster_ip}]"
-        )
+        host = _wrap_ipv6_address(node.boot_cluster_ip)
+    else:
+        host = _get_anon_rack_host(request, rack_controller)
     return (
         request.build_absolute_uri(route) + extra
         if not host
@@ -89,12 +100,7 @@ def get_apt_proxy(request, rack_controller=None, node=None):
             remote_ip = get_remote_ip(request)
             if remote_ip is not None:
                 subnet = Subnet.objects.get_best_subnet_for_ip(remote_ip)
-            use_dns = (
-                subnet is not None
-                and not subnet.dns_servers
-                and subnet.vlan.dhcp_on
-            )
-            if config["use_rack_proxy"] and use_dns:
+            if config["use_rack_proxy"] and _subnet_uses_dns(subnet):
                 # Client can use the MAAS proxy on the rack controller with
                 # DNS resolution providing better HA.
                 return "http://%s.%s:%d/" % (
diff --git a/src/maasserver/tests/test_compose_preseed.py b/src/maasserver/tests/test_compose_preseed.py
index c5a65c1..b733447 100644
--- a/src/maasserver/tests/test_compose_preseed.py
+++ b/src/maasserver/tests/test_compose_preseed.py
@@ -1273,9 +1273,12 @@ class TestBuildMetadataURL(MAASServerTestCase):
         node.boot_cluster_ip = factory.make_ip_address()
         node.save()
         request = make_HttpRequest()
-        subnet = factory.make_Subnet()
-        original_ip = subnet.get_next_ip_for_allocation()
-        request.META["HTTP_X_FORWARDED_FOR"] = str(original_ip)
+        subnet = factory.make_Subnet(dhcp_on=True, dns_servers=[])
+        rack_proxy_ip = subnet.get_next_ip_for_allocation()
+        region_proxy_ip = subnet.get_next_ip_for_allocation()
+        request.META[
+            "HTTP_X_FORWARDED_FOR"
+        ] = f"{rack_proxy_ip}, {region_proxy_ip}"
         route = "/MAAS"
         Config.objects.set_config("maas_internal_domain", factory.make_name())
         # not passing node to simulate anonymous request, i.e enlistment
@@ -1285,4 +1288,75 @@ class TestBuildMetadataURL(MAASServerTestCase):
                 request, route, node.get_boot_rack_controller()
             ),
         )
-        route = "/MAAS"
+
+    def test_uses_rack_ipv4_if_external_dns(self):
+        # regression test for LP:1982315
+        node = factory.make_Node()
+        request = make_HttpRequest()
+        subnet = factory.make_Subnet(
+            cidr=factory.make_ipv4_network(),
+            dhcp_on=True,
+            dns_servers=[factory.make_ip_address()],
+        )
+        rack_proxy_ip = subnet.get_next_ip_for_allocation()
+        region_proxy_ip = subnet.get_next_ip_for_allocation()
+        request.META[
+            "HTTP_X_FORWARDED_FOR"
+        ] = f"{rack_proxy_ip}, {region_proxy_ip}"
+        self.assertEqual(
+            f"{request.scheme}://{rack_proxy_ip}:5248/MAAS",
+            build_metadata_url(
+                request,
+                "/MAAS",
+                node.get_boot_rack_controller(),
+                node=node,
+            ),
+        )
+
+    def test_uses_rack_ipv6_if_external_dns(self):
+        # regression test for LP:1982315
+        node = factory.make_Node()
+        request = make_HttpRequest()
+        subnet = factory.make_Subnet(
+            cidr=factory.make_ipv6_network(),
+            dhcp_on=True,
+            dns_servers=[factory.make_ip_address()],
+        )
+        rack_proxy_ip = subnet.get_next_ip_for_allocation()
+        region_proxy_ip = subnet.get_next_ip_for_allocation()
+        request.META[
+            "HTTP_X_FORWARDED_FOR"
+        ] = f"{rack_proxy_ip}, {region_proxy_ip}"
+        self.assertEqual(
+            f"{request.scheme}://[{rack_proxy_ip}]:5248/MAAS",
+            build_metadata_url(
+                request,
+                "/MAAS",
+                node.get_boot_rack_controller(),
+                node=node,
+            ),
+        )
+
+    def test_uses_rack_ip_with_multiple_forwarded_for(self):
+        node = factory.make_Node()
+        request = make_HttpRequest()
+        subnet = factory.make_Subnet(
+            cidr=factory.make_ipv4_network(),
+            dhcp_on=True,
+            dns_servers=[factory.make_ip_address()],
+        )
+        rack_proxy_ip = subnet.get_next_ip_for_allocation()
+        region_proxy_ip = subnet.get_next_ip_for_allocation()
+        other_ip = subnet.get_next_ip_for_allocation()
+        request.META[
+            "HTTP_X_FORWARDED_FOR"
+        ] = f"{other_ip}, {rack_proxy_ip}, {region_proxy_ip}"
+        self.assertEqual(
+            f"{request.scheme}://{rack_proxy_ip}:5248/MAAS",
+            build_metadata_url(
+                request,
+                "/MAAS",
+                node.get_boot_rack_controller(),
+                node=node,
+            ),
+        )