feat(api)!: make IPClaim ipFamily optional + fix IPv6 family handling#69
Draft
scotwells wants to merge 5 commits into
Draft
feat(api)!: make IPClaim ipFamily optional + fix IPv6 family handling#69scotwells wants to merge 5 commits into
scotwells wants to merge 5 commits into
Conversation
Claiming a prefix from an IPv6 *child* pool without --family failed with a
misleading error:
$ datumctl ipam prefix claim --pool region-v6 --length 64
Error: --length /64 is out of range for IPv4 (max /32)
Child pools carry no spec.ipFamily — a child's family is derived from the
CIDR carved out of its parent and reported in status.ipFamily. The claim
command inferred the family from spec.ipFamily only, got empty, and
defaulted to IPv4, so every IPv6 length (/64, /128, …) looked out of range.
Infer the family from the pool's effective family via poolFamily(), which
prefers status.ipFamily and falls back to spec.ipFamily — the same helper
the pool list/show output already uses. Claims from IPv6 child pools now
work without --family.
Also, when the family was assumed (no --family/--cidr and none resolvable),
the out-of-range error now points the user at --family instead of just
asserting IPv4.
Adds a regression test for claiming a /64 from a child pool whose family
lives only in status.
The allocation path trusted the claim's spec.ipFamily but never checked it against the pool. The family was only recorded on the allocation and used for metrics; the block itself was chosen by prefix length alone. So a claim's family and the pool's family could silently diverge: - A family-mismatched claim returned a misleading 507 "pool exhausted" (when no block of that length fit) instead of a clear error. - Worse, an IPv6-family claim with a length that fits an IPv4 pool (e.g. /24 against 10.0.0.0/8) would allocate a real IPv4 block and persist an IPAllocation labeled IPv6 — a record whose family disagrees with its CIDR. Derive the pool's authoritative family from its CIDR (effectivePoolFamily) and reject a mismatched claim with ErrFamilyMismatch, mapped to HTTP 400 at the registry boundary. This protects allocation integrity for every client (CLI, kubectl, controllers), independent of the CLI-side family inference. - internal/allocator: add ErrFamilyMismatch; validate in AllocatePrefix before searching for a block. - internal/registry/ipam/ipclaim: map ErrFamilyMismatch to 400 BadRequest and a family_mismatch failure reason; unit test for the mapping. - test/e2e/claim-family-mismatch: chainsaw suite covering both mismatch directions (rejected) and a matching-family claim (still binds).
scotwells
changed the title
…om the pool BREAKING: spec.ipFamily on IPClaim is no longer required. The pool a claim targets fully determines the address family, so requiring the client to also state it (and keep it in sync) was redundant and error-prone. - The server derives the family from the resolved pool's CIDR and defaults it onto the claim, so the stored IPClaim and its IPAllocation always report a concrete, correct family — even when the client omits it. - When the client DOES send a family, it is validated against the pool and rejected on mismatch (ErrFamilyMismatch -> 400), as before. - A prefix length out of range for the resolved family is rejected with a clear 400 (ErrPrefixLengthOutOfRange) rather than a misleading 507. Details: - pkg/apis/ipam/v1alpha1: ipFamily gains +optional/omitempty; OpenAPI regenerated so it drops out of IPClaimSpec required. - internal/allocator: AllocatePrefix derives-or-validates the family and returns the effective family so callers can record it. - internal/registry/ipam/ipclaim: default the derived family onto the claim; relax validation (ipFamily optional, width bound widened when omitted); ipFamily immutability now treats an omitted update as unchanged. - cmd/milo-ipam: stop defaulting the claim family to IPv4 — leave it unset when it can't be inferred locally (e.g. --selector) and let the server derive it; only range-check length client-side when the family is known. Tests: - unit: family mismatch -> 400; omitted family -> server defaults it and binds. - e2e (chainsaw claim-family-mismatch): rejects both mismatch directions, accepts a matching-family claim, and accepts an omitted-family claim that binds with the pool's derived family.
scotwells
changed the title
Audit and improve the errors users actually see — from the CLI and, for kubectl/API callers, from the apiserver — so each says plainly what went wrong and how to resolve it, without leaking internals. Server (reach kubectl + CLI): - Family mismatch and out-of-range length now read in plain language and no longer append the internal "ipam: ..." sentinel text. Added a small userError wrapper so errors.Is still matches the sentinel while Error() stays clean (with a regression test). - Pool exhausted / not found, missing or conflicting poolRef/poolSelector, and no-selector-match now explain the cause and the remedy (e.g. "release prefixes to free space, or request a smaller block"). - Same for the parent-pool cases on child IPPool creation. CLI (cmd/milo-ipam): - 403 no longer says "RBAC"; it explains the org/project + access angle and that an unshared pool reads as "permission denied", not "not found". - Unclassified failures become "something went wrong: …" with a --verbose hint instead of a bare Go error. - Invalid CIDR drops the netip parser internals (kept as --verbose cause) and shows the expected form; client-setup failure gains a login/kubeconfig fix. No behavioral change; messages only. Remedies are grounded in real, supported actions — nothing fabricated.
…uidance
Second pass over user-facing messages, going further where it helps:
- Cancelling a release now reassures nothing changed ("cancelled — the
prefix was not released") instead of a bare "aborted".
- A mismatched typed confirmation explains what happened and how to retry,
and the non-interactive destructive-action refusal reads in plain language.
- --child-pool (unsupported here) now tells the user exactly how to get the
same result: claim the prefix, then `pool create --parent`.
- Generic 409s explain the likely cause (name already exists / changed since
read) and how to proceed, instead of a bare "conflict:".
- Dropped a redundant "(requested length N)" from the exhaustion message.
Messages only; remedies are grounded in real commands.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Claiming IPv6 space now "just works," and a claim can no longer silently allocate the wrong address family.
What this delivers
ipFamilyis optional on IPClaim. The pool determines the family, so callers no longer specify it — the server derives and records it. (Breaking schema change; still accepts a family if sent.)Covered by unit and end-to-end (Chainsaw) tests.
Related