fix: base62 identifiers produce invalid Kubernetes object names

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

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

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

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