[DEMO] S3 Transfer Acceleration #13791
Draft
Conversation
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
After removing the legacy S3 implementation in c833b26, the store registration for s3:// URLs was accidentally lost. This caused commands like `nix store info --store s3://bucket` to fail with "don't know how to open Nix store with scheme 's3'". This commit restores S3 support by: 1. Adding "s3" to HttpBinaryCacheStore's supported URI schemes when built with AWS CRT support 2. Fixing the S3-to-HTTPS URL conversion in TransferItem::init() to use the converted URL The S3 functionality is now handled through the curl-based file transfer layer with AWS SigV4 authentication, as intended by issue NixOS#13084.
S3 uploads were failing with "uploading to 's3://...' is not supported" because the enqueueItem function only allowed uploads to HTTP(S) URLs. Since the HttpBinaryCacheStore now handles S3 URLs and generates S3 URLs for upload requests, we need to allow S3 URLs in enqueueItem when AWS CRT support is available. The S3-to-HTTPS conversion already happens in the TransferItem constructor, so this change enables full read/write support for S3 stores. Fixes uploads for commands like: nix copy --to s3://bucket /nix/store/path
S3 buckets in non-default regions require specifying the region to avoid 301 PermanentRedirect errors. This commit fixes two issues: 1. Query parameters in S3 store URLs (like ?region=us-east-2) were being treated as store settings instead of URL parameters, causing "unknown setting" warnings. 2. URL construction in makeRequest() was incorrectly concatenating paths with query parameters. The fix: - HttpBinaryCacheStoreConfig now preserves query parameters for S3 URLs - makeRequest() properly constructs URLs preserving query parameters without corrupting the path Now users can specify: s3://bucket?region=us-east-2 to access buckets in specific AWS regions.
Add tests to prevent regression of S3 functionality: 1. Store Registration (commit 7a2f289): - Test that S3 scheme is recognized in supported URI schemes - Test that S3 store config can be created without errors 2. Upload Support (commit c0164e0): - Test that S3 uploads are not rejected with "not supported" error - Verify upload requests are accepted (may fail on credentials/network) 3. Region Handling (commit e618ac7e0): - Test that query parameters are preserved for S3 stores - Test URL parsing with various region specifications - Verify region parameters are correctly maintained
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')
…acheStoreConfig Following reviewer feedback, S3-specific settings should not be part of HttpBinaryCacheStoreConfig since they are invalid for regular HTTP stores. This commit: - Creates a new S3BinaryCacheStoreConfig that inherits from HttpBinaryCacheStoreConfig - Moves S3-specific settings (region, endpoint, profile, scheme) to the new class - Registers S3BinaryCacheStoreConfig to handle s3:// URIs - Keeps HttpBinaryCacheStoreConfig clean for regular HTTP/HTTPS stores - Updates tests to use the new S3BinaryCacheStoreConfig
Replace nullptr returns with proper exception throwing using AwsAuthError. This addresses reviewer feedback requesting proper Error types instead of nullptr for authentication failures. Changes: - Add AwsAuthError exception class - Update AwsCredentialProvider methods to throw exceptions - Change getCredentials() to return AwsCredentials directly - Update all tests to handle exception-based API - Fix filetransfer.cc to catch AwsAuthError and continue for public buckets
Refactor S3 test files to use parameterized testing with INSTANTIATE_TEST_SUITE_P as requested by reviewer.
Add support for AWS S3 Transfer Acceleration to improve upload and download speeds by routing transfers through CloudFront edge locations. Features: - New `use-transfer-acceleration` setting in S3BinaryCacheStoreConfig - Automatic endpoint transformation to use s3-accelerate.dualstack.amazonaws.com - DNS-compliant bucket name validation (required for virtual-hosted style) Implementation details: - Transfer acceleration uses virtual-hosted style addressing - Validates bucket names for DNS compliance (3-63 chars, lowercase, no dots) - Incompatible with custom endpoints (throws error if both are specified) - Preserves backward compatibility (disabled by default) Usage: ``` nix copy --to 's3://my-bucket?use-transfer-acceleration=true' ... ```
github-actions
bot
added
documentation
with-tests
|
(added the thing for dpulls, no one will hold that CI failure against you :)) |
xokdvium
reviewed
Aug 19, 2025
Comment on lines
+879
to
+888
| * Check if a bucket name is DNS-compliant for use with transfer acceleration | ||
| * Requirements: | ||
| * - 3-63 characters long | ||
| * - Only lowercase letters, numbers, and hyphens | ||
| * - Cannot start or end with hyphen | ||
| * - Cannot contain consecutive hyphens | ||
| * - Cannot contain dots (for transfer acceleration) | ||
| * - Cannot be an IP address | ||
| */ | ||
| static bool isDnsCompliantBucketName(const std::string & bucket) |
There was a problem hiding this comment.
Eh, this seems to be just the requirements for bucket names in general https://docs.aws.amazon.com/AmazonS3/latest/userguide/bucketnamingrules.html#general-purpose-bucket-names
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2025-08-25-nix-team-meeting-minutes-243/68497/1
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
Demo of how #12973 could be achieved on top of #13752. Look only at the last commit.
Context
Depends on #13752
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.