fix: main caddy service should send MFE requests to the mfe containers

[jfavellar90]

Since Nginx service and containers were removed in overhangio/tutor@e19f334, MFE traffic should be routed from caddy main service to caddy process running inside mfe containers.

[regisb]

Thanks for the PR @jfavellar90! Since this change is only required in the nightly branch, could you please rebase your branch?

Also, the "caddyfile" patch should probably look like this: (to be verified)

{{ MFE_HOST }}{{ port }} {
    import proxy "mfe:8000"
}

[jfavellar90]

@regisb I already rebased the branch over the nightly branch. When trying the change you proposed to the "caddyfile" patch:

{{ MFE_HOST }}{{ port }} {
    import proxy "mfe:8000"
}

It fails when running tutor config save since port variable is not in the context of that patch rendering. The variable is defined here in the main Caddyfile. is this error expected?

On the other hand, port 8000 is not open in the MFE docker compose definition. Should we open it and serve the traffic through or use the 80 port (already open because of caddy service running in the MFE container I think)?

[regisb]

It fails when running tutor config save since port variable is not in the context of that patch rendering.

This is very annoying. I created this port variable precisely to make it easy for plugin developers to add their own patches to the Caddyfile (see this pr).

The problem is that currently patches are rendered outside of the scope of the parent template. We could change the patch function to something like:

def patch(self, name: str, separator: str = "\n", suffix: str = "") -> str:
    patches = [patch.strip() for _, patch in plugins.iter_patches(self.config, name)]
    rendered = separator.join(patches)
    if rendered:
        rendered += suffix
    return rendered

And then patches would not be rendered outside of the parent context. We would lose the error message in case the plugin patch is invalid. But, more importantly, we would also lose the ability to have patches that themselves include patches (so called "recursive patches"). I verified the latter problem with the following unit test:

def test_recursive_patching(self) -> None:
    patches = {
        "1": {
            "plugin1": "{{ patch('2') }}",
        },
        "2": {"plugin2": "abcd"},
    }

    def mock_iter_patches(config, name):
        yield from patches[name].items()

    with patch.object(env.plugins, "iter_patches", new=mock_iter_patches):
        rendered = env.render_str({}, '{{ patch("1") }}')
    self.assertEqual("abcd", rendered)

Currently, this test passes. But with the proposed change, I get the following failure:

======================================================================
FAIL: test_recursive_patching (test_env.EnvTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/regis/projets/overhang/repos/overhang/tutor/tests/test_env.py", line 129, in test_recursive_patching
    self.assertEqual("abcd", rendered)
AssertionError: 'abcd' != "{{ patch('2') }}"
- abcd
+ {{ patch('2') }}

Another option is to create a global CADDY_SITE_PORT = "{% if not ENABLE_HTTPS or not ENABLE_WEB_PROXY %}:80{% endif %}" variable. But I'm reluctant to create a new variable for such a specific scenario.

Another option is to pass the port as an environment variable, and then use it as {{ MFE_HOST }}{$default_site_port}. According to the Caddy docs and my experiments, this should work.

I'll push this fix right now in the nightly and maple branches, so that you can update this PR.

On the other hand, port 8000 is not open in the MFE docker compose definition. Should we open it and serve the traffic through or use the 80 port (already open because of caddy service running in the MFE container I think)?

You don't need to open the port in docker-compose. Port 8002 (and not 8000 as I wrote earlier) is open inside the docker-compose (and k8s) network.

[regisb]

Here is the commit in the Maple branch: overhangio/tutor@72baae0

[regisb]

Fixed by this commit. Sorry to close this @jfavellar90 but I needed this change for local testing.