From bcc9846f0e6043a05d1370c91b908ff36b0521cb Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Tue, 11 Aug 2026 11:07:44 -0500 Subject: [PATCH] fix(#1537): allow s3 stores without static credentials s3 was the only storage protocol that mandated access_key/secret_key; gcs and azure already fall through to their default credential chains. Make the two optional so an ambient AWS identity (instance profile, IRSA, ECS task role, SSO) is used when no static keys are configured. - settings.py: drop access_key/secret_key from required_keys["s3"]. - storage.py::_validate_spec: drop them from required; reject exactly one of the pair (botocore would otherwise fail late with PartialCredentialsError). - storage.py::_create_filesystem: self.spec["access_key"] -> .get(...) or None so a missing OR empty-string credential is dropped and botocore resolves the chain (a forwarded "" is read as an explicit, invalid credential). Both-present behavior is unchanged; backward compatible. Adds unit tests for the ambient, both-present, empty-string, and partial-credential cases. --- src/datajoint/settings.py | 2 +- src/datajoint/storage.py | 21 +++++++++-- tests/unit/test_settings.py | 31 ++++++++++++++++ tests/unit/test_storage_adapter.py | 58 ++++++++++++++++++++++++++++++ 4 files changed, 108 insertions(+), 4 deletions(-) diff --git a/src/datajoint/settings.py b/src/datajoint/settings.py index 9bcce0201..4d2e8bac5 100644 --- a/src/datajoint/settings.py +++ b/src/datajoint/settings.py @@ -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"), } diff --git a/src/datajoint/storage.py b/src/datajoint/storage.py index 4f91b3d33..51e1d80cd 100644 --- a/src/datajoint/storage.py +++ b/src/datajoint/storage.py @@ -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: @@ -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}, ) diff --git a/tests/unit/test_settings.py b/tests/unit/test_settings.py index 3606c83c2..22de19d14 100644 --- a/tests/unit/test_settings.py +++ b/tests/unit/test_settings.py @@ -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() diff --git a/tests/unit/test_storage_adapter.py b/tests/unit/test_storage_adapter.py index b04c4903c..a60a58d9a 100644 --- a/tests/unit/test_storage_adapter.py +++ b/tests/unit/test_storage_adapter.py @@ -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()