From 3757a4d61854619c387ab6e34267a2dbb6eda6a6 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Wed, 28 Oct 2026 17:11:30 -0500 Subject: [PATCH 1/4] fix: Resolve a claim's class before quota admission Quota admission ran before the class was resolved, so a claim that named only a class reached it with no ipFamily and any family-conditioned charge missed it. The class is now resolved first, its name and family are written onto the claim, and admission sees the filled object. Key changes: - resolve the class, refuse a family mismatch, and default className and ipFamily before createValidation runs - release the class-lookup transaction immediately instead of holding it - check namespace liveness and stamp ownerRef after admission, as before Co-Authored-By: Claude Opus 5.5 --- internal/registry/ipam/ipclaim/storage.go | 64 +++++++++---------- .../ipam/ipclaim/storage_postgres_test.go | 46 +++++++++++++ 2 files changed, 77 insertions(+), 33 deletions(-) diff --git a/internal/registry/ipam/ipclaim/storage.go b/internal/registry/ipam/ipclaim/storage.go index 915d311..fd9208a 100644 --- a/internal/registry/ipam/ipclaim/storage.go +++ b/internal/registry/ipam/ipclaim/storage.go @@ -218,35 +218,11 @@ func (r *AllocatingREST) Create(ctx context.Context, obj runtime.Object, createV metrics.RecordAllocationFailure("ipclaim", "internal", ipFamily, project, org) return nil, err } - if createValidation != nil { - if err := createValidation(ctx, claim.DeepCopyObject()); err != nil { - metrics.RecordAllocationFailure("ipclaim", "internal", ipFamily, project, org) - return nil, err - } - } - - // Before any capacity is reserved: a namespace that is gone, or going, - // never releases what is bound into it. - if err := r.checkNamespaceLiveness(ctx, project, claim.Namespace); err != nil { - metrics.RecordAllocationFailure("ipclaim", "namespace_not_live", ipFamily, project, org) - failSpan(tracing.ReasonNamespaceNotLive) - return nil, err - } - if claim.Spec.ClassName == "" && claim.Spec.IPFamily == "" { metrics.RecordAllocationFailure("ipclaim", "internal", ipFamily, project, org) return nil, apierrors.NewBadRequest("spec.className or spec.ipFamily is required") } - // Overwrite any client-supplied ownerRef. The requestheader CA guarantees - // the Extra headers carrying the tenant identity, so that identity is the - // source of truth for who this claim is for. - claim.Spec.OwnerRef = &ipam.ObjectRef{ - APIGroup: id.APIGroup, - Kind: id.Kind, - Name: id.Name, - } - tx, err := r.db.Begin(ctx) if err != nil { metrics.RecordAllocationFailure("ipclaim", "tx_error", ipFamily, project, org) @@ -259,8 +235,8 @@ func (r *AllocatingREST) Create(ctx context.Context, obj runtime.Object, createV // then finds the pool that project offers. Cross-project sharing is the // reference, so there is no separate cross-project path here. class, err := allocator.ResolveClass(ctx, tx, claim.Spec.ClassName, v1alpha1.IPFamily(claim.Spec.IPFamily)) + _ = tx.Rollback(ctx) if err != nil { - _ = tx.Rollback(ctx) metrics.RecordAllocationFailure("ipclaim", "class_not_found", ipFamily, project, org) failSpan(tracing.ReasonPoolNotFound) switch { @@ -281,7 +257,6 @@ func (r *AllocatingREST) Create(ctx context.Context, obj runtime.Object, createV // 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( @@ -295,11 +270,37 @@ func (r *AllocatingREST) Create(ctx context.Context, obj runtime.Object, createV // 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.LocalName + claim.Spec.IPFamily = ipam.IPFamily(class.Spec.IPFamily) + ipFamily = string(claim.Spec.IPFamily) + span.SetAttributes(attribute.String(tracing.AttrClaimIPFamily, ipFamily)) + + if createValidation != nil { + if err := createValidation(ctx, claim.DeepCopyObject()); err != nil { + metrics.RecordAllocationFailure("ipclaim", "internal", ipFamily, project, org) + return nil, err + } + } + + // Before any capacity is reserved: a namespace that is gone, or going, + // never releases what is bound into it. + if err := r.checkNamespaceLiveness(ctx, project, claim.Namespace); err != nil { + metrics.RecordAllocationFailure("ipclaim", "namespace_not_live", ipFamily, project, org) + failSpan(tracing.ReasonNamespaceNotLive) + return nil, err + } + + // Overwrite any client-supplied ownerRef. The requestheader CA guarantees + // the Extra headers carrying the tenant identity, so that identity is the + // source of truth for who this claim is for. + claim.Spec.OwnerRef = &ipam.ObjectRef{ + APIGroup: id.APIGroup, + Kind: id.Kind, + Name: id.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. - _ = tx.Rollback(ctx) + // The chain is provisioned outside any transaction, level by level, + // exactly as it is for a block claim. obj, outcome, err := r.createScopeRange(ctx, span, claim, class, id, dryRun) result = outcome if err != nil { @@ -312,7 +313,6 @@ func (r *AllocatingREST) Create(ctx context.Context, obj runtime.Object, createV prefixLen, err := allocator.EffectivePrefixLength(class.IPClass, claim.Spec.PrefixLength) if err != nil { - _ = tx.Rollback(ctx) metrics.RecordAllocationFailure("ipclaim", "internal", ipFamily, project, org) return nil, ipamerrors.New(ipamerrors.ReasonPrefixLengthRejected, err.Error()) } @@ -326,7 +326,6 @@ func (r *AllocatingREST) Create(ctx context.Context, obj runtime.Object, createV // nothing to explain it. uniqueScope, scopeDigest, err := scope.ProjectAddressSpaceDigest(id.Name, claim.Spec.Scope, class.Spec.UniqueWithin, "uniqueWithin") if err != nil { - _ = tx.Rollback(ctx) metrics.RecordAllocationFailure("ipclaim", "internal", ipFamily, project, org) var missing *scope.MissingRoleError if errors.As(err, &missing) { @@ -338,9 +337,8 @@ func (r *AllocatingREST) Create(ctx context.Context, obj runtime.Object, createV // Resolving the pool provisions any missing level of the class's chain, and // each level commits on its own — so it runs BEFORE the allocation // transaction opens, not inside it. A pool is durable infrastructure that - // outlives the claim that caused it; holding this transaction across the + // outlives the claim that caused it; holding a transaction across the // chain would make a herd of first claims serialise behind the slowest. - _ = tx.Rollback(ctx) poolKey, err := allocator.ResolvePool(ctx, r.db, class, claim.Spec.Scope) if err != nil { metrics.RecordAllocationFailure("ipclaim", "pool_not_found", ipFamily, project, org) diff --git a/internal/registry/ipam/ipclaim/storage_postgres_test.go b/internal/registry/ipam/ipclaim/storage_postgres_test.go index e88268d..f8e5ccf 100644 --- a/internal/registry/ipam/ipclaim/storage_postgres_test.go +++ b/internal/registry/ipam/ipclaim/storage_postgres_test.go @@ -429,3 +429,49 @@ func TestCreateDefaultsTheReferenceNameIntoSpec(t *testing.T) { t.Errorf("stored spec.className = %q, want shared", got) } } + +func TestCreateFillsTheClassAndFamilyBeforeAdmission(t *testing.T) { + r, _ := newPostgresREST(t) + claim := newClassClaim() + claim.Spec.IPFamily = "" + + var admitted ipam.IPClaimSpec + validate := func(_ context.Context, obj runtime.Object) error { + admitted = obj.(*ipam.IPClaim).Spec + return nil + } + + obj, err := r.Create(claimCtx(testProject), claim, validate, &metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Create: %v", err) + } + if admitted.IPFamily != ipam.IPv4 || admitted.ClassName != "standard" { + t.Errorf("admission saw className %q ipFamily %q, want standard and IPv4", admitted.ClassName, admitted.IPFamily) + } + if got := obj.(*ipam.IPClaim).Spec.IPFamily; got != ipam.IPv4 { + t.Errorf("stored ipFamily = %q, want %q", got, ipam.IPv4) + } +} + +func TestCreateRefusesAFamilyMismatchBeforeAdmission(t *testing.T) { + r, db := newPostgresREST(t) + claim := newClassClaim() + claim.Spec.IPFamily = ipam.IPv6 + + admitted := false + validate := func(context.Context, runtime.Object) error { + admitted = true + return nil + } + + _, err := r.Create(claimCtx(testProject), claim, validate, &metav1.CreateOptions{}) + if !apierrors.IsBadRequest(err) { + t.Fatalf("Create returned %v, want a bad request", err) + } + if admitted { + t.Error("a mismatched claim reached admission") + } + if n := countRows(t, db, `SELECT count(*) FROM ipam_objects WHERE kind = 'IPClaim'`); n != 0 { + t.Errorf("refused claim left %d rows, want 0", n) + } +} From 984f43a7247ed9cb045a50ce7c1aeab7f96b3333 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Wed, 28 Oct 2026 17:13:27 -0500 Subject: [PATCH 2/4] feat: Mark classes and claims public or private A class can now say whether it hands out publicly routable or private addresses, and every claim carries that marker from its class. Quota and other admission checks can then tell a public address from a private one without looking up the class themselves. Key changes: - add optional addressScope (Public | Private) to IPClassSpec and IPClaimSpec - copy the class's addressScope onto the claim before admission and refuse a claim whose addressScope contradicts its class with AddressScopeMismatch - let a class's addressScope be set once from unset, then hold it immutable - keep a claim's addressScope immutable and forbid it on class references - validate the enum in the registry strategies and regenerate OpenAPI Co-Authored-By: Claude Opus 5.5 --- docs/api.md | 2 + .../ipam/ipamvalidation/addressscope.go | 16 ++++ internal/registry/ipam/ipclaim/storage.go | 8 ++ .../ipam/ipclaim/storage_postgres_test.go | 92 +++++++++++++++++++ internal/registry/ipam/ipclaim/strategy.go | 5 + .../registry/ipam/ipclaim/strategy_test.go | 37 ++++++++ internal/registry/ipam/ipclass/strategy.go | 5 + .../registry/ipam/ipclass/strategy_test.go | 55 +++++++++++ pkg/apis/ipam/types.go | 17 ++++ pkg/apis/ipam/v1alpha1/conversion_impl.go | 4 + pkg/apis/ipam/v1alpha1/types.go | 20 ++++ pkg/generated/openapi/zz_generated.openapi.go | 14 +++ pkg/ipamerrors/errors.go | 5 + 13 files changed, 280 insertions(+) create mode 100644 internal/registry/ipam/ipamvalidation/addressscope.go diff --git a/docs/api.md b/docs/api.md index 19ac37e..6a79bf1 100644 --- a/docs/api.md +++ b/docs/api.md @@ -22,6 +22,7 @@ IPv6 are never mixed. ```go IPFamily = IPv4 | IPv6 +AddressScope = Public | Private Strategy = FirstFit | BestFit | LeastUtilized ReclaimPolicy = Delete | Retain ClaimPhase = Pending | Bound | Releasing | Error @@ -69,6 +70,7 @@ The claim's CREATE response body contains the allocated CIDR. | Field | Type | Required | Notes | |-------|------|----------|-------| | `spec.ipFamily` | IPFamily | yes | Address family to allocate. | +| `spec.addressScope` | AddressScope | no | Copied from the claim's class on create; if set, must agree with the class. Immutable. | | `spec.prefixLength` | int (0–128) | yes | Requested sub-prefix size; must be valid for the family and within the pool's min/max. | | `spec.poolRef` | NamespacedRef | one of | Pin to a specific pool (optionally in another project via `projectRef`). | | `spec.poolSelector` | PoolSelector | one of | Pick a pool by labels (optionally scoped by `projectRef`). | diff --git a/internal/registry/ipam/ipamvalidation/addressscope.go b/internal/registry/ipam/ipamvalidation/addressscope.go new file mode 100644 index 0000000..e3a9af6 --- /dev/null +++ b/internal/registry/ipam/ipamvalidation/addressscope.go @@ -0,0 +1,16 @@ +package ipamvalidation + +import ( + "k8s.io/apimachinery/pkg/util/validation/field" + + "go.miloapis.com/ipam/pkg/apis/ipam" +) + +func AddressScope(s ipam.AddressScope, path *field.Path) field.ErrorList { + switch s { + case "", ipam.AddressScopePublic, ipam.AddressScopePrivate: + return nil + } + return field.ErrorList{field.NotSupported(path, s, + []string{string(ipam.AddressScopePublic), string(ipam.AddressScopePrivate)})} +} diff --git a/internal/registry/ipam/ipclaim/storage.go b/internal/registry/ipam/ipclaim/storage.go index fd9208a..0935cc7 100644 --- a/internal/registry/ipam/ipclaim/storage.go +++ b/internal/registry/ipam/ipclaim/storage.go @@ -263,6 +263,13 @@ func (r *AllocatingREST) Create(ctx context.Context, obj runtime.Object, createV "spec.ipFamily is %s but class %q hands out %s", claim.Spec.IPFamily, class.LocalName, class.Spec.IPFamily)) } + if claim.Spec.AddressScope != "" && string(claim.Spec.AddressScope) != string(class.Spec.AddressScope) { + metrics.RecordAllocationFailure("ipclaim", "invalid", ipFamily, project, org) + failSpan(tracing.ReasonPoolNotFound) + return nil, ipamerrors.New(ipamerrors.ReasonAddressScopeMismatch, fmt.Sprintf( + "spec.addressScope is %s but class %q has addressScope %q", + claim.Spec.AddressScope, class.Name, class.Spec.AddressScope)) + } // Default the class the same way a PersistentVolumeClaim is defaulted to // the cluster's StorageClass: the claim is persisted below, so the stored @@ -271,6 +278,7 @@ func (r *AllocatingREST) Create(ctx context.Context, obj runtime.Object, createV // what is written here is what the claim allocated under, for good. claim.Spec.ClassName = class.LocalName claim.Spec.IPFamily = ipam.IPFamily(class.Spec.IPFamily) + claim.Spec.AddressScope = ipam.AddressScope(class.Spec.AddressScope) ipFamily = string(claim.Spec.IPFamily) span.SetAttributes(attribute.String(tracing.AttrClaimIPFamily, ipFamily)) diff --git a/internal/registry/ipam/ipclaim/storage_postgres_test.go b/internal/registry/ipam/ipclaim/storage_postgres_test.go index f8e5ccf..eb0ea65 100644 --- a/internal/registry/ipam/ipclaim/storage_postgres_test.go +++ b/internal/registry/ipam/ipclaim/storage_postgres_test.go @@ -475,3 +475,95 @@ func TestCreateRefusesAFamilyMismatchBeforeAdmission(t *testing.T) { t.Errorf("refused claim left %d rows, want 0", n) } } + +func markStandardClassScope(t *testing.T, db *pgxpool.Pool, s ipamv1alpha1.AddressScope) { + t.Helper() + class := &ipamv1alpha1.IPClass{ + TypeMeta: metav1.TypeMeta{APIVersion: "ipam.miloapis.com/v1alpha1", Kind: "IPClass"}, + ObjectMeta: metav1.ObjectMeta{Name: "standard"}, + Spec: ipamv1alpha1.IPClassSpec{ + IPFamily: ipamv1alpha1.IPv4, AddressScope: s, DefaultPrefixLength: 24, + }, + } + data, err := json.Marshal(class) + if err != nil { + t.Fatalf("marshal class: %v", err) + } + if _, err := db.Exec(context.Background(), + `UPDATE ipam_objects SET data = $2 WHERE key = $1`, + tenant.Identity{Name: testProject}.ResourceKey("ipclasses", "standard"), data); err != nil { + t.Fatalf("update class: %v", err) + } +} + +func TestCreateCopiesTheAddressScopeFromTheClassBeforeAdmission(t *testing.T) { + r, db := newPostgresREST(t) + markStandardClassScope(t, db, ipamv1alpha1.AddressScopePublic) + claim := newClassClaim() + claim.Spec.IPFamily = "" + + var admitted ipam.AddressScope + validate := func(_ context.Context, obj runtime.Object) error { + admitted = obj.(*ipam.IPClaim).Spec.AddressScope + return nil + } + + obj, err := r.Create(claimCtx(testProject), claim, validate, &metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Create: %v", err) + } + if admitted != ipam.AddressScopePublic { + t.Errorf("admission saw addressScope %q, want %q", admitted, ipam.AddressScopePublic) + } + if got := obj.(*ipam.IPClaim).Spec.AddressScope; got != ipam.AddressScopePublic { + t.Errorf("stored addressScope = %q, want %q", got, ipam.AddressScopePublic) + } +} + +func TestCreateRefusesAnAddressScopeThatContradictsTheClass(t *testing.T) { + r, db := newPostgresREST(t) + markStandardClassScope(t, db, ipamv1alpha1.AddressScopePrivate) + claim := newClassClaim() + claim.Spec.AddressScope = ipam.AddressScopePublic + + admitted := false + validate := func(context.Context, runtime.Object) error { + admitted = true + return nil + } + + _, err := r.Create(claimCtx(testProject), claim, validate, &metav1.CreateOptions{}) + if !apierrors.IsBadRequest(err) { + t.Fatalf("Create returned %v, want a bad request", err) + } + if !strings.Contains(err.Error(), "Public") || !strings.Contains(err.Error(), "standard") { + t.Errorf("error %q should name both the scope asked for and the class", err) + } + if admitted { + t.Error("a contradicted claim reached admission") + } + if n := countRows(t, db, `SELECT count(*) FROM ipam_objects WHERE kind = 'IPClaim'`); n != 0 { + t.Errorf("refused claim left %d rows, want 0", n) + } +} + +func TestCreateLeavesTheAddressScopeEmptyWhenTheClassStatesNone(t *testing.T) { + r, _ := newPostgresREST(t) + + var admitted ipam.AddressScope = "unset" + validate := func(_ context.Context, obj runtime.Object) error { + admitted = obj.(*ipam.IPClaim).Spec.AddressScope + return nil + } + + obj, err := r.Create(claimCtx(testProject), newClassClaim(), validate, &metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Create: %v", err) + } + if admitted != "" { + t.Errorf("admission saw addressScope %q, want empty", admitted) + } + if got := obj.(*ipam.IPClaim).Spec.AddressScope; got != "" { + t.Errorf("stored addressScope = %q, want empty", got) + } +} diff --git a/internal/registry/ipam/ipclaim/strategy.go b/internal/registry/ipam/ipclaim/strategy.go index 0aa445e..74e08a8 100644 --- a/internal/registry/ipam/ipclaim/strategy.go +++ b/internal/registry/ipam/ipclaim/strategy.go @@ -15,6 +15,7 @@ import ( "sigs.k8s.io/structured-merge-diff/v6/fieldpath" "go.miloapis.com/ipam/internal/fieldindex" + "go.miloapis.com/ipam/internal/registry/ipam/ipamvalidation" "go.miloapis.com/ipam/internal/scope" "go.miloapis.com/ipam/pkg/apis/ipam" ) @@ -83,6 +84,9 @@ func (ipClaimStrategy) ValidateUpdate(_ context.Context, obj, old runtime.Object if n.Spec.IPFamily != o.Spec.IPFamily { allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "ipFamily"), "ipFamily is immutable")) } + if n.Spec.AddressScope != o.Spec.AddressScope { + allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "addressScope"), "addressScope is immutable")) + } if !equality.Semantic.DeepEqual(n.Spec.PrefixLength, o.Spec.PrefixLength) { allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "prefixLength"), "prefixLength is immutable")) } @@ -121,6 +125,7 @@ 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)})) } + allErrs = append(allErrs, ipamvalidation.AddressScope(c.Spec.AddressScope, specPath.Child("addressScope"))...) // 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 diff --git a/internal/registry/ipam/ipclaim/strategy_test.go b/internal/registry/ipam/ipclaim/strategy_test.go index b0e0211..46db992 100644 --- a/internal/registry/ipam/ipclaim/strategy_test.go +++ b/internal/registry/ipam/ipclaim/strategy_test.go @@ -1,6 +1,7 @@ package ipclaim import ( + "context" "strings" "testing" @@ -139,3 +140,39 @@ func TestValidatePrefixLengthDoesNotAssumeIPv4(t *testing.T) { }) } } + +func TestAClaimAddressScopeMustBeKnown(t *testing.T) { + for _, tc := range []struct { + scope ipam.AddressScope + valid bool + }{ + {"", true}, + {ipam.AddressScopePublic, true}, + {ipam.AddressScopePrivate, true}, + {"Internet", false}, + } { + t.Run(string(tc.scope), func(t *testing.T) { + errs := validateIPClaim(claim(func(c *ipam.IPClaim) { c.Spec.AddressScope = tc.scope })) + if tc.valid && len(errs) != 0 { + t.Fatalf("addressScope %q rejected: %v", tc.scope, errs) + } + if !tc.valid && (len(errs) == 0 || errs[0].Field != "spec.addressScope") { + t.Fatalf("errors = %v, want one on spec.addressScope", errs) + } + }) + } +} + +func TestAClaimAddressScopeIsImmutable(t *testing.T) { + old := claim(func(c *ipam.IPClaim) { c.Spec.AddressScope = ipam.AddressScopePublic }) + updated := old.DeepCopy() + updated.Spec.AddressScope = ipam.AddressScopePrivate + + errs := NewStrategy(nil).ValidateUpdate(context.Background(), updated, old) + for _, e := range errs { + if e.Field == "spec.addressScope" { + return + } + } + t.Fatalf("errors = %v, want one forbidding spec.addressScope", errs) +} diff --git a/internal/registry/ipam/ipclass/strategy.go b/internal/registry/ipam/ipclass/strategy.go index a3252a8..78dfd33 100644 --- a/internal/registry/ipam/ipclass/strategy.go +++ b/internal/registry/ipam/ipclass/strategy.go @@ -160,6 +160,9 @@ func (ipClassStrategy) ValidateUpdate(_ context.Context, obj, old runtime.Object if n.Spec.IPFamily != o.Spec.IPFamily { allErrs = append(allErrs, field.Forbidden(specPath.Child("ipFamily"), "spec.ipFamily is immutable")) } + if o.Spec.AddressScope != "" && n.Spec.AddressScope != o.Spec.AddressScope { + allErrs = append(allErrs, field.Forbidden(specPath.Child("addressScope"), "spec.addressScope is immutable once set")) + } if n.Spec.ParentClassName != o.Spec.ParentClassName { allErrs = append(allErrs, field.Forbidden(specPath.Child("parentClassName"), "spec.parentClassName is immutable")) } @@ -212,6 +215,7 @@ func validateReference(c *ipam.IPClass) field.ErrorList { set bool }{ {"ipFamily", c.Spec.IPFamily != ""}, + {"addressScope", c.Spec.AddressScope != ""}, {"parentClassName", c.Spec.ParentClassName != ""}, {"poolPer", len(c.Spec.PoolPer) > 0}, {"uniqueWithin", len(c.Spec.UniqueWithin) > 0}, @@ -246,6 +250,7 @@ func validateDefinition(c *ipam.IPClass) field.ErrorList { allErrs = append(allErrs, field.NotSupported(specPath.Child("ipFamily"), c.Spec.IPFamily, []string{string(ipam.IPv4), string(ipam.IPv6)})) } + allErrs = append(allErrs, ipamvalidation.AddressScope(c.Spec.AddressScope, specPath.Child("addressScope"))...) if c.Spec.ParentClassName == c.Name && c.Name != "" { allErrs = append(allErrs, field.Invalid(specPath.Child("parentClassName"), c.Spec.ParentClassName, diff --git a/internal/registry/ipam/ipclass/strategy_test.go b/internal/registry/ipam/ipclass/strategy_test.go index 2fc5057..6141f81 100644 --- a/internal/registry/ipam/ipclass/strategy_test.go +++ b/internal/registry/ipam/ipclass/strategy_test.go @@ -42,6 +42,7 @@ func TestAReferenceStatesNoPolicyOfItsOwn(t *testing.T) { mutate func(*ipam.IPClass) }{ {"ipFamily", func(c *ipam.IPClass) { c.Spec.IPFamily = ipam.IPv6 }}, + {"addressScope", func(c *ipam.IPClass) { c.Spec.AddressScope = ipam.AddressScopePublic }}, {"parentClassName", func(c *ipam.IPClass) { c.Spec.ParentClassName = "backbone" }}, {"poolPer", func(c *ipam.IPClass) { c.Spec.PoolPer = []string{"location"} }}, {"uniqueWithin", func(c *ipam.IPClass) { c.Spec.UniqueWithin = []string{"network"} }}, @@ -373,3 +374,57 @@ func TestTheSharedPoolWarningIsNotRepeatedOnUpdate(t *testing.T) { t.Errorf("update warned: %v", w) } } + +func TestAClassAddressScopeMustBeKnown(t *testing.T) { + for _, tc := range []struct { + scope ipam.AddressScope + valid bool + }{ + {"", true}, + {ipam.AddressScopePublic, true}, + {ipam.AddressScopePrivate, true}, + {"Internet", false}, + } { + t.Run(string(tc.scope), func(t *testing.T) { + errs := validateIPClass(definition("standard", func(c *ipam.IPClass) { c.Spec.AddressScope = tc.scope })) + if tc.valid && len(errs) != 0 { + t.Fatalf("addressScope %q rejected: %v", tc.scope, errs) + } + if !tc.valid && (len(errs) == 0 || errs[0].Field != "spec.addressScope") { + t.Fatalf("errors = %v, want one on spec.addressScope", errs) + } + }) + } +} + +func TestAClassAddressScopeCanBeSetOnceOnly(t *testing.T) { + strategy := ipClassStrategy{} + ctx := context.Background() + + for _, tc := range []struct { + name string + from, to ipam.AddressScope + allowed bool + }{ + {"unset to Public", "", ipam.AddressScopePublic, true}, + {"unset to Private", "", ipam.AddressScopePrivate, true}, + {"Public to Private", ipam.AddressScopePublic, ipam.AddressScopePrivate, false}, + {"Public to unset", ipam.AddressScopePublic, "", false}, + } { + t.Run(tc.name, func(t *testing.T) { + old := definition("standard", func(c *ipam.IPClass) { c.Spec.AddressScope = tc.from }) + updated := old.DeepCopy() + updated.Spec.AddressScope = tc.to + + var forbidden bool + for _, e := range strategy.ValidateUpdate(ctx, updated, old) { + if e.Field == "spec.addressScope" { + forbidden = true + } + } + if forbidden == tc.allowed { + t.Fatalf("%s: forbidden = %v, want allowed = %v", tc.name, forbidden, tc.allowed) + } + }) + } +} diff --git a/pkg/apis/ipam/types.go b/pkg/apis/ipam/types.go index 05813ca..3d6440a 100644 --- a/pkg/apis/ipam/types.go +++ b/pkg/apis/ipam/types.go @@ -10,6 +10,14 @@ const ( IPv6 IPFamily = "IPv6" ) +// AddressScope says whether addresses are publicly routable or private. +type AddressScope string + +const ( + AddressScopePublic AddressScope = "Public" + AddressScopePrivate AddressScope = "Private" +) + // Strategy selects how a free sub-block is chosen. // PoolSelectionStrategy chooses WHICH POOL serves a claim (set on IPClass). // AllocationStrategy chooses which free block is taken from inside it (set on @@ -215,6 +223,11 @@ type IPClassSpec struct { // immutable. Dual-stack is two classes, never one. IPFamily IPFamily + // AddressScope says whether this class hands out publicly routable or + // private addresses. Empty states neither. May be set once on a class that + // has none; immutable after that. + AddressScope AddressScope + // ParentClassName is the class whose allocations this one carves from. // Empty means allocations come from the pools offering this class directly. // Immutable: changing it strands every existing allocation outside its @@ -504,6 +517,10 @@ type IPClaimSpec struct { // ignored — and must agree, if set — when ClassName names a class. IPFamily IPFamily + // AddressScope is copied from the class on create. It must agree, if set, + // with the class the claim resolves to. Immutable. + AddressScope AddressScope + // Address names a specific address to bind. Omitted, the allocator chooses. Address string diff --git a/pkg/apis/ipam/v1alpha1/conversion_impl.go b/pkg/apis/ipam/v1alpha1/conversion_impl.go index fa41670..24cacd1 100644 --- a/pkg/apis/ipam/v1alpha1/conversion_impl.go +++ b/pkg/apis/ipam/v1alpha1/conversion_impl.go @@ -196,6 +196,7 @@ func convert_v1alpha1_IPClass_To_ipam(in *IPClass, out *ipam.IPClass) error { out.Spec = ipam.IPClassSpec{ Source: toIpamClassSource(in.Spec.Source), IPFamily: ipam.IPFamily(in.Spec.IPFamily), + AddressScope: ipam.AddressScope(in.Spec.AddressScope), ParentClassName: in.Spec.ParentClassName, PoolPer: copyStrings(in.Spec.PoolPer), UniqueWithin: copyStrings(in.Spec.UniqueWithin), @@ -221,6 +222,7 @@ func convert_ipam_IPClass_To_v1alpha1(in *ipam.IPClass, out *IPClass) error { out.Spec = IPClassSpec{ Source: toV1ClassSource(in.Spec.Source), IPFamily: IPFamily(in.Spec.IPFamily), + AddressScope: AddressScope(in.Spec.AddressScope), ParentClassName: in.Spec.ParentClassName, PoolPer: copyStrings(in.Spec.PoolPer), UniqueWithin: copyStrings(in.Spec.UniqueWithin), @@ -440,6 +442,7 @@ func convert_v1alpha1_IPClaim_To_ipam(in *IPClaim, out *ipam.IPClaim) error { out.Spec = ipam.IPClaimSpec{ ClassName: in.Spec.ClassName, IPFamily: ipam.IPFamily(in.Spec.IPFamily), + AddressScope: ipam.AddressScope(in.Spec.AddressScope), Address: in.Spec.Address, PrefixLength: copyInt32Ptr(in.Spec.PrefixLength), Scope: toIpamScope(in.Spec.Scope), @@ -463,6 +466,7 @@ func convert_ipam_IPClaim_To_v1alpha1(in *ipam.IPClaim, out *IPClaim) error { out.Spec = IPClaimSpec{ ClassName: in.Spec.ClassName, IPFamily: IPFamily(in.Spec.IPFamily), + AddressScope: AddressScope(in.Spec.AddressScope), Address: in.Spec.Address, PrefixLength: copyInt32Ptr(in.Spec.PrefixLength), Scope: toV1Scope(in.Spec.Scope), diff --git a/pkg/apis/ipam/v1alpha1/types.go b/pkg/apis/ipam/v1alpha1/types.go index 2310b21..02cc138 100644 --- a/pkg/apis/ipam/v1alpha1/types.go +++ b/pkg/apis/ipam/v1alpha1/types.go @@ -11,6 +11,15 @@ const ( IPv6 IPFamily = "IPv6" ) +// AddressScope says whether addresses are publicly routable or private. +// +kubebuilder:validation:Enum=Public;Private +type AddressScope string + +const ( + AddressScopePublic AddressScope = "Public" + AddressScopePrivate AddressScope = "Private" +) + // PoolSelectionStrategy chooses WHICH POOL serves a claim, when several pools // back one class. It is set on IPClass. // @@ -316,6 +325,12 @@ type IPClassSpec struct { // immutable. Dual-stack is two classes, never one. IPFamily IPFamily `json:"ipFamily"` + // AddressScope says whether this class hands out publicly routable or + // private addresses. Empty states neither. May be set once on a class that + // has none; immutable after that. + // +optional + AddressScope AddressScope `json:"addressScope,omitempty"` + // ParentClassName is the class whose allocations this one carves from, // resolved in this class's own project. Empty means allocations come from // the pools that offer this class directly via IPPool.spec.classNames. @@ -846,6 +861,11 @@ type IPClaimSpec struct { // +optional IPFamily IPFamily `json:"ipFamily,omitempty"` + // AddressScope is copied from the class on create. It must agree, if set, + // with the class the claim resolves to. Immutable. + // +optional + AddressScope AddressScope `json:"addressScope,omitempty"` + // Address names a specific address to bind. Omitted, the allocator chooses. // +optional Address string `json:"address,omitempty"` diff --git a/pkg/generated/openapi/zz_generated.openapi.go b/pkg/generated/openapi/zz_generated.openapi.go index 0bb4249..cd76e36 100644 --- a/pkg/generated/openapi/zz_generated.openapi.go +++ b/pkg/generated/openapi/zz_generated.openapi.go @@ -502,6 +502,13 @@ func schema_pkg_apis_ipam_v1alpha1_IPClaimSpec(ref common.ReferenceCallback) com Format: "", }, }, + "addressScope": { + SchemaProps: spec.SchemaProps{ + Description: "AddressScope is copied from the class on create. It must agree, if set, with the class the claim resolves to. Immutable.", + Type: []string{"string"}, + Format: "", + }, + }, "address": { SchemaProps: spec.SchemaProps{ Description: "Address names a specific address to bind. Omitted, the allocator chooses.", @@ -736,6 +743,13 @@ func schema_pkg_apis_ipam_v1alpha1_IPClassSpec(ref common.ReferenceCallback) com Format: "", }, }, + "addressScope": { + SchemaProps: spec.SchemaProps{ + Description: "AddressScope says whether this class hands out publicly routable or private addresses. Empty states neither. May be set once on a class that has none; immutable after that.", + Type: []string{"string"}, + Format: "", + }, + }, "parentClassName": { SchemaProps: spec.SchemaProps{ Description: "ParentClassName is the class whose allocations this one carves from, resolved in this class's own project. Empty means allocations come from the pools that offer this class directly via IPPool.spec.classNames.\n\nImmutable: changing it strands every existing allocation outside its declared ancestry.", diff --git a/pkg/ipamerrors/errors.go b/pkg/ipamerrors/errors.go index 4f6cf58..b7c21ba 100644 --- a/pkg/ipamerrors/errors.go +++ b/pkg/ipamerrors/errors.go @@ -67,6 +67,10 @@ const ( // when no class is named; it never overrides one that is. ReasonFamilyMismatch Reason = "FamilyMismatch" + // ReasonAddressScopeMismatch reports that the claim stated an address scope + // the class it resolved to does not carry. + ReasonAddressScopeMismatch Reason = "AddressScopeMismatch" + // ReasonScopeRolesMissing reports that the claim's scope did not carry // roles the class requires. MissingScopeRoles names them. ReasonScopeRolesMissing Reason = "ScopeRolesMissing" @@ -231,6 +235,7 @@ var knownReasons = map[Reason]bool{ ReasonNoOfferingPool: true, ReasonPrefixLengthRejected: true, ReasonFamilyMismatch: true, + ReasonAddressScopeMismatch: true, ReasonScopeRolesMissing: true, ReasonAllocationRetained: true, ReasonClaimExists: true, From 6a08a8309fad8f374b8ca6fc4ed505b494276276 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Wed, 28 Oct 2026 15:54:16 -0500 Subject: [PATCH 3/4] feat: Add a per-project public IPv4 claim quota Projects can now be limited in how many public IPv4 claims they hold, separately from the overall claim limit. The charge rides on the existing IPClaim rule and applies only to claims that are IPv4 and whose class marks them public. Key changes: - add the ipam.miloapis.com/public-ipv4claims metric and the public-ipv4claims-per-project limit (default 2, max 256) - charge it from the IPClaim rule via a metricCostConditions CEL guard on ipFamily and addressScope Co-Authored-By: Claude Opus 5.5 --- config/components/README.md | 2 +- .../service-catalog/serviceconfiguration.yaml | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/config/components/README.md b/config/components/README.md index 6c011d9..7118a1b 100644 --- a/config/components/README.md +++ b/config/components/README.md @@ -13,7 +13,7 @@ and order-insensitive within an overlay (with a few documented exceptions). | `postgres-migrations` | Job + ConfigMap that applies `migrations/*.sql` | | `observability` | `ServiceMonitor` + `GrafanaDashboard` resources | | `k6-performance-tests` | k6 SA/RBAC + `TestRun` resources for the perf suite | -| `service-catalog` | `Service` + `ServiceConfiguration` registering IPAM (incl. IPClaim quota) on the Milo control plane | +| `service-catalog` | `Service` + `ServiceConfiguration` registering IPAM (incl. IPClaim, public IPv4 IPClaim, and IPPool quota) on the Milo control plane | Order matters when: - `cert-manager-ca` must precede the deployment so its CSI volume can mount. diff --git a/config/components/service-catalog/serviceconfiguration.yaml b/config/components/service-catalog/serviceconfiguration.yaml index c60bf36..0c3bdd0 100644 --- a/config/components/service-catalog/serviceconfiguration.yaml +++ b/config/components/service-catalog/serviceconfiguration.yaml @@ -15,6 +15,13 @@ spec: outstanding allocation request or reservation against an IPAM pool. kind: Gauge unit: "1" + - name: ipam.miloapis.com/public-ipv4claims + displayName: Public IPv4 Claims + description: >- + Number of public IPv4 IPClaim objects a project holds. Each counts once + against this metric in addition to the IP Claims metric. + kind: Gauge + unit: "1" - name: ipam.miloapis.com/ippools displayName: IP Pools description: >- @@ -32,6 +39,14 @@ spec: unit: "1/{project}" defaultLimit: 100 maxLimit: 10000 + - name: public-ipv4claims-per-project + metric: ipam.miloapis.com/public-ipv4claims + consumerType: + apiGroup: resourcemanager.miloapis.com + kind: Project + unit: "1/{project}" + defaultLimit: 2 + maxLimit: 256 - name: ippools-per-project metric: ipam.miloapis.com/ippools consumerType: @@ -46,6 +61,9 @@ spec: kind: IPClaim metricCosts: ipam.miloapis.com/ipclaims: 1 + ipam.miloapis.com/public-ipv4claims: 1 + metricCostConditions: + ipam.miloapis.com/public-ipv4claims: "has(trigger.spec.ipFamily) && trigger.spec.ipFamily == 'IPv4' && has(trigger.spec.addressScope) && trigger.spec.addressScope == 'Public'" - selector: apiGroup: ipam.miloapis.com kind: IPPool From b55c89c9b1b7f7f67fe710ec171efe78490953a1 Mon Sep 17 00:00:00 2001 From: Scot Schuchert-Wells Date: Thu, 29 Oct 2026 23:40:22 +0000 Subject: [PATCH 4/4] fix: Ship the public IPv4 limit in its own component Production deploys every IPAM release from config/components/service-catalog, and a Milo without milo-os/milo#940 drops the metric cost condition and charges every IPClaim against the public IPv4 limit, private and IPv6 claims included. Move the metric, the limit and the condition into a separate component that an environment selects only once its Milo and service catalog honour conditions, so a release changes nothing where they do not. Key changes: - restore config/components/service-catalog to its previous quota rules - add config/components/service-catalog-public-ipv4, which layers the public IPv4 metric, limit and condition on top of it --- config/components/README.md | 3 +- .../kustomization.yaml | 47 +++++++++++++++++++ .../service-catalog/serviceconfiguration.yaml | 18 ------- 3 files changed, 49 insertions(+), 19 deletions(-) create mode 100644 config/components/service-catalog-public-ipv4/kustomization.yaml diff --git a/config/components/README.md b/config/components/README.md index 7118a1b..ff8b7b1 100644 --- a/config/components/README.md +++ b/config/components/README.md @@ -13,7 +13,8 @@ and order-insensitive within an overlay (with a few documented exceptions). | `postgres-migrations` | Job + ConfigMap that applies `migrations/*.sql` | | `observability` | `ServiceMonitor` + `GrafanaDashboard` resources | | `k6-performance-tests` | k6 SA/RBAC + `TestRun` resources for the perf suite | -| `service-catalog` | `Service` + `ServiceConfiguration` registering IPAM (incl. IPClaim, public IPv4 IPClaim, and IPPool quota) on the Milo control plane | +| `service-catalog` | `Service` + `ServiceConfiguration` registering IPAM (incl. IPClaim quota) on the Milo control plane | +| `service-catalog-public-ipv4` | `service-catalog` plus a per-project public IPv4 IPClaim limit; needs a Milo and service catalog that honour metric cost conditions | Order matters when: - `cert-manager-ca` must precede the deployment so its CSI volume can mount. diff --git a/config/components/service-catalog-public-ipv4/kustomization.yaml b/config/components/service-catalog-public-ipv4/kustomization.yaml new file mode 100644 index 0000000..41a89ee --- /dev/null +++ b/config/components/service-catalog-public-ipv4/kustomization.yaml @@ -0,0 +1,47 @@ +# The service catalog registration plus a per-project public IPv4 claim limit. +# +# The limit charges only IPv4 claims from a public class, through a metric cost +# condition. A Milo older than milo-os/milo#940 drops that condition and charges +# every IPClaim, so an environment selects this path in place of +# ../service-catalog only once its Milo and service catalog carry conditions. +apiVersion: kustomize.config.k8s.io/v1alpha1 +kind: Component + +components: + - ../service-catalog + +patches: + - target: + group: services.miloapis.com + version: v1alpha1 + kind: ServiceConfiguration + name: ipam-miloapis-com + patch: |- + - op: add + path: /spec/metrics/- + value: + name: ipam.miloapis.com/public-ipv4claims + displayName: Public IPv4 Claims + description: >- + Number of public IPv4 IPClaim objects a project holds. Each counts + once against this metric in addition to the IP Claims metric. + kind: Gauge + unit: "1" + - op: add + path: /spec/quota/limits/- + value: + name: public-ipv4claims-per-project + metric: ipam.miloapis.com/public-ipv4claims + consumerType: + apiGroup: resourcemanager.miloapis.com + kind: Project + unit: "1/{project}" + defaultLimit: 2 + maxLimit: 256 + - op: add + path: /spec/quota/metricRules/0/metricCosts/ipam.miloapis.com~1public-ipv4claims + value: 1 + - op: add + path: /spec/quota/metricRules/0/metricCostConditions + value: + ipam.miloapis.com/public-ipv4claims: "has(trigger.spec.ipFamily) && trigger.spec.ipFamily == 'IPv4' && has(trigger.spec.addressScope) && trigger.spec.addressScope == 'Public'" diff --git a/config/components/service-catalog/serviceconfiguration.yaml b/config/components/service-catalog/serviceconfiguration.yaml index 0c3bdd0..c60bf36 100644 --- a/config/components/service-catalog/serviceconfiguration.yaml +++ b/config/components/service-catalog/serviceconfiguration.yaml @@ -15,13 +15,6 @@ spec: outstanding allocation request or reservation against an IPAM pool. kind: Gauge unit: "1" - - name: ipam.miloapis.com/public-ipv4claims - displayName: Public IPv4 Claims - description: >- - Number of public IPv4 IPClaim objects a project holds. Each counts once - against this metric in addition to the IP Claims metric. - kind: Gauge - unit: "1" - name: ipam.miloapis.com/ippools displayName: IP Pools description: >- @@ -39,14 +32,6 @@ spec: unit: "1/{project}" defaultLimit: 100 maxLimit: 10000 - - name: public-ipv4claims-per-project - metric: ipam.miloapis.com/public-ipv4claims - consumerType: - apiGroup: resourcemanager.miloapis.com - kind: Project - unit: "1/{project}" - defaultLimit: 2 - maxLimit: 256 - name: ippools-per-project metric: ipam.miloapis.com/ippools consumerType: @@ -61,9 +46,6 @@ spec: kind: IPClaim metricCosts: ipam.miloapis.com/ipclaims: 1 - ipam.miloapis.com/public-ipv4claims: 1 - metricCostConditions: - ipam.miloapis.com/public-ipv4claims: "has(trigger.spec.ipFamily) && trigger.spec.ipFamily == 'IPv4' && has(trigger.spec.addressScope) && trigger.spec.addressScope == 'Public'" - selector: apiGroup: ipam.miloapis.com kind: IPPool