Repository navigation
Ship an optional admission policy reserving MetalLB's Service pins - #6
Conversation
The design leaves the handoff gap and tenant steering of MetalLB to an admission layer. The chart can now install it: a ValidatingAdmissionPolicy that lets only the driver and allowlisted principals set, change or remove the MetalLB pin, pool and sharing annotations (both prefixes), spec.loadBalancerIP and the driver's pinned marker. Writes that leave them unchanged are admitted, so controllers that rewrite whole Services keep working. Off by default; the namespace selector, exemptions and validation actions are values. Assisted-By: LLM Signed-off-by: Timofei Larkin <[email protected]>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Review: optional admission policy (layer 1)
The policy matches the design's Security section. The protected set is exactly the authoritative list: metallb.io/loadBalancerIPs, metallb.io/address-pool, metallb.io/allow-shared-ip, the same three under the legacy metallb.universe.tf/ prefix, Service.spec.loadBalancerIP, plus the driver's own pinned marker (chart/metallb-iad/templates/admission-policy.yaml:30-40,53-59). The comparison is old value against new value, so an unchanged pin passes and the tenant can still edit the rest of the Service, and removing a pin is gated, both as the design requires. The CREATE path short-circuits on oldObject, so there is no null dereference. The driver's ServiceAccount is appended to the allowlist, so the driver does not block itself, and the status subresource is not matched, so MetalLB's status writes pass. failurePolicy: Fail is the right default for a theft fence. The values file documents that cluster admins must be allowlisted because the policy binds system:masters too, which is easy to miss.
[MINOR] The default namespaceSelector: {} with [Deny] is cluster-wide
With enabled: true and the defaults, the policy applies to every namespace. A non-allowlisted deputy that copies a tenant's annotations, cloud-provider-kubevirt copying a guest Service's metallb.io/* pin onto the host Service, then has the whole host Service rejected, not just the gated field. The design calls layer 1 "partial" for such deputies; under [Deny] the effect is a hard failure of Service creation rather than a partial gap. The values comment already advises scoping the selector to tenant namespaces; worth either narrowing the default or calling out the cloud-provider-kubevirt namespace explicitly. enabled: false by default keeps this from biting out of the box.
Note, not for this PR
The design says the admission policy "must cover Gateway.spec.addresses" (README §5). This policy fences MetalLB's inputs on Services, which is correct for the MetalLB driver, MetalLB does not honour Gateway.spec.addresses. The Gateway-by-value surface is honoured by Cilium LB-IPAM, so covering it belongs to the Cilium driver's policy and the distribution bundle (cozystack/cozystack#3382), not here. Flagging so it has an owner rather than falling between the two.
LGTM, with the namespaceSelector default worth a second look.
Design §8 leaves two jobs to an admission layer: protecting reserved addresses during the handoff gap (§5), and stopping tenants from steering MetalLB at reserved addresses directly. Without it, any principal that can write a Service can grab an address. They can pin it (
metallb.io/loadBalancerIPs), pick the reserved pool (metallb.io/address-pool), share another Service's IP (allow-shared-ip) or setspec.loadBalancerIP. MetalLB honours all of these againstautoAssign: falsepools. In Cozystack this also reaches tenant Kubernetes clusters: cloud-provider-kubevirt copies a tenant Service's annotations andloadBalancerIPonto the host Service it creates.This PR lets the chart install that layer as an opt-in
ValidatingAdmissionPolicyand binding (admissionPolicy.enabled, off by default).What it enforces
On Service
CREATE/UPDATE, only the driver's ServiceAccount and allowlisted principals may set, change or remove:metallb.io/loadBalancerIPs,metallb.io/address-pool,metallb.io/allow-shared-ip, and the same three under the legacymetallb.universe.tf/prefix;spec.loadBalancerIP;metallb.drivers.local.sdn.cozystack.io/pinned, the driver's marker for pins it owns.The policy compares against the old object, so a write that leaves these fields as they were is always admitted. Controllers that
Updatewhole Services keep working, cloud-provider-kubevirt's port sync among them.local.sdn.cozystack.io/ip-address-claimstays writable by tenants; it is the sanctioned path. A denial names the offending annotations and points at the claim annotation instead.Values
namespaceSelector(default{}, all namespaces). Cozystack should scope it to tenant namespaces. Its kube-ovn chart setsmetallb.universe.tf/allow-shared-ipin a system namespace and would otherwise be denied.allowedUsernames/allowedGroups. These are exempt in addition tosystem:serviceaccount:<release-ns>:metallb-iad. The defaults aresystem:mastersandkubeadm:cluster-admins. Admission policies apply tosystem:masterstoo, and kubeadm has issued admin credentials inkubeadm:cluster-adminssince 1.29. Installations whose admins authenticate otherwise, such as through an SSO proxy, need their group listed.validationActions(default[Deny]).[Warn, Audit]reports would-be denials without enforcing them, which suits a first rollout.README and design §8 are updated to match.
Verification
helm lintpasses with the policy on and off. I applied the rendered policy to a throwaway kind cluster (Kubernetes v1.32.2) with the binding scoped by a namespace label. It type-checks cleanly (status.typeCheckingis empty). These cases behaved as expected:metallb.io/loadBalancerIPsmetallb.universe.tf/allow-shared-ipspec.loadBalancerIPpinnedonto the tenant's Servicekubectl replaces it whole, pin untouchedpinnedlocal.sdn.cozystack.io/ip-address-claimkubeadm:cluster-admins) setsaddress-poolThe admin case failed until
kubeadm:cluster-adminswas added to the defaults, which is why it is there. The chart has no template tests, and none are added here.