Run Copr infrastructure with podman kube play#4100
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 an impressive and comprehensive setup for running the COPR infrastructure locally using podman kube play. The use of an orchestration script, detailed Containerfiles, and Kubernetes manifests is a great step towards modernizing the development environment. My review focuses on enhancing security, maintainability, and robustness. Key areas for improvement include removing hardcoded secrets, reducing container privileges, and simplifying complex commands within manifests and container definitions. While this is a work-in-progress, addressing these points will establish a more secure and maintainable foundation for local COPR development.
Pull Request validationFailed🔴 Review - Missing review from a member (2 required) Success🟢 CI - All checks have passed |
nikromen
force-pushed
the
kubernetes
branch
3 times, most recently
from
c728efc to
3dd5d35
Compare
|
whoops, missclick |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive local deployment environment for the COPR infrastructure using podman kube play and Kubernetes manifests, supported by a justfile for automation and updated beaker test configurations. Feedback identifies a fragile DNS resolver detection in the backend HTTP server, a destructive database re-initialization policy on version mismatches, overly broad database user permissions, and a syntax error in the DistGit Apache configuration.
|
|
||
| ENTRYPOINT [] | ||
| # TODO: fix this resolver substitution, this is ugly hack | ||
| CMD ["sh", "-c", "RESOLVER=$(awk '/^nameserver/{print $2; exit}' /etc/resolv.conf) && sed -i \"s/__RESOLVER__/${RESOLVER}/g\" /etc/nginx/nginx.conf && exec nginx -g 'daemon off;'"] |
There was a problem hiding this comment.
The resolver detection is fragile. If /etc/resolv.conf does not contain a nameserver entry (which can happen in some container environments), the RESOLVER variable will be empty. This results in an invalid Nginx configuration (resolver valid=30s ...), causing the container to fail at startup. Consider adding a check or a fallback DNS server.
CMD ["sh", "-c", "RESOLVER=$(awk '/^nameserver/{print $2; exit}' /etc/resolv.conf); if [ -z \"$RESOLVER\" ]; then echo 'ERROR: No nameserver found in /etc/resolv.conf' >&2; exit 1; fi; sed -i \"s|__RESOLVER__|${RESOLVER}|g\" /etc/nginx/nginx.conf && exec nginx -g 'daemon off;'"]
| if [ "$EXISTING_VERSION" != "$CURRENT_VERSION" ]; then | ||
| echo "WARNING: Data version ($EXISTING_VERSION) doesn't match PostgreSQL version ($CURRENT_VERSION)" | ||
| echo "Cleaning old data and reinitializing..." | ||
| rm -rf "$PGDATA"/* |
There was a problem hiding this comment.
Automatically wiping the database data directory on a version mismatch is very destructive. While this is a development environment, it might lead to unexpected data loss if a user accidentally upgrades their container image. It would be safer to exit with an error message instructing the user to manually clear the volume if they wish to reinitialize.
| psql -c "CREATE USER \"$POSTGRESQL_USER\" WITH PASSWORD '$POSTGRESQL_PASSWORD';" | ||
| psql -c "CREATE DATABASE \"$POSTGRESQL_DATABASE\" OWNER \"$POSTGRESQL_USER\" ENCODING 'UTF-8';" | ||
| psql -c "GRANT ALL PRIVILEGES ON DATABASE \"$POSTGRESQL_DATABASE\" TO \"$POSTGRESQL_USER\";" | ||
| psql -c "ALTER USER \"$POSTGRESQL_USER\" WITH SUPERUSER;" # alembic migrations |
There was a problem hiding this comment.
Granting SUPERUSER privileges to the application user ($POSTGRESQL_USER) is a security risk, even in a development environment. While Alembic migrations might require elevated permissions for certain operations (like creating extensions), it is better to grant only the necessary privileges or run migrations as a separate, more privileged user.
|
|
||
| RUN set -ex && \ | ||
| rm -f /etc/httpd/conf.d/ssl.conf && \ | ||
| echo 'AliasMatch "/repo(/.*)/md5(/.*)\" "/var/lib/dist-git/cache/lookaside\$1\$2"' >> /etc/httpd/conf.d/dist-git/lookaside-copr.conf && \ |
There was a problem hiding this comment.
There is a typo in the AliasMatch directive. The trailing \" inside the single quotes will result in a literal backslash and double quote in the Apache configuration, which will cause the regex to fail to match standard Copr URLs.
echo 'AliasMatch "/repo(/.*)/md5(/.*)" "/var/lib/dist-git/cache/lookaside$1$2"' >> /etc/httpd/conf.d/dist-git/lookaside-copr.conf && \
|
/packit test |
|
/packit test |
|
/packit test |
1 similar comment
|
/packit test |
|
/packit test |
|
Can we have a better name of the directory rather than |
|
could be, eventually I would want to merge this with openshift deployment and see whether and when we could possibly migrate to it, but that is big if... it could be renamed later then |
|
/packit test |
|
I am not -1 on the PR because it is a self-contained change that doesn't affect anything else. But the fact that we cannot call it I hope that one day we will be able to refactor all of our deployment methods and share as much between them as possible. Now that I think about it, I think I had the same rant about the single machine testing-farm deployment. |
|
I want this one day to swallow kubernetes and openshift. But since it is not yet ready I can't. But I think we probably can delete the kubernetes - it was once created by someone who maintained his own copr stack and used it for their purposes - not by us. I think it is deprecated though and it can be deleted? Perhaps I should ask them |
Run the full Copr infrastructure locally with a single command. All services (frontend, backend, distgit, keygen, database, redis, resalloc, pulp) deploy as pods via podman kube play with Kustomize overlays. Four deployment modes: just up - latest development packages from @copr/copr-dev; just up-release - stable Fedora RPMs just up-pr 3127 - test a specific pull request just up-local - live editing with host source code mounted See other operational helpful commands by running just help Per-service ConfigMaps for easy configuration without image rebuilds. Multiple Kustomize overlays (base, dev, dev-local) for different use cases. Beaker integration test environment included - run the full test suite against the local stack via setup-local-copr.sh. Designed as the foundation for the possibility of future OpenShift deployment.
|
I am going to run tests (also locally against kube) to investigate whether anything is broken by my latest change /packit test |
|
both tests seems to work, @praiskup PTAL |
3 participants
I know this is a really long PR :/, this is summary what is capable of:
Run the full Copr infrastructure locally with a single command. All services (frontend, backend, distgit, keygen, database, redis, resalloc, pulp) deploy as pods via podman kube play with Kustomize overlays.
Four deployment modes:
just up- latest development packages from @copr/copr-dev;just up-release- stable Fedora RPMsjust up-pr3127 - test a specific pull requestjust up-local- live editing with host source code mountedSee other operational helpful commands by running
just helpPer-service ConfigMaps for easy configuration without image rebuilds. Multiple Kustomize overlays (base, dev, dev-local) for different use cases.
Beaker integration test environment included - run the full test suite against the local stack via setup-local-copr.sh.
Designed as the foundation for the possibility of future OpenShift deployment.