Skip to content

fix(review): #138 review findings, round 1 - #142

Merged
rubenvdlinde merged 11 commits into
feat/tenant-extra-hostnamesfrom
review/pr-138-fixes
Oct 14, 2026
Merged

fix(review): #138 review findings, round 1#142
rubenvdlinde merged 11 commits into
feat/tenant-extra-hostnamesfrom
review/pr-138-fixes

Conversation

@rjzondervan

@rjzondervan rjzondervan commented Oct 14, 2026

Copy link
Copy Markdown
Member

Fixes voor de review van #138 (#138 (review)). Merge in feat/tenant-extra-hostnames en vraag daarna de review opnieuw aan; de reviewer merget hier niets.

Bevindingen → commits

Bevinding Commit Wat er verandert
f2 🟡 Berekende primaire host niet herkend als dubbel 6b0bc18 primary_host() berekent de primaire host zoals de ApplicationSet doet als tenant.hostname leeg is; gelijk aan de gerenderde nextcloud.host voor alle 89 tenants.
f1 🔴 Extra host mag de host van een andere tenant claimen 34c94c0 validate_host_claims legt één register aan over alle tenantbestanden (primaire host, extraHostnames, frontend.host, frontend.extraHosts) en weigert een host die bij twee tenants hoort; ook bij een run op één bestand. Een extra host onder commonground.nu moet een conventienaam van de eigen tenant zijn.
f1 (pre-push) ab064c7 Een tweede bestand met een bestaande tenant.name is een fout (anders sloeg de check die tenant over). Het register wordt in run-tests.sh één keer gebouwd (--dump-host-claims): de suite van ~100 s terug naar ~16 s.
f1 (pre-push) fd6c9c2, 86e8c7f Fixtures die op tenant-dinkelland-prod leunen zeggen dat in hun kop; de checklijsten in CHECKS-AND-BALANCES.md, de changelog en de functiekop noemen alle nieuwe checks.
f5 🟢 Secretnamen botsen bij a-b.nl en a.b.nl 13687d4 Afgeleide TLS-secretnamen moeten uniek zijn (a-b.nl en a.b.nl).
f6 🟢 Hostnaam-regex accepteert localhost, 123 en labels >63 71e97bc Hostnaam vereist een punt, labels ≤ 63 en totaal ≤ 253; dezelfde helper nu ook voor tenant.hostname.
f6 (pre-push) 6da1eaa Een host waarvan het laatste label alleen cijfers is (een IP-adres als 81.24.6.82) wordt geweigerd.
f7 🟢 Extra Ingress negeert ingress.enabled en ingress.path 50934c5 De extra Ingress volgt ingress.enabled, path, pathType, annotaties en ingressClassName zoals templates/ingress.yaml van chart 8.9.0.
f7 (pre-push) bf29d92 extraHostnames samen met ingress.enabled: false wordt geweigerd (de HTTPRoute neemt nog één host); GATEWAY-API.md en ADDING-TENANT.md zeggen dat.
f8 🟢 Host verwijderen blijft in trusted_domains staan 72bb9e3 Docs: een host verwijderen = eerst config:system:get trusted_domains, dan config:system:delete trusted_domains <n>.

Aan de auteur (threads blijven open)

  • f3 🔴 Apex-host krijgt HSTS includeSubDomains: apex-host weigeren, opt-in eisen of hsts-include-subdomains uitzetten is een beleidskeuze die raakt aan de Zuiddrecht-use case (zuiddrecht.nl zelf). Wat de review aanraadt: geen kale apex zonder opt-in, en meten met curl -sI of de header zonder includeSubDomains komt.
  • f4 🟡 Geen eigen of betaald cert mogelijk per extra host: een betaald of eigen cert per extra host vraagt een schemakeuze (lijstitems als object met tls.issuer: none/secretName), of de docs zeggen dat alleen Let's Encrypt kan.

Buiten deze diff en niet aangepast: cutover-tenant.sh overschrijft trusted_domains index 1 zonder te kijken wat er staat, en kan zo een extra host wegschrijven.

Gedraaid op deze branch

  • nextcloud-platform/tests/run-tests.sh → 35 geslaagd, 0 gefaald (~16 s). De 11 nieuwe fixtures die moeten falen, slagen op de PR-head; de nieuwe checks vangen ze dus.
  • bash nextcloud-platform/scripts/validate-values.sh (alle 89 tenants) → exit 0, 0 errors, 5 warnings (dezelfde als op de head).
  • Render-vergelijking alle 89 tenants (inline values + helm template, chart 8.9.0) tegen efbbe861 → 178 bestanden, 0 verschillen.
  • Mutaties: elke nieuwe check apart weggehaald → zijn fixture wordt rood.
  • Pre-push adversarial check (aparte agent) → 3 rondes; de pre-push-commits hierboven zijn zijn bevindingen; laatste ronde alleen een docs-punt, opgelost in 86e8c7f.
  • niet gedraaid: scripts/verify.sh stopt lokaal op ontbrekende kubeconform; CI draait hem. Niets tegen een cluster.

🤖 Generated with Claude Code

rjzondervan and others added 11 commits October 14, 2026 15:28
@rjzondervan @claude
fix(review): #138 f2 — Berekende primaire host niet herkend als dubbel
6b0bc18 
#138 (comment)

validate_extra_hostnames vergeleek alleen met een expliciete tenant.hostname.
Nieuwe helper primary_host() berekent de primaire host zoals de ApplicationSet
(tenant.hostname, anders <org>.commonground.nu / <org>.<omgeving>.commonground.nu,
inclusief het ontbreken van 'demo' als suffix in het template). Gecontroleerd
tegen de gerenderde nextcloud.host van alle 89 tenants: gelijk.

Fixture tenant-extra-hostnames-berekende-primaire-host (zonder hostname) faalt
nu met 'bevat de primaire host'; tegen de PR-head-validator slaagde hij.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rjzondervan @claude
fix(review): #138 f1 — Extra host mag de host van een andere tenant c…
34c94c0 
…laimen

#138 (comment)

validate-values.sh bouwt nu per run een host-register over alle
tenantbestanden in values/tenants/ (primaire host zoals de ApplicationSet hem
bepaalt, tenant.extraHostnames, tenant.frontend.host, tenant.frontend.extraHosts)
en weigert elke host die twee tenants claimen (validate_host_claims). Een bestand
op de commandoregel vervangt het tenantbestand met dezelfde tenant.name, dus ook
`validate-values.sh <één bestand>` vergelijkt met de hele vloot en een
gewijzigde kopie botst niet met zichzelf. De check werkt in beide richtingen:
een nieuwe tenant B faalt ook als een bestaande A zijn host al als extra host
heeft (getest met een tijdelijk tenantbestand).

Een extra host onder commonground.nu moet volgens de naamconventie van
validate_hostname_convention bij de eigen tenant horen (convention_hosts:
<org>.commonground.nu / <org>.migrate.commonground.nu voor prod,
<org>.<suffix>.commonground.nu anders). Geen nieuwe opt-in.

Class sweep: binnen één tenant mag een extra host ook niet de host van de
eigen WOO-frontend zijn (zelfde botsing, andere Ingress). Checklijsten
bijgewerkt in de commentaarkop van validate_extra_hostnames,
docs/CHECKS-AND-BALANCES.md en changelog.d/tenant-extra-hostnames.md;
run-tests.sh noemt dat de botsingsfixtures op tenant-dinkelland-prod leunen
(de runner kent alleen enkelvoudige cases; de validator leest values/tenants/
zelf, dus de runner hoefde niet te veranderen).

Fixtures: andere-tenant (primaire host van dinkelland-prod), andere-frontend
(frontend.host van dinkelland-prod), commonground-vreemd, commonground-eigen
(PASS), eigen-frontend. De vier foute cases slagen tegen de PR-head-validator.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rjzondervan @claude
fix(review): #138 f5 — Secretnamen botsen bij a-b.nl en a.b.nl
13687d4 
#138 (comment)

validate_extra_hostnames weigert nu een afgeleide TLS-secretnaam die al bij
een andere extra host hoort (punten worden streepjes, dus a-b.nl en a.b.nl
gaven hetzelfde secret). Een dubbele host meldt alleen 'dubbel' en slaat de
overige checks over, zodat hij niet ook als secretbotsing telt.

Fixture tenant-extra-hostnames-zelfde-secret; slaagde tegen de
PR-head-validator. Class sweep: de checklijst in docs/CHECKS-AND-BALANCES.md,
changelog.d/tenant-extra-hostnames.md en de commentaarkop noemen nu een unieke
secretnaam. De secretnaam van de hoofd-Ingress (nextcloud-<tenant>-tls) kan
niet met een afgeleide naam botsen (die heeft altijd een host-deel).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rjzondervan @claude
fix(review): #138 f6 — Hostnaam-regex accepteert localhost, 123 en la…
71e97bc 
…bels >63

#138 (comment)

Nieuwe helper is_valid_hostname: RFC 1123-labels zoals voorheen, plus
minstens één punt, elk label max 63 en het geheel max 253 tekens.
validate_extra_hostnames gebruikt hem; de melding noemt de regels.

Class sweep: validate_hostname (tenant.hostname) gebruikte dezelfde regex en
had hetzelfde gat; ook die gebruikt nu is_valid_hostname. De 89 tenants blijven
groen.

Fixtures: tenant-extra-hostnames-localhost, -getal (123), -label-te-lang
(label van 64 tekens) en tenant-hostname-zonder-punt; alle vier slaagden tegen
de PR-head-validator. Een host > 253 werd voor extraHostnames al via de
secretnaamlengte gevangen, dus daar geen aparte fixture.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rjzondervan @claude
fix(review): #138 f7 — Extra Ingress negeert ingress.enabled en ingre…
50934c5 
…ss.path

#138 (comment)

De extra Ingress nextcloud-extra-hosts volgt nu templates/ingress.yaml van
chart 8.9.0: hij staat achter `if .Values.ingress.enabled` en neemt
ingress.path en ingress.pathType via de tpl over in plaats van vast `/` en
`Prefix`.

Class sweep (zelfde afwijking van de chart-template): annotations en
ingressClassName staan nu ook achter `with`, zoals in de chart, zodat een lege
waarde geen `annotations: {}` of lege `ingressClassName:` oplevert. Docs-tabel
in docs/ADDING-TENANT.md en changelog.d/tenant-extra-hostnames.md bijgewerkt.

Gerenderd met helm (dinkelland-prod + extraHostnames, chart 8.9.0): met de
standaardwaarden byte-identiek aan de PR-head; ingress.enabled=false geeft
geen enkele Ingress meer (PR-head rendert dan nog nextcloud-extra-hosts);
ingress.path=/x en pathType=ImplementationSpecific komen in beide Ingresses.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rjzondervan @claude
docs(review): #138 f8 — Host verwijderen blijft in trusted_domains staan
72bb9e3 
#138 (comment)

docs/ADDING-TENANT.md: een host uit extraHostnames halen prunet de Ingress,
maar de host blijft in config.php tot `occ config:system:delete
trusted_domains <n>`. Eerst `occ config:system:get trusted_domains` draaien om
de juiste <n> te vinden, want `set` op een bezette index overschrijft die entry.

Class sweep: dezelfde beperking staat in changelog.d/tenant-extra-hostnames.md
en noemt nu ook het verwijderen.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rjzondervan @claude
fix(review): #138 f7 — pre-push: extraHostnames vereist ingress.enabled
bf29d92 
#138 (comment)

Sinds f7 volgt nextcloud-extra-hosts ingress.enabled, net als de chart. Met
ingress.enabled: false (de Gateway-cutover) hadden de extra hosts dan geen
enkele route, want charts/tenant-httproute neemt alleen de primaire host.

validate_extra_hostnames weigert nu extraHostnames samen met een effectieve
ingress.enabled die niet true is. effective_ingress_enabled leest de waarde in
dezelfde valueFiles-volgorde als de ApplicationSet (common, env, db,
canary-overrides als tenant.canary, tenantbestand; de inline values zetten hem
niet). Alle 89 tenants: true, gelijk aan de renders (89 met Ingress).

Fixture tenant-extra-hostnames-zonder-ingress; met de check weggehaald slaagt
hij (mutatie gecontroleerd). docs/GATEWAY-API.md (extraHostnames-alinea en de
cutover-alinea) en docs/ADDING-TENANT.md zeggen het nu.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rjzondervan @claude
fix(review): #138 f1 — pre-push: dubbele tenant.name en register één …
ab064c7 
…keer bouwen

#138 (comment)

Dubbele tenant.name: build_host_claims hield één bestand per naam aan, dus een
tweede tenantbestand met dezelfde naam viel stil uit de botsingscheck.
dump_host_claims (het register van values/tenants/) geeft nu een error voor
elk tweede bestand daar met een bestaande naam. Alleen een bestand BUITEN
values/tenants/ (fixture, gewijzigde kopie) vervangt de tenant met dezelfde
naam; een bestand binnen values/tenants/ wordt nooit als vervanger gebruikt,
dus de omweg bestaat niet meer. Twee bestanden buiten de map met dezelfde naam
zijn ook een error. Getest met een tijdelijke kopie in values/tenants/ (error,
daarna verwijderd) en met fixture tenant-dubbele-naam, die een eigen vloot
heeft in tenant-dubbele-naam.fleet/ (run-tests.sh zet daarvoor
VALIDATE_TENANT_DIR). Met de dup-check weggehaald wordt die fixture rood.

Looptijd: run-tests.sh bouwde per case het register over 89 bestanden
opnieuw (~2,8 s per case). De validator accepteert nu VALIDATE_HOST_CLAIMS_FILE
(geschreven met `validate-values.sh --dump-host-claims`); run-tests.sh bouwt
het één keer. Ontbreekt het bestand, dan bouwt de validator het register zelf.
Eén fixture: 2,8 s -> 0,3 s; hele suite (34 cases): ~14 s.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rjzondervan @claude
fix(review): #138 f6 — pre-push: IP-adres als hostnaam weigeren
6da1eaa 
#138 (comment)

is_valid_hostname accepteerde 81.24.6.82; Kubernetes neemt geen IP-adres als
Ingress-host. Een naam waarvan het laatste label alleen cijfers is, wordt nu
geweigerd. Geldt via de helper voor zowel tenant.extraHostnames als
tenant.hostname; de meldingen noemen het.

Fixture tenant-extra-hostnames-ip-adres; tegen de validator van de vorige
commit slaagt hij (mutatie gecontroleerd). De 89 tenants blijven groen.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rjzondervan @claude
test(review): #138 f1 — pre-push: afhankelijkheid van dinkelland in f…
fd6c9c2 
…ixtures noemen

#138 (comment)

De fixtures tenant-extra-hostnames-andere-tenant en -andere-frontend toetsen
een botsing met tenant-dinkelland-prod. Een regel bovenin zegt nu dat ze
breken als de hosts van dinkelland veranderen.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rjzondervan @claude
docs(review): #138 f1 — pre-push: check lists bijgewerkt
86e8c7f 
#138 (comment)

De checklijsten noemen nu ook de checks uit de pre-push-ronde: extraHostnames
niet samen met ingress.enabled: false, geen IP-adres als host (ook voor
tenant.hostname), en geen dubbele tenant.name over tenantbestanden heen.
Bijgewerkt in docs/CHECKS-AND-BALANCES.md, changelog.d/tenant-extra-hostnames.md
en de commentaarkop van validate_extra_hostnames (git grep: geen andere lijsten).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rjzondervan rjzondervan added the change/platform Platform- of gemengde wijziging: raakt shared values, Argo-wiring, scripts of policy label Oct 14, 2026
@rjzondervan rjzondervan mentioned this pull request Oct 14, 2026
Open
@rubenvdlinde
rubenvdlinde merged commit a17181e into feat/tenant-extra-hostnames Oct 14, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

change/platform Platform- of gemengde wijziging: raakt shared values, Argo-wiring, scripts of policy

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rjzondervan @rubenvdlinde