Conversation
There was a problem hiding this comment.
Pull request overview
Adds an Elasticsearch service to the local Docker Compose stack to support the noticeboard feature, and updates the Django image build to install Poetry dependencies without installing the project package itself.
Changes:
- Add an
elasticsearchservice and persistent volume todocker-compose.yml. - Start Elasticsearch as part of the
scripts/start/development.shscaffolding services. - Change Django image dependency install to
poetry install --no-root.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| scripts/start/development.sh | Includes elasticsearch in the dev startup script’s compose services list. |
| docker-compose.yml | Defines the Elasticsearch service (image, ports, env, healthcheck) and adds esdata volume. |
| django/Dockerfile | Updates Poetry install step to avoid installing the root package during image build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| container_name: elastic | ||
| ports: | ||
| - "9200:9200" | ||
| - "9300:9300" |
There was a problem hiding this comment.
Port 9300 (Elasticsearch transport) is being published to the host. Unless you explicitly need node-to-node transport access from the host, avoid publishing 9300 and keep it internal to reduce attack surface and accidental coupling.
| - "9300:9300" |
| - "9200:9200" | ||
| - "9300:9300" |
There was a problem hiding this comment.
List indentation for ports here differs from the rest of the file (most sections use ports:\n- "..." without extra indentation). Please align formatting with the existing compose style for consistency and to reduce diff noise.
| - "9200:9200" | |
| - "9300:9300" | |
| - "9200:9200" | |
| - "9300:9300" |
|
Reviewed alongside the corresponding app PR (omniport-app-noticeboard#22). Good to see the missing ES service finally land in the compose. A few issues worth fixing before merge — calling out the security one first since it's the most serious: 🔴 Security blocker1. ES is published to the public internet with no auth ports:
- "9200:9200"
- "9300:9300"
environment:
- xpack.security.enabled=false
The other containers don't need ports:
- "127.0.0.1:9200:9200"And keep Functional issues2. ES Python deps still aren't in 3. 4. Healthcheck depends on test: ["CMD-SHELL", "curl --silent --fail localhost:9200/_cluster/health || exit 1"]The official test: ["CMD-SHELL", "wget -qO- http://localhost:9200/_cluster/health | grep -vq '\"status\":\"red\"'"](Note: 5. Django services don't depends_on:
elasticsearch:
condition: service_healthyCaveat: the file is 6. Operational concerns7. No JVM heap cap environment:
- ES_JAVA_OPTS=-Xms512m -Xmx512mTune for prod separately. 8. 9. Missing 10. Trailing whitespace on Production deployment caveatThis PR adds ES inside the noticeboard compose stack. That's fine for dev, but it's the opposite of what's wanted in prod (ES on a separate VM so the noticeboard host isn't carrying the index workload). Two suggestions:
|
|
After implementing #54 as an alternative shape and validating the full ES integration end-to-end on staging (against PR #22 + omniport-backend#213), I'd suggest closing this PR in favor of #54. The two PRs solve the same problem with very different tradeoffs. Quick comparison:
The biggest functional issue is the public port binding combined with security disabled — anyone reachable to the host on 9200 has full read/write/delete access to all indices. This is a regularly-exploited deployment vector (Shodan continuously scans for it). If you'd prefer to keep #52's inline approach instead of #54's override, I'd suggest at minimum:
But cleanest path: close this and merge #54 instead. Happy to consolidate if useful. |
The Poetry install was unpinned, which inside a cached Docker layer does not mean latest, it means frozen at whenever that layer was first built. The cached layer carried a Poetry too old to read lock-version 2.0, so the build failed on a lock the running image, at Poetry 1.8.3, could read perfectly well. Pinning both fixes that and makes the layer reproducible. setuptools was installed with --upgrade before Poetry ran, so a fresh build would take 82 or later, which removed pkg_resources. gunicorn 20.1.0 imports it at module scope, so the application server would have died at import. The lock already holds setuptools below 82 and this makes the two paths agree.
|
Closing this in favour of #54, which solves the same problem in a better shape. Three things decided it:
Nothing here is lost — #54 covers the same ground. It needs a rebase onto current master before it can merge. |
This PR adds Elasticsearch integration to the Docker Compose setup for the noticeboard feature.