openedx / wg-devops

Issue repository for the DevOps Working Group
1 stars 2 forks source link

Support `npm start` on host for MFE development #25

Open kdmccormick opened 2 years ago

kdmccormick commented 2 years ago

Background

There are ~9 MFEs enabled by default in tutor-mfe now, and that number grows every release. In prod, all MFEs share one container, so it's not problem. In dev, though, we spin up one container per MFE. This consumes an absurd number of resources, and probably isn't necessary since MFEs seem to run fine on everyone's maching with a simple npm clean-install && npm start.

Here's a good illustration of the problem: https://discuss.openedx.org/t/after-installing-a-tutor-nightly-system-is-working-slow-which-container-can-i-stop-temporary/10681

In addition to resource consumption, making changes to MFE code in Docker containers through bind-mounts is unnecessarily complex. It'd be better if developers could just edit MFE code on their host and see the changes manifest immediately.

Tasks

Prototype an overhaul of tutor-mfe's dev mode that solves this problem, something along the lines of:

Rationale

Goals:

Notes

Read @brian-smith-tcril 's comment here: https://github.com/openedx/wg-developer-experience/issues/154#issuecomment-1380912822

brian-smith-tcril commented 1 year ago
brian-smith-tcril commented 1 year ago

Update on the discovery/prototyping I've done so far:

Restating/clarifying goals

The main goal I have is parity with the devstack development experience.

For example, working on frontend-app-authn in devstack, all one needs to do is:

My feeling is, "if it is more complicated than that to do MFE dev with tutor we have failed."

MFE side plan

In this example, I

Tutor side plan

Current issues

tl;dr

CORS/CSRF are getting in the way of making connecting to services hosted in Tutor from a localhost MFE as easy as it is to connect to services hosted with devstack. Any suggestions on how handle this nicely would be amazing!

arbrandes commented 1 year ago

While I'm totally for parity with devstack in terms of developer experience and wholly support your efforts here, and while the current .env.development targets the devstack pretty explicitly, I don't think we should be hard-coding references to specific dev environments in the frontend repos.

I'm sure there's a simple way we can improve this (and both Tutor and the devstack) by using something like the module.config.js strategy.

Edit: yes, even if it means issuing PRs to the devstack, too.

kdmccormick commented 1 year ago

How to handle CORS/CSRF? I'm not sure how to handle CORS/CSRF between localhost and services in tutor containers.

@brian-smith-tcril Could you refresh me on what the issue is with CORS/CSRF?

Is it that LMS & CMS will reject requests originating from host-mode MFEs, which are hosted at localhost:1xxx? Would adding the host-served MFE domains to CORS_ORIGIN_WHITELIST and CSRF_TRUSTED_ORIGINS suffice?

ghassanmas commented 1 year ago

@kdmccormick I think yes, because by default tutor uses its own hostname for mfe/lms vs devstack which relies on the default localhost. There might be additional, settings, but this of course one of the needed step.

brian-smith-tcril commented 1 year ago

@kdmccormick I tried that: https://github.com/brian-smith-tcril/tutor-mfe/pull/2/files#diff-9dd8a655b8597bfd6cdb64b96b2a805174561cae462059559fdc4a2b8be0ac06R92-R93, When it didn't work I thought maybe localhost isn't referring to the host machine but instead the container's localhost? I did a little searching but nothing came up

ghassanmas commented 1 year ago

@brian-smith-tcril You are not specifying the port though, hence https://github.com/overhangio/tutor-mfe/blob/94b058ed559684583b89409f01a5a46549f4c389/tutormfe/patches/openedx-lms-development-settings#L25-L26

kdmccormick commented 1 year ago

Actually stopping serving the MFE in this case feels like overkill,

@brian-smith-tcril yeah, I agree, this seems like overkill, I'm not even sure how we'd do it if we wanted to.

simply updating the config to point to appropriate localhost URLs would be ideal

With runtime config, I am optimistic that we will be able to tell MFEs to link to host-served MFEs as appropriate by updating LMS's MFE_CONFIG. Is that what you had in mind as well?

brian-smith-tcril commented 1 year ago

With runtime config, I am optimistic that we will be able to tell MFEs to link to host-served MFEs as appropriate by updating LMS's MFE_CONFIG. Is that what you had in mind as well?

More or less yeah. I think in some (most? all?) cases we won't even need to modify the MFE_CONFIG we have in tutor at all, just tell the local MFE to get the config from tutor.

The main point of updating config within tutor is ensuring all the correct env vars are set (AUTHN_MICROFRONTEND_URL and AUTHN_MICROFRONTEND_DOMAIN for example)

kdmccormick commented 1 year ago

More or less yeah. I think in some (most? all?) cases we won't even need to modify the MFE_CONFIG we have in tutor at all, just tell the local MFE to get the config from tutor

Right, I was thinking not of tutor-mfe's config dictionary, but rather the MFE_CONFIG dict in LMS settings, which feeds the mfe_config API. Turns out it's just ACCOUNT_PROFILE_URL that would need to be updated there: https://github.com/brian-smith-tcril/tutor-mfe/pull/2/files#diff-9dd8a655b8597bfd6cdb64b96b2a805174561cae462059559fdc4a2b8be0ac06R101

The main point of updating config within tutor is ensuring all the correct env vars are set (AUTHN_MICROFRONTEND_URL and AUTHN_MICROFRONTEND_DOMAIN for example)

👍🏻

brian-smith-tcril commented 1 year ago

Is it that LMS & CMS will reject requests originating from host-mode MFEs, which are hosted at localhost:1xxx? Would adding the host-served MFE domains to CORS_ORIGIN_WHITELIST and CSRF_TRUSTED_ORIGINS suffice?

So I realized my mistake from before, I had just put localhost when I needed to have the port too. I've switched to looping through the MFE apps and adding localhost:{port} for each which seems to have resolved it.

kdmccormick commented 1 year ago

Woohoo!

kdmccormick commented 1 year ago

While I'm totally for parity with devstack in terms of developer experience and wholly support your efforts here, and while the current .env.development targets the devstack pretty explicitly, I don't think we should be hard-coding references to specific dev environments in the frontend repos. I'm sure there's a simple way we can improve this (and both Tutor and the devstack) by using something like the module.config.js strategy. Edit: yes, even if it means issuing PRs to the devstack, too.

I agree with @arbrandes here.

Fortunately, I think runtime config should simplify things. As long asNODE_ENV and MFE_CONFIG_API_URL are both set, I believe the MFE will use the LMS's runtime config API instead of looking for a .env file. I don't exactly know how to set those for each MFE on the host, though.

Unfortunately, it looks like tutor dev points MFEs to a templated, custom webpack config file: https://github.com/overhangio/tutor-mfe/blob/master/tutormfe/templates/mfe/apps/mfe/webpack.dev-tutor.config.js. I need to dive in and try to understand that file better. Does anyone have a sense of whether we could delete that config file, and just rely on webpack.dev.config.js?

arbrandes commented 1 year ago

tutor dev points MFEs to a templated, custom webpack config file:

Ah, I'd forgotten about that! Well, that's one way to go about doing it: instead of using the env files, we use that template with appropriate changes for each/all MFEs.

arbrandes commented 1 year ago

Does anyone have a sense of whether we could delete that config file

We need it for the proxy line. Caddy does this in production, but in dev mode, the only option is (was?) to have the webpack dev server do it.

brian-smith-tcril commented 1 year ago

I don't think we should be hard-coding references to specific dev environments in the frontend repos.

So as of right now with devstack, running npm start "just works." A big part of the reason for it "just working" is that .env.development is full of hardcoded devstack stuff.

I'm all for minimizing the amount of hardcoded dev env stuff we have in the MFE repos, and runtime config should go a long way towards this (that way we only need to provide an MFE_CONFIG_API_URL), but I think requiring that URL without providing a default is less than ideal.

Any thoughts on how to minimize hardcoded dev environment info in MFE repos while keeping the ability to run locally without requiring manual config?

kdmccormick commented 1 year ago

@arbrandes @brian-smith-tcril Do you think the user running this would be sufficient?

export MFE_CONFIG_API_URL="http://$(tutor config printvalue LMS_HOST)/api/mfe_config/v1"
npm start

I know that's a bit of a mouthful, but there are a few ways we could shorten it, for example...

Custom CLI command just to print the API URL:

MFE_CONFIG_API_URL="$(tutor mfe printconfigurl)" npm start

Custom CLI command that prints the path to an env, which can be sourced:

. "$(tutor mfe printenv)" 
npm start

Custom CLI command:

tutor npmstart  # would set MFE_CONFIG_API_URL appropriately and then run 'npm start' in the working dir

EDIT: That last command, tutor npmstart, could inspect its directory name (eg frontend-app-account) and use that information in order to also fulfill the role of this command:

Create a command that will serve the same effective purpose that make dev.stop.frontend-app-authn does in devstack

ghassanmas commented 1 year ago

We would need to define the path as well PUBLIC_PATH and APP_ID the former is important to stick with tutor, and assuming we choose to set PUBLIC_PATH to just / i.e. no need to define it, we would want to chage tutor-mfe patch for lms settings. But then we would diverge between tutor prodc v dev, hence we have bugs just about PUBLIC_PATH.

brian-smith-tcril commented 1 year ago

@kdmccormick the thing that immediately comes to mind with those suggestions is that they require having access to the tutor cli. With that in mind, I think I like

. "$(tutor mfe printenv)" 
npm start

the best. That way devs would only need to be in the venv they have tutor installed in once to generate the config, instead of needing to go into it every time they want to run the MFE.

I also think the tutor npmstart idea is an interesting one. I think we'd still want a command to manually switch an MFE to "local dev mode" without running npm start or being in the repo directory, but having a "one stop shop" command too could be cool.

kdmccormick commented 1 year ago

That way devs would only need to be in the venv they have tutor installed in once to generate the config, instead of needing to go into it every time they want to run the MFE.

@brian-smith-tcril Unfortunately, running . "$(tutor mfe printenv)" would only put the environment variables into the current shell. If one switched over a new shell and ran npm start, MFE_CONFIG_API_URL wouldn't be set.

I guess someone could enter a venv and run:

$ tutor mfe printenv
~/.local/share/tutor-root/yada/mfe/yada/env.development  # for example

and then later, from a shell outside of the venv, run:

$ . ~/.local/share/tutor-root/yada/mfe/yada/env.development
$ npm start

I guess I'm coming from a workflow where I have my tutor venv activated at all times, so the tutor command is always available. I image that a lot of people, especially virtualenvwrapper users, don't work that way...

brian-smith-tcril commented 1 year ago

I guess I was thinking more of a variation on that command where it creates a .env.development file (or something similar) with the tutor config info.

My workflow is very much a "only use the venv in the place I need it" one. I activate the venv and start tutor (or devstack), then open a new terminal tab and go into the MFE directory and run npm start (no venv)

I guess the thing I keep coming back to is, why is this https://github.com/openedx/frontend-template-application/blob/master/.env.development ok? If it's not ok, do we remove it from all MFEs and make everyone using devstack for MFE dev run

export MFE_CONFIG_API_URL="http://localhost:18000/api/mfe_config/v1"
npm start

if it is ok, why can't we just check in a tutor one too?

kdmccormick commented 1 year ago

why is this https://github.com/openedx/frontend-template-application/blob/master/.env.development ok?

Some thoughts on this:

kdmccormick commented 1 year ago

Oh, also: This issue is part of the greater "replace Devstack with Tutor" initiative. When we make tutor dev work as well as Devstack, we can deprecate Devstack, which would allow us to delete all of those Devstack-specific .env.development files.

kdmccormick commented 1 year ago

I guess I was thinking more of a variation on that command where it creates a .env.development file (or something similar) with the tutor config info.

I think that's reasonable!

kdmccormick commented 1 year ago

@ghassanmas

APP_ID

Oh yes, we do need to set that too. This isn't Tutor-specific, so I wonder if there is a way we could set it as a default in all MFE repos.

and assuming we choose to set PUBLIC_PATH to just / i.e. no need to define it

In that case, couldn't we leave out PUBLIC_PATH, because the default value (/) works well?

arbrandes commented 1 year ago

I'm with @kdmccormick on the reasons why we should avoid hard-coding yet another distribution into MFE repos. Which is to say, having .env.development hard-coded to point to the devstack is technically not ok. But we don't have to fix the past: just avoid the same mistake going forward.

Let's get back to the devstack, for a minute. One of the things it does is make dev.clone, which places repo checkouts in the expected places for development. If we were to rip out .env.development, there would be absolutely nothing stopping it from adding that back in as part of make dev.clone with appropriate values.

Correspondingly, there's nothing technical stopping us from adding similar functionality to Tutor. We don't need to go as far as tutor dev clone (though we could, for parity), but tutor mfe printenv dev > frontend-template-application/.env.development would go a long way. The only complication I can imagine is that this should be tutor-mfe's responsibility, and there is currently no way for a plugin to specify CLI commands.

arbrandes commented 1 year ago

Addendum: Kyle's use of the word "distribution" got me thinking: how does Debian solve this problem? For apt-get source in particular, I mean. It repackages the upstream source-code in a distro-friendly way, and gives the user a one-line command to get an easily-compilable source directory.

Meaning, I think I'm leaning towards tutor dev clone, after all. That would open the door for doing basically anything - short of altering upstream code - to prep the source directory for a better development experience.

kdmccormick commented 1 year ago

there is currently no way for a plugin to specify CLI commands.

@arbrandes https://docs.tutor.overhang.io/reference/api/hooks/catalog.html#tutor.hooks.Filters.CLI_COMMANDS -- check out the cookiecutter plugin for example usage :)

brian-smith-tcril commented 1 year ago

how does Debian solve this problem?

They maintain forks of everything and ensure the forks include debian directories that have everything needed for packaging. neofetch is a quick example

I think I'm leaning towards tutor dev clone

This feels like quite a good idea

tutor mfe printenv dev > frontend-template-application/.env.development would go a long way.

I'd like to still have a wrapper command for this, something like tutor mfe makedotenv that uses your current directory to generate the appropriate .env.development file.

One reason I'd like this is because of my workflow. I never touch the devstack checkouts. I keep all devstack stuff in a devstackworkspace directory, and then clone my forks of MFEs outside of that directory.

With all of this in mind, I think working to remove the current .env.development files from MFE repos could be a good use of time. We could:

arbrandes commented 1 year ago

docs.tutor.overhang.io/reference/api/hooks/catalog.html#tutor.hooks.Filters.CLI_COMMANDS

Ah, nice! Didn't realize you could also do commands! That's perfect, then.

@brian-smith-tcril, can't find fault with that proposal, but the fact is that removing the existing .env.development (and presumably adding it to .gitignore), while the right thing to do, is going to be an uphill battle. We'll need to create an ADR somewhere (possibly as a follow-on to this one in frontend-build), then get buy-in from the general MFE dev community. Which me might not get.

So what I'd do is:

  1. Sketch a potentially less controversial plan B (that is easily convertible to plan A later) and start moving with that. For instance, we can have Tutor create a .env.dev-tutor (following that other custom webpack file's naming scheme) and have it use that
  2. In parallel, start laying the ground work for plan A. Writing that ADR, giving folks time to react to it, etc.
arbrandes commented 1 year ago

PS: We can also use MFE domains as our "Plan A". In other words, we can start dropping .env.development from MFEs as we migrate them into Piral, as that will be the least of it in terms of changing the MFE dev workflow.

arbrandes commented 1 year ago

OR... we can just start using the JS configuration mechanism proposed in that ADR, and just forget the dotenv files - a hairy subject that ADR by no coincidence also eschews. :)

brian-smith-tcril commented 1 year ago

I think the easiest short-term "Plan B" solution would be writing out to .env.private

https://github.com/openedx/frontend-build/blob/a7605ee93f11e4717a0a8d915157d7937b42f970/config/webpack.dev.config.js#L26-L29

Longer term I think moving away from .env completely as per the ADR makes perfect sense!

kdmccormick commented 1 year ago

Oh hey, I didn't even know .env.private was a thing, that seems like it'd be great for the short term.

+1 that the long term would be to move to env.config.js, but without hard-coding anything Devstack or Tutor related into the repo, and then delete all those Devstack-specific .env.development files.

kdmccormick commented 1 year ago

With https://github.com/openedx/frontend-platform/pull/474/files nearing completion, I feel like the move from .env.development to env.config.js could start happening at any point, so we'll want to keep our eyes on that and make sure that Devstack-specific stuff doesn't get carried over into new env.config.js files.

brian-smith-tcril commented 1 year ago

Update on this:

As of https://github.com/brian-smith-tcril/tutor-mfe/pull/2/commits/ef070c9f7db720ad98aa14e7a0f527962838668b, I can now click on "Sign In" and the authn MFE loads properly.

Once I try to actually log in, however, I get another CSRF error. image image

with tutor dev logs --follow I'm getting

lms_1                        | 2023-05-22 14:05:43,588 INFO 23 [tracking] [user None] [ip 172.27.0.1] logger.py:41 - {"name": "/api/user/v2/account/login_session/", "context": {"user_id": null, "path": "/api/user/v2/account/login_session/", "course_id": "", "org_id": "", "enterprise_uuid": ""}, "username": "", "session": "", "ip": "172.27.0.1", "agent": "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/112.0.0.0 Safari/537.36", "host": "local.overhang.io", "referer": "http://localhost:1999/", "accept_language": "en-US,en;q=0.9", "event": "{\"GET\": {}, \"POST\": {\"email_or_username\": [\"superuser@tutor.local\"], \"next\": [\"/oauth2/authorize?client_id=cms-sso-dev&redirect_uri=http%3A%2F%2Flocal.overhang.io%3A8001%2Fcomplete%2Fedx-oauth2%2F%3Fredirect_state%3DflabcbdHgH0rrpP6ZYVLhodlcKYxZe9N&state=flabcbdHgH0rrpP6ZYVLhodlcKYxZe9N&response_type=code&scope=user_id+profile+email\"], \"password\": \"********\"}}", "time": "2023-05-22T14:05:43.588299+00:00", "event_type": "/api/user/v2/account/login_session/", "event_source": "server", "page": null}
lms_1                        | Forbidden (CSRF cookie not set.): /api/user/v2/account/login_session/
lms_1                        | 2023-05-22 14:05:43,593 WARNING 23 [django.security.csrf] [user None] [ip 172.27.0.1] log.py:224 - Forbidden (CSRF cookie not set.): /api/user/v2/account/login_session/
lms_1                        | [22/May/2023 14:05:43] "POST /api/user/v2/account/login_session/ HTTP/1.1" 403 9472

Another thing of note is that when I delete the cookies and refresh the MFE page, no cookies are created. I do get cookies when loading the MFE by clicking the "Sign In" button:

I've tried changing the SESSION_COOKIE_DOMAIN value in the .env for the authn MFE to a few different things but nothing has worked yet.

I will continue to investigate and post another update once I know more.

brian-smith-tcril commented 1 year ago

To clarify: my plan here is to get the authn MFE working with tutor hardcoded to expect it at localhost, and the MFE using config from a checked-in .env. Once that is in place I'll have something to reference as I build out the commands in tutor and the .env.private generation.

kdmccormick commented 1 year ago

Sounds good. I'm hoping localhost works out. If not, we might need to start running Caddy in dev mode so that we can serve MFEs under a consistent domain, whether container-mode or host-mode.

brian-smith-tcril commented 1 year ago

I did a bit more digging into the error I mentioned here https://github.com/openedx/wg-devops/issues/25 and I'm at a loss for what to try next.

The authn MFE appears to be correctly calling into getCsrfToken https://github.com/openedx/frontend-platform/blob/3f6ab4f83927cd96021170f5e3e605a93649f494/src/auth/AxiosCsrfTokenService.js#L19 using local.overhang.io for the URL param

It's not clear to me why getConfig() is returning that without the :8000 when process.env.LMS_BASE_URL has :8000 at the end. I thought that may be part of the issue, so I tried hardcoding the call to test.

authService.getCsrfTokenService().getCsrfToken("http://local.overhang.io:8000");

Even with it hardcoded, I'm getting the exact same CSRF error.

Any advice on things to try or places I should be looking would be huge. I'm feeling pretty lost at the moment.

brian-smith-tcril commented 1 year ago

Possible next steps:

brian-smith-tcril commented 1 year ago

Minor update: * When clicking "Sign in" on LMS we don't get a cookie * When clicking "SIGN IN" on Studio we do get a cookie

Edit: This was specific to changes I had made earlier. The cookies work as expected on upstream tutor.

brian-smith-tcril commented 1 year ago

Started down the caddy path, using https://caddy.community/t/setting-reverse-proxy-from-withing-docker-caddy-to-localhost-service/15369 as an example. One slight sticking point is:

, you have to make sure your service on the actual host is listening on whatever host.docker.internal resolves to (usually 172.17.0.1 - the docker0 interface). Meaning, if you configured that service on the host to only listen on 127.0.0.1, you will have to change that.

I'm thinking this should be possible at the webpack level (ideally in the config in frontend-build). Once I've proved the concept I'll have a better sense of how to ensure the dev experience is as seamless as possible.

regisb commented 1 year ago

Please see this PR, which is somewhat related: https://github.com/overhangio/tutor-mfe/pull/137

brian-smith-tcril commented 1 year ago

Leaving a comment with the status of my discovery

With the following changes (all pre https://github.com/overhangio/tutor-mfe/pull/137):

When running tutor dev launch with images rebuilt, and frontend-app-authn locally via npm run start-tutor

I know I tried a few cookie-related things (I think in edx-platform) but I can't find those changes at the moment.

Hopefully this is helpful to anyone trying to add local npm start functionality post https://github.com/overhangio/tutor-mfe/pull/137

arbrandes commented 6 months ago

Making a note here that @bradenmacdonald managed to run an MFE outside the container as part of his Vite PoC, but I don't see why we couldn't try to do the same thing (officially, I mean) with good-old webpack:

https://github.com/openedx/frontend-app-profile/pull/1000/commits/a8bf2b4fbb1ee53d93a816eba039b8e0418c6378#r1567764202

bradenmacdonald commented 6 months ago

Yes, since I figured out how to do it (with webpack), I run all my "dev" mode MFEs that way (on the host) - so much nicer to work with that way.