Make bucket and profile private

Add _ prefix to bucket and profile properties in S3ResourcePath

Author: dhirving

python/lsst/resources/s3.py

@@ -192,19 +192,29 @@ def _transfer_config(self) -> TransferConfig:
     @property
     def client(self) -> boto3.client:
         """Client object to address remote resource."""
-        return getS3Client(self.profile)
+        return getS3Client(self._profile)
 
     @property
-    def profile(self) -> str | None:
+    def _profile(self) -> str | None:
+        """Profile name to use for looking up S3 credentials and endpoint."""
         return self._uri.username
 
     @property
-    def bucket(self) -> str:
+    def _bucket(self) -> str:
+        """S3 bucket where the files are stored."""
+        # Notionally the bucket is stored in the 'hostname' part of the URI.
+        # However, Ceph S3 uses a "multi-tenant" syntax for bucket names in the
+        # form 'tenant:bucket'.  The part after the colon is parsed as the port
+        # portion of the URI, and urllib throws an exception if you try to read
+        # a non-integer port value.  So manually split off this portion of the
+        # URI.
         split = self._uri.netloc.split("@")
         num_components = len(split)
         if num_components == 2:
+            # There is a profile@ portion of the URL, so take the second half.
             bucket = split[1]
         elif num_components == 1:
+            # There is no profile@, so take the whole netloc.
             bucket = split[0]
         else:
             raise ValueError(f"Unexpected extra '@' in S3 URI: '{str(self)}'")
@@ -216,12 +226,12 @@ def bucket(self) -> str:
 
     @classmethod
     def _mexists(cls, uris: Iterable[ResourcePath]) -> dict[ResourcePath, bool]:
-        # Force client to be created before creating threads.
+        # Force client to be created for each profile before creating threads.
         profiles = set[str | None]()
         for path in uris:
             if path.scheme == "s3":
                 path = cast(S3ResourcePath, path)
-                profiles.add(path.profile)
+                profiles.add(path._profile)
         for profile in profiles:
             getS3Client(profile)
 
@@ -232,16 +242,16 @@ def exists(self) -> bool:
         """Check that the S3 resource exists."""
         if self.is_root:
             # Only check for the bucket since the path is irrelevant
-            return bucketExists(self.bucket, self.client)
-        exists, _ = s3CheckFileExists(self, bucket=self.bucket, client=self.client)
+            return bucketExists(self._bucket, self.client)
+        exists, _ = s3CheckFileExists(self, bucket=self._bucket, client=self.client)
         return exists
 
     @backoff.on_exception(backoff.expo, retryable_io_errors, max_time=max_retry_time)
     def size(self) -> int:
         """Return the size of the resource in bytes."""
         if self.dirLike:
             return 0
-        exists, sz = s3CheckFileExists(self, bucket=self.bucket, client=self.client)
+        exists, sz = s3CheckFileExists(self, bucket=self._bucket, client=self.client)
         if not exists:
             raise FileNotFoundError(f"Resource {self} does not exist")
         return sz
@@ -254,7 +264,7 @@ def remove(self) -> None:
         # for checking all the keys again, reponse is  HTTP 204 OK
         # response all the time
         try:
-            self.client.delete_object(Bucket=self.bucket, Key=self.relativeToPathRoot)
+            self.client.delete_object(Bucket=self._bucket, Key=self.relativeToPathRoot)
         except (self.client.exceptions.NoSuchKey, self.client.exceptions.NoSuchBucket) as err:
             raise FileNotFoundError("No such resource: {self}") from err
 
@@ -264,7 +274,7 @@ def read(self, size: int = -1) -> bytes:
         if size > 0:
             args["Range"] = f"bytes=0-{size-1}"
         try:
-            response = self.client.get_object(Bucket=self.bucket, Key=self.relativeToPathRoot, **args)
+            response = self.client.get_object(Bucket=self._bucket, Key=self.relativeToPathRoot, **args)
         except (self.client.exceptions.NoSuchKey, self.client.exceptions.NoSuchBucket) as err:
             raise FileNotFoundError(f"No such resource: {self}") from err
         except ClientError as err:
@@ -280,20 +290,20 @@ def write(self, data: bytes, overwrite: bool = True) -> None:
         if not overwrite and self.exists():
             raise FileExistsError(f"Remote resource {self} exists and overwrite has been disabled")
         with time_this(log, msg="Write to %s", args=(self,)):
-            self.client.put_object(Bucket=self.bucket, Key=self.relativeToPathRoot, Body=data)
+            self.client.put_object(Bucket=self._bucket, Key=self.relativeToPathRoot, Body=data)
 
     @backoff.on_exception(backoff.expo, all_retryable_errors, max_time=max_retry_time)
     def mkdir(self) -> None:
         """Write a directory key to S3."""
-        if not bucketExists(self.bucket, self.client):
-            raise ValueError(f"Bucket {self.bucket} does not exist for {self}!")
+        if not bucketExists(self._bucket, self.client):
+            raise ValueError(f"Bucket {self._bucket} does not exist for {self}!")
 
         if not self.dirLike:
             raise NotADirectoryError(f"Can not create a 'directory' for file-like URI {self}")
 
         # don't create S3 key when root is at the top-level of an Bucket
         if self.path != "/":
-            self.client.put_object(Bucket=self.bucket, Key=self.relativeToPathRoot)
+            self.client.put_object(Bucket=self._bucket, Key=self.relativeToPathRoot)
 
     @backoff.on_exception(backoff.expo, all_retryable_errors, max_time=max_retry_time)
     def _download_file(self, local_file: IO, progress: ProgressPercentage | None) -> None:
@@ -304,7 +314,7 @@ def _download_file(self, local_file: IO, progress: ProgressPercentage | None) ->
         """
         try:
             self.client.download_fileobj(
-                self.bucket,
+                self._bucket,
                 self.relativeToPathRoot,
                 local_file,
                 Callback=progress,
@@ -349,7 +359,7 @@ def _upload_file(self, local_file: ResourcePath, progress: ProgressPercentage |
         """
         try:
             self.client.upload_file(
-                local_file.ospath, self.bucket, self.relativeToPathRoot, Callback=progress
+                local_file.ospath, self._bucket, self.relativeToPathRoot, Callback=progress
             )
         except self.client.exceptions.NoSuchBucket as err:
             raise NotADirectoryError(f"Target does not exist: {err}") from err
@@ -360,11 +370,11 @@ def _upload_file(self, local_file: ResourcePath, progress: ProgressPercentage |
     @backoff.on_exception(backoff.expo, all_retryable_errors, max_time=max_retry_time)
     def _copy_from(self, src: S3ResourcePath) -> None:
         copy_source = {
-            "Bucket": src.bucket,
+            "Bucket": src._bucket,
             "Key": src.relativeToPathRoot,
         }
         try:
-            self.client.copy_object(CopySource=copy_source, Bucket=self.bucket, Key=self.relativeToPathRoot)
+            self.client.copy_object(CopySource=copy_source, Bucket=self._bucket, Key=self.relativeToPathRoot)
         except (self.client.exceptions.NoSuchKey, self.client.exceptions.NoSuchBucket) as err:
             raise FileNotFoundError("No such resource to transfer: {self}") from err
         except ClientError as err:
@@ -494,7 +504,7 @@ def walk(
         filenames = []
         files_there = False
 
-        for page in s3_paginator.paginate(Bucket=self.bucket, Prefix=prefix, Delimiter="/"):
+        for page in s3_paginator.paginate(Bucket=self._bucket, Prefix=prefix, Delimiter="/"):
             # All results are returned as full key names and we must
             # convert them back to the root form. The prefix is fixed
             # and delimited so that is a simple trim
@@ -532,7 +542,7 @@ def _openImpl(
         *,
         encoding: str | None = None,
     ) -> Iterator[ResourceHandleProtocol]:
-        with S3ResourceHandle(mode, log, self.client, self.bucket, self.relativeToPathRoot) as handle:
+        with S3ResourceHandle(mode, log, self.client, self._bucket, self.relativeToPathRoot) as handle:
             if "b" in mode:
                 yield handle
             else:
@@ -554,6 +564,6 @@ def generate_presigned_put_url(self, *, expiration_time_seconds: int) -> str:
     def _generate_presigned_url(self, method: str, expiration_time_seconds: int) -> str:
         return self.client.generate_presigned_url(
             method,
-            Params={"Bucket": self.bucket, "Key": self.relativeToPathRoot},
+            Params={"Bucket": self._bucket, "Key": self.relativeToPathRoot},
             ExpiresIn=expiration_time_seconds,
         )

tests/test_s3.py

@@ -272,20 +272,20 @@ def test_s3_endpoint_url(self):
 
     def test_uri_syntax(self):
         path1 = ResourcePath("s3://profile@bucket/path")
-        self.assertEqual(path1.bucket, "bucket")
-        self.assertEqual(path1.profile, "profile")
+        self.assertEqual(path1._bucket, "bucket")
+        self.assertEqual(path1._profile, "profile")
         path2 = ResourcePath("s3://bucket2/path")
-        self.assertEqual(path2.bucket, "bucket2")
-        self.assertIsNone(path2.profile)
+        self.assertEqual(path2._bucket, "bucket2")
+        self.assertIsNone(path2._profile)
 
     def test_ceph_uri_syntax(self):
         # The Ceph S3 'multi-tenant' syntax for buckets can include colons.
         path1 = ResourcePath("s3://profile@ceph:bucket/path")
-        self.assertEqual(path1.bucket, "ceph:bucket")
-        self.assertEqual(path1.profile, "profile")
+        self.assertEqual(path1._bucket, "ceph:bucket")
+        self.assertEqual(path1._profile, "profile")
         path2 = ResourcePath("s3://ceph:bucket2/path")
-        self.assertEqual(path2.bucket, "ceph:bucket2")
-        self.assertIsNone(path2.profile)
+        self.assertEqual(path2._bucket, "ceph:bucket2")
+        self.assertIsNone(path2._profile)
 
 
 if __name__ == "__main__":