Add upload_max_filesize, post_max_size, and max_execution_time configuration (#259)

Author: weiiwang01

config.yaml

@@ -100,3 +100,19 @@ options:
     description: >
       This setting specifies the duration, in seconds, that pebble will wait for a WordPress health check to complete
       before timing out. Use this setting to adjust the timeout based on expected system performance and conditions
+  upload_max_filesize:
+    type: string
+    default: 2M
+    description: >
+       The maximum size of an uploaded file. https://www.php.net/manual/en/ini.core.php#ini.upload-max-filesize
+  post_max_size:
+    type: string
+    default: 8M
+    description: >
+      Sets max size of post data allowed. https://www.php.net/manual/en/ini.core.php#ini.post-max-size
+  max_execution_time:
+    type: int
+    default: 30
+    description: >
+      This sets the maximum time in seconds a script is allowed to run before it is terminated by the parser.
+      https://www.php.net/manual/en/info.configuration.php#ini.max-execution-time

lib/charms/nginx_ingress_integrator/v0/nginx_route.py

@@ -1,5 +1,5 @@
 # Copyright 2024 Canonical Ltd.
-# Licensed under the Apache2.0, see LICENCE file in charm source for details.
+# Licensed under the Apache2.0. See LICENSE file in charm source for details.
 """Library for the nginx-route relation.
 
 This library contains the require and provide functions for handling
@@ -13,6 +13,7 @@
 
 Other optional arguments include:
 - additional_hostnames
+- backend_protocol
 - limit_rps
 - limit_whitelist
 - max_body_size
@@ -33,12 +34,12 @@
 ```python
 from charms.nginx_ingress_integrator.v0.nginx_route import NginxRouteRequirer
 
-# In your charm's `__init__` method.
+# In your charm's `__init__` method (assuming your app is listening on port 8080).
 require_nginx_route(
     charm=self,
-    service_hostname=self.config["external_hostname"],
+    service_hostname=self.app.name,
     service_name=self.app.name,
-    service_port=80
+    service_port=8080
 )
 
 ```
@@ -51,6 +52,23 @@
 You _must_ require nginx route as part of the `__init__` method
 rather than, for instance, a config-changed event handler, for the relation
 changed event to be properly handled.
+
+In the example above we're setting `service_hostname` (which translates to the
+external hostname for the application when related to nginx-ingress-integrator)
+to `self.app.name` here. This ensures by default the charm will be available on
+the name of the deployed juju application, but can be overridden in a
+production deployment by setting `service-hostname` on the
+nginx-ingress-integrator charm. For example:
+```bash
+juju deploy nginx-ingress-integrator
+juju deploy my-charm
+juju relate nginx-ingress-integrator my-charm:nginx-route
+# The service is now reachable on the ingress IP(s) of your k8s cluster at
+# 'http://my-charm'.
+juju config nginx-ingress-integrator service-hostname='my-charm.example.com'
+# The service is now reachable on the ingress IP(s) of your k8s cluster at
+# 'http://my-charm.example.com'.
+```
 """
 import logging
 import typing
@@ -61,14 +79,14 @@
 import ops.model
 
 # The unique Charmhub library identifier, never change it
-LIBID = "c13d5d639bcd09f8c4f5b195264ed53d"
+LIBID = "3c212b6ed3cf43dfbf9f2e322e634beb"
 
 # Increment this major API version when introducing breaking changes
 LIBAPI = 0
 
 # Increment this PATCH version before using `charmcraft publish-lib` or reset
 # to 0 if you are raising the major API version
-LIBPATCH = 1
+LIBPATCH = 7
 
 __all__ = ["require_nginx_route", "provide_nginx_route"]
 
@@ -101,7 +119,7 @@ class _NginxRouteCharmEvents(ops.charm.CharmEvents):
     nginx_route_broken = ops.framework.EventSource(_NginxRouteBrokenEvent)
 
 
-class _NginxRouteRequirer(ops.framework.Object):
+class NginxRouteRequirer(ops.framework.Object):
     """This class defines the functionality for the 'requires' side of the 'nginx-route' relation.
 
     Hook events observed:
@@ -130,7 +148,7 @@ def __init__(
             self._config_reconciliation,
         )
         # Set default values.
-        self._config: typing.Dict[str, typing.Union[str, int, bool]] = {
+        self.config: typing.Dict[str, typing.Union[str, int, bool]] = {
             "service-namespace": self._charm.model.name,
             **config,
         }
@@ -145,20 +163,25 @@ def _config_reconciliation(self, _event: typing.Any = None) -> None:
             delete_keys = {
                 relation_field
                 for relation_field in relation_app_data
-                if relation_field not in self._config
+                if relation_field not in self.config
             }
             for delete_key in delete_keys:
                 del relation_app_data[delete_key]
-            relation_app_data.update({k: str(v) for k, v in self._config.items()})
+            relation_app_data.update({k: str(v) for k, v in self.config.items()})
 
 
-def require_nginx_route(  # pylint: disable=too-many-locals,too-many-branches
+# C901 is ignored since the method has too many ifs but wouldn't be
+# necessarily good to reduce to smaller methods.
+# E501: line too long
+def require_nginx_route(  # pylint: disable=too-many-locals,too-many-branches,too-many-arguments # noqa: C901,E501
     *,
     charm: ops.charm.CharmBase,
     service_hostname: str,
     service_name: str,
     service_port: int,
     additional_hostnames: typing.Optional[str] = None,
+    backend_protocol: typing.Optional[str] = None,
+    enable_access_log: typing.Optional[bool] = None,
     limit_rps: typing.Optional[int] = None,
     limit_whitelist: typing.Optional[str] = None,
     max_body_size: typing.Optional[int] = None,
@@ -172,7 +195,7 @@ def require_nginx_route(  # pylint: disable=too-many-locals,too-many-branches
     session_cookie_max_age: typing.Optional[int] = None,
     tls_secret_name: typing.Optional[str] = None,
     nginx_route_relation_name: str = "nginx-route",
-) -> None:
+) -> NginxRouteRequirer:
     """Set up nginx-route relation handlers on the requirer side.
 
     This function must be invoked in the charm class constructor.
@@ -187,6 +210,10 @@ def require_nginx_route(  # pylint: disable=too-many-locals,too-many-branches
             option via relation.
         additional_hostnames: configure Nginx ingress integrator
             additional-hostnames option via relation, optional.
+        backend_protocol: configure Nginx ingress integrator
+            backend-protocol option via relation, optional.
+        enable_access_log: configure Nginx ingress
+            nginx.ingress.kubernetes.io/enable-access-log option.
         limit_rps: configure Nginx ingress integrator limit-rps
             option via relation, optional.
         limit_whitelist: configure Nginx ingress integrator
@@ -215,6 +242,9 @@ def require_nginx_route(  # pylint: disable=too-many-locals,too-many-branches
         nginx_route_relation_name: Specifies the relation name of
             the relation handled by this requirer class. The relation
             must have the nginx-route interface.
+
+    Returns:
+        the NginxRouteRequirer.
     """
     config: typing.Dict[str, typing.Union[str, int, bool]] = {}
     if service_hostname is not None:
@@ -225,6 +255,10 @@ def require_nginx_route(  # pylint: disable=too-many-locals,too-many-branches
         config["service-port"] = service_port
     if additional_hostnames is not None:
         config["additional-hostnames"] = additional_hostnames
+    if backend_protocol is not None:
+        config["backend-protocol"] = backend_protocol
+    if enable_access_log is not None:
+        config["enable-access-log"] = "true" if enable_access_log else "false"
     if limit_rps is not None:
         config["limit-rps"] = limit_rps
     if limit_whitelist is not None:
@@ -250,7 +284,7 @@ def require_nginx_route(  # pylint: disable=too-many-locals,too-many-branches
     if tls_secret_name is not None:
         config["tls-secret-name"] = tls_secret_name
 
-    _NginxRouteRequirer(
+    return NginxRouteRequirer(
         charm=charm, config=config, nginx_route_relation_name=nginx_route_relation_name
     )
 
@@ -297,6 +331,9 @@ def _on_relation_changed(self, event: ops.charm.RelationChangedEvent) -> None:
 
         Args:
             event: Event triggering the relation-changed hook for the relation.
+
+        Raises:
+            RuntimeError: if _on_relation changed is triggered by a broken relation.
         """
         # `self.unit` isn't available here, so use `self.model.unit`.
         if not self._charm.model.unit.is_leader():
@@ -370,8 +407,13 @@ def provide_nginx_route(
         on_nginx_route_broken: Callback function for the nginx-route-broken event.
         nginx_route_relation_name: Specifies the relation name of the relation handled by this
             provider class. The relation must have the nginx-route interface.
+
+    Raises:
+        RuntimeError: If provide_nginx_route was invoked twice with
+            the same nginx-route relation name
     """
-    if __provider_references.get(charm, {}).get(nginx_route_relation_name) is not None:
+    ref_dict: typing.Dict[str, typing.Any] = __provider_references.get(charm, {})
+    if ref_dict.get(nginx_route_relation_name) is not None:
         raise RuntimeError(
             "provide_nginx_route was invoked twice with the same nginx-route relation name"
         )

src/charm.py

@@ -55,6 +55,7 @@ class _ReplicaRelationNotReady(Exception):
 
     _WP_CONFIG_PATH = "/var/www/html/wp-config.php"
     _WP_UPLOADS_PATH = "/var/www/html/wp-content/uploads"
+    _PHP_INI_PATH = "/etc/php/7.4/apache2/php.ini"
     _CONTAINER_NAME = "wordpress"
     _SERVICE_NAME = "wordpress"
     _WORDPRESS_USER = "_daemon_"
@@ -769,7 +770,7 @@ def _init_pebble_layer(self):
                     "override": "replace",
                     "level": "alive",
                     "http": {"url": "http://localhost"},
-                    "period": f"{max(10, health_check_timeout*2)}s",
+                    "period": f"{max(10, health_check_timeout * 2)}s",
                     "timeout": f"{health_check_timeout}s",
                 },
             },
@@ -855,6 +856,37 @@ def _push_wp_config(self, wp_config: str) -> None:
             permissions=0o600,
         )
 
+    def _gen_php_ini(self) -> str:
+        """Generate the content of the php.ini based on the charm configuration.
+
+        Returns:
+            the content of the php.ini file.
+        """
+        current = self._current_php_ini()
+        php_configs = ["upload_max_filesize", "post_max_size", "max_execution_time"]
+        new = current
+        for php_config in php_configs:
+            search = f"^{php_config}\\s*=\\s*[^\\s]+"
+            php_config_value = self.config[php_config]
+            new = re.sub(search, f"{php_config} = {php_config_value}", new, flags=re.MULTILINE)
+        return new
+
+    def _current_php_ini(self) -> str:
+        """Retrieve the current version of the php.ini from the server.
+
+        Returns:
+            The content of the current php.ini file.
+        """
+        return self._container().pull(self._PHP_INI_PATH).read()
+
+    def _update_php_ini(self, config: str) -> None:
+        """Update the content of the php.ini file.
+
+        Args:
+            config: the content of php.ini file.
+        """
+        self._container().push(self._PHP_INI_PATH, source=config, permissions=0o644)
+
     def _core_reconciliation(self) -> None:
         """Reconciliation process for the WordPress core services, returns True if successful.
 
@@ -866,6 +898,9 @@ def _core_reconciliation(self) -> None:
         It will check if the current wp-config.php file matches the desired config.
         If not, update the wp-config.php file.
 
+        It will check if the current php.ini file matches the desired config.
+        If not, update the php.ini file.
+
         It will also check if WordPress is installed (WordPress-related tables exist in db).
         If not, install WordPress (create WordPress required tables in db).
 
@@ -890,6 +925,11 @@ def _core_reconciliation(self) -> None:
             logger.info("Changes detected in wp-config.php, updating")
             self._stop_server()
             self._push_wp_config(wp_config)
+        php_ini = self._gen_php_ini()
+        if php_ini != self._current_php_ini():
+            logger.info("Changes detected in php.ini, updating")
+            self._stop_server()
+            self._update_php_ini(php_ini)
         self._start_server()
         logger.info("Wait until the pebble container exists")
 

tests/integration/test_core.py

@@ -44,6 +44,39 @@ async def test_wordpress_functionality(wordpress: WordpressApp):
         )
 
 
+@pytest.mark.usefixtures("prepare_mysql")
+async def test_change_upload_limit(wordpress: WordpressApp):
+    """
+    arrange: after WordPress charm has been deployed and db relation established.
+    act: change upload limit related settings.
+    assert: upload limit change should be reflected in the upload page.
+    """
+    await wordpress.set_config({"upload_max_filesize": "16M"})
+    await wordpress.model.wait_for_idle(status="active")
+    password = await wordpress.get_default_admin_password()
+    for unit_ip in await wordpress.get_unit_ips():
+        wordpress_client = WordpressClient(
+            host=unit_ip,
+            username="admin",
+            password=password,
+            is_admin=True,
+        )
+        text = wordpress_client.get_post(f"http://{unit_ip}/wp-admin/upload.php")
+        # upload limit = min(upload_max_filesize, post_max_size)
+        assert "Maximum upload file size: 8 MB" in text
+    await wordpress.set_config({"post_max_size": "16M"})
+    await wordpress.model.wait_for_idle(status="active")
+    for unit_ip in await wordpress.get_unit_ips():
+        wordpress_client = WordpressClient(
+            host=unit_ip,
+            username="admin",
+            password=password,
+            is_admin=True,
+        )
+        text = wordpress_client.get_post(f"http://{unit_ip}/wp-admin/upload.php")
+        assert "Maximum upload file size: 16 MB" in text
+
+
 @pytest.mark.usefixtures("prepare_mysql", "prepare_swift")
 async def test_openstack_object_storage_plugin(
     wordpress: WordpressApp,

tests/unit/test_charm.py

@@ -7,6 +7,7 @@
 
 import json
 import secrets
+import textwrap
 import typing
 import unittest.mock
 
@@ -1085,3 +1086,27 @@ def test_wordpress_promtail_config(harness: ops.testing.Harness):
         ],
         "server": {"grpc_listen_port": 9095, "http_listen_port": 9080},
     }
+
+
+def test_php_ini(
+    harness: ops.testing.Harness,
+    setup_replica_consensus: typing.Callable[[], dict],
+):
+    """
+    arrange: after WordPress application unit consensus has been reached.
+    act: update php.ini related charm configurations.
+    assert: updated php.ini should be valid.
+    """
+    setup_replica_consensus()
+    charm: WordpressCharm = typing.cast(WordpressCharm, harness.charm)
+    harness.update_config(
+        {"upload_max_filesize": "16M", "post_max_size": "32M", "max_execution_time": 60}
+    )
+    assert charm._gen_php_ini() == textwrap.dedent(
+        """
+        [PHP]
+        post_max_size = 32M
+        upload_max_filesize = 16M
+        max_execution_time = 60
+        """
+    )

tests/unit/wordpress_mock.py

@@ -8,6 +8,7 @@
 import io
 import json
 import re
+import textwrap
 import typing
 import unittest.mock
 
@@ -357,7 +358,17 @@ def __init__(
             wordpress_database_mock: An instance of the WordPress database mock system.
         """
         self.original_pebble = None
-        self.fs: typing.Dict[str, str] = {"/proc/mounts": ""}
+        self.fs: typing.Dict[str, str] = {
+            "/proc/mounts": "",
+            WordpressCharm._PHP_INI_PATH: textwrap.dedent(
+                """
+                [PHP]
+                post_max_size = 8M
+                upload_max_filesize = 2M
+                max_execution_time = 30
+                """
+            ),
+        }
         self._wordpress_database_mock = wordpress_database_mock
         self.installed_plugins = set(WordpressCharm._WORDPRESS_DEFAULT_PLUGINS)
         self.installed_themes = set(WordpressCharm._WORDPRESS_DEFAULT_THEMES)