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,