launchpad-reviewers team mailing list archive

[Ines Almeida <mp+453543@xxxxxxxxxxxxxxxxxx>]

Ines Almeida has proposed merging ~ines-almeida/txpkgupload:fix-passive-ftp-through-haproxy into txpkgupload:master with ~ines-almeida/txpkgupload:allow-setting-passive-port-range as a prerequisite.

Commit message:
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)

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ines-almeida/txpkgupload/+git/txpkgupload/+merge/453543

Tested locally and in qastaging
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/txpkgupload:fix-passive-ftp-through-haproxy into txpkgupload:master.

diff --git a/charm/txpkgupload/reactive/txpkgupload.py b/charm/txpkgupload/reactive/txpkgupload.py
index f3d4f71..4bf7384 100644
--- a/charm/txpkgupload/reactive/txpkgupload.py
+++ b/charm/txpkgupload/reactive/txpkgupload.py
@@ -186,13 +186,44 @@ def deconfigure():
     clear_flag("service.configured")
 
 
+def get_loadbalancer_relation():
+    relations = hookenv.relations_of_type("loadbalancer")
+    if len(relations) == 0:
+        hookenv.log("No load balancer relation found.")
+        return None
+    elif len(relations) > 1:
+        # XXX ines-almeida 2023-10-13: We currently only support one load
+        # balancer, particularly because txpkgupload is only expecting one
+        # proxy address. We might want to allow more in the future, but the
+        # `src/txpkgupload/twistedftp.py` file will need rework for that.
+        hookenv.log("Found more than one load balancer.")
+        hookenv.status_set(
+            "blocked", "Txpkgupload should only have one load balancer."
+        )
+        return None
+
+    rel = relations[0]
+
+    # If there is a load balancer relation, then we need to re-configure
+    # txpkgupload to contain the load balancer's address
+    config = get_config()
+    config["proxy_address"] = rel["public-address"]
+    configure_txpkgupload(config)
+
+    return rel
+
+
 @when("config.changed")
 @when("loadbalancer.available", "service.configured")
 @when_not("txpkgupload.loadbalancer.configured")
 def configure_loadbalancer():
     hookenv.log("Configuring loadbalancer for txpkgupload")
-    config = hookenv.config()
 
+    rel = get_loadbalancer_relation()
+    if not rel:
+        return
+
+    config = hookenv.config()
     try:
         service_options_ftp = yaml.safe_load(
             config["haproxy_service_options_ftp"]
@@ -266,9 +297,7 @@ def configure_loadbalancer():
         },
     ]
     services_yaml = yaml.dump(services)
-
-    for rel in hookenv.relations_of_type("loadbalancer"):
-        hookenv.relation_set(rel["__relid__"], services=services_yaml)
+    hookenv.relation_set(rel["__relid__"], services=services_yaml)
 
     hookenv.log("Txpkgupload loadbalancer configured")
     set_flag("txpkgupload.loadbalancer.configured")
diff --git a/charm/txpkgupload/templates/txpkgupload_config.yaml.j2 b/charm/txpkgupload/templates/txpkgupload_config.yaml.j2
index eb5b224..8a1a34c 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_address: {{ proxy_address }}
diff --git a/src/txpkgupload/plugin.py b/src/txpkgupload/plugin.py
index dbacac5..2a02165 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)
 
+    # If there is a proxy between the service and the client, this will be
+    # needed for Passive FTP
+    proxy_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_address=config.get("proxy_address"),
         )
         ftp_service.name = "ftp"
         ftp_service.setServiceParent(services)
diff --git a/src/txpkgupload/twistedftp.py b/src/txpkgupload/twistedftp.py
index 4c019b6..f266d6e 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 for Passive FTP mode, for the
+        client to make the request to send data.
+
+        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 ftp.protocols.haproxy)
+        # then `self.transport.getHost().host` should automatically return
+        # the proxy address.
+        """
+        if self.factory.proxy_address:
+            host = self.factory.proxy_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 ftp.protocols.haproxy)
+    # we wouldn't need to pass the `proxy_address` address.
+    # For future reference, inserting "haproxy:" to the start of `strport`
+    # FTPServiceFactory.makeFTPService() wraps the endpoint automatically.
+
+    proxy_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_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,21 @@ class FTPServiceFactory(service.Service):
                 min_passive_port, max_passive_port
             )
 
+        if proxy_address:
+            factory.proxy_address = proxy_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_address=None,
     ):
         if port.isdigit():
             strport = "tcp6:%s" % port
@@ -402,5 +440,6 @@ class FTPServiceFactory(service.Service):
             idle_timeout,
             min_passive_port,
             max_passive_port,
+            proxy_address,
         )
         return strports.service(strport, factory.ftpfactory)