Skip to content

Commit bff3b41

Browse files
lovesegfaultEricson2314
lovesegfault
and
Ericson2314
committed
refactor(libstore): Replace AWS SDK with curl-based S3 implementation
This commit replaces the AWS C++ SDK with a lighter curl-based approach for S3 binary cache operations, addressing multiple issues and improving the overall architecture. ## Key Changes ### 1. Replaced AWS SDK with aws-crt-cpp + curl - Removed dependency on the heavy aws-cpp-sdk-s3 and aws-cpp-sdk-transfer - Added lightweight aws-crt-cpp for credential resolution only - Leverages curl's native AWS SigV4 authentication (requires curl >= 7.75.0) ### 2. Consolidated S3 support into HttpBinaryCacheStore - S3BinaryCacheStore now delegates to HttpBinaryCacheStore - Eliminates layer violation (S3 logic no longer in generic FileTransfer) - S3 URLs are converted to HTTPS URLs with proper authentication headers ### 3. Improved S3 URL handling - Added ParsedS3URL for structured S3 URL parsing - Function s3ToHttpsUrl converts ParsedS3URL to ParsedURL - Better support for custom endpoints and regions via query parameters ### 4. Fixed thread safety test - threadSafety_MultipleProviders now actually tests concurrent access - Spawns multiple threads accessing shared credential providers - Tests both concurrent credential fetching and provider creation ## Benefits - **Reduced memory usage**: Eliminates memory buffering issues that caused segfaults with large files (>3.5GB) - **Fixed upload reliability**: Resolves AWS SDK chunking errors (InvalidChunkSizeError) - **Resolved OpenSSL conflicts**: No more S2N engine override issues in sandboxed builds - **Lighter dependencies**: ~500 lines less code, simpler build process - **Better architecture**: Clean separation between storage and transport layers ## Breaking Changes - Multipart uploads are no longer supported (may be reimplemented later) - Build now requires curl >= 7.75.0 for AWS SigV4 support ## Migration No action required for most users. S3 URLs continue to work with the same syntax. Users directly using S3BinaryCacheStore class should migrate to standard HTTP binary cache stores with S3 endpoints. Fixes: #13084, #12671, #11748, #12403, #5947 PR: #13752 Co-authored-by: John Ericson <git@johnericson.me>
1 parent e2b9847 commit bff3b41

21 files changed

+1438
-797
lines changed

‎doc/manual/rl-next/s3-curl-implementation.md

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
---
2+
synopsis: "Improved S3 binary cache support via HTTP"
3+
prs: [13752]
4+
issues: [13084, 12671, 11748, 12403, 5947]
5+
---
6+
7+
S3 binary cache operations now happen via HTTP, leveraging `libcurl`'s native AWS
8+
SigV4 authentication instead of the AWS C++ SDK, providing significant
9+
improvements:
10+
11+
- **Reduced memory usage**: Eliminates memory buffering issues that caused
12+
segfaults with large files (>3.5GB)
13+
- **Fixed upload reliability**: Resolves AWS SDK chunking errors
14+
(`InvalidChunkSizeError`)
15+
- **Resolved OpenSSL conflicts**: No more S2N engine override issues in
16+
sandboxed builds
17+
- **Lighter dependencies**: Uses lightweight `aws-crt-cpp` instead of full
18+
`aws-cpp-sdk`, reducing build complexity
19+
20+
The new implementation requires curl >= 7.75.0 and `aws-crt-cpp` for credential
21+
management.
22+
23+
All existing S3 URL formats and parameters remain supported.
24+
25+
## Breaking changes
26+
27+
The legacy `S3BinaryCacheStore` implementation has been removed in favor of the
28+
new curl-based approach, as a result multipart uploads are no longer supported. They may be reimplemented in the future.
29+
30+
**Migration**: No action required for most users. S3 URLs continue to work
31+
with the same syntax. Users directly using `S3BinaryCacheStore` class
32+
should migrate to standard HTTP binary cache stores with S3 endpoints.
33+
34+
**Build requirement**: S3 support now requires curl >= 7.75.0 for AWS SigV4
35+
authentication. Build configuration will warn if `aws-crt-cpp` is available
36+
but S3 support is disabled due to an insufficient curl version.

‎packaging/components.nix

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -440,7 +440,7 @@ in
440440
441441
Example:
442442
```
443-
overrideScope (finalScope: prevScope: { aws-sdk-cpp = null; })
443+
overrideScope (finalScope: prevScope: { aws-crt-cpp = null; })
444444
```
445445
*/
446446
overrideScope = f: (scope.overrideScope f).nix-everything;

‎packaging/dependencies.nix

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -35,21 +35,6 @@ in
3535
scope: {
3636
inherit stdenv;
3737

38-
aws-sdk-cpp =
39-
(pkgs.aws-sdk-cpp.override {
40-
apis = [
41-
"identity-management"
42-
"s3"
43-
"transfer"
44-
];
45-
customMemoryManagement = false;
46-
}).overrideAttrs
47-
{
48-
# only a stripped down version is built, which takes a lot less resources
49-
# to build, so we don't need a "big-parallel" machine.
50-
requiredSystemFeatures = [ ];
51-
};
52-
5338
boehmgc =
5439
(pkgs.boehmgc.override {
5540
enableLargeConfig = true;

‎src/libstore-tests/filetransfer-s3.cc

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
#include "nix/store/filetransfer.hh"
2+
#include "nix/store/config.hh"
3+
#include "nix/store/http-binary-cache-store.hh"
4+
#include "nix/store/s3-binary-cache-store.hh"
5+
#include "nix/store/store-api.hh"
6+
#include "nix/util/types.hh"
7+
8+
#if NIX_WITH_S3_SUPPORT
9+
10+
# include <gtest/gtest.h>
11+
# include <gmock/gmock.h>
12+
13+
namespace nix {
14+
15+
class S3FileTransferTest : public ::testing::Test
16+
{
17+
protected:
18+
void SetUp() override
19+
{
20+
// Clean environment for predictable tests
21+
unsetenv("AWS_ACCESS_KEY_ID");
22+
unsetenv("AWS_SECRET_ACCESS_KEY");
23+
unsetenv("AWS_SESSION_TOKEN");
24+
unsetenv("AWS_PROFILE");
25+
}
26+
};
27+
28+
// Note: Basic S3 URL parsing tests are covered by ParsedS3URL tests in s3.cc
29+
// These tests focus on FileTransfer-specific S3 functionality
30+
31+
TEST_F(S3FileTransferTest, s3Request_WithMockCredentials)
32+
{
33+
// Set up mock credentials for testing
34+
setenv("AWS_ACCESS_KEY_ID", "AKIAIOSFODNN7EXAMPLE", 1);
35+
setenv("AWS_SECRET_ACCESS_KEY", "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", 1);
36+
37+
std::string s3Uri = "s3://test-bucket/test-key.txt?region=us-east-1";
38+
FileTransferRequest request(s3Uri);
39+
40+
// Test that request setup works with credentials
41+
EXPECT_TRUE(hasPrefix(request.uri, "s3://"));
42+
43+
// Clean up
44+
unsetenv("AWS_ACCESS_KEY_ID");
45+
unsetenv("AWS_SECRET_ACCESS_KEY");
46+
}
47+
48+
TEST_F(S3FileTransferTest, s3Request_WithSessionToken)
49+
{
50+
// Test session token handling
51+
setenv("AWS_ACCESS_KEY_ID", "ASIAIOSFODNN7EXAMPLE", 1);
52+
setenv("AWS_SECRET_ACCESS_KEY", "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", 1);
53+
setenv("AWS_SESSION_TOKEN", "AQoDYXdzEJr1K...example-session-token", 1);
54+
55+
std::string s3Uri = "s3://test-bucket/test-key.txt";
56+
FileTransferRequest request(s3Uri);
57+
58+
EXPECT_TRUE(hasPrefix(request.uri, "s3://"));
59+
60+
// Clean up
61+
unsetenv("AWS_ACCESS_KEY_ID");
62+
unsetenv("AWS_SECRET_ACCESS_KEY");
63+
unsetenv("AWS_SESSION_TOKEN");
64+
}
65+
66+
/**
67+
* Regression test for commit 7a2f2891e
68+
* Test that S3 store URLs are properly recognized and handled
69+
*/
70+
TEST_F(S3FileTransferTest, s3StoreRegistration)
71+
{
72+
// Test that S3 URI scheme is in the supported schemes (now in S3BinaryCacheStoreConfig)
73+
auto s3Schemes = S3BinaryCacheStoreConfig::uriSchemes();
74+
EXPECT_TRUE(s3Schemes.count("s3") > 0) << "S3 scheme should be in S3BinaryCacheStoreConfig URI schemes";
75+
76+
// Verify that HttpBinaryCacheStoreConfig does NOT include S3
77+
auto httpSchemes = HttpBinaryCacheStoreConfig::uriSchemes();
78+
EXPECT_FALSE(httpSchemes.count("s3") > 0) << "S3 scheme should NOT be in HttpBinaryCacheStoreConfig URI schemes";
79+
80+
// Test that S3 store can be opened without error
81+
try {
82+
auto storeUrl = "s3://test-bucket";
83+
auto parsedUrl = parseURL(storeUrl);
84+
EXPECT_EQ(parsedUrl.scheme, "s3");
85+
86+
// Verify that S3BinaryCacheStoreConfig accepts S3 URLs
87+
S3BinaryCacheStoreConfig config("s3", "test-bucket", {});
88+
EXPECT_EQ(config.cacheUri.scheme, "s3");
89+
EXPECT_EQ(config.cacheUri.authority->host, "test-bucket");
90+
} catch (const std::exception & e) {
91+
FAIL() << "Should be able to create S3 store config: " << e.what();
92+
}
93+
}
94+
95+
/**
96+
* Regression test for commit c0164e087
97+
* Test that S3 uploads are not rejected with "not supported" error
98+
*/
99+
TEST_F(S3FileTransferTest, s3UploadsNotRejected)
100+
{
101+
auto ft = makeFileTransfer();
102+
103+
// Create a mock upload request
104+
FileTransferRequest uploadReq("s3://test-bucket/test-file");
105+
uploadReq.data = std::string("test data");
106+
107+
// This should not throw "uploading to 's3://...' is not supported"
108+
// We're testing that S3 uploads aren't immediately rejected
109+
bool gotNotSupportedError = false;
110+
try {
111+
ft->upload(uploadReq);
112+
} catch (const Error & e) {
113+
std::string msg = e.what();
114+
if (msg.find("is not supported") != std::string::npos) {
115+
gotNotSupportedError = true;
116+
}
117+
} catch (...) {
118+
// Other errors are expected (no credentials, network issues, etc.)
119+
}
120+
121+
EXPECT_FALSE(gotNotSupportedError) << "S3 uploads should not be rejected with 'not supported' error";
122+
}
123+
124+
/**
125+
* Regression test for commit e618ac7e0
126+
* Test that S3 URLs with region query parameters are handled correctly
127+
*/
128+
TEST_F(S3FileTransferTest, s3RegionQueryParameters)
129+
{
130+
// Test that query parameters are preserved in S3 URLs
131+
StringMap params;
132+
params["region"] = "us-west-2";
133+
134+
S3BinaryCacheStoreConfig config("s3", "test-bucket", params);
135+
136+
// For S3 stores, query parameters should be preserved
137+
EXPECT_FALSE(config.cacheUri.query.empty()) << "S3 store should preserve query parameters";
138+
EXPECT_EQ(config.cacheUri.query["region"], "us-west-2") << "Region parameter should be preserved";
139+
140+
// Test with different regions
141+
StringMap params2;
142+
params2["region"] = "eu-central-1";
143+
144+
S3BinaryCacheStoreConfig config2("s3", "another-bucket", params2);
145+
EXPECT_EQ(config2.cacheUri.query["region"], "eu-central-1") << "Different region parameter should be preserved";
146+
}
147+
148+
} // namespace nix
149+
150+
#endif // NIX_WITH_S3_SUPPORT

‎src/libstore-tests/meson.build

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ sources = files(
6060
'derivation.cc',
6161
'derived-path.cc',
6262
'downstream-placeholder.cc',
63+
'filetransfer-s3.cc',
6364
'http-binary-cache-store.cc',
6465
'legacy-ssh-store.cc',
6566
'local-binary-cache-store.cc',
@@ -73,8 +74,8 @@ sources = files(
7374
'path-info.cc',
7475
'path.cc',
7576
'references.cc',
76-
's3-binary-cache-store.cc',
77-
's3.cc',
77+
's3-edge-cases.cc',
78+
's3-http-integration.cc',
7879
'serve-protocol.cc',
7980
'ssh-store.cc',
8081
'store-reference.cc',

‎src/libstore-tests/s3-binary-cache-store.cc

Lines changed: 0 additions & 18 deletions
This file was deleted.

0 commit comments

Comments
 (0)