From 4f5cbd5ccdbf309c7382dafc482fc7f39cd7050e Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Wed, 11 Dec 2024 15:53:36 +0000 Subject: [PATCH] feat(storage): retry SignBlob call for URL signing (#7862) --- .gitignore | 1 + Storage/src/Connection/RetryTrait.php | 21 ++++++-- Storage/src/SigningHelper.php | 48 +++++++++++++++--- Storage/tests/Unit/SigningHelperTest.php | 64 +++++++++++++++++++++++- 4 files changed, 123 insertions(+), 11 deletions(-) diff --git a/.gitignore b/.gitignore index 87c5820d73ce..208b6b17a1d6 100644 --- a/.gitignore +++ b/.gitignore @@ -17,3 +17,4 @@ keys/ .testing .split __pycache__ +.vscode/ diff --git a/Storage/src/Connection/RetryTrait.php b/Storage/src/Connection/RetryTrait.php index 253a10b682d4..4899e7e46f2d 100644 --- a/Storage/src/Connection/RetryTrait.php +++ b/Storage/src/Connection/RetryTrait.php @@ -69,6 +69,7 @@ trait RetryTrait 'objects.get', 'objects.list', 'serviceaccount.get', + 'signBlob.execute' ]; /** @@ -134,7 +135,9 @@ private function getRestRetryFunction($resource, $method, array $args) StorageClient::RETRY_IDEMPOTENT; return function ( - \Exception $exception + \Exception $exception, + $currentAttempt = 0, + $maxRetries = null ) use ( $isOpIdempotent, $preconditionNeeded, @@ -146,7 +149,9 @@ private function getRestRetryFunction($resource, $method, array $args) $isOpIdempotent, $preconditionNeeded, $preconditionSupplied, - $retryStrategy + $retryStrategy, + $currentAttempt, + $maxRetries ); }; } @@ -187,7 +192,8 @@ private function isPreConditionSupplied($methodName, array $args) * @param bool $isIdempotent * @param bool $preconditionNeeded * @param bool $preconditionSupplied - * @param int $maxRetries + * @param int|null $maxRetries The maximum number of retries allowd. + * Null for no limit. * @return bool */ private function retryDeciderFunction( @@ -195,8 +201,15 @@ private function retryDeciderFunction( $isIdempotent, $preconditionNeeded, $preconditionSupplied, - $retryStrategy + $retryStrategy, + $currentAttempt = 0, + $maxRetries = null ) { + // If maxRetries is specified, ensure we don't exceed it + if ($maxRetries !== null && $currentAttempt >= $maxRetries) { + return false; + } + if ($retryStrategy == StorageClient::RETRY_NEVER) { return false; } diff --git a/Storage/src/SigningHelper.php b/Storage/src/SigningHelper.php index bf48ab81f026..46cea73ec4c4 100644 --- a/Storage/src/SigningHelper.php +++ b/Storage/src/SigningHelper.php @@ -23,6 +23,8 @@ use Google\Cloud\Core\JsonTrait; use Google\Cloud\Core\Timestamp; use Google\Cloud\Storage\Connection\ConnectionInterface; +use Google\Cloud\Core\Exception\ServiceException; +use Google\Cloud\Storage\Connection\RetryTrait; /** * Provides common methods for signing storage URLs. @@ -33,6 +35,7 @@ class SigningHelper { use ArrayTrait; use JsonTrait; + use RetryTrait; const DEFAULT_URL_SIGNING_VERSION = 'v2'; const DEFAULT_DOWNLOAD_HOST = 'storage.googleapis.com'; @@ -40,6 +43,7 @@ class SigningHelper const V4_ALGO_NAME = 'GOOG4-RSA-SHA256'; const V4_TIMESTAMP_FORMAT = 'Ymd\THis\Z'; const V4_DATESTAMP_FORMAT = 'Ymd'; + const MAX_RETRIES = 5; /** * Create or fetch a SigningHelper instance. @@ -169,7 +173,7 @@ public function v2Sign(ConnectionInterface $connection, $expires, $resource, $ge $signedHeaders = []; foreach ($headers as $name => $value) { - $signedHeaders[] = $name .':'. $value; + $signedHeaders[] = $name . ':' . $value; } // Push the headers onto the end of the signing string. @@ -181,9 +185,10 @@ public function v2Sign(ConnectionInterface $connection, $expires, $resource, $ge $stringToSign = $this->createV2CanonicalRequest($toSign); - $signature = $credentials->signBlob($stringToSign, [ + // Use exponential backOff + $signature = $this->retrySignBlob(fn () => $credentials->signBlob($stringToSign, [ 'forceOpenssl' => $options['forceOpenssl'] - ]); + ])); // Start with user-provided query params and add required parameters. $params = $options['queryParams']; @@ -337,9 +342,11 @@ public function v4Sign(ConnectionInterface $connection, $expires, $resource, $ge $requestHash ]); - $signature = bin2hex(base64_decode($credentials->signBlob($stringToSign, [ - 'forceOpenssl' => $options['forceOpenssl'] - ]))); + $signature = bin2hex(base64_decode($this->retrySignBlob( + fn() => $credentials->signBlob($stringToSign, [ + 'forceOpenssl' => $options['forceOpenssl'] + ]) + ))); // Construct the modified resource name. If a custom hostname is provided, // this will remove the bucket name from the resource. @@ -882,4 +889,33 @@ private function buildQueryString(array $input) return implode('&', $q); } + + /** + * Retry logic for signBlob + * + * @param callable $signBlobFn A callable that perform the actual signBlob operation. + * @param string $resourceName The resource name for logging or retry strategy determination. + * @param array $args Arguments for the operations, include preconditions + * @return string The signature genarated by signBlob. + * @throws ServiceException If non-retryable error occur. + * @throws \RuntimeException If retries are exhausted. + */ + private function retrySignBlob(callable $signBlobFn, string $resourceName = 'signBlob', array $args = []) + { + $attempt = 0; + // Generate a retry decider function using the RetryTrait logic. + $retryDecider = $this->getRestRetryFunction($resourceName, 'execute', $args); + while (true) { + ++$attempt; + try { + // Attempt the operation + return $signBlobFn(); + } catch (\Exception $exception) { + if (!$retryDecider($exception, $attempt, self::MAX_RETRIES)) { + // Non-retryable error + throw $exception; + } + } + } + } } diff --git a/Storage/tests/Unit/SigningHelperTest.php b/Storage/tests/Unit/SigningHelperTest.php index 0bb9e52a505f..ddc0aec54c18 100644 --- a/Storage/tests/Unit/SigningHelperTest.php +++ b/Storage/tests/Unit/SigningHelperTest.php @@ -19,6 +19,7 @@ use Google\Auth\Credentials\ServiceAccountCredentials; use Google\Auth\SignBlobInterface; +use Google\Cloud\Core\Exception\ServiceException; use Google\Cloud\Core\RequestWrapper; use Google\Cloud\Core\Testing\TestHelpers; use Google\Cloud\Core\Timestamp; @@ -398,7 +399,7 @@ public function testV4SignCanonicalRequestSaveAsName() $this->helper->createV4CanonicalRequest = function ($request) use ($saveAsName) { parse_str($request[2], $query); - $expectedDisposition = 'attachment; filename="' . $saveAsName .'"'; + $expectedDisposition = 'attachment; filename="' . $saveAsName . '"'; $this->assertEquals($expectedDisposition, $query['response-content-disposition']); }; @@ -789,6 +790,67 @@ private function mockConnection($credentials = null, $rw = null) return $conn->reveal(); } + + public function testRetrySignBlobSuccessFirstAttempt() + { + $signBlobFn = function () { + return 'signature'; + }; + + $res = $this->helper->proxyPrivateMethodCall('retrySignBlob', [ + $signBlobFn + ]); + + $this->assertEquals('signature', $res); + } + + public function testRetrySignBlobSuccessAfterRetries() + { + $attempt = 0; + $signBlobFn = function () use (&$attempt) { + if (++$attempt < 3) { + throw new ServiceException('Transient error', 503); + } + return 'signature'; + }; + + $res = $this->helper->proxyPrivateMethodCall('retrySignBlob', [ + $signBlobFn + ]); + + $this->assertEquals('signature', $res); + $this->assertEquals(3, $attempt); + } + + public function testRetrySignBlobNonRetryableError() + { + $this->expectException(ServiceException::class); + $this->expectExceptionMessage('Non-retryable error'); + + $signBlobFn = function () { + throw new ServiceException('Non-retryable error', 400); + }; + + $res = $this->helper->proxyPrivateMethodCall('retrySignBlob', [ + $signBlobFn + ]); + } + + public function testRetrySignBlobThrowsExceptionAfterThreeAttempts() + { + $this->expectException(ServiceException::class); + $this->expectExceptionMessage('Transient error (5 attempts)'); + + $attempt = 0; + $signBlobFn = function () use (&$attempt) { + throw new ServiceException( + sprintf('Transient error (%s attempts)', ++$attempt), + 503 + ); + }; + + $this->helper->proxyPrivateMethodCall('retrySignBlob', [$signBlobFn]); + } } //@codingStandardsIgnoreStart