diff --git a/internal/controller/networkfabricidentity_controller_test.go b/internal/controller/networkfabricidentity_controller_test.go index bf9df30..387b213 100644 --- a/internal/controller/networkfabricidentity_controller_test.go +++ b/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) diff --git a/internal/fabricidentity/identity.go b/internal/fabricidentity/identity.go index 506a030..9a8b5e9 100644 --- a/internal/fabricidentity/identity.go +++ b/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)), diff --git a/internal/fabricidentity/identity_test.go b/internal/fabricidentity/identity_test.go index 0e45974..395c506 100644 --- a/internal/fabricidentity/identity_test.go +++ b/internal/fabricidentity/identity_test.go @@ -18,9 +18,19 @@ along with this program. If not, see . 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 +}