fix: state IPv6 on the fabric identity claim

internal/controller/networkfabricidentity_controller_test.go

@@ -20,15 +20,18 @@ package controller
 import (
 	"context"
 	"errors"
+	"fmt"
 	"sort"
 	"strings"
 	"testing"
 
 	ipamv1alpha1 "go.miloapis.com/ipam/pkg/apis/ipam/v1alpha1"
 	"go.miloapis.com/ipam/pkg/ipamerrors"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -64,6 +67,35 @@ type fakeIdentityIPAM struct {
 
 func allocationNameFor(claimName string) string { return "alloc-" + claimName }
 
+// refuseWhatTheServerWouldRefuse mirrors the address service's own admission of
+// a claim. The fake would otherwise bind anything, which is how a claim that no
+// real server has ever accepted passed every test here.
+//
+// Only the part this depends on is modelled: the server bounds a claim's prefix
+// length by the family stated on the claim, before it looks at the class, so an
+// identity block asked for without a family is read as an IPv4 length.
+func refuseWhatTheServerWouldRefuse(ipClaim *ipamv1alpha1.IPClaim) error {
+	if ipClaim.Spec.ClassName == "" && ipClaim.Spec.IPFamily == "" {
+		return apierrors.NewInvalid(
+			ipamv1alpha1.SchemeGroupVersion.WithKind("IPClaim").GroupKind(), ipClaim.Name,
+			field.ErrorList{field.Required(field.NewPath("spec"),
+				"one of className or ipFamily is required")})
+	}
+	if p := ipClaim.Spec.PrefixLength; p != nil {
+		maxLen := int32(32)
+		if ipClaim.Spec.IPFamily == ipamv1alpha1.IPv6 {
+			maxLen = 128
+		}
+		if *p <= 0 || *p > maxLen {
+			return apierrors.NewInvalid(
+				ipamv1alpha1.SchemeGroupVersion.WithKind("IPClaim").GroupKind(), ipClaim.Name,
+				field.ErrorList{field.Invalid(field.NewPath("spec", "prefixLength"), *p,
+					fmt.Sprintf("must be between 1 and %d", maxLen))})
+		}
+	}
+	return nil
+}
+
 func newFakeIdentityIPAM(t *testing.T) *fakeIdentityIPAM {
 	t.Helper()
 	scheme := runtime.NewScheme()
@@ -80,6 +112,10 @@ func newFakeIdentityIPAM(t *testing.T) *fakeIdentityIPAM {
 			}
 			f.created = append(f.created, ipClaim.Name)
 
+			if err := refuseWhatTheServerWouldRefuse(ipClaim); err != nil {
+				return err
+			}
+
 			// An allocation left behind by a deleted claim blocks the name it
 			// used, which is exactly what retention is for.
 			allocationName := allocationNameFor(ipClaim.Name)

internal/fabricidentity/identity.go

@@ -118,7 +118,14 @@ func Claim(ctx context.Context, ipamClient client.Client, request Request) (int6
 	ipClaim.Namespace = request.Namespace
 	ipClaim.Name = ClaimName(request.NetworkNamespace, request.NetworkName)
 	ipClaim.Spec = ipamv1alpha1.IPClaimSpec{
-		ClassName:    request.ClassName,
+		ClassName: request.ClassName,
+
+		// The class already fixes the family, but the server bounds a claim's
+		// prefix length from the family on the claim alone, before it resolves
+		// the class at all. Left unset, a /64 is read as an IPv4 length and
+		// refused, so every allocation fails.
+		IPFamily: ipamv1alpha1.IPv6,
+
 		Target:       ipamv1alpha1.TargetBlock,
 		PrefixLength: ptr.To(int32(BlockBits)),
 

internal/fabricidentity/identity_test.go

@@ -18,9 +18,19 @@ along with this program.  If not, see <https://www.gnu.org/licenses/>.
 package fabricidentity
 
 import (
+	"context"
 	"errors"
+	"fmt"
 	"strings"
 	"testing"
+
+	ipamv1alpha1 "go.miloapis.com/ipam/pkg/apis/ipam/v1alpha1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
 )
 
 // The identity is the block's index within the pool, which is the 32 bits
@@ -132,3 +142,87 @@ func indexOf(haystack, needle string) int {
 func asUnusable(err error, target **UnusableError) bool {
 	return errors.As(err, target)
 }
+
+// The address service bounds a claim's prefix length by the family stated on
+// the claim, before it resolves the class the claim names. A /64 asked for
+// without a family is therefore read as an IPv4 length and refused, and no
+// network is ever given an identity.
+func TestClaimStatesTheFamilyTheBlockIsReadFrom(t *testing.T) {
+	scheme := runtime.NewScheme()
+	if err := ipamv1alpha1.AddToScheme(scheme); err != nil {
+		t.Fatalf("build the IPAM scheme: %v", err)
+	}
+
+	ipamClient := fake.NewClientBuilder().WithScheme(scheme).WithInterceptorFuncs(interceptor.Funcs{
+		Create: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.CreateOption) error {
+			ipClaim, ok := obj.(*ipamv1alpha1.IPClaim)
+			if !ok {
+				return c.Create(ctx, obj, opts...)
+			}
+			if err := admit(ipClaim); err != nil {
+				return err
+			}
+			ipClaim.Status.Phase = ipamv1alpha1.ClaimBound
+			ipClaim.Status.AllocatedCIDR = "fd30:0:0:1::/64"
+			return c.Create(ctx, obj, opts...)
+		},
+	}).Build()
+
+	request := Request{
+		ClassName:        "datum-fabric-identity",
+		Namespace:        "datum-cloud",
+		NetworkNamespace: "ns-project",
+		NetworkName:      "taptest",
+	}
+
+	identity, err := Claim(context.Background(), ipamClient, request)
+	if err != nil {
+		t.Fatalf("allocate an identity: %v", err)
+	}
+	if identity != 1 {
+		t.Fatalf("expected identity 1, got %d", identity)
+	}
+
+	var written ipamv1alpha1.IPClaim
+	key := client.ObjectKey{Namespace: request.Namespace, Name: ClaimName(request.NetworkNamespace, request.NetworkName)}
+	if err := ipamClient.Get(context.Background(), key, &written); err != nil {
+		t.Fatalf("read the claim back: %v", err)
+	}
+
+	if written.Spec.IPFamily != ipamv1alpha1.IPv6 {
+		t.Fatalf("the claim must state IPv6, got %q; without it the server bounds a /%d against IPv4",
+			written.Spec.IPFamily, BlockBits)
+	}
+	if written.Spec.ClassName != request.ClassName {
+		t.Fatalf("expected the claim to name class %q, got %q", request.ClassName, written.Spec.ClassName)
+	}
+	if written.Spec.PrefixLength == nil || *written.Spec.PrefixLength != BlockBits {
+		t.Fatalf("expected the claim to ask for a /%d, got %v", BlockBits, written.Spec.PrefixLength)
+	}
+}
+
+// admit mirrors the address service's own validation of a claim, which the fake
+// client does not do. Only the rule this depends on is modelled: the bound on
+// prefix length comes from the family stated on the claim, not from the class.
+func admit(ipClaim *ipamv1alpha1.IPClaim) error {
+	if ipClaim.Spec.ClassName == "" && ipClaim.Spec.IPFamily == "" {
+		return apierrors.NewInvalid(
+			ipamv1alpha1.SchemeGroupVersion.WithKind("IPClaim").GroupKind(), ipClaim.Name,
+			field.ErrorList{field.Required(field.NewPath("spec"), "one of className or ipFamily is required")})
+	}
+	p := ipClaim.Spec.PrefixLength
+	if p == nil {
+		return nil
+	}
+	maxLen := int32(32)
+	if ipClaim.Spec.IPFamily == ipamv1alpha1.IPv6 {
+		maxLen = 128
+	}
+	if *p <= 0 || *p > maxLen {
+		return apierrors.NewInvalid(
+			ipamv1alpha1.SchemeGroupVersion.WithKind("IPClaim").GroupKind(), ipClaim.Name,
+			field.ErrorList{field.Invalid(field.NewPath("spec", "prefixLength"), *p,
+				fmt.Sprintf("must be between 1 and %d", maxLen))})
+	}
+	return nil
+}