Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Authn MFE is broken in nightly #122

Closed
kdmccormick opened this issue Apr 26, 2023 · 21 comments
Closed

Authn MFE is broken in nightly #122

kdmccormick opened this issue Apr 26, 2023 · 21 comments
Labels
bug Something isn't working

Comments

@kdmccormick
Copy link
Contributor

kdmccormick commented Apr 26, 2023

Steps to reproduce

Notes

  • I've confirmed that tutor local launch does not fix the issue.
  • @bmtcril confirms that that deleting all data and re-launching does not fix the issue.
  • @bmtcril was able to disable the mfe plugin, use the legacy login page, re-enable the mfe plugin, and then use the Learning MFE. This implies that the issue is not specific to MFEs or to authentication, but is an issue at intersection of the two and/or the an bug in the Authn MFE iteslf.
  • tutor dev also fails.
@ghassanmas
Copy link
Member

Hey, probably it would be useful to provide the console output, i,e are there specif API calls that are failing? And with nightly it means it suppose to run frontned-app-authn on master branch correct? both edx-platform and the mfe

@kdmccormick kdmccormick changed the title Authn MFE is broken in nightly local Authn MFE is broken in nightly Apr 26, 2023
@kdmccormick
Copy link
Contributor Author

  • Nothing in the LMS console for either local or dev.
  • Nothing in the Authn MFE browser console for local.
  • Debug view for Authn MFE in dev shows:
TypeError: secondaryProviders is undefined

Call Stack
 renderThirdPartyAuth
  src/login/LoginPage.jsx:213:38
 renderForm
  src/login/LoginPage.jsx:337:20
 render
  src/login/LoginPage.jsx:379:17
 finishClassComponent
  node_modules/react-dom/cjs/react-dom.development.js:17160:31
 updateClassComponent
  node_modules/react-dom/cjs/react-dom.development.js:17110:44
 beginWork
  node_modules/react-dom/cjs/react-dom.development.js:18620:16
 callCallback
  node_modules/react-dom/cjs/react-dom.development.js:188:14
 invokeGuardedCallbackDev
  node_modules/react-dom/cjs/react-dom.development.js:237:16
 invokeGuardedCallback
  node_modules/react-dom/cjs/react-dom.development.js:292:31
 beginWork$1
  node_modules/react-dom/cjs/react-dom.development.js:23203:28

@ghassanmas
Copy link
Member

One guess it might be due to frontend-platform, since it handles the init part, could you try running the authn while dumping the frontend-platform version to 4.1.0

@brian-smith-tcril
Copy link

brian-smith-tcril commented Apr 26, 2023

did a little testing on devstack to try to narrow down the issue

in a devstack venv

make dev.up.cms+studio

outside of the devstack venv
cloned latest frontend-app-authn, in the frontend-app-authn dir

nvm use
npm install
npm start

localhost:1999 is working as expected

@ghassanmas
Copy link
Member

Humm.. then it can be probably version node conflict, I think tutor tries to build using node 16 while the authin is already updated to 18 openedx/frontend-app-authn/pull/781 to test this one can just check insdie tutor-mfe contianer if assest exists in /openedx/dist/authin

@brian-smith-tcril
Copy link

tried

rm -rf node_modules/
nvm use 16
npm install
npm start

still seems to be working

@ghassanmas
Copy link
Member

ghassanmas commented Apr 26, 2023

Can you try running it using dynamic config enabled I think this one main difference between devstack and tutor. You would need to swt ENV variable to the minimum needed and then serve the other by setting dynamic config api

@ghassanmas
Copy link
Member

ghassanmas commented Apr 26, 2023

[update] I don't know if that's help. but I tried to run/build the authn mfe master while using tutor palm (using palm.master instead of master and it worked with no problem. In any case this probably mean the issue irrelevant to frontend-platform version

@regisb regisb added the bug Something isn't working label Apr 26, 2023
@ghassanmas
Copy link
Member

I was able to reproduce this issue, and to resolved in hacky way where I cloned maste of authn and then mounted the dist to mfe the container. When I noticed is that the mfe image doesn't have nightly suffix, as opposite the openedx image. So I guess it was using the olive image for nightly.

(edx-env) ➜  tutor-nightly tutor config printvalue DOCKER_IMAGE_OPENEDX
docker.io/overhangio/openedx:15.3.4-nightly
(edx-env) ➜  tutor-nightly tutor config printvalue MFE_DOCKER_IMAGE
docker.io/overhangio/openedx-mfe:15.0.5

So may be running tutor images build mfe suppose to resolve?.... it's building now for me, so I can update when it's done

@kdmccormick
Copy link
Contributor Author

@arbrandes will you be able to take a look? Is there anything we can do to help?

@arbrandes
Copy link
Collaborator

I can take a look over the next couple of days, yes!

@brian-smith-tcril
Copy link

I've been doing a bit of digging into this, when running tutor dev start I noticed

  Your models in app(s): 'third_party_auth' have changes that are not yet reflected in a migration, and so won't be applied.
  Run 'manage.py makemigrations' to make new migrations, and then re-run 'manage.py migrate' to apply them.

when breaking into the errors using a browser debugger I found issues with how we were setting thirdPartyAuthContext, it seems that has changed recently https://github.com/openedx/frontend-app-authn/blame/master/src/common-components/data/service.js#L23

I'm still looking into this, I'll make sure to share updates as I discover more.

@brian-smith-tcril
Copy link

Update: running tutor images build authn-dev fixed this for me.

Successfully built b058f87eab3d
Successfully tagged overhangio/openedx-authn-dev:15.0.5

compared to the current image on dockerhub (15.0.2 - 5 months ago) https://hub.docker.com/r/overhangio/authn-dev/tags

@brian-smith-tcril
Copy link

brian-smith-tcril commented May 11, 2023

@regisb do you have any insights into this? It seems the required frontend-app-authn change was openedx/frontend-app-authn@62e8f75

I think my main concern is that running tutor dev launch on nightly doesn't guarantee the latest MFE code is pulled.

@regisb
Copy link
Contributor

regisb commented May 16, 2023

We do not push nightly images to dockerhub for MFEs, so users need to re-build them locally. @brian-smith-tcril do I understand correctly that re-building fixes the issue for you?

@brian-smith-tcril
Copy link

do I understand correctly that re-building fixes the issue for you?

Yes

running tutor images build authn-dev fixed this for me.

I didn't test tutor local but I assume tutor images build mfe would work for that

@bmtcril
Copy link

bmtcril commented May 16, 2023

It worked on tutor local for myself and some other folks.

Is there any indication of when someone would need to rebuild the MFE image locally, or is it just "something broke, try rebuilding the image"? What causes the MFE image to be pushed to dockerhub now?

@regisb
Copy link
Contributor

regisb commented May 17, 2023

The docs state that nightly images can be pulled from dockerhub: https://docs.tutor.overhang.io/tutorials/nightly.html#upgrading-to-the-latest-version-of-open-edx

But this is factually incorrect. Only the "openedx" image is pushed to dockerhub: https://hub.docker.com/r/overhangio/openedx/tags?page=1&name=nightly CI does not push nightly images for plugins.

To offer a better user experience in nightly, we should also push nightly images for plugins. But it is unlikely that non-official images will go to the trouble of setting up a CI to push nightly images (with a different tag) to their registries.

So, I'm not sure what we should be doing here. Should we just rebuild all images automatically during launch on the nightly branch? Of course, at the very least we should fix the docs.

@arbrandes
Copy link
Collaborator

Should we just rebuild all images automatically during launch on the nightly branch?

If the images don't already exist locally, I think this is perfectly reasonable.

regisb added a commit to overhangio/tutor-indigo that referenced this issue May 26, 2023
This is to address overhangio/tutor-mfe#122
As a consequence of this change, images will be tagged with a "-nightly"
suffix. Next, we'll probably have to build them periodically in CI.
regisb added a commit to overhangio/tutor-minio that referenced this issue May 26, 2023
This is to address overhangio/tutor-mfe#122
As a consequence of this change, images will be tagged with a "-nightly"
suffix. Next, we'll probably have to build them periodically in CI.
regisb added a commit to overhangio/tutor-webui that referenced this issue May 26, 2023
This is to address overhangio/tutor-mfe#122
As a consequence of this change, images will be tagged with a "-nightly"
suffix. Next, we'll probably have to build them periodically in CI.
@regisb
Copy link
Contributor

regisb commented May 26, 2023

OK I've made the first step to resolve this issue. As of today, all Docker image tags from the nightly branches will be suffixed with "-nightly". This will prevent users from pulling Olive (or the current release) images when they really want to run nightly.

Next step will consist in running CI and push images periodically. Either that or force a re-build of all images on launch. I must admit that the thought of re-building all images from scratch every night (and fixing the issues every damn time) gives me cold sweats...

regisb added a commit to overhangio/tutor-cairn that referenced this issue May 26, 2023
This is to address overhangio/tutor-mfe#122
As a consequence of this change, images will be tagged with a "-nightly"
suffix. Next, we'll probably have to build them periodically in CI.
regisb added a commit to overhangio/tutor-android that referenced this issue May 26, 2023
This is to address overhangio/tutor-mfe#122
As a consequence of this change, images will be tagged with a "-nightly"
suffix. Next, we'll probably have to build them periodically in CI.
regisb added a commit to overhangio/tutor-jupyter that referenced this issue May 26, 2023
This is to address overhangio/tutor-mfe#122
As a consequence of this change, images will be tagged with a "-nightly"
suffix. Next, we'll probably have to build them periodically in CI.
regisb added a commit to overhangio/tutor-forum that referenced this issue May 26, 2023
This is to address overhangio/tutor-mfe#122
As a consequence of this change, images will be tagged with a "-nightly"
suffix. Next, we'll probably have to build them periodically in CI.
regisb added a commit to overhangio/tutor-notes that referenced this issue May 26, 2023
This is to address overhangio/tutor-mfe#122
As a consequence of this change, images will be tagged with a "-nightly"
suffix. Next, we'll probably have to build them periodically in CI.
regisb added a commit to overhangio/tutor-xqueue that referenced this issue May 26, 2023
This is to address overhangio/tutor-mfe#122
As a consequence of this change, images will be tagged with a "-nightly"
suffix. Next, we'll probably have to build them periodically in CI.
regisb added a commit to overhangio/tutor-discovery that referenced this issue May 26, 2023
This is to address overhangio/tutor-mfe#122
As a consequence of this change, images will be tagged with a "-nightly"
suffix. Next, we'll probably have to build them periodically in CI.
regisb added a commit to overhangio/tutor-ecommerce that referenced this issue May 26, 2023
This is to address overhangio/tutor-mfe#122
As a consequence of this change, images will be tagged with a "-nightly"
suffix. Next, we'll probably have to build them periodically in CI.
regisb added a commit that referenced this issue May 26, 2023
This is to address #122
As a consequence of this change, images will be tagged with a "-nightly"
suffix. Next, we'll probably have to build them periodically in CI.
@regisb regisb moved this to Backlog in Tutor project management Aug 28, 2023
@regisb
Copy link
Contributor

regisb commented Oct 2, 2023

I think we really don't want to re-build the mfe nightly images every day, so I'll close this. Feel free to reopen if you think differently.

@regisb regisb closed this as completed Oct 2, 2023
@github-project-automation github-project-automation bot moved this from Backlog to Done in Tutor project management Oct 2, 2023
moonesque pushed a commit to edSPIRIT/tutor-mfe that referenced this issue Nov 20, 2023
This is to address overhangio#122
As a consequence of this change, images will be tagged with a "-nightly"
suffix. Next, we'll probably have to build them periodically in CI.
moonesque pushed a commit to edSPIRIT/tutor-mfe that referenced this issue Nov 20, 2023
This is to address overhangio#122
As a consequence of this change, images will be tagged with a "-nightly"
suffix. Next, we'll probably have to build them periodically in CI.
hinakhadim pushed a commit to overhangio/tutor-indigo that referenced this issue Nov 26, 2023
This is to address overhangio/tutor-mfe#122
As a consequence of this change, images will be tagged with a "-nightly"
suffix. Next, we'll probably have to build them periodically in CI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Development

No branches or pull requests

6 participants