aurweb.spawn: Integrate FastAPI and nginx

aurweb.spawn used to launch only PHP’s built-in server. Now it spawns a
dummy FastAPI application too. Since both stacks spawn their own HTTP
server, aurweb.spawn also spawns nginx as a reverse proxy to mount them
under the same base URL, defined by aur_location in the configuration.
---
 TESTING              |  3 +-
 aurweb/asgi.py       |  8 +++++
 aurweb/spawn.py      | 80 +++++++++++++++++++++++++++++++++++++-------
 conf/config.defaults |  7 ++++
 4 files changed, 85 insertions(+), 13 deletions(-)
 create mode 100644 aurweb/asgi.py

On Sat, 2020-05-30 at 19:41 +0200, Frédéric Mangano-Tarumi wrote:
> aurweb.spawn used to launch only PHP’s built-in server. Now it spawns a
> dummy FastAPI application too. Since both stacks spawn their own HTTP
> server, aurweb.spawn also spawns nginx as a reverse proxy to mount them
> under the same base URL, defined by aur_location in the configuration.
> ---
>  TESTING              |  3 +-
>  aurweb/asgi.py       |  8 +++++
>  aurweb/spawn.py      | 80 +++++++++++++++++++++++++++++++++++++-------
>  conf/config.defaults |  7 ++++
>  4 files changed, 85 insertions(+), 13 deletions(-)
>  create mode 100644 aurweb/asgi.py
> 
> diff --git a/TESTING b/TESTING
> index a5e08cb8..31e3bcbd 100644
> --- a/TESTING
> +++ b/TESTING
> @@ -12,7 +12,8 @@ INSTALL.
>  2) Install the necessary packages:
>  
>      # pacman -S --needed php php-sqlite sqlite words fortune-mod \
> -                         python python-sqlalchemy python-alembic
> +                         python python-sqlalchemy python-alembic \
> +                         python-fastapi uvicorn nginx

Add this to .gitlab-ci.yml too.

>  
>     Ensure to enable the pdo_sqlite extension in php.ini.
>  
> diff --git a/aurweb/asgi.py b/aurweb/asgi.py
> new file mode 100644
> index 00000000..8d3deedc
> --- /dev/null
> +++ b/aurweb/asgi.py
> @@ -0,0 +1,8 @@
> +from fastapi import FastAPI
> +
> +app = FastAPI()
> +
> +
> +@app.get("/sso/")
> +async def hello():
> +    return {"message": "Hello from FastAPI!"}
> diff --git a/aurweb/spawn.py b/aurweb/spawn.py
> index 5fa646b5..df6a2ed5 100644
> --- a/aurweb/spawn.py
> +++ b/aurweb/spawn.py
> @@ -10,8 +10,10 @@ configuration anyway.
>  
>  import atexit
>  import argparse
> +import os
>  import subprocess
>  import sys
> +import tempfile
>  import time
>  import urllib
>  
> @@ -20,6 +22,7 @@ import aurweb.schema
>  
>  
>  children = []
> +temporary_dir = None
>  verbosity = 0
>  
>  
> @@ -35,10 +38,42 @@ class ProcessExceptions(Exception):
>          super().__init__("\n- ".join(messages))
>  
>  
> +def generate_nginx_config():
> +    """
> +    Generate an nginx configuration based on aurweb's configuration.
> +    The file is generated under `temporary_dir`.
> +    Returns the path to the created configuration file.
> +    """
> +    aur_location = aurweb.config.get("options", "aur_location")
> +    aur_location_parts = urllib.parse.urlsplit(aur_location)
> +    config_path = os.path.join(temporary_dir, "nginx.conf")
> +    config = open(config_path, "w")
> +    # We double nginx's braces because they conflict with Python's f-strings.
> +    config.write(f"""
> +        events {{}}
> +        daemon off;
> +        error_log /dev/stderr info;
> +        pid {os.path.join(temporary_dir, "nginx.pid")};
> +        http {{
> +            access_log /dev/stdout;
> +            server {{
> +                listen {aur_location_parts.netloc};
> +                location / {{
> +                    proxy_pass http://{aurweb.config.get("php", "bind_address")};
> +                }}
> +                location /sso {{
> +                    proxy_pass http://{aurweb.config.get("fastapi", "bind_address")};
> +                }}
> +            }}
> +        }}
> +    """)
> +    return config_path
> +
> +
>  def spawn_child(args):
>      """Open a subprocess and add it to the global state."""
>      if verbosity >= 1:
> -        print(f"Spawning {args}", file=sys.stderr)
> +        print(f":: Spawning {args}", file=sys.stderr)
>      children.append(subprocess.Popen(args))
>  
>  
> @@ -52,10 +87,29 @@ def start():
>      if children:
>          return
>      atexit.register(stop)
> -    aur_location = aurweb.config.get("options", "aur_location")
> -    aur_location_parts = urllib.parse.urlsplit(aur_location)
> -    htmldir = aurweb.config.get("options", "htmldir")
> -    spawn_child(["php", "-S", aur_location_parts.netloc, "-t", htmldir])
> +
> +    # Friendly message
> +    ruler = "-" * os.get_terminal_size().columns
> +    print(ruler)
> +    print("Spawing PHP and FastAPI, then nginx as a reverse proxy.")
> +    print("Check out " + aurweb.config.get("options", "aur_location"))
> +    print("Hit ^C to terminate everything.")
> +    print(ruler)

I would have a single print statement and replace ^C with CTRL+C. But
this is just a nitpick.

> +
> +    # PHP
> +    php_address = aurweb.config.get("php", "bind_address")
> +    htmldir = aurweb.config.get("php", "htmldir")
> +    spawn_child(["php", "-S", php_address, "-t", htmldir])
> +
> +    # FastAPI
> +    host, port = aurweb.config.get("fastapi", "bind_address").rsplit(":", 1)
> +    spawn_child(["python", "-m", "uvicorn",
> +                 "--host", host,
> +                 "--port", port,
> +                 "aurweb.asgi:app"])
> +
> +    # nginx
> +    spawn_child(["nginx", "-p", temporary_dir, "-c", generate_nginx_config()])
>  
>  
>  def stop():
> @@ -73,7 +127,7 @@ def stop():
>          try:
>              p.terminate()
>              if verbosity >= 1:
> -                print(f"Sent SIGTERM to {p.args}", file=sys.stderr)
> +                print(f":: Sent SIGTERM to {p.args}", file=sys.stderr)
>          except Exception as e:
>              exceptions.append(e)
>      for p in children:
> @@ -99,9 +153,11 @@ if __name__ == '__main__':
>                          help='increase verbosity')
>      args = parser.parse_args()
>      verbosity = args.verbose
> -    start()
> -    try:
> -        while True:
> -            time.sleep(60)
> -    except KeyboardInterrupt:
> -        stop()
> +    with tempfile.TemporaryDirectory(prefix="aurweb-") as tmpdirname:
> +        temporary_dir = tmpdirname
> +        start()
> +        try:
> +            while True:
> +                time.sleep(60)
> +        except KeyboardInterrupt:
> +            stop()
> diff --git a/conf/config.defaults b/conf/config.defaults
> index 86fe765c..ed495168 100644
> --- a/conf/config.defaults
> +++ b/conf/config.defaults
> @@ -41,9 +41,16 @@ cache = none
>  cache_pkginfo_ttl = 86400
>  memcache_servers = 127.0.0.1:11211
>  
> +[php]
> +; Address PHP should bind when spawned in development mode by aurweb.spawn.
> +bind_address = 127.0.0.1:8081
>  ; Directory containing aurweb's PHP code, required by aurweb.spawn.
>  ;htmldir = /path/to/web/html
>  
> +[fastapi]
> +; Address uvicorn should bind when spawned in development mode by aurweb.spawn.
> +bind_address = 127.0.0.1:8082
> +
>  [ratelimit]
>  request_limit = 4000
>  window_length = 86400

Otherwise, LGTM.

Filipe Laíns

Thanks a lot for the patch; two minor comments inline in addition to
what Filipe mentioned previously (I am fine with ^C instead of CTRL+C by
the way but don't care much either way).

On Sat, 30 May 2020 at 13:41:07, Frédéric Mangano-Tarumi wrote:
> aurweb.spawn used to launch only PHP\u2019s built-in server. Now it spawns a
> dummy FastAPI application too. Since both stacks spawn their own HTTP
> server, aurweb.spawn also spawns nginx as a reverse proxy to mount them
> under the same base URL, defined by aur_location in the configuration.
> ---
>  TESTING              |  3 +-
>  aurweb/asgi.py       |  8 +++++
>  aurweb/spawn.py      | 80 +++++++++++++++++++++++++++++++++++++-------
>  conf/config.defaults |  7 ++++
>  4 files changed, 85 insertions(+), 13 deletions(-)
>  create mode 100644 aurweb/asgi.py
> [...]
> diff --git a/aurweb/asgi.py b/aurweb/asgi.py
> new file mode 100644
> index 00000000..8d3deedc
> --- /dev/null
> +++ b/aurweb/asgi.py
> @@ -0,0 +1,8 @@
> +from fastapi import FastAPI
> +
> +app = FastAPI()
> +
> +
> +@app.get("/sso/")
> +async def hello():
> +    return {"message": "Hello from FastAPI!"}

I know that this is in preparation of the next step, SSO, but can we use
a separate path here, such as /hello/ or /test/? That would cause less
confusion to future readers of this patch (who may not have any context)
and would allow us to decouple the actual SSO patch series from this
patch more easily. For example, that series could start with a patch to
remove this test, and all followup patches would not mention this test
at all.

> diff --git a/conf/config.defaults b/conf/config.defaults
> index 86fe765c..ed495168 100644
> --- a/conf/config.defaults
> +++ b/conf/config.defaults
> @@ -41,9 +41,16 @@ cache = none
>  cache_pkginfo_ttl = 86400
>  memcache_servers = 127.0.0.1:11211
>  
> +[php]
> +; Address PHP should bind when spawned in development mode by aurweb.spawn.
> +bind_address = 127.0.0.1:8081
>  ; Directory containing aurweb's PHP code, required by aurweb.spawn.
>  ;htmldir = /path/to/web/html
>  
> +[fastapi]
> +; Address uvicorn should bind when spawned in development mode by aurweb.spawn.
> +bind_address = 127.0.0.1:8082
> +

Even if the dev-only options are indicated by comments, I am not sure
whether it's a good idea to mix production and development configuration
like this. Can we put them in a separate file or at least a separate
section without too much effort?

Best regards,
Lukas

Lukas Fleischer [2020-05-31 10:16:43 -0400]
> > diff --git a/conf/config.defaults b/conf/config.defaults
> > index 86fe765c..ed495168 100644
> > --- a/conf/config.defaults
> > +++ b/conf/config.defaults
> > @@ -41,9 +41,16 @@ cache = none
> >  cache_pkginfo_ttl = 86400
> >  memcache_servers = 127.0.0.1:11211
> >  
> > +[php]
> > +; Address PHP should bind when spawned in development mode by aurweb.spawn.
> > +bind_address = 127.0.0.1:8081
> >  ; Directory containing aurweb's PHP code, required by aurweb.spawn.
> >  ;htmldir = /path/to/web/html
> >  
> > +[fastapi]
> > +; Address uvicorn should bind when spawned in development mode by aurweb.spawn.
> > +bind_address = 127.0.0.1:8082
> > +
> 
> Even if the dev-only options are indicated by comments, I am not sure
> whether it's a good idea to mix production and development configuration
> like this. Can we put them in a separate file or at least a separate
> section without too much effort?

aurweb.config is quite trivial so we could easily add mechanisms for
handling environments or things like that. However, since these values
are just ignored by production, I don’t think it’s a big deal.

How about creating conf/config.dev with dev-specific options, and
options most developers need to change (disable_http_login, etc.). In
TESTING, we would change 3) to “Copy conf/config.dev to conf/config”.
The downside is that when we add new required options to config.dev,
developers need to update their conf/config manually.

A more powerful option would be to have conf.d/00-defaults.ini,
conf.d/60-dev.ini, and conf.d/90-user.ini. I think most developers are
familiar with conf.d configurations already. Such a switch would break
everyone’s setup though, including production, but the fix is easy.

What do you think?

On Sun, 31 May 2020 at 17:20:45, Frédéric Mangano-Tarumi wrote:
> > Even if the dev-only options are indicated by comments, I am not sure
> > whether it's a good idea to mix production and development configuration
> > like this. Can we put them in a separate file or at least a separate
> > section without too much effort?
> 
> aurweb.config is quite trivial so we could easily add mechanisms for
> handling environments or things like that. However, since these values
> are just ignored by production, I don\u2019t think it\u2019s a big deal.

> How about creating conf/config.dev with dev-specific options, and
> options most developers need to change (disable_http_login, etc.). In
> TESTING, we would change 3) to \u201cCopy conf/config.dev to conf/config\u201d.
> The downside is that when we add new required options to config.dev,
> developers need to update their conf/config manually.

With the current design, you already need to copy the config file to
/etc/aurweb/ on changes or use the AUR_CONFIG environment variable to
point to an alternative path (TESTING uses the latter). So maybe create
a development version of the config file as you suggested, add the
dev-only options only there and update the TESTING instructions to use
that file instead of the production config defaults?