Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/datajoint/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,7 @@ def get_store_spec(self, store: str | None = None, *, use_filepath_default: bool
# Define required and allowed keys by protocol
required_keys: dict[str, tuple[str, ...]] = {
"file": ("protocol", "location"),
"s3": ("protocol", "endpoint", "bucket", "access_key", "secret_key", "location"),
"s3": ("protocol", "endpoint", "bucket", "location"),
"gcs": ("protocol", "bucket", "location"),
"azure": ("protocol", "container", "location"),
}
Expand Down
21 changes: 18 additions & 3 deletions src/datajoint/storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -340,10 +340,21 @@ def _validate_spec(self):
if location and not Path(location).is_dir():
raise FileNotFoundError(f"Inaccessible local directory {location}")
elif self.protocol == "s3":
required = ["endpoint", "bucket", "access_key", "secret_key"]
required = ["endpoint", "bucket"]
missing = [k for k in required if not self.spec.get(k)]
if missing:
raise errors.DataJointError(f"Missing S3 configuration: {', '.join(missing)}")
# access_key/secret_key are optional: when both are absent the
# underlying botocore credential chain resolves an ambient identity
# (instance profile, IRSA, ECS task role, SSO), matching gcs/azure.
# But botocore treats exactly one as a partial credential and fails
# late (PartialCredentialsError at first access), so reject that here
# with a clear message.
if bool(self.spec.get("access_key")) != bool(self.spec.get("secret_key")):
raise errors.DataJointError(
"Incomplete S3 credentials: set both access_key and secret_key, "
"or neither to use ambient AWS credentials."
)

@property
def fs(self) -> fsspec.AbstractFileSystem:
Expand Down Expand Up @@ -376,10 +387,14 @@ def _create_filesystem(self) -> fsspec.AbstractFileSystem:
else:
endpoint_url = endpoint

# Coerce falsy (missing or empty-string) credentials to None so s3fs
# drops them and botocore falls through to the default chain. A
# forwarded "" is NOT equivalent: it survives s3fs's None-filter and
# botocore reads it as an explicit (invalid) credential.
return fsspec.filesystem(
"s3",
key=self.spec["access_key"],
secret=self.spec["secret_key"],
key=self.spec.get("access_key") or None,
secret=self.spec.get("secret_key") or None,
client_kwargs={"endpoint_url": endpoint_url},
)

Expand Down
31 changes: 31 additions & 0 deletions tests/unit/test_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,37 @@ def test_get_store_spec_file_protocol(self):
finally:
dj.config.stores = original_stores

def test_get_store_spec_s3_without_credentials(self):
"""s3 no longer requires access_key/secret_key (#1537) — matches gcs/azure."""
original_stores = dj.config.stores.copy()
try:
dj.config.stores["test_s3"] = {
"protocol": "s3",
"endpoint": "s3.amazonaws.com",
"bucket": "my-bucket",
"location": "prefix",
}
spec = dj.config.get_store_spec("test_s3")
assert spec["protocol"] == "s3"
assert "access_key" not in spec and "secret_key" not in spec
finally:
dj.config.stores = original_stores

def test_get_store_spec_s3_missing_bucket(self):
"""endpoint/bucket/location stay required for s3."""
original_stores = dj.config.stores.copy()
try:
dj.config.stores["bad_s3"] = {
"protocol": "s3",
"endpoint": "s3.amazonaws.com",
"location": "prefix",
# missing bucket
}
with pytest.raises(DataJointError, match="missing"):
dj.config.get_store_spec("bad_s3")
finally:
dj.config.stores = original_stores

def test_get_store_spec_missing_required(self):
"""Test missing required keys raises error."""
original_stores = dj.config.stores.copy()
Expand Down
58 changes: 58 additions & 0 deletions tests/unit/test_storage_adapter.py
Original file line number Diff line number Diff line change
Expand Up @@ -336,3 +336,61 @@ def _fake_entry_points(*, group=None):
assert adapter is not None
assert sa_mod.get_storage_adapter("bad") is None
assert any("bad" in rec.message and "boom" in rec.message for rec in caplog.records)


class TestS3AmbientCredentials:
"""s3 stores may omit access_key/secret_key and fall through to the
botocore credential chain, matching gcs/azure (#1537)."""

@staticmethod
def _backend(spec):
backend = StorageBackend.__new__(StorageBackend)
backend.spec = {"protocol": "s3", "endpoint": "s3.amazonaws.com", "bucket": "b", **spec}
backend.protocol = "s3"
backend._fs = None
return backend

def _captured_kwargs(self, monkeypatch, spec):
captured = {}

def fake_filesystem(protocol, **kwargs):
captured["protocol"] = protocol
captured.update(kwargs)
return object()

monkeypatch.setattr(storage.fsspec, "filesystem", fake_filesystem)
self._backend(spec)._create_filesystem()
return captured

def test_no_credentials_validates(self):
# both absent is valid — ambient identity resolves downstream
self._backend({})._validate_spec()

def test_no_credentials_forwards_none(self, monkeypatch):
kw = self._captured_kwargs(monkeypatch, {})
assert kw["key"] is None and kw["secret"] is None

def test_both_credentials_forwarded(self, monkeypatch):
kw = self._captured_kwargs(monkeypatch, {"access_key": "AK", "secret_key": "SK"})
assert kw["key"] == "AK" and kw["secret"] == "SK"

def test_empty_string_treated_as_absent(self, monkeypatch):
# "" survives s3fs's None-filter and botocore reads it as an explicit
# (invalid) credential, so it must be coerced to None
self._backend({"access_key": "", "secret_key": ""})._validate_spec()
kw = self._captured_kwargs(monkeypatch, {"access_key": "", "secret_key": ""})
assert kw["key"] is None and kw["secret"] is None

def test_partial_credentials_rejected(self):
with pytest.raises(DataJointError, match="Incomplete S3 credentials"):
self._backend({"access_key": "AK"})._validate_spec()
with pytest.raises(DataJointError, match="Incomplete S3 credentials"):
self._backend({"secret_key": "SK"})._validate_spec()

def test_missing_endpoint_or_bucket_still_required(self):
backend = StorageBackend.__new__(StorageBackend)
backend.spec = {"protocol": "s3", "bucket": "b"} # no endpoint
backend.protocol = "s3"
backend._fs = None
with pytest.raises(DataJointError, match="Missing S3 configuration"):
backend._validate_spec()
Loading