Merge pull request #128 from thomasmarwitz/remove-side-effects

.github/workflows/ci.yml

@@ -77,14 +77,6 @@ jobs:
           role-to-assume: arn:aws:iam::211125346859:role/github-poweruser
           aws-region: eu-central-1
 
-      # This is a work around as long as S3FSStore has the side effect of setting env variables
-      # to provide authentication. This can safely be removed as soon as this side effect is gone
-      - name: Remap AWS Environment Variables
-        if: steps.check-id-token.outcome == 'success'
-        run: |
-          echo "ACCESS_KEY_ID=${{ env.AWS_ACCESS_KEY_ID }}" >> $GITHUB_ENV
-          echo "SECRET_ACCESS_KEY=${{ env.AWS_SECRET_ACCESS_KEY }}" >> $GITHUB_ENV
-          echo "SESSION_TOKEN=${{ env.AWS_SESSION_TOKEN }}" >> $GITHUB_ENV
       # We set an env variable according to the result of the check
       # to only allow skipping of aws integration test when in fork.
       # When being run in the base repo, the aws integration test should always be executed.

docs/changes.rst

@@ -1,6 +1,11 @@
 Changelog
 *********
 
+1.10.0
+======
+* When using the S3FSStore, the environment variables 'AWS_ACCESS_KEY_ID' and 'AWS_SECRET_ACCESS_KEY' are no longer set automatically based on the values provided in the url to create the store. This was a side effect that has now been removed since the credentials are passed internally without the need of a detour through environment variables.
+
+
 1.9.2
 =====
 * Port setup to use the OSS QuantCo copier template (`copier template https://github.com/Quantco/copier-template-python-open-source`_) and (`pixi https://pixi.sh`_) as environment manager.

minimalkv/net/s3fsstore.py

@@ -184,13 +184,10 @@ def _from_parsed_url(
 
         if url_access_key_id is None:
             url_secret_access_key = os.environ.get("AWS_ACCESS_KEY_ID")
-        else:
-            os.environ["AWS_ACCESS_KEY_ID"] = url_access_key_id
+            # We allow attributes to be nonable, a potential use case might be a public bucket
 
         if url_secret_access_key is None:
             url_secret_access_key = os.environ.get("AWS_SECRET_ACCESS_KEY")
-        else:
-            os.environ["AWS_SECRET_ACCESS_KEY"] = url_secret_access_key
 
         credentials = Credentials(
             access_key_id=url_access_key_id,

tests/bucket_manager.py

@@ -49,13 +49,6 @@ def boto3_bucket_reference(
 
     The bucket is not created.
     """
-    import os
-
-    # Set environment variables for boto3
-    os.environ["AWS_ACCESS_KEY_ID"] = access_key
-    os.environ["AWS_SECRET_ACCESS_KEY"] = secret_key
-    os.environ["AWS_DEFAULT_REGION"] = "us-east-1"
-
     # Build endpoint host
     endpoint_url = None
     if host:

tests/test_s3fs_aws.py

@@ -26,10 +26,9 @@ def aws_credentials() -> tuple[str, str, Union[str, None]]:
         secret_key = aws_credentials.secret_key
         session_token = aws_credentials.token
     else:
-        # Don't look for AWS_ versions, they might be overwritten by test_boto3_store.py
-        access_key = os.environ.get("ACCESS_KEY_ID", None)
-        secret_key = os.environ.get("SECRET_ACCESS_KEY", None)
-        session_token = os.environ.get("SESSION_TOKEN", None)
+        access_key = os.environ.get("AWS_ACCESS_KEY_ID", None)
+        secret_key = os.environ.get("AWS_SECRET_ACCESS_KEY", None)
+        session_token = os.environ.get("AWS_SESSION_TOKEN", None)
 
     if not (access_key and secret_key):
         msg = "No s3 credentials available. "
@@ -45,8 +44,8 @@ def aws_credentials() -> tuple[str, str, Union[str, None]]:
                 msg += (
                     "If you want to execute this integration test, "
                     f"set '{env_var_name}' env variable to "
-                    "provide a valid AWS profile or set 'ACCESS_KEY_ID' and "
-                    "'SECRET_ACCESS_KEY' and optional 'SESSION_TOKEN'."
+                    "provide a valid AWS profile or set 'AWS_ACCESS_KEY_ID' and "
+                    "'AWS_SECRET_ACCESS_KEY' and optional 'AWS_SESSION_TOKEN'."
                 )
 
             pytest.skip(reason=msg)

tests/test_s3fs_minio.py

@@ -1,5 +1,8 @@
+import os
 from typing import NamedTuple
 
+import pytest
+
 from minimalkv import get_store_from_url
 
 
@@ -13,21 +16,23 @@ class User(NamedTuple):
     # user2 -> bucket2
     # Including one bucket_name per `User` here is just for readability
 
+    def get_store_from_config(self):
+        return get_store_from_url(
+            f"hs3://{self.access_key}:{self.secret_key}@localhost:9000/{self.bucket_name}?force_bucket_suffix=false&verify=false"
+        )
+
+
+user1 = User("user1", "password1", "bucket1")
+user2 = User("user2", "password2", "bucket2")
+
 
 def test_access_multiple_users():
     """Verify that two buckets of different users can be accessed in the same python process."""
-    user1 = User("user1", "password1", "bucket1")
-    user2 = User("user2", "password2", "bucket2")
-
-    bucket1 = get_store_from_url(
-        f"hs3://{user1.access_key}:{user1.secret_key}@localhost:9000/{user1.bucket_name}?force_bucket_suffix=false&verify=false"
-    )
+    bucket1 = user1.get_store_from_config()
 
     assert bucket1.keys() == ["file1.txt"]
 
-    bucket2 = get_store_from_url(
-        f"hs3://{user2.access_key}:{user2.secret_key}@localhost:9000/{user2.bucket_name}?force_bucket_suffix=false&verify=false"
-    )
+    bucket2 = user2.get_store_from_config()
 
     assert bucket2.keys() == ["file2.txt"]
 
@@ -41,11 +46,7 @@ def test_example_interaction():
     - get()
     - delete()
     """
-    user = User("user1", "password1", "bucket1")
-
-    bucket = get_store_from_url(
-        f"hs3://{user.access_key}:{user.secret_key}@localhost:9000/{user.bucket_name}?force_bucket_suffix=false&verify=false"
-    )
+    bucket = user1.get_store_from_config()
 
     new_filename = "some-non-existing-file"
     new_content = b"content"
@@ -58,3 +59,27 @@ def test_example_interaction():
 
     bucket.delete(new_filename)
     assert new_filename not in bucket.keys()
+
+
+@pytest.fixture
+def clean_env(monkeypatch):
+    # Important because another test sets the environment variables when accessing
+    # the bucket.
+    monkeypatch.setattr(os, "environ", {})
+    yield
+
+
+def test_no_env_side_effects():
+    pre_env_state = os.environ.copy()
+
+    bucket = user1.get_store_from_config()
+
+    assert dict(os.environ) == dict(
+        pre_env_state
+    ), "Retrieved bucket should not modify the environment."
+
+    bucket.keys()
+
+    assert (
+        os.environ == pre_env_state
+    ), "Performing operations on the bucket should not modify the environment."