Reconcile the claims five merges left behind - #10
Conversation
A sweep for inconsistencies rather than new work. The last five merges added Azure resources and controls; several summary claims never followed, so the overview now describes a cluster that no longer exists. Factually wrong, now corrected: - README said backups were "the next planned addition". Velero has been deployed at wave 4 with two schedules since before this, CNPG has its own ScheduledBackup, and maintenance.md documents the whole strategy. - README said the residual Azure surface was "one Bicep template". There are six, four of them standalone durable resources, and the Portability table had no row for audit logging at all — which has no portable equivalent, since the API-server audit feed is the provider's. - README and maintenance.md both asserted Kubernetes 1.36.2. The bicep pins the minor alias deliberately because the patch channel owns the patch level, so both files contradicted their own guidance — the README's own note says to check kubernetesVersion in the bicepparam, and maintenance.md's AKS section says patches are automatic. - The repo-layout block listed four of six Bicep files. Omitting backup-storage was defensible as a partial aid; missing three of six reads as a wrong inventory. - README's prerequisite list and install.md's INFRA_RG comment both enumerated what the durable RG holds without the audit workspace or the alerts. - The overview's "what the cluster actually enforces today" line did not mention audit logging, a control spanning two merged PRs. Style, within one file: the bicepparam's audit section marker was three lines where the file's other marker is one. Also fixes two runbook command blocks I collapsed onto single lines by writing them through a heredoc that ate the backslash continuations — §11's archive commands and §5c's action-group check. They ran correctly but broke the runbook's convention and were unreadable at 150 characters. Documents the 365-day archive properly, now that it is the accepted decision rather than a proposal: - loganalytics.bicep read as a flat retentionInDays: 30, so the IaC understated the real configuration. It now says the archive is set per-table in §11 and why it cannot live in the Bicep. - Records that retention is NOT retroactive, which is what makes the §11 step matter: raising retention later does not recover rows that have aged out, so every day it is deferred on a running cluster silently spends a day of history. Durable across a rebuild, since the workspace outlives the cluster. - States the access bar the exposure implies: read on this workspace is read on every secret in the cluster, to be granted at the same bar as a Key Vault role. A year of archived rows widens that rather than narrowing it. No behaviour change. Bicep compiles clean and every parameter value is byte-identical.
There was a problem hiding this comment.
Pull request overview
This PR performs a documentation/consistency sweep to align the repo’s high-level claims (README + runbooks) with the Azure/Bicep resources and controls introduced by the last several merges, explicitly calling out audit logging/alerting and the Kubernetes version pinning approach.
Changes:
- Update README to reflect current enforcement (including audit logging), infrastructure layout, portability touchpoints, and backup status.
- Clarify Log Analytics retention/archival handling (archive configured per-table in the runbook, not in Bicep) and restore runbook line-wrapping conventions.
- Align version-pin documentation to the intentional “Kubernetes 1.36 minor alias” approach.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates overview, portability touchpoints, infra layout, and status claims to match current platform controls/resources. |
| infra/loganalytics.bicep | Clarifies retention vs archive responsibilities (archive set via runbook after table exists). |
| infra/env/webservices.bicepparam | Simplifies audit-logging comment while keeping parameters unchanged. |
| docs/maintenance.md | Aligns Kubernetes pin documentation to the minor-alias strategy. |
| docs/install.md | Updates durable RG description and restores readable multi-line command formatting; documents non-retroactive retention implications. |
| docs/decisions.md | Adds explicit rationale about non-retroactive retention and the access implications of workspace read permissions. |
Suppressed comments (2)
README.md:66
- README storage section says MinIO “backs … backups” and implies “no Azure data PaaS”, but the same README later states Velero backs up to Azure Blob, and the repo’s infra layer provisions durable Azure Blob storage for backups (infra/backup-storage.bicep). This makes the storage overview internally inconsistent.
This issue also appears on line 199 of the same file.
Storage is in-cluster and portable: **MinIO** for S3-compatible object storage
(backs Loki, Thanos, and backups) and **CloudNativePG** for PostgreSQL — no Azure
data PaaS. Persistent volumes use a cheap StandardSSD StorageClass by default,
with a Premium class available opt-in.
README.md:202
- Portability section claims “In-cluster MinIO + CloudNativePG mean no Azure data PaaS”, but backups are explicitly using a durable Azure Blob storage account (Velero row above; infra/backup-storage.bicep). This undermines the portability summary as written; clarify that the “no Azure data PaaS” claim is about primary runtime data, not backups.
In-cluster MinIO + CloudNativePG mean **no Azure data PaaS**. The residual Azure
surface is the `infra/` Bicep layer (the cluster plus four standalone durable
resources), one StorageClass string, a couple of LB annotations, and one
secret-store object. Everything under `k8s/` moves unchanged.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
hakan-persson
left a comment
hakan-persson
left a comment
There was a problem hiding this comment.
Verified the load-bearing claims rather than reading for plausibility. All hold.
No behaviour change — confirmed with the compiler, not by inspection. The
compiled ARM from main and from this branch are byte-identical (az bicep build
on both, diffed as normalised JSON, exit 0). The bicepparam and
loganalytics.bicep edits touch only comments and @description strings;
parameter values are untouched.
(My first attempt compared Python hash() of the two templates and showed a
difference — that was string-hash randomisation between processes, not a real
diff. Mentioning it because "the hashes differ" would have been a plausible-looking
false finding.)
The factual corrections check out:
infra/aks.bicep:31and the bicepparam both pin'1.36'— the minor alias — so
the README's1.36.2was contradicting the repo's own stated policy. The live
cluster happens to runv1.36.2, which is exactly why pinning the minor and
letting the patch channel own the patch level is right, and why the README should
not have hardcoded it.- Six
infra/*.bicepfiles exist; the layout block listed four. Agreed that
missing three of six is a wrong inventory rather than a partial aid.
The retention non-retroactivity point is the one I most wanted written down, and
the framing is right: every day it is deferred on a running cluster silently spends
a day of the history the archive exists to keep. That is a materially different
statement from "we should get around to it".
One overlap to resolve before merge
This and #13 independently fix the same stale sentence — Backups (Velero → MinIO …) are the next planned addition — so they conflict textually (verified with a
trial merge).
Your version is better and should win: "two schedules", base backups and WAL
archiving, plus the pointer to #not-yet-implemented. Mine only corrected the
tense.
They are otherwise complementary — #13 fixes (backs Loki, Thanos, and backups),
which this PR leaves untouched and which is the other half of the same error, since
Velero's BackupStorageLocation has provider: azure.
Suggested order: merge this first, then I rebase #13 onto it and drop my version of
the shared sentence.
Approving.
A consistency sweep, not new work. The last five merges (#4, #5, #7, #8, #9) added
Azure resources and controls; several summary claims never followed, so the overview
described a cluster that no longer exists. No behaviour change — bicep compiles
clean and every parameter value is byte-identical.
I checked the things that could have drifted and mostly hadn't: doc references and
§anchors all resolve, the docs index is complete, and the README's chart-versiontable matches all 12 pinned
targetRevisions. I also probed the Azure API versionsrather than assuming —
activityLogAlerts@2020-10-01anddiagnosticSettings@2021-05-01-previewreally are the newest bicep knows, so thescheduledQueryRulesmistake from #6 is not repeated elsewhere.Factually wrong
ScheduledBackup, whole strategy inmaintenance.mdmaintenance.mdpinsbackup-storage,loganalytics,alertsinstall.mdINFRA_RGOn the version one: omitting
backup-storage.bicepfrom the layout block wasdefensible as a partial orientation aid — missing three of six is a wrong inventory,
which is why it moved.
Two runbook blocks I had broken
§11's archive commands and§5c's action-group check were single 150-characterlines. I wrote them through a heredoc that ate the
\continuations, so they rancorrectly but were unreadable and broke the runbook's convention everywhere else.
Both restored. (
§5c's came in viac60ce0a— the collapse originated in my text,not the v2 PR.)
The 365-day archive, documented as a decision
Now that it is accepted rather than proposed, three things needed saying:
loganalytics.bicepread as a flatretentionInDays: 30, so the IaC — thesource of truth in this repo — understated the real configuration. It now says the
archive is set per-table in §11 and why it cannot live in the Bicep.
rather than tidying-up: raising retention later does not recover rows that have
already aged out, so every day it is deferred on a running cluster silently spends a
day of the history the archive was meant to keep. Durable across a rebuild, since
the workspace outlives the cluster.
secret in the cluster, granted at the same bar as a Key Vault role rather than a
monitoring dashboard. One inference of mine worth checking — a year of archived rows
widens that rather than narrowing it.
Deliberately left alone
aks.bicep:101andbackup-storage.bicep:92carry 4-line inline comments, overthe ~2-line convention. Both pre-existing, and both are must-read-before-you-edit
warnings — the immutable CIDRs especially. Churning them is the wrong trade.
install.md's other long command lines (§1 quota, §6 secrets, §7a deploy) aregenuinely single-line commands in the existing style, not collapsed continuations.
Four CI jobs pass; references and anchors resolve; placeholders still 9.