fix(server): builtin-mode fixes for Stronghold (task storage, docker config dir) - #1
Merged
Merged
Conversation
In builtin mode the Vault router resets req.Storage to nil once the request handler returns (the deferred reset in routeCommon, vault/router.go), and the release and publish tasks run later, on the tasks manager worker. Reading req.Storage from the task then dereferences nil: the first storage read in the task, GetTrustedPGPPublicKeys, panics and takes the whole Vault process down with it. The tasks manager already hands the task its own storage, the mount's storage view kept from the first request; use it for every storage read inside the task. An external plugin never saw this, because there req.Storage is a gRPC storage client living in the plugin process, which the core's reset does not reach. The unit tests mocked RunTask and never executed the task at all, so the new suite runs both tasks after setting req.Storage to nil, the way the router does. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Vasily Marmer <vasily.marmer@flant.com> (cherry picked from commit 1fb5e0c)
…g dir The BuildKit auth provider persists registry token seeds under the docker config dir and creates that directory on the first token request, so a process without a writable home fails every pull with "mkdir /.docker: read-only file system", anonymous pulls included. A builtin backend runs exactly like that: Stronghold has readOnlyRootFilesystem, no HOME and only /tmp writable. Read the config file from the default location as before, then point config.Dir() at a directory under os.TempDir() when the default one cannot be created, so only the seeds move and credentials in the default config.json keep working. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Vasily Marmer <vasily.marmer@flant.com> (cherry picked from commit 48215e2)
The comment next to the config dir redirect read as if a registry token were written to disk. The file holds a locally generated random seed for the client-side token authority key; it is neither a token nor a credential, and it is never sent anywhere. Say so where the redirect is. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Vasily Marmer <vasily.marmer@flant.com> (cherry picked from commit 212a4b2)
…r config dir out of $HOME Review found that the release scenario proved only that req.Storage is not dereferenced: a task reading a fresh empty storage would pass too. The storage handed to the task now counts List and Get calls and the test requires at least one. The release scenario also reached config.LoadDefaultConfigFile, creating the developer's ~/.docker as a side effect; the docker config dir is pinned to a temp dir for the test. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Vasily Marmer <vasily.marmer@flant.com> (cherry picked from commit 296f7cd)
…truction Review of the first cut found two defects. config.SetDir was never restored, so the second build in the same process read config.json from the seed directory and lost the credentials kept at the default location; and the fallback was a guessable path in the shared temp dir that nothing created or validated. The redirect now holds only while NewDockerAuthProvider captures config.Dir(), under a mutex so concurrent builds cannot observe each other's redirect, and the seed directory is created once per process with os.MkdirTemp and a private mode. The tests drive the provider through GetTokenAuthority, the call buildkitd makes on a bearer challenge, instead of asserting on the global variable. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Vasily Marmer <vasily.marmer@flant.com> (cherry picked from commit e93a9e4)
The seed only ever needs to outlive one process, so prefer the memory-backed /dev/shm for its directory and fall back to os.TempDir() where there is none, so nothing lands on a node disk. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Vasily Marmer <vasily.marmer@flant.com> (cherry picked from commit a6576be)
Several trdl mounts, in one namespace or across namespaces, share one Stronghold process, and a per-process seed directory is shared between them. Create the directory per build instead and remove it once the build is over, so mounts share nothing through it and nothing outlives the build. The cost is a fresh seed per build: on a shared buildkitd the daemon cannot link a new build to the tokens a previous one fetched; with a builder pod per build there is nothing to link. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Vasily Marmer <vasily.marmer@flant.com> (cherry picked from commit 4a3a4fc)
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Two fixes for trdl compiled into Stronghold as a builtin secrets engine. Both are latent upstream (an external plugin never hits them) and both are opened against
werf/trdlas well: werf#426 and werf#427. This branch istrublast/main(b7e692d) plus those two commits, so Stronghold'sreplacecan point here instead of at a third fork.Key changes
fix(server): read task storage from the tasks manager, not the request(fix(server): read task storage from the tasks manager, not the request werf/trdl#426) — the release and publish tasks readreq.Storageafter the request handler returned; the Vault router resets it tonilin the deferred block ofrouteCommon, so the first storage read in the task panics and takes the Vault process down. The task now uses thestoragethe tasks manager hands it (four sites). New suiteserver/task_storage_test.goruns both tasks withreq.Storage = nil.fix(server): keep BuildKit token seeds off an unwritable docker config dir(fix(server): keep BuildKit token seeds off an unwritable docker config dir werf/trdl#427) — the BuildKit auth provider doesMkdirAll(config.Dir())for its token seeds, which is/.dockerin a pod withreadOnlyRootFilesystemand noHOME; every pull failed withmkdir /.docker: read-only file system. The seeds move toos.TempDir()/trdl-docker-configwhen the default dir cannot be created;config.jsonis still read from the default location.Why
Observed on a Stronghold 1.19 stand (
release-1.19, three replicas):trdl/<engine>/releasecrashed the active replica withpgp.GetTrustedPGPPublicKeys({...}, {0x0, 0x0})atpath_release.go:133; with the first fix the task reached the build and failed onmkdir /.docker; with both,git-signatures v0.0.1was released end to end through the kubernetes buildkitd driver (taskSUCCEEDEDin 3m00s, zero replica restarts, eight0.0.1targets in the TUF repository, sha512 of the linux-amd64 binary matching). Stronghold currently carriesreplace github.com/werf/trdl/server => github.com/vmrm/trdl/server v0.0.0-20260904213209-b50a4645f684, i.e. this exact branch head.Review focus / risks
config.SetDiris process-global state indocker/cli; the redirect happens only when the default dir is unwritable and after the config file has been read.🤖 Generated with Claude Code