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()