From 6070d7084925a4e70683b2d4139aeb357a9b9e82 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Tue, 22 Sep 2026 21:29:30 -0500 Subject: [PATCH] fix: record the class a claim resolved to, and stop guessing its family MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A claim may name a family instead of a class, and NSO files every primary interface address that way. The resolved class was written to the IPAllocation but never to the claim, so the object never recorded what it allocated under — and the default for a family can be repointed, making the answer unrecoverable. The claim is persisted after resolution, so spec.className is now defaulted from the resolved class the same way a PersistentVolumeClaim is defaulted to the cluster's StorageClass. It is already immutable, so it cannot drift. spec.ipFamily selects a default class; it was also silently ignored whenever a claim named one, so a claim could ask for IPv4, get IPv6, and see no error. A family that disagrees with the class is now refused. Agreement stays legal: the fabric-identity controller sets both fields on every claim it files. Prefix-length validation assumed IPv4 when the claim stated no family, so a /64 from an IPv6 class was rejected with "must be between 1 and 32" — the common shape under the class model. Validation runs before the class is resolved and cannot read it, so it now applies only the bound that holds for every family; EffectivePrefixLength already applies the family-specific one against the class. --- internal/registry/ipam/ipclaim/storage.go | 19 +++++ .../ipam/ipclaim/storage_postgres_test.go | 81 +++++++++++++++++++ internal/registry/ipam/ipclaim/strategy.go | 16 ++-- .../registry/ipam/ipclaim/strategy_test.go | 59 ++++++++++++++ pkg/ipamerrors/errors.go | 6 ++ 5 files changed, 172 insertions(+), 9 deletions(-) diff --git a/internal/registry/ipam/ipclaim/storage.go b/internal/registry/ipam/ipclaim/storage.go index 51822a5..b659d98 100644 --- a/internal/registry/ipam/ipclaim/storage.go +++ b/internal/registry/ipam/ipclaim/storage.go @@ -277,6 +277,25 @@ func (r *AllocatingREST) Create(ctx context.Context, obj runtime.Object, createV return nil, fmt.Errorf("resolve class: %w", err) } + // spec.ipFamily is a selector for the default class, never a second opinion + // about the class a claim names. Silently ignoring a disagreement let a + // claim ask for IPv4 from an IPv6 class and get IPv6 back with no error. + if claim.Spec.IPFamily != "" && string(claim.Spec.IPFamily) != string(class.Spec.IPFamily) { + _ = tx.Rollback(ctx) + metrics.RecordAllocationFailure("ipclaim", "invalid", ipFamily, project, org) + failSpan(tracing.ReasonPoolNotFound) + return nil, ipamerrors.New(ipamerrors.ReasonFamilyMismatch, fmt.Sprintf( + "spec.ipFamily is %s but class %q hands out %s", + claim.Spec.IPFamily, class.Name, class.Spec.IPFamily)) + } + + // Default the class the same way a PersistentVolumeClaim is defaulted to + // the cluster's StorageClass: the claim is persisted below, so the stored + // object and the create response both name the class this resolved to + // rather than leaving the choice implicit. spec.className is immutable, so + // what is written here is what the claim allocated under, for good. + claim.Spec.ClassName = class.Name + if claim.Spec.Target == ipam.TargetScopeRange { // The chain is provisioned outside this transaction, level by level, // exactly as it is for a block claim — so this one is released first. diff --git a/internal/registry/ipam/ipclaim/storage_postgres_test.go b/internal/registry/ipam/ipclaim/storage_postgres_test.go index eeb2f52..9a3bfd2 100644 --- a/internal/registry/ipam/ipclaim/storage_postgres_test.go +++ b/internal/registry/ipam/ipclaim/storage_postgres_test.go @@ -266,3 +266,84 @@ func TestCreatingTheSameClaimNameTwiceConflicts(t *testing.T) { t.Errorf("conflict message does not name the claim: %v", err) } } + +// markClassDefault annotates the seeded class as the default for its family, so +// a claim naming only a family resolves to it. +func markClassDefault(t *testing.T, db *pgxpool.Pool) { + t.Helper() + key := tenant.Identity{Name: testProject}.ResourceKey("ipclasses", "standard") + if _, err := db.Exec(context.Background(), + `UPDATE ipam_objects + SET data = convert_to(jsonb_set(ipam_data_to_jsonb(data), '{metadata,annotations}', $2::jsonb)::text, 'UTF8') + WHERE key = $1`, + key, `{"`+ipamv1alpha1.IsDefaultClassAnnotation+`":"true"}`); err != nil { + t.Fatalf("mark class default: %v", err) + } +} + +// A claim that names only a family is resolved against the default class, and +// the class it resolved to is written into the claim. Leaving it implicit meant +// the object never recorded what it allocated under: the default for a family +// can be repointed, so the answer is not recoverable afterwards. +func TestCreateDefaultsTheResolvedClassIntoSpec(t *testing.T) { + r, db := newPostgresREST(t) + markClassDefault(t, db) + + claim := &ipam.IPClaim{ + ObjectMeta: metav1.ObjectMeta{Name: "family-only", Namespace: "default"}, + Spec: ipam.IPClaimSpec{IPFamily: ipam.IPv4}, + } + obj, err := r.Create(claimCtx(testProject), claim, nil, &metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Create: %v", err) + } + if got := obj.(*ipam.IPClaim).Spec.ClassName; got != "standard" { + t.Errorf("returned spec.className = %q, want the class it resolved to", got) + } + + var stored string + if err := db.QueryRow(context.Background(), + `SELECT ipam_data_to_jsonb(data)->'spec'->>'className' FROM ipam_objects + WHERE kind='IPClaim' AND name='family-only'`).Scan(&stored); err != nil { + t.Fatalf("read stored claim: %v", err) + } + if stored != "standard" { + t.Errorf("stored spec.className = %q, want standard: the choice must outlive the request", stored) + } +} + +// A claim may state a family as a selector, but not as a second opinion about a +// class it names. This used to be ignored: the claim asked for IPv4, the IPv6 +// class answered, and nothing said so. +func TestCreateRefusesAFamilyTheClassDoesNotHandOut(t *testing.T) { + r, _ := newPostgresREST(t) + + claim := &ipam.IPClaim{ + ObjectMeta: metav1.ObjectMeta{Name: "mismatched", Namespace: "default"}, + Spec: ipam.IPClaimSpec{ClassName: "standard", IPFamily: ipam.IPv6}, + } + _, err := r.Create(claimCtx(testProject), claim, nil, &metav1.CreateOptions{}) + if err == nil { + t.Fatal("Create succeeded; a claim asking for IPv6 from an IPv4 class must be refused") + } + if !apierrors.IsBadRequest(err) { + t.Errorf("error = %v, want a bad request", err) + } + if !strings.Contains(err.Error(), "IPv6") || !strings.Contains(err.Error(), "standard") { + t.Errorf("error %q should name both the family asked for and the class", err) + } +} + +// Agreeing is fine, and has to stay fine: the fabric-identity controller sets +// both fields on every claim it files. +func TestCreateAcceptsAFamilyThatAgreesWithTheClass(t *testing.T) { + r, _ := newPostgresREST(t) + + claim := &ipam.IPClaim{ + ObjectMeta: metav1.ObjectMeta{Name: "agreeing", Namespace: "default"}, + Spec: ipam.IPClaimSpec{ClassName: "standard", IPFamily: ipam.IPv4}, + } + if _, err := r.Create(claimCtx(testProject), claim, nil, &metav1.CreateOptions{}); err != nil { + t.Fatalf("Create: %v", err) + } +} diff --git a/internal/registry/ipam/ipclaim/strategy.go b/internal/registry/ipam/ipclaim/strategy.go index 64e817c..c8bdef0 100644 --- a/internal/registry/ipam/ipclaim/strategy.go +++ b/internal/registry/ipam/ipclaim/strategy.go @@ -121,15 +121,13 @@ func validateIPClaim(c *ipam.IPClaim) field.ErrorList { allErrs = append(allErrs, field.NotSupported(specPath.Child("ipFamily"), c.Spec.IPFamily, []string{string(ipam.IPv4), string(ipam.IPv6)})) } - if p := c.Spec.PrefixLength; p != nil { - maxLen := int32(32) - if c.Spec.IPFamily == ipam.IPv6 { - maxLen = 128 - } - if *p <= 0 || *p > maxLen { - allErrs = append(allErrs, field.Invalid(specPath.Child("prefixLength"), *p, - fmt.Sprintf("must be between 1 and %d", maxLen))) - } + // Only the bound that holds for every family. The family-specific one comes + // from the class, in EffectivePrefixLength: this runs before the class is + // resolved and cannot read it, and assuming IPv4 here rejected a /64 from + // an IPv6 class whenever the claim named the class instead of the family. + if p := c.Spec.PrefixLength; p != nil && (*p <= 0 || *p > 128) { + allErrs = append(allErrs, field.Invalid(specPath.Child("prefixLength"), *p, + "must be between 1 and 128")) } switch c.Spec.Target { case "", ipam.TargetBlock: diff --git a/internal/registry/ipam/ipclaim/strategy_test.go b/internal/registry/ipam/ipclaim/strategy_test.go index 938cadb..b0e0211 100644 --- a/internal/registry/ipam/ipclaim/strategy_test.go +++ b/internal/registry/ipam/ipclaim/strategy_test.go @@ -5,6 +5,7 @@ import ( "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/validation/field" "go.miloapis.com/ipam/pkg/apis/ipam" ) @@ -80,3 +81,61 @@ func TestAScopeRefNeedsAKindAndAName(t *testing.T) { }) } } + +// Validation runs before the class is resolved, so it cannot know the family. +// Assuming IPv4 rejected a /64 from an IPv6 class whenever the claim named the +// class rather than the family — which is the form the class model encourages. +// Only the bound that holds for every family belongs here; the family-specific +// one is applied against the resolved class in EffectivePrefixLength. +func TestValidatePrefixLengthDoesNotAssumeIPv4(t *testing.T) { + prefix := func(n int32) *int32 { return &n } + + for _, tc := range []struct { + name string + claim *ipam.IPClaim + wantErr bool + }{ + { + name: "IPv6-sized prefix on a claim that names only its class", + claim: &ipam.IPClaim{Spec: ipam.IPClaimSpec{ + ClassName: "datum-subnet-ipv6", PrefixLength: prefix(64), + }}, + }, + { + name: "a /96 endpoint prefix, likewise", + claim: &ipam.IPClaim{Spec: ipam.IPClaimSpec{ + ClassName: "datum-endpoint-ipv6", PrefixLength: prefix(96), + }}, + }, + { + name: "beyond any address family", + claim: &ipam.IPClaim{Spec: ipam.IPClaimSpec{ + ClassName: "c", PrefixLength: prefix(129), + }}, + wantErr: true, + }, + { + name: "zero is not a prefix length", + claim: &ipam.IPClaim{Spec: ipam.IPClaimSpec{ + ClassName: "c", PrefixLength: prefix(0), + }}, + wantErr: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + errs := validateIPClaim(tc.claim) + var got *field.Error + for _, e := range errs { + if e.Field == "spec.prefixLength" { + got = e + } + } + if tc.wantErr && got == nil { + t.Errorf("no prefixLength error, want one") + } + if !tc.wantErr && got != nil { + t.Errorf("prefixLength rejected: %v", got) + } + }) + } +} diff --git a/pkg/ipamerrors/errors.go b/pkg/ipamerrors/errors.go index 4cb1d96..4f6cf58 100644 --- a/pkg/ipamerrors/errors.go +++ b/pkg/ipamerrors/errors.go @@ -62,6 +62,11 @@ const ( // stated one. ReasonPrefixLengthRejected Reason = "PrefixLengthRejected" + // ReasonFamilyMismatch reports that the claim stated an address family the + // class it names does not hand out. spec.ipFamily selects a default class + // when no class is named; it never overrides one that is. + ReasonFamilyMismatch Reason = "FamilyMismatch" + // ReasonScopeRolesMissing reports that the claim's scope did not carry // roles the class requires. MissingScopeRoles names them. ReasonScopeRolesMissing Reason = "ScopeRolesMissing" @@ -225,6 +230,7 @@ var knownReasons = map[Reason]bool{ ReasonNoDefaultClass: true, ReasonNoOfferingPool: true, ReasonPrefixLengthRejected: true, + ReasonFamilyMismatch: true, ReasonScopeRolesMissing: true, ReasonAllocationRetained: true, ReasonClaimExists: true,