Skip to content

Commit bd6e168

Browse files
[9.0] (backport #9322) Enhancement/5235 handle insufficient disk space errors in artifact unpack (#9820)
* Enhancement/5235 handle insufficient disk space errors in artifact unpack (#9322) * enhancement(5235): added stdlib wrappers in step_unpack * enhancement(5235): wrap errors in step_unpack * enhancement(5235): removed nolint:gosec comment from step_unpack * enhancement(5235): refactored unpack step to abstract the unpack function from upgrader. abstracted unzip and untar to be able to test unpack function. updated unpack step tests to test for stdlib failures in unzip and untar and added tests for unpack * enhancement(5235}: added unpackHandler interface to abstract the unpack function from upgrader * enhancement(5235): added getPackageMetadata into unpackHandler interface for testability * enhancement(5235): using unpacker.getPackageMetadata * enhancement(5235): added abstractions in upgrader for testability * enhancement(5235): removed unpack specific test from upgrade tests, added test cases in the upgrade error handling test * enhancement(5235): using formatted test assertion api (cherry picked from commit f70ff02) # Conflicts: # internal/pkg/agent/application/upgrade/step_unpack_test.go * resolve conflicts --------- Co-authored-by: Kaan Yalti <[email protected]>
1 parent fd7a745 commit bd6e168

File tree

4 files changed

+355
-63
lines changed

4 files changed

+355
-63
lines changed

internal/pkg/agent/application/upgrade/step_unpack.go

Lines changed: 74 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -35,16 +35,43 @@ type UnpackResult struct {
3535
VersionedHome string `json:"versioned-home" yaml:"versioned-home"`
3636
}
3737

38+
type copyFunc func(dst io.Writer, src io.Reader) (written int64, err error)
39+
type mkdirAllFunc func(name string, perm fs.FileMode) error
40+
type openFileFunc func(name string, flag int, perm fs.FileMode) (*os.File, error)
41+
type unarchiveFunc func(log *logger.Logger, archivePath, dataDir string, flavor string, copy copyFunc, mkdirAll mkdirAllFunc, openFile openFileFunc) (UnpackResult, error)
42+
43+
type unpacker struct {
44+
log *logger.Logger
45+
// Abstractsions for testability
46+
unzip unarchiveFunc
47+
untar unarchiveFunc
48+
// stdlib abstractions for testability
49+
copy copyFunc
50+
mkdirAll mkdirAllFunc
51+
openFile openFileFunc
52+
}
53+
54+
func newUnpacker(log *logger.Logger) *unpacker {
55+
return &unpacker{
56+
log: log,
57+
unzip: unzip,
58+
untar: untar,
59+
copy: io.Copy,
60+
mkdirAll: os.MkdirAll,
61+
openFile: os.OpenFile,
62+
}
63+
}
64+
3865
// unpack unpacks archive correctly, skips root (symlink, config...) unpacks data/*
39-
func (u *Upgrader) unpack(version, archivePath, dataDir string, flavor string) (UnpackResult, error) {
66+
func (u *unpacker) unpack(version, archivePath, dataDir string, flavor string) (UnpackResult, error) {
4067
// unpack must occur in directory that holds the installation directory
4168
// or the extraction will be double nested
4269
var unpackRes UnpackResult
4370
var err error
4471
if runtime.GOOS == windows {
45-
unpackRes, err = unzip(u.log, archivePath, dataDir, flavor)
72+
unpackRes, err = u.unzip(u.log, archivePath, dataDir, flavor, u.copy, u.mkdirAll, u.openFile)
4673
} else {
47-
unpackRes, err = untar(u.log, archivePath, dataDir, flavor)
74+
unpackRes, err = u.untar(u.log, archivePath, dataDir, flavor, u.copy, u.mkdirAll, u.openFile)
4875
}
4976

5077
if err != nil {
@@ -61,7 +88,7 @@ type packageMetadata struct {
6188
hash string
6289
}
6390

64-
func (u *Upgrader) getPackageMetadata(archivePath string) (packageMetadata, error) {
91+
func (u *unpacker) getPackageMetadata(archivePath string) (packageMetadata, error) {
6592
ext := filepath.Ext(archivePath)
6693
if ext == ".gz" {
6794
// if we got gzip extension we need another extension before last
@@ -78,7 +105,8 @@ func (u *Upgrader) getPackageMetadata(archivePath string) (packageMetadata, erro
78105
}
79106
}
80107

81-
func unzip(log *logger.Logger, archivePath, dataDir string, flavor string) (UnpackResult, error) {
108+
// injecting copy, mkdirAll and openFile for testability
109+
func unzip(log *logger.Logger, archivePath, dataDir string, flavor string, copy copyFunc, mkdirAll mkdirAllFunc, openFile openFileFunc) (UnpackResult, error) {
82110
var hash, rootDir string
83111
r, err := zip.OpenReader(archivePath)
84112
if err != nil {
@@ -148,8 +176,10 @@ func unzip(log *logger.Logger, archivePath, dataDir string, flavor string) (Unpa
148176
// check if the directory already exists
149177
_, err = os.Stat(dstPath)
150178
if errors.Is(err, fs.ErrNotExist) {
151-
// the directory does not exist, create it and any non-existing parent directory with the same permissions
152-
if err := os.MkdirAll(dstPath, f.Mode().Perm()&0770); err != nil {
179+
// the directory does not exist, create it and any non-existing
180+
// parent directory with the same permissions.
181+
// Using mkdirAll instead of os.MkdirAll so that we can mock it in tests.
182+
if err := mkdirAll(dstPath, f.Mode().Perm()&0770); err != nil {
153183
return fmt.Errorf("creating directory %q: %w", dstPath, err)
154184
}
155185
} else if err != nil {
@@ -162,13 +192,23 @@ func unzip(log *logger.Logger, archivePath, dataDir string, flavor string) (Unpa
162192
}
163193
}
164194

165-
_ = os.MkdirAll(dstPath, f.Mode()&0770)
195+
// Using mkdirAll instead of os.MkdirAll so that we can mock it in tests.
196+
err = mkdirAll(dstPath, f.Mode()&0770)
197+
if err != nil {
198+
return fmt.Errorf("creating directory %q: %w", dstPath, err)
199+
}
166200
} else {
167201
log.Debugw("Unpacking file", "archive", "zip", "file.path", dstPath)
168202
// create non-existing containing folders with 0770 permissions right now, we'll fix the permission of each
169-
// directory as we come across them while processing the other package entries
170-
_ = os.MkdirAll(filepath.Dir(dstPath), 0770)
171-
f, err := os.OpenFile(dstPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode()&0770)
203+
// directory as we come across them while processing the other
204+
// package entries
205+
// Using mkdirAll instead of os.MkdirAll so that we can mock it in tests.
206+
err = mkdirAll(filepath.Dir(dstPath), 0770)
207+
if err != nil {
208+
return fmt.Errorf("creating directory %q: %w", dstPath, err)
209+
}
210+
// Using openFile instead of os.OpenFile so that we can mock it in tests.
211+
f, err := openFile(dstPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode()&0770)
172212
if err != nil {
173213
return err
174214
}
@@ -178,7 +218,9 @@ func unzip(log *logger.Logger, archivePath, dataDir string, flavor string) (Unpa
178218
}
179219
}()
180220

181-
if _, err = io.Copy(f, rc); err != nil { //nolint:gosec // legacy
221+
// Using copy instead of io.Copy so that we can
222+
// mock it in tests.
223+
if _, err = copy(f, rc); err != nil {
182224
return err
183225
}
184226
}
@@ -313,7 +355,8 @@ func getPackageMetadataFromZipReader(r *zip.ReadCloser, fileNamePrefix string) (
313355
return ret, nil
314356
}
315357

316-
func untar(log *logger.Logger, archivePath, dataDir string, flavor string) (UnpackResult, error) {
358+
// injecting copy, mkdirAll and openFile for testability
359+
func untar(log *logger.Logger, archivePath, dataDir string, flavor string, copy copyFunc, mkdirAll mkdirAllFunc, openFile openFileFunc) (UnpackResult, error) {
317360
var versionedHome string
318361
var rootDir string
319362
var hash string
@@ -413,17 +456,23 @@ func untar(log *logger.Logger, archivePath, dataDir string, flavor string) (Unpa
413456
log.Debugw("Unpacking file", "archive", "tar", "file.path", abs)
414457
// create non-existing containing folders with 0750 permissions right now, we'll fix the permission of each
415458
// directory as we come across them while processing the other package entries
416-
if err = os.MkdirAll(filepath.Dir(abs), 0750); err != nil {
417-
return UnpackResult{}, errors.New(err, "TarInstaller: creating directory for file "+abs, errors.TypeFilesystem, errors.M(errors.MetaKeyPath, abs))
459+
// Using mkdirAll instead of os.MkdirAll so that we can
460+
// mock it in tests.
461+
if err = mkdirAll(filepath.Dir(abs), 0750); err != nil {
462+
return UnpackResult{}, goerrors.Join(err, errors.New("TarInstaller: creating directory for file "+abs, errors.TypeFilesystem, errors.M(errors.MetaKeyPath, abs)))
418463
}
419464

420465
// remove any world permissions from the file
421-
wf, err := os.OpenFile(abs, os.O_RDWR|os.O_CREATE|os.O_TRUNC, mode.Perm()&0770)
466+
// Using openFile instead of os.OpenFile so that we can
467+
// mock it in tests.
468+
wf, err := openFile(abs, os.O_RDWR|os.O_CREATE|os.O_TRUNC, mode.Perm()&0770)
422469
if err != nil {
423-
return UnpackResult{}, errors.New(err, "TarInstaller: creating file "+abs, errors.TypeFilesystem, errors.M(errors.MetaKeyPath, abs))
470+
return UnpackResult{}, goerrors.Join(err, errors.New("TarInstaller: creating file "+abs, errors.TypeFilesystem, errors.M(errors.MetaKeyPath, abs)))
424471
}
425472

426-
_, err = io.Copy(wf, tr) //nolint:gosec // legacy
473+
// Using copy instead of io.Copy so that we can
474+
// mock it in tests.
475+
_, err = copy(wf, tr)
427476
if closeErr := wf.Close(); closeErr != nil && err == nil {
428477
err = closeErr
429478
}
@@ -435,17 +484,20 @@ func untar(log *logger.Logger, archivePath, dataDir string, flavor string) (Unpa
435484
// check if the directory already exists
436485
_, err = os.Stat(abs)
437486
if errors.Is(err, fs.ErrNotExist) {
438-
// the directory does not exist, create it and any non-existing parent directory with the same permissions
439-
if err := os.MkdirAll(abs, mode.Perm()&0770); err != nil {
440-
return UnpackResult{}, errors.New(err, "TarInstaller: creating directory for file "+abs, errors.TypeFilesystem, errors.M(errors.MetaKeyPath, abs))
487+
// the directory does not exist, create it and any non-existing
488+
// parent directory with the same permissions.
489+
// Using mkdirAll instead of os.MkdirAll so that we can
490+
// mock it in tests.
491+
if err := mkdirAll(abs, mode.Perm()&0770); err != nil {
492+
return UnpackResult{}, goerrors.Join(err, errors.New("TarInstaller: creating directory for file "+abs, errors.TypeFilesystem, errors.M(errors.MetaKeyPath, abs)))
441493
}
442494
} else if err != nil {
443495
return UnpackResult{}, errors.New(err, "TarInstaller: stat() directory for file "+abs, errors.TypeFilesystem, errors.M(errors.MetaKeyPath, abs))
444496
} else {
445497
// directory already exists, set the appropriate permissions
446498
err = os.Chmod(abs, mode.Perm()&0770)
447499
if err != nil {
448-
return UnpackResult{}, errors.New(err, fmt.Sprintf("TarInstaller: setting permissions %O for directory %q", mode.Perm()&0770, abs), errors.TypeFilesystem, errors.M(errors.MetaKeyPath, abs))
500+
return UnpackResult{}, goerrors.Join(err, errors.New("TarInstaller: setting permissions %O for directory %q", mode.Perm()&0770, abs, errors.TypeFilesystem, errors.M(errors.MetaKeyPath, abs)))
449501
}
450502
}
451503
default:

0 commit comments

Comments
 (0)