Rebuild phoenix and redis-commander ourselves#471
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces custom Containerfiles for Phoenix and Redis Commander to mitigate CVEs, updates the Makefile with build-and-push targets, and reconfigures Docker Compose and OpenShift manifests to utilize a new image registry. Key feedback includes a critical environment variable mismatch in the OpenShift Redis Commander deployment, the use of unstable Fedora base images, and recommendations for PEP 668 compliance and dependency pinning to ensure build stability and reproducibility.
| - name: REDIS_HOSTS | ||
| value: local:valkey:6379 |
There was a problem hiding this comment.
The environment variables in this manifest do not match the expected variables in the new Containerfile.redis-commander. The image now expects REDIS_HOST and REDIS_PORT instead of REDIS_HOSTS. This mismatch will prevent the service from connecting to the database in OpenShift.
- name: REDIS_HOST
value: valkey
- name: REDIS_PORT
value: "6379"There was a problem hiding this comment.
whoops, forgot to push
| @@ -0,0 +1,5 @@ | |||
| # we have our own image for phoenix because the upstream one has CVEs in it that our CVE scanner picks up | |||
| FROM fedora:44 | |||
There was a problem hiding this comment.
Fedora 44 is not yet a stable release (Fedora 41 is the current stable). Using an unreleased or rawhide version can lead to unpredictable build failures or unstable dependencies. It is recommended to use a stable version like 41.
FROM fedora:41
| # we have our own image for phoenix because the upstream one has CVEs in it that our CVE scanner picks up | ||
| FROM fedora:44 | ||
| RUN dnf install -y python3-pip python3-numpy python3-scipy && dnf clean all | ||
| RUN pip3 install arize-phoenix |
There was a problem hiding this comment.
Modern Fedora versions enforce PEP 668, which prevents pip from installing packages into the system environment. You should use the --break-system-packages flag or a virtual environment for the build to succeed. Additionally, consider pinning the version of arize-phoenix (e.g., arize-phoenix==4.29.0) to ensure reproducible builds.
RUN pip3 install --break-system-packages arize-phoenix
| @@ -0,0 +1,8 @@ | |||
| FROM fedora:43 | |||
There was a problem hiding this comment.
Fedora 43 is not yet a stable release. It is recommended to use a stable version like 41.
FROM fedora:41
| ENV NODE_ENV=production | ||
|
|
||
| RUN dnf install -y nodejs npm && dnf clean all | ||
| RUN npm install -g redis-commander |
There was a problem hiding this comment.
It is recommended to pin the version of redis-commander (e.g., 0.9.0) to ensure reproducible builds and avoid unexpected breaking changes from upstream updates.
RUN npm install -g redis-commander@0.9.0
TomasTomecek
force-pushed
the
phoenix-redis-comm
branch
2 times, most recently
from
098704c to
e7b9d15
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces custom Containerfiles for Phoenix and Redis Commander to mitigate CVEs and updates the Makefile, Docker Compose, and OpenShift manifests to support local builds and image registry synchronization. The review feedback highlights critical security improvements, such as defining non-root users to comply with OpenShift requirements, and suggests using stable Fedora base images instead of future releases. Additionally, recommendations were made to use environment variables for the container registry to ensure consistency across the build and deployment configurations.
| RUN pip3 install arize-phoenix | ||
| CMD ["phoenix", "serve"] |
There was a problem hiding this comment.
The OpenShift deployment manifest for Phoenix (line 61) specifies runAsNonRoot: true. Since Fedora images default to the root user (UID 0), the pod will fail to start unless a non-root user is defined in the Containerfile. Additionally, using --no-cache-dir with pip and explicitly exposing ports are best practices for container images.
RUN pip3 install --no-cache-dir arize-phoenix && \
useradd -u 1001 -g 0 -m -s /sbin/nologin phoenix
USER 1001
EXPOSE 6006 4317
CMD ["phoenix", "serve"]
| && npm install -g redis-commander@~0.9.0 | ||
|
|
||
| EXPOSE 8081 | ||
| CMD exec redis-commander --redis-host $REDIS_HOST --redis-port $REDIS_PORT |
There was a problem hiding this comment.
To comply with the runAsNonRoot: true security context in the OpenShift deployment (line 55), the image should define a non-root user. Also, cleaning the npm cache helps reduce the final image size.
&& npm install -g redis-commander@~0.9.0 \
&& npm cache clean --force \
&& useradd -u 1001 -g 0 -m -s /sbin/nologin rediscommander
USER 1001
EXPOSE 8081
CMD exec redis-commander --redis-host $REDIS_HOST --redis-port $REDIS_PORT
| @@ -0,0 +1,5 @@ | |||
| # we have our own image for phoenix because the upstream one has CVEs in it that our CVE scanner picks up | |||
| FROM fedora:44 | |||
There was a problem hiding this comment.
Fedora 44 is a future release (expected around April 2026) and is likely not available as a stable container image yet. It is recommended to use a current stable version like fedora:41 or fedora:40 to ensure build reliability.
FROM fedora:41
| @@ -0,0 +1,10 @@ | |||
| FROM fedora:44 | |||
There was a problem hiding this comment.
Fedora 44 is a future release and may not be stable or available yet. Consider using a stable version like fedora:41.
FROM fedora:41
|
|
||
| # Extract container tool (podman or docker) from COMPOSE | ||
| CONTAINER_TOOL ?= $(shell command -v podman >/dev/null 2>&1 && echo "podman" || echo "docker") | ||
| REGISTRY ?= quay.io/jotnar |
There was a problem hiding this comment.
Exporting the REGISTRY variable ensures it is available to docker compose when building images, allowing the registry path to be synchronized between the Makefile and compose.yaml.
export REGISTRY ?= quay.io/jotnar
|
|
||
| phoenix: | ||
| image: docker.io/arizephoenix/phoenix:version-11.6.2 | ||
| image: quay.io/jotnar/phoenix:latest |
There was a problem hiding this comment.
Using the ${REGISTRY} environment variable (with a default) allows the registry to be overridden consistently across the Makefile and Compose, matching the logic introduced in the Makefile.
image: ${REGISTRY:-quay.io/jotnar}/phoenix:latest|
|
||
| redis-commander: | ||
| image: ghcr.io/joeferner/redis-commander:0.9.0 | ||
| image: quay.io/jotnar/redis-commander |
There was a problem hiding this comment.
The redis-commander image is missing a tag, which defaults to latest but should be explicit for consistency. Additionally, using the ${REGISTRY} variable ensures consistency with the Makefile.
image: ${REGISTRY:-quay.io/jotnar}/redis-commander:latest
so it's periodically rebuilt and free of CVEs context: packit#409 Signed-off-by: Tomas Tomecek <ttomecek@redhat.com>
so it's periodically rebuilt and free of CVEs context: packit#409 Signed-off-by: Tomas Tomecek <ttomecek@redhat.com> Assisted-by: Claude
TomasTomecek
force-pushed
the
phoenix-redis-comm
branch
from
e7b9d15 to
9fe5386
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces custom Containerfiles for the phoenix and redis-commander services to mitigate security vulnerabilities and migrates image hosting to a new registry. It also updates the Makefile to support image pushing and adjusts OpenShift deployment manifests with image triggers and updated environment variables. Review feedback suggests several improvements: using the JSON array format for CMD to ensure correct signal handling, optimizing image size by excluding weak dependencies, ensuring port consistency between Containerfiles and OpenShift manifests, and explicitly tagging images for consistency.
3 participants
Summary
Containerfile.redis-commanderandContainerfile.phoenixbased on Fedora so images are periodically rebuilt and free of CVEscompose.yamland OpenShift manifests to use the new images fromquay.io/jotnar/; update redis-commander env vars fromREDIS_HOSTStoREDIS_HOST/REDIS_PORTbuild-and-push.ymlto build and push both images to quay.ioTest plan
make buildbuilds both new images without errorsmake startbrings up redis-commander and connects to valkey correctly (checkhttp://localhost:8081)build-and-push-redis-commanderandbuild-and-push-phoenixjobs pass on mergeSupersedes: #409
🤖 Generated with Claude Code