Repository: peopledoc/vault-cli

Issue:
When used as lib, need to close the connection
We're getting this error:
```
ResourceWarning: unclosed <socket.socket fd=10, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=6, laddr=('x.x.x.x', 53920), raddr=('x.x.x.x', 8200)>
```
The session should be available as a context manager, to ensure it's properly closed.

Discussion:
codecov-io: # [Codecov](https://codecov.io/gh/peopledoc/vault-cli/pull/51?src=pr&el=h1) Report
> Merging [#51](https://codecov.io/gh/peopledoc/vault-cli/pull/51?src=pr&el=desc) into [master](https://codecov.io/gh/peopledoc/vault-cli/commit/9f29b0eb5ea00ffa0fdb429a0c7e793d2c6a45d9?src=pr&el=desc) will **decrease** coverage by `0.93%`.
> The diff coverage is `80.95%`.

[![Impacted file tree graph](https://codecov.io/gh/peopledoc/vault-cli/pull/51/graphs/tree.svg?width=650&token=fjUxXKxpOh&height=150&src=pr)](https://codecov.io/gh/peopledoc/vault-cli/pull/51?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
- Coverage     100%   99.06%   -0.94%     
==========================================
  Files           8        9       +1     
  Lines         416      426      +10     
  Branches       44       44              
==========================================
+ Hits          416      422       +6     
- Misses          0        4       +4
```

| Flag | Coverage Δ | |
|---|---|---|
| #integration | `66.9% <47.61%> (-0.65%)` | :arrow_down: |
| #unit | `99.06% <80.95%> (-0.94%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/peopledoc/vault-cli/pull/51?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [vault\_cli/sessions.py](https://codecov.io/gh/peopledoc/vault-cli/pull/51/diff?src=pr&el=tree#diff-dmF1bHRfY2xpL3Nlc3Npb25zLnB5) | `100% <100%> (ø)` | |
| [vault\_cli/client.py](https://codecov.io/gh/peopledoc/vault-cli/pull/51/diff?src=pr&el=tree#diff-dmF1bHRfY2xpL2NsaWVudC5weQ==) | `98.09% <50%> (-1.91%)` | :arrow_down: |
| [vault\_cli/requests.py](https://codecov.io/gh/peopledoc/vault-cli/pull/51/diff?src=pr&el=tree#diff-dmF1bHRfY2xpL3JlcXVlc3RzLnB5) | `98.3% <75%> (-1.7%)` | :arrow_down: |
| [vault\_cli/hvac.py](https://codecov.io/gh/peopledoc/vault-cli/pull/51/diff?src=pr&el=tree#diff-dmF1bHRfY2xpL2h2YWMucHk=) | `96.42% <80%> (-3.58%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/peopledoc/vault-cli/pull/51?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/peopledoc/vault-cli/pull/51?src=pr&el=footer). Last update [9f29b0e...b8c08a3](https://codecov.io/gh/peopledoc/vault-cli/pull/51?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/vault_cli/client.py b/vault_cli/client.py
index 9d142e5..28cfa2a 100644
--- a/vault_cli/client.py
+++ b/vault_cli/client.py
@@ -155,6 +155,16 @@ class VaultClientBase:
         else:
             raise ValueError("No authentication method supplied")
 
+    def __enter__(self):
+        return self
+
+    def __exit__(self, exc_type, exc_value, traceback):
+        """
+        Implement this with the relevant behaviour in children classes
+        for when exiting the client used as context manager.
+        """
+        pass
+
     def _browse_recursive_secrets(self, path: str) -> Iterable[str]:
         """
         Given a secret or folder path, return the path of all secrets
diff --git a/vault_cli/hvac.py b/vault_cli/hvac.py
index 8ce0b7d..dd3f772 100644
--- a/vault_cli/hvac.py
+++ b/vault_cli/hvac.py
@@ -21,13 +21,14 @@ from typing import Iterable
 
 import hvac
 
-from vault_cli import types
+from vault_cli import sessions, types
 from vault_cli.client import VaultAPIException, VaultClientBase
 
 
 class HVACVaultClient(VaultClientBase):
     def _init_session(self, url: str, verify: types.VerifyOrCABundle) -> None:
-        self.client = hvac.Client(url=url, verify=verify)
+        self.session = sessions.Session()
+        self.client = hvac.Client(url=url, verify=verify, session=self.session)
 
     def _authenticate_token(self, token: str) -> None:
         self.client.token = token
@@ -52,3 +53,6 @@ class HVACVaultClient(VaultClientBase):
 
     def set_secret(self, path: str, value: types.JSONValue) -> None:
         self.client.write(self.base_path + path, value=value)
+
+    def __exit__(self, exc_type, exc_value, traceback):
+        self.session.__exit__(exc_type, exc_value, traceback)
diff --git a/vault_cli/requests.py b/vault_cli/requests.py
index 9af175b..08a3cb3 100644
--- a/vault_cli/requests.py
+++ b/vault_cli/requests.py
@@ -21,30 +21,11 @@ from typing import Iterable
 from urllib.parse import urljoin
 
 import requests
-import urllib3
 
-from vault_cli import types
+from vault_cli import sessions, types
 from vault_cli.client import VaultAPIException, VaultClientBase
 
 
-class Session(requests.Session):
-    """A wrapper for requests.Session to override 'verify' property, ignoring
-    REQUESTS_CA_BUNDLE environment variable.
-
-    This is a workaround for
-    https://github.com/requests/requests/issues/3829
-    """
-
-    def merge_environment_settings(self, url, proxies, stream, verify, *args, **kwargs):
-        if self.verify is False:
-            verify = False
-            urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning)
-
-        return super(Session, self).merge_environment_settings(
-            url, proxies, stream, verify, *args, **kwargs
-        )
-
-
 class RequestsVaultClient(VaultClientBase):
     def _init_session(self, url: str, verify: types.VerifyOrCABundle) -> None:
         self.session = self.create_session(verify)
@@ -64,10 +45,13 @@ class RequestsVaultClient(VaultClientBase):
 
     @staticmethod
     def create_session(verify: types.VerifyOrCABundle) -> requests.Session:
-        session = Session()
+        session = sessions.Session()
         session.verify = verify
         return session
 
+    def __exit__(self, exc_type, exc_value, traceback):
+        self.session.__exit__(exc_type, exc_value, traceback)
+
     def _authenticate_token(self, token: str) -> None:
         self.session.headers.update({"X-Vault-Token": token})
 
diff --git a/vault_cli/sessions.py b/vault_cli/sessions.py
new file mode 100644
index 0000000..875916b
--- /dev/null
+++ b/vault_cli/sessions.py
@@ -0,0 +1,20 @@
+import requests
+import urllib3
+
+
+class Session(requests.Session):
+    """A wrapper for requests.Session to override 'verify' property, ignoring
+    REQUESTS_CA_BUNDLE environment variable.
+
+    This is a workaround for
+    https://github.com/requests/requests/issues/3829
+    """
+
+    def merge_environment_settings(self, url, proxies, stream, verify, *args, **kwargs):
+        if self.verify is False:
+            verify = False
+            urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning)
+
+        return super(Session, self).merge_environment_settings(
+            url, proxies, stream, verify, *args, **kwargs
+        )


Test patch:
diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py
index a0ee631..ee46d02 100644
--- a/tests/unit/test_client.py
+++ b/tests/unit/test_client.py
@@ -335,3 +335,14 @@ def test_vault_client_base_delete_all_secrets():
     list(result)
 
     assert deleted == ["a/c", "b/d"]
+
+
+def test_vault_client_base_context_manager():
+    class TestVaultClient(client.VaultClientBase):
+        def __init__(self):
+            pass
+
+    client_obj = TestVaultClient()
+
+    with client_obj as c:
+        assert c is client_obj
diff --git a/tests/unit/test_clients.py b/tests/unit/test_clients.py
index d954503..d551540 100644
--- a/tests/unit/test_clients.py
+++ b/tests/unit/test_clients.py
@@ -19,9 +19,13 @@ def get_client(backend, **additional_kwargs):
     return client.get_client_from_kwargs(**kwargs)
 
 
-@pytest.mark.parametrize("backend", ["requests", "hvac"])
-def test_token(requests_mock, backend):
-    client_obj = get_client(backend)
+@pytest.fixture(params=["requests", "hvac"])
+def any_backend(request):
+    yield request.param
+
+
+def test_token(requests_mock, any_backend):
+    client_obj = get_client(any_backend)
     requests_mock.get(
         "http://vault:8000/v1/bla/a",
         request_headers={"X-Vault-Token": "tok"},
@@ -33,8 +37,7 @@ def test_token(requests_mock, backend):
     assert requests_mock.called
 
 
-@pytest.mark.parametrize("backend", ["requests", "hvac"])
-def test_userpass(requests_mock, backend):
+def test_userpass(requests_mock, any_backend):
     requests_mock.post(
         "http://vault:8000/v1/auth/userpass/login/myuser",
         json={"auth": {"client_token": "newtok"}},
@@ -42,7 +45,7 @@ def test_userpass(requests_mock, backend):
 
     # Initialize a client, check that we get a token
     client_obj = get_client(
-        backend=backend, token=None, username="myuser", password="pass"
+        backend=any_backend, token=None, username="myuser", password="pass"
     )
 
     # Check that the token is used
@@ -57,16 +60,14 @@ def test_userpass(requests_mock, backend):
     assert requests_mock.request_history[0].json() == {"password": "pass"}
 
 
-@pytest.mark.parametrize("backend", ["requests", "hvac"])
-def test_get_secret(requests_mock, backend):
-    client_obj = get_client(backend)
+def test_get_secret(requests_mock, any_backend):
+    client_obj = get_client(any_backend)
     requests_mock.get("http://vault:8000/v1/bla/a", json={"data": {"value": "b"}})
     assert client_obj.get_secret("a") == "b"
 
 
-@pytest.mark.parametrize("backend", ["requests", "hvac"])
-def test_get_secret_not_found(requests_mock, backend):
-    client_obj = get_client(backend)
+def test_get_secret_not_found(requests_mock, any_backend):
+    client_obj = get_client(any_backend)
     requests_mock.get(
         "http://vault:8000/v1/bla/a", status_code=404, json={"errors": ["Not found"]}
     )
@@ -74,26 +75,23 @@ def test_get_secret_not_found(requests_mock, backend):
         assert client_obj.get_secret("a")
 
 
-@pytest.mark.parametrize("backend", ["requests", "hvac"])
-def test_get_secret_no_verify(requests_mock, backend):
-    client_obj = get_client(backend, verify=False)
+def test_get_secret_no_verify(requests_mock, any_backend):
+    client_obj = get_client(any_backend, verify=False)
     requests_mock.get("http://vault:8000/v1/bla/a", json={"data": {"value": "b"}})
     assert client_obj.get_secret("a") == "b"
     assert requests_mock.last_request.verify is False
 
 
-@pytest.mark.parametrize("backend", ["requests", "hvac"])
-def test_list_secrets(requests_mock, backend):
-    client_obj = get_client(backend)
+def test_list_secrets(requests_mock, any_backend):
+    client_obj = get_client(any_backend)
     requests_mock.get(
         "http://vault:8000/v1/bla/a?list=True", json={"data": {"keys": ["b"]}}
     )
     assert client_obj.list_secrets("a") == ["b"]
 
 
-@pytest.mark.parametrize("backend", ["requests", "hvac"])
-def test_list_secrets_empty(requests_mock, backend):
-    client_obj = get_client(backend)
+def test_list_secrets_empty(requests_mock, any_backend):
+    client_obj = get_client(any_backend)
     requests_mock.get(
         "http://vault:8000/v1/bla/a?list=True",
         status_code=404,
@@ -102,9 +100,8 @@ def test_list_secrets_empty(requests_mock, backend):
     assert client_obj.list_secrets("a") == []
 
 
-@pytest.mark.parametrize("backend", ["requests", "hvac"])
-def test_list_secrets_other_error(requests_mock, backend):
-    client_obj = get_client(backend)
+def test_list_secrets_other_error(requests_mock, any_backend):
+    client_obj = get_client(any_backend)
     requests_mock.get(
         "http://vault:8000/v1/bla/a?list=True",
         status_code=500,
@@ -115,18 +112,16 @@ def test_list_secrets_other_error(requests_mock, backend):
         client_obj.list_secrets("a")
 
 
-@pytest.mark.parametrize("backend", ["requests", "hvac"])
-def test_delete_secret(requests_mock, backend):
-    client_obj = get_client(backend)
+def test_delete_secret(requests_mock, any_backend):
+    client_obj = get_client(any_backend)
     requests_mock.delete("http://vault:8000/v1/bla/a", status_code=204)
     client_obj.delete_secret("a")
 
     assert requests_mock.called
 
 
-@pytest.mark.parametrize("backend", ["requests", "hvac"])
-def test_set_secret(requests_mock, backend):
-    client_obj = get_client(backend)
+def test_set_secret(requests_mock, any_backend):
+    client_obj = get_client(any_backend)
     # Both post and put can be used
     requests_mock.put("http://vault:8000/v1/bla/a", status_code=204, json={})
     requests_mock.post("http://vault:8000/v1/bla/a", status_code=204, json={})
@@ -134,3 +129,12 @@ def test_set_secret(requests_mock, backend):
 
     assert requests_mock.called
     assert requests_mock.request_history[0].json() == {"value": "b"}
+
+
+def test_set_context_manager(any_backend):
+    client_obj = get_client(any_backend)
+    with client_obj as c:
+        assert client_obj is c
+
+    # Assert connection is closed ?
+    # At least we make sure our context manager doesn't crash.