fix: record the class a claim resolved to, and stop guessing its family

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.

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)
+	}
+}

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:

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)
+			}
+		})
+	}
+}

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,