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

chore: update docker-compose.yml #15286

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

chore: update docker-compose.yml #15286

wants to merge 4 commits into from

Conversation

michael-markevich
Copy link

Based on the security assessment results (https://dhis2.atlassian.net/browse/SEC-48), I suggest updating the docker-compose.yml to move the composer configuration to the production setup. This includes using the official Postgis repository, an additional health check and security options, and a clean setup without a demo database and debugging.

The current docker-compose.yml can still be used for development purposes.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.24%. Comparing base (c63dbd2) to head (c3d3404).
Report is 1974 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master   #15286      +/-   ##
============================================
+ Coverage     65.83%   66.24%   +0.40%     
- Complexity    30925    31254     +329     
============================================
  Files          3483     3485       +2     
  Lines        129139   129791     +652     
  Branches      15046    15145      +99     
============================================
+ Hits          85015    85975     +960     
+ Misses        37076    36735     -341     
- Partials       7048     7081      +33     
Flag Coverage Δ
integration 49.79% <ø> (+0.06%) ⬆️
integration-h2 32.42% <ø> (+0.35%) ⬆️
unit 30.34% <ø> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 261 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a79344...c3d3404. Read the comment docs.

@Philip-Larsen-Donnelly Philip-Larsen-Donnelly requested review from tonsV2 and removed request for teleivo October 12, 2023 09:09
@tonsV2 tonsV2 requested a review from radnov October 12, 2023 11:43
@tonsV2
Copy link
Contributor

tonsV2 commented Oct 17, 2023

I would recommend creating a copy of the original docker compose file and naming it docker-compose.production.yml and make that the target of your updates.

Having both docker compose files in this PR make sense as we want to apply some of these changes to the original as well.

Copy link
Contributor

@tonsV2 tonsV2 left a comment

Choose a reason for hiding this comment

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

It might be beyond the scope of this PR but at some point we should probably supply some guide lines for log aggregation and backup.

https://docs.docker.com/storage/volumes/#back-up-restore-or-migrate-data-volumes
https://docs.docker.com/compose/compose-file/compose-file-v3/#logging

@@ -2,32 +2,35 @@ version: "3.8"

services:
web:
image: "${DHIS2_IMAGE:-dhis2/core-dev:local}"
image: "${DHIS2_IMAGE:-dhis2/core}"
Copy link
Contributor

Choose a reason for hiding this comment

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

A tag should be used here so we're not just pulling latest.

Perhaps use an environment variable, so we don't have to update the docker compose file, if we want to run a different version.


db:
image: ghcr.io/baosystems/postgis:12-3.3
image: postgis/postgis
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use this image?

I would recommend this one which is also what we're using on Kubernetes.

Regardless of which image we're using. A tag should be present so we're not just using latest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a quick note here - the postgis/postgis image won't suffice, as it's only built for linux/amd64 and we want this Docker compose file to also be usable by developers with the new Mac M* chips.

The ghcr.io/baosystems/postgis one is built for linux/arm64 as well and it provides some convenience, as we don't need to install postgis separately (like we do here for the dhis2-db stack in the IM), but it might actually be better to streamline this for consistency. 👍

Copy link
Author

Choose a reason for hiding this comment

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

@radnov @Philip-Larsen-Donnelly, as per previous discussions and as far as I understand this task, we are going to prepare and secure a docker-compose setup for production. So we should not use any third-party components or custom images, nor target Macbooks.

An image for developers (or anything following dev, but not stable branch, can use any images).

Copy link
Contributor

Choose a reason for hiding this comment

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

@michael-markevich Right, I should have been a bit more clear - the postgis/postgis image won't suffice for the developer docker compose file.

Still, it might be worth having the image we recommend for production use be the same one we use in the Instance Manager, as it has been tested the most. Technically, postgis/postgis is also a custom image, the "official" one is https://hub.docker.com/_/postgres.

ports:
- 127.0.0.1:5432:5432
volumes:
- db-dump:/docker-entrypoint-initdb.d/
- postgresql:/var/lib/postgresql
environment:
POSTGRES_USER: dhis
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend a stronger password here and the usage of a .env file.

The pattern we're using on the IM projects is to have a committed .env.example file in the root of the project which serves as an example. The readme and the file itself can contain some instructions if needed.

@amcgee amcgee changed the title Update docker-compose.yml chore: update docker-compose.yml Oct 26, 2023
Copy link

sonarqubecloud bot commented Nov 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@michael-markevich
Copy link
Author

michael-markevich commented Nov 6, 2023

@tonsV2 @radnov @Philip-Larsen-Donnelly
I updated the MR based on the feedback you provided. Here are some comments on the chosen approach:

  1. Docker manuals recommend using the merge functionality (https://docs.docker.com/compose/multiple-compose-files/merge/) to create various configurations/environments. This allows us to avoid duplicating the same configuration in multiple files.
  2. Following their logic and preferred new (since 2021) naming scheme, compose.yaml contains the production configuration of DHIS2, and compose.override.yaml contains overrides for the development environment.
  3. I didn't touch the password-related part but added .env.example file according to @tonsV2 suggestion.

In a nutshell, if both files are present, calling docker compose up will deploy a development build (as a result of merging two YAML files). If only compose.yaml is present (or specified explicitly with -f option), the production setup is deployed.

Copy link

This PR has not seen any activity in the last 5 months. The PR will be closed in 30 days if the stale label is not removed.

Please note that this is an automated message and we might very well be the reason why there has not been any activity lately.

Please remove the stale label if you would like to continue working on the PR. Make sure that you have requested a review by a dev or a team https://github.com/orgs/dhis2/teams.

@github-actions github-actions bot added the stale label May 24, 2024
@github-actions github-actions bot removed the stale label Jul 26, 2024
Copy link

This PR has not seen any activity in the last 5 months. The PR will be closed in 30 days if the stale label is not removed.

Please note that this is an automated message and we might very well be the reason why there has not been any activity lately.

Please remove the stale label if you would like to continue working on the PR. Make sure that you have requested a review by a dev or a team https://github.com/orgs/dhis2/teams.

@github-actions github-actions bot added stale and removed stale labels Dec 23, 2024
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