UN-3358 [FIX] Drop cross-region S3 buckets from connector listing#1931
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughReclassifies two S3 error codes ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
| Filename | Overview |
|---|---|
| unstract/connectors/src/unstract/connectors/filesystems/minio/exceptions.py | Reclassifies PermanentRedirect and IllegalLocationConstraintException from FAIL_OPEN → DROP; updates comments accordingly. Change is correct and well-documented. |
| unstract/connectors/tests/filesystems/test_miniofs.py | Flips the existing PermanentRedirect test assertion and adds a companion IllegalLocationConstraintException DROP test; both tests are correctly structured. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[_lsbuckets called] --> B[S3FileSystem._lsbuckets\nreturns raw bucket list]
B --> C{For each bucket:\n_is_bucket_accessible?}
C --> D[_call_s3 list_objects_v2 probe]
D --> E{Success?}
E -- Yes --> F[KEEP bucket]
E -- No --> G{s3_error_code lookup\nin BUCKET_PROBE_DISPOSITION}
G -- AccessDenied / AllAccessDisabled / NoSuchBucket / PermanentRedirect / IllegalLocationConstraintException --> H[DROP bucket]
G -- SlowDown / Throttling / ThrottlingException --> I[RETRY_FAIL_OPEN: Retry once then keep]
G -- FAIL_OPEN reserved extension point --> J[KEEP bucket]
G -- Unlisted code --> K[Propagate exception]
style H fill:#f96,stroke:#c00
style F fill:#6c6,stroke:#060
style I fill:#fc9,stroke:#a60
style K fill:#f66,stroke:#900
Reviews (2): Last reviewed commit: "greptile review" | Re-trigger Greptile
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
unstract/connectors/tests/filesystems/test_miniofs.py (1)
107-117: Good expectation flip; add coverage for the second reclassified code too.
PermanentRedirectis now covered correctly. Please add a sibling case forIllegalLocationConstraintExceptionso both newly dropped codes are regression-guarded.✅ Suggested test addition
+ def test_illegal_location_constraint_bucket_is_dropped(self) -> None: + fs = self._make_fs() + with patch.object( + fs, + "_call_s3", + new=AsyncMock( + side_effect=_translated_error("IllegalLocationConstraintException") + ), + ): + self.assertFalse(asyncio.run(fs._is_bucket_accessible("wrong-region")))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@unstract/connectors/tests/filesystems/test_miniofs.py` around lines 107 - 117, Add a sibling test alongside test_permanent_redirect_bucket_is_dropped that verifies IllegalLocationConstraintException is also treated as a dropped bucket: in the same test module create another case (e.g., test_illegal_location_constraint_bucket_is_dropped) that constructs fs via _make_fs(), patches fs._call_s3 with AsyncMock(side_effect=_translated_error("IllegalLocationConstraintException")), and asserts asyncio.run(fs._is_bucket_accessible("other-region")) is False; reference the existing test_permanent_redirect_bucket_is_dropped, _call_s3, and _is_bucket_accessible to mirror the structure exactly so both reclassified error codes are regression-guarded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@unstract/connectors/tests/filesystems/test_miniofs.py`:
- Around line 107-117: Add a sibling test alongside
test_permanent_redirect_bucket_is_dropped that verifies
IllegalLocationConstraintException is also treated as a dropped bucket: in the
same test module create another case (e.g.,
test_illegal_location_constraint_bucket_is_dropped) that constructs fs via
_make_fs(), patches fs._call_s3 with
AsyncMock(side_effect=_translated_error("IllegalLocationConstraintException")),
and asserts asyncio.run(fs._is_bucket_accessible("other-region")) is False;
reference the existing test_permanent_redirect_bucket_is_dropped, _call_s3, and
_is_bucket_accessible to mirror the structure exactly so both reclassified error
codes are regression-guarded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2bf4069c-15d6-4eee-bace-252c264a596b
📒 Files selected for processing (2)
unstract/connectors/src/unstract/connectors/filesystems/minio/exceptions.pyunstract/connectors/tests/filesystems/test_miniofs.py
Contributor
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
|
chandrasekharan-zipstack
approved these changes
Apr 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
PermanentRedirectandIllegalLocationConstraintExceptionfromFAIL_OPENtoDROPinBUCKET_PROBE_DISPOSITION(unstract/connectors/.../filesystems/minio/exceptions.py).BucketProbeDisposition.FAIL_OPENdocstring; the enum value itself is retained as a future extension point.test_permanent_redirect_bucket_is_kept→test_permanent_redirect_bucket_is_dropped.Why
After #1925, staging AWS-S3 still listed buckets the connector could not actually browse. Any bucket in a region different from the configured
endpoint_urlreturnedPermanentRedirecton the per-bucket probe, which the disposition table treated asFAIL_OPENand kept visible. Clicking such a bucket then surfaced[Errno 78] The bucket you are attempting to access must be addressed using the specified endpoint. Local MinIO never reproduced this because it is single-region; only AWS-S3 with cross-region buckets in the account exercises the path.MinioFSis pinned to a singleendpoint_urland s3fs does not auto-redirect across regions, so cross-region buckets are structurally unreachable through this connector regardless of IAM — dropping them is strictly better UX than leaving the user a broken click target. The same conclusion applies under IRSA: credential source is orthogonal to region routing, so this fix covers both auth modes.How
The probe disposition table is the single point of control for
_AccessFilteredS3FileSystem._is_bucket_accessible. Switching the two region-routing codes fromFAIL_OPENtoDROPmakes the filter hide cross-region buckets at listing time, so the failing click path is unreachable.RETRY_FAIL_OPEN(throttling),AccessDenied,NoSuchBucket, and unclassified-code propagation are untouched.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
No. The change is scoped to two S3 error codes inside the bucket-listing filter. Per-bucket browsing, write paths, credential-test path, and the other dispositions are unchanged. The only visible behavior change is that buckets returning
PermanentRedirect/IllegalLocationConstraintExceptionno longer appear in the connector root — which matches the documented expectation inprompting/iam-s3-bucket/list-bucket-user/staging-test-setup.md.Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
UN-3358 [FIX] List only S3 buckets the connector credentials can browse).Dependencies Versions
Notes on Testing
cd unstract/connectors && .venv/bin/python -m pytest tests/filesystems/test_miniofs.py::TestAccessFilteredS3FileSystem -v— 13/13 pass, including the renamedtest_permanent_redirect_bucket_is_dropped.prompting/iam-s3-bucket/list-bucket-user/test-setup.md): denied bucket still hidden, allowed bucket still browsable — theAccessDeniedpath is untouched.prompting/iam-s3-bucket/list-bucket-user/staging-test-setup.mdstep 5a): onlyunstract-staging-iam-bucketshould appear in the connector root; the previously-leaking cross-region buckets are gone, and the[Errno 78]reproduction is no longer reachable from the UI.Screenshots
Checklist
I have read and understood the Contribution Guidelines.