From 6070d7084925a4e70683b2d4139aeb357a9b9e82 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Tue, 22 Sep 2026 21:29:30 -0500 Subject: [PATCH 1/2] 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, From b410019fa3a2c4d279e56b5ad94438de54069759 Mon Sep 17 00:00:00 2001 From: Scot Schuchert-Wells Date: Thu, 29 Oct 2026 22:19:18 +0000 Subject: [PATCH 2/2] fix: Keep the class name a claim looked up A claim that resolves its class through a reference was stored with the definition's name from the source project. In the claim's own project that name is missing or names another class, and spec.className is immutable, so every later lookup of the claim's class from its project broke. Key changes: - record the name looked up in the caller's project on ResolvedClass - store that name on the claim and use it in the family mismatch error - add tests for a named reference and a default reference --- internal/allocator/class.go | 9 +- internal/registry/ipam/ipclaim/storage.go | 4 +- .../ipam/ipclaim/storage_postgres_test.go | 82 +++++++++++++++++++ 3 files changed, 91 insertions(+), 4 deletions(-) diff --git a/internal/allocator/class.go b/internal/allocator/class.go index 8e20b25..1d0c029 100644 --- a/internal/allocator/class.go +++ b/internal/allocator/class.go @@ -51,6 +51,11 @@ type ResolvedClass struct { // the consumer here would merge two same-named classes in two defining // projects onto one ipam_pool_identity row. Project string + + // LocalName is the name the caller looked up in its own project. It differs + // from the definition's Name when that lookup went through a reference, and + // it is the only name that resolves again from the caller's project. + LocalName string } func classStorageKey(project, name string) string { @@ -105,7 +110,7 @@ func resolveClassIn(ctx context.Context, tx pgx.Tx, project, name string) (*Reso return nil, err } if class.Spec.Source == nil { - return &ResolvedClass{IPClass: class, Project: project}, nil + return &ResolvedClass{IPClass: class, Project: project, LocalName: name}, nil } src := *class.Spec.Source @@ -117,7 +122,7 @@ func resolveClassIn(ctx context.Context, tx pgx.Tx, project, name string) (*Reso return nil, fmt.Errorf("%w: %q in project %q names %q in project %q", ErrChainedReference, name, project, src.Name, src.Project) } - return &ResolvedClass{IPClass: target, Project: src.Project}, nil + return &ResolvedClass{IPClass: target, Project: src.Project, LocalName: name}, nil } // LoadClass reads the class a caller named, from the caller's own project. diff --git a/internal/registry/ipam/ipclaim/storage.go b/internal/registry/ipam/ipclaim/storage.go index b659d98..915d311 100644 --- a/internal/registry/ipam/ipclaim/storage.go +++ b/internal/registry/ipam/ipclaim/storage.go @@ -286,7 +286,7 @@ func (r *AllocatingREST) Create(ctx context.Context, obj runtime.Object, createV 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)) + claim.Spec.IPFamily, class.LocalName, class.Spec.IPFamily)) } // Default the class the same way a PersistentVolumeClaim is defaulted to @@ -294,7 +294,7 @@ func (r *AllocatingREST) Create(ctx context.Context, obj runtime.Object, createV // 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 + claim.Spec.ClassName = class.LocalName if claim.Spec.Target == ipam.TargetScopeRange { // The chain is provisioned outside this transaction, level by level, diff --git a/internal/registry/ipam/ipclaim/storage_postgres_test.go b/internal/registry/ipam/ipclaim/storage_postgres_test.go index 9a3bfd2..e88268d 100644 --- a/internal/registry/ipam/ipclaim/storage_postgres_test.go +++ b/internal/registry/ipam/ipclaim/storage_postgres_test.go @@ -347,3 +347,85 @@ func TestCreateAcceptsAFamilyThatAgreesWithTheClass(t *testing.T) { t.Fatalf("Create: %v", err) } } + +// seedReference adds a class named name to project that points at the seeded +// "standard" definition in testProject, optionally as the project's default. +func seedReference(t *testing.T, db *pgxpool.Pool, project, name string, isDefault bool) { + t.Helper() + class := &ipamv1alpha1.IPClass{ + TypeMeta: metav1.TypeMeta{APIVersion: "ipam.miloapis.com/v1alpha1", Kind: "IPClass"}, + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: ipamv1alpha1.IPClassSpec{ + Source: &ipamv1alpha1.ClassSourceRef{Project: testProject, Name: "standard"}, + }, + } + if isDefault { + class.Annotations = map[string]string{ipamv1alpha1.IsDefaultClassAnnotation: "true"} + } + data, err := json.Marshal(class) + if err != nil { + t.Fatalf("marshal reference: %v", err) + } + if _, err := db.Exec(context.Background(), + `INSERT INTO ipam_objects (key, kind, name, data) VALUES ($1,'IPClass',$2,$3)`, + tenant.Identity{Name: project}.ResourceKey("ipclasses", name), name, data); err != nil { + t.Fatalf("seed reference in %s: %v", project, err) + } +} + +func storedClassName(t *testing.T, db *pgxpool.Pool, claim string) string { + t.Helper() + 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=$1`, claim).Scan(&stored); err != nil { + t.Fatalf("read stored claim: %v", err) + } + return stored +} + +// A claim that reaches a class through a reference keeps the reference's name. +// The definition's name belongs to the source project: in the claim's own +// project it names nothing, or another class, and spec.className is immutable. +func TestCreateKeepsTheReferenceNameAClaimNamed(t *testing.T) { + r, db := newPostgresREST(t) + const consumer = "consumer-project" + seedReference(t, db, consumer, "shared", false) + + claim := &ipam.IPClaim{ + ObjectMeta: metav1.ObjectMeta{Name: "through-reference", Namespace: "default"}, + Spec: ipam.IPClaimSpec{ClassName: "shared"}, + } + obj, err := r.Create(claimCtx(consumer), claim, nil, &metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Create: %v", err) + } + if got := obj.(*ipam.IPClaim).Spec.ClassName; got != "shared" { + t.Errorf("returned spec.className = %q, want shared", got) + } + if got := storedClassName(t, db, "through-reference"); got != "shared" { + t.Errorf("stored spec.className = %q, want shared", got) + } +} + +// The same when the reference is the consumer project's default for the family. +func TestCreateDefaultsTheReferenceNameIntoSpec(t *testing.T) { + r, db := newPostgresREST(t) + const consumer = "consumer-project" + seedReference(t, db, consumer, "shared", true) + + claim := &ipam.IPClaim{ + ObjectMeta: metav1.ObjectMeta{Name: "default-reference", Namespace: "default"}, + Spec: ipam.IPClaimSpec{IPFamily: ipam.IPv4}, + } + obj, err := r.Create(claimCtx(consumer), claim, nil, &metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Create: %v", err) + } + if got := obj.(*ipam.IPClaim).Spec.ClassName; got != "shared" { + t.Errorf("returned spec.className = %q, want shared", got) + } + if got := storedClassName(t, db, "default-reference"); got != "shared" { + t.Errorf("stored spec.className = %q, want shared", got) + } +}