Skip to content

Commit 102852e

Browse files
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. - 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) - S3BinaryCacheStore now delegates to HttpBinaryCacheStore - Function s3ToHttpsUrl converts ParsedS3URL to ParsedURL - Multipart uploads are no longer supported (may be reimplemented later) - Build now requires curl >= 7.75.0 for AWS SigV4 support Fixes: #13084, #12671, #11748, #12403, #5947
1 parent 339d00e commit 102852e

19 files changed

+1536
-801
lines changed
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, s3RequestWithMockCredentials)
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, s3RequestWithSessionToken)
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 & 1 deletion
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,7 +74,8 @@ sources = files(
7374
'path-info.cc',
7475
'path.cc',
7576
'references.cc',
76-
's3-binary-cache-store.cc',
77+
's3-edge-cases.cc',
78+
's3-http-integration.cc',
7779
's3.cc',
7880
'serve-protocol.cc',
7981
'ssh-store.cc',

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

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

0 commit comments

Comments
 (0)