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

Create Docker installation files #7

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Create Docker installation files #7

wants to merge 17 commits into from

Conversation

timwoj
Copy link
Member

@timwoj timwoj commented Jan 21, 2025

This moves the existing manual installation steps to a docker version that should simplify management a little, along with letting us move everything to another host. A sample version of it is running at https://packages-docker.zeek.org, though without any package information currently due to a disk space issue with the package scanning.

There still feels like a bit too much manual editing of files during the installation, but it's probably fine and I'm just being picky.

@timwoj timwoj force-pushed the topic/timw/docker branch from aabfe5c to 6dc2333 Compare January 21, 2025 22:51
Copy link

@evantypanski evantypanski left a comment

Choose a reason for hiding this comment

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

I combed through and had a couple comments, but I am likely not the best reviewer here :)

@timwoj timwoj force-pushed the topic/timw/docker branch from 6dc2333 to 9e1b49a Compare January 29, 2025 18:38
Copy link

@evantypanski evantypanski 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 clear, I don't feel like I can give a great review here but it looks good to me and it's been a while sitting around)

@timwoj timwoj force-pushed the topic/timw/docker branch from 9e1b49a to b34fb38 Compare February 1, 2025 03:48
Copy link
Member

@bbannier bbannier left a comment

Choose a reason for hiding this comment

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

This looks much better than the previous setup.

I was unable to generate certificates like documented in the README with something failing in LE (maybe I picked an incorrect domain). What I ended up doing was remove the SSL config from nginx which allowed everything to start up even without certs. I could then see the site, but the database was probably unpopulated so the package listing was empty and I could not see a package details view. Ideally we'd have a way to get info for e.g., 10 packages, or at least document how the db can be populated manually (when I did this in the past it was very slow though).

timwoj and others added 15 commits February 3, 2025 13:18
This also removes the bash invocation from the first line, since
php was complaining about parsing it.
- Mount the bropkg webroot directly on the nginx VM to allow access
  to the images and CSS. Also mount it in read-only mode.
- Don't try to override the logging for nginx. It goes to 'docker logs'
  by default.
- chown/chmod files as they're being copied for php, which is
  considerably faster than doing it after.
- Don't expose the port for php publicly
- be more explicit about versions of anything we depend on
- avoid unneeded Docker layers
- slight trimming of installed packages
@timwoj timwoj force-pushed the topic/timw/docker branch from 250cc78 to fcc4453 Compare February 3, 2025 20:27
@timwoj timwoj force-pushed the topic/timw/docker branch from 521521b to 606fb82 Compare February 3, 2025 21:40
le_fixpermissions() {
echo "[INFO] Fixing permissions"
chown -R "${CHOWN:-root:root}" "${CERT_DIR_PATH}"
find "${CERT_DIR_PATH}" -type d -exec chmod 755 {} \;
Copy link
Member

Choose a reason for hiding this comment

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

If CERT_DIR_PATH does not exist this will definitely fail, let's make sure it exists by prefixing with

    mkdir -p "${CERT_DIR_PATH}"


le_fixpermissions() {
echo "[INFO] Fixing permissions"
chown -R "${CHOWN:-root:root}" "${CERT_DIR_PATH}"
Copy link
Member

Choose a reason for hiding this comment

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

Running in my macos dev environment this failed for me since root is not a valid group name here and the failure was hard to see in all the output of the script. I'd suggest to move this to the top as a documented customization point (this part passed for me once I invoked the script with CHOWN=<username>:staff ...).

Comment on lines +14 to +17
cat >/etc/cron.d/certbot.cron <<EOF
# Run Let's Encrypt certbot renew twice a day at random times
0 0,12 * * * root /usr/bin/python -c 'import random; import time; time.sleep(random.random() * 3600)' && $SCRIPT_DIR/ssl-update.sh
EOF
Copy link
Member

Choose a reason for hiding this comment

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

This failed for me in my macos dev env which does not have cron installed so /etc/cron.d does not exist (and would't be user-writeable anyway). Let's at least make this conditional on the existance of that dir (though it still might not be writeable by the user running this). For dev I wouldn't want to run this as root at all, so that must not be a requirement.

Comment on lines +20 to +22
- Run the `cert_setup/init-certs.sh` script. This will generate a Let's Encrypt
certificate, store it in the location that nginx container will use, and add
a cron task to automatically update it.
Copy link
Member

Choose a reason for hiding this comment

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

I have no idea how I should set this up for development since LE seems to expect me to own a domain where I can put a challenge file; for all my attempts this part failed. Can you please add a dummy cert at the location expected for the default DOMAIN so the dev workflow just works?

I suspect all of this works for you since you have some cert in data/certbot/letsencrypt/live/; if you do a new checkout somewhere else and follow these instructions you should run into similar issues (do not assume that you have a host set up with a challenge since this is neither documented nor needed for frontend or backend development).

Comment on lines +35 to +36
docker compose -f docker-compose-dev.yml build
docker compose -f docker-compose-dev.yml up -d
Copy link
Member

Choose a reason for hiding this comment

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

This only starts the services in the dev config (the database), users also need to start the remaining services.

Suggested change
docker compose -f docker-compose-dev.yml build
docker compose -f docker-compose-dev.yml up -d
docker compose -f docker-compose-dev.yml -f docker-compose.yml build
docker compose -f docker-compose-dev.yml -f docker-compose.yml up -d

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

Successfully merging this pull request may close these issues.

4 participants