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

WIP: Implement multi-tenanted mode (thread-safe seamless mode) #3698

Draft
wants to merge 58 commits into
base: main
Choose a base branch
from

Conversation

instification
Copy link
Member

@instification instification commented Sep 29, 2022

See plip: #3774

How to test

The traefik setup in docker-compose.yml is configured to run a single backend and a single frontend, but use HTTP headers to change the backend site that the frontend is set to visit.

  1. Run docker compose up backend frontend reverse-proxy
  2. Once the backend and frontend have successfully started (you can visit http://plone.localhost and see the homepage), visit the backend at plone.localhost:8080 and create a second site with an ID of Plone2.
  3. Change the title of the homepage so it is clear which site is what from the homepage
  4. Visit http://plone2.localhost to confirm you can see the second Plone site's homepage

Implementation tasks

  • Detect host / x-internal-api-path headers from request
  • Create hook and HOC url helpers
  • Update all code to use new helpers instead of inspecting config singleton
  • Update documentation/training
  • Deprecate previous methods such as flattenToAppUrl? (perhaps by logging a warning when used)

Convert following components to use new helpers hook/hoc

List of files to convert

Non Component

  • src/middleware/api.js
  • src/express-middleware/devproxy.js
  • src/start-client.jsx
  • src/server.jsx
  • src/actions/workflow/workflow.js
  • src/helpers/Widget/widget.js
  • src/helpers/useUrlHelpers.js
  • src/helpers/Sitemap/Sitemap.js
  • src/helpers/index.js
  • src/helpers/Url/Url.test.js
  • src/helpers/Url/Url.js
  • src/helpers/Api/Api.test.js
  • src/helpers/Api/Api.plone.rest.test.js
  • src/helpers/Robots/Robots.js
  • src/reducers/types/types.js
  • src/reducers/controlpanels/controlpanels.js
  • src/reducers/controlpanels/controlpanels.test.js
  • src/reducers/querystringsearch/querystringsearch.js
  • src/reducers/navigation/navigation.js
  • src/reducers/navigation/navigation.test.js
  • src/reducers/content/content.test.js
  • src/reducers/content/content.js
  • src/reducers/search/search.test.js
  • src/reducers/search/search.js
  • src/reducers/workingcopy/workingcopy.js
  • src/reducers/actions/actions.js
  • src/reducers/site/site.test.js
  • src/reducers/navroot/navroot.test.js
  • src/reducers/navroot/navroot.js
  • src/reducers/breadcrumbs/breadcrumbs.test.js
  • src/reducers/breadcrumbs/breadcrumbs.js

Functional Components

  • src/components/theme/Comments/Comments.jsx
  • src/components/theme/Footer/Footer.jsx
  • src/components/theme/LanguageSelector/LanguageSelector.js
  • src/components/theme/ContentMetadataTags/ContentMetadataTags.jsx
  • src/components/theme/Navigation/ContextNavigation.jsx
  • src/components/theme/Navigation/NavItem.jsx
  • src/components/theme/CorsError/CorsError.jsx
  • src/components/theme/Image/Image.jsx
  • src/components/theme/RequestTimeout/RequestTimeout.jsx
  • src/components/theme/ConnectionRefused/ConnectionRefused.jsx
  • src/components/theme/Logo/Logo.jsx
  • src/components/theme/View/ImageView.jsx
  • src/components/theme/View/NewsItemView.jsx
  • src/components/theme/View/LinkView.jsx
  • src/components/theme/View/EventView.jsx
  • src/components/theme/View/FileView.jsx
  • src/components/theme/EventDetails/EventDetails.jsx
  • src/components/theme/Anontools/Anontools.jsx
  • src/components/theme/Widgets/RelationWidget.jsx
  • src/components/theme/Widgets/FileWidget.jsx
  • src/components/theme/Widgets/ImageWidget.jsx
  • src/components/manage/Blocks/LeadImage/LeadImageSidebar.jsx
  • src/components/manage/Blocks/Video/Body.jsx
  • src/components/manage/Blocks/Image/View.jsx
  • src/components/manage/Blocks/Image/ImageSidebar.jsx
  • src/components/manage/Blocks/Teaser/DefaultBody.jsx
  • src/components/manage/Blocks/Listing/SummaryTemplate.jsx
  • src/components/manage/Blocks/Listing/ImageGallery.jsx
  • src/components/manage/Blocks/Listing/DefaultTemplate.jsx
  • src/components/manage/Blocks/HeroImageLeft/View.jsx
  • src/components/manage/Sidebar/ObjectBrowserNav.jsx
  • src/components/manage/LinkMore/LinkMore.jsx
  • src/components/manage/AnchorPlugin/components/Link/index.jsx
  • src/components/manage/Controlpanels/Relations/BrokenRelations.jsx
  • src/components/manage/Toolbar/Types.jsx
  • src/components/manage/Toolbar/PersonalTools.jsx
  • src/components/manage/Multilingual/TranslationObject.jsx
  • src/components/manage/Multilingual/ManageTranslations.jsx
  • src/components/manage/Multilingual/CreateTranslation.jsx
  • src/components/manage/UniversalLink/UniversalLink.jsx
  • src/components/manage/Workflow/Workflow.jsx
  • src/components/manage/Widgets/UrlWidget.jsx
  • src/components/manage/Widgets/InternalUrlWidget.jsx
  • src/components/manage/Widgets/FileWidget.jsx
  • src/components/manage/Widgets/RegistryImageWidget.jsx
  • src/components/manage/WorkingCopyToastsFactory/WorkingCopyToastsFactory.jsx

Class Components

  • src/components/theme/View/View.jsx
  • src/components/manage/Blocks/Image/Edit.jsx
  • src/components/manage/Blocks/HeroImageLeft/Edit.jsx
  • src/components/manage/Sidebar/ObjectBrowserBody.jsx
  • src/components/manage/Contents/Contents.jsx
  • src/components/manage/AnchorPlugin/components/LinkButton/AddLinkForm.jsx
  • src/components/manage/Toolbar/More.jsx
  • src/components/manage/Add/Add.jsx
  • src/components/manage/Widgets/ReferenceWidget.jsx
  • src/components/manage/Widgets/ObjectBrowserWidget.jsx

Files/Tests which use deprecated components but don't need converting

  • src/components/theme/Navigation/ContextNavigation.test.jsx
  • src/components/theme/View/View.test.jsx
  • src/components/theme/Logo/Logo.test.jsx
  • src/components/theme/View/EventView.test.jsx
  • src/components/theme/View/NewsItemView.test.jsx
  • src/components/theme/EventDetails/EventDetails.test.jsx
  • src/components/manage/Widgets/RegistryImageWidget.test.jsx

@netlify
Copy link

netlify bot commented Sep 29, 2022

Deploy Preview for volto ready!

Name Link
🔨 Latest commit 4200751
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/651c31532fb9bf0008e0ead1
😎 Deploy Preview https://deploy-preview-3698--volto.netlify.app/deploying/seamless-mode.html
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@instification instification marked this pull request as draft September 29, 2022 15:12
@cypress
Copy link

cypress bot commented Sep 29, 2022

20 failed tests on run #7193 ↗︎

20 469 20 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Add flattenHTMLToAppURL
Project: Volto Commit: d217ad866f
Status: Failed Duration: 12:56 💡
Started: Sep 13, 2023 3:58 PM Ended: Sep 13, 2023 4:11 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

src/server.jsx Outdated
res.locals.detectedInternalHost = `${
req.headers['x-forwarded-proto'] || req.protocol
}://${req.headers['x-internal-host']}`;
config.settings.internalApiPath = res.locals.detectedInternalHost;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing that we're unsure of (at least I am) and needs testing is this line.

config.settings.internalApiPath = res.locals.detectedInternalHost;

Basically, I think it would be a good idea that all our URL helper functions stop depending on config.settings.apiPath and config.settings.internalApiPath and always use information either from the browser window or server HOST. CC @sneridagh

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we should talk about it during the conference.
I added it to my todo list.

@instification
Copy link
Member Author

I made a couple of changes

  • Set header to X-Internal-Api-Path to make it more obvious
  • Removed the use of X-Forwarded-Proto as the main point is to avoid using ssl where it's unnecessary. Instead the full url can be set in the header as it is a custom header anyway.

I'm not sure if

res.locals.detectedInternalHost = req.headers['x-internal-api-path'];
config.settings.internalApiPath = res.locals.detectedInternalHost;

can just be shortened to

config.settings.internalApiPath =  req.headers['x-internal-api-path'];

I'm not quite sure what res.locals is doing here.

@tiberiuichim
Copy link
Contributor

@instification It's awesome that you're tackling this issue.

There was a problem, at some point with cross-request pollution. Imagine that Volto's nodejs server is handling all requests, and because of nodejs's async nature (as far as I understand it, and I'm not sure of this) you may get execution of code "out of sync" between two different separate requests. If the config object is a global, and is mutated but shared between the requests, you'll get issues.

https://stackoverflow.com/questions/35649998/is-it-possible-to-have-thread-local-variables-in-node

@wesleybl
Copy link
Member

wesleybl commented Oct 5, 2022

@instification @tiberiuichim today I can use the variables RAZZLE_INTERNAL_API_PATH and RAZZLE_API_PATH, so that Plone requests made by the Volto server use RAZZLE_INTERNAL_API_PATH and requests made by the browser use RAZZLE_API_PATH?

@instification
Copy link
Member Author

@instification @tiberiuichim today I can use the variables RAZZLE_INTERNAL_API_PATH and RAZZLE_API_PATH, so that Plone requests made by the Volto server use RAZZLE_INTERNAL_API_PATH and requests made by the browser use RAZZLE_API_PATH?

@wesleybl yes that's correct as I understand it :)

@sneridagh
Copy link
Member

@wesleybl RAZZLE_INTERNAL_API_PATH is intended to be used in containerised environments. Specially the ones that the network topology prevents an easy communication between containers private/public interfaces (k8s, etc...) For the rest, it is fine that your Volto SSR server access to the public backend API URL.

@sneridagh
Copy link
Member

@instification It does look good, but I think we should be a bit cautious about this one.
Specially I'm concerned about Express and make sure that it does work under heavy load conditions. We had problems in the past with the fact that NodeJS does not have the "threadlocals" concept, thus it's difficult to have proper requests isolation.

It would be great if you lead this and test it in your servers, having good first hand feedback will help, for sure. If that adds some load tests it would be amazing.

Let's talk during the conference about the details.
Thanks for looking into this!

@instification
Copy link
Member Author

@sneridagh I have already started to look at isolating it following @tiberiuichim's advice to move away from using the global config object. I made some progress, certainly for api requests it is possible to look up per request whether there is a header set (for SSR requests) or whether the window.env.apiPath is set. I will try to get a proof of concept pushed here before the conference.

I don't think there are too many other places where apiPath is being read, but I am not 100% sure how simple it will be to access the req or window objects in those scenarios.

Presumably if we get the thread isolation issue resolved via not relying on the config object, then the load equation would be similar to if we were running with fixed api paths via env vars. But I think having some benchmarks and comparisons will be important.

Look forward to discussing more at the conference!

@tiberiuichim tiberiuichim added this to In progress in PloneConf 2022 Sprint via automation Oct 15, 2022
@tiberiuichim tiberiuichim added this to the Plone Conference 2022 milestone Oct 15, 2022
@instification instification self-assigned this Oct 15, 2022
@djay djay changed the title WIP: Implement thread-safe multihost seamless mode WIP: Implement multi-tenanted mode (thread-safe multihost seamless mode) Sep 27, 2023
@djay djay changed the title WIP: Implement multi-tenanted mode (thread-safe multihost seamless mode) WIP: Implement multi-tenanted mode (thread-safe seamless mode) Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants