-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(docker): Expose PostgreSQL configuration files on host for tuning in GeoNode Docker setup #13886
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
base: master
Are you sure you want to change the base?
feat(docker): Expose PostgreSQL configuration files on host for tuning in GeoNode Docker setup #13886
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @brynsofz on file. In order for us to review and merge your code, please contact the project maintainers to get yourself added. |
Summary of ChangesHello @brynsofz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the GeoNode Docker setup by externalizing PostgreSQL configuration files. By mounting Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a great step towards making the PostgreSQL configuration more flexible and tunable in the Docker setup by exposing the configuration files to the host. The use of read-only volumes for configuration is a good security practice.
However, there are a couple of significant issues:
- Inconsistent Environments: The changes are only applied to
docker-compose.yml. Thedocker-compose-dev.ymlanddocker-compose-test.ymlfiles are left unchanged, which will lead to different database configurations and behaviors between production, development, and testing environments. It's crucial to apply these changes consistently across all relevantdocker-composefiles. - Breaking Change: The
POSTGRESQL_MAX_CONNECTIONSenvironment variable is no longer used. This is a breaking change that should be documented. While the newconf.dapproach is more powerful, users should be guided on how to migrate their settings.
I've also added a suggestion to improve the security of the default pg_hba.conf settings.
| db: | ||
| # use geonode official postgis 15 image | ||
| image: geonode/postgis:15-3.5-latest | ||
| command: postgres -c "max_connections=${POSTGRESQL_MAX_CONNECTIONS}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The POSTGRESQL_MAX_CONNECTIONS environment variable, which was used here to configure max_connections, is no longer effective with the new configuration approach. This is a potentially breaking change for users who were relying on it. It would be helpful to document this change and provide instructions for setting max_connections via the new conf.d directory mechanism.
| # host all all all reject | ||
|
|
||
| # Or allow with password (current default): | ||
| host all all all scram-sha-256 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For better security, it's recommended to follow the principle of least privilege by denying all connections by default. The current configuration allows any connection that provides a valid password. Consider changing this catch-all rule to reject to ensure only explicitly authorized connections are permitted. An example of this is already commented out on line 36.
host all all all reject
|
@brynsofz Pull requests need an issue that describe the goal and plan. Further I am not convinced, exposing the postgresql database of geonode worldwide in general. You can still use a SSH Tunnel for getting access (as example for a more secure way) |
Checklist
For all pull requests:
The following are required only for core and extension modules (they are welcomed, but not required, for contrib modules):
Submitting the PR does not require you to check all items, but by the time it gets merged, they should be either satisfied or inapplicable.