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 51822a5..915d311 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.LocalName, 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.LocalName + 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..e88268d 100644 --- a/internal/registry/ipam/ipclaim/storage_postgres_test.go +++ b/internal/registry/ipam/ipclaim/storage_postgres_test.go @@ -266,3 +266,166 @@ 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) + } +} + +// 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) + } +} 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,