[Timofei Larkin (lllamnyp)]

What

Adds a design proposal under design-proposals/ip-address-claims/ introducing IP addresses as a first-class resource: a cluster-scoped IPAddress, a namespaced IPAddressClaim, an IPAddressClass, and a provisioner contract in the shape of CSI. The kinds live in local.sdn.cozystack.io.

Why

A tenant cannot own an address. One appears as a side effect of creating a Service type: LoadBalancer, and evaporates when that Service is deleted. So an address cannot be:

• held across a workload rebuild (delete the Service, lose the address — someone else may get it),
• moved from one workload to another during a cutover,
• quota'd (there is no object to count; the only lever is pre-carving a pool per tenant),
• gated (anyone who can create a Service can cause an address to be allocated — RBAC authorizes verbs on resources, not fields),
• enumerated ("which addresses do we own, and who holds them?" has no answer short of listing every Service in the cluster).

The experience we want is the AWS one: allocate an address, keep it, attach it, detach it, attach it to something else, release it when done.

The model

The PersistentVolume pattern, applied to addresses:

storage	addresses	role
StorageClass	IPAddressClass	which pool, which provisioner, which announcer
PersistentVolume	IPAddress (cluster-scoped)	the address, with a claimRef and a reclaim policy
PersistentVolumeClaim	IPAddressClaim (namespaced)	"give me one" — the whole tenant-facing API
CSI driver	a provisioner contract	how a class's backend allocates, adopts, and pins

Two things fall out of the primitives survey and are worth a reviewer's attention:

• An address source union (fromClass vs providerRef), directly analogous to PV's volume-source union. Self-allocating backends (MetalLB, Cilium, kube-vip) have no reservation concept anywhere, so the ledger is the record of reservation. Cloud backends (AWS, GCP) already hold the reservation with a stable handle and their own IAM, so the provisioner must adopt, not allocate. Surviving a backend where allocation isn't ours is the evidence the shape isn't bent around MetalLB.
• A Pin capability, which the entire model depends on: a backend that cannot be told which address to use can never attach a reserved one, so a class over it must reject claims outright rather than allocate an address it can never bind.

The sharp edge (please read the Security section)

The per-backend "pin this IP" annotation (metallb.io/loadBalancerIPs, lbipam.cilium.io/ips, …) is a privilege-escalation surface: a self-allocating backend has no concept of a reservation, so it will happily hand one tenant an address another tenant has reserved but not yet attached.

The design answers this in two layers rather than one invariant, because a single one cannot hold:

• Layer 1, admission. Tenants may only write a reference to an RBAC-gated object in their own namespace (local.sdn.cozystack.io/ip-address-claim); the driver writes the raw pin. A ValidatingAdmissionPolicy identifies the driver by ServiceAccount and denies every pool-select and pin input the backend honours — legacy spellings and spec.loadBalancerIP included — to principals outside an admin-configurable allowlist. A deputy that forwards user content is denied, not allowlisted.
• Layer 2, reconciliation. Identity-keyed admission is defeated by anyone who can impersonate an allowlisted identity, so the IPAddress inventory is authoritative: any collision surfaces as status.phase: Conflict naming the offending Service, and a reservation is never silently overwritten.

Who may draw from which class is stated as a contract rather than solved here: namespace-keyed, so machinery-minted claims are covered; the empty className an explicit decision; addressName refused to tenants or checked in a webhook. The natural home is an optional per-namespace, per-class count in the core, whose shape is an open question.

Implementation status

The class-agnostic core (cozystack/address-controller) and a reference per-class driver for MetalLB (cozystack/metallb-iad) exist, with reservations held as placeholder Services so the backend's own accounting refuses to double-assign. The driver's main branch has no admission policy. The open PR cozystack/metallb-iad#6 adds it as an opt-in template; cozystack/cozystack#3382 packages both for Cozystack and carries a distribution-specific policy. cozystack/address-controller#7 adds the opt-in auto-claim path of §6.

The load-bearing evidence is a second consumer: cozyplane wears reserved addresses through this contract with no module import, no CRD dependency and no informer on the claim kinds — one annotation on a Service it already owns. That exercises the association seam. The allocation seam (a second provisioner) remains outstanding.

The document lists, under Implementation status, every rule it specifies that the code does not yet have.

Decisions

Two implementation findings changed the design rather than filling it in, and are recorded under decisions/:

• 0001 — the reference driver reserves through placeholder Services and lets MetalLB allocate, after a driver-side allocator proved invisible to the backend; the handoff gap and MetalLB's sticky assignments are the recorded cost.
• 0002 — the ledger adopts nothing; the core claims, opt-in per class, on behalf of Services that cannot name a claim, after tenant-cluster Services rendered by a cloud-controller-manager sat pending. Such claims follow their owner rather than the class reclaim policy.

Notes for reviewers

• Naming is settled. The kinds stay IPAddress*: at the ledger level ExternalIP beside a FloatingIP would be two names for one thing, and ExternalIP* would invite confusion with Service.spec.externalIPs, the field behind CVE-2020-8554. The group is local.sdn.cozystack.io; the argument is in Positioning, and design-proposal: application endpoints and EndpointAttachment #45 now shares the group.
• Two operating modes fall out of RBAC alone: self-service, where tenants create claims, and brokered, where platform machinery does and tenants attach and detach but never release. Metering follows the claim.
• Prerequisite for design-proposal: application endpoints and EndpointAttachment #45 (EndpointAttachment), which attaches an application's endpoint to an address and consumes the claim and the association annotation defined here. This proposal does not wait on it.
• The datapath is out of scope. Whole-IP 1:1 NAT (all ports in, and the workload egressing as that address) is a CNI concern tracked separately. This is the allocation half; that is the forwarding half. Allocation is useful on its own with a stock LoadBalancer Service.
• Nothing upstream is coming. KEP-1880 shipped ServiceCIDR/IPAddress and explicitly named "any generalization onto something like an IPAM API" a non-goal; loadBalancerIP was deprecated in 1.24 with no core replacement. Cluster API's IPAddressClaim/IPAddress is literally this pattern but has only ever been wired to Machine addressing, with no reclaim policy and no association.

The test that is the proposal

Claim → bind → associate → delete the Service → the address is still held → associate it to a different workload → the same address comes back.

That single integration test is precisely what cannot pass today.

[coderabbitai]

Walkthrough

The proposal defines three Kubernetes IP resource kinds, a provisioner capability contract, claim-based Service association, reconciliation and conflict handling, security controls, lifecycle states, testing scenarios, rollout sequencing, and alternatives.

Changes: Public IP claims

Layer / File(s)	Summary
Resource model and scope design-proposals/public-ip-claims/README.md	Introduces the proposal scope and defines IPAddressClass, IPAddressClaim, and IPAddress resources with their fields and invariants.
Provisioning and Service association design-proposals/public-ip-claims/README.md	Defines address-source modes, Allocate/Adopt/Pin capabilities, claim-based Service association, allocator ordering, and backend examples.
Security and lifecycle behavior design-proposals/public-ip-claims/README.md	Specifies admission and reconciliation controls, compatibility behavior, failure states, and testing scenarios.
Rollout and alternatives design-proposals/public-ip-claims/README.md	Outlines rollout sequencing, open questions, and rejected alternatives.

Estimated code review effort: 2 (Simple) | ~10 minutes

Pre-merge checks: 4

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Title check		The title clearly identifies the main change: a design proposal for first-class IP address, IPAddressClaim, and IPAddressClass resources. It is specific and related to the changeset.
Description check		The description covers the What, Why, model, security, implementation status, decisions, and reviewer notes for a design proposal. The template requires a decision-record statement for revisions to a …

Full details: Description check

Explanation

The description covers the What, Why, model, security, implementation status, decisions, and reviewer notes for a design proposal. The template requires a decision-record statement for revisions to a merged design proposal. The author states that decisions 0001 and 0002 were recorded under decisions/, which satisfies the first checkbox's condition. The DCO checkbox is not addressed in the description, but it is a commit-level attestation that cannot be verified from the description text. Two issues remain: the description says the proposal lives under design-proposals/ip-address-claims/, but the raw summary lists design-proposals/public-ip-claims/README.md, and the description uses the group local.sdn.cozystack.io and kinds IPAddress* while the objectives and summary reference ipam.cozystack.io. These conflicts do not block the check but should be reconciled.

Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch design/public-ip-claims

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

CodeRabbit Verify

• Run CodeRabbit Verify to test the changes in this pull request.

[gemini-code-assist]

Code Review

This pull request introduces a design proposal to treat public IPs as first-class resources in Cozystack, modeling them after the Kubernetes PersistentVolume pattern with PublicIP, PublicIPClaim, and PublicIPClass resources. The feedback highlights three key areas for improvement: updating the PublicIPClaim status schema to support dual-stack scenarios by using a list of IP objects, configuring MetalLB's IPAddressPool with autoAssign: false to prevent IP conflicts with standard services, and clarifying how the ValidatingAdmissionPolicy will securely identify the controller principal.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

If a Dual stack claim binds to two separate PublicIP objects (as suggested in Open Question 5), the current status schema for PublicIPClaim is insufficient because publicIPName and address are single scalar strings.

To support dual-stack where one claim maps to multiple PublicIP resources, the status should be designed to accommodate multiple names and addresses (e.g., using a list of IP status objects).

Suggested change

	status:
	phase: Bound
	publicIPName: pip-203-0-113-7
	address: 203.0.113.7 # what the tenant reads, and puts in DNS
	status:
	phase: Bound
	publicIPs:
	- name: pip-203-0-113-7
	address: 203.0.113.7

[gemini-code-assist]

There is a potential conflict between the Cozystack provisioner and MetalLB's automatic allocation. If a single IPAddressPool covering the entire range is configured in MetalLB (with autoAssign: true by default), MetalLB will automatically allocate any free IP from this pool to standard Services that do not use claims.

Since MetalLB is unaware of unassociated PublicIP reservations, it could allocate a reserved (but currently unattached) IP to a standard Service, leading to IP conflicts or binding failures when the claim owner later attempts to associate it.

To prevent this, the proposal should specify that the MetalLB IPAddressPool managed by the provisioner must have autoAssign: false configured, or clarify how standard Services are prevented from stealing reserved IPs.

[gemini-code-assist]

Using a ValidatingAdmissionPolicy to reject any principal except the controller from writing the pin annotations is a robust security measure. However, identifying "the controller" securely in a ValidatingAdmissionPolicy can be challenging and brittle if based solely on hardcoded usernames or namespaces.

Consider specifying how the policy will securely identify the controller principal (e.g., by matching a specific ServiceAccount name and namespace, or utilizing a specific SubjectGroup) to ensure it is both secure and maintainable across different deployment environments.

[mattia-eleuteri]

A few reactions grounded in a production Cozystack deployment that is squarely the
world this proposal has to serve: MetalLB-only for LoadBalancer allocation
(Cilium LB-IPAM unused), a single shared, auto-assign pool spanning all
tenants, and cozy-proxy whole-IP 1:1 NAT as the datapath for tenant VMs.

Strong +1 on the shape, and especially on the security move as a standalone win

The PV/PVC/StorageClass framing is the right altitude, and the Security section
describes a hole that is live in our cluster today: our public pool is
auto-assign and cross-tenant, so anyone who can create a Service can write
metallb.io/loadBalancerIPs: <any address in the pool> and be handed an address
another tenant is relying on. The ValidatingAdmissionPolicy that forbids any
principal but the controller from writing a pin annotation is worth landing on
its own, independent of the rest of the model. Please keep it in phase 1 as you
say. One ask: the VAP must explicitly allowlist the platform's own controllers and
the GitOps reconcilers — in our cluster the system istio Services already carry
pool annotations written by non-tenant principals, and a naive policy would break
reconciliation.

The "regression, not a wishlist" framing overstates it for at least one adopter

kube-ovn is installed for us and the OvnEip/IptablesEip CRDs exist, but we run
zero OvnEip instances — the reserve-then-associate capability was never
actually adopted; the 1:1 datapath goes through cozy-proxy instead. So for us this
is a new capability, not one being lost. The motivation stands on its own
merits (hold / move / quota / enumerate / bill); I'd lean less on the regression
argument, since for a real deployment it isn't one.

Suggested additional open question: multiple public addresses per workload

Fine to defer the design, but I'd like it captured explicitly, because it
interacts with the allocation/datapath split. A workload may want more than one
public address (1 v4 + 1 v6, or 2 v4), and the three cases are not the same:

1. Dual-stack (v4 + v6): clean — one dual-stack Service, two PublicIPs, one
   claim. This is essentially open question 5.
2. Two same-family, ingress-only (DNAT N→1): both addresses DNAT to the same
   backend, egress via the primary. The secondaries are entry points, not
   owned addresses.
3. Two same-family, each with full 1:1 whole-IP semantics: the hard one.
   Whole-IP is inherently 1 public ↔ 1 internal address, so N owned addresses
   require N internal addresses on the workload — bringing up an interface (or
   address) per claim, plus guest-side config the platform can't perform
   (cloud-init/DHCP inside the guest). This lands squarely in the deferred datapath
   half.

Two API observations that this open question should keep in mind:

• Association is currently 1 claim ↔ 1 Service (the annotation names a single
  claim; "two Services reference one claim → reject"). Expressing "this VM holds N
  addresses" would need a workload to reference a set of claims. Open questions
  5 and 7 already circle this — they might collapse into one "a workload may bind N
  claims" question.
• The Pin "reject rather than half-support" principle should probably extend to
  this: a backend/datapath that can't attach N owned addresses to one workload
  should be able to say so (a declared capability), so a class refuses such a
  claim rather than reserving an address it can never land.

None of this blocks the core question — "is an address a resource?" is a clear yes,
and the single-address path is well worth landing first. Just flagging the
multi-attach case as an open question so the allocation/datapath boundary is drawn
with it in mind rather than around it.

[Timofei Larkin (lllamnyp)]

Thanks — this is the production grounding the proposal needed, and all three points landed. The revision just pushed reflects them.

VAP allowlist — agreed, and it's now explicit, but admission is only half of it. You're right that a naive "reject everyone but the controller" breaks reconciliation the moment a GitOps reconciler or a system Service writes a pool annotation. The policy now identifies the controller by its ServiceAccount (configured at install, not a hardcoded username) and denies the write only for principals outside an explicit, admin-configurable allowlist (controller SA + GitOps/system SAs and groups). It also covers the pool-select annotation (metallb.io/address-pool), not just the IP pin — naming the reserved pool is as dangerous as pinning an address.

But identity-keyed admission has a ceiling: anyone who can impersonate an allowlisted ServiceAccount — a cluster-admin can — sails past it. Rather than pretend otherwise, the design now stands on two layers. Layer 1 is that admission policy, which reduces the blast radius for tenants and namespaced principals — and it's still worth landing on its own, since the theft window it closes is live in any cluster running a shared auto-assign pool. Layer 2 is reconciliation: the IPAddress inventory is authoritative, so any collision — an impersonated write, a direct MetalLB edit, a misconfigured pool — surfaces as IPAddress.status.phase: Conflict with the offending Service named, and the rightful reservation is never silently overwritten. The cluster-admin case is stated as a trust boundary (they're the cluster's trust root; handing out a reserved address is within their authority, not an escalation), handled by detection rather than a prevention promise the model can't keep.

Regression framing — dropped entirely, and further than "for you it's new." In Cozystack it isn't a regression at all: the platform uses kube-ovn only for subnets and VPCs, never its EIP objects, so there's no reserve-then-associate capability being lost. Anyone driving OvnEip deliberately is doing so outside Cozystack. The proposal now argues purely on merits — hold, move, quota, enumerate, bill.

Multiple addresses per workload — captured, but as explicitly deferred scope, not part of the core model. It's a step beyond "an address is a resource," and it's not clear the platform-owned case is even real. The hard sub-case you name — N owned same-family addresses, each needing its own internal address plus in-guest config the platform can't perform — is the one that looks orthogonal to reservation rather than an extension of it, since those extra addresses tend to live on interfaces the platform neither owns nor sees. So the model keeps 1 claim ↔ 1 Service, and the multi-attach case waits for a concrete platform-owned use case to shape it. Dual-stack (one v4 + one v6) is the one narrow multiplicity the model does admit — the claim status is a list from the start — but that's kept separate from the harder "N owned addresses" case.

Two changes worth flagging for your setup specifically. The kinds are renamed IPAddress/IPAddressClaim/IPAddressClass in ipam.cozystack.io — a class names a pool, and pools aren't necessarily public. And because you run MetalLB with a shared auto-assign pool, note that autoAssign: false on a reserved pool only stops automatic selection (a Service can still name the pool or pin an address), so the proposal adds an allocator-first reconciliation path: MetalLB assigns the moment it sees a Service, before any claim exists, and the controller reconciles those addresses back into the inventory toward eventual consistency rather than assuming every address began life as a claim. One honest gap comes with that: an eager allocator can still hand a plain Service an address that is already reserved-but-unattached, which reconciliation detects but cannot cleanly undo — a real fix likely waits for the per-provider provisioners that own allocation, and it's flagged in the doc as deferred to implementation.

[Andrei Kvapil (kvaps)]

Overall this is cleaner than what I had — I'll close my additive-service PR in favor of this direction. The object model and the two-layer security story look right, and the controller/backend logic is fine.

The structural decision I want to confirm we're aligned on: this proposal deliberately covers only the allocation half (address as a resource — reserve / hold / move / quota / enumerate) and pushes the exposure half (what/how to publish, ports, RO vs RW, dropping external: bool) onto #29. That split is the right answer to "does this object own the IP or the fact of exposure": ExternalIP/Claim own the address, #29 + the app operators own the target. Let's make that boundary explicit in both docs — a reader will otherwise expect one object to do both.

What I'd like Timofei Larkin (@lllamnyp) to expand — the API surface for existing multi-service apps. The concrete driver is Postgres, which exposes three endpoints (-rw primary, -ro replicas, -r any); today external: true publishes only the primary.

1. Exposing specific endpoints. Replace external: bool with an expose: [] list, each entry {target, class, [claimRef], ...}, where target is an app-defined vocabulary (rw|ro|r for Postgres; the chart owns the target → selector/ports mapping). An omitted target stays ClusterIP.
2. Same endpoint on multiple classes. Because it's a list, not a map keyed by target, the same target can appear twice with different class — e.g. rw on both a public range and the internal GPN range. Please define deterministic naming for the resulting Services/claims.
3. Order-first or not? Two binding modes, distinguished by whether the entry carries a claimRef: dynamic (the operator mints an ephemeral claim from class — the default, no pre-ordering, backend sugar only) vs static (claimRef to a pre-reserved held address, for the Elastic-IP hold/move case). State the default per app type (VM → static/hold, managed services → dynamic). The answer to "must a user reserve an IP before exposing anything" should be no by default.
4. Whole-IP / Service ownership. ExternalIP must not create or own the Service — the app stays the sole owner. Whole-IP vs port-list, ports and ICMP are fields of the app's expose[] entry (only VM allows WholeIP), and the chart keeps rendering the existing service.kubernetes.io/service-proxy-name + networking.cozystack.io/wholeIP contract with cozy-proxy unchanged. The association controller writes only the backend pin annotation (metallb.io/loadBalancerIPs) via SSA under its own field manager, so app and controller own disjoint fields and never fight. A held-but-unattached address is kept alive by a separate holder Service in the ipam namespace, never a second owner on the app's Service.
5. external: true deprecation. Additive migration with auto-translation: external: true + empty expose synthesizes the legacy entry (for Postgres a single {target: rw, class: <default>}, matching today's primary-only external Service); external + non-empty expose is a validation error. This needs a per-app table, since the legacy meaning of external differs across apps (vpn, tcp-balancer, vm-instance).

Two things to correct in the current draft:

• Open question 3 (ExposureClass) is stale. ExposureClass/ServiceExposure and the network.cozystack.io group no longer exist — they were dropped in favor of native loadBalancerClass (#3164 / #3218). The real question is how ExternalIPClass relates to loadBalancerClass and to #29's class, not whether it's a second class kind next to ExposureClass.
• The "one claim per workload / one address per workload" non-goal conflicts with a live requirement: a managed service reachable on a public address and an internal (GPN) address at the same time. Either drop that non-goal or state explicitly that multi-network is expressed as multiple expose[] entries on the #29 side (one external Service per address).

Naming nit: I'd lean toward ExternalIP / ExternalIPClaim / ExternalIPClass over IPAddress*. It's narrower and accurate (external to the cluster — true for both the public and the internal-routable ranges), and it sidesteps the networking.k8s.io/IPAddress (KEP-1880) and Cluster-API IPAddress collisions the doc already flags. One caveat: mild overlap with the Service.spec.externalIPs field, worth a sentence in the doc. Group stays ipam.cozystack.io.

[Aleksei Sviridkin (lexfrei)]

The shape is right and I want this to land, but three findings hit the shape itself, so requesting changes. All three can be resolved as design changes or as explicit open questions.

1. Gateway is missing from the association surface. The accepted external-database-exposure proposal puts a tenant's external traffic on the single LB IP of the tenant Gateway, so the address a tenant most wants to keep across a rebuild belongs to a Service rendered by Cilium, and a Service annotation can't reach it. Gateway API already has Gateway.spec.addresses for this (Cilium supports it with LB-IPAM; io.cilium/lb-ipam-ips in infrastructure annotations is the legacy path). Which Cilium release supports it needs checking against the deployed 1.19.x. Please add Gateway as an association target, or at least an open question.

2. Reservation can be enforced in the allocator's own accounting, with detection kept as backstop. Neither MetalLB nor Cilium will give one IP to a second Service without an explicit sharing key: the second Service just stays Pending. A controller-owned holder Service pinning each reserved-but-unattached address makes the §6 "eager allocator got there first" theft impossible on both backends (the keep-alive mechanism already sketched in this thread). Also §6 is more pessimistic than §7 allows: with autoAssign: false a plain Service can never draw a reserved address automatically, so the remaining gap exists only through annotation writers, which the two layers already govern. I'd make holder-based reservation the primary mechanism and rewrite §6.

3. The allowlist model breaks on confused deputies, and one is in-tree today. Tenant HelmReleases are emitted without spec.serviceAccountName, so helm-controller applies tenant-driven content under its own privileged identity; allowlisting Flux allowlists whatever a chart renders from tenant values. Sharper: cloud-provider-kubevirt copies tenant-cluster Service annotations verbatim into the infra Service it creates (pkg/provider/loadbalancer.go, no filter, no config knob), and every Kubernetes-app tenant is admin of their tenant cluster, so any tenant can push metallb.io/loadBalancerIPs through the CCM. Allowlist the CCM and layer 1 is bypassed; deny it and tenant-cluster LB Services with any gated annotation break. The Security section should treat "trusted SA applying tenant-authored content" as a first-class threat and make annotation filtering at the deputy (a strip-list in the CCM, Flux impersonation for tenant releases) a precondition of layer 1.

Minor, non-blocking:

• "Two Services reference one claim, the second is rejected": name the mechanism. A VAP can't read the claim (CEL sees only the request object and params), so admission-time rejection needs a webhook. I'd borrow the Gateway API attachment model instead: attachment declared on one side, conflicts resolved deterministically (oldest creationTimestamp wins), outcome reported as Accepted/Conflicted status conditions. It also scales to open question 6 (several Services on disjoint ports is "many routes, one listener") and leaves room for weights later.
• §6 implies inventorying every LB address in the cluster, foreign auto-assign pools included. I'd scope the authoritative ledger to class-managed ranges and keep the rest observe-only, otherwise Conflict noise on unmanaged pools is guaranteed.
• PR title and body still say PublicIP* and still make the regression argument the doc has dropped; the directory is still public-ip-claims.
• +1 to ExternalIP* naming. Since KEP-1880 went GA in 1.33, networking.k8s.io/v1 IPAddress exists in every cluster by default, so kubectl get ipaddress ambiguity is daily. Worth the one caveat sentence on Service.spec.externalIPs (CVE-2020-8554).
• There is no analogue of PVC.spec.volumeName: a tenant can't ask for a specific previously-held address, pre-binding is admin-side only via claimRef. Fine if intentional, worth one line.
• "bill" appears in the motivation but not in goals or the model; a Dual claim against a single-stack Service isn't in the failure cases.

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[coderabbitai]

🧹 Nitpick comments (3)

design-proposals/public-ip-claims/README.md (3)

508-514: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Clarify the parenthetical example of an allowlisted system principal.

"system controllers (system Istio Services ship with pool annotations already set)" is confusing — it's unclear whether "system Istio Services" is the principal or the annotation source. Consider rephrasing, e.g. "system controllers (e.g. an Istio installation whose Services ship with pool annotations pre-set)".

🤖 Prompt for AI Agents

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

In `@design-proposals/public-ip-claims/README.md` around lines 508 - 514, Clarify
the allowlist example in the parenthetical within the ServiceAccount policy
description: rephrase “system Istio Services” to make clear that an Istio
installation is the system controller or annotation source, while its Services
ship with pool annotations pre-set. Preserve the surrounding explanation of
admin-configured allowlisted principals.

---

300-317: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider naming the multi-address capability gap in the capability table itself.

The review feedback asked to consider capability declarations for backends that cannot attach multiple reserved addresses to one workload. The doc defers "multiple platform-owned addresses per workload" as a non-goal (lines 162-169), but the capability table (lines 303-308) doesn't foreshadow a future MultiAttach-style capability the way it does for Pin. Since dual-stack is explicitly in scope (open question 4) and would need two IPAddress objects bound to one Service, a note here that a backend's ability (or not) to attach more than one reserved address is a capability question for later would tighten the connection between §4 and the dual-stack open question.

🤖 Prompt for AI Agents

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

In `@design-proposals/public-ip-claims/README.md` around lines 300 - 317, Add a
brief note to the capability table or immediately surrounding discussion
introducing a future multi-address attachment capability, such as MultiAttach.
Connect it to backends attaching multiple reserved addresses to one workload,
particularly dual-stack Services requiring two IPAddress objects, while
preserving the current non-goal and deferring the capability’s final design.

---

113-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Blank lines inside blockquote break rendering (MD028).

Static analysis flags blank lines inside the blockquote block spanning these pull-quotes; some renderers will terminate the blockquote early. Separate the quotes with > continuation or a blockquote marker on the blank line instead of a bare blank line.

🤖 Prompt for AI Agents

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

In `@design-proposals/public-ip-claims/README.md` around lines 113 - 116, Update
the pull-quote block in the README so the blank line separating the two quoted
paragraphs retains the blockquote marker, preventing the blockquote from being
terminated while preserving both quotes.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@design-proposals/public-ip-claims/README.md`:
- Around line 508-514: Clarify the allowlist example in the parenthetical within
the ServiceAccount policy description: rephrase “system Istio Services” to make
clear that an Istio installation is the system controller or annotation source,
while its Services ship with pool annotations pre-set. Preserve the surrounding
explanation of admin-configured allowlisted principals.
- Around line 300-317: Add a brief note to the capability table or immediately
surrounding discussion introducing a future multi-address attachment capability,
such as MultiAttach. Connect it to backends attaching multiple reserved
addresses to one workload, particularly dual-stack Services requiring two
IPAddress objects, while preserving the current non-goal and deferring the
capability’s final design.
- Around line 113-116: Update the pull-quote block in the README so the blank
line separating the two quoted paragraphs retains the blockquote marker,
preventing the blockquote from being terminated while preserving both quotes.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 92f6ae23-9417-427d-bba0-ee0da03e51ba

📥 Commits

Reviewing files that changed from the base of the PR and between 789654a and bfc1675.

📒 Files selected for processing (1)

• design-proposals/public-ip-claims/README.md

[Timofei Larkin (lllamnyp)]

Two updates that bear on the open naming question, plus the argument for the API group the implementation ships. The document has just been updated to match both.

The contract has a second consumer, and it isn't MetalLB-shaped

The model is implemented and running: address-controller is the class-agnostic core (the PVC/PV binding analog), metallb-iad the reference per-class driver. One backend proves little on its own, so the evidence worth putting in front of reviewers is the second consumer.

Cozyplane consumes reservations through this contract, and does it through one annotation and nothing else: no module import, no CRD dependency, no informer on the claim kinds. A FloatingIP (or a VPCGateway's NAT identity) names an IPAddressClaim; the controller copies that into local.sdn.cozystack.io/ip-address-claim on the Service it already owns, and the claim's driver pins the address there. The annotation key is a duplicated constant mirroring the core's well_known.go as the named authority. Reservation is strictly optional: with the mechanism absent entirely, the LB implementation auto-assigns and everything else behaves identically — reserved and dynamic are one code path.

Validated end to end on a three-node dev cluster with the full stack (address-controller + metallb-iad + MetalLB + cozyplane): a FloatingIP naming a claim wore exactly the reserved address, survived delete-and-rebind with the same address, and stayed externally reachable throughout.

To be clear about the dependency direction: this proposal does not wait on cozyplane, and cozyplane is not the reason to land it. Attachable/detachable addresses are a prerequisite for #45, and the MetalLB path serves stock Cozystack today. Cozyplane is corroboration, not motivation — it happens to be the sharpest available test of one specific claim, below.

That claim is narrower than the whole design. Rollout §2 names a Cilium provisioner as the phase that validates or falsifies this — that's the allocation seam, and it's still outstanding. What cozyplane exercises is the association seam: whether a backend with an entirely different datapath can wear a reserved address while knowing nothing about the ledger beyond one annotation. It can, and the coupling turned out looser than this document assumed it would need to be.

The API group: local.sdn.cozystack.io, not ipam.cozystack.io

The document said ipam.cozystack.io while the implementation shipped local.sdn.cozystack.io. That divergence was deliberate and belongs in this thread rather than in another repository's commit message; the document now says local.sdn.cozystack.io and carries the argument. Andrei Kvapil (@kvaps), this is the one part of your review I'd push back on ("Group stays ipam.cozystack.io").

The argument is just this: networking APIs in Cozystack shouldn't accumulate several different group names. Where we stand today:

• sdn.cozystack.io already exists — SecurityGroup, served by the Cozystack API as a tenant-facing interface over Cilium network policies.
• Cozyplane serves its own SecurityGroup in that same group, plus local.sdn.cozystack.io for its CRD-served kinds. The two aggregated servers nominally collide on sdn.cozystack.io, but not in practice: with cozyplane you don't run Cilium, so cozystack-api has no reason to serve that group there.
• And now addresses arrive as a third networking feature looking for a home.

My position is that all of it is one sdn family, with the local. prefix marking CRD-served kinds and the bare group the aggregated ones. ipam.cozystack.io isn't wrong on its own terms — it just adds a fourth name to a subject area that should have one.

One caveat so this isn't mistaken for an appeal to precedent: local.sdn.cozystack.io is new. Nothing serves it today, and address-controller would be among the first things to introduce it. And nothing about the group string creates a Cozystack dependency for standalone users — they install the same three CRDs under any name.

If there's disagreement about folding addresses into sdn, I'd rather we revisit group naming across all three at once — Cozystack API, address-controller, cozyplane — than settle it per-component and end up with exactly the spread this is trying to avoid.

Naming: two questions, not one

Andrei Kvapil (@kvaps) Aleksei Sviridkin (@lexfrei) — on ExternalIP*. There are two axes here and I'd like them decided separately, because the proposal spans both:

1. Kinds. The case for ExternalIP* is ergonomic and real: networking.k8s.io/v1 IPAddress has been GA and present in every cluster since 1.33, so kubectl get ipaddress is ambiguous daily. The case against is narrower than it first looks — at the claim level both read fine, since ExternalIPClaim and IPAddressClaim are equally unambiguous next to a FloatingIP. Where it bites is the bare cluster-scoped ledger kind: ExternalIP and FloatingIP side by side in one group family are two names for "the external address", while IPAddress reads as the ledger entry it is. Add the Service.spec.externalIPs overlap you already flagged (CVE-2020-8554). I don't have a strong preference and I'm happy to go with the consensus here.
2. Group. Here I do: local.sdn.cozystack.io, for the reasons above.

Renaming is mechanical in the core and the driver, but it isn't free — it also touches cozyplane's API surface and its generated clients. So I'd rather settle both axes and do it once than carry two names through another cycle.

Housekeeping

The push also renames the directory public-ip-claims → ip-address-claims, adds an Implementation status section carrying the above, and records that the ValidatingAdmissionPolicy of Security is implemented in neither repository — it remains the outstanding half of phase 1, not a phase 2 item. The PR title is updated as well; it had still been advertising PublicIP*, a name this document dropped in its first revision.

Aleksei Sviridkin (@lexfrei)'s three findings are a separate reply, still to come.

[Timofei Larkin (lllamnyp)]

Aleksei Sviridkin (@lexfrei) — all three land. Two became corrections to the document, just pushed; the third moves into implementation. Where each went:

1. Gateway is missing from the association surface

Agreed, and the underlying problem is more basic than a missing target: §5 assumed the address consumer is a Service somebody wrote. Under Gateway API the implementation renders the data-plane Service from the Gateway, so there is no user-authored Service to annotate and the object the user does write has no way to name an address.

What the document now says is the part that's design rather than mechanism: the contract is an annotated object that resolves to an address consumer, with Service the first member and Gateway the obvious second. Which object a given provisioner annotates and where it lands the pin — spec.addresses, infrastructure annotations propagated onto the generated Service, or the generated Service itself — is provisioner business, and is being worked out in the address-controller rather than specified here.

One consequence is not provisioner business and is now in Security: Gateway.spec.addresses names an address by value, so it is the same escalation surface as a backend pin annotation, and the admission policy has to cover it. Supporting Gateways without that would reopen exactly the hole layer 1 exists to close.

Which Cilium release honours spec.addresses against the deployed 1.19.x I haven't verified and haven't asserted; it stays the open question you framed.

2. Reservation enforced in the allocator's own accounting

Agreed, and this was the document being behind its own implementation. The MetalLB provisioner already holds every reserved-but-unattached address with a placeholder Service — selectorless, in a provisioner-owned namespace, assigned by MetalLB and never announced. §6 is rewritten with that as the primary guarantee and detection as the backstop, and your §7 point is taken: with autoAssign: false a plain Service can't draw a reserved address automatically, so what's left is annotation writers, which both layers already govern.

Two things the rewrite keeps rather than smooths over: the handoff (releasing the holder and pinning the workload's Service isn't atomic, and the address is briefly unheld), and your scoping point — the ledger is authoritative only over ranges a class manages, with everything else observed rather than adjudicated, so foreign auto-assign pools don't generate Conflict noise.

3. The allowlist breaks on confused deputies

Agreed, and this is the one that changed what the document is allowed to claim. An identity allowlist assumes a trusted principal writes content it authored, and that fails wherever a privileged controller applies material a user supplied — a reconciler that applies user-authored releases under its own identity instead of impersonating, a cloud-controller-manager copying Service annotations verbatim out of a user-controlled cluster. Allowlist the deputy and everyone behind it inherits the gated write; deny it and reconciliation breaks.

Security now states annotation filtering at the deputy as a precondition of layer 1, not a hardening extra, and says plainly that layer 1 is partial for the users behind an unfiltered deputy, with layer 2 covering them meanwhile. That's a weaker claim than the section made before, and the honest one.

The minor points

• Two Services on one claim — you're right that a VAP can't do it; CEL sees the request object and the policy's params, not arbitrary cluster state. It's now specified as reconciliation with a deterministic winner and a Conflicted loser. Fixing it turned up the same error one bullet down: "backend lacks Pin → rejected at admission" was equally impossible, since there's no capability registry for admission to consult either. That one is now the provisioner refusing and reporting on the claim.
• No PVC.spec.volumeName analogue — there is one; the document simply never mentioned it. spec.addressName is in the claim example now.
• "Bill" — out of the motivation. Enumeration and quota are what the object model delivers; billing is something a platform builds on top, and claiming it here overstated the design.
• Dual claim against a single-stack Service — added to the failure cases.
• PR title, body and directory — all three done; nothing still advertises PublicIP* or runs the regression argument.
• +1 on ExternalIP* — noted with kvaps'. That's live in the API-group comment above, where the kinds and the group are split into separate questions.

The changes-requested is yours to clear when you've had a look.

[Andrei Kvapil (kvaps)]

The implementation evidence changes what this document is. A class-agnostic core plus a reference driver, and a second consumer that touches the contract through one annotation and no imports, is the strongest possible answer to "is this shape bent around MetalLB" — much stronger than the argument alone. The address-source union is the right spine, and the Security section is unusually honest: naming the confused-deputy case, saying layer 1 is partial for users behind an unfiltered deputy, and framing cluster-admin as a trust boundary rather than pretending to close it. The response to Aleksei Sviridkin (@lexfrei) is thorough — generalising Gateway into "an annotated object that resolves to an address consumer" is a better answer than adding a second target.

Four things.

1. addressName is tenant-writable and entirely ungoverned. It appears exactly once in 738 lines — commented out in the claim example, described as "the PVC spec.volumeName analogue" — and nowhere else. Not in Security, not in Failure and edge cases, not in RBAC.

That matters because IPAddress is cluster-scoped and IPAddressClaim is the tenant-facing object. The security model rests on the invariant stated in Security §1: a tenant may only name an object in their own namespace, because that is what RBAC can gate. addressName breaks exactly that invariant — it names a cluster-scoped object by name, so a tenant could bind a specific Available address in a class they were never granted, or name an address whose class contradicts their own className. Layer 1 gates the association annotation and the backend pins and never sees this field.

Note Aleksei Sviridkin (@lexfrei) read the document as having no volumeName analogue at all ("pre-binding is admin-side only via claimRef"). One of the two readings is wrong, which is itself the signal. Either drop the field and say pre-binding is admin-side via claimRef, or specify the checks: class match, class-level authorization, and the behaviour when it names a Bound or Released address.

2. ExposureClass is stated as existing, and it does not. Scope says "an ExposureClass kind exists today in network.cozystack.io", and open question 3 is anchored to "the fate of ExposureClass/ServiceExposure". On main today, ExposureClass appears only in ROADMAP.md and docs/changelogs/v1.6.0.md, and network.cozystack.io has zero occurrences under packages/ or api/. The kind was never shipped — the direction went to native loadBalancerClass (cozystack/cozystack#3164, cozystack/cozystack#3218). As written, open question 3 cannot be answered, because its subject does not exist. The real question is how IPAddressClass relates to loadBalancerClass.

3. The one-claim-per-workload non-goal still excludes a live case. The added rebuttal answers the VM-as-router example — extra addresses on a tunnel interface the platform never sees — and that answer is right. It does not reach the case I raised in July: a managed service reachable on a public address and an internal GPN address at the same time. Both are platform-owned, both are real Services, and "a workload holds one claim" forbids it. One sentence settles it: multi-network is expressed as several Services, each with its own claim.

4. Group name, non-blocking. local.sdn.cozystack.io files an IPAM primitive under the SDN module, but its consumers include Gateways and cloud providers that are not SDN. ipam.cozystack.io reads closer to what the object is. Your call.

On naming: you and I disagree about IPAddress* versus ExternalIP*, the Naming section states your reasoning openly, and that is enough — I am not going to press it further.

None of these blocks the document. Point 1 is the one to settle before it turns into code — as a commented-out line with no spec text around it, it is currently ambiguous enough that two reviewers read it differently, and the invariant it touches is the one the whole security model rests on. Points 2 and 3 are a few lines each.

Approving. Note this does not clear Aleksei Sviridkin (@lexfrei)'s outstanding review.

[Timofei Larkin (lllamnyp)]

Four replies in one, after today's push (d6e35ac, rebased onto main).

Aleksei Sviridkin (@lexfrei) — since your review the document has changed in four pushes, the last two today, and I owe you a summary rather than a diff.

Your three findings: Gateway is an association target as "an annotated object that resolves to an address consumer", with Gateway.spec.addresses named as a fenced input, and it is now rollout phase 2 ahead of the Cilium provisioner, since the accepted database-exposure proposal puts a tenant's traffic on the Gateway's single address. Holder Services are the primary reservation mechanism and §6 is rewritten around them. Deputy-side filtering is a precondition of layer 1, and the text now goes further: a deputy that forwards user content is denied, not allowlisted, and its Services reach the ledger through the claim annotation or the core's opt-in auto-claim.

Two things the implementation taught us are recorded as decision records under the proposal rather than silently amended: the reference driver lets MetalLB allocate through placeholder Services after a driver-side allocator proved invisible to the backend (0001), and the backwards inventory is dropped in favour of auto-claim for Services that cannot name a claim (0002).

Also from the implementation: the fence must cover spec.loadBalancerIP and MetalLB's legacy annotation prefix, which the controller still parses; the policy ships Warn and Audit modes; two Services on one claim resolve as "the live holder keeps it" with an Associated condition on the claim; and IPAddress gains conditions with transition times.

Would you take another look?

Andrei Kvapil (@kvaps) — your approval of 2026-08-31 was dismissed automatically by the push that followed it four seconds later, so I am asking for it again.

Of your four points: addressName is specified and now tied to the volumeName precedent, which authorizes nothing and leaves pre-binding as the admin's lever; ExposureClass is gone; the public-plus-internal case is explicitly two Services with two claims; the group stays local.sdn, and #45 now sits there too. Since then the document has caught up with the implementation and carries two decision records for the places where implementation changed the design. The entitlement question that #45's review raised is answered as a contract here: namespace-keyed, so machinery-minted claims are covered, with an optional per-namespace, per-class count in the core as the deferred home.

IvanHunters — thanks, and sorry for the delay. All four findings hold, and the answer to your question is: the entitlement policy belongs in the distribution, because its parameter is the Tenant object, which this controller never sees. Please open the PR against cozystack/cozystack.

What the proposal now states, so the two sides stop pointing at each other, is the contract any such policy meets: it keys on the request namespace rather than the requesting user, because platform machinery mints claims under its own identity; the empty className case is an explicit decision, never a convenience clause; and spec.addressName is either refused to tenant principals outright, which is the reference policy's choice, or checked in a webhook, since CEL cannot resolve it. The longer-term home is an optional per-namespace, per-class count in the core, mirroring the per-class quota scoping Kubernetes applies to storage; its shape is an open question and the tenant chart is the obvious renderer.

myasnikovdaniil — done in the latest push: Status and Date are gone, and the Decisions section is in, with two records for the places where implementation changed the design.

[Andrei Kvapil (kvaps)]

LGTM as of d6e35ac. All four from my last review are in: addressName filters and never authorizes, ExposureClass is gone, two Services with two claims covers public plus internal, and the group choice is written down. The per-class entitlement as quota, zero meaning not entitled, is the same shape we use for GPU budgets in #99.

[IvanHunters]

Architectural review (design + reference implementation at address-controller#7 / metallb-iad#5)

Reviewed this as a network/platform architect, with the reference controllers checked out alongside the document because the Implementation status section makes claims about them. The model is sound: the PV pattern fits, the allocation/association split is the right seam, and the two decision records close real defects from the first iteration. Most of what follows is about one thing, the association handoff, plus a couple of places where the document and the code have drifted apart.

[CRITICAL] Association has no serviceability precondition, so the handoff silently black-holes the reserved address

associate() acts on the presence of the claim annotation alone; it never checks svc.Spec.Type or svc.Spec.LoadBalancerClass (metallb-iad internal/controller/service_controller.go). The handoff then runs unconditionally: set associatedTo, delete the hold placeholder (which frees the address in MetalLB's books), write the pin.

If the annotated Service is one MetalLB will not serve, a non-empty loadBalancerClass while the speaker runs with an empty --lb-class, or a non-LoadBalancer type, the address is never announced. And because associatedTo now names a live Service, the driver's IPAddress reconciler takes the "associated" branch and deletes placeholders rather than re-arming the hold (internal/controller/ipaddress_controller.go). The end state: the address is unheld in MetalLB, announced nowhere, available for an eager allocator to hand to someone else, while the ledger reports associatedTo set and the claim Bound. That is a silent failure of the exact invariant this proposal exists to provide, and it hits the headline use case, the address a customer has in a firewall allow-list.

The document flags the §5 loadBalancerClass match check as unimplemented, but not this active consequence: the handoff tears down the hold before confirming the target can be served. The contract should require association to refuse a consumer it cannot serve, with an event, before it releases the hold.

[HIGH] As shipped there is no admission layer, though the document says there is

Rollout states the admission policy "exists as the driver's opt-in template." In the reviewed branches there is no webhook and no ValidatingAdmissionPolicy manifest: the chart ships only role.yaml and controller.yaml, and the only references to the policy are in README/design.md. It lives in metallb-iad#6, not here. So in this state Security's layer 1 is absent and the reserved-but-unattached theft window the proposal calls its sharp edge is open. The driver ClusterRole also grants services: update/patch cluster-wide, which is the escalation surface layer 1 is meant to fence. The document should say layer 1 is not yet in the controllers rather than imply it ships with them.

[HIGH] The association handoff is the fragile core: non-atomic and detection-only

Three facets of one seam:

• Release-then-pin is not atomic. If the pin write fails or the controller restarts after the placeholder is deleted, associatedTo is already set to the live Service, so the hold is not re-armed and the address stays unheld until the first successful pin, not merely "bounded by reconcile latency" as decision 0001 states.
• Detaching from a live Service is not clean. unpin clears associatedTo and removes the pin, but MetalLB keeps the IP on the live Service (sticky, confirmed against MetalLB controller/service.go and the docs), so the next detectConflicts drives that address to Conflict. A routine detach cannot settle without recreating the Service.
• Conflict is detected after assignment, not prevented (detectConflicts keys on status.loadBalancer.ingress). For the impersonation case that is the stated design; for the two cases above it means a real-traffic window.

None of these is fatal on its own, but together the association seam carries most of the design's risk. It may be worth treating as one unit rather than guarding case by case.

[MEDIUM] Auto-claim has no structural guard against the driver's own holders

The core's wantsAutoClaim has no namespace or placeholder-label exclusion and no predicate, and the manager cache is cluster-wide, so it sees the driver's placeholder Services. Protection rests on the opt-out annotation (metallb-iad#5) plus the status.loadBalancer.ingress == 0 race. The driver's own association reconciler already skips placeholders structurally by namespace and label; the same guard in wantsAutoClaim would make the invariant independent of deploy order and of whether #5 is merged (both #5 and #7 are still open). Related: the single-default-class and single-auto-claim-class rules are enforced only by a runtime event, not admission or CRD validation, so two of either fails silently, two defaults leave every pending claim stuck, two auto-claim classes turn auto-claim off.

[MEDIUM] Controller-side scaling

A cluster-wide Service informer runs regardless of whether auto-claim is enabled anywhere, and several hot paths do unindexed full lists: withdrawAssociations lists every IPAddress on every Service deletion, missingFamilies/bindAvailableAddress list every IPAddress per claim reconcile, and all unbound claims are re-queued on any IPAddress or class event. Fine at hundreds of objects, degrades around 10k+. The placeholder ClusterIP cost is a related ceiling: one ClusterIP per reserved address ties reservation capacity to the host Service CIDR (a /24 is 254, a /16 is 65k), not to the external pool. The decision record notes the placeholder is "cheap but not free" without naming that ceiling.

Smaller items

• A Lost address stays in claimRef and in claim.status.addresses and accrues; nothing clears a fromClass Lost ledger entry.
• No entitlement object: a tenant may name any className, and quota bounds count, not source. (Acknowledged as deferred.)
• releaseDelay is unimplemented, so Released addresses accumulate under Retain until an admin clears claimRef. (Acknowledged.)
• The driver ClusterRole could scope services: create/delete to the placeholder namespace and keep only update/patch cluster-wide.
• The BGP path is not discussed: prefix aggregation and route-table growth under scattered reservations.

What looks right

Conflict is recoverable rather than terminal, allocation is idempotent through deterministic placeholder names so the driver and MetalLB cannot split-brain, claimRef UID matching drops stale bindings, address immutability and the union source are in place, and the finalizer plus namespace-deletion handling mirror PVC protection correctly.

[Timofei Larkin (lllamnyp)]

IvanHunters, thank you for reading the code beside the document. Each finding, and where it landed, as of a07dd3e.

Critical, association without a serviceability check. Confirmed in the driver. The document shares the blame: §5 specified the loadBalancerClass check as a refusal but said nothing about ordering, so a driver that checks after the handoff was compliant. §5 now requires the check before the hold is released: type: LoadBalancer, a matching loadBalancerClass, and a backend that will act on the pin. A consumer that fails it is refused with an event and associatedTo stays unset. The driver follows this thread.

No admission layer as shipped. Correct, and a wording error on my side: "ships" described an open PR. Implementation status and Rollout now say the chart on main has no policy and layer 1 is absent as shipped. metallb-iad#6 is one way to provide it, and optional: a distribution can package its own admission layer, for example in Cozystack's wrapper chart, which is what cozystack#3382 does.

The handoff. The window lasts until the first successful pin, not one reconcile interval; §6 and record 0001 say so now. Detach from a live Service ending in Conflict, and detection after assignment, were already in §5 and record 0001 and stay as the stated design.

Auto-claim guards. The core now skips any Service carrying a backend pool or pin input, which excludes the driver's placeholders without the opt-out annotation and without knowing the driver's namespace. Duplicate default or auto-claim classes are stated as a runtime condition, as Kubernetes does for duplicate default StorageClasses.

Scaling. The ClusterIP-per-placeholder ceiling is now in record 0001. The controller findings are filed against the implementation: cozystack/address-controller#8 (full ledger lists and the unbound-claim requeue), cozystack/address-controller#9 (the always-on Service informer in #7), cozystack/metallb-iad#7 (withdrawAssociations).

Smaller items. A Lost address keeps its claimRef until an admin acts, now in the failure cases. BGP has a sentence in §7: MetalLB announces host routes whether or not an address is reserved, so reservations do not change route-table growth. The RBAC scoping is a driver change.

Would you take another look?

[IvanHunters]

Re-review of a07dd3e

The previous round is fully addressed, and well. For the record: §5 now requires the serviceability and loadBalancerClass checks before the hold is released and spells out why the order matters; Implementation status and Security now state plainly that the chart on main ships no admission policy, so layer 1 is absent as shipped; decision 0001 corrects the handoff window to "unheld until the first successful pin" and records the Service-CIDR ceiling on reserved addresses; the controller-side scaling items are filed as cozystack/address-controller#8, cozystack/address-controller#9, and cozystack/metallb-iad#7; and the §6 auto-claim guard now excludes any Service carrying a backend pool or pin input, which covers the driver's placeholders without depending on the opt-out annotation or the driver's namespace. That last one is a better guard than I suggested.

A fresh read of the rewritten revision turned up a few new things, most of them introduced by the prose reflow.

[HIGH] The banner claims phase 1 is implemented; the document's own status says it is not

The opening note states "phase 1 of Rollout is implemented." Rollout defines phase 1 as "CRDs, core controller, the MetalLB provisioner, and the admission policy," and adds that "the §5 loadBalancerClass and serviceability checks and the held-before-minted rule of §6 complete this phase." But that same line says the policy "is in an open PR," and the §5 checks and held-before-minted sit in the "Specified here and not yet implemented" list. By the document's own definition phase 1 is not complete. This is the first line a reader sees, and it overstates maturity of exactly the pieces this review was about. Suggest "phase 1 partially implemented" with the gap named, or narrow the banner to the core controllers.

[HIGH] §4 presents the capability contract as modelled and enforced; elsewhere the document says it is neither

§4 reads "Each provisioner declares capabilities, and the core refuses what a backend cannot do," with a table of hard consequences. The banner says the opposite, "the provisioner contract's own API (capabilities are not modelled)," Failure and edge cases says "There is no capability registry for admission to consult," and there are no capability types in api/v1alpha1. Two issues: the capability contract does not exist yet, so §4 should read as the intended shape rather than a live mechanism, and the refusal is the provisioner's, not the core's (the core has no registry to consult). Please reconcile §4 with the status lists.

[MEDIUM] Auto-claim and the new serviceability check exclude each other for a class with a non-empty loadBalancerClass

Auto-claim fires only for a Service with no loadBalancerClass (§6, and wantsAutoClaim requires svc.Spec.LoadBalancerClass == nil). The §5 serviceability check requires the consuming Service's loadBalancerClass to match the class, and the field is immutable and set at creation. cloud-provider-kubevirt, the motivating auto-claim producer, renders the host Service without knowing the class, so it cannot set that field. The result: if an auto-claim class has a non-empty loadBalancerClass, every auto-claimed Service fails serviceability and the address is never announced. Auto-claim is therefore only consistent with a class whose loadBalancerClass is empty (the cluster default), and nothing in the document states that constraint or enforces it. This gap is a consequence of adding the serviceability check, so it is worth resolving in the same pass: either state that an auto-claim class must use the default loadBalancerClass, or have auto-claim stamp the class's value onto the minted Service.

[MINOR] Status lists disagree with the prose on the Lost claim phase and claim conditions

"In the code and not in this text" lists "a Lost claim phase," but Failure and edge cases uses it twice ("The claim is not Lost", "The claim reports Lost"), and the enum is in api/v1alpha1/ipaddressclaim_types.go. Separately, "claim conditions" are in the "in the code, not in the text" list while the Associated condition on a claim is in the "specified, not yet implemented" list and is also described in §5. Worth splitting "the conditions field exists" from "the specific Associated condition is not implemented," and dropping Lost from the code-only list.

[NIT] Decision 0001 date

0001 is dated 2026-07-20, but it cites the review of 2026-07-23 as motivation and is marked implemented in metallb-iad#1 "(merged 2026-07-23)." The date predates both.

One thing to confirm rather than a finding: the §1.2 table marks PureLB "none found" for a pin mechanism, and Open question 2 leans on that to say it "cannot be a backend." PureLB has historically honoured spec.loadBalancerIP; worth a check against PureLB source before that conclusion stands, though the open question already flags it.

The design itself reads sound and the specified-versus-implemented split is otherwise careful. The blocking items here are the two overclaims and the auto-claim interaction; the rest is cleanup.

[Timofei Larkin (lllamnyp)]

IvanHunters, as of 759dd6c.

The banner now says the controllers of phase 1 exist and are pending review; Implementation status lists what phase 1 still lacks.

§4 is rewritten as what exists and what is intended. As implemented the contract is the provisioner name stamped onto claims and the source union on the address; nothing declares capabilities, and a driver that cannot serve a claim leaves it Pending. The table stays as the intended shape, with the refusal the driver's.

On auto-claim and loadBalancerClass: the minted Service keeps a nil field, and nothing can set it afterwards, since the field is immutable once the Service is type: LoadBalancer and MetalLB reads no annotation in its place (its filter checks spec.loadBalancerClass only). So an unclassed Service is served by the default implementation, and an auto-claim class is a class of that implementation. §6 states this as the field's semantics rather than a rule of the design, reports an event on a class marked for auto-claim with a non-empty value, and defers auto-claim into another implementation to a mutating admission webhook that sets the field at creation.

The status lists no longer contradict the text: Lost is a claim phase the text uses, and the claim's conditions field is code-only while the Associated condition remains specified. Record 0001 keeps its date, which is the commit's, and says lexfrei's review came three days later and made the same argument independently.

PureLB, checked: it honours spec.loadBalancerIP and has no annotation (purelb#132). The §1.2 table records that and the open question is gone.

[IvanHunters]

Re-review of 759dd6c

The round-2 items are resolved. The banner no longer claims phase 1 is implemented and now matches Rollout. §4 separates the implemented contract (the provisioner-name stamp and the source union, with no capability registry) from the intended capability shape, and the refusal is the driver's. §6 states that an auto-claim class is a class of the default implementation and reports an event when its loadBalancerClass is non-empty, with cross-implementation auto-claim deferred to a mutating webhook. The status lists and the prose now agree on the Lost claim phase and on claim conditions. PureLB is corrected to spec.loadBalancerIP and its open question is dropped.

What remains is genuinely open: the entitlement object, in-tree versus out-of-tree provisioners, Gateway, and the claim-to-consumer cardinality. The specified-versus-implemented split reads honestly throughout.

Approving the design. The implementation-level items from the earlier round are tracked separately in cozystack/address-controller#8, cozystack/address-controller#9, and cozystack/metallb-iad#7.

[Aleksei Sviridkin (lexfrei)]

Timofei Larkin (@lllamnyp) LGTM for the design. My previous blocking points are addressed, and the document separates the intended contract from what is implemented.