diff --git a/config/components/README.md b/config/components/README.md index 6c011d9..ff8b7b1 100644 --- a/config/components/README.md +++ b/config/components/README.md @@ -14,6 +14,7 @@ and order-insensitive within an overlay (with a few documented exceptions). | `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-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/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 915d311..0935cc7 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,13 +257,19 @@ 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( "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 @@ -295,11 +277,38 @@ 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) + claim.Spec.AddressScope = ipam.AddressScope(class.Spec.AddressScope) + 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 +321,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 +334,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 +345,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..eb0ea65 100644 --- a/internal/registry/ipam/ipclaim/storage_postgres_test.go +++ b/internal/registry/ipam/ipclaim/storage_postgres_test.go @@ -429,3 +429,141 @@ 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) + } +} + +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,