Harden parseErrorCodeFromS3 to never throw on non-XML responses 7.3#12997
Harden parseErrorCodeFromS3 to never throw on non-XML responses 7.3#12997spraza merged 5 commits intoapple:release-7.3from
Conversation
…e#12986) Add parseErrorCodeFromS3 function that safely extracts S3 error codes from XML responses, returning "" instead of throwing on parse failures since many HTTP error responses (502/503 HTML pages, empty bodies) are not XML. Wire it into doRequest_impl error handling to log S3ErrorCode and detect bad requests. Add unit tests. (cherry picked from commit 7388f54)
8e7cae3 to
8de84e6
Compare
Result of foundationdb-pr-cluster-tests-73 on Linux RHEL 9
|
Result of foundationdb-pr-73 on Linux RHEL 9
|
Result of foundationdb-pr-clang-73 on Linux RHEL 9
|
Widen the S3 error logging from only HTTP 400 to all 4xx client errors (403, 404, 409, etc). Remove unused badRequestCode variable. Rename trace event from S3BlobStoreBadRequest to S3BlobStoreClientError.
Result of foundationdb-pr-clang-73 on Linux RHEL 9
|
Result of foundationdb-pr-73 on Linux RHEL 9
|
Result of foundationdb-pr-clang-73 on Linux RHEL 9
|
Result of foundationdb-pr-73 on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests-73 on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests-73 on Linux RHEL 9
|
Result of foundationdb-pr-clang-73 on Linux RHEL 9
|
Result of foundationdb-pr-73 on Linux RHEL 9
|
Result of foundationdb-pr-clang-73 on Linux RHEL 9
|
Result of foundationdb-pr-73 on Linux RHEL 9
|
| std::string s3Error = parseErrorCodeFromS3(r->data.content); | ||
| event.detail("S3ErrorCode", s3Error); | ||
| if (r->code >= 400 && r->code < 500) { | ||
| TraceEvent(SevWarnAlways, "S3BlobStoreClientError") |
There was a problem hiding this comment.
I would write this as event and let the detail calls below append to the existing one. Otherwise we have a new one at unlimited frequency
There was a problem hiding this comment.
Ugh. Yeah. This is redundant now. Let me push...
diff --git a/fdbclient/S3BlobStore.actor.cpp b/fdbclient/S3BlobStore.actor.cpp
index d0675c5c82..5f9e6a175b 100644
--- a/fdbclient/S3BlobStore.actor.cpp
+++ b/fdbclient/S3BlobStore.actor.cpp
@@ -1083,12 +1083,6 @@ ACTOR Future<Reference<HTTP::IncomingResponse>> doRequest_impl(Reference<S3BlobS
event.detail("HttpResponseContent", r->data.content);
std::string s3Error = parseErrorCodeFromS3(r->data.content);
event.detail("S3ErrorCode", s3Error);
- if (r->code >= 400 && r->code < 500) {
- TraceEvent(SevWarnAlways, "S3BlobStoreClientError")
- .detail("HttpCode", r->code)
- .detail("HttpResponseContent", r->data.content)
- .detail("S3Error", s3Error);
- }
}
event.detail("Verb", verb)
Result of foundationdb-pr-clang-73 on Linux RHEL 9
|
|
|
Result of foundationdb-pr-73 on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests-73 on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests-73 on Linux RHEL 9
|
|
I ran a quick 25k test |
Result of foundationdb-pr-clang-73 on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests-73 on Linux RHEL 9
|
Result of foundationdb-pr-73 on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests-73 on Linux RHEL 9
|
…pple#12997) * Harden parseErrorCodeFromS3 to never throw on non-XML responses (apple#12986) Add parseErrorCodeFromS3 function that safely extracts S3 error codes from XML responses, returning "" instead of throwing on parse failures since many HTTP error responses (502/503 HTML pages, empty bodies) are not XML. Wire it into doRequest_impl error handling to log S3ErrorCode and detect bad requests. Add unit tests. (cherry picked from commit 7388f54) * Log all S3 4xx client errors, not just 400 Widen the S3 error logging from only HTTP 400 to all 4xx client errors (403, 404, 409, etc). Remove unused badRequestCode variable. Rename trace event from S3BlobStoreBadRequest to S3BlobStoreClientError. * Fix trace * Simplify S3BlobStore request error handling (merge PR apple#13001) * Remove separate S3BlobStoreClientError event, use existing event --------- Co-authored-by: michael stack <stack@duboce.com>
* Harden parseErrorCodeFromS3 to never throw on non-XML responses 7.3 (#12997) - 7.3 * Harden parseErrorCodeFromS3 to never throw on non-XML responses (#12986) - main Add parseErrorCodeFromS3 function that safely extracts S3 error codes from XML responses, returning "" instead of throwing on parse failures since many HTTP error responses (502/503 HTML pages, empty bodies) are not XML. Wire it into doRequest_impl error handling to log S3ErrorCode and detect bad requests. Add unit tests. (cherry picked from commit 7388f54) * Log all S3 4xx client errors, not just 400 Widen the S3 error logging from only HTTP 400 to all 4xx client errors (403, 404, 409, etc). Remove unused badRequestCode variable. Rename trace event from S3BlobStoreBadRequest to S3BlobStoreClientError. * Fix trace * Simplify S3BlobStore request error handling (merge PR #13001) * Remove separate S3BlobStoreClientError event, use existing event
Return "" instead of throwing on parse failures since many HTTP error responses (502/503 HTML pages, empty bodies) are not XML. Remove unused backup_parse_s3_response_failure error code. Fix const-ref parameter and add unit tests.
This is backport of #12996 and then makes use of the backported method so this PR subsumes #12979