From f590da491605011c0c2ebd6a6878052cd9f500ca Mon Sep 17 00:00:00 2001 From: Christian Goll Date: Thu, 11 Dec 2025 15:25:56 +0100 Subject: [PATCH] fix wwctl image import --update --- CHANGELOG.md | 1 + internal/app/wwctl/image/imprt/main_test.go | 288 ++++++++++++++++++++ internal/app/wwctl/image/imprt/root.go | 4 +- internal/pkg/api/image/image.go | 3 +- 4 files changed, 293 insertions(+), 3 deletions(-) create mode 100644 internal/app/wwctl/image/imprt/main_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index bd8fb98c..dca287f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Use device names in netplan bonds. #2013 - Fix ImageDelete API not returning error when checking if image is used by nodes/profiles - Fix warewulf-dracut to not run the wwinit module if root is not set to `root=wwclient*` +- Fix `wwctl image import --update` #2066 ### Dependencies diff --git a/internal/app/wwctl/image/imprt/main_test.go b/internal/app/wwctl/image/imprt/main_test.go new file mode 100644 index 00000000..0161f3d4 --- /dev/null +++ b/internal/app/wwctl/image/imprt/main_test.go @@ -0,0 +1,288 @@ +package imprt + +import ( + "archive/tar" + "bytes" + "crypto/sha256" + "encoding/json" + "fmt" + "os" + "strings" + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/warewulf/warewulf/internal/pkg/testenv" + "github.com/warewulf/warewulf/internal/pkg/util" +) + +func createDummyDockerArchive(t *testing.T, path string) { + // 1. Create layer content + var layerBuf bytes.Buffer + tw := tar.NewWriter(&layerBuf) + + // Add "." directory + if err := tw.WriteHeader(&tar.Header{ + Name: ".", + Typeflag: tar.TypeDir, + Mode: 0755, + Uid: os.Getuid(), + Gid: os.Getgid(), + }); err != nil { + t.Fatal(err) + } + + // Add "bin" directory + if err := tw.WriteHeader(&tar.Header{ + Name: "bin", + Typeflag: tar.TypeDir, + Mode: 0755, + Uid: os.Getuid(), + Gid: os.Getgid(), + }); err != nil { + t.Fatal(err) + } + + // Add /bin/sh which is often checked/needed + hdr := &tar.Header{ + Name: "bin/sh", + Mode: 0755, + Size: int64(len("shell")), + Uid: os.Getuid(), + Gid: os.Getgid(), + } + if err := tw.WriteHeader(hdr); err != nil { + t.Fatal(err) + } + if _, err := tw.Write([]byte("shell")); err != nil { + t.Fatal(err) + } + if err := tw.Close(); err != nil { + t.Fatal(err) + } + layerBytes := layerBuf.Bytes() + + // Calculate DiffID (SHA256 of uncompressed layer) + layerSHA := sha256.Sum256(layerBytes) + diffID := fmt.Sprintf("sha256:%x", layerSHA) + + // 2. config.json + configStruct := struct { + Architecture string `json:"architecture"` + OS string `json:"os"` + RootFS struct { + Type string `json:"type"` + DiffIDs []string `json:"diff_ids"` + } `json:"rootfs"` + }{ + Architecture: "amd64", + OS: "linux", + } + configStruct.RootFS.Type = "layers" + configStruct.RootFS.DiffIDs = []string{diffID} + + configJSON, err := json.Marshal(configStruct) + if err != nil { + t.Fatal(err) + } + + // 3. manifest.json + manifestStruct := []struct { + Config string `json:"Config"` + RepoTags []string `json:"RepoTags"` + Layers []string `json:"Layers"` + }{ + { + Config: "config.json", + RepoTags: []string{"test:latest"}, + Layers: []string{"layer.tar"}, + }, + } + manifestJSON, err := json.Marshal(manifestStruct) + if err != nil { + t.Fatal(err) + } + + // 4. Create the tarball + f, err := os.Create(path) + if err != nil { + t.Fatal(err) + } + defer f.Close() + + archiveTw := tar.NewWriter(f) + + // Write layer.tar + if err := archiveTw.WriteHeader(&tar.Header{Name: "layer.tar", Size: int64(len(layerBytes))}); err != nil { + t.Fatal(err) + } + if _, err := archiveTw.Write(layerBytes); err != nil { + t.Fatal(err) + } + + // Write config.json + if err := archiveTw.WriteHeader(&tar.Header{Name: "config.json", Size: int64(len(configJSON))}); err != nil { + t.Fatal(err) + } + if _, err := archiveTw.Write(configJSON); err != nil { + t.Fatal(err) + } + + // Write manifest.json + if err := archiveTw.WriteHeader(&tar.Header{Name: "manifest.json", Size: int64(len(manifestJSON))}); err != nil { + t.Fatal(err) + } + if _, err := archiveTw.Write(manifestJSON); err != nil { + t.Fatal(err) + } + + if err := archiveTw.Close(); err != nil { + t.Fatal(err) + } +} + +func resetFlags() { + SetUpdate = false + SetForce = false + SetBuild = false + SyncUser = false +} + +func Test_CobraRunE_Import(t *testing.T) { + t.Run("Basic Import", func(t *testing.T) { + resetFlags() + env := testenv.New(t) + defer env.RemoveAll() + + archivePath := env.GetPath("test-image.tar") + createDummyDockerArchive(t, archivePath) + + args := []string{"file://" + archivePath, "test-image"} + err := CobraRunE(&cobra.Command{}, args) + + if err != nil { + if strings.Contains(err.Error(), "operation not permitted") || strings.Contains(err.Error(), "chown") { + t.Logf("Caught expected error in unprivileged environment: %v", err) + return + } + t.Fatalf("Unexpected error: %v", err) + } + + assert.True(t, util.IsDir(env.GetPath("var/lib/warewulf/chroots/test-image/rootfs")), "rootfs should exist") + }) + + t.Run("Import With Name Arg", func(t *testing.T) { + resetFlags() + env := testenv.New(t) + defer env.RemoveAll() + + archivePath := env.GetPath("test-image.tar") + createDummyDockerArchive(t, archivePath) + + args := []string{"file://" + archivePath, "custom-name"} + err := CobraRunE(&cobra.Command{}, args) + + if err != nil { + if strings.Contains(err.Error(), "operation not permitted") || strings.Contains(err.Error(), "chown") { + t.Logf("Caught expected error in unprivileged environment: %v", err) + return + } + t.Fatalf("Unexpected error: %v", err) + } + + assert.True(t, util.IsDir(env.GetPath("var/lib/warewulf/chroots/custom-name/rootfs")), "rootfs with custom name should exist") + }) + + t.Run("Import Existing No Update", func(t *testing.T) { + resetFlags() + env := testenv.New(t) + defer env.RemoveAll() + + archivePath := env.GetPath("test-image.tar") + createDummyDockerArchive(t, archivePath) + + // Pre-create the chroot directory to simulate existing image + env.MkdirAll("var/lib/warewulf/chroots/existing-image/rootfs") + + args := []string{"file://" + archivePath, "existing-image"} + err := CobraRunE(&cobra.Command{}, args) + + assert.Error(t, err) + assert.Contains(t, err.Error(), "exists") + assert.Contains(t, err.Error(), "specify --force, --update") + }) + t.Run("Import Existing force", func(t *testing.T) { + resetFlags() + env := testenv.New(t) + defer env.RemoveAll() + + archivePath := env.GetPath("test-image.tar") + createDummyDockerArchive(t, archivePath) + + // Pre-create the chroot directory to simulate existing image + env.MkdirAll("var/lib/warewulf/chroots/existing-image/rootfs") + + args := []string{"file://" + archivePath, "--force", "existing-image"} + err := CobraRunE(&cobra.Command{}, args) + if err != nil { + if strings.Contains(err.Error(), "operation not permitted") || strings.Contains(err.Error(), "chown") { + t.Logf("Caught expected error in unprivileged environment: %v", err) + return + } + t.Fatalf("Unexpected error: %v", err) + } + + assert.True(t, util.IsDir(env.GetPath("var/lib/warewulf/chroots/existing-image/rootfs")), "rootfs with custom name should exist") + }) + + t.Run("Import Existing With Update", func(t *testing.T) { + resetFlags() + SetUpdate = true + + env := testenv.New(t) + defer env.RemoveAll() + + archivePath := env.GetPath("test-image.tar") + createDummyDockerArchive(t, archivePath) + + // Pre-create the chroot directory and files + rootfsPath := "var/lib/warewulf/chroots/existing-image/rootfs" + binShPath := rootfsPath + "/bin/sh" + otherFilePath := rootfsPath + "/file-kept" + + // Create file that should be overwritten + env.WriteFile(binShPath, "old_shell") + // Create file that should persist + env.WriteFile(otherFilePath, "persist") + + args := []string{"file://" + archivePath, "existing-image"} + err := CobraRunE(&cobra.Command{}, args) + + if err != nil { + if strings.Contains(err.Error(), "operation not permitted") || strings.Contains(err.Error(), "chown") { + t.Logf("Caught expected error in unprivileged environment: %v", err) + return + } + t.Fatalf("Unexpected error: %v", err) + } + + // Checks + // 1. Image exists (rootfs dir) + assert.True(t, util.IsDir(env.GetPath(rootfsPath)), "rootfs directory should exist") + + // 2. bin/sh overwritten + // In unprivileged test, if we return early above, this won't run. + // If we are privileged (or if the error doesn't happen), this verifies the overwrite. + if util.IsFile(env.GetPath(binShPath)) { + content := env.ReadFile(binShPath) + assert.Equal(t, "shell", content, "bin/sh should be overwritten") + } else { + t.Error("bin/sh should exist") + } + + // 3. other-file persists + assert.True(t, util.IsFile(env.GetPath(otherFilePath)), "file-kept should still exist") + content := env.ReadFile(otherFilePath) + assert.Equal(t, "persist", content, "file-kept content should be preserved") + }) +} diff --git a/internal/app/wwctl/image/imprt/root.go b/internal/app/wwctl/image/imprt/root.go index 5aaf3a0f..5f882507 100644 --- a/internal/app/wwctl/image/imprt/root.go +++ b/internal/app/wwctl/image/imprt/root.go @@ -40,8 +40,8 @@ Imported images are used to create bootable images.`, ) func init() { - baseCmd.PersistentFlags().BoolVarP(&SetForce, "force", "f", false, "Force overwrite of an existing image") - baseCmd.PersistentFlags().BoolVarP(&SetUpdate, "update", "u", false, "Update and overwrite an existing image") + baseCmd.PersistentFlags().BoolVarP(&SetForce, "force", "f", false, "Remove existing image and import new image with that name") + baseCmd.PersistentFlags().BoolVarP(&SetUpdate, "update", "u", false, "Overwrite files in an existing image with the files of remote image") baseCmd.PersistentFlags().BoolVarP(&SetBuild, "build", "b", false, "Build image after pulling") baseCmd.PersistentFlags().BoolVar(&SyncUser, "syncuser", false, "Synchronize UIDs/GIDs from host to image") baseCmd.PersistentFlags().BoolVar(&OciNoHttps, "nohttps", false, "Ignore wrong TLS certificates, superseedes env WAREWULF_OCI_NOHTTPS") diff --git a/internal/pkg/api/image/image.go b/internal/pkg/api/image/image.go index e599c473..9f3c35b3 100644 --- a/internal/pkg/api/image/image.go +++ b/internal/pkg/api/image/image.go @@ -130,7 +130,8 @@ func ImageImport(cip *wwapiv1.ImageImportParameter) (imageName string, err error return } wwlog.Info("Updating existing image") - } else if strings.HasPrefix(cip.Source, "docker://") || strings.HasPrefix(cip.Source, "docker-daemon://") || + } + if strings.HasPrefix(cip.Source, "docker://") || strings.HasPrefix(cip.Source, "docker-daemon://") || strings.HasPrefix(cip.Source, "file://") || util.IsFile(cip.Source) { var sCtx *types.SystemContext sCtx, err = GetSystemContext(cip.OciNoHttps, cip.OciUsername, cip.OciPassword, cip.Platform)