Skip to content

Commit

Permalink
Bug fix. Documentation says backup blob URL can optionally contain
Browse files Browse the repository at this point in the history 
key/secret/token; if absent from the URL, it'll be read from the
blob-credentials file. This is not how the code works; it requires
credentials are supplied in the blob URL.

TODO: Add test and fix case of credentials in URL but no
blob-credentials file -- it currently hangs.

Here is what the doc. says:

Blob Credential Files
In order to help safeguard blob store credentials, the <SECRET> can optionally be omitted from blobstore:// URLs on the command line. Omitted secrets will be resolved at connect time using 1 or more Blob Credential files.

From https://apple.github.io/foundationdb/backups.html

This PR includes a few small changes which make it so we always
read credentials file before making a connection. Credentials
age so its probably what we want. Its also more amenable for
the user if they only have to supply the credentials in one
place instead of in two (once on blob store URL and again in
the credentials file).

* documentation/sphinx/source/backups.rst
 Minor edit. Add more examples making it clearer how to do S3
 backup URLs in particular.

* fdbclient/S3BlobStore.actor.cpp
 Before each connection, call updateSecret every time.

* fdbclient/S3Client_cli.actor.cpp
 Minor cleanup of usage.
  • Loading branch information
michael stack committed Dec 11, 2024
1 parent 0032465 commit a961e2f
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 36 deletions.
27 changes: 21 additions & 6 deletions documentation/sphinx/source/backups.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,17 +77,17 @@ For local directories, the Backup URL format is
An example would be ``file:///home/backups`` which would refer to the directory ``/home/backups``.
Note that since paths must be absolute this will result in three slashes (/) in a row in the URL.

Note that for local directory URLs the actual backup files will not be written to <base_dir> directly but rather to a uniquely timestamped subdirectory. When starting a restore the path to the timestamped subdirectory must be specified.
For local directory URLs the actual backup files will not be written to <base_dir> directly but rather to a uniquely timestamped subdirectory. When starting a restore the path to the timestamped subdirectory must be specified.

For blob store backup locations, the Backup URL format is

::

blobstore://[<api_key>][:<secret>[:<security_token>]]@<hostname>[:<port>]/<name>?bucket=<bucket_name>[&region=<region_name>][&<param>=<value>]...]

<api_key> - API key to use for authentication. Optional.
<secret> - API key's secret. Optional.
<security_token> - Security token if temporary credentials are used. Optional.
<api_key> - API key to use for authentication. If S3, it is AWS_ACCESS_KEY_ID. Optional.
<secret> - API key's secret. If S3, it is AWS_SECRET_ACCESS_KEY. Optional.
<security_token> - Security token if temporary credentials are used. If S3, it is AWS_SESSION_TOKEN. Optional.
<hostname> - Remote hostname or IP address to connect to
<port> - Remote port to connect to. Optional. Default is 80.
<name> - Name of the backup within the backup bucket. It can contain '/' characters in order to organize backups into a folder-like structure.
Expand All @@ -101,6 +101,7 @@ A single bucket (specified by <bucket_name>) can hold any number of backups, eac
If <secret> is not specified, it will be looked up in :ref:`blob credential sources<blob-credential-files>`.

An example blob store Backup URL would be ``blobstore://myKey:[email protected]:80/dec_1_2017_0400?bucket=backups``.
Another for an S3 blobstore would look like: ``blobstore://${AWS_ACCESS_KEY_ID}:${AWS_SECRET_ACCESS_KEY}:${AWS_SESSION_TOKEN}@backup-12345-us-west-2.s3.amazonaws.com/dailies?bucket=backup-12345-us-west-2&region=us-west-2``

Blob store Backup URLs can have optional parameters at the end which set various limits or options used when communicating with the store. All values must be positive decimal integers unless otherwise specified. The speed related default values are not very restrictive. A parameter is applied individually on each ``backup_agent``, meaning that a global restriction should be calculated based on the number of agent running. The most likely parameter a user would want to change is ``max_send_bytes_per_second`` (or ``sbps`` for short) which determines the upload speed to the blob service.

Expand Down Expand Up @@ -199,8 +200,22 @@ If temporary credentials are being used, the following schema is also supported

{
"accounts" : {
"@host" : { "api_key" : user, "secret" : "SECRETKEY", token: "TOKEN1" },
"@host2" : { "api_key" : user2, "secret" : "SECRETKEY2", token: "TOKEN2" }
"@host" : { "api_key" : user, "secret" : "SECRETKEY", "token": "TOKEN1" },
"@host2" : { "api_key" : user2, "secret" : "SECRETKEY2", "token": "TOKEN2" }
}
}

For example:

::

{
"accounts": {
"@backup-12345-us-west-2.s3.amazonaws.com": {
"api_key": "AWS_ACCESS_KEY_ID",
"secret": "AWS_SECRET_ACCESS_KEY",
"token": "AWS_SESSION_TOKEN"
}
}
}

Expand Down
33 changes: 10 additions & 23 deletions fdbclient/S3BlobStore.actor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -668,17 +668,13 @@ ACTOR Future<Void> updateSecret_impl(Reference<S3BlobStoreEndpoint> b) {
if (pFiles == nullptr)
return Void();

if (!b->credentials.present()) {
return Void();
}

state std::vector<Future<Optional<json_spirit::mObject>>> reads;
for (auto& f : *pFiles)
reads.push_back(tryReadJSONFile(f));

wait(waitForAll(reads));

std::string accessKey = b->lookupKey ? "" : b->credentials.get().key;
std::string accessKey = b->lookupKey || !b->credentials.present() ? "" : b->credentials.get().key;
std::string credentialsFileKey = accessKey + "@" + b->host;

int invalid = 0;
Expand All @@ -695,21 +691,14 @@ ACTOR Future<Void> updateSecret_impl(Reference<S3BlobStoreEndpoint> b) {
JSONDoc accounts(doc.last().get_obj());
if (accounts.has(credentialsFileKey, false) && accounts.last().type() == json_spirit::obj_type) {
JSONDoc account(accounts.last());
S3BlobStoreEndpoint::Credentials creds = b->credentials.get();
if (b->lookupKey) {
std::string apiKey;
if (account.tryGet("api_key", apiKey))
creds.key = apiKey;
else
continue;
}
if (b->lookupSecret) {
std::string secret;
if (account.tryGet("secret", secret))
creds.secret = secret;
else
continue;
}
S3BlobStoreEndpoint::Credentials creds = b->credentials.present() ?
b->credentials.get(): S3BlobStoreEndpoint::Credentials();
std::string apiKey;
if (account.tryGet("api_key", apiKey))
creds.key = apiKey;
std::string secret;
if (account.tryGet("secret", secret))
creds.secret = secret;
std::string token;
if (account.tryGet("token", token))
creds.securityToken = token;
Expand Down Expand Up @@ -791,9 +780,7 @@ ACTOR Future<S3BlobStoreEndpoint::ReusableConnection> connect_impl(Reference<S3B
.detail("ExpiresIn", b->knobs.max_connection_life)
.detail("Proxy", b->proxyHost.orDefault(""));

if (b->lookupKey || b->lookupSecret || b->knobs.sdk_auth)
wait(b->updateSecret());

wait(b->updateSecret());
return S3BlobStoreEndpoint::ReusableConnection({ conn, now() + b->knobs.max_connection_life });
}

Expand Down
14 changes: 7 additions & 7 deletions fdbclient/S3Client_cli.actor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,9 @@ static void printUsage(std::string const& programName) {
" Has no effect unless --log is specified.\n"
" --blob-credentials FILE\n"
" File containing blob credentials in JSON format.\n"
" The same credential format/file fdbbackup uses.\n" TLS_HELP
" The same credential format/file fdbbackup uses.\n"
" Consulted on each connection setup if present.\n"
" See 'Blob Credential Files' in https://apple.github.io/foundationdb/backups.html.\n"
" --build-flags Print build information and exit.\n"
" --knob-KNOBNAME KNOBVALUE\n"
" Changes a knob value. KNOBNAME should be lowercase.\n"
Expand All @@ -107,15 +109,13 @@ static void printUsage(std::string const& programName) {
" If SOURCE is an s3 bucket URL, TARGET must be a directory and vice versa.\n"
" See 'Backup URLs' in https://apple.github.io/foundationdb/backups.html for\n"
" the fdb s3 'blobstore://' url format.\n"
" TARGET Where to place the copy.\n"
" TARGET Where to place the copy.\n" TLS_HELP
"Examples:\n"
" "
<< programName
<< " --blob-credentials /path/to/credentials.json /path/to/source /path/to/target\n"
" "
<< programName
<< " --knob_http_verbose_level=10 --log "
"'blobstore://localhost:8333/x?bucket=backup&region=us&secure_connection=0' dir3\n";
<< " --knob_http_verbose_level=10 --log --knob_blobstore_encryption_type=aws:kms "
<< " --tls-ca-file /etc/ssl/cert.pem --tls-certificate-file /tmp/cert.pem --tls-key-file /tmp/key.pem "
"'blobstore://AWS_ACCESS_KEY_ID:AWS_SECRET_ACCESS_KEY:AWS_SESSION_TOKEN@localhost:8333/x?bucket=backup&region=us' dir3\n";
return;
}

Expand Down

0 comments on commit a961e2f

Please sign in to comment.