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

Align to GeoNode master (4.2.0) and improve/fix docker initialization #457

Merged
merged 27 commits into from
Aug 22, 2023

Conversation

afabiani
Copy link
Member

@afabiani afabiani commented Jul 27, 2023

This PR introduces improvements and fixes to docker-compose and initialization scripts in general:

  • Removing some unused variables.
  • Removing some unused and obsolete containers, such as Jenkins.
  • Removing the incorrectly written and non-functioning part for setting the password of GeoServer.
  • Inverting the startup dependency from GeoServer -> Django to Django -> GeoServer.
  • Ensuring that OAUTH2 is properly initialized on restart in all its parts.
  • Enhancing NGINX to make sure that GeoServer functions well behind any proxy, including HTTPS.

Some implementation details:

  1. Environment file

DOCKERHOST and DOCKER_HOST_IP variables removed as useless and misleading. Instead, it exposed NGINX_BASE_URL which was calculated automatically and badly.

The variables

GEONODE_LB_HOST_IP=django
GEONODE_LB_PORT=8000
GEOSERVER_LB_HOST_IP=geoserver
GEOSERVER_LB_PORT=8080

are important, they are used by NGINX to create the upstream proxyPass and by GeoServer to create the internal OAuth2 endpoints. They represent the name and port of the GeoNode and GeoServer containers. In general it is not necessary to change them, but in an environment like Microsoft Azure these must be set to the address of the subnet as the names are not internally resolved.

Increased MAX_DOCUMENT_SIZE=200 because the previous default of 2Mb did not allow rendering even one document.

Updated create-envfile.py to also enable staging mode to test HTTPS on localhost, which was previously totally ignored.

  1. Dockerfiles

Removed useless stuff and started it from geonode/geonode-base:latest-ubuntu-22.10

  1. Readme

Added a Quick Start section with docker

  1. Docker Compose

Fixed healthchecks which are now faster and more efficient and switched Django -> GeoServer startup dependency, because it's the Django entrypoint that sets OAutg2 parameters in the GEOSERVER_DATA_DIR before it starts.

Updated to POSTgresql 15

  1. UWSGI + NGINX

Enabled proxyPass instead of uwsgiPass because we set headers correctly in geonode.conf only with proxyPass

  1. GeoServer docker entry points

Fixed the .sh to set OAuth2 which previously worked badly and only partially checking when there was the localhost value. Now they update everything and always.

  1. nginx/geonode.conf.envsubst

Made proxy passes dynamic

set $upstream $GEOSERVER_LB_HOST_IP:$GEOSERVER_LB_PORT;

set $upstream $GEONODE_LB_HOST_IP:$GEONODE_LB_PORT;

Updated the GeoServer conf so that the GUI works fine behind any proxy.

  1. A general reformat to the black code

  2. Version bump

from 4.0.0.final to 4.2.0.dev

try:
r = requests.request('get', url, verify=False)
r = requests.request("get", url, verify=False)

Check failure

Code scanning / CodeQL

Request without certificate validation

This request may run without certificate validation because [it is disabled](1).


def get_version():
import {{project_name}}.version
return {{project_name}}.version.get_version(__version__)
import {{ project_name }}.version

Check failure

Code scanning / CodeQL

Syntax error

Syntax Error (in Python 3).
@afabiani afabiani self-assigned this Jul 29, 2023
@afabiani afabiani added this to the 4.2.0 milestone Jul 29, 2023
@afabiani afabiani changed the title - Align to GeoNode master (4.2.0) Align to GeoNode master (4.2.0) and improve/fix docker initialization Jul 29, 2023
@ridoo
Copy link

ridoo commented Jul 31, 2023

@afabiani these are quite good changes! However, the PR raises a maintenance question:

What do you actually recommend when users have to decide between a GeoNode Fork or the creation of a separate Geonode project via geonode-project template?

The main problem I see is to keep track of all the changes you make on this repository (e.g. those within this PR). Once a project has been created, it gets detached from version control. If a user decides to upgrade their GeoNode project, they would have to follow the commit history and apply all changes manually, right? Wouldn't it be better to just use a GeoNode fork so the changes become visible once you do a merge/pull?

Is there is a discussion (issue/documentation) about Forking vs. GeoNode-Project somewhere? Some good practice guidance would be helpful too.

I'd be happy for any pointer :-)

@afabiani
Copy link
Member Author

Those changes won't ba backported. Those will be eventually part of the 4.2.0 train, currently under development.

@ridoo
Copy link

ridoo commented Jul 31, 2023

Ok .. but what about mainaining a project which got generated from the geonode-project repository? When upgrading to 4.2.0 .. I would have to go through the commit history and apply the changes manually on that project. Or do I miss something?

@afabiani
Copy link
Member Author

The idea would be to update the documentation of the 4.2.0 release with a specific section for this topic. However we can open a discussion on the mailing list to further investigate the problem and hear from the community if there are some suitable proposals/ideas.

@ridoo
Copy link

ridoo commented Jul 31, 2023

However we can open a discussion on the mailing list to further investigate the problem and hear from the community if there are some suitable proposals/ideas.

You mean [email protected]? I am subscribed, but it seems it makes not much noise in my inbox. However, I will open a discussion thread there.

edit: mail address .. geonode-devel is the right one -- in fact I only found the geonode-devel-requests address in my inbox

@ridoo
Copy link

ridoo commented Jul 31, 2023

However, I will open a discussion thread there.

@afabiani my email awaits approval 🤷‍♂️

#
#########################################################################
import os
import os

Check notice

Code scanning / CodeQL

Module is imported more than once

This import of module os is redundant, as it was previously imported [on line 20](1).
Copy link

Choose a reason for hiding this comment

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

Remove redundant import

GEOSERVER_ADMIN_USER = os.getenv("GEOSERVER_ADMIN_USER", "admin")
GEOSERVER_ADMIN_PASSWORD = os.getenv("GEOSERVER_ADMIN_PASSWORD", "geoserver")
GEOSERVER_FACTORY_PASSWORD = os.getenv("GEOSERVER_FACTORY_PASSWORD", "geoserver")
geoserver_rest_baseurl = f"http://localhost:{GEOSERVER_LB_PORT}/geoserver/rest"
Copy link

Choose a reason for hiding this comment

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

a strayed localhost, I suspect?!

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope it's ok. It will always refer to the local container.

@giohappy giohappy requested a review from ridoo August 22, 2023 11:19
@giohappy
Copy link
Contributor

@ridoo if you agree I would merge the PR

@ridoo
Copy link

ridoo commented Aug 22, 2023

@giohappy if it is ok for you, I'd take a deeper look later this day, before just saying "ok".

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@giohappy giohappy self-requested a review August 22, 2023 13:11
Copy link

@ridoo ridoo left a comment

Choose a reason for hiding this comment

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

I finished review .. however, I touched some parts which relate to Geonode core as well (the PR puts the changes made upstream here) .

However, I did not want to block the PR.

.env.sample Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
docker kill $(docker ps -q); docker rm $(docker ps -a -q); docker rmi $(docker images -q); docker volume ls -qf dangling=true | xargs -r docker volume rm; docker system prune -a
docker volume prune
Copy link

Choose a reason for hiding this comment

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

This file is quite dangerous as it removes everything non-geonode related, too! To my experience first-time users do not understand the implications immediately. Having a cleaned Docker setup is afterwards, well, .. surprising, at least.

Leverage the --filter flag:

# delete all containers with name including "thuenen" 
docker rm $(docker ps -aq --filter=name=thuenen)
# delete all volumes with name including "thuenen"
docker volume rm $(docker volume ls -q --filter name=thuenen)
# delete all images having a `4.0` tag starting with `geonode`
docker rmi $(docker images -q --filter=reference='geonode/*:4.0')

docker/geoserver/entrypoint.sh Outdated Show resolved Hide resolved
docker/geoserver/entrypoint.sh Outdated Show resolved Hide resolved
#
#########################################################################
import os
import os
Copy link

Choose a reason for hiding this comment

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

Remove redundant import

docker/nginx/Dockerfile Outdated Show resolved Hide resolved
src/uwsgi.ini Show resolved Hide resolved
@giohappy giohappy merged commit c4430ba into master Aug 22, 2023
4 checks passed
@giohappy giohappy deleted the align_with_geonode_master branch August 22, 2023 17:07
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