Dockerfile with hardened image and GitHub Action

[SebastianGode]

No description provided.

[SebastianGode]

recheck

[Aloento]

@ecosquad-autoreview review

[ecosquad-autoreview]

Review of PR #73 — Hardened image & GitHub Actions CI

Summary

The PR replaces the Zuul-based build/publish pipeline with GitHub Actions, and rewrites the Dockerfile to move from a Fedora 38 base to a Debian 13 Python base pulled from the internal DHI Artifactory mirror. It switches packaging deps from dnf names to Debian/trixie names, installs psycopg2 as a Debian package, creates a proper non-root user with a home directory, and drops the dead commented-out clone steps.

Overall the direction is reasonable (non-root user, pinned base, apt cleanup). CI checks (GitGuardian, CodeQL, build) pass. However there are a few correctness/robustness issues I want flagged before merge, one of which is likely to break the image at runtime.

Findings

Warning (should fix before merge)

1. setup.py install with PEP 668 / externally-managed Python may fail or install into a broken state — Dockerfile:66

   RUN cd /usr/app/apimon && python3 setup.py install
   

   On Debian 13 the system Python is externally managed. You set PIP_BREAK_SYSTEM_PACKAGES=1 and use pip ... --break-system-packages for the requirements, but python3 setup.py install does not go through pip and is not affected by PIP_BREAK_SYSTEM_PACKAGES. More importantly, setup.py install is deprecated and, on modern setuptools, often fails or misbehaves. Worse: you switch to USER apimon at the end, but setup.py install runs as root and installs into /usr/lib/python3/... (system site-packages). That works, but it's inconsistent with the pip-based approach.
   Fix: install the package the same way you install the requirements, e.g.

   RUN pip install --no-cache-dir --break-system-packages .
   

   (the repo has a proper setup.py/setup.cfg with console entry points, so a normal pip install of the project will generate the apimon-scheduler/apimon-executor/apimon-epmon scripts that scripts/entrypoint.sh relies on.) This also avoids the deprecated setup.py install path and keeps everything consistent with PEP 668 handling.

2. Runtime deps installed via apt are duplicated/inconsistent with requirements.txt — Dockerfile apt block
   You install python3-sqlalchemy and python3-psycopg2 via apt, but requirements.txt also pulls sqlalchemy and psycopg2-binary through pip (with --break-system-packages). This produces two conflicting installations of SQLAlchemy and two PostgreSQL drivers (psycopg2 from apt vs psycopg2-binary from pip), which can lead to version skew or ImportError/ABI mismatches at runtime (e.g. apt psycopg2 compiled against a different libpq than what pip's build picks up).
   Fix: pick one source of truth. Since you're already pip-installing the full requirements.txt, drop the apt versions of python3-sqlalchemy and python3-psycopg2 (keep only what has no pip equivalent, e.g. python3-dev, python3-dnspython if needed by a system tool, etc.). Keep the apt set limited to native build/runtime deps that pip can't provide (git, gcc, ncat, procps, iproute2, xz-utils, passwd, python3-dev).

3. build-args uses the secret ARTIFACTORY_URL value as the build arg, but the Dockerfile's default is a host, not a URL — all three workflows + Dockerfile:14
   The Dockerfile declares ARG ARTIFACTORY_URL=artifactory.devops.telekom.de and uses it as a registry host in FROM ${ARTIFACTORY_URL}/dhi.io/python:.... The workflows pass ARTIFACTORY_URL=${{ secrets.ARTIFACTORY_URL }}. If secrets.ARTIFACTORY_URL is a full URL (e.g. https://artifactory... or artifactory.../something), then FROM will break because a registry reference cannot contain a scheme or a path. The docker/login-action also uses registry: ${{ secrets.ARTIFACTORY_URL }}, which expects just the host. Confirm the secret contains only the bare registry host (no scheme, no path). If it might contain a scheme, derive the host explicitly, e.g. store a dedicated ARTIFACTORY_REGISTRY secret for the host and keep the URL separate, or strip the scheme in the workflow. This is easy to get subtly wrong and will fail the push jobs (the PR builds use push: false and the build check passed, so a malformed host may not have been exercised by the push: true path).

Suggestion (nice to have)

4. Consistent naming/structure between the two workflows — .github/workflows/docker-build.yml vs docker-build-push-on-tag-or-release.yaml
docker-build.yml defines both a build job and a push_if_merged job, but the trigger is only pull_request (which does not include a closed/merged event that carries github.event.pull_request.merged == true in a useful way). The push_if_merged job will effectively never run, because pull_request events are not delivered with merged == true (a merged PR fires a push to the base, not a pull_request event). If you intend "build on PR, push on merge", the push job should live in a workflow triggered on push to the default branch, not on pull_request. The separate tag/release workflow already handles pushing; you may be able to drop push_if_merged entirely and rely on the tag/release + default-branch push flow. Please confirm the intended publish trigger so there isn't a dead job.

5. type=schedule in the metadata-action tags — all three workflows
   type=schedule only produces a tag when the job runs on a schedule; it's harmless here but adds a stray latest/date-ish tag on non-schedule runs in some versions. Not a blocker, just note it.

Checks

CI is green: GitGuardian (no secrets), CodeQL, and the build job all pass. push_if_merged is skipped as expected. Note the push paths (push: true in the tag/release workflow and push_if_merged) were not exercised in this PR, so findings #2 and #3 should be validated by running a real tag push before considering the pipeline fully verified.

Verdict

Request changes — the setup.py install / PEP 668 inconsistency (#1) and the duplicated SQLAlchemy/psycopg2 installs (#2) are likely to break the runtime image, and #3 needs confirmation of the ARTIFACTORY_URL secret format before the push jobs can be trusted.

[sergiubz]

Deleting zuul.yaml drops the otc-tox-pep8 and otc-tox-py311 jobs while the new workflows only build the image, so lint and unit tests no longer run on PRs; please add a tox job (pep8 + py311) before merging.

[sergiubz]

The final image is the -dev base with gcc, python3-dev and git left installed, so the runtime ships the full build toolchain; use a multi-stage build and copy the installed app into a minimal runtime stage.

[sergiubz]

git config --global runs as root so it lands in /root/.gitconfig, which the apimon runtime user (HOME=/home/apimon) will not read; set it after USER apimon or write to /etc/gitconfig.

[sergiubz]

On a merged pull_request these rules resolve to the head branch or pr-, not main or latest, so the published image gets a feature-branch tag; push on push to main or add type=raw,value=latest,enable={{is_default_branch}}.