_as_doc_id returned None for a hostile id. That was guarded at the one
call site, but the helper could still be misused into "$in needs an
array" - and a caller who forgot the guard would have got an error
rather than a safe no-op.
It now returns an EMPTY LIST for anything that is not a reading id. An
empty list is a valid $in argument that matches nothing, so the helper
cannot produce a query error, cannot match every document, and cannot be
interpreted as a query operator.
Verified against the live database: a hostile id matches 0 documents and
updates 0 documents.
164 tests.
The review of PR #14 found a critical defect and two real bugs. All
verified against the live database, not just read.
CRITICAL - the advice would never have been stored
--------------------------------------------------
`_as_doc_id` accepted only an ObjectId or a string that parses as one.
But **all 47 live readings have string `_id`s** - 24 random characters
from the original app's importer, like `l78ZidBJ3QaYn98tvB88x4uM`. The
helper turned every one of them into None, so the update matched nothing
and the advice was silently never stored in production.
The mongomock fixture generates ObjectIds, so the tests could not see it.
`_as_doc_id` now returns both forms and the query uses `$in` (a plain
list would be an exact ARRAY match in Mongo, which is not what is
wanted). Verified against the live database: the advice now lands on a
real reading, with the brewer's comment untouched.
There are now tests for BOTH id shapes, with the string shape built
explicitly, because the fixture would not produce it.
The advice was written about the wrong reading
----------------------------------------------
`latest` was taken from the date-sorted tail of the run, so editing an
OLDER reading generated advice about the NEWEST one. The advice is now
written against the reading that was actually saved.
A leaked MongoClient per reading
--------------------------------
The background thread built a `BeerRepository()` - and therefore a
`MongoClient` - for every advice write, and never closed it. A
MongoClient spawns three monitor threads and pymongo does NOT reap them
on `close()`; they go only when the client is garbage collected.
Measured: three threads and a connection pool accumulated per reading
recorded. There is now one shared repository for the life of the
process, which is what the request-scoped repository does too. Measured
again after the fix: 4 threads total, not 16 over five writes.
Also
----
- `BeerRepository` gained `close()` and the context-manager protocol.
- The advice writer now logs instead of dying silently if the write
fails.
164 tests (up from 160).
Two things I wanted to be sure of before this merges.
Stale advice
------------
The advice is written for the gravity that was there when it was
generated. If the brewer then edits the reading - to fix a typo, or to
mark it final - the old advice no longer describes it, and the page was
showing advice that contradicted the number next to it.
`upsert_reading` now clears the advice fields, so the stale advice goes
and a fresh one is generated (the form already regenerates on every save).
Only the advice fields are cleared - the brewer's own comment is never
touched.
The injection guard
-------------------
The reading's `_id` is an ObjectId, so the original `_as_id` coercion
(which turns run ids into plain strings, to stop a caller passing a dict
that Mongo would interpret as a query operator) did not apply - it turned
the ObjectId into an empty string and the update silently matched
nothing. `_as_doc_id` accepts only an ObjectId or a string that parses as
one.
Verified against a live injection attempt: `{"$ne": None}`, `{"$gt": ""}`,
a list, an int, None and a non-ObjectId string all return None and match
nothing. There is now a test that pins it, because it is a security
property that would otherwise only be covered by reading the code.
160 tests (up from 158).
Stores a model's advice for each recorded reading, in its own field, so the
brewer's own comment is never overwritten.
Fire and forget: the advice is generated in a background thread, so
recording a reading never blocks on the model. A failure is recorded as a
visible note saying the advice could not be generated, because a silent
failure is indistinguishable from "still thinking".
The model is `glm-5.3:cloud` on the Ollama instance at 192.168.178.45:11434.
Several models on that instance are retired and fail at call time while
still appearing in the model list, so the default is one that was probed.
The system prompt tells the model never to call a reading impossible: a
gravity below 1.000 is always a mis-entry, but the rest of the run may be a
genuine long fermentation and still worth understanding.
150 tests.
A second, deeper review of PR #12 found five things the first review did
not. All are fixed here.
The band was not the band
-------------------------
The fermentation curve's shaded range was drawn as ONE dataset with
`fill: "+1"`, which fills to the NEXT dataset - and the next dataset was
the median line, not the maximum. So the band shaded min-to-median, the
upper half of the range was drawn as a hairline but never shaded, and the
tooltip said "range min to max" while the picture showed something else.
The band is now two datasets - the min curve filled down to a max curve -
with the max hidden from the legend.
Rising gravity
--------------
`attenuation_percent` rejected a final gravity below 1.000 and one
implying more than 100% attenuation, but a run whose gravity went UP
produced a NEGATIVE attenuation - which was then averaged into the
headline figure and plotted as a negative bar. Verified against the live
code: OG 1010 -> FG 1020 gave -100.0%. Now rejected, and reported by
`implausible_runs` with the reason "the gravity went up, which is usually
a mis-ordered pair of readings".
Undated readings
----------------
An undated reading sorted to the front of the run, so it became the
original gravity - which put the OG after the FG and produced a negative
attenuation. The OG now comes from the earliest *dated* reading, and
`days_to_target` skips the readings it cannot place on the timeline rather
than giving up on the whole run. Verified: readings 1050 (dated), 1005
(undated), 1010 (dated) gave None instead of 24 days, and an OG of 1005
instead of 1050.
Active runs on the curve
------------------------
The curve chart filtered on `curve.length > 1` only, so it plotted the
active runs as well - a partial fermentation on a chart meant to describe
how a completed one behaves. It now excludes them, along with the
suspect runs.
Four fetches, one payload
-------------------------
`/api/runs` carries every reading of every run, and four charts each
fetched it independently - so the analysis page made two identical
round-trips of the whole collection. There is now one shared fetch.
Also
----
- The "Readings that need a second look" table links to the run, so a
suspect reading can be corrected in one click.
- `days_to_1015` and the `curves` variable were computed and never used.
Removed.
- A reading below 1.000 now says what it is - "beer cannot be less dense
than water, so this is almost certainly a typo (a 1.012 reading entered
as 012?)". The reading is still SAVED: rejecting it would lose the
brewer's actual observation, and seeing it recorded is how they spot the
mistake.
142 tests (up from 134).
The re-review returned APPROVED with two low-severity notes.
- The prose said "Across 15 finished brews", which reads as though there
are only 15 brews. It now says "Across 15 of 16 finished brews", so the
exclusion is visible in the sentence itself and not only in the card
header.
- `/api/runs/<run_id>` did not carry the `suspect` flag that
`/api/runs` does, so a consumer of the detail endpoint could not tell an
impossible run from a good one. It does now, with a test.
134 tests.
Review finding, and it was right. The PR claimed that runs with impossible
readings were "excluded from the averages". That was only true of the
attenuation average. Run 12's bad final gravity was still poisoning three
other statistics:
- **Brew time** - its 56 days were folded into the average, making a
typical brew look like 13.7 days when it is 10.9.
- **Time to 1.010** - its 56 days did the same to that figure (8.4d).
- **The history page's ABV** - an 18.64% ABV, which made run 12 the
"strongest" beer in the history and pulled the average up.
- **The fermentation curve** - its curve plunges from 1.042 to 0.900, which
dragged the median line down and stretched the min-max envelope to cover
a gravity below water.
One bad reading was reaching four different statistics.
What changed
------------
- The suspect runs are computed once and excluded from every aggregate:
the attenuation, brew-time and time-to-1.010 averages, the fastest and
slowest brews, the history page's ABV average, and its strongest and
lightest.
- `/api/runs` now carries a `suspect` flag, and the charts skip the flagged
runs - the attenuation chart and the fermentation curve.
- The suspect run stays in the table, with a "not counted" chip and a tinted
row, so it can be corrected. It is excluded from the figures, not
hidden. The card header says "16 brews (15 counted)" and the prose says
how many brews the figures are based on.
Tests
-----
133 (up from 127). New coverage: the suspect run is excluded from the brew
time and time-to-1.010 averages, it is still listed for review, it is
flagged in the API, it is not the strongest brew in the history, and the
charts filter it out.
The `logged_in` fixture moved to conftest.py, where both test modules can
use it.
The RUNBOOK still said the workflow did not deploy to the cluster. It does
now - it builds, pushes, applies the manifests, rolls the pods and verifies
the app from inside the pod. The manual deploy path is documented alongside
it, along with the dry-run step that catches the schema errors kubectl apply
silently swallows.
Adds the analysis page the old app never had: how consistently the brews
ferment, and how long each one takes.
What it shows
-------------
- **Attenuation by brew** - what fraction of the original gravity was
fermented away, for every finished brew.
- **Fermentation curves** - the typical shape of a fermentation, drawn as a
median curve with the min-max envelope behind it. Plotting every brew
individually gave 16 near-identical lines and an unreadable legend.
- **Every finished brew** - OG, FG, ABV, attenuation, days in the fermenter,
days to reach 1.010, and the finish date.
- **Readings that need a second look** - readings outside the plausible
hydrometer range.
- Headline figures: average attenuation and its spread, average brew time,
average time to 1.010.
Data quality
------------
One live run (run 12) has a final gravity of 900, which worked out as 338%
attenuation and dragged the average to 103.2%. That is not a real result,
so:
- Attenuation is rejected when the final gravity is not physically possible
for the original gravity.
- Runs that fail that check are EXCLUDED from the averages rather than
silently distorting them, and are listed on the page as needing review.
- The average now reads 87.5% +/- 7.4 percentage points across 16 brews.
Chart bugs found while building this
------------------------------------
- **The gravity charts had their y axis reversed.** With `reverse: true`
the high reading sits at the BOTTOM, so the fermentation curve climbed
left to right - it read as gravity *rising* as the beer fermented. A
brewer reads a fermentation as a descending line. The point-scale run
chart on the run detail page had the same bug. Both are fixed.
- **The curve chart's x axis was treated as categorical.** The x values are
numeric (0-100), and Chart.js defaults a line chart's x axis to
category, which collapsed every point onto index 0 and rendered the
curve as a sliver at the origin. The axis is now explicitly linear.
- **The curve was interpolated by exact key lookup**, so it only worked when
every brew happened to be read at the same progress points. It now
interpolates between a brew's actual readings, and the curve carries 21
points across the full range.
- **Long beer names were clipped** at the left edge of the attenuation
chart ("...x crying out loud"). Names are trimmed to 18 characters with
an ellipsis; the tooltip and the table carry the full name.
Tests
-----
127 (up from 105). The two chart regressions are covered by reading
`charts.js` and asserting the gravity axis is not reversed and that long
names are trimmed - these are exactly the kind of change that silently
reverts, and they are invisible to a screenshot test.
I had built a parallel mechanism for this repo rather than copying the
workflow that is already known to work on this cluster. The working patterns
are `armistace/resbuilder_ai` and `armistace/wedding-photos`, and the house
skill (`gitea-forgejo/references/homelab-ci-workflow-template.md`) documents
them. This rewrites the workflow to match them.
What changed, and why each piece was wrong:
- ONE job, not three. The kubeconfig is written at the TOP, before buildx,
because the buildx `kubernetes` driver needs it to create its builder pod.
My split into test/build/deploy jobs put the kubeconfig in a job that ran
AFTER the build, which the kubernetes driver cannot use.
- `driver: kubernetes` with `namespace=gitea-runner`, not the `remote`
driver. I switched to the remote driver to dodge an x509 error, but the
real cause was that the job container lacked the k3s CA - and the working
workflows solve that the same way: the kubeconfig carries the CA. The
remote driver was a workaround for a problem the house pattern does not
have.
- The CA-refresh script and the pinned-fingerprint file are deleted. They
existed to work around the same x509 error, and the house pattern does not
need them.
- The deploy step installs kubectl and applies `kube/*.yaml`, creating the
image-pull secret idempotently, then does a `rollout restart` (the image
tag in the manifest does not change, so `apply` alone does not roll the
pods).
- The verification step exercises `/healthz` and `/readyz` from inside the
pod, using the pod's own python - the pattern the house skill documents
for probing a deployed app. There is no `curl` in the slim image.
The Deployment manifest also changes:
- `runAsUser: 999` at the POD level, matching the `beer` user in the image.
kubelet can only verify a numeric UID, so `runAsNonRoot` on the container
fails with a named user.
- `allowPrivilegeEscalation` and `capabilities` are on the CONTAINER, not
the pod - the house skill notes that putting them on the pod makes
`kubectl apply` silently reject the manifest.
- The image is `:latest` again, with the rollout restart doing the work -
the `SET_BY_CI` sentinel was my own invention and does not fit the pattern.
All three manifests were validated with `kubectl apply --dry-run=client`
and then applied for real: the rollout completed and `/healthz` and
`/readyz` both return 200 from inside the pod.
The cluster-count guard I added used `grep -c '"name"'`, which counts
LINES. The jsonpath output is a single line, so the count was 1 for a
kubeconfig with clusters and 1 for one without - which made the guard
reject every kubeconfig:
ERROR: the kubeconfig has no cluster entries.
The count is now done with `grep -o . | wc -l`, which counts occurrences
rather than lines. Tested against both cases:
- a real kubeconfig with a stale CA -> the CA is refreshed and kubectl works
- a kubeconfig with no clusters -> refuses with a clear message
Adds the pages that were missing: an "Active Beers" view of what is in the
fermenter right now, and the per-run metadata (canonical name, tap number,
original gravity, brew notes) that could not be derived from the readings.
Why the metadata is needed
--------------------------
The `beer_data` collection stores a beer name on *each reading*, and that
name drifts within a run - run 4 was entered as "Necatron-Christmas Beer",
then "Nocturna the Christmas beer", then "Nocturna Christmas beer". There was
nowhere to record a tap number, a confirmed original gravity, or a note about
the brew.
The new `beer_runs` collection holds one document per run with exactly those
details. It starts empty; `scripts/backfill_run_metadata.py` populates it
from the existing readings. The script is idempotent, only ever inserts into
`beer_runs`, never touches `beer_data`, and never overwrites a name you have
already set by hand.
The active beers page
---------------------
A card per brew that is still in the fermenter, showing the tap number, the
current gravity, the projected alcohol, how many days it has been brewing, a
sparkline of the gravity so far, and the latest comment.
A run with only one reading and no recorded original gravity has no reliable
estimate, so the alcohol figure shows a dash rather than a misleading number
- record the OG or take a second reading and it appears. The same applies to
the existing reads, where the legacy app's assumed OG of 1012 produced
*negative* alcohol for a perfectly normal OG.
Also adds the history page: alcohol by brew, brews per month, every run in a
table, a comparison of the standard ABV formula against the legacy one, and
a list of readings that fall outside the plausible hydrometer range.
Changes to the existing pages
-----------------------------
- The brew-process page and the run detail page now show the canonical run
name and the latest comment, and the run detail page lets you edit the
metadata inline.
- Gravity is displayed as a decimal reading (1.039) with the point scale
beneath it (39 points), which is the way a brewer reads it. Labelling the
stored `1039` as "1039 points" would have been wrong.
- A run's display name now prefers the name on its *final* reading - the one
written when the brew was finished, so the name the operator settled on -
falling back to the most-used name.
- `upsert_run_metadata` now clears a field when it is passed as `None`, so a
name or tap number can be deleted. `upsert_run_metadata_partial` is the
non-clearing variant, used by the backfill.
Bugs found and fixed along the way
----------------------------------
- The chart script was included both in the base template and again by each
page, so every chart was drawn twice. A test now asserts it is loaded
exactly once per page.
- The chart library was never referenced by the base template, so all charts
were silently dead. That was fixed in the previous PR.
- An unfinished run reported a *measured* ABV. `abv_percent()` now returns
None for a run that is still in the fermenter - its figure is a projection,
not a measurement. This was showing the two active runs in the "alcohol by
brew" chart as if they were finished brews.
- The `/api/runs` endpoints the charts fetch from did not exist, so every
chart was silently empty. They are back, with tests.
- The "brews over time" chart left out the months with no brews, which
spaced the busy months evenly and made the chart read as continuous
activity. The quiet months are now included with a zero.
Tests
-----
105 (up from 74). New coverage: the active page contents, the tap number,
the projected alcohol, the "needs 2nd reading" case, metadata saving and
clearing, the stored name overriding the drifting reading name, the next
free tap number, the partial-update semantics, the history page, the
outlier list, and the point-scale formatting.
Data safety
-----------
`beer_data` is untouched by this change. The backfill script only inserts
into `beer_runs` and skips any run whose name has already been set by hand.
Round-2 review findings.
- `_ensure_indexes` swallowed a `PyMongoError` with a bare `pass`. An index
failure does not stop the app serving, but it means the lookup is a
collection scan, which gets slow as the data grows — so it is now logged
as a warning with the traceback.
- The Content-Security-Policy string is now a module-level `CSP` constant
rather than inline in the response handler, and it gained
`object-src 'none'` (the app uses no plugins). The test now also asserts
that neither `unsafe-inline` nor `unsafe-eval` appears — either would
defeat the point of the policy.
- `api_run` returned a bare dict for its 404 while the success path used
`jsonify`. It is now `jsonify({"error": "not found"})` for consistency.
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.
Review findings, all from the independent review of this PR
------------------------------------------------------------
- Mongo indexes: the run metadata is always looked up by `beer_run_id` and
the readings by `(beer_run_id, date)`. Both are now indexed.
- Content-Security-Policy: the app serves only its own vendored assets, so
a strict `default-src 'self'` policy is enforceable. Adding it required
removing the inline `style=` attributes from the templates (they became
`.mb-lg` / `.mt-lg` utility classes) and the one inline `<script> (the run
id now travels as a `data-run-id` attribute). A test asserts no template
has either.
- Job timeouts: the `test` and `build` jobs now have `timeout-minutes`, so
a wedged build stops occupying a runner.
- OG range: raised from 1300 to 1500 points, so a strong specialty brew
(a 1.150 wort is real) can be recorded.
Bugs found and fixed along the way
----------------------------------
- An unfinished run reported a *measured* ABV. `abv_percent()` now returns
None for a run that is still in the fermenter — its figure is a projection,
not a measurement. This was showing the two active runs in the "alcohol by
brew" chart as if they were finished.
- The `/api/runs` endpoints the charts fetch from did not exist, so every
chart was silently empty. They are back, with tests.
- The "brews over time" chart left out the months with no brews, which
spaced the busy months evenly and made the chart read as continuous
activity. The quiet months are now included with a zero.
- The run-detail chart was dropped when PR 1 was scoped down, leaving dead
code behind. It is restored.
Tests: 105 (up from 91).
Adds the pages that were missing: an "Active Beers" view of what is in the
fermenter right now, and the per-run metadata (canonical name, tap number,
original gravity, brew notes) that could not be derived from the readings.
Why the metadata is needed
--------------------------
The `beer_data` collection stores a beer name on *each reading*, and that
name drifts within a run - run 4 was entered as "Necatron-Christmas Beer",
then "Nocturna the Christmas beer", then "Nocturna Christmas beer". There was
nowhere to record a tap number, a confirmed original gravity, or a note about
the brew.
The new `beer_runs` collection holds one document per run with exactly those
details. It starts empty; `scripts/backfill_run_metadata.py` populates it
from the existing readings. The script is idempotent, only ever inserts into
`beer_runs`, never touches `beer_data`, and never overwrites a name you have
already set by hand.
The active beers page
---------------------
A card per brew that is still in the fermenter, showing the tap number, the
current gravity, the projected alcohol, how many days it has been brewing, a
sparkline of the gravity so far, and the latest comment on a reading.
A run with only one reading and no recorded original gravity has no reliable
estimate, so the alcohol figure shows a dash rather than a misleading number
- record the OG or take a second reading and it appears. The same applies to
the existing reads, where the legacy app's assumed OG of 1012 produced
*negative* alcohol for a perfectly normal OG.
Also adds the history page: alcohol by brew, brews per month, every run in a
table, a comparison of the standard ABV formula against the legacy one, and
a list of readings that fall outside the plausible hydrometer range.
Changes to the existing pages
-----------------------------
- The brew-process page and the run detail page now show the canonical run
name and the latest comment, and the run detail page lets you edit the
metadata inline.
- Gravity is displayed as a decimal reading (1.039) with the point scale
beneath it (39 points), which is the way a brewer reads it. Labelling the
stored `1039` as "1039 points" would have been wrong.
- A run's display name now prefers the name on its *final* reading - the one
written when the brew was finished, so the name the operator settled on -
falling back to the most-used name.
- `upsert_run_metadata` now clears a field when it is passed as `None`, so a
name or tap number can be deleted. `upsert_run_metadata_partial` is the
non-clearing variant, used by the backfill.
Bugs found and fixed along the way
----------------------------------
- The chart script was included both in the base template and again by each
page, so every chart was drawn twice. A test now asserts it is loaded
exactly once per page.
- The chart library was never referenced by the base template, so all charts
were silently dead. That was fixed in the previous PR.
Tests
-----
91 (up from 74). New coverage: the active page contents, the tap number, the
projected alcohol, the "needs 2nd reading" case, metadata saving and
clearing, the stored name overriding the drifting reading name, the next free
tap number, the partial-update semantics, the history page, the outlier
list, and the point-scale formatting.
Data safety
-----------
`beer_data` is untouched by this change. The backfill script only inserts into
`beer_runs` and skips any run whose name has already been set by hand.
Round-2 review findings, all low severity.
- `curl` now has `--connect-timeout` and `--max-time`, so an unreachable API
server fails the job instead of hanging the runner indefinitely.
- A guard for a kubeconfig with no cluster entries, or an empty one, with a
clear message rather than an opaque kubectl error.
- The fingerprint mismatch message now prints both the expected and the
actual value, which cuts the time to diagnose the next CA rotation.
- `set -euo pipefail` was already there; the script now passes shellcheck
clean, and a shellcheck step was added to the CI test job so the
operational scripts get the same treatment as the Python code.
The CA fingerprint is not a secret. It is a public value that anyone can
read straight off the certificate, so pinning it in a Gitea secret was the
wrong shape:
- A change to it would be invisible - no commit, no review.
- Nobody with secret-write access could silently repoint the CI runner at a
different CA.
It now lives in `scripts/cluster-ca.sha256`, so changing it is a reviewable
commit. The script reads it from there, and still accepts an override as
its first argument.
This also removes the need for a `KUBE_SERVER_CA_FINGERPRINT` secret, which
the bot token cannot create anyway - the Actions secrets API is owner-only.
The behaviour is unchanged: the CA is fetched over an unverified connection
but only USED if it matches the pinned fingerprint, so an intercepted CA is
refused. Re-tested against the live API server - the matching fingerprint
refreshes the CA and kubectl then works; a wrong one is refused and the
stale CA is left in place.
The review was right on both counts, and both are fixed.
1. MITM. Fetching the CA over an unverified connection (`curl -sk`) and then
trusting it is a privilege-escalation hole: the runner holds cluster-admin
credentials, so an attacker able to intercept the fetch could hand us a CA
of their choosing and then read everything sent over the connection.
The CA is now PINNED. It is still fetched over an insecure connection, but
it is only USED if its SHA-256 fingerprint matches
`KUBE_SERVER_CA_FINGERPRINT`. An intercepted CA will not hash to the
pinned value, so it is refused. If the secret is not set, the job fails
rather than trusting an unverified CA.
2. The hardcoded cluster name. `kubectl config set-cluster default` creates a
NEW cluster entry when the kubeconfig's entry is called something else,
leaving the original (and its stale CA) in use. The name is now read from
the kubeconfig: `{.clusters[0].name}`.
The bash is also moved out of the YAML into `scripts/kube-ca-refresh.sh`, so
it can be read, reviewed and tested on its own rather than as a wall of
shell inside a workflow file.
All three paths are tested against the live API server:
- no fingerprint supplied -> refuses, exit 1
- wrong fingerprint -> refuses, exit 1
- correct fingerprint -> refreshes the CA, exit 0, kubectl then works
The deploy job failed on its first run:
error: error validating "kube/beer_namespace.yaml": error validating
data: failed to download openapi: Get "https://192.168.178.160:6443/openapi/v2":
tls: failed to verify certificate: x509: certificate signed by unknown authority
This is the same class of problem as the buildx driver earlier: the
certificate-authority-data embedded in the kubeconfig secret does not match
the CA the API server is currently presenting.
Rather than requiring the secret to be regenerated by hand every time the
cluster's CA is rotated, the job now refreshes it:
- The API server publishes its own CA at `/cacerts`.
- If kubectl cannot verify the server, the job fetches that CA (over an
insecure connection — verifying it is the problem being solved) and pins
it explicitly with `kubectl config set-cluster`.
- It then retries, and fails with a clear message if that still does not
work.
Verified by deliberately corrupting the CA in a working kubeconfig: the
failure reproduces exactly, and the recovery path restores connectivity
(`Server Version: v1.34.5+k3s1`).
The secret is also switched from `KUBEC_CONFIG_BUILDX` to
`KUBEC_CONFIG_BUILDX_NEW_2`, which is the one the working `wedding-photos`
workflow uses.
Two small changes to the deploy job:
- The registry password went through `-u armistace:$PASSWORD` on the curl
command line, where it is visible in the process list for the duration of
the request. It now goes through a curl config file on stdin instead.
- The kubeconfig is written with mode 600 rather than the default, and is
removed at the end of the job (with `if: always()`, so it is removed even
if a later step fails). It is cluster-admin-equivalent, so it should not
be readable by anything else on the runner.
Verified: `curl --config -` against the registry returns the same digest as
the `-u` form.
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