diff --git a/go.mod b/go.mod index 5bb2430e9..744641fc2 100644 --- a/go.mod +++ b/go.mod @@ -39,6 +39,7 @@ require ( k8s.io/cri-api v0.33.2 k8s.io/kubectl v0.33.2 k8s.io/kubelet v0.33.2 + k8s.io/mount-utils v0.33.2 sigs.k8s.io/controller-runtime v0.21.0 sigs.k8s.io/hydrophone v0.7.0 ) @@ -65,6 +66,7 @@ require ( github.com/mattn/go-isatty v0.0.20 // indirect github.com/mitchellh/go-wordwrap v1.0.1 // indirect github.com/moby/spdystream v0.5.0 // indirect + github.com/moby/sys/mountinfo v0.7.2 // indirect github.com/moby/term v0.5.2 // indirect github.com/monochromegane/go-gitignore v0.0.0-20200626010858-205db1a8cc00 // indirect github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f // indirect diff --git a/go.sum b/go.sum index 0e4d380e9..8618a5cbb 100644 --- a/go.sum +++ b/go.sum @@ -177,6 +177,8 @@ github.com/mitchellh/go-wordwrap v1.0.1 h1:TLuKupo69TCn6TQSyGxwI1EblZZEsQ0vMlAFQ github.com/mitchellh/go-wordwrap v1.0.1/go.mod h1:R62XHJLzvMFRBbcrT7m7WgmE1eOyTSsCt+hzestvNj0= github.com/moby/spdystream v0.5.0 h1:7r0J1Si3QO/kjRitvSLVVFUjxMEb/YLj6S9FF62JBCU= github.com/moby/spdystream v0.5.0/go.mod h1:xBAYlnt/ay+11ShkdFKNAG7LsyK/tmNBVvVOwrfMgdI= +github.com/moby/sys/mountinfo v0.7.2 h1:1shs6aH5s4o5H2zQLn796ADW1wMrIwHsyJ2v9KouLrg= +github.com/moby/sys/mountinfo v0.7.2/go.mod h1:1YOa8w8Ih7uW0wALDUgT1dTTSBrZ+HiBLGws92L2RU4= github.com/moby/term v0.5.2 h1:6qk3FJAFDs6i/q3W/pQ97SX192qKfZgGjCQqfCJkgzQ= github.com/moby/term v0.5.2/go.mod h1:d3djjFCrjnB+fl8NJux+EJzu0msscUP+f8it8hPkFLc= github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= @@ -266,6 +268,8 @@ golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8U golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/crypto v0.40.0 h1:r4x+VvoG5Fm+eJcxMaY8CQM7Lb0l1lsmjGBQ6s8BfKM= golang.org/x/crypto v0.40.0/go.mod h1:Qr1vMER5WyS2dfPHAlsOj01wgLbsyWtFn/aY+5+ZdxY= +golang.org/x/exp v0.0.0-20241217172543-b2144cdd0a67 h1:1UoZQm6f0P/ZO0w1Ri+f+ifG/gXhegadRdwBIXEFWDo= +golang.org/x/exp v0.0.0-20241217172543-b2144cdd0a67/go.mod h1:qj5a5QZpwLU2NLQudwIN5koi3beDhSAlJwa67PuM98c= golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.26.0 h1:EGMPT//Ezu+ylkCijjPc+f4Aih7sZvaAr+O3EHBxvZg= @@ -351,6 +355,8 @@ k8s.io/kubelet v0.33.2 h1:wxEau5/563oJb3j3KfrCKlNWWx35YlSgDLOYUBCQ0pg= k8s.io/kubelet v0.33.2/go.mod h1:way8VCDTUMiX1HTOvJv7M3xS/xNysJI6qh7TOqMe5KM= k8s.io/metrics v0.33.2 h1:gNCBmtnUMDMCRg9Ly5ehxP3OdKISMsOnh1vzk01iCgE= k8s.io/metrics v0.33.2/go.mod h1:yxoAosKGRsZisv3BGekC5W6T1J8XSV+PoUEevACRv7c= +k8s.io/mount-utils v0.33.2 h1:mZAFhoGs/MwJziVlUpA072vqMhXRc0LGl/W3wybLP20= +k8s.io/mount-utils v0.33.2/go.mod h1:1JR4rKymg8B8bCPo618hpSAdrpO6XLh0Acqok/xVwPE= k8s.io/utils v0.0.0-20250321185631-1f6e0b77f77e h1:KqK5c/ghOm8xkHYhlodbp6i6+r+ChV2vuAuVRdFbLro= k8s.io/utils v0.0.0-20250321185631-1f6e0b77f77e/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0= sigs.k8s.io/controller-runtime v0.21.0 h1:CYfjpEuicjUecRk+KAeyYh+ouUBn4llGyDYytIGcJS8= diff --git a/internal/artifact/install.go b/internal/artifact/install.go index c2ccb536a..b3c7a3749 100644 --- a/internal/artifact/install.go +++ b/internal/artifact/install.go @@ -12,6 +12,8 @@ import ( "strings" "github.com/pkg/errors" + + "github.com/aws/eks-hybrid/internal/system" ) // DefaultDirPerms are the permissions assigned to a directory when an Install* func is called @@ -21,7 +23,7 @@ const DefaultDirPerms = fs.ModeDir | 0o755 // InstallFile installs src to dst with perms permissions. It ensures any base paths exist // before installing. func InstallFile(dst string, src io.Reader, perms fs.FileMode) error { - if err := os.RemoveAll(dst); err != nil { + if err := system.SafeRemoveAll(dst, false, false); err != nil { return err } if err := os.MkdirAll(path.Dir(dst), DefaultDirPerms); err != nil { diff --git a/internal/cleanup/cleanup.go b/internal/cleanup/cleanup.go index f4763204b..5b5816a57 100644 --- a/internal/cleanup/cleanup.go +++ b/internal/cleanup/cleanup.go @@ -2,11 +2,12 @@ package cleanup import ( "fmt" - "os" "path/filepath" "strings" "go.uber.org/zap" + + "github.com/aws/eks-hybrid/internal/system" ) // Directories to clean up when force flag is enabled @@ -57,5 +58,5 @@ func (c *Force) Cleanup() error { func (c *Force) removeDir(dir string) error { c.logger.Info("Removing directory", zap.String("path", dir)) - return os.RemoveAll(dir) + return system.SafeRemoveAll(dir, false, false) } diff --git a/internal/cni/install.go b/internal/cni/install.go index 1c8ef2d08..ba23c7ce8 100644 --- a/internal/cni/install.go +++ b/internal/cni/install.go @@ -2,13 +2,13 @@ package cni import ( "context" - "os" "path/filepath" "github.com/pkg/errors" "go.uber.org/zap" "github.com/aws/eks-hybrid/internal/artifact" + "github.com/aws/eks-hybrid/internal/system" "github.com/aws/eks-hybrid/internal/tracker" ) @@ -93,7 +93,7 @@ func downloadFileTo(ctx context.Context, opts InstallOptions) error { } func Uninstall() error { - return os.RemoveAll(rootDir) + return system.SafeRemoveAll(rootDir, false, false) } // Upgrade re-installs the cni-plugins available from the source diff --git a/internal/containerd/install.go b/internal/containerd/install.go index 600941bac..6ce153f04 100644 --- a/internal/containerd/install.go +++ b/internal/containerd/install.go @@ -3,7 +3,6 @@ package containerd import ( "context" "fmt" - "os" "os/exec" "time" @@ -59,7 +58,7 @@ func Uninstall(ctx context.Context, source Source) error { return errors.Wrap(err, "uninstalling containerd") } - if err := os.RemoveAll(containerdConfigDir); err != nil { + if err := system.SafeRemoveAll(containerdConfigDir, false, false); err != nil { return errors.Wrap(err, "removing containerd config files") } } diff --git a/internal/flows/uninstall.go b/internal/flows/uninstall.go index 7da8e72c5..1fa718a42 100644 --- a/internal/flows/uninstall.go +++ b/internal/flows/uninstall.go @@ -3,7 +3,6 @@ package flows import ( "context" "fmt" - "os" "time" "github.com/aws/aws-sdk-go-v2/aws/retry" @@ -21,6 +20,7 @@ import ( "github.com/aws/eks-hybrid/internal/kubelet" "github.com/aws/eks-hybrid/internal/packagemanager" "github.com/aws/eks-hybrid/internal/ssm" + "github.com/aws/eks-hybrid/internal/system" "github.com/aws/eks-hybrid/internal/tracker" ) @@ -167,7 +167,7 @@ func (u *Uninstaller) cleanup() error { return err } - if err := os.RemoveAll(eksConfigDir); err != nil { + if err := system.SafeRemoveAll(eksConfigDir, false, false); err != nil { return err } diff --git a/internal/iamauthenticator/install.go b/internal/iamauthenticator/install.go index 039abd0ca..e36b8524f 100644 --- a/internal/iamauthenticator/install.go +++ b/internal/iamauthenticator/install.go @@ -2,13 +2,13 @@ package iamauthenticator import ( "context" - "os" "path/filepath" "github.com/pkg/errors" "go.uber.org/zap" "github.com/aws/eks-hybrid/internal/artifact" + "github.com/aws/eks-hybrid/internal/system" "github.com/aws/eks-hybrid/internal/tracker" ) @@ -86,7 +86,7 @@ func downloadFileTo(ctx context.Context, opts InstallOptions) error { } func Uninstall() error { - return os.RemoveAll(IAMAuthenticatorBinPath) + return system.SafeRemoveAll(IAMAuthenticatorBinPath, false, false) } func Upgrade(ctx context.Context, src IAMAuthenticatorSource, log *zap.Logger) error { diff --git a/internal/iamrolesanywhere/install.go b/internal/iamrolesanywhere/install.go index fa2ea22d3..325ae33c5 100644 --- a/internal/iamrolesanywhere/install.go +++ b/internal/iamrolesanywhere/install.go @@ -2,7 +2,6 @@ package iamrolesanywhere import ( "context" - "os" "path" "path/filepath" @@ -10,6 +9,7 @@ import ( "go.uber.org/zap" "github.com/aws/eks-hybrid/internal/artifact" + "github.com/aws/eks-hybrid/internal/system" "github.com/aws/eks-hybrid/internal/tracker" ) @@ -85,13 +85,13 @@ func downloadFileTo(ctx context.Context, opts InstallOptions) error { } func Uninstall() error { - if err := os.RemoveAll(SigningHelperServiceFilePath); err != nil { + if err := system.SafeRemoveAll(SigningHelperServiceFilePath, false, false); err != nil { return err } - if err := os.RemoveAll(path.Dir(EksHybridAwsCredentialsPath)); err != nil { + if err := system.SafeRemoveAll(path.Dir(EksHybridAwsCredentialsPath), false, false); err != nil { return err } - return os.RemoveAll(SigningHelperBinPath) + return system.SafeRemoveAll(SigningHelperBinPath, false, false) } func Upgrade(ctx context.Context, signingHelperSrc SigningHelperSource, log *zap.Logger) error { diff --git a/internal/imagecredentialprovider/install.go b/internal/imagecredentialprovider/install.go index fb4c3f844..2682ae1a7 100644 --- a/internal/imagecredentialprovider/install.go +++ b/internal/imagecredentialprovider/install.go @@ -2,7 +2,6 @@ package imagecredentialprovider import ( "context" - "os" "path" "path/filepath" @@ -10,6 +9,7 @@ import ( "go.uber.org/zap" "github.com/aws/eks-hybrid/internal/artifact" + "github.com/aws/eks-hybrid/internal/system" "github.com/aws/eks-hybrid/internal/tracker" ) @@ -87,7 +87,7 @@ func downloadFileTo(ctx context.Context, opts InstallOptions) error { } func Uninstall() error { - return os.RemoveAll(path.Dir(BinPath)) + return system.SafeRemoveAll(path.Dir(BinPath), false, false) } func Upgrade(ctx context.Context, src Source, log *zap.Logger) error { diff --git a/internal/kubectl/install.go b/internal/kubectl/install.go index d505f8cca..a43e4f687 100644 --- a/internal/kubectl/install.go +++ b/internal/kubectl/install.go @@ -2,13 +2,13 @@ package kubectl import ( "context" - "os" "path/filepath" "github.com/pkg/errors" "go.uber.org/zap" "github.com/aws/eks-hybrid/internal/artifact" + "github.com/aws/eks-hybrid/internal/system" "github.com/aws/eks-hybrid/internal/tracker" ) @@ -86,7 +86,7 @@ func downloadFileTo(ctx context.Context, opts InstallOptions) error { } func Uninstall() error { - return os.RemoveAll(BinPath) + return system.SafeRemoveAll(BinPath, false, false) } func Upgrade(ctx context.Context, src Source, log *zap.Logger) error { diff --git a/internal/kubelet/install.go b/internal/kubelet/install.go index 9d9f756b9..f508db6cb 100644 --- a/internal/kubelet/install.go +++ b/internal/kubelet/install.go @@ -13,6 +13,7 @@ import ( "go.uber.org/zap" "github.com/aws/eks-hybrid/internal/artifact" + "github.com/aws/eks-hybrid/internal/system" "github.com/aws/eks-hybrid/internal/tracker" ) @@ -128,7 +129,7 @@ func Uninstall(opts UninstallOptions) error { } for _, path := range pathsToRemove { - if err := os.RemoveAll(path); err != nil { + if err := system.SafeRemoveAll(path, false, false); err != nil { allErrors = append(allErrors, err) } } diff --git a/internal/ssm/daemon.go b/internal/ssm/daemon.go index 78487e04b..0308e13e2 100644 --- a/internal/ssm/daemon.go +++ b/internal/ssm/daemon.go @@ -95,7 +95,7 @@ func (s *ssm) PostLaunch() error { return fmt.Errorf("creating path: %v", err) } - err = os.RemoveAll(symlinkedAWSConfigPath) + err = system.SafeRemoveAll(symlinkedAWSConfigPath, false, false) if err != nil && !os.IsNotExist(err) { return fmt.Errorf("removing directory %s: %v", symlinkedAWSConfigPath, err) } diff --git a/internal/ssm/install.go b/internal/ssm/install.go index b0071abbe..4ac66b7f3 100644 --- a/internal/ssm/install.go +++ b/internal/ssm/install.go @@ -16,6 +16,7 @@ import ( "go.uber.org/zap" "github.com/aws/eks-hybrid/internal/artifact" + "github.com/aws/eks-hybrid/internal/system" "github.com/aws/eks-hybrid/internal/tracker" "github.com/aws/eks-hybrid/internal/util" "github.com/aws/eks-hybrid/internal/util/cmd" @@ -203,7 +204,7 @@ func Uninstall(ctx context.Context, opts UninstallOptions) error { } func removeFileOrDir(path, errorMessage string) error { - if err := os.RemoveAll(path); err != nil { + if err := system.SafeRemoveAll(path, false, false); err != nil { return errors.Wrap(err, errorMessage) } return nil @@ -224,7 +225,7 @@ func uninstallPreRegisterComponents(ctx context.Context, pkgSource PkgSource) er if err := cmd.Retry(ctx, ssmPkg.UninstallCmd, 5*time.Second); err != nil { return errors.Wrapf(err, "uninstalling ssm") } - return os.RemoveAll(defaultInstallerPath) + return system.SafeRemoveAll(defaultInstallerPath, false, false) } func runInstallWithRetries(ctx context.Context, installerPath, region string) error { diff --git a/internal/system/safe_remove.go b/internal/system/safe_remove.go new file mode 100644 index 000000000..651af8429 --- /dev/null +++ b/internal/system/safe_remove.go @@ -0,0 +1,194 @@ +package system + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "time" + + mountutils "k8s.io/mount-utils" +) + +// SafeRemover provides safe removal capabilities with unmount support +type SafeRemover struct { + mounter mountutils.Interface +} + +// NewSafeRemover creates a new SafeRemover instance +func NewSafeRemover() *SafeRemover { + return &SafeRemover{ + mounter: mountutils.New(""), + } +} + +// SafeRemoveAll safely removes a directory, optionally handling mount points +// allowUnmount: if true, attempts to unmount any mount points found +// forceUnmount: if true, uses force unmount when graceful unmount fails +func SafeRemoveAll(path string, allowUnmount bool, forceUnmount bool) error { + remover := NewSafeRemover() + return remover.SafeRemoveAll(path, allowUnmount, forceUnmount) +} + +// SafeRemoveAll is the main entry point that handles both modes +func (sr *SafeRemover) SafeRemoveAll(path string, allowUnmount bool, forceUnmount bool) error { + // Clean and get absolute path + absPath, err := filepath.Abs(path) + if err != nil { + return fmt.Errorf("failed to get absolute path: %w", err) + } + + // Find all mount points within the target path + mountPoints, err := sr.findMountPointsInPath(absPath) + if err != nil { + // If mount utilities are not supported on this platform, fall back to regular removal + if strings.Contains(err.Error(), "not supported") { + return os.RemoveAll(absPath) + } + return fmt.Errorf("failed to find mount points: %w", err) + } + + if len(mountPoints) == 0 { + return os.RemoveAll(absPath) + } + + // Decide action based on allowUnmount flag + if !allowUnmount { + // Safe mode: refuse to delete if mount points exist + return fmt.Errorf("cannot delete %s: contains %d mount points %v (mount points detected)", + absPath, len(mountPoints), mountPoints) + } + + // Unmount mode: attempt to unmount all found mount points + return sr.unmountAndRemove(absPath, mountPoints, forceUnmount) +} + +// findMountPointsInPath finds all mount points within the given path +func (sr *SafeRemover) findMountPointsInPath(targetPath string) ([]string, error) { + var mountPoints []string + + // Walk through the directory tree to find mount points + err := filepath.WalkDir(targetPath, func(path string, d os.DirEntry, err error) error { + if err != nil { + // Skip inaccessible paths but continue walking + return nil + } + + // Check if this path (file or directory) is a mount point + isMounted, err := sr.mounter.IsMountPoint(path) + if err != nil { + // Skip paths we can't check but continue walking + return nil + } + + if isMounted { + mountPoints = append(mountPoints, path) + // Skip walking into directory mount points since they're separate filesystems + // This is an optimization - we don't need to check subdirectories of mount points + // For file mount points, there are no subdirectories to skip + if d.IsDir() && path != targetPath { + return filepath.SkipDir + } + } + + return nil + }) + + if err != nil { + return nil, fmt.Errorf("failed to walk directory tree: %w", err) + } + + return mountPoints, nil +} + +// unmountAndRemove handles the unmount and removal process +func (sr *SafeRemover) unmountAndRemove(targetPath string, mountPoints []string, forceUnmount bool) error { + // Sort mount points by depth (deepest first for unmounting) + sortedMounts := sr.sortMountPointsByDepth(mountPoints) + + // Attempt to unmount all found mount points + for _, mountPoint := range sortedMounts { + if err := sr.unmountWithRetry(mountPoint, forceUnmount); err != nil { + return fmt.Errorf("failed to unmount %s: %w", mountPoint, err) + } + } + + // Wait for unmount operations to complete + time.Sleep(200 * time.Millisecond) + + // Verify all mount points are gone + if err := sr.verifyUnmounted(mountPoints); err != nil { + return fmt.Errorf("verification failed: %w", err) + } + + // Now safe to remove the directory + return os.RemoveAll(targetPath) +} + +// sortMountPointsByDepth sorts mount points by depth (deepest first) +func (sr *SafeRemover) sortMountPointsByDepth(mountPoints []string) []string { + // Create a copy of the slice + sorted := make([]string, 0, len(mountPoints)) + sorted = append(sorted, mountPoints...) + + // Simple bubble sort by path depth (number of separators) + for i := 0; i < len(sorted); i++ { + for j := i + 1; j < len(sorted); j++ { + depthI := strings.Count(sorted[i], string(filepath.Separator)) + depthJ := strings.Count(sorted[j], string(filepath.Separator)) + + // Sort deepest first + if depthI < depthJ { + sorted[i], sorted[j] = sorted[j], sorted[i] + } + } + } + + return sorted +} + +// unmountWithRetry attempts to unmount a path with retry logic +func (sr *SafeRemover) unmountWithRetry(mountPoint string, forceUnmount bool) error { + maxRetries := 3 + + for attempt := 1; attempt <= maxRetries; attempt++ { + // Try graceful unmount first + err := sr.mounter.Unmount(mountPoint) + if err == nil { + return nil + } + + // If graceful unmount fails and force is enabled, try force unmount + if forceUnmount { + if err := sr.platformForceUnmount(mountPoint); err == nil { + return nil + } + } + + // Wait before retry (except on last attempt) + if attempt < maxRetries { + sleepDuration := time.Duration(attempt) * time.Second + time.Sleep(sleepDuration) + } + } + + return fmt.Errorf("failed to unmount after %d attempts", maxRetries) +} + + + +// verifyUnmounted checks that all mount points have been successfully unmounted +func (sr *SafeRemover) verifyUnmounted(mountPoints []string) error { + for _, mountPoint := range mountPoints { + isMounted, err := sr.mounter.IsMountPoint(mountPoint) + if err != nil { + // If we can't check, continue but log the issue + continue + } + if isMounted { + return fmt.Errorf("mount point %s is still mounted after unmount attempt", mountPoint) + } + } + + return nil +} \ No newline at end of file diff --git a/internal/system/safe_remove_linux.go b/internal/system/safe_remove_linux.go new file mode 100644 index 000000000..3c08a2173 --- /dev/null +++ b/internal/system/safe_remove_linux.go @@ -0,0 +1,28 @@ +//go:build linux + +package system + +import ( + "fmt" + "syscall" +) + +// platformForceUnmount performs a force unmount using Linux-specific system calls +func (sr *SafeRemover) platformForceUnmount(mountPoint string) error { + // Try lazy unmount first (MNT_DETACH) - safer option + if err := syscall.Unmount(mountPoint, syscall.MNT_DETACH); err == nil { + return nil + } + + // Try force unmount (MNT_FORCE) - more aggressive + if err := syscall.Unmount(mountPoint, syscall.MNT_FORCE); err == nil { + return nil + } + + // Try both - force + detach + if err := syscall.Unmount(mountPoint, syscall.MNT_FORCE|syscall.MNT_DETACH); err == nil { + return nil + } + + return fmt.Errorf("all unmount methods failed") +} \ No newline at end of file diff --git a/internal/system/safe_remove_other.go b/internal/system/safe_remove_other.go new file mode 100644 index 000000000..74a7ffeff --- /dev/null +++ b/internal/system/safe_remove_other.go @@ -0,0 +1,14 @@ +//go:build !linux + +package system + +import ( + "fmt" +) + +// platformForceUnmount performs a force unmount for non-Linux platforms +func (sr *SafeRemover) platformForceUnmount(mountPoint string) error { + // On non-Linux platforms, we don't have the same syscall constants + // Fall back to using the mount interface only + return fmt.Errorf("force unmount not supported on this platform") +} \ No newline at end of file diff --git a/internal/system/safe_remove_test.go b/internal/system/safe_remove_test.go new file mode 100644 index 000000000..dcb7c98f0 --- /dev/null +++ b/internal/system/safe_remove_test.go @@ -0,0 +1,462 @@ +package system + +import ( + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + mountutils "k8s.io/mount-utils" +) + +// MockMounter is a mock implementation of mount.Interface for testing +type MockMounter struct { + mock.Mock +} + +func (m *MockMounter) Mount(source string, target string, fstype string, options []string) error { + args := m.Called(source, target, fstype, options) + return args.Error(0) +} + +func (m *MockMounter) MountSensitive(source string, target string, fstype string, options []string, sensitiveOptions []string) error { + args := m.Called(source, target, fstype, options, sensitiveOptions) + return args.Error(0) +} + +func (m *MockMounter) MountSensitiveWithoutSystemd(source string, target string, fstype string, options []string, sensitiveOptions []string) error { + args := m.Called(source, target, fstype, options, sensitiveOptions) + return args.Error(0) +} + +func (m *MockMounter) MountSensitiveWithoutSystemdWithMountFlags(source string, target string, fstype string, options []string, sensitiveOptions []string, mountFlags []string) error { + args := m.Called(source, target, fstype, options, sensitiveOptions, mountFlags) + return args.Error(0) +} + +func (m *MockMounter) Unmount(target string) error { + args := m.Called(target) + return args.Error(0) +} + +func (m *MockMounter) List() ([]mountutils.MountPoint, error) { + args := m.Called() + return args.Get(0).([]mountutils.MountPoint), args.Error(1) +} + +func (m *MockMounter) IsMountPoint(file string) (bool, error) { + args := m.Called(file) + return args.Bool(0), args.Error(1) +} + +func (m *MockMounter) IsLikelyNotMountPoint(file string) (bool, error) { + args := m.Called(file) + return args.Bool(0), args.Error(1) +} + +func (m *MockMounter) GetMountRefs(pathname string) ([]string, error) { + args := m.Called(pathname) + return args.Get(0).([]string), args.Error(1) +} + +func (m *MockMounter) CanSafelySkipMountPointCheck() bool { + args := m.Called() + return args.Bool(0) +} + +func TestNewSafeRemover(t *testing.T) { + remover := NewSafeRemover() + assert.NotNil(t, remover) + assert.NotNil(t, remover.mounter) +} + +func TestSafeRemoveAll_Function(t *testing.T) { + // Test the package-level function + tempDir, err := os.MkdirTemp("", "safe-remove-test-*") + require.NoError(t, err) + defer os.RemoveAll(tempDir) + + // Create a test file + testFile := filepath.Join(tempDir, "test.txt") + err = os.WriteFile(testFile, []byte("test"), 0644) + require.NoError(t, err) + + // Should remove successfully when no mount points + err = SafeRemoveAll(tempDir, false, false) + assert.NoError(t, err) + assert.NoFileExists(t, tempDir) +} + +func TestSafeRemover_SafeRemoveAll_NoMountPoints(t *testing.T) { + mockMounter := &MockMounter{} + remover := &SafeRemover{mounter: mockMounter} + + tempDir, err := os.MkdirTemp("", "safe-remove-test-*") + require.NoError(t, err) + defer os.RemoveAll(tempDir) + + // Create test structure + testFile := filepath.Join(tempDir, "test.txt") + err = os.WriteFile(testFile, []byte("test"), 0644) + require.NoError(t, err) + + subDir := filepath.Join(tempDir, "subdir") + err = os.MkdirAll(subDir, 0755) + require.NoError(t, err) + + // Mock: no mount points found + mockMounter.On("IsMountPoint", mock.AnythingOfType("string")).Return(false, nil) + + err = remover.SafeRemoveAll(tempDir, false, false) + assert.NoError(t, err) + assert.NoFileExists(t, tempDir) + + mockMounter.AssertExpectations(t) +} + +func TestSafeRemover_SafeRemoveAll_WithMountPoints_DisallowUnmount(t *testing.T) { + mockMounter := &MockMounter{} + remover := &SafeRemover{mounter: mockMounter} + + tempDir, err := os.MkdirTemp("", "safe-remove-test-*") + require.NoError(t, err) + defer os.RemoveAll(tempDir) + + mountDir := filepath.Join(tempDir, "mounted") + err = os.MkdirAll(mountDir, 0755) + require.NoError(t, err) + + // Mock: mount point found, unmount not allowed + mockMounter.On("IsMountPoint", tempDir).Return(false, nil) + mockMounter.On("IsMountPoint", mountDir).Return(true, nil) + + err = remover.SafeRemoveAll(tempDir, false, false) + assert.Error(t, err) + assert.Contains(t, err.Error(), "contains 1 mount points") + assert.Contains(t, err.Error(), "mount points detected") + + mockMounter.AssertExpectations(t) +} + +func TestSafeRemover_SafeRemoveAll_WithMountPoints_AllowUnmount_Success(t *testing.T) { + mockMounter := &MockMounter{} + remover := &SafeRemover{mounter: mockMounter} + + tempDir, err := os.MkdirTemp("", "safe-remove-test-*") + require.NoError(t, err) + defer os.RemoveAll(tempDir) + + mountDir := filepath.Join(tempDir, "mounted") + err = os.MkdirAll(mountDir, 0755) + require.NoError(t, err) + + // Mock: mount point found during discovery + mockMounter.On("IsMountPoint", tempDir).Return(false, nil).Once() + mockMounter.On("IsMountPoint", mountDir).Return(true, nil).Once() + // Mock: unmount succeeds + mockMounter.On("Unmount", mountDir).Return(nil).Once() + // Mock: verification shows mount point is gone + mockMounter.On("IsMountPoint", mountDir).Return(false, nil).Once() + + err = remover.SafeRemoveAll(tempDir, true, false) + assert.NoError(t, err) + assert.NoFileExists(t, tempDir) + + mockMounter.AssertExpectations(t) +} + +func TestSafeRemover_SafeRemoveAll_WithMountPoints_UnmountFails(t *testing.T) { + mockMounter := &MockMounter{} + remover := &SafeRemover{mounter: mockMounter} + + tempDir, err := os.MkdirTemp("", "safe-remove-test-*") + require.NoError(t, err) + defer os.RemoveAll(tempDir) + + mountDir := filepath.Join(tempDir, "mounted") + err = os.MkdirAll(mountDir, 0755) + require.NoError(t, err) + + // Mock: mount point found, unmount fails + mockMounter.On("IsMountPoint", tempDir).Return(false, nil) + mockMounter.On("IsMountPoint", mountDir).Return(true, nil) + mockMounter.On("Unmount", mountDir).Return(fmt.Errorf("unmount failed")).Times(3) // 3 retries + + err = remover.SafeRemoveAll(tempDir, true, false) + assert.Error(t, err) + assert.Contains(t, err.Error(), "failed to unmount") + + mockMounter.AssertExpectations(t) +} + +func TestSafeRemover_SafeRemoveAll_PlatformNotSupported(t *testing.T) { + mockMounter := &MockMounter{} + remover := &SafeRemover{mounter: mockMounter} + + tempDir, err := os.MkdirTemp("", "safe-remove-test-*") + require.NoError(t, err) + defer os.RemoveAll(tempDir) + + // Mock: platform not supported error + mockMounter.On("IsMountPoint", mock.AnythingOfType("string")).Return(false, fmt.Errorf("util/mount on this platform is not supported")) + + err = remover.SafeRemoveAll(tempDir, false, false) + assert.NoError(t, err) // Should fallback to os.RemoveAll + assert.NoFileExists(t, tempDir) + + mockMounter.AssertExpectations(t) +} + +func TestSafeRemover_findMountPointsInPath_NoMountPoints(t *testing.T) { + mockMounter := &MockMounter{} + remover := &SafeRemover{mounter: mockMounter} + + tempDir, err := os.MkdirTemp("", "safe-remove-test-*") + require.NoError(t, err) + defer os.RemoveAll(tempDir) + + // Create test structure + subDir := filepath.Join(tempDir, "subdir") + err = os.MkdirAll(subDir, 0755) + require.NoError(t, err) + + testFile := filepath.Join(tempDir, "test.txt") + err = os.WriteFile(testFile, []byte("test"), 0644) + require.NoError(t, err) + + // Mock: no mount points + mockMounter.On("IsMountPoint", mock.AnythingOfType("string")).Return(false, nil) + + mountPoints, err := remover.findMountPointsInPath(tempDir) + assert.NoError(t, err) + assert.Empty(t, mountPoints) + + mockMounter.AssertExpectations(t) +} + +func TestSafeRemover_findMountPointsInPath_WithDirectoryMountPoint(t *testing.T) { + mockMounter := &MockMounter{} + remover := &SafeRemover{mounter: mockMounter} + + tempDir, err := os.MkdirTemp("", "safe-remove-test-*") + require.NoError(t, err) + defer os.RemoveAll(tempDir) + + mountDir := filepath.Join(tempDir, "mounted") + err = os.MkdirAll(mountDir, 0755) + require.NoError(t, err) + + subMountDir := filepath.Join(mountDir, "submount") + err = os.MkdirAll(subMountDir, 0755) + require.NoError(t, err) + + // Mock: mountDir is a mount point, should skip checking submount + mockMounter.On("IsMountPoint", tempDir).Return(false, nil) + mockMounter.On("IsMountPoint", mountDir).Return(true, nil) + // subMountDir should not be checked due to SkipDir optimization + + mountPoints, err := remover.findMountPointsInPath(tempDir) + assert.NoError(t, err) + assert.Len(t, mountPoints, 1) + assert.Contains(t, mountPoints, mountDir) + + mockMounter.AssertExpectations(t) +} + +func TestSafeRemover_findMountPointsInPath_WithFileMountPoint(t *testing.T) { + mockMounter := &MockMounter{} + remover := &SafeRemover{mounter: mockMounter} + + tempDir, err := os.MkdirTemp("", "safe-remove-test-*") + require.NoError(t, err) + defer os.RemoveAll(tempDir) + + // Create a regular file + mountedFile := filepath.Join(tempDir, "mounted-file.txt") + err = os.WriteFile(mountedFile, []byte("mounted"), 0644) + require.NoError(t, err) + + // Create a regular directory + regularDir := filepath.Join(tempDir, "regular") + err = os.MkdirAll(regularDir, 0755) + require.NoError(t, err) + + // Mock: file is a mount point, directory is not + mockMounter.On("IsMountPoint", tempDir).Return(false, nil) + mockMounter.On("IsMountPoint", mountedFile).Return(true, nil) + mockMounter.On("IsMountPoint", regularDir).Return(false, nil) + + mountPoints, err := remover.findMountPointsInPath(tempDir) + assert.NoError(t, err) + assert.Len(t, mountPoints, 1) + assert.Contains(t, mountPoints, mountedFile) + + mockMounter.AssertExpectations(t) +} + +func TestSafeRemover_findMountPointsInPath_ErrorHandling(t *testing.T) { + mockMounter := &MockMounter{} + remover := &SafeRemover{mounter: mockMounter} + + tempDir, err := os.MkdirTemp("", "safe-remove-test-*") + require.NoError(t, err) + defer os.RemoveAll(tempDir) + + // Mock: IsMountPoint returns error, should be skipped + mockMounter.On("IsMountPoint", mock.AnythingOfType("string")).Return(false, fmt.Errorf("permission denied")) + + mountPoints, err := remover.findMountPointsInPath(tempDir) + assert.NoError(t, err) // Errors are skipped, not propagated + assert.Empty(t, mountPoints) + + mockMounter.AssertExpectations(t) +} + +func TestSafeRemover_sortMountPointsByDepth(t *testing.T) { + remover := &SafeRemover{} + + mountPoints := []string{ + "/a", + "/a/b/c/d", + "/a/b", + "/a/b/c", + } + + sorted := remover.sortMountPointsByDepth(mountPoints) + + expected := []string{ + "/a/b/c/d", // depth 4 + "/a/b/c", // depth 3 + "/a/b", // depth 2 + "/a", // depth 1 + } + + assert.Equal(t, expected, sorted) +} + +func TestSafeRemover_unmountWithRetry_Success(t *testing.T) { + mockMounter := &MockMounter{} + remover := &SafeRemover{mounter: mockMounter} + + mountPoint := "/test/mount" + + // Mock: unmount succeeds on first try + mockMounter.On("Unmount", mountPoint).Return(nil).Once() + + err := remover.unmountWithRetry(mountPoint, false) + assert.NoError(t, err) + + mockMounter.AssertExpectations(t) +} + +func TestSafeRemover_unmountWithRetry_FailsWithoutForce(t *testing.T) { + mockMounter := &MockMounter{} + remover := &SafeRemover{mounter: mockMounter} + + mountPoint := "/test/mount" + + // Mock: unmount fails 3 times + mockMounter.On("Unmount", mountPoint).Return(fmt.Errorf("device busy")).Times(3) + + err := remover.unmountWithRetry(mountPoint, false) + assert.Error(t, err) + assert.Contains(t, err.Error(), "failed to unmount after 3 attempts") + + mockMounter.AssertExpectations(t) +} + +func TestSafeRemover_verifyUnmounted_Success(t *testing.T) { + mockMounter := &MockMounter{} + remover := &SafeRemover{mounter: mockMounter} + + mountPoints := []string{"/test/mount1", "/test/mount2"} + + // Mock: all mount points are unmounted + for _, mp := range mountPoints { + mockMounter.On("IsMountPoint", mp).Return(false, nil) + } + + err := remover.verifyUnmounted(mountPoints) + assert.NoError(t, err) + + mockMounter.AssertExpectations(t) +} + +func TestSafeRemover_verifyUnmounted_StillMounted(t *testing.T) { + mockMounter := &MockMounter{} + remover := &SafeRemover{mounter: mockMounter} + + mountPoints := []string{"/test/mount1", "/test/mount2"} + + // Mock: first mount point is still mounted + mockMounter.On("IsMountPoint", "/test/mount1").Return(true, nil) + + err := remover.verifyUnmounted(mountPoints) + assert.Error(t, err) + assert.Contains(t, err.Error(), "is still mounted after unmount attempt") + + mockMounter.AssertExpectations(t) +} + +func TestSafeRemover_verifyUnmounted_ErrorChecking(t *testing.T) { + mockMounter := &MockMounter{} + remover := &SafeRemover{mounter: mockMounter} + + mountPoints := []string{"/test/mount1"} + + // Mock: error checking mount status (should be ignored) + mockMounter.On("IsMountPoint", "/test/mount1").Return(false, fmt.Errorf("permission denied")) + + err := remover.verifyUnmounted(mountPoints) + assert.NoError(t, err) // Errors are ignored in verification + + mockMounter.AssertExpectations(t) +} + +func TestSafeRemover_InvalidPath(t *testing.T) { + mockMounter := &MockMounter{} + remover := &SafeRemover{mounter: mockMounter} + + // Test with invalid path characters that might cause filepath.Abs to fail + invalidPath := string([]byte{0, 1, 2}) // null bytes and control characters + + err := remover.SafeRemoveAll(invalidPath, false, false) + // On most systems, this should either work or fail gracefully + // The exact behavior depends on the OS, so we just ensure it doesn't panic + _ = err // We don't assert specific error since behavior varies by OS +} + +// Benchmark tests +func BenchmarkSafeRemover_findMountPointsInPath(b *testing.B) { + mockMounter := &MockMounter{} + remover := &SafeRemover{mounter: mockMounter} + + tempDir, err := os.MkdirTemp("", "benchmark-test-*") + require.NoError(b, err) + defer os.RemoveAll(tempDir) + + // Create a moderately complex directory structure + for i := 0; i < 10; i++ { + dir := filepath.Join(tempDir, fmt.Sprintf("dir%d", i)) + err = os.MkdirAll(dir, 0755) + require.NoError(b, err) + + for j := 0; j < 5; j++ { + subdir := filepath.Join(dir, fmt.Sprintf("subdir%d", j)) + err = os.MkdirAll(subdir, 0755) + require.NoError(b, err) + } + } + + // Mock: no mount points + mockMounter.On("IsMountPoint", mock.AnythingOfType("string")).Return(false, nil) + + b.ResetTimer() + for i := 0; i < b.N; i++ { + _, err := remover.findMountPointsInPath(tempDir) + require.NoError(b, err) + } +} \ No newline at end of file diff --git a/internal/tracker/artifacts.go b/internal/tracker/artifacts.go index 1d3c57e7d..70120dfd7 100644 --- a/internal/tracker/artifacts.go +++ b/internal/tracker/artifacts.go @@ -10,6 +10,7 @@ import ( "sigs.k8s.io/yaml" "github.com/aws/eks-hybrid/internal/artifact" + "github.com/aws/eks-hybrid/internal/system" "github.com/aws/eks-hybrid/internal/util" ) @@ -81,7 +82,7 @@ func (tracker *Tracker) Save() error { } func Clear() error { - return os.RemoveAll(path.Dir(trackerFile)) + return system.SafeRemoveAll(path.Dir(trackerFile), false, false) } // GetInstalledArtifacts reads the tracker file and returns the current diff --git a/test/integration/cases/uninstall-with-mounted-filesystem/run.sh b/test/integration/cases/uninstall-with-mounted-filesystem/run.sh new file mode 100755 index 000000000..d993ee686 --- /dev/null +++ b/test/integration/cases/uninstall-with-mounted-filesystem/run.sh @@ -0,0 +1,73 @@ +#!/usr/bin/env bash + +set -o errexit +set -o nounset +set -o pipefail + +source /helpers.sh +source /test-constants.sh + +mock::aws +wait::dbus-ready + +# remove previously installed containerd to test installation via nodeadm +dnf remove -y containerd + +# Install a version to test uninstall functionality +nodeadm install $CURRENT_VERSION --credential-provider ssm + +# Create test directories and files that would normally be cleaned up +mkdir -p /var/lib/kubelet/test-data +mkdir -p /var/lib/cni/test-data +mkdir -p /etc/kubernetes/test-data + +# Create some test files +echo "test-kubelet-data" > /var/lib/kubelet/test-data/file +echo "test-cni-data" > /var/lib/cni/test-data/file +echo "test-k8s-data" > /etc/kubernetes/test-data/file + +echo "๐Ÿงช Testing normal uninstall behavior..." +echo "๐Ÿ“ Files before uninstall:" +ls -la /var/lib/kubelet/test-data/ || echo "kubelet test-data not found" +ls -la /var/lib/cni/test-data/ || echo "cni test-data not found" +ls -la /etc/kubernetes/test-data/ || echo "kubernetes test-data not found" + +# Test 1: Normal uninstall - should work with current SafeRemoveAll(dst, false, false) +echo "๐Ÿงช Testing normal uninstall..." +nodeadm uninstall --skip node-validation,pod-validation + +echo "๐Ÿ“ Files after normal uninstall:" +ls -la /var/lib/kubelet/test-data/ 2>/dev/null || echo "โœ… kubelet test-data removed" +ls -la /var/lib/cni/test-data/ 2>/dev/null || echo "โœ… cni test-data removed" +ls -la /etc/kubernetes/test-data/ 2>/dev/null || echo "โœ… kubernetes test-data removed" + +# Test 2: Install again and test force uninstall +echo "๐Ÿงช Installing again for force uninstall test..." +nodeadm install $CURRENT_VERSION --credential-provider ssm + +# Create test data again +mkdir -p /var/lib/kubelet/test-force +mkdir -p /var/lib/cni/test-force +mkdir -p /etc/kubernetes/test-force + +echo "test-kubelet-force" > /var/lib/kubelet/test-force/file +echo "test-cni-force" > /var/lib/cni/test-force/file +echo "test-k8s-force" > /etc/kubernetes/test-force/file + +echo "๐Ÿงช Testing force uninstall..." +nodeadm uninstall --skip node-validation,pod-validation --force + +echo "๐Ÿ“ Files after force uninstall:" +ls -la /var/lib/kubelet/test-force/ 2>/dev/null || echo "โœ… kubelet test-force removed" +ls -la /var/lib/cni/test-force/ 2>/dev/null || echo "โœ… cni test-force removed" +ls -la /etc/kubernetes/test-force/ 2>/dev/null || echo "โœ… kubernetes test-force removed" + +# Verify key directories are cleaned up as expected +assert::path-not-exist /var/lib/kubelet/test-force/file +assert::path-not-exist /var/lib/cni/test-force/file +assert::path-not-exist /etc/kubernetes/test-force/file + +echo "โœ… SafeRemoveAll integration test completed successfully" +echo "๐Ÿ“ Note: This test validates current behavior with SafeRemoveAll(dst, false, false)" +echo "๐Ÿ“ When you extend to use allowUnmount/forceUnmount parameters, this test" +echo "๐Ÿ“ can be enhanced to test actual mount point handling." \ No newline at end of file diff --git a/test/integration/helpers.sh b/test/integration/helpers.sh index d5105d032..7a0ca0798 100644 --- a/test/integration/helpers.sh +++ b/test/integration/helpers.sh @@ -434,4 +434,126 @@ function mock::iamra-certificate-path() { echo "IAMRA One or both files are missing" exit 1 fi +} + +# Mount point testing helpers for SafeRemoveAll integration tests +function assert::command-success() { + if [ "$#" -ne 1 ]; then + echo "Usage: assert::command-success 'command'" + exit 1 + fi + local COMMAND=$1 + if ! eval "$COMMAND"; then + echo "Command failed: $COMMAND" + exit 1 + fi +} + +function assert::command-failure() { + if [ "$#" -ne 1 ]; then + echo "Usage: assert::command-failure 'command'" + exit 1 + fi + local COMMAND=$1 + if eval "$COMMAND"; then + echo "Command unexpectedly succeeded: $COMMAND" + exit 1 + fi +} + +function assert::mount-point-exists() { + if [ "$#" -ne 1 ]; then + echo "Usage: assert::mount-point-exists PATH" + exit 1 + fi + local PATH=$1 + if ! is_mount_point "$PATH"; then + echo "Path $PATH is not a mount point" + exit 1 + fi +} + +function assert::mount-point-not-exists() { + if [ "$#" -ne 1 ]; then + echo "Usage: assert::mount-point-not-exists PATH" + exit 1 + fi + local PATH=$1 + if is_mount_point "$PATH"; then + echo "Path $PATH is unexpectedly a mount point" + exit 1 + fi +} + +# Helper function to check if a path is a mount point using /proc/mounts +function is_mount_point() { + local PATH=$1 + # Resolve to absolute path + PATH=$(realpath "$PATH" 2>/dev/null || echo "$PATH") + + # Check if path exists in /proc/mounts + if grep -q " $PATH " /proc/mounts 2>/dev/null; then + return 0 + fi + + # Alternative check: use cut to get second field + if cut -d' ' -f2 /proc/mounts | grep -q "^$PATH$" 2>/dev/null; then + return 0 + fi + + # Alternative: use findmnt if available + if command -v findmnt >/dev/null 2>&1; then + if findmnt "$PATH" >/dev/null 2>&1; then + return 0 + fi + fi + + return 1 +} + +function create::bind-mount() { + if [ "$#" -ne 2 ]; then + echo "Usage: create::bind-mount SOURCE TARGET" + exit 1 + fi + local SOURCE=$1 + local TARGET=$2 + + # Ensure target exists + if [ -d "$SOURCE" ]; then + mkdir -p "$TARGET" + else + mkdir -p "$(dirname "$TARGET")" + touch "$TARGET" + fi + + mount --bind "$SOURCE" "$TARGET" + echo "Created bind mount: $SOURCE -> $TARGET" +} + +function cleanup::mount() { + if [ "$#" -ne 1 ]; then + echo "Usage: cleanup::mount PATH" + exit 1 + fi + local PATH=$1 + + if is_mount_point "$PATH"; then + umount "$PATH" || { + echo "Warning: Failed to unmount $PATH, trying lazy unmount" + umount -l "$PATH" || echo "Warning: Lazy unmount also failed for $PATH" + } + echo "Cleaned up mount: $PATH" + fi +} + +function list::mount-points-in-path() { + if [ "$#" -ne 1 ]; then + echo "Usage: list::mount-points-in-path PATH" + exit 1 + fi + local PATH=$1 + + echo "Mount points under $PATH:" + findmnt -R "$PATH" 2>/dev/null || echo "No mount points found under $PATH" } \ No newline at end of file