Merge ~ines-almeida/txpkgupload:fix-passive-ftp-through-haproxy into txpkgupload:master

[Ines Almeida]

Update txpkgupload + its charm to allow FTP through haproxy

The proxy's address is needed for passive twisted FTP requests in our txpkgupload so that the host address we send to the client is the proxy address (which will redirect the upload to the txpkgupload unit) instead of the txpkgupload unit address itself (which the client won't have access to)

Tested locally and in qastaging

[Ines Almeida (ines-almeida)]

Depends on: https://code.launchpad.net/~ines-almeida/txpkgupload/+git/txpkgupload/+merge/453441

[Colin Watson (cjwatson)]

review: Needs Fixing

[Ines Almeida (ines-almeida)]

Comments addressed

[Ines Almeida (ines-almeida)]

After this gets merged, I will create a MP to the mojo specs updating the txpkgupload build_label, re-deploy and later another one to update the charm version, and re-deploy again.

[Colin Watson (cjwatson)]

review: Approve

[Ines Almeida (ines-almeida)]

Thanks for the review!

[Colin Watson (cjwatson)]

review: Approve

diff --git a/charm/txpkgupload/config.yaml b/charm/txpkgupload/config.yaml
index b53064e..c4fb6bf 100644
--- a/charm/txpkgupload/config.yaml
+++ b/charm/txpkgupload/config.yaml
@@ -3,6 +3,12 @@ options:
     type: int
     default: 2221
     description: The port to run the FTP server on.
+  haproxy_ipv4_address:
+    type: string
+    description: >
+      The IPv4 address of the haproxy unit related to this charm. Only
+      one haproxy unit is expected.
+    default:
   haproxy_server_options:
     type: string
     description: >
diff --git a/charm/txpkgupload/reactive/txpkgupload.py b/charm/txpkgupload/reactive/txpkgupload.py
index 962ff92..d219464 100644
--- a/charm/txpkgupload/reactive/txpkgupload.py
+++ b/charm/txpkgupload/reactive/txpkgupload.py
@@ -192,6 +192,16 @@ def configure_loadbalancer():
     hookenv.log("Configuring loadbalancer for txpkgupload")
     config = hookenv.config()
 
+    if config.get("haproxy_ipv4_address"):
+        hookenv.log(
+            "Waiting for 'haproxy_ipv4_address' value to be configured"
+        )
+        hookenv.status_set(
+            "blocked",
+            "Blocked until 'haproxy_ipv4_address' value is configured",
+        )
+        return
+
     try:
         service_options_ftp = yaml.safe_load(
             config["haproxy_service_options_ftp"]
@@ -266,7 +276,6 @@ def configure_loadbalancer():
         },
     ]
     services_yaml = yaml.dump(services)
-
     for rel in hookenv.relations_of_type("loadbalancer"):
         hookenv.relation_set(rel["__relid__"], services=services_yaml)
 
diff --git a/charm/txpkgupload/templates/txpkgupload_config.yaml.j2 b/charm/txpkgupload/templates/txpkgupload_config.yaml.j2
index eb5b224..4b02352 100644
--- a/charm/txpkgupload/templates/txpkgupload_config.yaml.j2
+++ b/charm/txpkgupload/templates/txpkgupload_config.yaml.j2
@@ -19,3 +19,5 @@ temp_dir: "{{ base_dir }}/tmp-incoming"
 
 min_passive_port: {{ min_passive_port }}
 max_passive_port: {{ max_passive_port }}
+
+proxy_ipv4_address: {{ haproxy_ipv4_address }}
diff --git a/src/txpkgupload/plugin.py b/src/txpkgupload/plugin.py
index dbacac5..a1df540 100644
--- a/src/txpkgupload/plugin.py
+++ b/src/txpkgupload/plugin.py
@@ -136,6 +136,10 @@ class Config(Schema):
     min_passive_port = Int(if_missing=None)
     max_passive_port = Int(if_missing=None)
 
+    # IPv4 address of the proxy between the service and the client, if it
+    # exists.
+    proxy_ipv4_address = String(if_missing=None)
+
     @classmethod
     def parse(cls, stream):
         """Load a YAML configuration from `stream` and validate."""
@@ -251,6 +255,7 @@ class PkgUploadServiceMaker:
             idle_timeout=config["idle_timeout"],
             min_passive_port=config.get("min_passive_port"),
             max_passive_port=config.get("max_passive_port"),
+            proxy_ipv4_address=config.get("proxy_ipv4_address"),
         )
         ftp_service.name = "ftp"
         ftp_service.setServiceParent(services)
diff --git a/src/txpkgupload/tests/test_plugin.py b/src/txpkgupload/tests/test_plugin.py
index ac07566..20740d6 100644
--- a/src/txpkgupload/tests/test_plugin.py
+++ b/src/txpkgupload/tests/test_plugin.py
@@ -113,6 +113,7 @@ class TestConfig(TestCase):
             "idle_timeout": 3600,
             "max_passive_port": None,
             "min_passive_port": None,
+            "proxy_ipv4_address": None,
             "oops": {
                 "directory": "",
                 "reporter": "PKGUPLOAD",
diff --git a/src/txpkgupload/twistedftp.py b/src/txpkgupload/twistedftp.py
index 4c019b6..bfc195b 100644
--- a/src/txpkgupload/twistedftp.py
+++ b/src/txpkgupload/twistedftp.py
@@ -199,6 +199,41 @@ class FTPWithEPSV(ftp.FTP):
             "No port available in range %s" %
             (self.passivePortRange,))
 
+    def getHostAddress(self):
+        """
+        Returns IPv4 host address to be sent back the client during a Passive
+        FTP request.
+
+        If there is a proxy between service and the client, return the proxy
+        address. Else return the address of the unit txpkgupload is running on.
+
+        XXX ines-almeida 2023-10-13 ideally, we wouldn't need this method.
+        If we decide to use the `haproxy` wrapper (see
+        twisted.protocols.haproxy) then `self.transport.getHost().host`
+        should automatically return the proxy address.
+        """
+        if self.factory.proxy_ipv4_address:
+            host = self.factory.proxy_ipv4_address
+        else:
+            host = self.transport.getHost().host
+
+        # Returns either an IPv4Address or an IPv6Address
+        address = ipaddress.ip_address(six.ensure_text(host))
+
+        if isinstance(address, ipaddress.IPv4Address):
+            return str(address)
+
+        if not address.ipv4_mapped:
+            # There's no standard defining the behaviour of PASV with
+            # IPv6, so just claim it as unimplemented.  (Some servers
+            # return something like '0,0,0,0' in the host part of the
+            # response in order that at least clients that ignore the
+            # host part can work, and if it becomes necessary then we
+            # could do that too.)
+            return defer.fail(ftp.CmdNotImplementedError('PASV'))
+
+        return str(address.ipv4_mapped)
+
     def ftp_PASV(self):
         """
         Request for a passive connection
@@ -215,24 +250,7 @@ class FTPWithEPSV(ftp.FTP):
             return defer.fail(ftp.BadCmdSequenceError(
                 'may not send PASV after EPSV ALL'))
 
-        host = self.transport.getHost().host
-        try:
-            address = ipaddress.IPv6Address(six.ensure_text(host))
-        except ipaddress.AddressValueError:
-            pass
-        else:
-            if address.ipv4_mapped is not None:
-                # IPv4-mapped addresses are usable, but we need to make sure
-                # they're encoded as IPv4 in the response.
-                host = str(address.ipv4_mapped)
-            else:
-                # There's no standard defining the behaviour of PASV with
-                # IPv6, so just claim it as unimplemented.  (Some servers
-                # return something like '0,0,0,0' in the host part of the
-                # response in order that at least clients that ignore the
-                # host part can work, and if it becomes necessary then we
-                # could do that too.)
-                return defer.fail(ftp.CmdNotImplementedError('PASV'))
+        host = self.getHostAddress()
 
         # if we have a DTP port set up, lose it.
         if self.dtpFactory is not None:
@@ -357,6 +375,16 @@ class FTPWithEPSV(ftp.FTP):
         return self.dtpFactory.deferred.addCallbacks(connected, connFailed)
 
 
+class FTPFactoryWithProxy(ftp.FTPFactory):
+    # XXX ines-almeida 2023-10-13 ideally, we wouldn't need this class.
+    # If we decide to use the `haproxy` wrapper (see twisted.protocols.haproxy)
+    # we wouldn't need to pass the `proxy_ipv4_address` address.
+    # For future reference, inserting "haproxy:" to the start of `strport`
+    # FTPServiceFactory.makeFTPService() wraps the endpoint automatically.
+
+    proxy_ipv4_address = None
+
+
 class FTPServiceFactory(service.Service):
     """A factory that makes an `FTPService`"""
 
@@ -368,11 +396,12 @@ class FTPServiceFactory(service.Service):
         idle_timeout,
         min_passive_port,
         max_passive_port,
+        proxy_ipv4_address=None,
     ):
         realm = FTPRealm(root, temp_dir)
         portal = Portal(realm)
         portal.registerChecker(AccessCheck())
-        factory = ftp.FTPFactory(portal)
+        factory = FTPFactoryWithProxy(portal)
 
         factory.tld = root
         factory.protocol = FTPWithEPSV
@@ -384,12 +413,20 @@ class FTPServiceFactory(service.Service):
                 min_passive_port, max_passive_port
             )
 
+        factory.proxy_ipv4_address = proxy_ipv4_address
+
         self.ftpfactory = factory
         self.portno = port
 
     @staticmethod
     def makeFTPService(
-        port, root, temp_dir, idle_timeout, min_passive_port, max_passive_port
+        port,
+        root,
+        temp_dir,
+        idle_timeout,
+        min_passive_port,
+        max_passive_port,
+        proxy_ipv4_address=None,
     ):
         if port.isdigit():
             strport = "tcp6:%s" % port
@@ -402,5 +439,6 @@ class FTPServiceFactory(service.Service):
             idle_timeout,
             min_passive_port,
             max_passive_port,
+            proxy_ipv4_address,
         )
         return strports.service(strport, factory.ftpfactory)