s3 public buckets#144
Conversation
Greptile SummaryThis PR adds support for anonymous access to public S3 buckets via a new Confidence Score: 5/5Safe to merge; only a stale docstring warrants attention before final polish. All P-findings are P2 (docstring quality). The core logic — value-based env-var check, UNSIGNED signature config, and session short-circuit — is correct and well-tested. credentials.py docstring should be updated to reflect both the new UNSIGNED path and the correct RUNAI_STREAMER_NO_BOTO3_SESSION semantics. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[_build_s3_client called] --> B[get_credentials]
B --> C{RUNAI_STREAMER_S3_UNSIGNED == 1?}
C -- Yes --> D[Resolve CA bundle if needed\nset AWS_CA_BUNDLE]
D --> E[Return session=None]
C -- No --> F{RUNAI_STREAMER_NO_BOTO3_SESSION set?}
F -- Yes --> E
F -- No --> G[Create boto3.Session\nResolve & freeze credentials]
G --> H[Return session]
E --> I[_build_client_config]
H --> I
I --> J{UNSIGNED flag?}
J -- Yes --> K[Config with signature_version=UNSIGNED]
J -- No --> L{Path addressing?}
L -- Yes --> M[Config with addressing_style=path]
L -- No --> N[Config=None]
K --> O{session is None?}
M --> O
N --> O
O -- Yes --> P[boto3.client s3 with config]
O -- No --> Q[session.client s3 with config]
Reviews (7): Last reviewed commit: "credentials test" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def test_list_safetensors_unsigned(self): | ||
| with patch.dict(os.environ, {RUNAI_STREAMER_S3_UNSIGNED_ENV_VAR: "1"}): | ||
| result = list_safetensors(f"s3://{self.PUBLIC_BUCKET}/") | ||
| self.assertIn(f"s3://{self.PUBLIC_BUCKET}/model.safetensors", result) |
There was a problem hiding this comment.
RUNAI_STREAMER_S3_UNSIGNED=0 (opt-out case)
The test suite verifies the "1" (opt-in) path but never asserts that setting the variable to "0" keeps authenticated mode active. Given the presence-check bug above, a regression test like this would have caught it at review time. Consider adding a small test that sets the env var to "0" and confirms credentials are still used (or at minimum confirms listing a private bucket still works).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors S3 client construction into shared helpers, adds unsigned (anonymous) S3 access controlled by Changes
Sequence Diagram(s)sequenceDiagram
participant Streamer as Streamer
participant Builder as S3 Client Builder
participant Creds as Credential Provider
participant S3 as S3 / MinIO
Streamer->>Builder: request S3 client (glob / pull / stream)
alt RUNAI_STREAMER_S3_UNSIGNED = "1"
Builder->>Builder: set signature_version = botocore.UNSIGNED
Builder->>S3: create anonymous boto3 client
else signed (default)
Builder->>Creds: get_credentials()
Creds-->>Builder: session + credentials
Builder->>S3: create boto3 client with credentials
end
Streamer->>S3: list / get / stream object requests
S3-->>Streamer: object listings / file bytes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/s3/test_s3.py (1)
65-107: Isolate the public-bucket fixture per test class.Using a fixed bucket name and leaving the remote bucket/policy in place makes this integration test stateful across reruns and parallel workers. Please generate a unique bucket name per run and delete the uploaded object/bucket in
tearDownClass()so failures aren't masked by leftover state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/s3/test_s3.py` around lines 65 - 107, The test class TestS3UnsignedPublicBucket currently uses a constant PUBLIC_BUCKET and leaves objects/policies behind; change setUpClass to generate a unique bucket name (e.g., PUBLIC_BUCKET = f"public-test-bucket-{uuid4().hex}") before calling s3_admin.create_bucket and use that name when uploading the file via server.upload_file and create_random_safetensors, and update tearDownClass to remove the uploaded object, delete the bucket policy and delete the bucket (and cleanup temp_dir) using the same s3_admin client so each run is isolated and no leftover state masks failures; keep references to PUBLIC_BUCKET, setUpClass, tearDownClass, create_random_safetensors and server.upload_file to locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@py/runai_model_streamer_s3/runai_model_streamer_s3/files/files.py`:
- Around line 13-27: The code treats RUNAI_STREAMER_S3_UNSIGNED as enabled if
the env var exists; change both checks to parse the variable value (e.g.,
os.getenv(RUNAI_STREAMER_S3_UNSIGNED_ENV_VAR, "0") == "1") so only an explicit
"1" enables unsigned mode: update _build_client_config to set
config_kwargs["signature_version"] = UNSIGNED only when the env var value equals
"1", and update _build_s3_client to set unsigned =
(os.getenv(RUNAI_STREAMER_S3_UNSIGNED_ENV_VAR, "0") == "1") so session selection
uses the parsed value; also add a regression test asserting that
RUNAI_STREAMER_S3_UNSIGNED="0" results in signed client behavior (uses
get_credentials/session).
---
Nitpick comments:
In `@tests/s3/test_s3.py`:
- Around line 65-107: The test class TestS3UnsignedPublicBucket currently uses a
constant PUBLIC_BUCKET and leaves objects/policies behind; change setUpClass to
generate a unique bucket name (e.g., PUBLIC_BUCKET =
f"public-test-bucket-{uuid4().hex}") before calling s3_admin.create_bucket and
use that name when uploading the file via server.upload_file and
create_random_safetensors, and update tearDownClass to remove the uploaded
object, delete the bucket policy and delete the bucket (and cleanup temp_dir)
using the same s3_admin client so each run is isolated and no leftover state
masks failures; keep references to PUBLIC_BUCKET, setUpClass, tearDownClass,
create_random_safetensors and server.upload_file to locate the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f52a1463-6e6f-4cc3-850e-f26e9345d807
📒 Files selected for processing (4)
docs/src/env-vars.mddocs/src/usage.mdpy/runai_model_streamer_s3/runai_model_streamer_s3/files/files.pytests/s3/test_s3.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@py/runai_model_streamer_s3/runai_model_streamer_s3/files/files.py`:
- Around line 21-27: The _build_s3_client function skips calling get_credentials
when unsigned mode is true, which prevents credentials.py from applying the CA
bundle (AWS_CA_BUNDLE) needed for TLS to S3-compatible endpoints; always invoke
get_credentials(credentials) to allow credentials.py to set up the CA bundle and
then decide whether to use the returned session for client creation based on
unsigned: call get_credentials(...) unconditionally (to perform CA bundle
setup), assign its first return value to a session variable, and then if
unsigned is true ignore that session and return boto3.client("s3",
config=client_config) or otherwise use session.client("s3",
config=client_config); keep _build_client_config() usage unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 7a549731-5347-487d-8bdb-9d486b0e7ee8
📒 Files selected for processing (1)
py/runai_model_streamer_s3/runai_model_streamer_s3/files/files.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
py/runai_model_streamer_s3/runai_model_streamer_s3/files/tests/test_files.py (1)
67-97: Strengthen_build_s3_clienttests with argument-level assertions.
On Line 76, Line 87, and Line 96 these tests currently validate control flow by call counts only. Adding a couple of call-arg assertions will better catch regressions in wiring (profile, service name, and unsigned config usage).Suggested test hardening
@@ def test_credentials_used_when_unsigned_disabled(self, mock_get_credentials, mock_boto3): mock_session = MagicMock() mock_get_credentials.return_value = (mock_session, MagicMock()) with patch.dict(os.environ, {files.RUNAI_STREAMER_S3_UNSIGNED_ENV_VAR: "0"}): files._build_s3_client(None) - mock_get_credentials.assert_called_once() + mock_get_credentials.assert_called_once_with(None) mock_session.client.assert_called_once() mock_boto3.client.assert_not_called() @@ def test_credentials_used_when_unsigned_absent(self, mock_get_credentials, mock_boto3): mock_session = MagicMock() mock_get_credentials.return_value = (mock_session, MagicMock()) with patch.dict(os.environ, _env_without_unsigned(), clear=True): files._build_s3_client(None) - mock_get_credentials.assert_called_once() + mock_get_credentials.assert_called_once_with(None) mock_session.client.assert_called_once() mock_boto3.client.assert_not_called() @@ def test_credentials_not_used_when_unsigned_enabled(self, mock_get_credentials, mock_boto3): with patch.dict(os.environ, {files.RUNAI_STREAMER_S3_UNSIGNED_ENV_VAR: "1"}): files._build_s3_client(None) mock_get_credentials.assert_not_called() mock_boto3.client.assert_called_once() + args, kwargs = mock_boto3.client.call_args + service_name = kwargs.get("service_name", args[0] if args else None) + self.assertEqual(service_name, "s3") + self.assertEqual(kwargs["config"].signature_version, UNSIGNED)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@py/runai_model_streamer_s3/runai_model_streamer_s3/files/tests/test_files.py` around lines 67 - 97, The tests in TestBuildS3Client only assert call counts; strengthen them by asserting call arguments so wiring regressions are caught: in each test that calls files._build_s3_client(pass_profile) assert that the session-based client (mock_session.client) was called with service name "s3" and the provided profile/parameters, and in the unsigned-enabled test assert boto3.client was called with service name "s3" and the unsigned config (signature version disabled) instead of only checking call counts; also assert get_credentials was invoked with the expected profile when credentials are expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@py/runai_model_streamer_s3/runai_model_streamer_s3/files/tests/test_files.py`:
- Around line 67-97: The tests in TestBuildS3Client only assert call counts;
strengthen them by asserting call arguments so wiring regressions are caught: in
each test that calls files._build_s3_client(pass_profile) assert that the
session-based client (mock_session.client) was called with service name "s3" and
the provided profile/parameters, and in the unsigned-enabled test assert
boto3.client was called with service name "s3" and the unsigned config
(signature version disabled) instead of only checking call counts; also assert
get_credentials was invoked with the expected profile when credentials are
expected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 86a09f0e-cebe-4eeb-ae77-1ea7e5642df0
📒 Files selected for processing (1)
py/runai_model_streamer_s3/runai_model_streamer_s3/files/tests/test_files.py
| def _build_s3_client(credentials: Optional[S3Credentials]): | ||
| session, _ = get_credentials(credentials) | ||
| client_config = _build_client_config() | ||
| if session is None: | ||
| s3 = boto3.client("s3", config=client_config) | ||
| else: | ||
| s3 = session.client("s3", config=client_config) | ||
|
|
||
| return boto3.client("s3", config=client_config) | ||
| return session.client("s3", config=client_config) |
There was a problem hiding this comment.
get_credentials called unconditionally in unsigned mode
_build_s3_client always calls get_credentials on line 22 before checking whether unsigned mode is active. In environments where no credentials are configured (the primary use-case for public-bucket access), get_credentials may raise or return unexpected values, defeating the purpose of the unsigned flag. The bundled test test_credentials_not_used_when_unsigned_enabled directly asserts mock_get_credentials.assert_not_called() when RUNAI_STREAMER_S3_UNSIGNED=1 — this assertion will fail with the current implementation, as will mock_boto3.client.assert_called_once() (the session returned by the unchecked get_credentials call causes session.client() to be used instead).
The fix is to short-circuit before credential resolution when unsigned mode is on:
def _build_s3_client(credentials: Optional[S3Credentials]):
client_config = _build_client_config()
if os.getenv(RUNAI_STREAMER_S3_UNSIGNED_ENV_VAR, "0") == "1":
return boto3.client("s3", config=client_config)
session, _ = get_credentials(credentials)
if session is None:
return boto3.client("s3", config=client_config)
return session.client("s3", config=client_config)There was a problem hiding this comment.
get_credentials checks the unsigmned flag internally, and if set ignores credentials resolution
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
py/runai_model_streamer_s3/runai_model_streamer_s3/credentials/credentials.py (1)
24-40:⚠️ Potential issue | 🟡 MinorUpdate return type annotation to reflect that session can be None.
The function returns
Nonefor the session on lines 40 and 43 (unsigned andRUNAI_STREAMER_NO_BOTO3_SESSIONmodes), but the type annotation on line 24 declaresTuple[boto3.Session, S3Credentials]withoutOptional. The docstring already documents this behavior, so update the annotation to match:Proposed fix
-def get_credentials(credentials: Optional[S3Credentials] = None) -> Tuple[boto3.Session, S3Credentials]: +def get_credentials(credentials: Optional[S3Credentials] = None) -> Tuple[Optional[boto3.Session], S3Credentials]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@py/runai_model_streamer_s3/runai_model_streamer_s3/credentials/credentials.py` around lines 24 - 40, The return type of get_credentials is inaccurate because the function can return None for the boto3 session; update its annotation from Tuple[boto3.Session, S3Credentials] to Tuple[Optional[boto3.Session], S3Credentials] (ensure Optional is imported from typing if not already) so the signature reflects that get_credentials may return None for the session when the unsigned/NO_BOTO3_SESSION path (e.g., RUNAI_STREAMER_S3_UNSIGNED_ENV_VAR logic) is taken.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@py/runai_model_streamer_s3/runai_model_streamer_s3/credentials/credentials.py`:
- Around line 24-40: The return type of get_credentials is inaccurate because
the function can return None for the boto3 session; update its annotation from
Tuple[boto3.Session, S3Credentials] to Tuple[Optional[boto3.Session],
S3Credentials] (ensure Optional is imported from typing if not already) so the
signature reflects that get_credentials may return None for the session when the
unsigned/NO_BOTO3_SESSION path (e.g., RUNAI_STREAMER_S3_UNSIGNED_ENV_VAR logic)
is taken.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 86284989-bd35-485b-b37a-5b2cf679c18a
📒 Files selected for processing (3)
py/runai_model_streamer_s3/runai_model_streamer_s3/credentials/credentials.pypy/runai_model_streamer_s3/runai_model_streamer_s3/files/files.pypy/runai_model_streamer_s3/runai_model_streamer_s3/files/tests/test_files.py
noa-neria
force-pushed
the
boto3-unsigned
branch
from
32073d0 to
8caa8fb
Compare
2 participants
Add support of s3 public buckets as requested in #142
Add environment variable flag
RUNAI_STREAMER_S3_UNSIGNEDwith zero default valueUsers should pass
RUNAI_STREAMER_S3_UNSIGNED=1for public bucketsSummary by CodeRabbit
New Features
Documentation
Refactor
Credentials
Tests