Merge ~ines-almeida/txpkgupload:allow-setting-passive-port-range into txpkgupload:master

[Ines Almeida]

Add port range config to txpkgupload charm

Also added a small fix to the txpkgupload code that was missing in the last MP.

[Ines Almeida (ines-almeida)]

This is related to: https://code.launchpad.net/~ines-almeida/txpkgupload/+git/txpkgupload/+merge/452960

[Colin Watson (cjwatson)]

Just bear in mind that if you're changing the charm and the payload at the same time, it's usually worth making sure that the payload is rolled out first in any existing environments so that you don't run into upgrade problems.

review: Approve

[Otto Co-Pilot (otto-copilot)]

Running landing tests failed
https://jenkins.ols.canonical.com/online-services/job/txpkgupload/141/

7f74fec... by Ines Almeida on 2023-10-13

Update unit tests

diff --git a/charm/txpkgupload/config.yaml b/charm/txpkgupload/config.yaml
index 4fa1ef3..b53064e 100644
--- a/charm/txpkgupload/config.yaml
+++ b/charm/txpkgupload/config.yaml
@@ -5,8 +5,10 @@ options:
     description: The port to run the FTP server on.
   haproxy_server_options:
     type: string
-    description: Options to add to HAProxy "server" lines.
-    default: check inter 5000 rise 2 fall 5 maxconn 16
+    description: >
+      Options to add to HAProxy "server" lines. If "{port}" is part of the
+      value, it will be replaced by the 'ftp_port'/'sftp_port'.
+    default: check port {port} inter 5000 rise 2 fall 5 maxconn 16
   haproxy_service_options_ftp:
     type: string
     description: HAProxy options for the FTP port.
@@ -27,6 +29,18 @@ options:
       Hosts that should be allowed to rsync logs.  Note that this relies on
       the nrpe subordinate charm to set up /etc/rsyncd.conf properly.
     default: ""
+  min_passive_port:
+    type: int
+    default:
+    description: > 
+      The minimum port number possible to use for passive FTP. This config will
+      only take effect if `max_passive_port` is also set.
+  max_passive_port:
+    type: int
+    default:
+    description: > 
+      The maximum port number possible to use for passive FTP. This config will
+      only take effect if `min_passive_port` is also set.
   oops_prefix:
     type: string
     default: local
diff --git a/charm/txpkgupload/reactive/txpkgupload.py b/charm/txpkgupload/reactive/txpkgupload.py
index 3789135..962ff92 100644
--- a/charm/txpkgupload/reactive/txpkgupload.py
+++ b/charm/txpkgupload/reactive/txpkgupload.py
@@ -196,6 +196,7 @@ def configure_loadbalancer():
         service_options_ftp = yaml.safe_load(
             config["haproxy_service_options_ftp"]
         )
+        service_options_ftp = list(service_options_ftp)
     except yaml.YAMLError:
         hookenv.log("Could not parse haproxy_service_options_ftp YAML")
         hookenv.status_set(
@@ -206,6 +207,7 @@ def configure_loadbalancer():
         service_options_sftp = yaml.safe_load(
             config["haproxy_service_options_sftp"]
         )
+        service_options_sftp = list(service_options_sftp)
     except yaml.YAMLError:
         hookenv.log("Could not parse haproxy_service_options_sftp YAML")
         hookenv.status_set(
@@ -216,32 +218,49 @@ def configure_loadbalancer():
 
     unit_name = hookenv.local_unit().replace("/", "-")
     unit_ip = hookenv.unit_private_ip()
+
+    # For passive FTP transfer, we want our haproxy to bind to not only the
+    # usual FTP/SFTP ports but also to higher ports
+    if config.get("min_passive_port") and config.get("max_passive_port"):
+        extra_ports = (
+            f"bind 0.0.0.0:"
+            f"{config['min_passive_port']}-{config['max_passive_port']}"
+        )
+        service_options_ftp.append(extra_ports)
+        service_options_sftp.append(extra_ports)
+
+    ftp_port = config["ftp_port"]
+    sftp_port = config["sftp_port"]
+
+    # Note that we don't send specific ports for the services.servers list.
+    # Not setting a port makes the haproxy use the same port used in the
+    # client's request.
     services = [
         {
             "service_name": "txpkgupload-ftp",
-            "service_port": config["ftp_port"],
+            "service_port": ftp_port,
             "service_host": "0.0.0.0",
-            "service_options": list(service_options_ftp),
+            "service_options": service_options_ftp,
             "servers": [
                 [
                     f"ftp_{unit_name}",
                     unit_ip,
-                    config["ftp_port"],
-                    server_options,
+                    "",
+                    server_options.replace("{port}", str(ftp_port)),
                 ]
             ],
         },
         {
             "service_name": "txpkgupload-sftp",
-            "service_port": config["sftp_port"],
+            "service_port": sftp_port,
             "service_host": "0.0.0.0",
-            "service_options": list(service_options_sftp),
+            "service_options": service_options_sftp,
             "servers": [
                 [
                     f"sftp_{unit_name}",
                     unit_ip,
-                    config["sftp_port"],
-                    server_options,
+                    "",
+                    server_options.replace("{port}", str(sftp_port)),
                 ]
             ],
         },
diff --git a/charm/txpkgupload/templates/txpkgupload_config.yaml.j2 b/charm/txpkgupload/templates/txpkgupload_config.yaml.j2
index ea1368c..eb5b224 100644
--- a/charm/txpkgupload/templates/txpkgupload_config.yaml.j2
+++ b/charm/txpkgupload/templates/txpkgupload_config.yaml.j2
@@ -17,3 +17,5 @@ fsroot: "{{ fsroot }}"
 
 temp_dir: "{{ base_dir }}/tmp-incoming"
 
+min_passive_port: {{ min_passive_port }}
+max_passive_port: {{ max_passive_port }}
diff --git a/src/txpkgupload/plugin.py b/src/txpkgupload/plugin.py
index 32ede15..dbacac5 100644
--- a/src/txpkgupload/plugin.py
+++ b/src/txpkgupload/plugin.py
@@ -132,6 +132,10 @@ class Config(Schema):
     # same filesystem as fsroot.
     temp_dir = String(if_missing=None)
 
+    # Range of ports for Passive FTP
+    min_passive_port = Int(if_missing=None)
+    max_passive_port = Int(if_missing=None)
+
     @classmethod
     def parse(cls, stream):
         """Load a YAML configuration from `stream` and validate."""
diff --git a/src/txpkgupload/tests/test_plugin.py b/src/txpkgupload/tests/test_plugin.py
index bb5ee29..ac07566 100644
--- a/src/txpkgupload/tests/test_plugin.py
+++ b/src/txpkgupload/tests/test_plugin.py
@@ -111,6 +111,8 @@ class TestConfig(TestCase):
                 "port": "2121",
                 },
             "idle_timeout": 3600,
+            "max_passive_port": None,
+            "min_passive_port": None,
             "oops": {
                 "directory": "",
                 "reporter": "PKGUPLOAD",