From 69425a303b2a09eb8eda38e11d962e51476b1898 Mon Sep 17 00:00:00 2001 From: Stephen Atherton Date: Wed, 7 Feb 2018 21:50:43 -0800 Subject: [PATCH] Improved error handling for cases where blob account credentials are either not found in the provided credentials sources and/or some of the credentials sources provided are not readable or parseable. --- fdbrpc/BlobStore.actor.cpp | 91 ++++++++++++++++++++++++-------------- flow/error_definitions.h | 2 + 2 files changed, 59 insertions(+), 34 deletions(-) diff --git a/fdbrpc/BlobStore.actor.cpp b/fdbrpc/BlobStore.actor.cpp index 328a9ecb37..9d0b510ffc 100644 --- a/fdbrpc/BlobStore.actor.cpp +++ b/fdbrpc/BlobStore.actor.cpp @@ -221,21 +221,27 @@ ACTOR Future deleteRecursively_impl(Reference b, std::s state Future done = b->listBucketStream(bucket, resultStream, prefix, '/', std::numeric_limits::max()); // Wrap done in an actor which will send end_of_stream since listBucketStream() does not (so that many calls can write to the same stream) done = map(done, [=](Void) { - resultStream.sendError(end_of_stream()); - return Void(); - }); + resultStream.sendError(end_of_stream()); + return Void(); + }); state std::list> deleteFutures; try { loop { - BlobStoreEndpoint::ListResult list = waitNext(resultStream.getFuture()); - for(auto &object : list.objects) { - int *pNumDeletedCopy = pNumDeleted; // avoid capture of this - deleteFutures.push_back(map(b->deleteObject(bucket, object.name), [pNumDeletedCopy](Void) -> Void { - if(pNumDeletedCopy != nullptr) - ++*pNumDeletedCopy; - return Void(); - })); + choose { + // Throw if done throws, otherwise don't stop until end_of_stream + when(Void _ = wait(done)) {} + + when(BlobStoreEndpoint::ListResult list = waitNext(resultStream.getFuture())) { + for(auto &object : list.objects) { + int *pNumDeletedCopy = pNumDeleted; // avoid capture of this + deleteFutures.push_back(map(b->deleteObject(bucket, object.name), [pNumDeletedCopy](Void) -> Void { + if(pNumDeletedCopy != nullptr) + ++*pNumDeletedCopy; + return Void(); + })); + } + } } // This is just a precaution to avoid having too many outstanding delete actors waiting to run @@ -290,9 +296,12 @@ Future BlobStoreEndpoint::objectSize(std::string const &bucket, std::st // Try to read a file, parse it as JSON, and return the resulting document. // It will NOT throw if any errors are encountered, it will just return an empty // JSON object and will log trace events for the errors encountered. -ACTOR Future tryReadJSONFile(std::string path) { +ACTOR Future> tryReadJSONFile(std::string path) { state std::string content; + // Event type to be logged in the event of an exception + state const char *errorEventType = "BlobCredentialFileError"; + try { state Reference f = wait(IAsyncFileSystem::filesystem()->open(path, IAsyncFile::OPEN_NO_AIO | IAsyncFile::OPEN_READONLY | IAsyncFile::OPEN_UNCACHED, 0)); state int64_t size = wait(f->size()); @@ -300,25 +309,22 @@ ACTOR Future tryReadJSONFile(std::string path) { int r = wait(f->read(mutateString(buf), size, 0)); ASSERT(r == size); content = buf.toString(); - } catch(Error &e) { - if(e.code() != error_code_actor_cancelled) - TraceEvent(SevWarn, "BlobCredentialFileError").detail("File", path).error(e).suppressFor(60, true); - return json_spirit::mObject(); - } - try { + // Any exceptions from hehre forward are parse failures + errorEventType = "BlobCredentialFileParseFailed"; json_spirit::mValue json; json_spirit::read_string(content, json); if(json.type() == json_spirit::obj_type) return json.get_obj(); else TraceEvent(SevWarn, "BlobCredentialFileNotJSONObject").detail("File", path).suppressFor(60, true); + } catch(Error &e) { if(e.code() != error_code_actor_cancelled) - TraceEvent(SevWarn, "BlobCredentialFileParseFailed").detail("File", path).error(e).suppressFor(60, true); + TraceEvent(SevWarn, errorEventType).detail("File", path).error(e).suppressFor(60, true); } - return json_spirit::mObject(); + return Optional(); } ACTOR Future updateSecret_impl(Reference b) { @@ -326,7 +332,7 @@ ACTOR Future updateSecret_impl(Reference b) { if(pFiles == nullptr) return Void(); - state std::vector> reads; + state std::vector>> reads; for(auto &f : *pFiles) reads.push_back(tryReadJSONFile(f)); @@ -334,13 +340,22 @@ ACTOR Future updateSecret_impl(Reference b) { std::string key = b->key + "@" + b->host; + int invalid = 0; + for(auto &f : reads) { - JSONDoc doc(f.get()); + // If value not present then the credentials file wasn't readable or valid. Continue to check other results. + if(!f.get().present()) { + ++invalid; + continue; + } + + JSONDoc doc(f.get().get()); if(doc.has("accounts") && doc.last().type() == json_spirit::obj_type) { JSONDoc accounts(doc.last().get_obj()); if(accounts.has(key, false) && accounts.last().type() == json_spirit::obj_type) { JSONDoc account(accounts.last()); std::string secret; + // Once we find a matching account, use it. if(account.tryGet("secret", secret)) { b->secret = secret; return Void(); @@ -349,7 +364,12 @@ ACTOR Future updateSecret_impl(Reference b) { } } - return Void(); + // If any sources were invalid + if(invalid > 0) + throw backup_auth_unreadable(); + + // All sources were valid but didn't contain the desired info + throw backup_auth_missing(); } Future BlobStoreEndpoint::updateSecret() { @@ -451,12 +471,9 @@ ACTOR Future> doRequest_impl(Reference listBucket_impl(Reference done = bstore->listBucketStream(bucket, resultStream, prefix, delimiter, maxDepth, recurseFilter); // Wrap done in an actor which sends end_of_stream because list does not so that many lists can write to the same stream done = map(done, [=](Void) { - resultStream.sendError(end_of_stream()); - return Void(); - }); + resultStream.sendError(end_of_stream()); + return Void(); + }); try { loop { - BlobStoreEndpoint::ListResult info = waitNext(resultStream.getFuture()); - results.commonPrefixes.insert(results.commonPrefixes.end(), info.commonPrefixes.begin(), info.commonPrefixes.end()); - results.objects.insert(results.objects.end(), info.objects.begin(), info.objects.end()); + choose { + // Throw if done throws, otherwise don't stop until end_of_stream + when(Void _ = wait(done)) {} + + when(BlobStoreEndpoint::ListResult info = waitNext(resultStream.getFuture())) { + results.commonPrefixes.insert(results.commonPrefixes.end(), info.commonPrefixes.begin(), info.commonPrefixes.end()); + results.objects.insert(results.objects.end(), info.objects.begin(), info.objects.end()); + } + } } } catch(Error &e) { if(e.code() != error_code_end_of_stream) diff --git a/flow/error_definitions.h b/flow/error_definitions.h index 372103b11b..02d69c3dad 100644 --- a/flow/error_definitions.h +++ b/flow/error_definitions.h @@ -174,6 +174,8 @@ ERROR( backup_bad_block_size, 2313, "Backup file block size too small") ERROR( backup_invalid_url, 2314, "Backup Container URL invalid") ERROR( backup_invalid_info, 2315, "Backup Container URL invalid") ERROR( backup_cannot_expire, 2316, "Cannot expire requested data from backup without violating minimum restorability") +ERROR( backup_auth_missing, 2317, "Cannot find authentication details (such as a password or secret key) for the specified Backup Container URL") +ERROR( backup_auth_unreadable, 2318, "Cannot read or parse one or more sources of authentication information for Backup Container URLs") ERROR( restore_invalid_version, 2361, "Invalid restore version") ERROR( restore_corrupted_data, 2362, "Corrupted backup data") ERROR( restore_missing_data, 2363, "Missing backup data")