Skip to content

fix: base62 identifiers produce invalid Kubernetes object names - #437

Merged
scotwells merged 1 commit into
mainfrom
fix/name-safe-crd-identifiers
Aug 21, 2026
Merged

fix: base62 identifiers produce invalid Kubernetes object names#437
scotwells merged 1 commit into
mainfrom
fix/name-safe-crd-identifiers

Conversation

@scotwells

@scotwells scotwells commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

What breaks today

Galactic cannot attach a workload to a VPC whose identifier was generated rather than hand-picked. BGPVRFInstance and BGPAdvertisement names interpolate the base62 VPC/attachment identifiers straight into metadata.name, and base62 (baseconv Digits62) encodes the value 36 as A. Kubernetes object names must be lowercase RFC 1123 subdomains, so the API server rejects the CRD and the CNI ADD fails.

Nothing has hit this because every identifier in the tree is small and hand-written (10, 20, 21). Randomly generated 48-bit VPC identifiers are a different story — roughly 99% of them contain at least one uppercase character (f2a752e6b439 renders as 1dLaEmCAp), and a controller in datum-cloud/cloud is about to start generating them. This breaks on the first one.

The fix

Both name helpers encode each identifier as the lowercase hex value it represents:

BGPVRFInstanceName("1dLaEmCAp", "dfw-worker")   →  f2a752e6b439-dfw-worker
BGPAdvertisementName("1dLaEmCAp", "2Bc")        →  f2a752e6b439-270a

Why hex rather than sanitizing base62. Hex is what these identifiers already are everywhere above the kernel — intf.HexToBase62 takes hex in, and base62 exists solely to fit G%09s%03s%s inside the kernel's 15-character interface-name limit, which metadata.name does not have. It is lowercase by construction, contains no - to confuse the VPC-segment parsing in internal/gc, and a name can be read straight back against the identifier the API published. Lowercasing base62 instead would collapse A (36) onto a (10) — two different VPCs, one name.

Kernel interface names are unchanged. This PR only touches metadata.name.

An identifier that is not valid base62 at all (nothing in production produces one) is hashed under an x prefix rather than emitted as an invalid name.

Garbage collection

internal/gc decides whether a kernel VRF is orphaned by matching the base62 VPC in its interface name against the VPC segment of that node's BGPAdvertisement names — a cross-domain join that would silently start deleting live VRFs if only one side changed. Both sides now agree, and during an upgrade a VPC is matched under either form, so a CRD written by a not-yet-upgraded node keeps its VRF alive.

What operators need to do

Nothing blocking, and nothing in config/ or deploy/ names these objects.

Existing BGPAdvertisement/BGPVRFInstance objects keep their old names — they are not renamed in place (a name change is a delete-and-recreate). Live pods keep working: their advertisements stay valid, and galactic-router's GC collects the old objects once the pods that created them are gone. A rolling node drain, or deleting the stale objects by hand, makes that immediate. The same precedent exists in legacyVRFNameRegex, which is how this repo handled its previous VRF rename.

Tests

New unit tests assert both helpers produce names passing validation.IsDNS1123Subdomain for identifiers that contain uppercase (including a real random 48-bit VPC rendering), that the two helpers agree on the VPC segment for a node name containing -, that A and a stay distinct, and that the kernel-VRF-to-CRD join in internal/gc holds across the upgrade window.

go build ./... and go test ./internal/... were run in a Linux container. Everything in scope passes; the only failures are the pre-existing environmental ones (tests needing /sys/fs/bpf, the vrf module, iptables or netns creation), which fail identically on an unmodified checkout. golangci-lint run is clean for the touched packages. task test:e2e (Kind) was not run.

Related

Unblocks generated VPC identifiers, which #17 and #197 both lead to — today every identifier in the tree is hand-picked and small, which is the only reason this has not fired. The first consumer is the VPC controller in datum-cloud/cloud#6.

Not affected by this PR: #332, #329 and #328 concern kernel and allocation state, and kernel interface names are unchanged here.

🤖 Generated with Claude Code

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) <noreply@anthropic.com>
@scotwells
scotwells requested a review from privateip August 21, 2026 01:17
@scotwells
scotwells marked this pull request as ready for review August 21, 2026 01:17
@scotwells
scotwells requested a review from a team as a code owner August 21, 2026 01:17
@scotwells
scotwells merged commit 7294481 into main Aug 21, 2026
11 checks passed
@scotwells
scotwells deleted the fix/name-safe-crd-identifiers branch August 21, 2026 01:21
privateip added a commit that referenced this pull request Aug 21, 2026
Rebasing this branch onto main picked up #437's base62-to-hex CRD-name
encoding, which required updating TestBGPAdvertisementName/
TestTenantIdentifier's expected values in the same conflict resolution.
That reintroduced the literal "00G" a third time across this file,
tripping golangci-lint's goconst check. Add testAttachmentBase62
alongside the existing testVPC/testVPCBase62/testAttachment fixture
constants and use it at both sites.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants