Skip to content

Commit 9f12bd7

Browse files
committed
fix(libstore): misc fixes for curl-based s3
1. Added S3-specific settings (region, endpoint, profile, scheme) to HttpBinaryCacheStoreConfig to prevent 'unknown setting' warnings 2. Fixed URL construction in makeRequest() to properly handle: - S3 store URLs with query parameters (like ?region=us-east-2) - Paths that contain their own query parameters (like nar/file.nar?hash=abc) - Proper placement of path before query parameters in URLs 3. Fixed parseS3Url() to allow empty keys for store-level operations The key is filled in by specific operations (e.g., 'nix-cache-info')
1 parent feb5a8f commit 9f12bd7

File tree

3 files changed

+84
-11
lines changed

3 files changed

+84
-11
lines changed

src/libstore/filetransfer.cc

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -918,8 +918,23 @@ struct curlFileTransfer : public FileTransfer
918918

919919
if (!endpoint.empty()) {
920920
// Custom endpoint (e.g., MinIO, custom S3-compatible service)
921-
httpsUrl.authority = ParsedURL::Authority{.host = endpoint};
922-
httpsUrl.path = "/" + bucket + "/" + key;
921+
// Parse the endpoint if it's a full URL
922+
if (hasPrefix(endpoint, "http://") || hasPrefix(endpoint, "https://")) {
923+
try {
924+
auto endpointUrl = parseURL(endpoint);
925+
httpsUrl.scheme = endpointUrl.scheme;
926+
httpsUrl.authority = endpointUrl.authority;
927+
httpsUrl.path = "/" + bucket + "/" + key;
928+
} catch (BadURL & e) {
929+
// If parsing fails, treat it as a hostname
930+
httpsUrl.authority = ParsedURL::Authority{.host = endpoint};
931+
httpsUrl.path = "/" + bucket + "/" + key;
932+
}
933+
} else {
934+
// Endpoint is just a hostname
935+
httpsUrl.authority = ParsedURL::Authority{.host = endpoint};
936+
httpsUrl.path = "/" + bucket + "/" + key;
937+
}
923938
} else {
924939
// Standard AWS S3 endpoint
925940
httpsUrl.authority = ParsedURL::Authority{.host = "s3." + region + ".amazonaws.com"};
@@ -952,8 +967,8 @@ struct curlFileTransfer : public FileTransfer
952967
key = key.substr(1);
953968
}
954969

955-
if (key.empty())
956-
throw nix::Error("S3 URI missing object key");
970+
// Allow empty keys for store-level operations
971+
// The key will be filled in by the specific operation (e.g., "nix-cache-info")
957972

958973
return S3Url{.bucket = bucket, .key = key, .params = parsed.query};
959974
} catch (BadURL & e) {

src/libstore/http-binary-cache-store.cc

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,11 @@ class HttpBinaryCacheStore : public virtual BinaryCacheStore
8787
void init() override
8888
{
8989
// FIXME: do this lazily?
90-
if (auto cacheInfo = diskCache->upToDateCacheExists(config->cacheUri.to_string())) {
90+
// For consistent cache key handling, use the reference without parameters
91+
// This matches what's used in Store::queryPathInfo() lookups
92+
auto cacheKey = config->getReference().render(/*withParams=*/false);
93+
94+
if (auto cacheInfo = diskCache->upToDateCacheExists(cacheKey)) {
9195
config->wantMassQuery.setDefault(cacheInfo->wantMassQuery);
9296
config->priority.setDefault(cacheInfo->priority);
9397
} else {
@@ -96,8 +100,7 @@ class HttpBinaryCacheStore : public virtual BinaryCacheStore
96100
} catch (UploadToHTTP &) {
97101
throw Error("'%s' does not appear to be a binary cache", config->cacheUri.to_string());
98102
}
99-
diskCache->createCache(
100-
config->cacheUri.to_string(), config->storeDir, config->wantMassQuery, config->priority);
103+
diskCache->createCache(cacheKey, config->storeDir, config->wantMassQuery, config->priority);
101104
}
102105
}
103106

@@ -179,10 +182,24 @@ class HttpBinaryCacheStore : public virtual BinaryCacheStore
179182
if (hasPrefix(path, "https://") || hasPrefix(path, "http://") || hasPrefix(path, "file://")) {
180183
return FileTransferRequest(path);
181184
} else {
182-
// Properly construct URL preserving query parameters
183-
auto baseUrl = config->cacheUri;
184-
baseUrl.path = baseUrl.path + "/" + path;
185-
return FileTransferRequest(baseUrl.to_string());
185+
// For S3 URLs, we need to properly construct the URL with path before query parameters
186+
if (config->cacheUri.scheme == "s3") {
187+
auto s3Url = config->cacheUri;
188+
189+
// Check if path contains its own query parameters
190+
auto questionPos = path.find('?');
191+
if (questionPos != std::string::npos) {
192+
// Path has query params - don't include base query params
193+
s3Url.query.clear();
194+
}
195+
196+
// Append the path and reconstruct with query parameters in the right place
197+
s3Url.path = s3Url.path + "/" + path;
198+
return FileTransferRequest(s3Url.to_string());
199+
} else {
200+
// For non-S3 URLs, use the original simpler approach
201+
return FileTransferRequest(config->cacheUri.to_string() + "/" + path);
202+
}
186203
}
187204
}
188205

src/libstore/include/nix/store/http-binary-cache-store.hh

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,47 @@ struct HttpBinaryCacheStoreConfig : std::enable_shared_from_this<HttpBinaryCache
1414

1515
ParsedURL cacheUri;
1616

17+
#if NIX_WITH_AWS_CRT_SUPPORT
18+
// S3-specific settings that are passed as query parameters
19+
// These are used by the curl-based S3 implementation
20+
const Setting<std::string> region{
21+
this,
22+
"us-east-1",
23+
"region",
24+
R"(
25+
The region of the S3 bucket. If your bucket is not in
26+
`us-east-1`, you should always explicitly specify the region
27+
parameter.
28+
)"};
29+
30+
const Setting<std::string> endpoint{
31+
this,
32+
"",
33+
"endpoint",
34+
R"(
35+
The S3 endpoint to use. By default, Nix uses the standard
36+
AWS S3 endpoint.
37+
)"};
38+
39+
const Setting<std::string> profile{
40+
this,
41+
"",
42+
"profile",
43+
R"(
44+
The name of the AWS configuration profile to use. By default
45+
Nix uses the `default` profile.
46+
)"};
47+
48+
const Setting<std::string> scheme{
49+
this,
50+
"",
51+
"scheme",
52+
R"(
53+
The scheme to use for S3 requests (http or https). By default,
54+
https is used.
55+
)"};
56+
#endif
57+
1758
static const std::string name()
1859
{
1960
return "HTTP Binary Cache Store";

0 commit comments

Comments
 (0)