driver: use stdlib tar for docker-container config files - #3996
driver: use stdlib tar for docker-container config files#3996crazy-max wants to merge 2 commits into
Conversation
Use a small tar writer for docker-container config files instead of the go-archive archiver. The helper only handles the generated config tree and reads file contents through os.Root. Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
| if len(files) > 0 { | ||
| dirs[configDir] = struct{}{} | ||
| } |
There was a problem hiding this comment.
configDir is the only name reaching writeConfigTar without passing configArchivePath. It's safe today only because len(files) > 0 makes it a prefix of every validated path. Would it be worth asserting it explicitly as well?
There was a problem hiding this comment.
Yeah, fair point. configDir is currently derived from path.Base(confutil.DefaultBuildKitConfigDir), so it's buildkit today, but it's still the one path component added to the archive before going through the same explicit validation path. I will tighten that so the archive root is validated too, or refactor it so we don't pass an arbitrary root string around.
|
|
||
| func writeConfigTar(tw *tar.Writer, dirs []string, entries []configTarEntry) error { | ||
| for _, dir := range dirs { | ||
| if err := tw.WriteHeader(&tar.Header{ |
There was a problem hiding this comment.
Should we force a format format here? I think Go should select the appropriate one, but something to potentially be aware of.
There was a problem hiding this comment.
I think leaving Format unset is the right behavior here. The stdlib writer will pick the first format that can encode the header, which keeps simple paths as simple tar headers and still allows longer generated registry paths if needed. Forcing USTAR would make some valid config paths fail, and forcing PAX would add extended headers even when they are not needed.
| require.Nil(t, rc) | ||
| } | ||
|
|
||
| func readTarEntries(t *testing.T, r io.Reader) map[string]tarEntry { |
There was a problem hiding this comment.
I guess here the entries are unordered, but does order matter? (e.g. ensure parent is written before child?)
There was a problem hiding this comment.
Order shouldn't matter for Docker extraction in practice, but the implementation does write all directory entries before file entries, and both sets are sorted for deterministic output. The current test reads into a map, so it doesn't prove that ordering. I will add an explicit assertion for the tar entry sequence so this behavior is covered.
Create the docker-container BuildKit config archive directly from the in-memory file map instead of staging files through a temporary directory. Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
24822b8 to
d3d2609
Compare
2 participants
closes #3982
This replaces the docker-container driver's config archive path with a focused stdlib tar implementation. The driver now builds the
/etc/buildkitconfig archive directly from the in-memory file map before copying it into the BuildKit container.The first commit removes the direct use of
github.com/moby/go-archivefor docker-container config copies and introduces a small tar writer based onarchive/tar. The second commit removes the temporary directory staging step, validates archive paths before writing tar headers, writes deterministic directory and file entries.The old path used a generic archive library and a host temporary directory for a very narrow job. We only need to copy generated BuildKit config files into a container, so staging those bytes on disk and then archiving them added complexity without buying us useful behavior. The new implementation makes the contract explicit: config keys are relative slash paths under
/etc/buildkit, unsafe names are rejected before tar headers are written, and the copy path no longer depends on host filesystem staging.