Skip to content

fetchers: tarball cache used past ttl #16390

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tomberek wants to merge 1 commit into
base: master
Choose a base branch
from
+51 −6
Open

fetchers: tarball cache used past ttl #16390

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 24 additions & 6 deletions src/libfetchers/tarball.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,11 @@ DownloadFileResult downloadFile(
}

static DownloadTarballResult downloadTarball_(
const Settings & settings, const std::string & urlS, const Headers & headers, const std::string & displayPrefix)
const Settings & settings,
const std::string & urlS,
const Headers & headers,
const std::string & displayPrefix,
const std::optional<Hash> & expectedNarHash)
{
ParsedURL url = parseURL(urlS);

Expand Down Expand Up @@ -150,10 +154,23 @@ static DownloadTarballResult downloadTarball_(
if (cached && !settings.getTarballCache()->hasObject(getRevAttr(cached->value, "treeHash")))
cached.reset();

if (cached && !cached->expired)
/* We previously downloaded this tarball and it's younger than
`tarballTtl`, so no need to check the server. */
return attrsToResult(cached->value);
if (cached) {
if (!cached->expired)
/* We previously downloaded this tarball and it's younger than
`tarballTtl`, so no need to check the server. */
return attrsToResult(cached->value);

/* The cached entry is stale, but if its content still matches
the pinned NAR hash, there's no need to re-check the server
either. Exception: a TTL of 0 (e.g. `--refresh`) means the
caller explicitly wants us to verify against the server, so
don't let a merely-matching old hash short-circuit that. */
if (expectedNarHash && settings.tarballTtl.get() != 0) {
auto treeHash = getRevAttr(cached->value, "treeHash");
if (settings.getTarballCache()->treeHashToNarHash(settings, treeHash) == *expectedNarHash)
return attrsToResult(cached->value);
}
}

auto _res = std::make_shared<Sync<FileTransferResult>>();

Expand Down Expand Up @@ -491,7 +508,8 @@ struct TarballInputScheme : CurlInputScheme
{
auto input(_input);

auto result = downloadTarball_(settings, getStrAttr(input.attrs, "url"), {}, "«" + input.to_string() + "»");
auto result = downloadTarball_(
settings, getStrAttr(input.attrs, "url"), {}, "«" + input.to_string() + "»", input.getNarHash());

if (result.immutableUrl) {
auto immutableInput = Input::fromURL(*result.immutableUrl);
Expand Down
27 changes: 27 additions & 0 deletions tests/functional/tarball.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,33 @@ test_tarball() {
nix-build -o "$TEST_ROOT"/result -E "import (fetchTree { type = \"tarball\"; url = \"file://$tarball\"; })"
nix-build -o "$TEST_ROOT"/result -E "import (fetchTree { type = \"tarball\"; url = \"file://$tarball\"; narHash = \"$hash\"; })"

# Once a URL has been fetched with a given narHash, re-fetching the same
# URL after its cache entry has genuinely expired (a nonzero TTL that has
# elapsed) should not require re-reading it. Prove this by corrupting the
# file in place and expecting the fetch to still succeed. (`--tarball-ttl
# 0` is excluded here since that's the explicit "always re-verify"
# signal used by `--refresh`, which must NOT be short-circuited.)
cp "$tarball" "$tarball.bak"
echo garbage > "$tarball"
sleep 2
nix-build -o "$TEST_ROOT"/result --tarball-ttl 1 -E "import (fetchTree { type = \"tarball\"; url = \"file://$tarball\"; narHash = \"$hash\"; })"
mv "$tarball.bak" "$tarball"

# `--refresh` (tarball-ttl 0) must always re-verify against the source,
# even if a narHash is pinned, so a mismatch must still be reported.
# Use a different URL with different (but validly archived) content, so
# the failure is a hash mismatch rather than an unpack error, and so the
# cache entry for `$tarball` itself isn't disturbed by the expected
# failure.
otherRoot=$TEST_ROOT/tarball-other
rm -rf "$otherRoot"
mkdir -p "$otherRoot"
echo "different content" > "$otherRoot/default.nix"
cp "${config_nix}" dependencies.builder*.sh "$otherRoot/"
otherTarball=$TEST_ROOT/tarball-other.tar$ext
(cd "$TEST_ROOT" && GNUTAR_REPRODUCIBLE=1 tar --mtime="$otherRoot"/default.nix --owner=0 --group=0 --numeric-owner --sort=name -c -f - tarball-other) | $compressor > "$otherTarball"
expectStderr 102 nix eval --refresh --raw --expr "(fetchTree { type = \"tarball\"; url = \"file://$otherTarball\"; narHash = \"$hash\"; }).outPath" | grepQuiet "NAR hash mismatch"

[[ $(nix eval --impure --expr "(fetchTree \"file://$tarball\").lastModified") = 1000000000 ]]

nix-instantiate --strict --eval -E "!((import (fetchTree { type = \"tarball\"; url = \"file://$tarball\"; narHash = \"$hash\"; })) ? submodules)" >&2
Expand Down
Loading