[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

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

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: 80356641-7fb9-4cd5-a915-16420ececddf

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• Trigger review

• 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.