fix(#1537): allow s3 stores without static credentials - #1538
Open
dimitri-yatsenko wants to merge 1 commit into
Open
fix(#1537): allow s3 stores without static credentials#1538dimitri-yatsenko wants to merge 1 commit into
dimitri-yatsenko wants to merge 1 commit into
Conversation
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.
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
1 participant
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.
Closes #1537.
Makes
access_key/secret_keyoptional forprotocol: s3, so an ambient AWS identity (EC2 instance profile, EKS/IRSA service-account role, ECS task role, or a workstation withAWS_PROFILE/SSO) is used when no static keys are configured. This bringss3in line withgcsandazure, which already fall through to their default credential chains — it removes an outlier rather than adding a mode.Changes
settings.py— dropaccess_key/secret_keyfromrequired_keys["s3"](endpoint/bucket/locationstay required).storage.py::_validate_spec— drop them fromrequired; additionally reject exactly one of the pair, which botocore would otherwise accept and then fail late on withPartialCredentialsErrorat first object access.storage.py::_create_filesystem—self.spec["access_key"]→self.spec.get("access_key") or None(same forsecret_key). This is the easy-to-miss subscript that would raiseKeyErrorafter validation passes. Theor Nonealso coerces an empty string to absent: a forwarded""survives s3fs'sNone-filter and botocore reads it as an explicit (invalid) credential.Compatibility
Both-present behavior is exactly as today. Absence is the same signal s3fs itself uses for "automatic credentials", so no new config key is introduced (which would break older clients against the strict
allowed_keyswhitelist).Tests
Unit tests for: ambient (no keys) validates and forwards neither kwarg; both-present forwarded unchanged; empty-string treated as absent; exactly-one rejected with a clear
DataJointError;endpoint/bucketstill required. Settings-level tests that the s3 spec no longer requires the credentials but still requiresbucket.