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

Add node base image #5

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add node base image #5

wants to merge 1 commit into from

Conversation

djbe
Copy link
Contributor

@djbe djbe commented Jun 18, 2024

Combination of nuxt image (node stuff) and PHP image (script commands).

I'm pretty sure we can just apply this base image to all existing node projects… It's only once we update the modules to use the new ports & commands that the new stuff will be used. Note: we could (and should) create components so that we can mix the old & new stuff.

Copy link
Member

@jorenvandeweyer jorenvandeweyer left a comment

Choose a reason for hiding this comment

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

This violates some best practices:

  • Single Responsibility Principle
    • Each container should only have 1 process running
  • Complexity and Maintenance
    • Adding an extra unnecessary layer of complexity, Kubernetes already provides an ingress.

(One process per container, Ideal node docker image)


Secondly, Node Containers should never serve static files. I'd also apply these principles to Nuxt & Laravel images.

Eg. For Nuxt there should be 2 images build from a Nuxt repo.

  1. Build the Nuxt Project
  2. Copy static files to a nginx image (devs should make sure all their static files are under /assets
  3. Copy nuxt server to node image
  4. The ingress in kubernetes should be configured to route all calls prefixed with /assets/ to the nginx container, all other calls should be routed to the nuxt server.

@djbe
Copy link
Contributor Author

djbe commented Jun 22, 2024

First point:

No. Your articles, and every other, talk about "concerns". A single concern per app. Not a single process. The whole concern of this base image is serving the web app/api/whatever.

What we're not doing is mixing this with other web apps. And that's the whole point of all those articles. Don't run your frontend and backend on the same container. Don't run your database service on the same container as your web app, etc… Concern.

The combination of nginx and Node in one container is for 1 concern. Serving your API content. They don't do different things, they work as 1.

Regarding complexity/maintenance:

  • This reduces both. We have 1 location where we need to manage common configuration for ALL our node/nuxt/php/… images, and we can thus ensure they all work in the same secure & efficient way.
  • We don't need to complicate our ingress overmuch with a whole bunch of nginx configuration. The whole point of an ingress is to route traffic from a hostname+path to an application. All the rest should IMO be the responsibility of the application itself.

Second point:

You're making a LOT of assumptions there.

  • Assuming that static assets are in 1 location
  • Assuming we know these locations beforehand
  • Putting the onus of managing routes for static/dynamic stuff in the cluster, I'd prefer if we don't need to bother with it. Differentiating between front & back-end, sure, but the rest shouldn't matter.
  • Doubling the number of images is also not ideal, especially if they "belong together". It complicates deployments, management, slows down builds, and so forth.

Third point you didn't cover:

Reproducibility. Currently a lot of devs run stuff locally, which does not match server at all. There's a lot of differences, most related to ingress (besides services, which they can mostly simulate locally).

With these base images, that include the most important bits of nginx (headers/compression/static routing), they can reproduce this all with ease locally when running in docker.


Fourth point:

The only point you didn't explicitly mention here (but did in discussions on Slack), is about a process dying but the container remaining alive. That is a side effect of using supervisor, and as I mentioned before, there is a clear solution for this, which we will implement. It is not a blocker, and this discussion should consider this "solved".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants