diff --git a/pkg/svc/image/exec.go b/pkg/svc/image/exec.go index 937a015c55..ab8a4ec9f8 100644 --- a/pkg/svc/image/exec.go +++ b/pkg/svc/image/exec.go @@ -78,3 +78,14 @@ func (e *ContainerExecutor) ExecInContainer( return stdout.String(), nil } + +// removeNodeArchive deletes an export or import archive from a node. It always +// runs on a context detached from the caller and bounded by nodeCleanupTimeout, +// so a cancellation that lands while removal is starting cannot abort it and +// leave the archive behind. +func (e *ContainerExecutor) removeNodeArchive(ctx context.Context, nodeName, tmpPath string) { + cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), nodeCleanupTimeout) + defer cancel() + + _, _ = e.ExecInContainer(cleanupCtx, nodeName, []string{"rm", "-f", tmpPath}) +} diff --git a/pkg/svc/image/export_test.go b/pkg/svc/image/export_test.go index c89fcc25ed..ab2621daf3 100644 --- a/pkg/svc/image/export_test.go +++ b/pkg/svc/image/export_test.go @@ -4,18 +4,22 @@ package image // IsHelperContainerForTest exposes isHelperContainer for white-box testing. var IsHelperContainerForTest = isHelperContainer -// Every test in this package's binary sees a fixed export archive name, so +// Every test in this package's binary sees fixed export and import archive names, so // node-side command expectations stay exact. The default generator is exposed // below and tested on its own. // //nolint:gochecknoinits // The seam must be in place before parallel tests start. func init() { newExportArchiveName = func() string { return "ksail-images-export.tar" } + newImportArchiveName = func() string { return "ksail-images-import.tar" } } // DefaultExportArchiveNameForTest exposes the production archive name generator. var DefaultExportArchiveNameForTest = defaultExportArchiveName +// DefaultImportArchiveNameForTest exposes the production import archive name generator. +var DefaultImportArchiveNameForTest = defaultImportArchiveName + // RemoveNodeArchiveForTest exposes removeNodeArchive for white-box testing. var RemoveNodeArchiveForTest = (*Exporter).removeNodeArchive diff --git a/pkg/svc/image/exporter.go b/pkg/svc/image/exporter.go index 0f8a6cd836..0e15579ff3 100644 --- a/pkg/svc/image/exporter.go +++ b/pkg/svc/image/exporter.go @@ -28,11 +28,11 @@ const ( ctrNamespaceArg = "--namespace=k8s.io" // ctrImages is the ctr subcommand group for image operations. ctrImages = "images" - // nodeCleanupTimeout bounds removing an export archive from a node after the - // command's context is done, so a cancelled export still cleans up. + // nodeCleanupTimeout bounds removing an export or import archive from a node + // after the command's context is done, so a cancelled operation still cleans up. nodeCleanupTimeout = 30 * time.Second - // exportArchiveSuffixBytes is the random suffix length of an export archive name. - exportArchiveSuffixBytes = 8 + // nodeArchiveSuffixBytes is the random suffix length of a node-side archive name. + nodeArchiveSuffixBytes = 8 ) // newExportArchiveName returns the file name of one export's archive on a node. @@ -46,10 +46,15 @@ var newExportArchiveName = defaultExportArchiveName // defaultExportArchiveName returns a random node-side archive name, so concurrent // exports on the same node never write to the same file. func defaultExportArchiveName() string { - suffix := make([]byte, exportArchiveSuffixBytes) + return randomNodeArchiveName("ksail-images-export-") +} + +// randomNodeArchiveName returns prefix followed by a random hex suffix and ".tar". +func randomNodeArchiveName(prefix string) string { + suffix := make([]byte, nodeArchiveSuffixBytes) _, _ = rand.Read(suffix) - return "ksail-images-export-" + hex.EncodeToString(suffix) + ".tar" + return prefix + hex.EncodeToString(suffix) + ".tar" } // Error definitions. @@ -403,13 +408,7 @@ func (e *Exporter) writeFileFromTar(dstPath string, tarReader *tar.Reader) error return nil } -// removeNodeArchive deletes an export archive from a node. It always runs on a -// context detached from the caller and bounded by nodeCleanupTimeout, so a -// cancellation that lands while removal is starting cannot abort it and leave -// the archive behind. +// removeNodeArchive deletes an export archive from a node. func (e *Exporter) removeNodeArchive(ctx context.Context, nodeName, tmpPath string) { - cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), nodeCleanupTimeout) - defer cancel() - - _, _ = e.executor.ExecInContainer(cleanupCtx, nodeName, []string{"rm", "-f", tmpPath}) + e.executor.removeNodeArchive(ctx, nodeName, tmpPath) } diff --git a/pkg/svc/image/importer.go b/pkg/svc/image/importer.go index e8da8c6451..af2a073851 100644 --- a/pkg/svc/image/importer.go +++ b/pkg/svc/image/importer.go @@ -24,6 +24,19 @@ type ImportOptions struct { InputPath string } +// newImportArchiveName returns the file name of one import's archive on a node. +// Each import gets its own name, so two imports on the same node never write to +// or remove each other's archive. Tests replace it to keep the node-side path +// predictable. +// +//nolint:gochecknoglobals // Test seam: tests pin the name so node-side commands stay exact. +var newImportArchiveName = defaultImportArchiveName + +// defaultImportArchiveName returns a random node-side import archive name. +func defaultImportArchiveName() string { + return randomNodeArchiveName("ksail-images-import-") +} + // Importer handles importing container images to cluster containerd. type Importer struct { dockerClient dockerclient.Client @@ -161,8 +174,12 @@ func (i *Importer) importImagesToNode( inputPath string, tmpBasePath string, ) error { - // Copy the tar file from host to container - tmpPath := tmpBasePath + "/ksail-images-import.tar" + // Copy the tar file from host to container. Each import gets its own archive + // name, and the archive is removed however the import ends, even after a + // partial copy or a cancellation. + tmpPath := tmpBasePath + "/" + newImportArchiveName() + + defer i.executor.removeNodeArchive(ctx, nodeName, tmpPath) err := i.copyToContainer(ctx, nodeName, inputPath, tmpPath) if err != nil { @@ -184,9 +201,6 @@ func (i *Importer) importImagesToNode( return fmt.Errorf("ctr import failed: %w", err) } - // Clean up temporary file in container - _, _ = i.executor.ExecInContainer(ctx, nodeName, []string{"rm", "-f", tmpPath}) - return nil } diff --git a/pkg/svc/image/importer_test.go b/pkg/svc/image/importer_test.go index 1aafe5034e..40c05fd656 100644 --- a/pkg/svc/image/importer_test.go +++ b/pkg/svc/image/importer_test.go @@ -7,6 +7,8 @@ import ( "io" "os" "path/filepath" + "regexp" + "slices" "testing" v1alpha1 "github.com/devantler-tech/ksail/v7/pkg/apis/cluster/v1alpha1" @@ -24,6 +26,10 @@ var ( errImportCopyFailed = errors.New("copy to container failed") ) +// kindImportPath is the node-side import archive on a Kind node under the fixed +// test archive name. +const kindImportPath = "/root/ksail-images-import.tar" + func TestNewImporter(t *testing.T) { t.Parallel() @@ -256,11 +262,11 @@ func TestImportSuccessVanilla(t *testing.T) { Return(nil).Once() // Mock exec for import command - Kind uses /root path - importPath := "/root/ksail-images-import.tar" + importPath := kindImportPath setupImportExecMockForImporter(ctx, t, mockClient, nodeName, importPath) // Mock exec for cleanup - setupExecMockForImporter(ctx, t, mockClient, nodeName) + setupCleanupMockForImporter(t, mockClient, nodeName, importPath) } importer := image.NewImporter(mockClient) @@ -322,7 +328,7 @@ func TestImportSuccessK3s(t *testing.T) { setupImportExecMockForImporter(ctx, t, mockClient, nodeName, "/tmp/ksail-images-import.tar") // Mock exec for cleanup - setupExecMockForImporter(ctx, t, mockClient, nodeName) + setupCleanupMockForImporter(t, mockClient, nodeName, "/tmp/ksail-images-import.tar") } importer := image.NewImporter(mockClient) @@ -364,6 +370,7 @@ func TestImportCopyToContainerFails(t *testing.T) { mockClient.EXPECT(). CopyToContainer(ctx, "my-cluster-control-plane", "/root", mock.Anything, container.CopyToContainerOptions{}). Return(errImportCopyFailed) + setupCleanupMockForImporter(t, mockClient, "my-cluster-control-plane", kindImportPath) importer := image.NewImporter(mockClient) err = importer.Import( @@ -419,6 +426,7 @@ func TestImportExecFails(t *testing.T) { mockClient.EXPECT(). ContainerExecInspect(ctx, execID). Return(container.ExecInspect{ExitCode: 1}, nil) + setupCleanupMockForImporter(t, mockClient, "my-cluster-control-plane", kindImportPath) importer := image.NewImporter(mockClient) err = importer.Import( @@ -472,7 +480,7 @@ func TestImportEmptyProvider(t *testing.T) { ) // Mock exec for cleanup - setupExecMockForImporter(ctx, t, mockClient, "my-cluster-control-plane") + setupCleanupMockForImporter(t, mockClient, "my-cluster-control-plane", kindImportPath) importer := image.NewImporter(mockClient) err = importer.Import(ctx, "my-cluster", v1alpha1.DistributionVanilla, "", image.ImportOptions{ @@ -520,12 +528,13 @@ func TestImportMultipleNodesPartialFailure(t *testing.T) { "my-cluster-control-plane", "/root/ksail-images-import.tar", ) - setupExecMockForImporter(ctx, t, mockClient, "my-cluster-control-plane") + setupCleanupMockForImporter(t, mockClient, "my-cluster-control-plane", kindImportPath) // Second node fails during copy mockClient.EXPECT(). CopyToContainer(ctx, "my-cluster-worker", "/root", mock.Anything, container.CopyToContainerOptions{}). Return(errImportCopyFailed).Once() + setupCleanupMockForImporter(t, mockClient, "my-cluster-worker", kindImportPath) importer := image.NewImporter(mockClient) err = importer.Import( @@ -579,7 +588,7 @@ func TestImportLargeFile(t *testing.T) { "my-cluster-control-plane", "/root/ksail-images-import.tar", ) - setupExecMockForImporter(ctx, t, mockClient, "my-cluster-control-plane") + setupCleanupMockForImporter(t, mockClient, "my-cluster-control-plane", kindImportPath) importer := image.NewImporter(mockClient) err = importer.Import( @@ -595,27 +604,32 @@ func TestImportLargeFile(t *testing.T) { require.NoError(t, err) } -// setupExecMockForImporter is a helper to set up ContainerExec* mocks for simple cases. -func setupExecMockForImporter( - ctx context.Context, +// setupCleanupMockForImporter expects the import archive at archivePath to be +// removed from containerName exactly once, on a live context detached from the +// caller, so cleanup also runs after a failed or cancelled import. +func setupCleanupMockForImporter( t *testing.T, mockClient *docker.MockAPIClient, containerName string, + archivePath string, ) { t.Helper() - execID := "exec-" + containerName + execID := "exec-cleanup-" + containerName mockClient.EXPECT(). - ContainerExecCreate(ctx, containerName, mock.Anything). + ContainerExecCreate(detachedCleanupContext(), containerName, + mock.MatchedBy(func(opts container.ExecOptions) bool { + return slices.Equal(opts.Cmd, []string{"rm", "-f", archivePath}) + })). Return(container.ExecCreateResponse{ID: execID}, nil).Once() mockClient.EXPECT(). - ContainerExecAttach(ctx, execID, container.ExecStartOptions{}). + ContainerExecAttach(detachedCleanupContext(), execID, container.ExecStartOptions{}). Return(mockDockerStreamResponse("", ""), nil).Once() mockClient.EXPECT(). - ContainerExecInspect(ctx, execID). + ContainerExecInspect(detachedCleanupContext(), execID). Return(container.ExecInspect{ExitCode: 0}, nil).Once() } @@ -652,3 +666,65 @@ func setupImportExecMockForImporter( ContainerExecInspect(ctx, execID). Return(container.ExecInspect{ExitCode: 0}, nil).Once() } + +// Two imports on the same node must never share an archive, so every generated +// name is distinct. +func TestDefaultImportArchiveNamesAreDistinct(t *testing.T) { + t.Parallel() + + pattern := regexp.MustCompile(`^ksail-images-import-[0-9a-f]{16}\.tar$`) + seen := make(map[string]struct{}) + + for range 64 { + name := image.DefaultImportArchiveNameForTest() + if !pattern.MatchString(name) { + t.Fatalf("archive name %q does not match %s", name, pattern) + } + + if _, duplicate := seen[name]; duplicate { + t.Fatalf("archive name %q was generated twice", name) + } + + seen[name] = struct{}{} + } +} + +// A cancelled import must still remove its archive from the node, so cleanup +// runs on a live context after the caller's context is done. +func TestImportCancelledStillRemovesArchive(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithCancel(context.Background()) + mockClient := docker.NewMockAPIClient(t) + + inputPath := filepath.Join(t.TempDir(), "images.tar") + require.NoError(t, os.WriteFile(inputPath, []byte("fake tar content"), 0o600)) + + mockClient.EXPECT(). + ContainerList(ctx, mock.Anything). + Return([]container.Summary{ + { + Names: []string{"/my-cluster-control-plane"}, + Labels: map[string]string{"io.x-k8s.kind.role": "control-plane"}, + }, + }, nil) + + mockClient.EXPECT(). + CopyToContainer(ctx, "my-cluster-control-plane", "/root", mock.Anything, container.CopyToContainerOptions{}). + RunAndReturn(func(context.Context, string, string, io.Reader, container.CopyToContainerOptions) error { + cancel() + + return context.Canceled + }) + setupCleanupMockForImporter(t, mockClient, "my-cluster-control-plane", kindImportPath) + + err := image.NewImporter(mockClient).Import( + ctx, + "my-cluster", + v1alpha1.DistributionVanilla, + v1alpha1.ProviderDocker, + image.ImportOptions{InputPath: inputPath}, + ) + + require.ErrorIs(t, err, context.Canceled) +} diff --git a/pkg/svc/image/multi_export_test.go b/pkg/svc/image/multi_export_test.go index 1d4547df44..2ec7491fa5 100644 --- a/pkg/svc/image/multi_export_test.go +++ b/pkg/svc/image/multi_export_test.go @@ -14,6 +14,7 @@ import ( "runtime" "slices" "strings" + "sync" "testing" v1alpha1 "github.com/devantler-tech/ksail/v7/pkg/apis/cluster/v1alpha1" @@ -766,3 +767,120 @@ func TestExportNormalizesNamesWhenOneNodeCoversThePlan(t *testing.T) { require.Equal(t, []string{"docker.io/library/fixture-a:v1"}, archiveNamedReferences(t, output)) assertImageArchiveReadable(t, output, 1) } + +// exportOperands records the image operands each node's export command named. +type exportOperands struct { + mu sync.Mutex + byNode map[string][]string +} + +func (operands *exportOperands) record(node string, refs []string) { + operands.mu.Lock() + defer operands.mu.Unlock() + + operands.byNode[node] = append(operands.byNode[node], refs...) +} + +func (operands *exportOperands) of(node string) []string { + operands.mu.Lock() + defer operands.mu.Unlock() + + return slices.Clone(operands.byNode[node]) +} + +// setupOperandNodeArchive makes node hold held, and answers its export with an +// archive of exactly the references the export command named, so an export that +// asks a node for more than it should is visible in the archive. +func setupOperandNodeArchive( + ctx context.Context, + t *testing.T, + client *docker.MockAPIClient, + node string, + held []string, + operands *exportOperands, +) { + t.Helper() + + optionalNodeExec(ctx, client, node, "list", + []string{ctrCommand, "--namespace=k8s.io", "images", "list", "-q"}, + strings.Join(held, "\n")+"\n") + optionalNodeExec(ctx, client, node, "platform", []string{"uname", "-m"}, "x86_64\n") + optionalNodeExec(ctx, client, node, "help", + []string{ctrCommand, "--namespace=k8s.io", "images", "export", "--help"}, + "OPTIONS:\n --local Use client-side export\n --help, -h Show help\n") + + exportPrefix := []string{ + ctrCommand, "--namespace=k8s.io", "images", "export", "--platform", "linux/amd64", + "--local", "--skip-manifest-json", kindExporterTarPath, + } + + client.EXPECT(). + ContainerExecCreate(ctx, node, mock.MatchedBy(func(options container.ExecOptions) bool { + return len(options.Cmd) > len(exportPrefix) && + slices.Equal(options.Cmd[:len(exportPrefix)], exportPrefix) + })). + RunAndReturn(func( + _ context.Context, _ string, options container.ExecOptions, + ) (container.ExecCreateResponse, error) { + operands.record(node, options.Cmd[len(exportPrefix):]) + + return container.ExecCreateResponse{ID: node + "-export"}, nil + }). + Maybe() + client.EXPECT().ContainerExecAttach(ctx, node+"-export", container.ExecStartOptions{}). + Return(mockDockerStreamResponse("", ""), nil).Maybe() + client.EXPECT().ContainerExecInspect(ctx, node+"-export"). + Return(container.ExecInspect{ExitCode: 0}, nil).Maybe() + + client.EXPECT().CopyFromContainer(ctx, node, kindExporterTarPath). + RunAndReturn(func(context.Context, string, string) (io.ReadCloser, container.PathStat, error) { + archive := createExportTar(t, namedNodeArchive(t, operands.of(node)...)) + + return io.NopCloser(bytes.NewReader(archive)), container.PathStat{}, nil + }). + Maybe() + optionalNodeExec(detachedCleanupContext(), client, node, "cleanup", + []string{"rm", "-f", kindExporterTarPath}, "") +} + +// A selected export that asks each node for a proper subset of what it holds +// must name only the requested images, both in the archive and in the +// operands sent to each node, so exporting every image a node holds cannot pass. +func TestExportSelectsAProperSubsetOfEachNodesImages(t *testing.T) { + t.Parallel() + + const ( + wantedOnControlPlane = "docker.io/library/fixture-a:v1" + unwantedOnControl = "docker.io/library/fixture-c:v1" + wantedOnWorker = "docker.io/library/fixture-b:v1" + unwantedOnWorker = "docker.io/library/fixture-d:v1" + workerNodeName = "my-cluster-worker" + ) + + ctx := context.Background() + client := docker.NewMockAPIClient(t) + operands := &exportOperands{byNode: make(map[string][]string)} + + client.EXPECT(). + ContainerList(ctx, mock.Anything). + Return(kindMultiNodeSummaries()[:2], nil) + + setupOperandNodeArchive(ctx, t, client, kindExporterNodeName, + []string{wantedOnControlPlane, unwantedOnControl}, operands) + setupOperandNodeArchive(ctx, t, client, workerNodeName, + []string{wantedOnWorker, unwantedOnWorker}, operands) + + output := filepath.Join(t.TempDir(), "images.tar") + + err := image.NewExporter(client).Export(ctx, "my-cluster", v1alpha1.DistributionVanilla, + v1alpha1.ProviderDocker, image.ExportOptions{ + OutputPath: output, + Images: []string{wantedOnControlPlane, wantedOnWorker}, + }) + require.NoError(t, err) + require.ElementsMatch(t, []string{wantedOnControlPlane, wantedOnWorker}, + archiveNamedReferences(t, output)) + assertImageArchiveReadable(t, output, 2) + require.Equal(t, []string{wantedOnControlPlane}, operands.of(kindExporterNodeName)) + require.Equal(t, []string{wantedOnWorker}, operands.of(workerNodeName)) +}