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

feat(#174): bastion Dockerfile and compose file #177

Merged
merged 46 commits into from
Dec 5, 2024

Conversation

mrjones-plip
Copy link
Contributor

@mrjones-plip mrjones-plip commented Oct 25, 2024

Description

This PR:

  • Adds new Dockerfile to build a bastion container - based on Alpine 3.20
  • adds compose file docker-compose.bastion.yml to define a bastion host to allow SSH tunnels to access postgres
  • locks down postgres by removing the ports: lines from docker file so it doesn't expose the port externally (unless you run pgadmin compose file)
  • splits off pgadmin into it's own compose file. most of the time this shouldn't be run alongside postgres because it exposes a publicly accessible HTTP port that with no password set. While you do need to know the POSTGRES_PASSWORD, I think this is still to "insecure by default" so we should silo them off by default instead
  • adds ports: file to pgadmin compose file for postgres server
  • defaults all e2e tests to go through bastion host, just as it will in production

closes #174

Code review checklist

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or e2e where appropriate
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

@mrjones-plip mrjones-plip marked this pull request as draft October 25, 2024 22:17
@mrjones-plip mrjones-plip changed the title Create a bastion Dockerfile and compose file per #174 feat(#174): bastion Dockerfile and compose file Oct 26, 2024
@mrjones-plip mrjones-plip changed the title feat(#174): bastion Dockerfile and compose file feat(#174): bastion Dockerfile and compose file Oct 26, 2024
@mrjones-plip
Copy link
Contributor Author

mrjones-plip commented Oct 26, 2024

@witash and @njuguna-n - is it OK to remove these two lines that expose the postgres ports ? for a sane default for production I don't think we want this exposed - I've removed it in this PR.

Related: For development I think we should use pgadmin. I also think we should break out pgadmin to it's own docker file so it's not run by default in production. - I went ahead and did this while I was in there!

Thought: Instead of docker-compose.pgadmin.yml, we could call this docker-compose.developer-postgres.yml or something. It would not only add pgadmin service but also expose 5432 for postgres server defined in docker-compose.postgres.yml. This is nice b/c you can easily run it in development and it's clear what it's point is. I think most production instances should not expose pgadmin on 5050 by default - but maybe I'm taking it too far?

services:
  postgres:
    ports:
      - 5432:5432

  pgadmin:
    image: dpage/pgadmin4
    environment:
      PGADMIN_DEFAULT_EMAIL: ${PGADMIN_DEFAULT_EMAIL:-pgadmin4@pgadmin.org}
      PGADMIN_DEFAULT_PASSWORD: ${PGADMIN_DEFAULT_PASSWORD:-admin}
      PGADMIN_CONFIG_SERVER_MODE: 'False'
    ports:
      - "${PGADMIN_PORT:-5050}:80"

@mrjones-plip
Copy link
Contributor Author

@njuguna-n or @witash - before I send this PR out for formal review - can you point me toward how I should add tests for the new bastion container? Maybe an existing pattern we have? I'm a test n00b and need a little guidance 🙏

thanks!

@njuguna-n
Copy link
Contributor

Hello @mrjones-plip for this one you are looking at adding e2e tests right? You can have a look at existing e2e tests here for some guidance.

@mrjones-plip mrjones-plip marked this pull request as ready for review November 20, 2024 05:48
@mrjones-plip
Copy link
Contributor Author

While ready for review, I think we should wait until #187 is merged. Then I can get all a green CI on this branch - otherwise right now green CI is impossible because of #186

@lorerod and @dianabarsan - if you can't wait, you can have a preliminary look at e2d and new bastion container respectively! Otherwise, feel free to wait until this PR is fully baked.

@mrjones-plip
Copy link
Contributor Author

hmmm - I note cleanup never runs, it just hangs here:

 Container cht-sync-postgres-1  Started
      ✔ should handle PostgreSQL downtime gracefully (26937ms)
    Incremental sync
      ✔ should process document edits (18288ms)
      1) should process contact deletes
      ✔ should process person deletes (18039ms)
      ✔ should process report deletes (12023ms)
      ✔ should process incremental inserts (18023ms)
    Conflict resolution
Failed to edit document with id 00172650-a2d1-540b-b308-d53ff75f102c: Document update conflict.
      ✔ should handle update conflicts (19058ms)
Failed to delete document with id 00172650-a2d1-540b-b308-d53ff75f102c: Document update conflict.
      ✔ should handle delete conflicts (19039ms)


  15 passing (3m)
  1 failing

  1) Main workflow Test Suite
       Incremental sync
         should process contact deletes:

      AssertionError: expected 1 to equal +0
      + expected - actual

      -1
      +0
      
      at Proxy.assertEqual (node_modules/chai/lib/chai/core/assertions.js:1038:12)
      at Proxy.methodWrapper (node_modules/chai/lib/chai/utils/addMethod.js:57:25)
      at doAsserterAsyncAndAddThen (node_modules/chai-as-promised/lib/chai-as-promised.js:289:22)
      at Proxy.<anonymous> (node_modules/chai-as-promised/lib/chai-as-promised.js:255:20)
      at Proxy.overwritingMethodWrapper (node_modules/chai/lib/chai/utils/overwriteMethod.js:78:33)
      at Proxy.<anonymous> (node_modules/chai-exclude/chai-exclude.js:122:21)
      at Proxy.overwritingMethodWrapper (node_modules/chai/lib/chai/utils/overwriteMethod.js:78:33)
      at Context.<anonymous> (file:///home/mrjones/Documents/MedicMobile/cht-sync/tests/e2e-test.spec.js:221:49)
      at process.processTicksAndRejections (node:internal/process/task_queues:105:5)

Maybe I'm not being patient enough? !

@mrjones-plip mrjones-plip requested a review from lorerod December 3, 2024 18:43
Copy link
Contributor

@lorerod lorerod left a comment

Choose a reason for hiding this comment

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

Thank you @mrjones-plip!
And thanks for also working in the documentation of this change.

Copy link
Contributor

@lorerod lorerod Dec 3, 2024

Choose a reason for hiding this comment

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

@mrjones-plip Could we explore dynamic secret generation or docker secrets instead? We can do it on a separate PR if you think it is a good idea.

@mrjones-plip mrjones-plip merged commit 27d1739 into main Dec 5, 2024
6 checks passed
@mrjones-plip mrjones-plip deleted the 174-bastion-postgres-access branch December 5, 2024 14:24
@medic-ci
Copy link

medic-ci commented Dec 5, 2024

🎉 This PR is included in version 1.2.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

Access to containerized instances of postgres
4 participants