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

Nginx unit #361

Merged
merged 1 commit into from
Jan 15, 2021
Merged

Nginx unit #361

merged 1 commit into from
Jan 15, 2021

Conversation

tobiasge
Copy link
Member

@tobiasge tobiasge commented Nov 10, 2020

Related Issue: -

New Behavior

  • Use Nginx Unit to serve the application

Contrast to Current Behavior

  • Gunicorn was used

Discussion: Benefits and Drawbacks

Removes Nginx container and with that the need to

  • Collect static files at startup
  • Shared volume for static files

In the current version Nginx Unit is build from sources, so the build will take a little bit longer

Changes to the Wiki

  • "Deployment", "Getting Started" and "TLS" pages need an update.

Proposed Release Note Entry

  • We are now using Nginx Unit to serve the application. This makes the additional Nginx container unnecessary.
  • After upgrade the docker-compose.override.yml must be changed to forward traffic directly to the Netbox container
  • The Netbox Python installation is now contained in a Python virtual environment located in /opt/netbox/venv

Double Check

  • I have read the comments and followed the PR template.
  • I have explained my PR according to the information in the comments.
  • My PR targets the develop branch.

@cimnine cimnine added the enhancement The issue describes an enhancement that we would like to implement in the future. label Nov 13, 2020
Copy link
Collaborator

@cimnine cimnine left a comment

Choose a reason for hiding this comment

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

Some preliminary comments. As discussed in Slack, I'd like to evaluate making 'alpine' the base image and getting python and nginx-unit from there to avoid keeping nginx-unit manually up-to-date.

Dockerfile Show resolved Hide resolved
build.sh Outdated Show resolved Hide resolved
docker/nginx-unit.json Outdated Show resolved Hide resolved
@tobiasge tobiasge force-pushed the nginx-unit branch 3 times, most recently from b31a001 to bfbe6d6 Compare November 23, 2020 10:23
Copy link
Collaborator

@cimnine cimnine left a comment

Choose a reason for hiding this comment

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

I'm looking forward to when we have this change merged! Getting rid of the separate nginx container and the nginx-config volume hack would mean a lot.

But until then and additionally to the other comments we'll have to change the Github Action configuration as well. Here we could test with alpine:edge as an alternative build in there probably, but stick to a specific version of Alpine Linux, i.e. alpine:3.12 at the moment, for our regular builds.

As usual, feel free to disagree with any comment, but please state your reason for doing so :) Also feel free to ask for help with the build files.

Dockerfile Outdated Show resolved Hide resolved
docker/docker-entrypoint.sh Outdated Show resolved Hide resolved
docker/docker-entrypoint.sh Outdated Show resolved Hide resolved
build.sh Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
build.sh Outdated Show resolved Hide resolved
@tobiasge tobiasge force-pushed the nginx-unit branch 2 times, most recently from de55626 to 26bc006 Compare November 28, 2020 17:27
@cimnine
Copy link
Collaborator

cimnine commented Dec 1, 2020

But until then and additionally to the other comments we'll have to change the Github Action configuration as well. Here we could test with alpine:edge as an alternative build in there probably, but stick to a specific version of Alpine Linux, i.e. alpine:3.12 at the moment, for our regular builds.

This comment was regarding the following code snippet: push.yml#L20-L23

@cimnine cimnine added this to the 0.28.0 milestone Dec 15, 2020
@cimnine cimnine mentioned this pull request Dec 15, 2020
3 tasks
@tobiasge tobiasge requested a review from cimnine December 15, 2020 09:49
Copy link
Collaborator

@cimnine cimnine left a comment

Choose a reason for hiding this comment

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

To be honest, I'm no particular fan of the change of the unitd configuration socket from unix-socket to tcp-socket. I believe it's easier to get wrong, e.g. security wise. It will also be harder to debug – for us in bug reports, but also for operation personnel –, because it'll never be clear in what configuration state a particular container is. I.e. was the configuration manipulated in the meantime?

Or in other words: I'm a fan of containers that are just configured at the start through files/secrets/environment variables, but are otherwise immutable. If the configuration changes, then restart/redeploy the container.

In your commit message (efaabf2) you've argued like this:

With this and the change made to unit in Alpine users can now load certificates into
the container an serve Netbox over an encrypted configuration.

I would still argue that this should be the job of a dedicated container, not the job of Netbox. (It's job is to serve the application.) E.g. in Kubernetes there are usually Ingress services taking care of this. Also, the big clouds have services like these. And usually they also take care of load balancing.

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
docker/launch-netbox.sh Outdated Show resolved Hide resolved
docker/launch-netbox.sh Outdated Show resolved Hide resolved
Copy link
Member Author

@tobiasge tobiasge left a comment

Choose a reason for hiding this comment

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

To be honest, I'm no particular fan of the change of the unitd configuration socket from unix-socket to tcp-socket. I believe it's easier to get wrong, e.g. security wise. It will also be harder to debug – for us in bug reports, but also for operation personnel –, because it'll never be clear in what configuration state a particular container is. I.e. was the configuration manipulated in the meantime?

Ok, I will revert this change.

Dockerfile Outdated Show resolved Hide resolved
We now serve Netbox with an nginx-unit instance instead of Gunicorn.
This allows us to get rid of the extra Nginx container because Unit is
also serving the static files. The static files are now collected at container
buildtime instead of every startup.
@tobiasge
Copy link
Member Author

The build is now based on alpine:3.13 so I think we could go forward with a release.

@tobiasge tobiasge requested a review from cimnine January 15, 2021 08:42
@cimnine
Copy link
Collaborator

cimnine commented Jan 15, 2021

We will need more text in the release notes:

  • Introduction of Python Virtual Env
  • Changes to the docker-compose.override.yml are required, because there is not nginx service anymore and the port mapping must be applied to the netbox service now.

We will also have to make quite some changes to the wiki to explain how the virtual env works (or at least that it's there) and how people can now build custom containers that include their plugins now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement The issue describes an enhancement that we would like to implement in the future.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants