chore: refactor terraform paths to a central structure #20566
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Oct 30, 2025
Member
Author
b7321d6 to
52064b7
Compare
52064b7 to
bbf6058
Compare
Emyrk
commented
Nov 10, 2025
Comment on lines
75
to
192
| func (l Layout) ExtractArchive(ctx context.Context, logger slog.Logger, fs afero.Fs, cfg *proto.Config) error { | ||
| logger.Info(ctx, "unpacking template source archive", | ||
| slog.F("size_bytes", len(cfg.TemplateSourceArchive)), | ||
| ) | ||
|
|
||
| err := fs.MkdirAll(l.WorkDirectory(), 0o700) | ||
| if err != nil { | ||
| return xerrors.Errorf("create work directory %q: %w", l.WorkDirectory(), err) | ||
| } | ||
|
|
||
| reader := tar.NewReader(bytes.NewBuffer(cfg.TemplateSourceArchive)) | ||
| // for safety, nil out the reference on Config, since the reader now owns it. | ||
| cfg.TemplateSourceArchive = nil | ||
| for { | ||
| header, err := reader.Next() | ||
| if err != nil { | ||
| if xerrors.Is(err, io.EOF) { | ||
| break | ||
| } | ||
| return xerrors.Errorf("read template source archive: %w", err) | ||
| } | ||
| logger.Debug(context.Background(), "read archive entry", | ||
| slog.F("name", header.Name), | ||
| slog.F("mod_time", header.ModTime), | ||
| slog.F("size", header.Size)) | ||
|
|
||
| // Security: don't untar absolute or relative paths, as this can allow a malicious tar to overwrite | ||
| // files outside the workdir. | ||
| if !filepath.IsLocal(header.Name) { | ||
| return xerrors.Errorf("refusing to extract to non-local path") | ||
| } | ||
| // nolint: gosec | ||
| headerPath := filepath.Join(l.WorkDirectory(), header.Name) | ||
| if !strings.HasPrefix(headerPath, filepath.Clean(l.WorkDirectory())) { | ||
| return xerrors.New("tar attempts to target relative upper directory") | ||
| } | ||
| mode := header.FileInfo().Mode() | ||
| if mode == 0 { | ||
| mode = 0o600 | ||
| } | ||
|
|
||
| // Always check for context cancellation before reading the next header. | ||
| // This is mainly important for unit tests, since a canceled context means | ||
| // the underlying directory is going to be deleted. There still exists | ||
| // the small race condition that the context is canceled after this, and | ||
| // before the disk write. | ||
| if ctx.Err() != nil { | ||
| return xerrors.Errorf("context canceled: %w", ctx.Err()) | ||
| } | ||
| switch header.Typeflag { | ||
| case tar.TypeDir: | ||
| err = fs.MkdirAll(headerPath, mode) | ||
| if err != nil { | ||
| return xerrors.Errorf("mkdir %q: %w", headerPath, err) | ||
| } | ||
| logger.Debug(context.Background(), "extracted directory", | ||
| slog.F("path", headerPath), | ||
| slog.F("mode", fmt.Sprintf("%O", mode))) | ||
| case tar.TypeReg: | ||
| file, err := fs.OpenFile(headerPath, os.O_CREATE|os.O_RDWR, mode) | ||
| if err != nil { | ||
| return xerrors.Errorf("create file %q (mode %s): %w", headerPath, mode, err) | ||
| } | ||
|
|
||
| hash := crc32.NewIEEE() | ||
| hashReader := io.TeeReader(reader, hash) | ||
| // Max file size of 10MiB. | ||
| size, err := io.CopyN(file, hashReader, 10<<20) | ||
| if xerrors.Is(err, io.EOF) { | ||
| err = nil | ||
| } | ||
| if err != nil { | ||
| _ = file.Close() | ||
| return xerrors.Errorf("copy file %q: %w", headerPath, err) | ||
| } | ||
| err = file.Close() | ||
| if err != nil { | ||
| return xerrors.Errorf("close file %q: %s", headerPath, err) | ||
| } | ||
| logger.Debug(context.Background(), "extracted file", | ||
| slog.F("size_bytes", size), | ||
| slog.F("path", headerPath), | ||
| slog.F("mode", mode), | ||
| slog.F("checksum", fmt.Sprintf("%x", hash.Sum(nil)))) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // Cleanup removes the work directory and all of its contents. | ||
| func (l Layout) Cleanup(ctx context.Context, logger slog.Logger, fs afero.Fs) { | ||
| var err error | ||
| path := l.WorkDirectory() | ||
|
|
||
| for attempt := 0; attempt < 5; attempt++ { | ||
| err := fs.RemoveAll(path) | ||
| if err != nil { | ||
| // On Windows, open files cannot be removed. | ||
| // When the provisioner daemon is shutting down, | ||
| // it may take a few milliseconds for processes to exit. | ||
| // See: https://github.com/golang/go/issues/50510 | ||
| logger.Debug(ctx, "failed to clean work directory; trying again", slog.Error(err)) | ||
| // TODO: Should we abort earlier if the context is done? | ||
| time.Sleep(250 * time.Millisecond) | ||
| continue | ||
| } | ||
| logger.Debug(ctx, "cleaned up work directory") | ||
| return | ||
| } | ||
|
|
||
| // Returning an error at this point cannot do any good. The caller cannot resolve | ||
| // this. There is a routine cleanup task that will remove old work directories | ||
| // when this fails. | ||
| logger.Error(ctx, "failed to clean up work directory after multiple attempts", | ||
| slog.F("path", path), slog.Error(err)) | ||
| } |
Member
Author
There was a problem hiding this comment.
Extract and Cleanup are moved to here so the experiment can implement a different implementation for these.
I think this data-structure would benefit from also containing the fs.FS, but that was a larger refactor than I wanted to do.
The structure handles building paths for all relevant files.
bbf6058 to
fc6bb0a
Compare
dannykopping
approved these changes
Nov 11, 2025
Contributor
dannykopping
left a comment
There was a problem hiding this comment.
This is really nice!
| } | ||
|
|
||
| reader := tar.NewReader(bytes.NewBuffer(cfg.TemplateSourceArchive)) | ||
| // for safety, nil out the reference on Config, since the reader now owns it. |
Contributor
There was a problem hiding this comment.
Why pass a reference at all?
It doesn't seem like we need it, and it introduces some racy potential.
Member
Author
There was a problem hiding this comment.
This code was just moved, not changed. So I'm going to leave it as is for now.
provisionersdk/tfpath/tfpath.go
Outdated
| if !filepath.IsLocal(header.Name) { | ||
| return xerrors.Errorf("refusing to extract to non-local path") | ||
| } | ||
| // nolint: gosec |
Contributor
There was a problem hiding this comment.
Needs a comment please
Member
Author
There was a problem hiding this comment.
More moved code, but why do we even need this comment?
Going to remove it
Member
Author
There was a problem hiding this comment.
Error: provisionersdk/tfpath/tfpath.go:107:17: G305: File traversal when extracting zip/tar archive (gosec)
TIL
Member
Author
There was a problem hiding this comment.
I am going to leave the no lint and make an issue to follow up on this.
fc6bb0a to
b88ee78
Compare
b88ee78 to
e8bf6de
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactors all Terraform file path logic into a centralized tfpath package. This consolidates all path construction into a single, testable Layout type.
Instead of passing around
stringfor directories, pass around theLayoutwhich has the file location methods on it.This design is also to support an experiment where the filepaths will be changed if the experiment is enabled. Allowing for a singular struct to determine filepath locations based on some config. The code that uses said paths will remain unchanged.
Prior Art
Inspired from our
clihandling of config filescoder/cli/config/file.go
Line 18 in c61fca4
Naming
I was going back and forth on names. Some others I thought about:
tfpathtfdirtflayoutterradirThoughts
Should possibly add
afero.FSto this type. Then our tests can run without using the actual FS 🤷