Repository navigation
infra: capture the server's base setup, a rebuild runbook and the firewall fix - #17
Conversation
AbdikhaniAbdirashid
left a comment
AbdikhaniAbdirashid
left a comment
There was a problem hiding this comment.
Approving. Putting Phase 0 into git was overdue, and finding the firewall gap while doing it is exactly why it was worth writing down. Two line comments, both on the runbook rather than the fix.
First, a correction to my own review of #15. I wrote there that "the namespace isn't in git" and asked whether your runbook covered it. That was wrong, and reading this PR is what showed me: relay is defined in k8s/platform/deployer.yaml, and k8s/README.md spells out the order — platform files by hand first, then k8s/app/ by the pipeline. It even explains why a Namespace in k8s/app/ would be wrong, since deployer can't create one. Nothing to fix; the gap I thought I'd found doesn't exist. Sorry for the noise.
The firewall fix, checked from outside. I ran a TCP connect against each port from my own machine:
22 open 6443 closed
80 open 5432 closed
443 open 10250 closed
30795/32338 closed
Exactly what you describe. That NodePort hole was real — curl -k https://<ip>:32338 reaching the hello app means both "layers" of firewall were one layer with a hole in it. Good catch by whoever found it.
One thing I'd add to the reboot section: it says to reboot by hand at a quiet time and check /version afterwards. Worth also saying what to check before: that the last deploy is actually the one running, so a reboot doesn't quietly become the moment an old image comes back.
| - **one.com network firewall:** exactly three rules, IPv4 TCP 22, 80 and 443. No `All` rule | ||
| and nothing for IPv6. ufw can't filter ports that Kubernetes publishes (NodePorts), so this | ||
| firewall is the only thing blocking them. When changing rules, add the new one before | ||
| deleting an old one, so SSH never drops. Check from outside: `nc -z -G 3 <vps-ip> 30795` |
There was a problem hiding this comment.
This test can never fail, so it can never catch a regression.
-G isn't a netcat-openbsd flag (that's the BSD/macOS nc). On Ubuntu it's -w. With -G, nc exits on a usage error before it connects to anything — and the runbook's criterion is "must fail", so it looks like it passed.
I ran it against an open port to be sure:
nc -z -G 3 <vps-ip> 443 exit=1 <- 443 is open
nc -z -w 3 <vps-ip> 443 exit=0 <- correct flag sees it
So nc -z -G 3 <vps-ip> 30795 would report "blocked" even if that NodePort were wide open again.
That's the same shape as the lesson this PR records — "in 0.12 we tested that 80/443 worked, but never that other ports were blocked" — so I'd rather we fix it here than inherit a check we trust and shouldn't.
-w works on both the server and my laptop:
nc -z -w 3 <vps-ip> 30795 # must fail (exit 1)
nc -z -w 3 <vps-ip> 443 # must work (exit 0)Worth running both directions, not just the negative one — a check that only ever asserts failure is how we got here.
| ## 5. GitHub settings (not in git; made with `gh`, stored by GitHub) | ||
|
|
||
| - Ruleset `protect-main`: PR + 1 approval, approval after the last push, no force-push, no | ||
| deletion. Squash merges only. Required check `api-test`. |
There was a problem hiding this comment.
Two of these don't match what's configured right now. I read the ruleset back with gh api repos/:owner/:repo/rulesets:
| Documented | Actual |
|---|---|
| approval after the last push | dismiss_stale_reviews_on_push: false — an approval survives later pushes |
Required check api-test |
no required status checks at all |
The api-test one I think is deliberate: #16's merge plan says to add it last, after #14–#16 are in, so none of them needs a fresh approval. So this line describes the target, not today.
The stale-approval one I haven't seen planned anywhere, and it's the one that worries me more. Right now I can approve a PR and you can push anything to it afterwards and still merge — which is roughly what happened with my #9, where your review sits on a commit two pushes old.
Could this section say which settings are current and which are the end state? A rebuild runbook that quietly makes us stricter is fine; one that claims protections we don't have is worse than saying nothing, because we'd both repeat the claim at the defense.
There was a problem hiding this comment.
Correcting myself: the first half of this is wrong, and your doc was right.
I checked dismiss_stale_reviews_on_push and found it false, and concluded the "approval after the last push" line was aspirational. I'd checked the wrong setting. The full pull_request rule is:
"dismiss_stale_reviews_on_push": false,
"require_last_push_approval": true,
"require_extra_approval_for_unattributed_changes": true,
"required_approving_review_count": 1require_last_push_approval: true is the one that matters. An existing approval isn't deleted on push, but the PR still can't merge until someone who isn't the pusher approves the latest push. Net effect is exactly what the runbook says.
I found out the hard way: #16 went to REVIEW_REQUIRED the moment you pushed d00063d, even with my approval of 67b29dd still on it. The protection works, and I'd just told you it didn't exist.
Sorry — and specifically withdraw the line about #9. Nothing could have been pushed past my approval there.
The second half still stands. Required status checks are genuinely empty (required checks: []), so the api-test line in the runbook is the target state rather than today's. That's deliberate per #16's merge plan, which adds it last. Worth marking which of the two it is, but the stale-approval row needs no change at all.
…20) Closes Phase 1 in the docs: Abdikhani's review points from #17 and #18, plus what the live tests showed. ## From the reviews - **The port check works on both macOS and Linux, and in both directions** (#17). `nc -G` is macOS-only: on Ubuntu's netcat-openbsd it's a usage error, so `nc` exits 1 before connecting, and a "must fail" check passes even when the port is open. The runbook and CONTEXT now say: 443 must answer AND 30795 must not. On macOS use `-G`, on Linux `-w`. - **Before a reboot,** check that `/version` shows `origin/main`'s commit, so a reboot can't quietly bring back an old image (#17). - **GitHub settings, as they are now** (#17): the required checks `api-test` and `deploy-script-test` have been on since 29 Oct, and the runbook explains what `require_last_push_approval` really does. - **ADR 0004's check** uses the tag list instead of one guessed tag (#18). A public package answers with its tags; a private one answers `DENIED`. Tested as written: it lists the 4 images built so far. ## Phase 1 is done - **"Where we are":** every merge is tested, built, deployed and checked from outside. - **Two replicas on one node** are zero-downtime rollouts plus protection against one crashing process, **not** high availability (from the #15 review). - **Git workflow:** the required checks, and that an approval counts only for the newest push. ## The live tests (29 Oct) - **Zero downtime:** we deployed #17 while `ab` sent traffic (20 clients, keep-alive, 5 min). **76,466 requests, 0 failed.** The rollout took 4 s, and the image pull about 1 s. - **Rollback:** we deployed #15's merge commit by hand. It has no image, so it ended in ErrImagePull. After 180 s, `relay-deploy` redeployed the last good commit. During the test, **119,824 requests, 0 failed**, and all **1,787 `/version` checks answered the live version**. The old pods were never stopped.
## What Step 5.2: the Deployment, Service and Ingress that put the dashboard on `https://cdn.myrelay.tech`. One file in `k8s/app/`, so `relay-deploy` applies it on every merge to main, with the image tag replaced by the commit's SHA. Same shape as the API's manifests, with four differences: | | Why | |---|---| | Port 8080, not 80 | `nginx-unprivileged` runs as uid 101 and can't bind a low port | | Readiness + liveness probes on `/index.html` | a rollout never sends traffic to a pod whose nginx hasn't started | | `cpu: 10m`, `memory: 24Mi` requests, 64Mi limit | the VPS is shared with Postgres, the API and Grafana; serving static files needs almost nothing, and the requests tell the scheduler this pod is cheap | | `cdn.myrelay.tech` + `letsencrypt-prod` | its own host and certificate | Two pods with `maxUnavailable: 0`, and the image tagged by git SHA rather than `latest`, so a rollback can name an exact build. ## Changed since the draft - **Moved from `k8s/dashboard/` into `k8s/app/`.** As written, `relay-deploy` would never have applied it: it only globs `"$REPO"/k8s/app/*.yaml`. The pipeline would have gone green and deployed nothing. - **Image tag `PLACEHOLDER_SHA` -> `__GIT_SHA__`.** `relay-deploy` only substitutes `__GIT_SHA__`, so the tag would have stayed as literal text: `ImagePullBackOff`, the rollout never ready, and the whole commit rolled back — API included. - **Added a 5 s `preStop`.** Removing a pod from the Service and telling it to stop happen at the same moment, but Traefik needs a moment to hear about the removal. Without the pause nginx is already shutting down while Traefik still routes here, and those requests fail. `maxUnavailable: 0` doesn't cover this: it guarantees a replacement is ready before the old pod goes, but says nothing about the old pod's last seconds. The API has the same 5 s. This was the one piece of our four-part zero-downtime recipe the dashboard was missing. `terminationGracePeriodSeconds` stays at its default 30 s, which comfortably holds the 5 s; nginx serves static files, so there is nothing to drain afterwards. - **Labels now follow the API's convention:** `app.kubernetes.io/name: relay-dashboard` instead of `app: relay-dashboard`, on the Deployment, its selector, the pod template and the Service selector. A Deployment's selector is immutable once it exists, so this is free now and a delete-and-recreate later. Checked that all three still agree — a selector that doesn't match the pod labels gives a Service with no endpoints, which looks like a broken app rather than a broken label. ## Pod Security: this would have been refused The file was first written on 26 Oct, before #21. Checked it against the `restricted` level that #21 puts on namespace `relay`, and it was missing two of the four required fields — `runAsNonRoot` and `seccompProfile`. The image already runs as uid 101, but Kubernetes doesn't look inside the image: being non-root has to be declared or the API server refuses the pod. Verified rather than reasoned about, in a throwaway namespace `ps-test` carrying the same labels (`restricted`, `enforce-version: v1.36`), deleted afterwards: | Manifest | Result | |---|---| | This PR's file | `deployment/service/ingress created (server dry run)` — no warnings | | Same file with the two fields removed (control) | `Warning: would violate PodSecurity "restricted:v1.36": runAsNonRoot != true …, seccompProfile …` | The control is the point. A clean dry run on its own doesn't show the check is running — same trap as the `nc -G` check in #17 and the `externalIPs == null` rule in #21. The control names both fields, so the pass means something. One note on Pod Security worth writing down somewhere: it enforces on **Pods**, not on Deployments. Applying a violating Deployment **succeeds** with a warning, and then the pods never appear. So `kubectl apply` returning cleanly is not evidence — the warning line is. Re-checked after the move and the `preStop` addition, against namespace `relay` itself with the commit's real SHA substituted: ``` deployment.apps/relay-dashboard created (server dry run) service/relay-dashboard created (server dry run) ingress.networking.k8s.io/relay-dashboard created (server dry run) ``` ## The image exists #30 is merged, so the pipeline now builds, smoke-tests and pushes `relay-dashboard:<sha>` on every merge to main — not only when the dashboard changed, because the tag has to exist for whatever commit the deploy applies. Confirmed against the registry rather than trusting the green tick: ``` relay-dashboard:c145056 -> HTTP 200 ``` That is also why the image and the manifest are two PRs and not one: the tag has to exist before a manifest references it, or the first rollout fails on a missing image. ## Not in this PR - `readOnlyRootFilesystem` stays `false`: nginx writes its cache and pid file under `/var`, so turning it on means emptyDir mounts for both. `restricted` doesn't require it. Happy to add it if you'd rather — your Postgres does the equivalent. - Nothing else. The label mismatch that used to be listed here is fixed in this PR instead of deferred (see "Changed since the draft"). ## For the reviewer - Are the resource requests sensible next to Postgres, or should I leave more headroom? - Liveness probe on a static file server: worth having, or the same objection as Postgres? - The certificate is new, so expect a minute or two of a self-signed warning on `cdn.myrelay.tech` right after the merge while cert-manager finishes the ACME challenge. --------- Co-authored-by: Abdikhani Abdirashid <Abdikhani_abdirashd@hotmail.com>
Phase 0 was set up by hand on the server, so its settings only existed there and in our notes. This PR puts them in git, with the order to rebuild a lost server. It also records a firewall gap that an independent review found, and which is now fixed.
The firewall gap (fixed 29 Oct)
IPv4 All 0.0.0.0/0rule and anIPv6 All ::/0rule, so it filtered nothing. Our "two layers" was really one layer: ufw.curl -k https://<vps-ip>:32338returned the hello app.Allrules. The panel now has exactly IPv4 TCP 22, 80 and 443.ssh relayworks;https://api.myrelay.techand the ACME check path answer 200.What's in it
infra/server/sshd-hardening.conf: keys only, no root login. Installed as/etc/ssh/sshd_config.d/00-hardening.conf, where the00-makes it win over one.com's50-cloud-init.conf.infra/server/k3s-config.yaml:secrets-encryption: true. It must be in place before k3s is installed.infra/server/firewall.sh: the ufw rules as a script. Its comment explains the limit of ufw with NodePorts.infra/server/README.md: the rebuild runbook, in order:Automatic-Rebootstays off on purpose.docs/CONTEXT.md:Checked against the live server
diff, ignoring comments).sudo ufw status verboseshows exactly the rules infirewall.sh.k3s v1.36.5+k3s1andcert-manager-controller:v1.21.2.