Repository navigation
Name an existing MetalLB pool instead of rendering one per class - #8
Conversation
A class now names an IPAddressPool in parameters.pool, which the admin owns with its advertisement; the driver no longer renders a pool and L2Advertisement from parameters.addresses. The class controller and the claim controller refuse a class whose pool is missing, auto-assigns, does not admit placeholders through serviceAllocation, or is named by another class; a refusal blocks only new allocation placeholders. Placeholders draw from the named pool and carry the --lb-class of the MetalLB instance the driver fronts, and an auto-claim class served by a non-default instance gets a warning. This follows decision 0003 of the ip-address-claims proposal. 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: name an existing MetalLB pool instead of rendering one (implements community#124)
The core of #124 is here and reads well. parameters.pool replaces parameters.addresses, the driver no longer renders the pool or the advertisement, and ParseClassParameters rejects the old addresses key with a migration message and requires a pool (internal/driver/driver.go). The four refusals are in classPool (internal/controller/pools.go:44-73): invalid parameters, pool missing, PoolProblem, and another class naming the same pool. classesNamingPool is provisioner-scoped, excludes the class itself, and sorts (pools.go:77-93). Treating an unset autoAssign as auto-assigning is correct, since MetalLB defaults it to true. --lb-class is wired to the placeholder's spec.loadBalancerClass and to the AutoClaimUnservable warning. Good.
Two problems, one of them a silent data-plane failure.
[MAJOR] A pool scoped by serviceAllocation passes the class check but the consumer never gets the address
PoolProblem checks the pool's serviceAllocation against the placeholder only: admitsNamespace(alloc, placeholderNS) and serviceSelectors against {PlaceholderLabel: MarkerValue} (internal/driver/driver.go, PoolProblem). The consumer Service must satisfy the same serviceAllocation for MetalLB to assign it the pinned address. I confirmed against MetalLB source that this is enforced on the explicit paths, not only on auto-assign: Assign (the pinned-IP path) and AllocateFromPool (the named-pool path, which calls Assign) both run isPoolCompatibleWithService, which checks ServiceAllocations.Namespaces and ServiceAllocations.ServiceSelectors. A Service that does not match gets AllocationFailed and no address.
Nothing makes the consumer match. associate writes only metallb.io/loadBalancerIPs and the pinned annotation on the consumer (internal/controller/service_controller.go), not a label, and serviceSelectors match labels, not annotations. So:
- A pool whose
serviceAllocation.namespaceslists the placeholder namespace but not the tenant's: the class is accepted (placeholder matches), the reservation holds, and the pin on the tenant Service is rejected by MetalLB. The address is reserved and announced nowhere, with the class reported healthy. - A pool with any
serviceSelectors: the consumer carries no matching label and no code path adds one, so it can never match.
This is exactly the defense-in-depth §8 recommends ("IPAddressPool.spec.serviceAllocation can restrict the pool to a controller-set label or namespace"). Following that advice silently breaks announcement. The consumer-side serviceability check of §5 is unimplemented, so nothing catches it. The "controller-set label" the design mentions has no implementation: the driver would need to stamp a label on associated Services for a serviceSelectors-scoped pool to work, and to validate the consumer's namespace, not the placeholder's, against serviceAllocation.namespaces. At minimum the check should validate the consumer, and a serviceSelectors-scoped pool should be refused outright until the label is stamped, rather than accepted and left silent.
[MEDIUM] status.loadBalancerClass is not reported, so #124's loadBalancerClass mechanism is unwired on this side
#124 has the provisioner "report it as status.loadBalancerClass on every class it serves," and §5 has a controller "read the class's status.loadBalancerClass first." The ClassReconciler writes no status at all (internal/controller/class_controller.go, Reconcile emits events only), and the imported API ([email protected], which predates #124) has no Status on IPAddressClass. So the value lives only on the driver's flag and on placeholder spec.loadBalancerClass; it is never surfaced where §5 reads it. This looks like staging behind a core API bump, which is reasonable, but it should be stated in the PR, and until it lands the §5 serviceability and loadBalancerClass checks cannot run on either side.
[LOW] The serviceAllocation matcher is a parallel reimplementation
admitsNamespace and matchesAnySelector (internal/driver/driver.go) re-derive MetalLB's namespace and selector matching. They look right for the current fields (empty admits all, namespaces OR namespaceSelectors, any serviceSelector matches), but a second implementation drifts from the backend over time, MetalLB's priority, or a future serviceAllocation field, would not be reflected. Worth a test against a real MetalLB config, or a note that the semantics are pinned to a MetalLB version.
The pool-reference model itself is a clear improvement and is implemented cleanly. The serviceAllocation consumer gap is the one to resolve before merge.
Assisted-By: LLM Signed-off-by: Timofei Larkin <[email protected]>
The class check vetted a pool's serviceAllocation for placeholders only, with a copy of MetalLB's matching, while consumer Services, which MetalLB checks against the same serviceAllocation, went unchecked. Both sides belong to the design's serviceability check, which this change does not implement. The class check now refuses only what decision 0003 lists: invalid parameters, a missing or auto-assigning pool, and a pool named by two classes. The docs state what serviceAllocation must admit. Assisted-By: LLM Signed-off-by: Timofei Larkin <[email protected]>
|
Thanks for the review. Changed in d9b11fc; per point: [MAJOR]
What did need fixing here was the placeholder-only [MEDIUM] [LOW] Reimplemented matcher. Removed with the check in d9b11fc. When serviceability brings it back, it will carry the MetalLB version it follows. Importing MetalLB's types instead was considered and rejected; design §3 gives the reason: they live in MetalLB's root module, whose requirements would set this module's Kubernetes and controller-runtime versions. |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Re-review of d9b11fc
The MAJOR is resolved the right way. The placeholder-only serviceAllocation check is gone, PoolProblem now refuses only an auto-assigning pool, so the class check matches decision 0003 exactly: invalid parameters, a missing pool, an auto-assigning pool, and a pool named by two classes. The consumer-side serviceAllocation requirement is now documented in the README and the design doc, attributed to the §5 serviceability check and marked not-yet-implemented, and the docs record that MetalLB enforces serviceAllocation on pinned Services too. That is the right scope: the false "class vetted" signal is gone and the remaining check sits where it belongs.
On status.loadBalancerClass: agreed, and my earlier framing was off. The driver has its value from --lb-class, so the driver-side §5 check does not need the status field; it is third parties creating Services that do, which is listed under Not in this PR with the core API field pending.
Checked the rest against the merged design and the code: the driver no longer creates or deletes a pool or advertisement, so an admin pool survives class deletion; claim_controller resolves the pool per claim and does not tear down an existing placeholder when the pool develops a problem; NewPlaceholder leaves spec.loadBalancerClass nil for the default implementation and sets it only for a non-empty --lb-class, both covered by tests; and the old parameters.addresses is refused with a migration hint. Tests pass.
LGTM.
The base branch was changed.
|
Its base PR merged, so this now targets main. Retargeting dismissed the approval; the content is unchanged since it was approved and it merges cleanly. Needs a re-approval. |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Re-approving. The branch is unchanged since my review of d9b11fc (same head), so that review stands: the pool-reference model is clean, the class check matches decision 0003, and the serviceAllocation consumer check is correctly deferred to the §5 serviceability work. LGTM.
Implements decision 0003 of the
ip-address-claimsproposal (cozystack/community#124): a class names an existing MetalLBIPAddressPoolinstead of listing CIDRs for the driver to render. Stacked on #5, which moves the module togithub.com/cozystack/address-controllerand provides the auto-claim annotation constants.Changes
Class parameters.
parameters.poolnames a pool in the MetalLB namespace.parameters.addressesis rejected by name, and unknown fields are rejected.No rendering. The class controller no longer creates
iad-<class>pools orL2Advertisements. The admin owns the pool and its advertisement. The ClusterRole drops write access to both kinds.Refusal. A class is refused for new reservations if:
autoAssignas true);The class gets a
PoolRefusedevent, and each claim that would be provisioned gets one too. A refusal blocks only new allocation placeholders: a placeholder MetalLB already assigned is still recorded, and existing addresses stay held. Claims and classes are re-reconciled on pool changes.--lb-class/loadBalancerClassvalue. This is the--lb-classof the MetalLB instance the driver fronts. Every placeholder carries it asspec.loadBalancerClass. A class markedis-auto-claim-classgets anAutoClaimUnservableevent when the value is non-empty, because auto-claimed Services carry none.Holds. A hold placeholder names the class's pool. If the class's parameters name none, it holds by the pin alone, which MetalLB honours from any pool that admits the Service.
Docs. Design §3 is rewritten, the "L2 only, CIDRs only" gap is removed, and the README gains the new class shape, a migration section, and steps for reserving part of a pool that auto-assigns today. The design doc records why MetalLB objects stay unstructured: its API types live in MetalLB's root module, whose requirements would set this module's Kubernetes and controller-runtime versions.
Migration
After upgrading, a class with
parameters.addressesgets no new reservations, and its existing addresses stay held. To migrate, remove the class's owner references from the renderediad-<class>pool and advertisement, then setparameters.pool: iad-<class>. The README has the commands. The owner references must go first: otherwise deleting the class later deletes the pool.Not in this PR
IPAddressClass.status.loadBalancerClass. The core API has no such field yet.serviceAllocation. MetalLB applies it to placeholders and to pinned consumer Services alike, so both sides go into that check together. Until then the docs state what a pool'sserviceAllocationmust admit.go test ./internal/...,golangci-lint runandhelm lintpass.