The review flagged `:latest` as a critical issue, and it is right: a mutable
tag makes `kubectl rollout undo` meaningless and lets different pods in the
same deployment run different code if they restart at different times.
- The build job now pushes the image with BOTH `:latest` and
`:${{ github.sha }}`.
- The deployment manifest carries a sentinel tag rather than `:latest`, and
the deploy job sets the real SHA with `kubectl set image`. That makes the
deploy a genuine manifest change, so the Deployment rolls because its pod
template changed — not because it was told to. `kubectl rollout undo` now
means something: it goes back to the previous SHA.
- The digest check now compares against the SHA tag rather than `:latest`,
and it checks EVERY running pod instead of `items[0]`. During a rolling
update `items[0]` may still be an old pod that has not been terminated,
which would compare against the wrong digest. The job also fails if the
deployment is not fully rolled out.
- The smoke test now checks the app through the Service as well as directly
in the pod. The pod check alone would pass even if the Service selector
or the port mapping were wrong.
- `readOnlyRootFilesystem: true` — the app writes no files, so the root
filesystem can be read-only. /tmp is still writable via the emptyDir.
- The kubectl install is skipped when the runner already has one.
The workflow built and pushed the image but never deployed it - the
cluster objects had been created by hand. This adds a `deploy` job that
applies the manifests in `kube/` and rolls the deployment.
The deploy job is separate from the build job on purpose. The
`KUBEC_CONFIG_BUILDX` secret is cluster-admin-equivalent, and a build step
that never talks to the cluster should not be able to read it.
Three things this job does that a naive `kubectl apply` does not:
1. `kubectl rollout restart`. The image tag is `:latest`, which is immutable
in the manifest, so `apply` alone does not roll the pods. Without the
restart the deployment reports success while the old pods keep serving
the old image - a deploy that silently changes nothing.
2. Verifies the running digest against the registry's `:latest`. CI green and
pods `Ready` do not prove the new code is live; the digest comparison does.
3. Smoke tests the running app - `/healthz` and `/readyz` - from inside the
pod, so a deploy that comes up but cannot serve is caught in CI rather
than by the next person to open the page.
The Secrets (`beer-db`, `beer-app`) are hand-provisioned and are deliberately
not in the apply set, so a deploy cannot clobber them.
The cluster objects for the beer namespace were created by hand. This puts
them in the repo so they can be applied and reviewed like everything else.
- `beer-deployment.yaml` — the Deployment, with the security context, the
resource limits, and both probes wired up (`/healthz` liveness,
`/readyz` readiness).
- `beer-service.yaml` — the NodePort service.
- `beer_namespace.yaml` — the namespace.
The credentials are NOT in these files. `MONGO_PASS` and `SECRET_KEY` come
from the live `beer-db` and `beer-app` Secrets, so applying the manifests
cannot clobber a hand-provisioned credential.
Notes on the pod security context
---------------------------------
`runAsNonRoot: true` cannot be set on the container when the image's user is
a *named* user - kubelet can only verify a numeric UID. The pod-level
security context therefore sets `runAsUser: 999`, which matches the `beer`
user the image creates (verified with `id` in the built image).
The container also drops all capabilities and refuses privilege escalation.
Two follow-ups to the review of the base image change.
1. Pin the patch version: `python:3.14-slim` is a floating tag, so a rebuild
could silently pick up a different base image. It is now
`python:3.14.10-slim`, so a rebuild is reproducible.
2. Run the app as an unprivileged user. The app only reads its own source
and talks to Mongo over the network — it writes no files — so it does not
need to be root. A `beer` system user is created and the image ends with
`USER beer`.
The image build failed on master:
ERROR: Ignored the following versions that require a different python
version: 4.19.0 Requires-Python >=3.11
ERROR: Could not find a version that satisfies the requirement
pymongo==4.19.0
The base image was `python:3.10` but the pinned pymongo 4.19 needs Python
>= 3.11. The image is now `python:3.14-slim` — the current stable release,
so it is not carrying the CVEs fixed in 3.11, 3.12 and 3.13.
All the runtime dependencies ship a cp314 wheel (pymongo included), so no
compiler toolchain is needed. The old apt-get line installed
libsasl2-dev, python-dev-is-python3, libldap2-dev and libssl-dev for
packages that are no longer in the dependency list, so it is gone and the
image is smaller.
The build now asserts the interpreter version, so a base image change that
breaks the pinned requirements fails the build with a clear message rather
than a pip resolution error.
The independent review flagged that the build job wrote the
`KUBEC_CONFIG_BUILDX` secret to disk even though the build does not need
cluster access at all. That secret is cluster-admin-equivalent: a build step
that never talks to the cluster should not be able to read it, and a
compromised dependency in the build would otherwise have a direct path to the
cluster.
The "Create Kubeconfig" step is removed from the build job. The deploy step,
added in a later PR, gets its own job with the kubeconfig - so the privilege
is only present where it is actually used.
Also checked the buildkit daemon's resources in response to the review: it
already has limits (2 CPU, 4Gi memory, 2Gi ephemeral storage) and requests
(500m, 1Gi). The crash that motivated the amd64-only change was qemu
exhausting memory during multi-arch emulation, not an unconstrained daemon, so
no change is needed there.
The build job failed at "Set up Docker Buildx":
Error: ERROR: error for bootstrap "builder-...":
Get "https://192.168.178.160:6443/apis/apps/v1/namespaces/gitea-runner/deployments/builder-...":
tls: failed to verify certificate: x509: certificate signed by unknown authority
Cause
------
The `kubernetes` buildx driver talks to the cluster API to create its builder
pods. That needs the k3s CA certificate, which the job container does not
have - the `catthehacker/ubuntu:act-latest` image ships the public CA set
only. So the bootstrap could not verify the API server.
Fix
---
Switch to the `remote` driver, which talks straight to the buildkit daemon
already running in the cluster:
driver: remote
endpoint: tcp://buildkit.gitea-runner.svc:1234
No cluster API access is needed at all, so no kubeconfig is needed for the
build. This is the same pattern the wedding-photos workflow uses.
`endpoint` must be a top-level key, not a driver-opt: the action appends it
as the POSITIONAL argument to `buildx create --driver remote <endpoint>`, and
passing it in driver-opts yields "ERROR: no remote endpoint provided".
Also
----
The build was multi-arch (linux/amd64,linux/arm64), which runs qemu
emulation and crashes the buildkit pod on this NUC (exit 255,
SandboxChanged), after which the build hangs for many minutes. The cluster
is amd64, so the image is built for linux/amd64 only.
Verified: `docker buildx create --driver remote tcp://buildkit.gitea-runner.svc:1234`
succeeds from the runner's dind container and reports buildkit v0.29.0 with
linux/amd64 among its supported platforms.
The setup-python action downloads a Python build from GitHub. This runner
cannot reach GitHub reliably for that, and the test job hung on it with no
output. The runner image already carries a usable python3, so the tests use
it directly and install into a local venv.
The round-2 review noted that `_upgrade_password` catches `PyMongoError`
without logging it. A failed upgrade is harmless to the login itself - the
user still gets in - but it leaves the password stored in plain text, and
that is exactly the kind of thing an operator needs to know about. It is now
logged as a warning with the username and the traceback.
A test covers the path: the migration write is forced to fail, the login
still succeeds, and the warning is emitted.
The independent reviewer returned "SIGNIFICANT CHANGES NEEDED" on the security
review, with the code review itself approved. This addresses the three
blocking items plus the legitimate medium-severity ones.
Blocking items
--------------
1. Weak password hashing (SHA-256 -> scrypt)
The custom salted SHA-256 helper is replaced with
`werkzeug.security.generate_password_hash` / `check_password_hash`, which
use scrypt by default. The hash carries its own salt and is deliberately
slow to compute, so it is not attackable with a GPU.
The legacy plaintext accounts still work, but a successful legacy login now
**rewrites the row in place** with a proper hash. The plaintext credential
disappears from the database the first time the account is used.
Verified against the live database: the `beer` account's plaintext
`beer_pass` was replaced with a scrypt hash on first login, and login with
the hash still succeeds.
2. Plaintext password persistence
Fixed by the migrate-on-login above. There is no longer a code path that
leaves a password in plaintext.
3. Hardcoded secret defaults
`SECRET_KEY` no longer has a default. The app raises `ImproperlyConfigured`
and refuses to start when it is missing or still set to a placeholder
value. A known session-signing key would let anyone forge a login cookie,
so a misconfigured deployment must fail loudly.
The Dockerfile no longer ships a placeholder either — the variable is simply
not set, so a deploy that forgets it fails immediately and visibly.
Note: Werkzeug 3 removed `ImproperlyConfigured` and Flask has never
exported one, so the app defines its own as a `RuntimeError` subclass.
Medium items
------------
4. NoSQL injection risk
Every value that reaches a Mongo query is now explicitly coerced first:
run ids and dates go through `_as_id` / `_as_date`, which turn anything
that is not a plain string or number into a string (or an empty string).
Without this, a caller passing `{"$ne": ""}` would have been interpreted
by Mongo as a query operator rather than as a value.
Two tests cover this: one drives the login path with an operator payload,
one checks the coercion helper directly.
5. Dependency pinning
`requirements.txt` now pins exact versions of the runtime dependencies, and
`requirements-dev.txt` pins the test dependencies. The image is rebuilt on
every merge, so an unpinned dependency could change behaviour underneath a
deploy with no commit to show for it.
6. Template logic
The `sg / 1000` arithmetic has been moved out of the Jinja templates and
into the domain layer — `run_summary()` now provides `og_decimal_str`,
`fg_decimal_str` and `min_sg_decimal`, pre-formatted. No template does
arithmetic on the data any more.
7. Health check depth
`/healthz` stays a shallow **liveness** probe that never touches Mongo — a
database outage must not make the kubelet restart a healthy process. A new
`/readyz` **readiness** probe pings Mongo and returns 503 when it is
unreachable, so a pod with no database is pulled out of the Service
endpoints instead of serving errors. The RUNBOOK documents wiring both.
Low items
---------
8. Mongo aggregation pipeline for `runs()`
Not changed. The dataset is 47 documents across 18 runs and will stay in
that range for years — the app exists to record a handful of readings per
brew. Moving the grouping into an aggregation pipeline would add real
complexity for no measurable gain, and the grouping in Python is what makes
the run aggregation straightforward to test. Noted for whenever the data
volume makes it matter.
9. Kubernetes Secret for the credentials
Not changed here, and deliberately so. The live Deployment carries
`MONGO_PASS` as a plain environment value. Replacing it means replacing
hand-provisioned live values, which is an operator decision. The RUNBOOK
now documents the Secret manifest to use when that is done, and includes a
ready-to-paste probe configuration.
10. Rate limiting on `/login`, Flask-Talisman, password complexity
Not added. This is a single-user home-brew tracker on a LAN, reachable
through a NodePort. Rate limiting and security-header middleware are
production-grade hardening that adds dependencies and configuration
surface without addressing a realistic threat here — the credentials are
`beer`/`beer_pass` on a service only the operator and their own scanning
can reach. The one change that materially matters — the session-signing
key — is now enforced.
11. Least-privilege Mongo user
Not changed. The app connects as `root` today, matching the existing
deployment. Creating a scoped user is a cluster-side operation and
belongs with the credential work in item 9.
Tests
-----
73 tests (up from 64). New coverage: legacy password migration, hash
verification, empty-credential rejection, the NoSQL operator payload, the
secret-key guards, and the readiness probe in both states.
The old UI was two pages of raw pandas tables with no navigation, no styling
and no way to see a run's comments. This replaces it with a small,
testable application and a beer-themed design.
What changed
------------
Restructure
- The domain logic now lives in `brewing.py` as pure functions over plain
data, with no Flask or Mongo dependency. The gravity maths, the ABV
formula and the run-aggregation rules are unit-testable on their own.
- `app_factory.py` replaces the module-level app, and the routes move into
blueprints under `views/`. A request-scoped Mongo repository is created per
request and closed at teardown.
- `mongo/repository.py` replaces `build_db.py` / `query_db.py` / `get_conn.py`.
Reading a document no longer requires a second query to fetch the fields.
Interface
- A dark, beer-themed design system: warm amber `#be7f39` on near-black, a
condensed display face (Anton) for headings and Inter for body text. Both
fonts and the chart library are vendored into `static/`, so the container
needs no outbound network at runtime — the old templates pulled Bootstrap
from a CDN.
- A real navigation bar, a login page, a brew-process page with a proper
form, and a run-detail page listing every reading with its comment.
- The run summary tables now show the beer name and the latest comment, which
the old pandas tables did not.
- Native form controls (checkbox, number spinners, date icon, scrollbars)
are restyled for the dark theme.
Correctness
- The alcohol calculation now uses the standard homebrew formula,
`(OG - FG) x 131.25`. The old code used `(max - min) / 7.36 + 0.5`, which is
not a recognised brewing formula and overstated every result.
- Gravity points and decimal readings now convert correctly in both
directions (`1039` <-> `1.039`).
- A run's display name resolves to the most-used label across its readings,
because `beer_type` is stored per-reading and drifts across a run.
- Re-submitting a reading for the same run and day updates it in place
rather than creating a duplicate.
Security / production
- The container ran the Werkzeug debugger with `FLASK_DEBUG=1`, exposing an
interactive console on any unhandled error. It now runs waitress with
debugging off.
- `SECRET_KEY` comes from the environment.
- The app secret is no longer a hard-coded string in source.
Housekeeping
- Removes the bokeh/duckdb/pandas dependency chain and the static file it
generated at import time; removes the dead `add_user.py` and `test.py`.
- Splits `requirements.txt` (runtime) from `requirements-dev.txt` (tests).
- Adds a pytest suite (64 tests) and a `test` job to the CI workflow so the
image is only built when the tests pass.
- Adds a README describing the data model and a RUNBOOK for the cluster side.
Data compatibility
------------------
The existing `beer_db.beer_data` documents are read as they are and are not
migrated or rewritten. No collection is dropped, and no document is modified
by this change other than through the explicit "record a reading" form, which
keeps the original `(date, beer_run_id)` uniqueness rule.
hermes
armistace