From dc7634cc28cfb75b59964b53f3bf9f3796097bc9 Mon Sep 17 00:00:00 2001 From: Tiago Martins Date: Wed, 15 Jan 2025 11:11:30 +0000 Subject: [PATCH] fix(follow): file handling of artifacts with directories Signed-off-by: Tiago Martins --- internal/follower/follower.go | 67 +++++++++--- internal/follower/follower_test.go | 170 ++++++++++++++++++++++++++++- internal/utils/extract.go | 2 +- 3 files changed, 219 insertions(+), 20 deletions(-) diff --git a/internal/follower/follower.go b/internal/follower/follower.go index 36060d38..0ef996ad 100644 --- a/internal/follower/follower.go +++ b/internal/follower/follower.go @@ -1,5 +1,5 @@ // SPDX-License-Identifier: Apache-2.0 -// Copyright (C) 2023 The Falco Authors +// Copyright (C) 2025 The Falco Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -203,52 +203,83 @@ func (f *Follower) follow(ctx context.Context) { return } + // Move files to their destination + if err := f.moveFiles(filePaths, dstDir); err != nil { + return + } + + f.logger.Info("Artifact correctly installed", + f.logger.Args("followerName", f.ref, "artifactName", f.ref, "type", res.Type, "digest", res.Digest, "directory", dstDir)) + f.currentDigest = desc.Digest.String() +} + +// moveFiles moves files from their temporary location to the destination directory. +// It preserves the directory structure relative to the temporary directory. +// For example, if a file is at "tmpDir/subdir/file.yaml", it will be moved to +// "dstDir/subdir/file.yaml". This ensures that files in subdirectories are moved +// correctly as individual files, not as entire directories. +func (f *Follower) moveFiles(filePaths []string, dstDir string) error { // Install the artifacts if necessary. for _, path := range filePaths { - baseName := filepath.Base(path) - f.logger.Debug("Installing file", f.logger.Args("followerName", f.ref, "fileName", baseName)) - dstPath := filepath.Join(dstDir, baseName) + // Get the relative path from the temporary directory to preserve directory structure + relPath, err := filepath.Rel(f.tmpDir, path) + if err != nil { + f.logger.Error("Unable to get relative path", f.logger.Args("followerName", f.ref, "path", path, "reason", err.Error())) + return err + } + + dstPath := filepath.Join(dstDir, relPath) + // Ensure the parent directory exists + if err := os.MkdirAll(filepath.Dir(dstPath), 0o750); err != nil { + f.logger.Error("Unable to create destination directory", f.logger.Args( + "followerName", f.ref, + "directory", filepath.Dir(dstPath), + "reason", err.Error(), + )) + return err + } + + f.logger.Debug("Installing file", f.logger.Args("followerName", f.ref, "path", relPath)) // Check if the file exists. - f.logger.Debug("Checking if file already exists", f.logger.Args("followerName", f.ref, "fileName", baseName, "directory", dstDir)) + f.logger.Debug("Checking if file already exists", f.logger.Args("followerName", f.ref, "path", relPath, "directory", dstDir)) exists, err := utils.FileExists(dstPath) if err != nil { - f.logger.Error("Unable to check existence for file", f.logger.Args("followerName", f.ref, "fileName", baseName, "reason", err.Error())) - return + f.logger.Error("Unable to check existence for file", f.logger.Args("followerName", f.ref, "path", relPath, "reason", err.Error())) + return err } if !exists { - f.logger.Debug("Moving file", f.logger.Args("followerName", f.ref, "fileName", baseName, "destDirectory", dstDir)) + f.logger.Debug("Moving file", f.logger.Args("followerName", f.ref, "path", relPath, "destDirectory", dstDir)) if err = utils.Move(path, dstPath); err != nil { - f.logger.Error("Unable to move file", f.logger.Args("followerName", f.ref, "fileName", baseName, "destDirectory", dstDir, "reason", err.Error())) - return + f.logger.Error("Unable to move file", f.logger.Args("followerName", f.ref, "path", relPath, "destDirectory", dstDir, "reason", err.Error())) + return err } f.logger.Debug("File correctly installed", f.logger.Args("followerName", f.ref, "path", path)) // It's done, move to the next file. continue } - f.logger.Debug(fmt.Sprintf("file %q already exists in %q, checking if it is equal to the existing one", baseName, dstDir), + + f.logger.Debug(fmt.Sprintf("file %q already exists in %q, checking if it is equal to the existing one", relPath, dstDir), f.logger.Args("followerName", f.ref)) + // Check if the files are equal. eq, err := equal([]string{path, dstPath}) if err != nil { - f.logger.Error("Unable to compare files", f.logger.Args("followerName", f.ref, "newFile", path, "existingFile", dstPath, "reason", err.Error())) - return + f.logger.Error("Unable to compare files", f.logger.Args("followerName", f.ref, "existingFile", dstPath, "reason", err.Error())) + return err } if !eq { f.logger.Debug(fmt.Sprintf("Overwriting file %q with file %q", dstPath, path), f.logger.Args("followerName", f.ref)) if err = utils.Move(path, dstPath); err != nil { f.logger.Error("Unable to overwrite file", f.logger.Args("followerName", f.ref, "existingFile", dstPath, "reason", err.Error())) - return + return err } } else { f.logger.Debug("The two file are equal, nothing to be done") } } - - f.logger.Info("Artifact correctly installed", - f.logger.Args("followerName", f.ref, "artifactName", f.ref, "type", res.Type, "digest", res.Digest, "directory", dstDir)) - f.currentDigest = desc.Digest.String() + return nil } // pull downloads, extracts, and installs the artifact. diff --git a/internal/follower/follower_test.go b/internal/follower/follower_test.go index 5385bf0b..f450973e 100644 --- a/internal/follower/follower_test.go +++ b/internal/follower/follower_test.go @@ -1,5 +1,5 @@ // SPDX-License-Identifier: Apache-2.0 -// Copyright (C) 2024 The Falco Authors +// Copyright (C) 2025 The Falco Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -17,6 +17,7 @@ package follower import ( "os" + "path/filepath" "testing" "github.com/pterm/pterm" @@ -135,3 +136,170 @@ func TestCheckRequirements(t *testing.T) { }) } } + +func TestMoveFiles(t *testing.T) { + type testFile struct { + path string + content string + replace bool + } + + tests := []struct { + name string + files []testFile + existing []testFile + }{ + { + name: "basic file at root", + files: []testFile{ + { + path: "file1.yaml", + content: "content1", + }, + }, + }, + { + name: "file in subdirectory", + files: []testFile{ + { + path: "subdir/file2.yaml", + content: "content2", + }, + }, + }, + { + name: "multiple files in different directories", + files: []testFile{ + { + path: "file1.yaml", + content: "content1", + }, + { + path: "subdir/file2.yaml", + content: "content2", + }, + { + path: "subdir/nested/file3.yaml", + content: "content3", + }, + }, + }, + { + name: "existing file with identical content", + files: []testFile{ + { + path: "file1.yaml", + content: "content1", + replace: false, + }, + }, + existing: []testFile{ + { + path: "file1.yaml", + content: "content1", + }, + }, + }, + { + name: "existing file with different content", + files: []testFile{ + { + path: "file1.yaml", + content: "new content", + replace: true, + }, + }, + existing: []testFile{ + { + path: "file1.yaml", + content: "old content", + }, + }, + }, + { + name: "mix of new and existing files", + files: []testFile{ + { + path: "file1.yaml", + content: "content1", + replace: false, + }, + { + path: "subdir/file2.yaml", + content: "new content2", + replace: true, + }, + }, + existing: []testFile{ + { + path: "file1.yaml", + content: "content1", + }, + { + path: "subdir/file2.yaml", + content: "old content2", + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "falcoctl-test-*") + assert.NoError(t, err) + defer os.RemoveAll(tmpDir) + + dstDir, err := os.MkdirTemp("", "falcoctl-dst-*") + assert.NoError(t, err) + defer os.RemoveAll(dstDir) + + // Setup existing files + for _, ef := range tt.existing { + dstPath := filepath.Join(dstDir, ef.path) + err = os.MkdirAll(filepath.Dir(dstPath), 0o755) + assert.NoError(t, err) + err = os.WriteFile(dstPath, []byte(ef.content), 0o644) + assert.NoError(t, err) + } + + f, err := New("test-registry/test-ref", output.NewPrinter(pterm.LogLevelDebug, pterm.LogFormatterJSON, os.Stdout), &Config{ + RulesfilesDir: dstDir, + TmpDir: tmpDir, + }) + assert.NoError(t, err) + + var paths []string + for _, tf := range tt.files { + fullPath := filepath.Join(f.tmpDir, tf.path) + err = os.MkdirAll(filepath.Dir(fullPath), 0o755) + assert.NoError(t, err) + err = os.WriteFile(fullPath, []byte(tf.content), 0o644) + assert.NoError(t, err) + paths = append(paths, fullPath) + } + + f.currentDigest = "test-digest" + err = f.moveFiles(paths, dstDir) + assert.NoError(t, err) + + for _, tf := range tt.files { + dstPath := filepath.Join(dstDir, tf.path) + _, err = os.Stat(dstPath) + assert.NoError(t, err, "file should exist at %s", dstPath) + + content, err := os.ReadFile(dstPath) + assert.NoError(t, err) + assert.Equal(t, tf.content, string(content), "file content should match at %s", dstPath) + + // For files marked as replace=false, verify they have identical content with existing files + if !tf.replace { + for _, ef := range tt.existing { + if ef.path == tf.path { + assert.Equal(t, ef.content, string(content), "file content should not change when replace=false: %s", dstPath) + } + } + } + } + }) + } +} diff --git a/internal/utils/extract.go b/internal/utils/extract.go index 72ec6571..b1f84271 100644 --- a/internal/utils/extract.go +++ b/internal/utils/extract.go @@ -86,7 +86,6 @@ func ExtractTarGz(ctx context.Context, gzipStream io.Reader, destDir string, str continue } info := header.FileInfo() - files = append(files, path) switch header.Typeflag { case tar.TypeDir: @@ -106,6 +105,7 @@ func ExtractTarGz(ctx context.Context, gzipStream io.Reader, destDir string, str if err = outFile.Close(); err != nil { return nil, err } + files = append(files, path) case tar.TypeLink: name := header.Linkname if stripPathComponents > 0 {