diff --git a/internal/crdnames/crdnames.go b/internal/crdnames/crdnames.go index 6a8eda96..49aadded 100644 --- a/internal/crdnames/crdnames.go +++ b/internal/crdnames/crdnames.go @@ -13,8 +13,12 @@ package crdnames import ( + "crypto/sha256" + "encoding/hex" "fmt" "strings" + + "go.datum.net/galactic/internal/plumbing/intf" ) // AnnotationAllocatedSubnetIPv6 is the BGPAdvertisement annotation key prefix @@ -92,6 +96,25 @@ func NetNSKey(containerID string) string { return fmt.Sprintf("%s.%s", AnnotationNetNS, truncate(containerID)) } +// nameSegmentHashLen is the number of hex characters kept from the SHA-256 fallback. +const nameSegmentHashLen = 12 + +// nameSegment renders a base62 identifier as the lowercase hex value it encodes, since +// metadata.name must be a lowercase RFC 1123 subdomain and base62 turns uppercase at 36. +// Input that is not valid base62 is hashed under an "x" prefix no hex encoding can produce. +func nameSegment(id string) string { + if encoded, err := intf.Base62ToHex(id); err == nil && encoded != "" { + return encoded + } + sum := sha256.Sum256([]byte(id)) + return "x" + hex.EncodeToString(sum[:])[:nameSegmentHashLen] +} + +// VPCSegment returns the leading segment every BGP CRD name starts with for a base62 VPC. +func VPCSegment(vpc string) string { + return nameSegment(vpc) +} + // BGPVRFInstanceName returns the deterministic name for a BGPVRFInstance. // Unlike BGPAdvertisementName, this is keyed by (vpc, node) rather than // (vpc, vpcAttachment): the underlying kernel VRF is shared by every @@ -102,14 +125,15 @@ func NetNSKey(containerID string) string { // all — the kernel side never does, since interface names only need to be // unique within one host's own namespace. func BGPVRFInstanceName(vpc, nodeName string) string { - return fmt.Sprintf("%s-%s", vpc, nodeName) + return fmt.Sprintf("%s-%s", VPCSegment(vpc), nodeName) } // BGPAdvertisementName returns the deterministic name for a // BGPAdvertisement. Each VPCAttachment is unique per interface across the -// cluster, so the (vpc, vpcAttachment) pair is a reliable 1:1 key. +// cluster, so the (vpc, vpcAttachment) pair is a reliable 1:1 key. Both segments +// are encoded by nameSegment, the VPC one identically to BGPVRFInstanceName. func BGPAdvertisementName(vpc, vpcAttachment string) string { - return fmt.Sprintf("%s-%s", vpc, vpcAttachment) + return fmt.Sprintf("%s-%s", VPCSegment(vpc), nameSegment(vpcAttachment)) } // vipNameReplacer sanitizes an IP address for use inside a Kubernetes diff --git a/internal/crdnames/crdnames_test.go b/internal/crdnames/crdnames_test.go index 748442c9..8d52c30e 100644 --- a/internal/crdnames/crdnames_test.go +++ b/internal/crdnames/crdnames_test.go @@ -7,6 +7,8 @@ package crdnames import ( "strings" "testing" + + "k8s.io/apimachinery/pkg/util/validation" ) func TestServiceVIPBindingName(t *testing.T) { @@ -34,10 +36,13 @@ func TestServiceVIPBindingName(t *testing.T) { } } +// testVPCBase62 is base62 for 1234, padded as an interface name carries it. +const testVPCBase62 = "0000000jU" + func TestBGPVRFInstanceName(t *testing.T) { tests := []struct{ vpc, nodeName, want string }{ - {"abc", "worker-1", "abc-worker-1"}, - {"0000000jU", "dfw-worker", "0000000jU-dfw-worker"}, + {"abc", "worker-1", "98de-worker-1"}, + {testVPCBase62, "dfw-worker", "4d2-dfw-worker"}, } for _, tt := range tests { got := BGPVRFInstanceName(tt.vpc, tt.nodeName) @@ -63,8 +68,8 @@ func TestBGPVRFInstanceNameSharedAcrossAttachments(t *testing.T) { func TestBGPAdvertisementName(t *testing.T) { tests := []struct{ vpc, attachment, want string }{ - {"abc", "def", "abc-def"}, - {"0000000jU", "00G", "0000000jU-00G"}, + {"abc", "def", "98de-c6a7"}, + {testVPCBase62, "00G", "4d2-2a"}, } for _, tt := range tests { got := BGPAdvertisementName(tt.vpc, tt.attachment) @@ -111,3 +116,71 @@ func TestAnnotationKeyNameLength(t *testing.T) { }) } } + +// TestCRDNamesAreValidObjectNames covers the reason nameSegment exists: base62 +// (baseconv Digits62) encodes the value 36 as "A", so a realistic randomly +// generated 48-bit VPC identifier almost always carries uppercase, and +// metadata.name must be a lowercase RFC 1123 subdomain. +func TestCRDNamesAreValidObjectNames(t *testing.T) { + // Real base62 renderings of 48-bit VPC identifiers, plus the smallest + // values that produce an uppercase character at all. + vpcs := []string{"1dLaEmCAp", testVPCBase62, "A", "zZ", "ZZZZZZZZZ", "10"} + attachments := []string{"A", "00G", "ZZZ", "20"} + nodes := []string{"dfw-worker", "iad-worker-control-plane"} + + for _, vpc := range vpcs { + for _, node := range nodes { + assertValidObjectName(t, "BGPVRFInstanceName", BGPVRFInstanceName(vpc, node)) + } + for _, att := range attachments { + assertValidObjectName(t, "BGPAdvertisementName", BGPAdvertisementName(vpc, att)) + } + } +} + +// TestNameSegmentFallbackIsValid covers identifiers that are not valid base62 +// at all (nothing in production should produce one, but a name must never be +// rejected by the API server because of it). +func TestNameSegmentFallbackIsValid(t *testing.T) { + for _, id := range []string{"vpc-other", "", "attach-a", "Not/Base62"} { + got := nameSegment(id) + assertValidObjectName(t, "nameSegment", got+"-node") + if got != nameSegment(id) { + t.Errorf("nameSegment(%q) is not deterministic", id) + } + if !strings.HasPrefix(got, "x") { + t.Errorf("nameSegment(%q) = %q, want the non-base62 fallback prefix %q", id, got, "x") + } + } +} + +// TestNameSegmentDistinguishesCase guards the property a plain strings.ToLower +// would lose: base62 "A" is 36 and "a" is 10, two different identifiers. +func TestNameSegmentDistinguishesCase(t *testing.T) { + if nameSegment("A") == nameSegment("a") { + t.Errorf("nameSegment collapsed distinct base62 identifiers %q and %q onto %q", "A", "a", nameSegment("A")) + } +} + +// TestVPCSegmentSharedByBothNames is what internal/gc's orphan rule depends on: +// it matches a BGPVRFInstance against the BGPAdvertisements of the same VPC by +// cutting each name at the first '-', so both helpers must encode the VPC +// identically — including when the node name itself contains '-'. +func TestVPCSegmentSharedByBothNames(t *testing.T) { + const vpc, att, node = "1dLaEmCAp", "2Bc", "dfw-worker-control" + advVPC, _, _ := strings.Cut(BGPAdvertisementName(vpc, att), "-") + vrfVPC, _, _ := strings.Cut(BGPVRFInstanceName(vpc, node), "-") + if advVPC != vrfVPC || advVPC != VPCSegment(vpc) { + t.Errorf("VPC segments disagree: advertisement %q, VRF instance %q, VPCSegment %q", advVPC, vrfVPC, VPCSegment(vpc)) + } +} + +func assertValidObjectName(t *testing.T, who, name string) { + t.Helper() + if errs := validation.IsDNS1123Subdomain(name); len(errs) > 0 { + t.Errorf("%s produced %q, which is not a valid Kubernetes object name: %v", who, name, errs) + } + if name != strings.ToLower(name) { + t.Errorf("%s produced %q, which contains uppercase characters", who, name) + } +} diff --git a/internal/gc/gc.go b/internal/gc/gc.go index 833f40c9..e72d6afc 100644 --- a/internal/gc/gc.go +++ b/internal/gc/gc.go @@ -18,6 +18,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" + "go.datum.net/galactic/internal/crdnames" "go.datum.net/galactic/internal/plumbing/ebpf/nptv6map" "go.datum.net/galactic/internal/plumbing/ebpf/uformat" "go.datum.net/galactic/internal/plumbing/ebpf/usidmap" @@ -131,6 +132,35 @@ func vpcFromName(name string) string { return vpc } +// vpcKeys returns every form a VPC may appear as in a CRD name: the value +// itself, plus its encoded form (crdnames.VPCSegment). CRD names written +// before the encoding change carry the raw base62 VPC, which is also what a +// kernel VRF name yields, so both have to join for as long as either exists. +func vpcKeys(vpc string) []string { + encoded := crdnames.VPCSegment(vpc) + if encoded == vpc { + return []string{vpc} + } + return []string{vpc, encoded} +} + +// addVPC records every key form of vpc in set. +func addVPC(set map[string]struct{}, vpc string) { + for _, key := range vpcKeys(vpc) { + set[key] = struct{}{} + } +} + +// vpcInSet reports whether any key form of vpc is present in set. +func vpcInSet(set map[string]struct{}, vpc string) bool { + for _, key := range vpcKeys(vpc) { + if _, ok := set[key]; ok { + return true + } + } + return false +} + // CollectOrphanedCRDs scans BGPAdvertisement and BGPVRFInstance CRDs owned by // nodeName's BGPRouter(s) in the given namespace and returns those whose // associated container(s)/attachment(s) no longer exist on this node. @@ -180,7 +210,7 @@ func CollectOrphanedCRDs(ctx context.Context, k8s client.Client, namespace, node // No netns annotations — skip (might be legacy or manually // created). We cannot determine if it is orphaned, so its VPC // must be treated as surviving. - vpcSurvives[vpc] = struct{}{} + addVPC(vpcSurvives, vpc) continue } @@ -194,7 +224,7 @@ func CollectOrphanedCRDs(ctx context.Context, k8s client.Client, namespace, node if liveContainerID != "" { // At least one container that attached to this // vpc/vpcAttachment is still alive — not orphaned. - vpcSurvives[vpc] = struct{}{} + addVPC(vpcSurvives, vpc) continue } @@ -230,7 +260,7 @@ func CollectOrphanedCRDs(ctx context.Context, k8s client.Client, namespace, node if _, ownedByThisNode := routerNames[inst.Spec.RouterRef.Name]; !ownedByThisNode { continue } - if _, survives := vpcSurvives[vpcFromName(inst.Name)]; survives { + if vpcInSet(vpcSurvives, vpcFromName(inst.Name)) { continue } orphaned = append(orphaned, OrphanedCRD{ @@ -332,7 +362,7 @@ func CollectOrphanedVRFs(ctx context.Context, k8s client.Client, namespace, node if _, ownedByThisNode := routerNames[adv.Spec.RouterRef.Name]; !ownedByThisNode { continue } - activeVPCs[vpcFromName(adv.Name)] = struct{}{} + addVPC(activeVPCs, vpcFromName(adv.Name)) } var orphaned []string @@ -343,7 +373,7 @@ func CollectOrphanedVRFs(ctx context.Context, k8s client.Client, namespace, node continue } - if _, exists := activeVPCs[vpc]; !exists { + if !vpcInSet(activeVPCs, vpc) { orphaned = append(orphaned, v.Name) } } diff --git a/internal/gc/gc_test.go b/internal/gc/gc_test.go index c5533893..2e97ab7d 100644 --- a/internal/gc/gc_test.go +++ b/internal/gc/gc_test.go @@ -9,6 +9,8 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "go.datum.net/galactic/internal/crdnames" + "go.datum.net/galactic/internal/plumbing/intf" bgpv1alpha1 "go.datum.net/network/api/v1alpha1" ) @@ -294,3 +296,44 @@ func TestVPCFromName(t *testing.T) { }) } } + +// TestVRFNameJoinsCRDNames pins the join CollectOrphanedVRFs makes: the base62 +// VPC recovered from a kernel VRF interface name has to encode to the same +// segment the BGP CRD names for that VPC start with, whether or not the +// interface name carried the template's zero padding. +func TestVRFNameJoinsCRDNames(t *testing.T) { + for _, vpc := range []string{"1dLaEmCAp", "0000000jU"} { + parsed, ok := vpcFromVRFName(intf.GenerateInterfaceNameVRF(vpc)) + if !ok { + t.Fatalf("vpcFromVRFName(%q) did not match", intf.GenerateInterfaceNameVRF(vpc)) + } + advVPC := vpcFromName(crdnames.BGPAdvertisementName(vpc, "2Bc")) + if got := crdnames.VPCSegment(parsed); got != advVPC { + t.Errorf("kernel VRF for VPC %q resolves to %q, but its BGPAdvertisements are named after %q", vpc, got, advVPC) + } + } +} + +// TestVPCKeysJoinLegacyAndCurrentNames covers the upgrade window in which one +// VPC has CRDs named both before and after the name encoding changed: neither +// side may look orphaned to the other. +func TestVPCKeysJoinLegacyAndCurrentNames(t *testing.T) { + const vpc = "10" // base62, as a pre-rename CRD name and a kernel VRF name carry it + encoded := crdnames.VPCSegment(vpc) + + legacy := map[string]struct{}{} + addVPC(legacy, vpc) + if !vpcInSet(legacy, encoded) { + t.Errorf("a current-named CRD (%q) does not join a pre-rename one (%q)", encoded, vpc) + } + + current := map[string]struct{}{} + addVPC(current, encoded) + if !vpcInSet(current, vpc) { + t.Errorf("a pre-rename CRD (%q) does not join a current-named one (%q)", vpc, encoded) + } + + if vpcInSet(current, "zz") { + t.Errorf("unrelated VPC %q matched the set for %q", "zz", vpc) + } +}