bugfix: LocationConstraint middleware (#3002)

aws · Sep 25, 2024 · bdd23b9 · bdd23b9
1 parent f6d2c64
commit bdd23b9
Show file tree

Hide file tree

Showing 3 changed files with 103 additions and 4 deletions.
diff --git a/.changes/nextrelease/s3-locationconstraint.json 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
@@ -234,6 +234,8 @@
  */
 class S3Client extends AwsClient implements S3ClientInterface
 {
+    private 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
@@ -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,12 @@ public function testAddsLocationConstraintAutomatically($region, $target, $comma
         } else {
             $this->assertStringNotContainsString($text, $body);
         }
+        //ensures original configuration is not overwritten
+        if ($target !== 'us-east-1'
+            && $command->getName() === 'CreateBucket'
+        ) {
+            $this->assertStringContainsString('<Bucket><Type>foo</Type></Bucket>', $body);
+        }
     }
 
     public function getTestCasesForLocationConstraints()
@@ -537,6 +550,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 +2531,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
+        ];
+    }
 }