From d56ec952d91338f600745925fab56e4bb7dffa6d Mon Sep 17 00:00:00 2001 From: Rajan Agaskar Date: Wed, 18 Jun 2025 15:02:07 -0700 Subject: [PATCH 1/2] Tar uncompress can accomodate paths w/ symlinks. - Windows recently changed such that System-user run processes receive C:\Windows\SystemTemp. In turn, the Windows 2019.x stemcell was updated such that C:\Windows\SystemTemp is a symlink to the boshTmpDir (`\var\vcap\data\tmp\` via bosh-agent changes). - This behavior change subsequently caused bosh-utils tests to fail, since these tests attempt to extract tars to a path including a symlink, which tar does not permit. - There are at least two ways to fix this problem: the bosh-utils test environment can arbitarily configure a temp directory that it "knows" does not include a symlink (e.g., by hard-coding to C:\Windows\Temp, which is NOT symlinked). An advantage here is behavior changes are limited to Windows behavior. A disadvantage is this shifts the responsibility for that tar extract destination paths do NOT include symlinks downstream. To be fair, downstream consumers have _ALWAYS_ had this responsibility, _however_, Windows behavior has changed such that it is almost certain that prior invocations of tar extraction with temp dirs that were previously successful will now fail. Alternatively, one can choose to change the tar uncompress behavior such that it will resolve the path _prior_ to an extraction. This has a larger impact in terms of change as it will also affect linux users, HOWEVER, this _should_ represent additive behavior to what consumers previously experienced. That is, current users of this method should be unaffected, including those users who would otherwise be impacted by the Windows stemcell changes. Further, I cannot currently imagine any previously valid invocations that would fail as a result of these changes, thus this latter change seems like a better choice, even given the behavior change. - As mentioned above, this commit should fix any tar extraction issues observed in the DecompressFileToDir function after upgrading to the Windows 2019.87 stemcell. This change should be backwards compatible. --- fileutil/fileutil_suite_test.go | 6 ++++-- fileutil/tarball_compressor.go | 8 ++++++-- fileutil/tarball_compressor_test.go | 4 +++- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/fileutil/fileutil_suite_test.go b/fileutil/fileutil_suite_test.go index 0c60ad39..6d6a467d 100644 --- a/fileutil/fileutil_suite_test.go +++ b/fileutil/fileutil_suite_test.go @@ -59,10 +59,12 @@ func localCopyFSForGo122(dir string, fsys fs.FS) error { var _ = BeforeEach(func() { assetsDirFS := os.DirFS(filepath.Join(".", "test_assets")) - testAssetsDir = GinkgoT().TempDir() + var err error + testAssetsDir, err = filepath.EvalSymlinks(GinkgoT().TempDir()) + Expect(err).ToNot(HaveOccurred()) // TODO: use `os.CopyFS` instead of `localCopyFSForGo122` once we upgrade Golang versions - err := localCopyFSForGo122(testAssetsDir, assetsDirFS) + err = localCopyFSForGo122(testAssetsDir, assetsDirFS) Expect(err).NotTo(HaveOccurred()) testAssetsFixtureDir = filepath.Join(testAssetsDir, "fixture_dir") diff --git a/fileutil/tarball_compressor.go b/fileutil/tarball_compressor.go index df65d144..997f1f3c 100644 --- a/fileutil/tarball_compressor.go +++ b/fileutil/tarball_compressor.go @@ -57,7 +57,11 @@ func (c tarballCompressor) DecompressFileToDir(tarballPath string, dir string, o sameOwnerOption = "--same-owner" } - args := []string{sameOwnerOption, "-xzf", tarballPath, "-C", dir} + resolvedTarballPath, err := c.fs.ReadAndFollowLink(tarballPath) + if err != nil { + return bosherr.WrapError(err, "Resolving tarball path") + } + args := []string{sameOwnerOption, "-xzf", resolvedTarballPath, "-C", dir} if options.StripComponents != 0 { args = append(args, fmt.Sprintf("--strip-components=%d", options.StripComponents)) } @@ -65,7 +69,7 @@ func (c tarballCompressor) DecompressFileToDir(tarballPath string, dir string, o if options.PathInArchive != "" { args = append(args, options.PathInArchive) } - _, _, _, err := c.cmdRunner.RunCommand("tar", args...) + _, _, _, err = c.cmdRunner.RunCommand("tar", args...) if err != nil { return bosherr.WrapError(err, "Shelling out to tar") } diff --git a/fileutil/tarball_compressor_test.go b/fileutil/tarball_compressor_test.go index eace9202..2a80682e 100644 --- a/fileutil/tarball_compressor_test.go +++ b/fileutil/tarball_compressor_test.go @@ -35,7 +35,9 @@ var _ = Describe("tarballCompressor", func() { cmdRunner = boshsys.NewExecCmdRunner(logger) fs = boshsys.NewOsFileSystem(logger) - dstDir = GinkgoT().TempDir() + var err error + dstDir, err = filepath.EvalSymlinks(GinkgoT().TempDir()) + Expect(err).ToNot(HaveOccurred()) compressor = NewTarballCompressor(cmdRunner, fs) fixtureSrcTgz = filepath.Join(testAssetsDir, "compressor-decompress-file-to-dir.tgz") From dea3738d13c28f239caa32639fc248a295ce3b4a Mon Sep 17 00:00:00 2001 From: Rajan Agaskar Date: Wed, 18 Jun 2025 16:14:09 -0700 Subject: [PATCH 2/2] Fix test failing on Github Actions. - The "home dir" test was failing when the suite was run w/ Github Actions. We see these failures _begin_ with b6aa58b, which I believe was an attempt to address failures occuring as a knock-on effect on the changes in 5ec2200, which apparently did not sufficiently consider the various users which the test can be run as. It doesn't seem like those changes were correct for the Github Actions context, where user.Name does not seem to be set (it may be that the intended string _was_ user.Username, because I'm not sure the filepath.Base was needed for the Name case). In any circumstance, user.Username seems to work for both cases -- if possible it may be sensible to set a hardcoded expectation around the value of this string itself (e.g. as "runneradmin") to make these kind of cross-context failures easier to track down, although I suspect this kind of check would have to switch on environment. - In any case, for now, this change appears to allow _both_ Github Actions and other CI pipeline test runners that may be in use to successfully pass, and also seems to reasonably represent the original intent of the test (the created homedir should contain the username). --- system/os_file_system_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/os_file_system_test.go b/system/os_file_system_test.go index 5bdc56fe..2de7115d 100644 --- a/system/os_file_system_test.go +++ b/system/os_file_system_test.go @@ -58,7 +58,7 @@ var _ = Describe("OS FileSystem", func() { Expect(err).ToNot(HaveOccurred()) // If a regular user, the home directory will end with the username - expDir := fmt.Sprintf(`\%s`, filepath.Base(currentUser.Name)) + expDir := fmt.Sprintf(`\%s`, filepath.Base(currentUser.Username)) // If a System or LocalSystem user, the home directory will be different // ref: https://learn.microsoft.com/en-us/windows-server/identity/ad-ds/manage/understand-security-identifiers