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

Docker #6

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Docker #6

wants to merge 21 commits into from

Conversation

meyerkev
Copy link

@meyerkev meyerkev commented Aug 20, 2024

We now have a makefile to turn up docker-compose

make up
make down
make clean to destroy all postgres data
make reset to restart all services
make reset-clean to restart all services and destroy all data
make REPO= docker-push to push to Docker

alekratz and others added 15 commits August 19, 2024 23:39
siikr.conf should probably be treated as a configuration or env file, so
it should not have execute permissions. It should only be invoked by
using `source` in your shell.
The Dockerfile tries to emulate the simple_setup.sh script as closely as
possible. A couple of quirks:

* php-zmq does not appear to have a Debian package, and the package on
  PECL is from 2016 and the repo has been abandoned. Instead, we need to
  compile it from source. Not a huge deal, but still annoying.
* Since the msgrouter service can't really run in Docker (since it uses
  systemd services) I have opted to use supervisord. See here:
  https://docs.docker.com/config/containers/multi-service_container/

Signed-off-by: Alek Ratzloff <[email protected]>
Since siikr.conf is more of an environment file, the extension should
reflect that. I have also renamed it to siikr.example.env - this is so
that if the configuation is changed, it will only go into the example
env file, and not cause issues with pulling down into a modified
siikr.env file. Additionally, it helps prevent in-dev configuration from
accidentally getting committed in the case that someone uses
`git commit -a` a bit carelessly.

Signed-off-by: Alek Ratzloff <[email protected]>
…n step to php-fpm.Dockerfile

Signed-off-by: Alek Ratzloff <[email protected]>
…nd update README + env example

* docker-compose.yaml for docker-compose command
* postgres.Dockerfile sets up Postgres appropriately
* create_database.sh script will create the Postgres database from the
  SQL in the siikr directory in the Docker container
* README includes base instructions for using docker-compose
* Update siikr.example.env to include the POSTGRES_* environment
  variables necessary for Docker deployment

Signed-off-by: Alek Ratzloff <[email protected]>
* DB host was not previously available in the user configuration, this
  is now added in case your DB is hosted anywhere besides localhost (as
  is the case with Docker)
* All of the `new PDO` calls were either using a hard-coded database
  user or getting the currently logged in UNIX user, rather than the
  configured user. The password was also being set to null. This is
  resolved with the `get_db()` function in the internal/globals.php
  file. All calls to `new PDO` are also updated so we don't have to
  update it in 6 different places, either.

Signed-off-by: Alek Ratzloff <[email protected]>
* Forward port 80 -> 8080
* Install nginx in php container
* Add a php-fpm-siikr.conf file for Docker
* Add an nginx-siikr.conf file Docker

Signed-off-by: Alek Ratzloff <[email protected]>
* Use `isset` on the $_GET key, instead of checking for implicit
  membership
* Add a catch for `DivisionByZeroError` in the show_page.php - not sure
  why the `catch (Exception $e)` wasn't catching it. (We can fix it
  later)

Signed-off-by: Alek Ratzloff <[email protected]>
docker/postgres.Dockerfile Outdated Show resolved Hide resolved
Copy link
Owner

@EGjoni EGjoni left a comment

Choose a reason for hiding this comment

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

Looks good!

I marked as "Request Changes" in the hopes that having this use Postgres14 instead of 16 isn't too much of a pain.

Also see comments re php-zmq and get_db(). I suspect the latter is why indexing wasn't working for you.

/**
* Get a handle to the database.
*/
function get_db() {
Copy link
Owner

Choose a reason for hiding this comment

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

The db may be signed into by two separate users. One as www-data for client accessible scripts, and one as a system user for long running tasks that don't eat up a server thread. So this abstraction would ideally accept an argument to differentiate.

Whichever the approach, most of the variables required are imported from the auth/credentials.php file, which means we have to specify global in order to access them.

function get_db() {
    global $db_host; 
    global $db_name;
    global $db_user;
    global $db_pass;

    return new PDO("pgsql:host=$db_host;dbname=$db_name", $db_user, $db_pass);
}

I will be making these globals into proper constants with the next push. But for now I suspect this is why indexing isn't working for you.

Copy link
Author

Choose a reason for hiding this comment

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

OK.

Copy link
Author

Choose a reason for hiding this comment

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

So undo that use of globals and reset all the get_db() calls #5 built?

Copy link
Owner

@EGjoni EGjoni Aug 20, 2024

Choose a reason for hiding this comment

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

Yeah, just for now. If for no other reason than to minimize implementation confounders. I'll clean it up on the next version push.

Copy link
Author

Choose a reason for hiding this comment

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

So we'll probably need to setup the www-data user in the sql somehow since these are two isolated users here.

Copy link
Owner

Choose a reason for hiding this comment

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

I thought I had done that already as part of the schema and setup_simple.sh. But apparently not. It should be to
the script's php_user, and not the harcoded www-data though.

Man I really should have spent more time cleaning this up before publishing it.

Copy link
Author

Choose a reason for hiding this comment

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

Take a look at my latest push?

Copy link
Owner

Choose a reason for hiding this comment

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

Anyway, siikrweb should be fine for all of the basic functionality tasks I think. You're unlikely to need any of the maintenance functions for a little while.

docker/php-fpm.Dockerfile Outdated Show resolved Hide resolved
-- Manually added, needed for healthchecks
-- Should never ever have a password
CREATE ROLE postgres WITH LOGIN SUPERUSER;

Copy link
Author

Choose a reason for hiding this comment

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

We're going to have to add www-data here, I think.

Copy link
Author

Choose a reason for hiding this comment

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

Or make it a script configured via secrets somehow.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, this question has come up before.

But now it occurs to me, does this docker composition approach even allow for connecting via unix sockets?
Giving www-data this sort of permission is really only handy if they aren't going to be manually using credentials to get access via a host anyway.

Copy link
Author

Choose a reason for hiding this comment

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

You're almost definitionally connecting between hosts.

Copy link
Owner

Choose a reason for hiding this comment

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

But that will still require some very annoying juggling with the setup scripts...hmm

Copy link
Author

Choose a reason for hiding this comment

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

Don't get too involved with this just yet until I see how this works on the NAS that powers on but I've never actually connected to the internet before.

If they have k8s, that's an init container or a helm manifest or something.

Copy link
Owner

Choose a reason for hiding this comment

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

I mean, it's a thing that needs doing regardless.

I'd been writing most of the code under the assumption that no one was going to ask me to publish it, and it shows.

Copy link
Owner

@EGjoni EGjoni left a comment

Choose a reason for hiding this comment

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

Looks good! Does it work?

@meyerkev
Copy link
Author

Looks good! Does it work?

Hard to tell. It's not giving me any results.

@EGjoni
Copy link
Owner

EGjoni commented Aug 20, 2024

Looks good! Does it work?

Hard to tell. It's not giving me any results.

sudo tail -f /var/log/postgresql/postgresql-14-main.log
sudo tail -f /var/log/php8.2-fpm.log

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.

3 participants