From d52a758cbb12a577dabd62836883d3c4c6cf9d79 Mon Sep 17 00:00:00 2001 From: Sean O'Brien Date: Tue, 24 Sep 2024 15:03:22 -0400 Subject: [PATCH] bugfix: LocationConstraint middleware --- .../nextrelease/s3-locationconstraint.json | 7 ++ src/S3/S3Client.php | 24 +++++- tests/S3/S3ClientTest.php | 74 ++++++++++++++++++- 3 files changed, 101 insertions(+), 4 deletions(-) create mode 100644 .changes/nextrelease/s3-locationconstraint.json diff --git a/.changes/nextrelease/s3-locationconstraint.json b/.changes/nextrelease/s3-locationconstraint.json new file mode 100644 index 0000000000..d50838b933 --- /dev/null +++ b/.changes/nextrelease/s3-locationconstraint.json @@ -0,0 +1,7 @@ +[ + { + "type": "bugfix", + "category": "S3", + "description": "Updates location constraint middleware to exclude directory buckets and retain original configuration." + } +] diff --git a/src/S3/S3Client.php b/src/S3/S3Client.php index bcd0bbcc6f..5bad0a4479 100644 --- a/src/S3/S3Client.php +++ b/src/S3/S3Client.php @@ -234,6 +234,8 @@ */ class S3Client extends AwsClient implements S3ClientInterface { + public const DIRECTORY_BUCKET_REGEX = '/^[a-zA-Z0-9_-]+--[a-z0-9]+-az\d+--x-s3' + .'(?!.*(?:-s3alias|--ol-s3|\.mrap))$/'; use S3ClientTrait; /** @var array */ @@ -574,14 +576,20 @@ private function getLocationConstraintMiddleware() $region = $this->getRegion(); return static function (callable $handler) use ($region) { return function (Command $command, $request = null) use ($handler, $region) { - if ($command->getName() === 'CreateBucket') { + if ($command->getName() === 'CreateBucket' + && !self::isDirectoryBucket($command['Bucket']) + ) { $locationConstraint = $command['CreateBucketConfiguration']['LocationConstraint'] ?? null; if ($locationConstraint === 'us-east-1') { unset($command['CreateBucketConfiguration']); } elseif ('us-east-1' !== $region && empty($locationConstraint)) { - $command['CreateBucketConfiguration'] = ['LocationConstraint' => $region]; + if (isset($command['CreateBucketConfiguration'])) { + $command['CreateBucketConfiguration']['LocationConstraint'] = $region; + } else { + $command['CreateBucketConfiguration'] = ['LocationConstraint' => $region]; + } } } @@ -858,6 +866,18 @@ private function addBuiltIns($args) $this->clientBuiltIns[$key] = $value; } + /** + * Determines whether a bucket is a directory bucket. + * Only considers the availability zone/suffix format + * + * @param string $bucket + * @return bool + */ + public static function isDirectoryBucket(string $bucket): bool + { + return preg_match(self::DIRECTORY_BUCKET_REGEX, $bucket) === 1; + } + /** @internal */ public static function _applyRetryConfig($value, $args, HandlerList $list) { diff --git a/tests/S3/S3ClientTest.php b/tests/S3/S3ClientTest.php index f5341557c5..32fe9f061d 100644 --- a/tests/S3/S3ClientTest.php +++ b/tests/S3/S3ClientTest.php @@ -511,9 +511,16 @@ public function testProxiesToObjectCopy() public function testAddsLocationConstraintAutomatically($region, $target, $command, $contains) { $client = $this->getTestClient('S3', ['region' => $region]); - $params = ['Bucket' => 'foo']; + $params = [ + 'Bucket' => 'foo', + 'CreateBucketConfiguration' => [ + 'Bucket' => [ + 'Type' => 'foo' + ] + ] + ]; if ($region !== $target) { - $params['CreateBucketConfiguration'] = ['LocationConstraint' => $target]; + $params['CreateBucketConfiguration']['LocationConstraint'] = $target; } $command = $client->getCommand($command, $params); @@ -524,6 +531,10 @@ public function testAddsLocationConstraintAutomatically($region, $target, $comma } else { $this->assertStringNotContainsString($text, $body); } + //ensures original configuration is not overwritten + if ($command === 'CreateBucket') { + $this->assertStringContainsString('foo', $body); + } } public function getTestCasesForLocationConstraints() @@ -537,6 +548,33 @@ public function getTestCasesForLocationConstraints() ]; } + /** + * @param string $bucket + * + * @dataProvider directoryBucketLocationConstraintProvider + */ + public function testDoesNotAddLocationConstraintForDirectoryBuckets( + string $bucket + ): void + { + $client = $this->getTestClient('s3'); + $params = ['Bucket' => $bucket]; + $command = $client->getCommand('CreateBucket', $params); + $body = (string) \Aws\serialize($command)->getBody(); + $this->assertStringNotContainsString('LocationConstraint', $body); + } + + public function directoryBucketLocationConstraintProvider(): array + { + return [ + ['bucket-base-name--usw2-az1--x-s3'], + ['mybucket123--euw1-az2--x-s3'], + ['test_bucket_name--apne1-az3--x-s3'], + ['valid-name--aps1-az4--x-s3'], + ['s3_express_demo_directory_bucket--usw2-az1--x-s3'] + ]; + } + public function testSaveAsParamAddsSink() { $client = $this->getTestClient('S3', [ @@ -2491,4 +2529,36 @@ public function testS3RetriesOnNotParsableBody(array $retrySettings) $client->listBuckets(); $this->assertEquals(0, $retries); } + + /** + * @param string $bucketName + * @param bool $expected + * + * @return void + * + * @dataProvider directoryBucketProvider + */ + public function testIsDirectoryBucket(string $bucketName, bool $expected): void + { + $client = $this->getTestClient('s3'); + $this->assertEquals($expected, $client::isDirectoryBucket($bucketName)); + } + + public function directoryBucketProvider(): array + { + return [ + ['bucket-base-name--usw2-az1--x-s3', true], + ['mybucket123--euw1-az2--x-s3', true], + ['test_bucket_name--apne1-az3--x-s3', true], + ['valid-name--aps1-az4--x-s3', true], + ['s3_express_demo_directory_bucket--usw2-az1--x-s3', true], + ['bucket-name--usw2-az1--s3alias', false], // ends with -s3alias + ['bucketname--usw2-az1--ol-s3', false], // ends with --ol-s3 + ['bucketname--usw2-az1.mrap', false], // ends with .mrap + ['invalid_bucket_name--az1--x-s3', false], // missing region in azid + ['name--usw2-az1', false], // missing --x-s3 at the end + ['wrong-format--usw2az1--x-s3', false], // missing hyphen in azid part + ['too-short--x-s3', false], // invalid azid format, missing prefix + ]; + } }