feat(site-router): add routed site-to-site IPsec gateway app (Phase 1) - #3426

Actionable comments posted: 13

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cmd/site-router-controller/main.go`:
- Around line 66-70: Update the shared ValidateManagementCIDR validator to
reject parsed networks whose IP does not have To4() != nil, while preserving
valid IPv4 CIDR handling and existing validation errors. Ensure the
managementCIDR flag path uses this validator so IPv6 values cannot start the
controller.

In `@hack/e2e-chainsaw/site-router/chainsaw-test.yaml`:
- Around line 425-494: Validate that GW_IP, BACKEND_IP, NODE_IP, API_CLUSTERIP,
and OTHER_POD_IP are all non-empty immediately after resolution and before any
assert_dropped or probe_from_b_stdout calls. Fail the negative-security step
with a clear error identifying every missing target, while preserving the
existing probe and attribution checks when all targets resolve.

In `@hack/select-e2e.sh`:
- Around line 37-40: Update the TODO comment in the site-router enablement
instructions to reference the active VyOS entry in
packages/system/vm-default-images/values.yaml and instruct CI to stamp
packages/system/vm-default-images/images/vyos-router-disk.tag, removing the
incorrect packages/system/vm-images and uncomment guidance.

In `@internal/vyos/render/render.go`:
- Around line 824-875: Update renderIPSec and the peer-name generation around
sanitisePeerName so names are tracked for the current tunnel batch and duplicate
sanitized descriptions receive a deterministic unique suffix, such as their
tunnel index. Use the disambiguated name consistently for both the site-to-site
peer and authentication PSK paths, while preserving existing names for
non-colliding descriptions and fallback behavior for empty descriptions.

In `@packages/apps/site-router/docs/PR-BODY.md`:
- Around line 45-47: Update the follow-up list in
packages/apps/site-router/docs/PR-BODY.md around lines 45-47 by removing the
landed reproducible VyOS build item and retaining only still-pending publishing,
digest stamping, or empirical validation work. Update
packages/apps/site-router/docs/image-lifecycle.md around line 3 to describe the
VyOS image entry as enabled and digest-pinned, replacing the outdated
“commented-out” wording.

In `@packages/apps/site-router/docs/security-model.md`:
- Around line 33-40: Update the Boundary A description to state that the
IPsec-decrypted local-input drop is emitted only when managementCIDR enforcement
is active, and explicitly qualify the open-management test path where the rule
is absent. Keep the existing explanation of tunnel coverage and the separate
Boundary B behavior unchanged.

In `@packages/apps/site-router/templates/_helpers.tpl`:
- Around line 109-120: Update the managementCIDR validation in
site-router.assertSafeVyOSInputs to perform semantic IPv4 CIDR and prefix-range
validation, not only regex shape checking. Reuse an available real IPv4-prefix
parser during template admission/rendering, and fail with the existing refusal
behavior when parsing fails before managementCIDR reaches the VyOS
configuration.

In `@packages/apps/site-router/tests/secret_cloudinit_test.yaml`:
- Around line 208-263: Update the Boundary A security-model documentation to
state that the guest guard seeds are applied only when managementCIDR
enforcement is enabled, not regardless of managementCIDR. Align the description
with the T08 tests and preserve the documented exception that
allowOpenManagement=true seeds no guest firewall guards.

In `@packages/apps/site-router/values.yaml`:
- Around line 98-100: Update the managementCIDR validation in values.yaml and
its admission/render validation to reject out-of-range IPv4 octets and prefixes
outside /0–/32, not merely unsafe characters. Preserve support for an empty
value when allowOpenManagement=true and the documented default CIDR, then add
regression cases covering malformed octets and prefix lengths.
- Around line 74-81: Enforce a minimum of one CPU core for resources.cpu in the
values schema/API definition, rejecting zero and negative values while retaining
whole-core validation. Regenerate the derived schema and API types, then extend
the validation tests to cover both zero and negative CPU values. Anchor the
changes to the resources.cpu definition and its existing validation tests.

In `@packages/system/site-router-controller/templates/rbac.yaml`:
- Around line 27-39: Restrict the RBAC permissions represented by the
ClusterRole rather than granting cluster-wide Secret and ConfigMap list/watch
access. Replace broad resources under the unnamed core API rules with
namespace-scoped bindings or narrowly targeted named-object access for the
specific per-instance Secrets and the cozy-system/cozystack ConfigMap required
by the controller, while preserving only the necessary read operations.
- Around line 16-24: Constrain the RBAC rules in rbac.yaml so the controller
does not receive cluster-wide patch access to arbitrary Pods or Namespaces.
Replace the broad patch permissions for the resources listed in the rules with
the narrowest supported authorization scope, and preserve only the read verbs
needed for discovery; if Kubernetes RBAC cannot express the required
single-resource or label restriction, remove patch from these cluster-wide rules
and handle the targeted updates through an appropriately scoped mechanism.

In `@packages/system/site-router-controller/values.yaml`:
- Around line 7-16: The values configuration currently hard-codes the kube-ovn
default in managementCidr. Update managementCidr to derive from the cluster’s
networking.podCIDR, or make it unset and require an explicit override for
non-default Pod CIDRs, while preserving the required match with the controller
--management-cidr and app chart managementCIDR values.

---

Nitpick comments:
In `@hack/e2e-chainsaw/site-router/chainsaw-test.yaml`:
- Around line 557-566: Update the JSONPath predicate in the gateway pod query to
access the ovn.kubernetes.io/port_security annotation using bracket notation
instead of escaped dotted-key notation. Keep the existing filtering for
annotations equal to "false" and the surrounding validation unchanged.

In `@internal/controller/siterouter/status.go`:
- Around line 65-68: Reduce repeated API-server load from the pod List in
surfacePendingRoutePods by gating or backing off the namespace relist instead of
executing it on every runtimePollInterval reconcile. Prefer relisting only when
the programmed route set changes, or otherwise enforce a slower bounded
surfacing cadence, while preserving pending-route detection and existing error
handling.

In `@internal/vyos/client.go`:
- Around line 97-113: Update WithInsecureSkipVerify to derive the transport from
the client’s current c.http.Transport, preserving custom transport settings
supplied through WithHTTPClient and avoiding mutation of the caller’s client
transport. Apply TLS configuration with InsecureSkipVerify and MinVersion set to
tls.VersionTLS12 in both the cloned and fallback transport paths.

In `@packages/system/cozystack-api/templates/rbac.yaml`:
- Around line 9-17: Restrict the RBAC permission for the ConfigMap named
cozystack to the cozy-system namespace instead of granting it cluster-wide
through the ClusterRole. Update the surrounding rbac.yaml binding or role
structure while preserving the existing get-only, name-scoped access required by
SiteRouter admission.

In `@packages/system/vyos-router-image/images/vyos-router-disk/Dockerfile`:
- Around line 10-11: Replace the ADD instruction in the Dockerfile with COPY for
the local vyos-router-amd64.qcow2 file, preserving the existing source and
destination paths.

Verdict: NOT LGTM

Large, well-executed Phase-1 feature, but it is enabled in the default presets (isp-full / isp-full-generic) and registered in the NaaS catalog, while its only supported load path cannot succeed on any cluster today (the golden VyOS image is not published and the digest is a zero placeholder). The failure surfaces as a silently stuck DataVolume with no legible status. The key negative-security acceptance suite that would prove the isolation model is deferred and never runs in CI.

What the PR does

Adds a catalog app site-router: routed (L3) site-to-site connectivity between tenants over an IKEv2 IPsec tunnel terminated in a VyOS gateway (KubeVirt VM). The chart materializes the VM + boot disk + tunnel Service type: LoadBalancer + Secrets (PSK, api-key) + Cilium policies. A new site-router-controller does what the chart cannot express: deny-set validation of remote networks, kube-ovn return-route annotation, relaxation of port_security on the gateway port, live config push to VyOS over its management API, tunnel/BGP observability and status. Scope: +14239/-17, 100 files.

Blocking findings

[MAJOR] Feature is enabled by default without a working default load path
packages/core/platform/values-isp-full.yaml:11, values-isp-full-generic.yaml:11, templates/bundles/naas.yaml:17-20, packages/apps/site-router/templates/dv.yaml:21-23. On a standard install the AppDefinition is registered in the NaaS catalog and the controller starts, but the default instance (image.enabled=false) clones a non-existent PVC cozy-public/vm-default-images-vyos-router (the golden image is an unpublished follow-up). A catalog tenant gets a DataVolume stuck in Pending, a VM that never boots, and no Ready=False / Event. Suggested fix: do not include site-router in the presets and do not register it in the catalog until the golden image is published and vyos-router-disk.tag is stamped with a real digest; also emit a legible status on the missing-source path.

[MAJOR] Non-resolvable image reference (zero-placeholder digest)
packages/system/vm-default-images/images/vyos-router-disk.tag:1 = ...@sha256:0000...0000. dv.yaml is an unconditional {{- range .Values.images }}, so the added vyos-router entry renders a DataVolume importing docker://…@sha256:0000…, which never resolves. The package is opt-in, but on every cluster that enables it an upgrade adds a permanently failing CDI importer and a stuck 12Gi PVC. Suggested fix: skip entries whose digest is the placeholder until a real one is stamped.

Minor findings

[MINOR] internal/controller/siterouter/reconciler.go:398-421,662-669 — a deny-set failure on the reconcile path is invisible, and the docstring over-promises. validateRemoteCIDRs returns a hard error and classify does return ctrl.Result{}, err (669) before updateStatus (332): no Event, no condition, although the docstring (400-402) claims "status surfaces it machine-readably". On a CIDR reconfigure (or if admission is bypassed) the HR stays Ready=True while routes are silently not programmed. Minor because the primary tenant path is synchronous fail-closed admission. Fix: emit a Warning Event (as vyospush.go:329 does) and correct the docstring.

[MINOR] packages/apps/site-router/values.schema.json (bgp.required=["enabled"]) + internal/controller/siterouter/vyospush_test.go:786 — bgp.enabled=true without localASN is a silent no-op. The schema only requires enabled, though the description says localASN is required. The controller silently skips BGP with no Event/condition. Fix: make localASN required-when-enabled, or surface Ready=False / Event.

Discrepancies with the PR claims

• [PARTIAL] PR body item 8 "Negative-security acceptance suite passes": the suite is committed, but hack/select-e2e.sh:42 sets DEFERRED_SUITES="site-router" and strip_deferred removes it from all CI paths, so it never runs. The security guards (Cilium egressDeny, forward default-deny, source filter, two-boundary API isolation) have zero executed evidence of a packet drop, only render-shape unit tests.
• [OK, verified] "api-key not tenant-readable": confirmed. The api-key Secret is absent from both the AppDefinition secrets.include and the dashboard-resourcemap.yaml Role; only the PSK (tenant's own key) and the tunnel Service are exposed to use.

Main operational risk

The isolation security model is not proven on the data plane. templates/vm.yaml:28 permanently bakes ovn.kubernetes.io/port_security: "false" onto the gateway pod (the controller only verifies, it does not re-enforce: reconciler.go:502), with an acknowledged boot window where the port is relaxed before the VyOS firewall is stamped. The only compensating control is the in-guest VyOS firewall, whose drop behavior is exactly what the deferred negative-security e2e would prove, and that suite does not run in CI. Cross-tenant reach is bounded by Cilium identity (hence not Critical), but the headline isolation claim rests on render-shape unit tests rather than executed evidence.

Verified, non-blocking notes

• charts-direct-edit on packages/apps/site-router/charts/cozy-lib is a false positive: it is a symlink (git mode 120000), same as apps/vpn / apps/kafka. Not a vendored edit.
• The 3 chart_lint.render_errors are harness artifacts (site-router renders cleanly under tenant-*; platform OCIRepository is a lookup dependency; the cozystack-api _cluster nil is a runtime cozystack-values injection).
• The deny-set (denyset.go:84-89) intentionally leaves NodeCIDRs / LBPools empty, so overlap with the node subnet / LB pool is not rejected, but the effect is self-inflicted (tenant's own routing), not cross-tenant.
• VyOS management uses InsecureSkipVerify (internal/vyos/client.go:97), bounded by --management-cidr + a per-instance token.
• The upgrade adds a cluster-wide read-only grant (secrets / configmaps / services) to the controller SA, judged acceptable least-privilege (read-only, no exec / secret writes).
• Watch / pod-discovery wiring is correct (the lineage webhook stamps apps.cozystack.io/application.* on the virt-launcher pod), admission is fail-closed on Create+Update, leader election is on (--leader-elect, replicas:1), securityContext is hardened, cozyrds resourceNames match the Role, no reconcile amplifier, go build/vet/test green, deny-set / admission tests are non-vacuous (checked by mutation).

Recommended follow-up

Run an end-to-end test on a disposable dev cluster: (1) whether the zero-digest DataVolume wedges the vm-default-images HelmRelease on an opt-in upgrade or just retries in the background; (2) whether the default site-router instance without the golden image gives any legible status; (3) wire up and run the deferred negative-security Chainsaw suite to get real evidence of a packet drop.

Verdict

NOT LGTM. The engineering quality is high and most Phase-1 residuals are documented honestly, but on several tenant-reachable paths the security model's stated isolation properties do not hold as written, and the guards' runtime efficacy is deferred from CI. Reviewed at c2abcf19 against merge-base 2f8c953c.

Findings

[MAJOR] internal/siterouter/denyset/denyset.go:90-106, internal/controller/siterouter/reconciler.go:407-431, internal/vyos/render/render.go:649-709 — node/host network is not in the deny-set → node source-impersonation into tenant pods.

ClusterNetworksFromConfigMap only ever populates pod/service/join (plus the always-reserved link-local/loopback/default-route); NodeCIDRs/LBPools stay empty. A tenant may therefore legally declare the node subnet as a remoteCIDR — admission and reconcile both pass — and the controller then emits a TUNNEL-INGRESS accept source ∈ <node-subnet> AND dest ∈ pod/svc. The authenticated tunnel peer (the tenant's remote site) can then send decrypted packets sourced as a cluster node IP to that tenant's workloads, defeating any node-IP-based trust those workloads apply. Separately this is a doc/comment contradiction: reconciler.go:407-408 and the security-model "Deny-set validation" section state node/LB-pool are rejected, while residual #6 admits they are not. Fix: reject any remoteCIDR overlapping the node/host network, and correct the overclaiming comment + doc.

[MAJOR] internal/controller/siterouter/vyospush.go:648-661 → internal/vyos/render/render.go:686-695 — the guest source filter is not a tenant-isolation control, contrary to the doc.

tenantNetworkCIDRs returns the whole cluster pod+service CIDR, so each TUNNEL-INGRESS accept is destination-constrained to all tenants, not this instance's networks. A decrypted packet with a valid remote source and a destination that is another tenant's pod IP is accepted and forwarded by the gateway. docs/security-model.md presents this filter as enforcing "must never let tunnel traffic reach ... unrelated tenants" — it does not; cross-tenant containment rests entirely on Cilium identity at the destination pod (which a static review cannot verify). Either stop claiming the guest filter provides tenant isolation, or scope the destination set per-tenant.

[MAJOR] values.schema.json (image), templates/dv.yaml:35-37, templates/vm.yaml:28, docs/security-model.md — tenant-settable boot image nullifies every guest-side guard while port_security is off from boot.

image.enabled/url is exposed in the cozyrd openAPISchema to tenant use (dashboard-resourcemap binds use) with no operator gating, letting a tenant boot an arbitrary image as the gateway VM, which carries ovn.kubernetes.io/port_security: "false" from pod creation. Every guest-side guard (source filter, forward default-deny, Boundary-A management drop, management firewall) lives inside the image the tenant just replaced. The threat model assumes the shipped VyOS guest enforces them and does not cover the custom-image case. The remaining controls are the two namespaced Cilium policies (egressDeny covers only 169.254.0.0/16 by default; gateway-ingress is additively re-opened per the documented Boundary-B residual) plus the unverified baseline Cilium identity. Net: a tenant obtains an anti-spoof-disabled pod running arbitrary code on the tenant OVN subnet, egress-denied only to link-local. Fix: gate image.* behind an operator/admin tier (not tenant use), or document the trust boundary and default security.egressDenyCIDRs to include node/management ranges.

[MINOR] internal/controller/siterouter/cnimediation.go:143-160 — removeRoutes deletes a co-tenant's live route on delete. When the deleting instance's gateway pod is already gone (gatewayIP==""), ownership falls back to dst-matching and drops every entry whose dst ∈ this instance's remoteCIDRs, including a co-tenant site-router's entry for the same dst (gw = the co-tenant's IP). Transient blackhole of the co-tenant's return path until its next 30s reconcile — the exact co-tenant clobber the gateway-IP-known branch was written to avoid.

[MINOR] internal/controller/siterouter/cnimediation.go:85-118 — route flap when two same-namespace SiteRouters declare the same remoteCIDR. Each reconcile upserts the shared dst's gw to its own gateway IP, so the two instances flip the route back and forth every 30s, churning the namespace annotation and disrupting that dst's return path. No conflict detection.

[MINOR] internal/controller/siterouter/reconciler.go:312-317 — deny-set hard error short-circuits before route withdrawal. A remoteCIDR valid when routes were programmed but later overlapping a cluster network (admin re-scopes pod/svc CIDR) makes validateRemoteCIDRs return before programNamespaceRoutes/removeNamespaceRoutes, so stale kube-ovn return routes persist and blackhole the now-cluster-owned range. Silent (Event only; HR stays Ready).

[MINOR] mediation health has no durable status signal. The SiteRouter app-instance has no status subresource (D9); every mediation failure (InvalidRemoteCIDR, ConfigureFailed, SourceFilterPending, PortSecurityRelaxationPending, PSK/APIKey/TunnelAddress pending) surfaces only as an ephemeral Warning Event on the HelmRelease, which stays Ready=True (chart applied) while the tunnel is down. Events age out (~1h); reconciler.go:420 acknowledges this. Recommend a durable condition or a documented "check Events + WorkloadMonitor" runbook.

[MINOR] values.schema.json — input-validation asymmetry. peer.address, staticRoutes.destination/nextHop, bgp.neighbors.address are unconstrained free strings and are not deny-set-checked, while managementCIDR has a strict IPv4 pattern and remoteCIDRs is deny-set-validated. Not an injection hole (the API push uses structured JSON Operation values; the cloud-init path is guarded by assertSafeVyOSInputs), but a hostile value fails late as a ConfigureFailed→Degraded requeue loop rather than a clean admission rejection, and a staticRoute toward a cluster CIDR is pushed to the gateway. Add CIDR/IP/hostname patterns for admission-time rejection + defense-in-depth.

[MINOR] cmd/site-router-controller/main.go:67 — --management-cidr help says it governs "HTTPS 443 and SSH (22)", but the render (render.go:517-521) and cloud-init never open 22 (the appliance has no SSH). Remove the SSH mention so operators don't assume 22 is reachable/controlled by this flag.

Claim mismatches

• [PARTIAL] security-model.md "Deny-set validation ... rejects ... node / LB-pool networks" — node/LB-pool are not enforced (Finding 1).
• [PARTIAL] security-model.md threat model "must never let tunnel traffic reach ... unrelated tenants" as a property of the guest source filter — the destination set is cluster-wide pod+svc; cross-tenant containment is Cilium-identity-only (Finding 2).
• [PARTIAL] security-model.md guest-guard guarantees are silently void when image.enabled=true (Finding 3).

Caveats

• Upgrade path: N/A — all-new packages (apps/site-router, system/site-router-controller, system/site-router-rd, system/vyos-router-image), new SiteRouter kind, new cozy-site-router namespace. No pre-existing customer objects; no upgrade-regression surface (verified: no removed/renamed in-tree identifiers).
• Fresh-install: sound — PackageSource dependsOn (networking/kubevirt/cdi/engine/controller), naas gated on bundles.iaas.enabled (avoids the isp-hosted #3376 deadlock), controller dependsOn victoria-metrics-operator for its unconditional VMPodScrape, cozystack-api ConfigMap grant name-scoped. go build + all Go unit tests + all 63 helm-unittests green; make generate not stale.
• Render-blindness / live-only claims: the kube-ovn v1.15.10 "port_security reconciled only at pod create" behavior and the VyOS 1.5 firewall leaf syntax are author-claimed "validated live" — not executed by this static review (needs a live cluster / booted gateway).
• Cross-tenant/node containment (Findings 1-3) cannot be verified statically (live Cilium/kube-ovn required).
• Negative-security e2e (the Phase-1 acceptance gate proving the guards actually drop spoofed / world-destined packets) is committed but deferred from CI (hack/select-e2e.sh DEFERRED_SUITES="site-router"), so the guards' runtime efficacy is unproven in CI.
• Management-API isolation: managementCIDR defaults to the whole pod CIDR (every pod), and Boundary B is additively re-opened, so the api-key Secret's namespace RBAC isolation is effectively the sole control on the API. That isolation holds cross-tenant (api-key excluded from the dashboard Role + AppDef secrets.include — verified), but a tenant admin can read its own gateway's token and reconfigure the router (within the tenant's own blast radius).

Recommended follow-ups

• cozystack-pr-test negative-isolation run: (a) a spoofing custom-image gateway, (b) a tunnel-peer packet sourced as a node IP and destined to another tenant's pod IP — verify no cross-tenant/node reach; plus run the deferred negative-security chainsaw suite once the golden image ships.

Checked and sound (recorded so they need not be re-litigated): deny-set admission wired into both Create and Update; controller RBAC is strictly least-privilege (get-only on secrets/services/configmaps); secrets are redacted before truncation (correct order — a secret straddling the 256-byte cut cannot leak); deny-set masks host bits, rejects /0 and non-IPv4 (incl. IPv4-mapped-IPv6), fails closed on a ConfigMap read error, and matches admission↔reconcile via the shared helper; the cozyrd hides the api-key and exposes PSK + tunnel Service with the correct release.prefix; push failure is fail-closed (hash not recorded, Degraded + requeue, no more-open end state); reconcile is single-writer under leader election; the cloud-init config.boot path is guarded by assertSafeVyOSInputs.

Verdict: request changes

One hard merge blocker (the PR's own new CI gate fails on the committed tree), and one cross-tenant reachability question on the tunnel-ingress filter that the stated Cilium mitigation does not actually close. The security spine (deny-set, admission fail-closed, api-key isolation, RBAC) is otherwise solid and was verified against the code.

Blockers

1. Placeholder image digest breaks this PR's own CI gate and makes every gateway non-bootable

packages/apps/site-router/images/vyos-router-disk.tag carries an all-zeros digest (...@sha256:0000…0000).

• This PR adds hack/image-refs-no-placeholder.bats, which make unit-tests runs in CI. Reproduced: grep -rIlE '@sha256:0{64}' packages/ --exclude-dir=charts returns exactly this file, so the gate exit 1s. The PR ships a guard its own tree violates — the unit-tests job is red and it cannot merge as-is.
• Functionally: the VyOS golden image is not published, so a tenant's SiteRouter boot DataVolume resolves nowhere → boot PVC stays Pending forever and the VM never boots. dv.yaml's required guard only rejects an empty .tag; a well-formed placeholder digest passes render. Since the app is wired into the naas bundle, an install/upgrade exposes the SiteRouter kind in the tenant catalog while no gateway can boot.

Fix: publish the cozystack-owned VyOS image and stamp the real digest before merge; do not activate the catalog entry until the image exists.

Security (needs resolution before merge)

2. Decrypted tunnel ingress can reach any tenant's pods, and the documented Cilium backstop does not enforce cross-tenant isolation for it

• internal/controller/siterouter/vyospush.go:665 (tenantNetworkCIDRs) builds the decrypted-traffic destination allow-list as PodCIDR (the whole cluster pod CIDR, e.g. 10.244.0.0/16) plus this-namespace Service ClusterIPs. Services are tenant-scoped; pods are not.
• internal/vyos/render/render.go:686 (renderTunnelIngressFilter) turns that into accepts of source ∈ remoteCIDR AND destination ∈ podCIDR. The IPsec local selector is 0.0.0.0/0 and no NAT is rendered, so the decrypted packet keeps the remote peer's real source IP.
• docs/security-model.md:27 states this pod-CIDR breadth is safe because "a destination inside the cluster pod CIDR proceeds to Cilium, whose identity policy enforces cross-tenant isolation." The tenant baseline allow-external-communication (packages/apps/tenant/templates/networkpolicy.yaml) unconditionally admits fromEntities: [world, cluster]. A decrypted packet with an external source IP is classified as identity world, so a victim pod in another tenant governed by that baseline will accept it. The stated mitigation therefore does not hold for this traffic.

Failure scenario: tenant A sets remoteCIDRs=[203.0.113.0/24]; a host on that subnet sends a packet to tenant B's pod IP; it is decrypted, matches source∈203.0.113.0/24 AND dest∈podCIDR, is forwarded, and tenant B's pod accepts it as world ingress.

What is proven from code: the filter's destination breadth (whole pod CIDR), source-IP preservation, and that the baseline admits world. What is not empirically confirmed (needs a live cluster, blocked on the golden image): the actual cross-tenant delivery through kube-ovn. Please either scope the destination to the owning tenant's own pods, or empirically prove the delivery is denied and correct the security-model wording.

Non-blocking findings

3. BGP is a half-wired feature

The public schema api/apps/v1alpha1/siterouter/types.go (BGP) exposes only enabled / localASN / neighbors. resolveInputs (vyospush.go:591) constructs render.BGPPeer{PeerAddress, PeerAsn} only. The render fields AdvertisedNetworks, RouterID, Timers, and BGPPasswords (render.go:140) are unreachable from the controller — dead code. A tenant setting bgp.enabled=true gets a session that originates no routes; the instance stays Ready with no warning event (unlike the missing-ASN path, which does warn). Either document this as receive-only for Phase 1, or wire advertisedNetworks into the schema.

4. No runtime detection of a managementCIDR lockout

The chart and controller defaults are both the whole pod CIDR 10.244.0.0/16. ValidateManagementCIDR only checks the flag is a valid CIDR, not that it covers the controller's source. On a cluster with a non-default pod CIDR, or if the chart managementCIDR and the controller --management-cidr drift, the controller silently firewalls itself out of every gateway, surfacing only as an opaque ConfigureFailed. Consider deriving the CIDR from the same ConfigMap the deny-set already reads, or emitting a distinct diagnostic when pushes time out.

5. Checklist item 9 ("unit green in CI") is partial

helm-unittest (60 tests) and the Go unit tests pass locally, but make unit-tests (the CI job) also runs the new placeholder-digest bats gate, which is red on the committed tag (see Blocker 1).

6. Stale scaffolding comments

internal/controller/siterouter/reconciler.go:15,258,414 describe fully-implemented methods (validateRemoteCIDRs, programNamespaceRoutes, verifyGatewayPortSecurityRelaxed, removeNamespaceRoutes, …) as "ordered stubs" / "no-op stub" / "placeholder returning nil". Update the comments so they don't misrepresent what is implemented.

7. Repo hygiene

No ```release-note ``` fenced block in the PR body (CONTRIBUTING requires one). Add'site-router': 'area/networking'to.github/workflows/pr-labeler.yaml` (already noted in the description).

Verified sound

• Deny-set validator (denyset.Validate): masking, IPv4-only rejection (incl. 4-in-6), /0 rejection, unconditional link-local/loopback, and bidirectional Overlaps are all correct; admission and controller share the same helper, so they cannot diverge.
• Admission fails closed and is wired on both Create and Update (pkg/registry/apps/application/rest.go:208,561); deny-set discovery errors (ConfigMap non-NotFound / Node / Service list) propagate.
• The api-key Secret is genuinely not tenant-readable: not in the cozyrd secrets.include, not in dashboard-resourcemap.yaml, and the tenant ClusterRoles grant no blanket get secrets; the guest login is locked (encrypted-password "*", no service ssh).
• Controller RBAC is least-privilege (secrets: get only, no cluster-wide list); secret redaction happens before message truncation; route merge/withdraw keying does not hijack a co-tenant's same-destination route; --leader-elect is hardcoded, so the in-memory hash cache is safe.

Verdict

NOT LGTM. Still not reviewable in one round: +20948/-84 over 127 files, so silence outside the findings is budget, not approval.

31 of 41 earlier findings are closed, each re-checked by mutation; nothing closed has regressed.

Findings

• [MAJOR] internal/controller/siterouter/reconciler.go:1015, a failed ConfigMap read re-stamps the management ACL and locks the controller out

• [MAJOR] hack/e2e-chainsaw/site-router/chainsaw-test.yaml:903, the set -e diagnostic loss fixed in step 12 survives in the acceptance gate

• [MAJOR] packages/apps/site-router/templates/dashboard-resourcemap.yaml:17, nothing holds the tenant-facing Secret grant, and the excluded Secret has tls.key

• [MAJOR] internal/controller/siterouter/reconciler.go:1081, a new tenant pod is unreachable until the 30s poll; the e2e cause is not established

• [MAJOR] internal/vyos/render/render.go:692, the rule set still has one unbounded factor, pushed under a fixed 30s timeout

• [MAJOR] packages/apps/site-router/templates/secret-cloudinit.yaml:49, an appliance bump desynchronises the pinned disk from the config.boot re-applied every boot

• [MAJOR] packages/apps/site-router/templates/_helpers.tpl:120, the management firewall admits 443 from the whole pod CIDR, so the token is the only control

• [MAJOR] packages/apps/site-router/values.yaml:129, the platform-owned _ values have no writer, and a hand-set one dies on the next edit

• [MAJOR] packages/apps/site-router/templates/service.yaml:20, loadBalancerClass has no immutability guard, though this repo has one for the same value

• [MAJOR] packages/system/site-router-rd/cozyrds/site-router.yaml:4, still no install-wait annotations on a release that waits on a LoadBalancer and a 12Gi import

• [MAJOR] internal/siterouter/denyset/discovery.go:41, still two uncached cluster-wide LISTs per instance every 30s, on one worker

• [MINOR] pkg/registry/apps/application/rest.go:662, the conflict-retry carry-over re-sends a changed annotation stale

• [MINOR] internal/siterouter/denyset/denyset.go:161, a deny-set rejection echoes a node or LoadBalancer address back to the tenant

• [MINOR] internal/controller/siterouter/reconciler.go:505, the controller half of the deny-set enumeration is untested

• [MINOR] internal/vyos/client.go:150, WithInsecureSkipVerify is dead exported code, and three comments still describe it as live

Still open from my earlier rounds

The placeholder-appliance CRITICAL is fixed, and not here: 41ef4a643 on main teaches the overlay to repair an unstamped images/*.tag. Run against the fixture that fails on this head it gives overlaid=1 drift=0, where this head gives drift=1. Rebase and it retires. Nothing here reverts it: this branch does not touch that script.

Four stay open, now recorded in followups.md as accepted residuals: the uncached LISTs, the unbounded rule product, the install-wait annotations, D9. An acknowledgement is a decision, so the first three are re-filed above.

Caveats

• The e2e enters through a hand-written HelmRelease, not a kind: SiteRouter, so aggregated-API admission, the _-key rejection and the cozyrd conversion all go unexercised.
• E2E (in-tree) was still running when this was written, so nothing rests on it. The one red check is the API-owner governance gate, not code.
• Needs a cluster: the LoadBalancer wait with no announcer, a rejected loadBalancerClass change, the VyOS config migration on a bump.

LGTM.

Both blockers from my last review are closed, each checked by mutation rather than by reading:

• e7c1664c9 — resolveManagementCIDR now returns an error and resolveInputs propagates it, so a failed ConfigMap read skips the push and the guest keeps the ACL it has. Restoring the default substitution reddens TestResolveManagementCIDR/a_ConfigMap_that_cannot_be_read_fails_instead_of_falling_back.
• 5deb9cfe8 — the loadBalancerClass guard is ported from extra/ingress, with an error naming the exact recovery command and warning that the external address is reallocated. Removing the guard reddens three tests covering set, change and clear.

The rebase also brought 41ef4a643, which retires the placeholder-appliance finding entirely, and ae2670869 corrects the cause recorded for the probe wait.

Scope of this approval, stated plainly: it clears the findings I raised. It is not a certification of all 21k lines, and the non-blocking notes from my previous review stay non-blocking. The Require API owner review gate is separate and still needs an API owner.

Blocking this pending a more detailed review. VyOS' license is not compatible with Cozystack.