From 447e8f94890748e8f3af58eb966b2fdf81962d58 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Mon, 8 Sep 2025 16:31:50 +1200 Subject: [PATCH 1/6] Allow custom filenames for arch-specific artifacts Take the base filename from the DEPLOY_ISO/DEPLOY_INITRD environment variables instead of hardcoding it. Assisted-By: Cursor --- pkg/imagehandler/imagehandler.go | 51 +++++++++++++++------- pkg/imagehandler/imagehandler_test.go | 63 +++++++++++++++++++-------- 2 files changed, 79 insertions(+), 35 deletions(-) diff --git a/pkg/imagehandler/imagehandler.go b/pkg/imagehandler/imagehandler.go index 1135fee3..c3a78b8a 100644 --- a/pkg/imagehandler/imagehandler.go +++ b/pkg/imagehandler/imagehandler.go @@ -19,7 +19,9 @@ import ( "net/url" "os" "path" + "path/filepath" "regexp" + "strings" "sync" "github.com/go-logr/logr" @@ -32,7 +34,34 @@ const ( hostArchitectureKey = "host" ) -var deployImagePattern = regexp.MustCompile(`ironic-python-agent_(\w+)\.(iso|initramfs)`) +// matchArchFilename attempts to match a target filename against a base filename. +// Returns "host" for exact matches, the architecture for pattern matches, or nil if no match. +func matchArchFilename(baseFilename, targetFilename string) *string { + if baseFilename == "" { + return nil + } + + if targetFilename == baseFilename { + arch := hostArchitectureKey + return &arch + } + + base := filepath.Base(baseFilename) + ext := filepath.Ext(base) + baseName := strings.TrimSuffix(base, ext) + + // Create pattern: basename_ARCH.extension + patternStr := fmt.Sprintf(`%s_(\w+)%s`, regexp.QuoteMeta(baseName), regexp.QuoteMeta(ext)) + pattern := regexp.MustCompile(patternStr) + + matches := pattern.FindStringSubmatch(filepath.Base(targetFilename)) + if len(matches) == 2 { + arch := matches[1] + return &arch + } + + return nil +} type ironicImage struct { filename string @@ -41,33 +70,23 @@ type ironicImage struct { } func parseDeployImage(envInputs *env.EnvInputs, filename string) (ironicImage, error) { - if filename == envInputs.DeployISO { + if arch := matchArchFilename(envInputs.DeployISO, filename); arch != nil { return ironicImage{ filename: filename, - arch: hostArchitectureKey, + arch: *arch, iso: true, }, nil } - if filename == envInputs.DeployInitrd { + if arch := matchArchFilename(envInputs.DeployInitrd, filename); arch != nil { return ironicImage{ filename: filename, - arch: hostArchitectureKey, + arch: *arch, iso: false, }, nil } - matches := deployImagePattern.FindStringSubmatch(filename) - - if len(matches) != 3 { - return ironicImage{}, fmt.Errorf("failed to parse ironic image name: %s", filename) - } - - return ironicImage{ - filename: filename, - arch: matches[1], - iso: matches[2] == "iso", - }, nil + return ironicImage{}, fmt.Errorf("failed to parse ironic image name: %s", filename) } type InvalidBaseImageError struct { diff --git a/pkg/imagehandler/imagehandler_test.go b/pkg/imagehandler/imagehandler_test.go index 037f23c1..f8587791 100644 --- a/pkg/imagehandler/imagehandler_test.go +++ b/pkg/imagehandler/imagehandler_test.go @@ -192,8 +192,8 @@ func TestNewImageHandlerStatic(t *testing.T) { func TestImagePattern(t *testing.T) { envInputs := &env.EnvInputs{ - DeployISO: "/shared/ironic-python-agent.iso", - DeployInitrd: "/shared/ironic-python-agent.initramfs", + DeployISO: "/config/ipa.iso", + DeployInitrd: "/config/ipa.initramfs", } tcs := []struct { @@ -204,36 +204,52 @@ func TestImagePattern(t *testing.T) { error bool }{ { - - name: "host iso", - filename: envInputs.DeployISO, + name: "host iso - exact path match", + filename: "/config/ipa.iso", arch: "host", iso: true, }, { - - name: "host initramfs", - filename: envInputs.DeployInitrd, + name: "host initramfs - exact path match", + filename: "/config/ipa.initramfs", arch: "host", }, { - - name: "host initramfs absolute path", - filename: "/shared/ironic-python-agent.initramfs", - arch: "host", + name: "aarch64 iso - same directory as config", + filename: "/config/ipa_aarch64.iso", + arch: "aarch64", + iso: true, }, { - - name: "aarch64 iso", - filename: "ironic-python-agent_aarch64.iso", + name: "aarch64 initramfs - different directory from config", + filename: "/images/ipa_aarch64.initramfs", arch: "aarch64", + }, + { + name: "x86_64 iso - different directory from config", + filename: "/images/ipa_x86_64.iso", + arch: "x86_64", iso: true, }, { - - name: "aarch64 initramfs", - filename: "ironic-python-agent_aarch64.initramfs", - arch: "aarch64", + name: "x86_64 initramfs - relative path", + filename: "ipa_x86_64.initramfs", + arch: "x86_64", + }, + { + name: "invalid filename - different base name", + filename: "/images/different-file_x86_64.iso", + error: true, + }, + { + name: "invalid filename - different base name with arch pattern", + filename: "different-file_aarch64.initramfs", + error: true, + }, + { + name: "invalid filename - no arch pattern", + filename: "some-other-file.iso", + error: true, }, } @@ -246,6 +262,15 @@ func TestImagePattern(t *testing.T) { return } + if err == nil && tc.error { + t.Errorf("expected error but got none") + return + } + + if tc.error { + continue + } + if ii.arch != tc.arch { t.Errorf("arch: expected %s but got %s", tc.arch, ii.arch) return From 0ad81e92c16d9a37008bb373952953e75d66b9be Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Mon, 8 Sep 2025 16:39:27 +1200 Subject: [PATCH 2/6] Allow . as arch separator Using a . instead of an _ is the standard for e.g. CoreOS images on the mirror. ABI uses agent.x86_64.iso. Since x86_64 already has an underscore in it, it may parse ambiguously in other contexts. Allow both so as not to break any existing code in development. Assisted-By: Cursor --- pkg/imagehandler/imagehandler.go | 4 ++-- pkg/imagehandler/imagehandler_test.go | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/pkg/imagehandler/imagehandler.go b/pkg/imagehandler/imagehandler.go index c3a78b8a..a987d127 100644 --- a/pkg/imagehandler/imagehandler.go +++ b/pkg/imagehandler/imagehandler.go @@ -50,8 +50,8 @@ func matchArchFilename(baseFilename, targetFilename string) *string { ext := filepath.Ext(base) baseName := strings.TrimSuffix(base, ext) - // Create pattern: basename_ARCH.extension - patternStr := fmt.Sprintf(`%s_(\w+)%s`, regexp.QuoteMeta(baseName), regexp.QuoteMeta(ext)) + // Create pattern: basename[_.]ARCH.extension + patternStr := fmt.Sprintf(`%s[_.](\w+)%s`, regexp.QuoteMeta(baseName), regexp.QuoteMeta(ext)) pattern := regexp.MustCompile(patternStr) matches := pattern.FindStringSubmatch(filepath.Base(targetFilename)) diff --git a/pkg/imagehandler/imagehandler_test.go b/pkg/imagehandler/imagehandler_test.go index f8587791..19281d4a 100644 --- a/pkg/imagehandler/imagehandler_test.go +++ b/pkg/imagehandler/imagehandler_test.go @@ -236,6 +236,17 @@ func TestImagePattern(t *testing.T) { filename: "ipa_x86_64.initramfs", arch: "x86_64", }, + { + name: "aarch64 iso - period separator", + filename: "ipa.aarch64.iso", + arch: "aarch64", + iso: true, + }, + { + name: "x86_64 initramfs - period separator", + filename: "/images/ipa.x86_64.initramfs", + arch: "x86_64", + }, { name: "invalid filename - different base name", filename: "/images/different-file_x86_64.iso", @@ -246,6 +257,11 @@ func TestImagePattern(t *testing.T) { filename: "different-file_aarch64.initramfs", error: true, }, + { + name: "invalid filename - different base name with period separator", + filename: "different-file.aarch64.initramfs", + error: true, + }, { name: "invalid filename - no arch pattern", filename: "some-other-file.iso", From 3d1b0b9b29dc0faf08af1991f8c2caba0aa51ddf Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Mon, 8 Sep 2025 21:08:11 +1200 Subject: [PATCH 3/6] Always find DEPLOY_ISO and DEPLOY_INITRD A regression from 583e13f963e7b47fdf8b92c908e65142d14a493b meant that the DEPLOY_ISO and DEPLOY_INITRD files would not be found unless they happened to be in the directory specified as IMAGE_SHARED_DIR (by default /shared/html/images). This change means that these files will always be found if specified, and that the directory/directories that contain them will be searched for arch-specific images unless the search directory is explicitly overridden by IMAGE_SHARED_DIR. Assisted-By: Cursor --- pkg/env/env.go | 2 +- pkg/imagehandler/imagehandler.go | 58 ++++++++--- pkg/imagehandler/imagehandler_test.go | 141 ++++++++++++++++++++++++++ 3 files changed, 187 insertions(+), 14 deletions(-) diff --git a/pkg/env/env.go b/pkg/env/env.go index b69486df..b73bc711 100644 --- a/pkg/env/env.go +++ b/pkg/env/env.go @@ -10,7 +10,7 @@ import ( type EnvInputs struct { DeployISO string `envconfig:"DEPLOY_ISO" required:"true"` DeployInitrd string `envconfig:"DEPLOY_INITRD" required:"true"` - ImageSharedDir string `envconfig:"IMAGE_SHARED_DIR" default:"/shared/html/images"` + ImageSharedDir string `envconfig:"IMAGE_SHARED_DIR"` IronicBaseURL string `envconfig:"IRONIC_BASE_URL"` IronicInspectorBaseURL string `envconfig:"IRONIC_INSPECTOR_BASE_URL"` IronicAgentImage string `envconfig:"IRONIC_AGENT_IMAGE" required:"true"` diff --git a/pkg/imagehandler/imagehandler.go b/pkg/imagehandler/imagehandler.go index a987d127..94eb22ea 100644 --- a/pkg/imagehandler/imagehandler.go +++ b/pkg/imagehandler/imagehandler.go @@ -122,34 +122,66 @@ type ImageHandler interface { RemoveImage(key string) } -func NewImageHandler(logger logr.Logger, baseURL *url.URL, envInputs *env.EnvInputs) (ImageHandler, error) { - imageFiles, err := os.ReadDir(envInputs.ImageSharedDir) +func findOSImageCandidates(logger logr.Logger, envInputs *env.EnvInputs, filePaths []string) []string { + var searchDirs []string - if err != nil { - return &imageFileSystem{}, err + if envInputs.ImageSharedDir != "" { + searchDirs = append(searchDirs, envInputs.ImageSharedDir) + + if filepath.Dir(envInputs.DeployISO) != envInputs.ImageSharedDir { + filePaths = append(filePaths, envInputs.DeployISO) + } + if filepath.Dir(envInputs.DeployInitrd) != envInputs.ImageSharedDir { + filePaths = append(filePaths, envInputs.DeployInitrd) + } + } else { + dirSet := make(map[string]bool) + dirSet[filepath.Dir(envInputs.DeployISO)] = true + dirSet[filepath.Dir(envInputs.DeployInitrd)] = true + + for dir := range dirSet { + searchDirs = append(searchDirs, dir) + } } + for _, searchDir := range searchDirs { + imageFiles, err := os.ReadDir(searchDir) + if err != nil { + logger.Info("failed to read directory, continuing", "dir", searchDir, "error", err) + continue + } + logger.Info("reading image files", "dir", searchDir, "len", len(imageFiles)) + for _, imageFile := range imageFiles { + fullPath := path.Join(searchDir, imageFile.Name()) + filePaths = append(filePaths, fullPath) + } + } + + return filePaths +} + +func NewImageHandler(logger logr.Logger, baseURL *url.URL, envInputs *env.EnvInputs) (ImageHandler, error) { + filePaths := findOSImageCandidates(logger, envInputs, nil) + isoFiles := map[string]*baseIso{} initramfsFiles := map[string]*baseInitramfs{} - logger.Info("reading image files", "dir", envInputs.ImageSharedDir, "len", len(imageFiles)) - for _, imageFile := range imageFiles { - filename := imageFile.Name() - - logger.Info("load image", "imageFile", imageFile.Name()) + logger.Info("processing image files", "total", len(filePaths)) + for _, filePath := range filePaths { + logger.Info("load image", "file", filePath) - ironicImage, err := parseDeployImage(envInputs, filename) + ironicImage, err := parseDeployImage(envInputs, filePath) if err != nil { - logger.Info("failed to parse ironic image, continuing") + logger.Info("failed to parse ironic image, continuing", "file", filePath) continue } logger.Info("image loaded", "filename", ironicImage.filename, "arch", ironicImage.arch, "iso", ironicImage.iso) if ironicImage.iso { - isoFiles[ironicImage.arch] = newBaseIso(path.Join(envInputs.ImageSharedDir, filename)) + isoFiles[ironicImage.arch] = newBaseIso(filePath) } else { - initramfsFiles[ironicImage.arch] = newBaseInitramfs(path.Join(envInputs.ImageSharedDir, filename)) + initramfsFiles[ironicImage.arch] = newBaseInitramfs(filePath) } } diff --git a/pkg/imagehandler/imagehandler_test.go b/pkg/imagehandler/imagehandler_test.go index 19281d4a..113f3df5 100644 --- a/pkg/imagehandler/imagehandler_test.go +++ b/pkg/imagehandler/imagehandler_test.go @@ -298,3 +298,144 @@ func TestImagePattern(t *testing.T) { } } } + +func TestImagePatternBaseImagesOutsideSharedDir(t *testing.T) { + // Test that base deploy images can be located anywhere, not just in ImageSharedDir + envInputs := &env.EnvInputs{ + DeployISO: "/config/base/ipa.iso", // Outside shared dir + DeployInitrd: "/config/base/ipa.initramfs", // Outside shared dir + ImageSharedDir: "/shared/images", // Different directory + } + + tcs := []struct { + name string + filename string + arch string + iso bool + error bool + }{ + { + name: "base iso outside shared dir", + filename: "/config/base/ipa.iso", + arch: "host", + iso: true, + }, + { + name: "base initramfs outside shared dir", + filename: "/config/base/ipa.initramfs", + arch: "host", + }, + { + name: "arch-specific iso in shared dir", + filename: "/shared/images/ipa_aarch64.iso", + arch: "aarch64", + iso: true, + }, + { + name: "arch-specific initramfs in shared dir", + filename: "/shared/images/ipa.x86_64.initramfs", + arch: "x86_64", + }, + } + + for _, tc := range tcs { + t.Logf("testing %s", tc.name) + ii, err := parseDeployImage(envInputs, tc.filename) + + if err != nil && !tc.error { + t.Errorf("got error: %v", err) + return + } + + if err == nil && tc.error { + t.Errorf("expected error but got none") + return + } + + if tc.error { + continue + } + + if ii.arch != tc.arch { + t.Errorf("arch: expected %s but got %s", tc.arch, ii.arch) + return + } + + if ii.iso != tc.iso { + t.Errorf("iso: expected %t but got %t", tc.iso, ii.iso) + return + } + } +} + +func TestImagePatternAutoDiscovery(t *testing.T) { + envInputs := &env.EnvInputs{ + DeployISO: "/config/base/ipa.iso", + DeployInitrd: "/opt/images/ipa.initramfs", + } + + tcs := []struct { + name string + filename string + arch string + iso bool + error bool + }{ + { + name: "base iso auto-discovery", + filename: "/config/base/ipa.iso", + arch: "host", + iso: true, + }, + { + name: "base initramfs auto-discovery", + filename: "/opt/images/ipa.initramfs", + arch: "host", + }, + { + name: "arch-specific iso in base iso directory", + filename: "/config/base/ipa_aarch64.iso", + arch: "aarch64", + iso: true, + }, + { + name: "arch-specific initramfs in base initramfs directory", + filename: "/opt/images/ipa.x86_64.initramfs", + arch: "x86_64", + }, + { + name: "invalid filename in auto-discovered directory", + filename: "/config/base/different-file_x86_64.iso", + error: true, + }, + } + + for _, tc := range tcs { + t.Logf("testing %s", tc.name) + ii, err := parseDeployImage(envInputs, tc.filename) + + if err != nil && !tc.error { + t.Errorf("got error: %v", err) + return + } + + if err == nil && tc.error { + t.Errorf("expected error but got none") + return + } + + if tc.error { + continue + } + + if ii.arch != tc.arch { + t.Errorf("arch: expected %s but got %s", tc.arch, ii.arch) + return + } + + if ii.iso != tc.iso { + t.Errorf("iso: expected %t but got %t", tc.iso, ii.iso) + return + } + } +} From 1310fa5adbc3338fe6aac8b6506359656442f84e Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Mon, 8 Sep 2025 21:26:41 +1200 Subject: [PATCH 4/6] Rename ironicImage to osImage The input images here are nothing to do with ironic, so choose a less confusing name. Assisted-By: Cursor --- pkg/imagehandler/imagehandler.go | 22 +++++++++++----------- pkg/imagehandler/imagehandler_test.go | 6 +++--- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/pkg/imagehandler/imagehandler.go b/pkg/imagehandler/imagehandler.go index 94eb22ea..b9b1fca0 100644 --- a/pkg/imagehandler/imagehandler.go +++ b/pkg/imagehandler/imagehandler.go @@ -63,15 +63,15 @@ func matchArchFilename(baseFilename, targetFilename string) *string { return nil } -type ironicImage struct { +type osImage struct { filename string arch string iso bool } -func parseDeployImage(envInputs *env.EnvInputs, filename string) (ironicImage, error) { +func loadOSImage(envInputs *env.EnvInputs, filename string) (osImage, error) { if arch := matchArchFilename(envInputs.DeployISO, filename); arch != nil { - return ironicImage{ + return osImage{ filename: filename, arch: *arch, iso: true, @@ -79,14 +79,14 @@ func parseDeployImage(envInputs *env.EnvInputs, filename string) (ironicImage, e } if arch := matchArchFilename(envInputs.DeployInitrd, filename); arch != nil { - return ironicImage{ + return osImage{ filename: filename, arch: *arch, iso: false, }, nil } - return ironicImage{}, fmt.Errorf("failed to parse ironic image name: %s", filename) + return osImage{}, fmt.Errorf("failed to load os image name: %s", filename) } type InvalidBaseImageError struct { @@ -170,18 +170,18 @@ func NewImageHandler(logger logr.Logger, baseURL *url.URL, envInputs *env.EnvInp for _, filePath := range filePaths { logger.Info("load image", "file", filePath) - ironicImage, err := parseDeployImage(envInputs, filePath) + osImage, err := loadOSImage(envInputs, filePath) if err != nil { - logger.Info("failed to parse ironic image, continuing", "file", filePath) + logger.Info("failed to load os image, continuing", "file", filePath) continue } - logger.Info("image loaded", "filename", ironicImage.filename, "arch", ironicImage.arch, "iso", ironicImage.iso) + logger.Info("image loaded", "filename", osImage.filename, "arch", osImage.arch, "iso", osImage.iso) - if ironicImage.iso { - isoFiles[ironicImage.arch] = newBaseIso(filePath) + if osImage.iso { + isoFiles[osImage.arch] = newBaseIso(filePath) } else { - initramfsFiles[ironicImage.arch] = newBaseInitramfs(filePath) + initramfsFiles[osImage.arch] = newBaseInitramfs(filePath) } } diff --git a/pkg/imagehandler/imagehandler_test.go b/pkg/imagehandler/imagehandler_test.go index 113f3df5..b7ea3fcb 100644 --- a/pkg/imagehandler/imagehandler_test.go +++ b/pkg/imagehandler/imagehandler_test.go @@ -271,7 +271,7 @@ func TestImagePattern(t *testing.T) { for _, tc := range tcs { t.Logf("testing %s", tc.name) - ii, err := parseDeployImage(envInputs, tc.filename) + ii, err := loadOSImage(envInputs, tc.filename) if err != nil && !tc.error { t.Errorf("got error: %v", err) @@ -340,7 +340,7 @@ func TestImagePatternBaseImagesOutsideSharedDir(t *testing.T) { for _, tc := range tcs { t.Logf("testing %s", tc.name) - ii, err := parseDeployImage(envInputs, tc.filename) + ii, err := loadOSImage(envInputs, tc.filename) if err != nil && !tc.error { t.Errorf("got error: %v", err) @@ -412,7 +412,7 @@ func TestImagePatternAutoDiscovery(t *testing.T) { for _, tc := range tcs { t.Logf("testing %s", tc.name) - ii, err := parseDeployImage(envInputs, tc.filename) + ii, err := loadOSImage(envInputs, tc.filename) if err != nil && !tc.error { t.Errorf("got error: %v", err) From 0323263c9a76024bf9a1ff6cd3299bc99fe79f5e Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Mon, 8 Sep 2025 23:01:40 +1200 Subject: [PATCH 5/6] Make use of host-architecture image If no architecture-specific images are provided, fall back to the host architecture image when the requested architecture is the host architecture. Assisted-By: Cursor --- pkg/env/env.go | 14 +++++++ pkg/imagehandler/imagehandler.go | 28 +++++++++++--- pkg/imagehandler/imagehandler_test.go | 55 +++++++++++++++++++++++++++ pkg/imageprovider/rhcos.go | 9 +---- 4 files changed, 93 insertions(+), 13 deletions(-) diff --git a/pkg/env/env.go b/pkg/env/env.go index b73bc711..7ff56db1 100644 --- a/pkg/env/env.go +++ b/pkg/env/env.go @@ -2,6 +2,7 @@ package env import ( "os" + "runtime" "github.com/kelseyhightower/envconfig" "github.com/pkg/errors" @@ -44,3 +45,16 @@ func (env *EnvInputs) RegistriesConf() (data []byte, err error) { } return } + +// HostArchitecture returns the standardized architecture name for the current host. +// Maps Go's GOARCH values to the architecture names used in image files. +func HostArchitecture() string { + switch runtime.GOARCH { + case "amd64": + return "x86_64" + case "arm64": + return "aarch64" + default: + return runtime.GOARCH + } +} diff --git a/pkg/imagehandler/imagehandler.go b/pkg/imagehandler/imagehandler.go index b9b1fca0..6e0fb124 100644 --- a/pkg/imagehandler/imagehandler.go +++ b/pkg/imagehandler/imagehandler.go @@ -206,13 +206,31 @@ func (f *imageFileSystem) getBaseImage(arch string, initramfs bool) baseFile { } f.log.Info("getBaseImage", "arch", arch, "initramfs", initramfs) - if initramfs { - file := f.initramfsFiles[arch] - return file - } else { - file := f.isoFiles[arch] + + getFile := func(arch string) baseFile { + if initramfs { + if file, exists := f.initramfsFiles[arch]; exists { + return file + } + } else { + if file, exists := f.isoFiles[arch]; exists { + return file + } + } + return nil + } + + if file := getFile(arch); file != nil { return file } + + if arch == env.HostArchitecture() { + if file := getFile(hostArchitectureKey); file != nil { + return file + } + } + + return nil } func (f *imageFileSystem) getNameForKey(key string) (name string, err error) { diff --git a/pkg/imagehandler/imagehandler_test.go b/pkg/imagehandler/imagehandler_test.go index b7ea3fcb..07ebc9a8 100644 --- a/pkg/imagehandler/imagehandler_test.go +++ b/pkg/imagehandler/imagehandler_test.go @@ -18,6 +18,8 @@ import ( "net/http" "net/http/httptest" "net/url" + "os" + "path/filepath" "strings" "sync" "testing" @@ -439,3 +441,56 @@ func TestImagePatternAutoDiscovery(t *testing.T) { } } } + +func TestArchitectureFallback(t *testing.T) { + tempDir := t.TempDir() + + envInputs := &env.EnvInputs{ + DeployISO: filepath.Join(tempDir, "ipa.iso"), + DeployInitrd: filepath.Join(tempDir, "ipa.initramfs"), + ImageSharedDir: tempDir, + } + + // Create host images only (no architecture-specific images) + err := os.WriteFile(envInputs.DeployISO, []byte("test iso"), 0644) + if err != nil { + t.Fatal(err) + } + err = os.WriteFile(envInputs.DeployInitrd, []byte("test initramfs"), 0644) + if err != nil { + t.Fatal(err) + } + + baseUrl, err := url.Parse("http://base.test:1234") + if err != nil { + t.Fatalf("unexpected error %v", err) + } + + logger := zap.New(zap.UseDevMode(true)) + handler, err := NewImageHandler(logger, baseUrl, envInputs) + if err != nil { + t.Fatal(err) + } + + // Get the host architecture that should trigger fallback + hostArch := env.HostArchitecture() + + // Test ISO fallback - should succeed because it falls back to host image + isoURL, err := handler.ServeImage("test-key", hostArch, []byte{}, false, false) + if err != nil { + t.Errorf("Expected ISO fallback to succeed for arch %s, got error: %v", hostArch, err) + } + if isoURL == "" { + t.Errorf("Expected ISO URL for arch %s, got empty string", hostArch) + } + + // Test initramfs fallback - should succeed because it falls back to host image + initramfsURL, err := handler.ServeImage("test-key-initramfs", hostArch, []byte{}, true, false) + if err != nil { + t.Errorf("Expected initramfs fallback to succeed for arch %s, got error: %v", hostArch, err) + } + if initramfsURL == "" { + t.Errorf("Expected initramfs URL for arch %s, got empty string", hostArch) + } + +} diff --git a/pkg/imageprovider/rhcos.go b/pkg/imageprovider/rhcos.go index 276303ca..8f6ba547 100644 --- a/pkg/imageprovider/rhcos.go +++ b/pkg/imageprovider/rhcos.go @@ -3,7 +3,6 @@ package imageprovider import ( "errors" "fmt" - "runtime" "strings" "github.com/go-logr/logr" @@ -28,13 +27,7 @@ func NewRHCOSImageProvider(imageServer imagehandler.ImageHandler, inputs *env.En panic(err) } - architecture := "" - switch runtime.GOARCH { - case "amd64": - architecture = "x86_64" - case "arm64": - architecture = "aarch64" - } + architecture := env.HostArchitecture() return &rhcosImageProvider{ ImageHandler: imageServer, From 4872cf8081c3826ec450d5751c374621b179b823 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Mon, 8 Sep 2025 23:21:52 +1200 Subject: [PATCH 6/6] Implement SupportsArchitecture() If we don't have images available for the requested architecture, return false from SupportsArchitecture(). This ensures that the correct error message is set in the controller, and avoids a segfault when we try to call ServeImage() with an invalid architecture. Assisted-By: Cursor --- cmd/static-server/main_test.go | 3 +- pkg/imagehandler/imagehandler.go | 5 ++ pkg/imagehandler/imagehandler_test.go | 79 ++++++++++++++++++++++++++- pkg/imageprovider/rhcos.go | 14 +---- 4 files changed, 85 insertions(+), 16 deletions(-) diff --git a/cmd/static-server/main_test.go b/cmd/static-server/main_test.go index 4e96b0bf..941ab4f8 100644 --- a/cmd/static-server/main_test.go +++ b/cmd/static-server/main_test.go @@ -46,7 +46,8 @@ func (f *fakeImageFileSystem) ServeImage(name string, arch string, ignitionConte f.imagesServed = append(f.imagesServed, name) return "", nil } -func (f *fakeImageFileSystem) RemoveImage(name string) {} +func (f *fakeImageFileSystem) RemoveImage(name string) {} +func (f *fakeImageFileSystem) HasImagesForArchitecture(arch string) bool { return true } func TestLoadStaticNMState(t *testing.T) { fifs := &fakeImageFileSystem{imagesServed: []string{}} diff --git a/pkg/imagehandler/imagehandler.go b/pkg/imagehandler/imagehandler.go index 6e0fb124..618f5131 100644 --- a/pkg/imagehandler/imagehandler.go +++ b/pkg/imagehandler/imagehandler.go @@ -120,6 +120,7 @@ type ImageHandler interface { FileSystem() http.FileSystem ServeImage(key string, arch string, ignitionContent []byte, initramfs, static bool) (string, error) RemoveImage(key string) + HasImagesForArchitecture(arch string) bool } func findOSImageCandidates(logger logr.Logger, envInputs *env.EnvInputs, filePaths []string) []string { @@ -233,6 +234,10 @@ func (f *imageFileSystem) getBaseImage(arch string, initramfs bool) baseFile { return nil } +func (f *imageFileSystem) HasImagesForArchitecture(arch string) bool { + return f.getBaseImage(arch, false) != nil && f.getBaseImage(arch, true) != nil +} + func (f *imageFileSystem) getNameForKey(key string) (name string, err error) { if img, exists := f.images[key]; exists { return img.name, nil diff --git a/pkg/imagehandler/imagehandler_test.go b/pkg/imagehandler/imagehandler_test.go index 07ebc9a8..5b746620 100644 --- a/pkg/imagehandler/imagehandler_test.go +++ b/pkg/imagehandler/imagehandler_test.go @@ -452,11 +452,11 @@ func TestArchitectureFallback(t *testing.T) { } // Create host images only (no architecture-specific images) - err := os.WriteFile(envInputs.DeployISO, []byte("test iso"), 0644) + err := os.WriteFile(envInputs.DeployISO, []byte("test iso"), 0600) if err != nil { t.Fatal(err) } - err = os.WriteFile(envInputs.DeployInitrd, []byte("test initramfs"), 0644) + err = os.WriteFile(envInputs.DeployInitrd, []byte("test initramfs"), 0600) if err != nil { t.Fatal(err) } @@ -475,6 +475,11 @@ func TestArchitectureFallback(t *testing.T) { // Get the host architecture that should trigger fallback hostArch := env.HostArchitecture() + // Test that host architecture is supported + if !handler.HasImagesForArchitecture(hostArch) { + t.Errorf("Expected HasImagesForArchitecture to return true for host architecture %s, but got false", hostArch) + } + // Test ISO fallback - should succeed because it falls back to host image isoURL, err := handler.ServeImage("test-key", hostArch, []byte{}, false, false) if err != nil { @@ -493,4 +498,74 @@ func TestArchitectureFallback(t *testing.T) { t.Errorf("Expected initramfs URL for arch %s, got empty string", hostArch) } + // Test that non-host architecture is not supported + nonHostArch := "some_other_arch" + if nonHostArch == hostArch { + nonHostArch = "definitely_not_host_arch" + } + + if handler.HasImagesForArchitecture(nonHostArch) { + t.Errorf("Expected HasImagesForArchitecture to return false for non-host architecture %s, but got true", nonHostArch) + } +} + +func TestHasImagesForArchitecture(t *testing.T) { + tempDir := t.TempDir() + + envInputs := &env.EnvInputs{ + DeployISO: filepath.Join(tempDir, "ipa.iso"), + DeployInitrd: filepath.Join(tempDir, "ipa.initramfs"), + ImageSharedDir: tempDir, + } + + // Create host images only (no architecture-specific images) + err := os.WriteFile(envInputs.DeployISO, []byte("test iso"), 0600) + if err != nil { + t.Fatal(err) + } + err = os.WriteFile(envInputs.DeployInitrd, []byte("test initramfs"), 0600) + if err != nil { + t.Fatal(err) + } + + // Create architecture-specific images for aarch64 + err = os.WriteFile(filepath.Join(tempDir, "ipa_aarch64.iso"), []byte("aarch64 iso"), 0600) + if err != nil { + t.Fatal(err) + } + err = os.WriteFile(filepath.Join(tempDir, "ipa_aarch64.initramfs"), []byte("aarch64 initramfs"), 0600) + if err != nil { + t.Fatal(err) + } + + baseUrl, err := url.Parse("http://base.test:1234") + if err != nil { + t.Fatalf("unexpected error %v", err) + } + + logger := zap.New(zap.UseDevMode(true)) + handler, err := NewImageHandler(logger, baseUrl, envInputs) + if err != nil { + t.Fatal(err) + } + + tests := []struct { + arch string + supported bool + desc string + }{ + {"aarch64", true, "aarch64 with both ISO and initramfs files"}, + {"ppc64le", false, "ppc64le with no files available"}, + {env.HostArchitecture(), true, "host architecture with fallback to host files"}, + {"unsupported_arch", false, "unsupported architecture"}, + } + + for _, tc := range tests { + t.Run(tc.desc, func(t *testing.T) { + supported := handler.HasImagesForArchitecture(tc.arch) + if supported != tc.supported { + t.Errorf("HasImagesForArchitecture(%s): expected %t, got %t", tc.arch, tc.supported, supported) + } + }) + } } diff --git a/pkg/imageprovider/rhcos.go b/pkg/imageprovider/rhcos.go index 8f6ba547..17f08895 100644 --- a/pkg/imageprovider/rhcos.go +++ b/pkg/imageprovider/rhcos.go @@ -18,7 +18,6 @@ type rhcosImageProvider struct { ImageHandler imagehandler.ImageHandler EnvInputs *env.EnvInputs RegistriesConf []byte - Architecture string } func NewRHCOSImageProvider(imageServer imagehandler.ImageHandler, inputs *env.EnvInputs) imageprovider.ImageProvider { @@ -27,26 +26,15 @@ func NewRHCOSImageProvider(imageServer imagehandler.ImageHandler, inputs *env.En panic(err) } - architecture := env.HostArchitecture() - return &rhcosImageProvider{ ImageHandler: imageServer, EnvInputs: inputs, RegistriesConf: registries, - Architecture: architecture, } } func (ip *rhcosImageProvider) SupportsArchitecture(arch string) bool { - if ip.Architecture == arch { - return true - } - - if ip.Architecture == "x86_64" && arch == "aarch64" { - return true - } - - return false + return ip.ImageHandler.HasImagesForArchitecture(arch) } func (ip *rhcosImageProvider) SupportsFormat(format metal3.ImageFormat) bool {