From 5a846f832622006f130dccd373a5e9c50e862f7b Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Thu, 20 Aug 2026 16:37:58 -0500 Subject: [PATCH] fix: encode base62 identifiers as hex in BGP CRD names Base62 (baseconv Digits62) encodes the value 36 as "A", and metadata.name must be a lowercase RFC 1123 subdomain, so BGPVRFInstanceName and BGPAdvertisementName produce names the API server rejects for any identifier carrying an uppercase character. Encode both name segments as the lowercase hex value the base62 identifier represents. Kernel interface names are untouched: base62 exists there for the 15-character limit. internal/gc's kernel-VRF-to-CRD join now encodes the VPC it parsed out of an interface name before matching, and tolerates both the pre-rename and current forms while CRDs written by an older node age out. Co-Authored-By: Claude Opus 5 (1M context) --- internal/crdnames/crdnames.go | 30 +++++++++-- internal/crdnames/crdnames_test.go | 81 ++++++++++++++++++++++++++++-- internal/gc/gc.go | 40 +++++++++++++-- internal/gc/gc_test.go | 43 ++++++++++++++++ 4 files changed, 182 insertions(+), 12 deletions(-) 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) + } +}