-
Notifications
You must be signed in to change notification settings - Fork 272
feat: TargetArch() utility and arch-aware path resolution #2258
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
20a4968
74d7e74
867d4c2
52e9646
e7543ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| package fc | ||
|
|
||
| import ( | ||
| "os" | ||
| "path/filepath" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/e2b-dev/infra/packages/orchestrator/pkg/cfg" | ||
| "github.com/e2b-dev/infra/packages/shared/pkg/utils" | ||
| ) | ||
|
|
||
| func TestFirecrackerPath_ArchPrefixed(t *testing.T) { | ||
| t.Parallel() | ||
| dir := t.TempDir() | ||
| arch := utils.TargetArch() | ||
|
|
||
| // Create the arch-prefixed binary | ||
| archDir := filepath.Join(dir, "v1.12.0", arch) | ||
| require.NoError(t, os.MkdirAll(archDir, 0o755)) | ||
| require.NoError(t, os.WriteFile(filepath.Join(archDir, "firecracker"), []byte("binary"), 0o755)) | ||
|
|
||
| config := cfg.BuilderConfig{FirecrackerVersionsDir: dir} | ||
| fc := Config{FirecrackerVersion: "v1.12.0"} | ||
|
|
||
| result := fc.FirecrackerPath(config) | ||
|
|
||
| assert.Equal(t, filepath.Join(dir, "v1.12.0", arch, "firecracker"), result) | ||
| } | ||
|
|
||
| func TestFirecrackerPath_LegacyFallback(t *testing.T) { | ||
| t.Parallel() | ||
| dir := t.TempDir() | ||
|
|
||
| // Only create the legacy flat binary (no arch subdirectory) | ||
| require.NoError(t, os.MkdirAll(filepath.Join(dir, "v1.12.0"), 0o755)) | ||
| require.NoError(t, os.WriteFile(filepath.Join(dir, "v1.12.0", "firecracker"), []byte("binary"), 0o755)) | ||
|
|
||
| config := cfg.BuilderConfig{FirecrackerVersionsDir: dir} | ||
| fc := Config{FirecrackerVersion: "v1.12.0"} | ||
|
|
||
| result := fc.FirecrackerPath(config) | ||
|
|
||
| assert.Equal(t, filepath.Join(dir, "v1.12.0", "firecracker"), result) | ||
| } | ||
|
|
||
| func TestFirecrackerPath_NeitherExists(t *testing.T) { | ||
| t.Parallel() | ||
| dir := t.TempDir() | ||
|
|
||
| // No binary at all — should return legacy flat path | ||
| config := cfg.BuilderConfig{FirecrackerVersionsDir: dir} | ||
| fc := Config{FirecrackerVersion: "v1.12.0"} | ||
|
|
||
| result := fc.FirecrackerPath(config) | ||
|
|
||
| assert.Equal(t, filepath.Join(dir, "v1.12.0", "firecracker"), result) | ||
| } | ||
|
|
||
| func TestHostKernelPath_ArchPrefixed(t *testing.T) { | ||
| t.Parallel() | ||
| dir := t.TempDir() | ||
| arch := utils.TargetArch() | ||
|
|
||
| // Create the arch-prefixed kernel | ||
| archDir := filepath.Join(dir, "vmlinux-6.1.102", arch) | ||
| require.NoError(t, os.MkdirAll(archDir, 0o755)) | ||
| require.NoError(t, os.WriteFile(filepath.Join(archDir, "vmlinux.bin"), []byte("kernel"), 0o644)) | ||
|
|
||
| config := cfg.BuilderConfig{HostKernelsDir: dir} | ||
| fc := Config{KernelVersion: "vmlinux-6.1.102"} | ||
|
|
||
| result := fc.HostKernelPath(config) | ||
|
|
||
| assert.Equal(t, filepath.Join(dir, "vmlinux-6.1.102", arch, "vmlinux.bin"), result) | ||
| } | ||
|
|
||
| func TestHostKernelPath_LegacyFallback(t *testing.T) { | ||
| t.Parallel() | ||
| dir := t.TempDir() | ||
|
|
||
| // Only create the legacy flat kernel | ||
| require.NoError(t, os.MkdirAll(filepath.Join(dir, "vmlinux-6.1.102"), 0o755)) | ||
| require.NoError(t, os.WriteFile(filepath.Join(dir, "vmlinux-6.1.102", "vmlinux.bin"), []byte("kernel"), 0o644)) | ||
|
|
||
| config := cfg.BuilderConfig{HostKernelsDir: dir} | ||
| fc := Config{KernelVersion: "vmlinux-6.1.102"} | ||
|
|
||
| result := fc.HostKernelPath(config) | ||
|
|
||
| assert.Equal(t, filepath.Join(dir, "vmlinux-6.1.102", "vmlinux.bin"), result) | ||
| } | ||
|
|
||
| func TestHostKernelPath_PrefersArchOverLegacy(t *testing.T) { | ||
| t.Parallel() | ||
| dir := t.TempDir() | ||
| arch := utils.TargetArch() | ||
|
|
||
| // Create BOTH arch-prefixed and legacy flat kernels | ||
| require.NoError(t, os.MkdirAll(filepath.Join(dir, "vmlinux-6.1.102", arch), 0o755)) | ||
| require.NoError(t, os.WriteFile(filepath.Join(dir, "vmlinux-6.1.102", arch, "vmlinux.bin"), []byte("arch-kernel"), 0o644)) | ||
| require.NoError(t, os.WriteFile(filepath.Join(dir, "vmlinux-6.1.102", "vmlinux.bin"), []byte("legacy-kernel"), 0o644)) | ||
|
|
||
| config := cfg.BuilderConfig{HostKernelsDir: dir} | ||
| fc := Config{KernelVersion: "vmlinux-6.1.102"} | ||
|
|
||
| result := fc.HostKernelPath(config) | ||
|
|
||
| // Should prefer the arch-prefixed path | ||
| assert.Equal(t, filepath.Join(dir, "vmlinux-6.1.102", arch, "vmlinux.bin"), result) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,40 @@ package utils | |
| import ( | ||
| "fmt" | ||
| "os" | ||
| "runtime" | ||
| "strings" | ||
| "sync" | ||
| ) | ||
|
|
||
| // archAliases normalizes common architecture names to Go convention. | ||
| var archAliases = map[string]string{ | ||
| "amd64": "amd64", | ||
| "x86_64": "amd64", | ||
| "arm64": "arm64", | ||
| "aarch64": "arm64", | ||
| } | ||
|
|
||
| var archWarningOnce sync.Once | ||
|
|
||
| // TargetArch returns the target architecture for binary paths and OCI platform. | ||
| // If TARGET_ARCH is set, it is normalized to Go convention ("amd64" or "arm64"); | ||
| // otherwise defaults to the host architecture (runtime.GOARCH). | ||
| func TargetArch() string { | ||
| if arch := os.Getenv("TARGET_ARCH"); arch != "" { | ||
| if normalized, ok := archAliases[arch]; ok { | ||
| return normalized | ||
| } | ||
|
|
||
| archWarningOnce.Do(func() { | ||
| fmt.Fprintf(os.Stderr, "WARNING: unrecognized TARGET_ARCH=%q, falling back to %s\n", arch, runtime.GOARCH) | ||
| }) | ||
|
Comment on lines
+20
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The archAliases map lookup in TargetArch() is case-sensitive, so TARGET_ARCH=AMD64 or TARGET_ARCH=ARM64 (uppercase, common in some CI/CD environments) fails the lookup, emits a misleading warning, and returns runtime.GOARCH instead of the intended architecture. On a cross-compilation host (e.g., arm64 node with TARGET_ARCH=AMD64), this silently selects the wrong architecture for kernel paths and OCI platform. Fix: normalize with strings.ToLower(arch) before the map lookup in env.go:25. Extended reasoning...What the bug is and how it manifests The TargetArch() function in packages/shared/pkg/utils/env.go reads the TARGET_ARCH environment variable and normalizes common aliases (e.g., x86_64->amd64, aarch64->arm64) via the archAliases map. However, all map keys are lowercase, so any uppercase or mixed-case value (e.g., AMD64, ARM64, X86_64) will fail the lookup. The specific code path that triggers it In env.go around line 25: When arch is "AMD64", the map lookup misses (key "AMD64" does not exist; only "amd64" does), the warning is printed, and runtime.GOARCH (the host arch) is returned. Why existing code does not prevent it The fix needs to be applied before the map lookup: archAliases[strings.ToLower(arch)]. The strings package is already imported in this file (used by RequiredEnv/OptionalEnv), so this is a trivial one-line change. Impact On a cross-compilation host -- for example, an arm64 CI runner with TARGET_ARCH=AMD64 to build amd64 binaries -- the function returns "arm64" instead of "amd64". This propagates to: (1) the Firecracker binary path (FirecrackerPath builds an arch-prefixed path using TargetArch()), (2) the kernel path (HostKernelPath), and (3) the OCI DefaultPlatform(). The operator does see a warning, so this is not completely silent -- but a WARNING to stderr is easy to overlook in noisy CI logs, and the downstream behavior (wrong arch binary, wrong OCI platform) is incorrect. Step-by-step proof
How to fix it Change env.go line 25 from: to: This makes TargetArch() case-insensitive and robustly handles the full set of real-world CI inputs.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't fix — Go/Docker/Debian arch conventions are always lowercase (amd64, arm64). Adding strings.ToLower would mask actual typos rather than surfacing them via the warning. |
||
|
|
||
| return runtime.GOARCH | ||
| } | ||
|
|
||
| return runtime.GOARCH | ||
| } | ||
|
|
||
| // RequiredEnv returns the value of the environment variable for key if it is set, non-empty and not only whitespace. | ||
| // It panics otherwise. | ||
| // | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| package utils | ||
|
|
||
| import ( | ||
| "runtime" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestTargetArch_DefaultsToHostArch(t *testing.T) { | ||
| t.Setenv("TARGET_ARCH", "") | ||
|
|
||
| result := TargetArch() | ||
|
|
||
| assert.Equal(t, runtime.GOARCH, result) | ||
| } | ||
|
|
||
| func TestTargetArch_RespectsValidOverride(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| arch string | ||
| expected string | ||
| }{ | ||
| {name: "amd64", arch: "amd64", expected: "amd64"}, | ||
| {name: "arm64", arch: "arm64", expected: "arm64"}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Setenv("TARGET_ARCH", tt.arch) | ||
|
|
||
| result := TargetArch() | ||
|
|
||
| assert.Equal(t, tt.expected, result) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestTargetArch_NormalizesAliases(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| arch string | ||
| expected string | ||
| }{ | ||
| {name: "x86_64 → amd64", arch: "x86_64", expected: "amd64"}, | ||
| {name: "aarch64 → arm64", arch: "aarch64", expected: "arm64"}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Setenv("TARGET_ARCH", tt.arch) | ||
|
|
||
| result := TargetArch() | ||
|
|
||
| assert.Equal(t, tt.expected, result) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestTargetArch_FallsBackOnUnknown(t *testing.T) { | ||
| t.Setenv("TARGET_ARCH", "mips") | ||
|
|
||
| result := TargetArch() | ||
|
|
||
| assert.Equal(t, runtime.GOARCH, result) | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.