[Timofei Larkin (lllamnyp)]

The ip-address-claims proposal's Security section says spec.addressName "cannot be checked in CEL, because resolving an address to its class needs a second object read. A deployment either refuses the field to tenant principals, which is the reference policy's choice, or checks it in a webhook." Neither address-controller nor metallb-iad shipped that refusal. A review of cozystack/cozystack#3382 also noted that any tenant can take any Available address through the field. Stacked on #12.

Changes

• Policy. templates/admission-policy.yaml installs address-controller-claim-address-name, a ValidatingAdmissionPolicy and binding, when admissionPolicy.enabled is set (default off).

  • On claim CREATE/UPDATE, it refuses any non-allowlisted principal that sets, changes or removes spec.addressName.
  • It compares the old value with the new one, so writes that leave the field unchanged pass.
  • failurePolicy: Fail.

• Values. validationActions (default [Deny]), namespaceSelector (default all namespaces), allowedUsernames, and allowedGroups (default system:masters and kubeadm:cluster-admins, because admission policies bind system:masters too). In brokered mode, machinery that moves addresses between namespaces through spec.addressName must be listed.

• Test. test/admission/policy_test.go starts envtest with the chart's CRDs and renders the policy with helm template. It then checks, as server-side dry runs under impersonated identities:

  • refused: a tenant creating a claim that names an address, or changing or removing the field;
  • admitted: a plain claim, an edit that leaves the field alone, a namespace outside the binding, allowlisted machinery, and an admin.

  make test already provides envtest assets. With the rule replaced by true, the test fails.

• README. A new Admission policy section.

Not covered

Which classes a namespace may claim from at all. The proposal leaves that to an entitlement object, still open.

[coderabbitai]

Important

• Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 37f6a47a-6c45-47d2-988d-a857e8112fd2

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

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

[IvanHunters]

Review: optional admission policy reserving IPAddressClaim.spec.addressName

Conforms to the design, which says spec.addressName cannot be resolved in CEL and a deployment either refuses the field to tenant principals or checks it in a webhook; this ships the "refuse to tenants" option. The CEL is correct: the deny predicate is newAddressName == oldAddressName (chart/address-controller/templates/admission-policy.yaml:37), the oldObject access is guarded by a request.operation == "UPDATE" short-circuit so there is no null dereference on CREATE, both CREATE and UPDATE are matched (a tenant setting the field at create is denied because oldAddressName is empty), failurePolicy: Fail is fail-closed, and the groups match is has()-guarded.

Two things done right that are worth calling out: the controller's ServiceAccount is deliberately NOT allowlisted, which is correct because the controller only reads claim.Spec.AddressName and never writes it (unlike the metallb-iad driver, which writes the gated Service fields and so must self-allowlist); and spec.className is left ungated, matching the design's decision to defer class entitlement to a separate object above this component.

[MINOR] Default namespaceSelector: {} applies cluster-wide

With enabled: true and the defaults the policy matches every namespace. This fails safe (more gating, not less), the values and README say to scope it, and it matches the sibling default, so it is a note rather than a defect.

LGTM.

[IvanHunters]

Re-approving. The PR's own changes are unchanged since my review of 758596f; the head moved only because sibling PRs merged into the base. The admission policy at the new head is identical: it gates spec.addressName for non-allowlisted principals, guards oldObject on CREATE, correctly does not self-allowlist the controller SA, and leaves className to the entitlement object. The namespaceSelector: {} default note stands as non-blocking.

One merge-ordering note, not a review objection: this PR is based on feat/inventory-metrics (#12), which still carries my open request-changes, so it wants a resolved base before it lands. LGTM on its own content.

The merge-base changed after approval.

[IvanHunters]

Re-approving. Unchanged since my review; the head moved only because the sibling PRs merged into the base (the admission policy at the new head is byte-identical to the approved one). The namespaceSelector: {} default note stands as non-blocking, and the merge-ordering note holds: this sits on the metrics branch (#12), so it wants a resolved base before it lands. LGTM.