From 038d47bcc05d3de5b377c330576bcc76648ccb8b Mon Sep 17 00:00:00 2001 From: David Steele Date: Tue, 8 Aug 2017 17:15:01 -0400 Subject: [PATCH] Retry when S3 returns an internal error (500). --- doc/xml/release.xml | 4 + lib/pgBackRest/Common/Http/Client.pm | 4 +- lib/pgBackRest/Storage/S3/Request.pm | 143 ++++++++------ .../pgBackRestTest/Common/ContainerTest.pm | 4 + test/lib/pgBackRestTest/Common/DefineTest.pm | 9 + .../Module/Storage/StorageS3RequestTest.pm | 178 ++++++++++++++++++ 6 files changed, 288 insertions(+), 54 deletions(-) create mode 100644 test/lib/pgBackRestTest/Module/Storage/StorageS3RequestTest.pm diff --git a/doc/xml/release.xml b/doc/xml/release.xml index da220d801..df344274e 100644 --- a/doc/xml/release.xml +++ b/doc/xml/release.xml @@ -202,6 +202,10 @@ + +

Retry when S3 returns an internal error (500).

+ +

Add bIgnoreMissing parameter to Local->manifest().

 diff --git a/lib/pgBackRest/Common/Http/Client.pm b/lib/pgBackRest/Common/Http/Client.pm index 75054bb83..49f7fc00a 100644 --- a/lib/pgBackRest/Common/Http/Client.pm +++ b/lib/pgBackRest/Common/Http/Client.pm @@ -49,6 +49,7 @@ sub new $strOperation, $strHost, $strVerb, + $iPort, $strUri, $hQuery, $hRequestHeader, @@ -64,6 +65,7 @@ sub new __PACKAGE__ . '->new', \@_, {name => 'strHost', trace => true}, {name => 'strVerb', trace => true}, + {name => 'iPort', optional => true, default => 443, trace => true}, {name => 'strUri', optional => true, default => qw(/), trace => true}, {name => 'hQuery', optional => true, trace => true}, {name => 'hRequestHeader', optional => true, trace => true}, @@ -81,7 +83,7 @@ sub new eval { $oSocket = IO::Socket::SSL->new( - PeerHost => $strHost, PeerPort => 'https', SSL_verify_mode => $bVerifySsl ? SSL_VERIFY_PEER : SSL_VERIFY_NONE, + PeerHost => $strHost, PeerPort => $iPort, SSL_verify_mode => $bVerifySsl ? SSL_VERIFY_PEER : SSL_VERIFY_NONE, SSL_ca_path => $strCaPath, SSL_ca_file => $strCaFile); return 1; diff --git a/lib/pgBackRest/Storage/S3/Request.pm b/lib/pgBackRest/Storage/S3/Request.pm index 7a6c77ec7..3b46f5128 100644 --- a/lib/pgBackRest/Storage/S3/Request.pm +++ b/lib/pgBackRest/Storage/S3/Request.pm @@ -46,6 +46,12 @@ use constant S3_RESPONSE_TYPE_NONE => 'none'; use constant S3_RESPONSE_TYPE_XML => 'xml'; push @EXPORT, qw(S3_RESPONSE_TYPE_XML); +use constant S3_RESPONSE_CODE_SUCCESS => 200; +use constant S3_RESPONSE_CODE_ERROR_NOT_FOUND => 404; +use constant S3_RESPONSE_CODE_ERROR_INTERNAL => 500; + +use constant S3_RETRY_MAX => 2; + #################################################################################################################################### # new #################################################################################################################################### @@ -66,6 +72,7 @@ sub new $self->{strAccessKeyId}, $self->{strSecretAccessKey}, $self->{strHost}, + $self->{iPort}, $self->{bVerifySsl}, $self->{strCaPath}, $self->{strCaFile}, @@ -80,6 +87,7 @@ sub new {name => 'strAccessKeyId', trace => true}, {name => 'strSecretAccessKey', trace => true}, {name => 'strHost', optional => true, trace => true}, + {name => 'iPort', optional => true, trace => true}, {name => 'bVerifySsl', optional => true, default => true, trace => true}, {name => 'strCaPath', optional => true, trace => true}, {name => 'strCaFile', optional => true, trace => true}, @@ -128,75 +136,104 @@ sub request {name => 'bIgnoreMissing', optional => true, default => false, trace => true}, ); - # Get datetime to be used for auth requests - my $strDateTime = s3DateTime(); - - # Set content length and hash - $hHeader->{&S3_HEADER_CONTENT_SHA256} = defined($rstrBody) ? sha256_hex($$rstrBody) : PAYLOAD_DEFAULT_HASH; - $hHeader->{&S3_HEADER_CONTENT_LENGTH} = defined($rstrBody) ? length($$rstrBody) : 0; - - # Generate authorization header - $hHeader = s3AuthorizationHeader( - $self->{strRegion}, "$self->{strBucket}.$self->{strEndPoint}", $strVerb, $strUri, httpQuery($hQuery), $strDateTime, - $hHeader, $self->{strAccessKeyId}, $self->{strSecretAccessKey}, $hHeader->{&S3_HEADER_CONTENT_SHA256}); - - # Send the request - my $oHttpClient = new pgBackRest::Common::Http::Client( - $self->{strHost}, $strVerb, - {strUri => $strUri, hQuery => $hQuery, hRequestHeader => $hHeader, rstrRequestBody => $rstrBody, - bVerifySsl => $self->{bVerifySsl}, strCaPath => $self->{strCaPath}, strCaFile => $self->{strCaFile}, - lBufferMax => $self->{lBufferMax}}); - - # Check response code - my $iReponseCode = $oHttpClient->responseCode(); + # Server response my $oResponse; - if ($iReponseCode == 200) + # Allow retries on S3 internal failures + my $bRetry = false; + my $iRetryTotal = 0; + + do { - # Save the response headers locally - $self->{hResponseHeader} = $oHttpClient->responseHeader(); + # Assume that a retry will not be attempted which is true in most cases + $bRetry = false; - # XML response is expected - if ($strResponseType eq S3_RESPONSE_TYPE_XML) + # Get datetime to be used for auth requests + my $strDateTime = s3DateTime(); + + # Set content length and hash + $hHeader->{&S3_HEADER_CONTENT_SHA256} = defined($rstrBody) ? sha256_hex($$rstrBody) : PAYLOAD_DEFAULT_HASH; + $hHeader->{&S3_HEADER_CONTENT_LENGTH} = defined($rstrBody) ? length($$rstrBody) : 0; + + # Generate authorization header + $hHeader = s3AuthorizationHeader( + $self->{strRegion}, "$self->{strBucket}.$self->{strEndPoint}", $strVerb, $strUri, httpQuery($hQuery), $strDateTime, + $hHeader, $self->{strAccessKeyId}, $self->{strSecretAccessKey}, $hHeader->{&S3_HEADER_CONTENT_SHA256}); + + # Send the request + my $oHttpClient = new pgBackRest::Common::Http::Client( + $self->{strHost}, $strVerb, + {iPort => $self->{iPort}, strUri => $strUri, hQuery => $hQuery, hRequestHeader => $hHeader, + rstrRequestBody => $rstrBody, bVerifySsl => $self->{bVerifySsl}, strCaPath => $self->{strCaPath}, + strCaFile => $self->{strCaFile}, lBufferMax => $self->{lBufferMax}}); + + # Check response code + my $iReponseCode = $oHttpClient->responseCode(); + + if ($iReponseCode == S3_RESPONSE_CODE_SUCCESS) { - my $rtResponseBody = $oHttpClient->responseBody(); + # Save the response headers locally + $self->{hResponseHeader} = $oHttpClient->responseHeader(); - if ($oHttpClient->contentLength() == 0 || !defined($$rtResponseBody)) + # XML response is expected + if ($strResponseType eq S3_RESPONSE_TYPE_XML) { - confess &log(ERROR, - "response type '${strResponseType}' was requested but content length is zero or content is missing", - ERROR_PROTOCOL); + my $rtResponseBody = $oHttpClient->responseBody(); + + if ($oHttpClient->contentLength() == 0 || !defined($$rtResponseBody)) + { + confess &log(ERROR, + "response type '${strResponseType}' was requested but content length is zero or content is missing", + ERROR_PROTOCOL); + } + + $oResponse = xmlParse($$rtResponseBody); } - - $oResponse = xmlParse($$rtResponseBody); - } - # An IO object is expected for file responses - elsif ($strResponseType eq S3_RESPONSE_TYPE_IO) - { - $oResponse = $oHttpClient; - } - } - else - { - if ($iReponseCode == 404) - { - if (!$bIgnoreMissing) + # An IO object is expected for file responses + elsif ($strResponseType eq S3_RESPONSE_TYPE_IO) { - confess &log(ERROR, "unable to open '${strUri}': No such file or directory", ERROR_FILE_MISSING); + $oResponse = $oHttpClient; } } else { - my $rstrResponseBody = $oHttpClient->responseBody(); + # If file was not found + if ($iReponseCode == S3_RESPONSE_CODE_ERROR_NOT_FOUND) + { + # If missing files should not be ignored then error + if (!$bIgnoreMissing) + { + confess &log(ERROR, "unable to open '${strUri}': No such file or directory", ERROR_FILE_MISSING); + } - confess &log(ERROR, - "S3 request error [$iReponseCode] " . $oHttpClient->responseMessage() . - "\n*** request header ***\n" . $oHttpClient->requestHeaderText() . - "\n*** reponse header ***\n" . $oHttpClient->responseHeaderText() . - (defined($$rstrResponseBody) ? "\n*** response body ***\n${$rstrResponseBody}" : ''), - ERROR_PROTOCOL); + $bRetry = false; + } + # Else a more serrious error + else + { + # Retry for S3 internal errors + if ($iReponseCode == S3_RESPONSE_CODE_ERROR_INTERNAL) + { + $iRetryTotal++; + $bRetry = $iRetryTotal <= S3_RETRY_MAX; + } + + # If no retry then throw the error + if (!$bRetry) + { + my $rstrResponseBody = $oHttpClient->responseBody(); + + confess &log(ERROR, + "S3 request error [$iReponseCode] " . $oHttpClient->responseMessage() . + "\n*** request header ***\n" . $oHttpClient->requestHeaderText() . + "\n*** reponse header ***\n" . $oHttpClient->responseHeaderText() . + (defined($$rstrResponseBody) ? "\n*** response body ***\n${$rstrResponseBody}" : ''), + ERROR_PROTOCOL); + } + } } } + while ($bRetry); # Return from function and log return values if any return logDebugReturn diff --git a/test/lib/pgBackRestTest/Common/ContainerTest.pm b/test/lib/pgBackRestTest/Common/ContainerTest.pm index f7c28e61a..dad773ca6 100755 --- a/test/lib/pgBackRestTest/Common/ContainerTest.pm +++ b/test/lib/pgBackRestTest/Common/ContainerTest.pm @@ -61,6 +61,10 @@ use constant LIB_COVER_EXE => '/usr/bin use constant CERT_FAKE_PATH => '/etc/fake-cert'; use constant CERT_FAKE_CA => CERT_FAKE_PATH . '/ca.crt'; push @EXPORT, qw(CERT_FAKE_CA); +use constant CERT_FAKE_SERVER => CERT_FAKE_PATH . '/server.crt'; + push @EXPORT, qw(CERT_FAKE_SERVER); +use constant CERT_FAKE_SERVER_KEY => CERT_FAKE_PATH . '/server.key'; + push @EXPORT, qw(CERT_FAKE_SERVER_KEY); #################################################################################################################################### # Container Debug - speeds container debugging by splitting each section into a separate intermediate container diff --git a/test/lib/pgBackRestTest/Common/DefineTest.pm b/test/lib/pgBackRestTest/Common/DefineTest.pm index 3b29a1f2f..be39c94f0 100644 --- a/test/lib/pgBackRestTest/Common/DefineTest.pm +++ b/test/lib/pgBackRestTest/Common/DefineTest.pm @@ -210,6 +210,15 @@ my $oTestDef = &TESTDEF_NAME => 's3-cert', &TESTDEF_TOTAL => 1, }, + { + &TESTDEF_NAME => 's3-request', + &TESTDEF_TOTAL => 2, + + &TESTDEF_COVERAGE => + { + 'Storage/S3/Request' => TESTDEF_COVERAGE_PARTIAL, + }, + }, { &TESTDEF_NAME => 's3', &TESTDEF_TOTAL => 7, diff --git a/test/lib/pgBackRestTest/Module/Storage/StorageS3RequestTest.pm b/test/lib/pgBackRestTest/Module/Storage/StorageS3RequestTest.pm new file mode 100644 index 000000000..1a6042748 --- /dev/null +++ b/test/lib/pgBackRestTest/Module/Storage/StorageS3RequestTest.pm @@ -0,0 +1,178 @@ +#################################################################################################################################### +# S3 Request Tests +#################################################################################################################################### +package pgBackRestTest::Module::Storage::StorageS3RequestTest; +use parent 'pgBackRestTest::Common::RunTest'; + +#################################################################################################################################### +# Perl includes +#################################################################################################################################### +use strict; +use warnings FATAL => qw(all); +use Carp qw(confess); +use English '-no_match_vars'; + + use IO::Socket::SSL; +use POSIX qw(strftime); + +use pgBackRest::Common::Exception; +use pgBackRest::Common::Http::Client; +use pgBackRest::Common::Log; +use pgBackRest::Common::Wait; +use pgBackRest::Storage::S3::Request; + +use pgBackRestTest::Common::ContainerTest; +use pgBackRestTest::Common::ExecuteTest; +use pgBackRestTest::Common::RunTest; + +#################################################################################################################################### +# Port to use for testing +#################################################################################################################################### +use constant HTTPS_TEST_PORT => 9443; + +#################################################################################################################################### +# httpsServerResponse +#################################################################################################################################### +sub httpsServerResponse +{ + my $self = shift; + my $iResponseCode = shift; + my $strContent = shift; + + # Write header + $self->{oConnection}->write("HTTP/1.1 ${iResponseCode} GenericMessage\r\n"); + $self->{oConnection}->write(HTTP_HEADER_CONTENT_LENGTH . ': ' . (defined($strContent) ? length($strContent) : 0) . "\r\n"); + + # Write new line before content (even if there isn't any) + $self->{oConnection}->write("\r\n"); + + # Write content + if (defined($strContent)) + { + $self->{oConnection}->write($strContent); + } + + # This will block until the connection is closed by the client + $self->{oConnection}->read(); +} + +#################################################################################################################################### +# httpsServerAccept +#################################################################################################################################### +sub httpsServerAccept +{ + my $self = shift; + + # Wait for a connection + $self->{oConnection} = $self->{oSocketServer}->accept() + or confess "failed to accept or handshake $!, $SSL_ERROR"; + &log(INFO, " * socket server connected"); +} + +#################################################################################################################################### +# httpsServer +#################################################################################################################################### +sub httpsServer +{ + my $self = shift; + my $fnServer = shift; + + # Fork off the server + if (fork() == 0) + { + # Run server function + $fnServer->(); + + exit 0; + } +} + +#################################################################################################################################### +# Start the https testing server +#################################################################################################################################### +sub initModule +{ + my $self = shift; + + # Open the domain socket + $self->{oSocketServer} = IO::Socket::SSL->new( + LocalAddr => '127.0.0.1', LocalPort => HTTPS_TEST_PORT, Listen => 1, SSL_cert_file => CERT_FAKE_SERVER, + SSL_key_file => CERT_FAKE_SERVER_KEY) + or confess "unable to open https server for testing: $!"; + &log(INFO, " * socket server open"); +} + +#################################################################################################################################### +# Stop the https testing server +#################################################################################################################################### +sub cleanModule +{ + my $self = shift; + + # Shutdown server + $self->{oSocketServer}->close(); + &log(INFO, " * socket server closed"); +} + +#################################################################################################################################### +# run +#################################################################################################################################### +sub run +{ + my $self = shift; + + # Initialize request object + my $oS3Request = new pgBackRest::Storage::S3::Request( + BOGUS, BOGUS, BOGUS, BOGUS, BOGUS, {strHost => '127.0.0.1', iPort => HTTPS_TEST_PORT, bVerifySsl => false}); + + ################################################################################################################################ + if ($self->begin('success')) + { + $self->httpsServer(sub + { + $self->httpsServerAccept(); + $self->httpsServerResponse(200); + + #----------------------------------------------------------------------------------------------------------------------- + $self->httpsServerAccept(); + $self->httpsServerResponse(200); + }); + + #--------------------------------------------------------------------------------------------------------------------------- + $self->testResult(sub {$oS3Request->request(HTTP_VERB_GET)}, undef, 'successful request'); + $self->testResult(sub {$oS3Request->request(HTTP_VERB_GET)}, undef, 'successful request'); + } + + ################################################################################################################################ + if ($self->begin('retry')) + { + $self->httpsServer(sub + { + $self->httpsServerAccept(); + $self->httpsServerResponse(500); + + $self->httpsServerAccept(); + $self->httpsServerResponse(500); + + $self->httpsServerAccept(); + $self->httpsServerResponse(200); + + #----------------------------------------------------------------------------------------------------------------------- + $self->httpsServerAccept(); + $self->httpsServerResponse(500); + + $self->httpsServerAccept(); + $self->httpsServerResponse(500); + + $self->httpsServerAccept(); + $self->httpsServerResponse(500); + }); + + #--------------------------------------------------------------------------------------------------------------------------- + $self->testResult(sub {$oS3Request->request(HTTP_VERB_GET)}, undef, 'successful request after retries'); + $self->testException( + sub {$oS3Request->request(HTTP_VERB_GET)}, ERROR_PROTOCOL, 'S3 request error \[500\] GenericMessage.*'); + } +} + +1;