entrouvert
/
passerelle
15
0
Fork 0
Code Issues Pull Requests 16 Releases Activity

proxy: allow 0 timeout to use system default (#88886) #506

Open
tnoel wants to merge 1 commits from wip/88886-proxy-timeout-0-for-system into main
pull from: wip/88886-proxy-timeout-0-for-system
merge into: entrouvert:main
Conversation 0 Commits 1 Files Changed 3 +17 -3
tnoel commented 2024-03-29 18:37:50 +01:00
Owner
Copy Link
No description provided.
tnoel added 1 commit 2024-03-29 18:37:50 +01:00
gitea/passerelle/pipeline/head This commit looks good Details
9f8375b2a2 proxy: allow 0 timeout to use system default (#88886)
tnoel force-pushed wip/88886-proxy-timeout-0-for-system from 9f8375b2a2 to e6911e00be 2024-03-29 19:59:12 +01:00 Compare
yweber reviewed 2024-04-04 15:18:46 +02:00
passerelle/apps/proxy/models.py
@ -38,2 +38,3 @@
upstream_base_url = models.URLField(_('Upstream Service Base URL'))
http_timeout = models.PositiveIntegerField(_('Timeout on upstream (in seconds)'), default=20)
http_timeout = models.PositiveIntegerField(
_('Timeout on upstream (in seconds, 0 for system default)'), default=20
yweber commented 2024-04-04 15:18:46 +02:00
Owner
Copy Link

Si c'est bien requests derrière, d'après la doc, None comme timeout c'est timeout infinie plutôt qu'une valeur raisonnable utilisée par défaut.

Je ne sais pas si c'est volontaire, personnellement je ne m'y attendais pas ;)

Si c'est bien requests derrière, [d'après la doc](https://docs.python-requests.org/en/latest/user/advanced/#timeouts), `None` comme timeout c'est timeout infinie plutôt qu'une valeur raisonnable utilisée par défaut. Je ne sais pas si c'est volontaire, personnellement je ne m'y attendais pas ;)
fpeters commented 2024-04-04 15:20:51 +02:00
Owner
Copy Link

On a dans passerelle/utils/init.py,

self.timeout = timeout if timeout is not None else settings.REQUESTS_TIMEOUT
On a dans passerelle/utils/__init__.py, ``` self.timeout = timeout if timeout is not None else settings.REQUESTS_TIMEOUT ```
yweber commented 2024-04-04 15:26:30 +02:00
Owner
Copy Link

Ok, merci :)

Ok, merci :)
yweber commented 2024-04-04 18:05:21 +02:00
Owner
Copy Link

Comme j'ai eu un peu de mal à voir comment l'ensemble fonctionnait (entre notre Session custom, les requêtes préparés de requests et celles récupérés et testés par responses) j'ai creusé et j'ai l'impression d'être tombé sur un os.

J'ai l'impression que la requête émise n'est jamais préparé par notre Session (enfin j'imagine), le résultat c'est qu'on dirait bien qu'avec le timeout à None on tombe sur un timeout infini et pas sur la valeur de settings.REQUESTS_TIMEOUT

J'ai fais des tests (en lançant un netcat qui ne répondais jamais) pour vérifier et j'ai écris une poc (un peu dégueu) avec deux tests qui semblent aller dans ce sens (le 1er passe le 2nd jamais) :

diff --git a/tests/test_proxy.py b/tests/test_proxy.py
index 9441918e..de8a3a73 100644
--- a/tests/test_proxy.py
+++ b/tests/test_proxy.py
@@ -17,6 +17,7 @@
 import pytest
 import responses
 import responses.matchers
+from django.conf import settings
 
 import tests.utils
 from passerelle.apps.proxy.models import Resource
@@ -177,3 +178,35 @@ def test_timeout(mocked_responses, app, proxy, endpoint):
     )
     resp = app.get(endpoint + '/foo/bar', status=200)
     assert resp.text == 'ok'
+
+@pytest.fixture
+def sock_9999():
+    import socket
+    srv = socket.create_server(('localhost', 9999), reuse_port=True)
+    yield srv
+    srv.close()
+
+@pytest.mark.timeout(12)
+def test_tmout_2(app, db, sock_9999):
+    proxy = tests.utils.setup_access_rights(
+            Resource.objects.create(slug='echo-timeout', upstream_base_url='http://127.0.0.1:9999/')
+        )
+    endpoint = tests.utils.generic_endpoint_url('proxy', 'request', slug=proxy.slug)
+
+    proxy.http_timeout = 2
+    proxy.save()
+    resp = app.get(endpoint + '/foo/bar', status=500)
+    assert resp.json['err_class'] == 'requests.exceptions.ReadTimeout'
+
+@pytest.mark.timeout(settings.REQUESTS_TIMEOUT + 10)
+def test_tmout_default(app, db, sock_9999):
+    proxy = tests.utils.setup_access_rights(
+            Resource.objects.create(slug='echo-timeout', upstream_base_url='http://127.0.0.1:9999/')
+        )
+    endpoint = tests.utils.generic_endpoint_url('proxy', 'request', slug=proxy.slug)
+
+
+    proxy.http_timeout = 0  # system default
+    proxy.save()
+    resp = app.get(endpoint + '/foo/bar', status=500)
+    assert resp.json['err_class'] == 'requests.exceptions.ReadTimeout'
diff --git a/tox.ini b/tox.ini
index c3a86f9c..f11da477 100644
--- a/tox.ini
+++ b/tox.ini
@@ -20,6 +20,7 @@ deps =
   pytest-cov
   pytest-django
   pytest-xdist
+  pytest-timeout
   pytest!=6.0.0
   WebTest
   mock<4
Comme j'ai eu un peu de mal à voir comment l'ensemble fonctionnait (entre notre Session custom, les requêtes préparés de requests et celles récupérés et testés par responses) j'ai creusé et j'ai l'impression d'être tombé sur un os. J'ai l'impression que la requête émise n'est jamais préparé par notre Session (enfin j'imagine), le résultat c'est qu'on dirait bien qu'avec le timeout à None on tombe sur un timeout infini et pas sur la valeur de settings.REQUESTS_TIMEOUT J'ai fais des tests (en lançant un netcat qui ne répondais jamais) pour vérifier et j'ai écris une poc (un peu dégueu) avec deux tests qui semblent aller dans ce sens (le 1er passe le 2nd jamais) : ```diff diff --git a/tests/test_proxy.py b/tests/test_proxy.py index 9441918e..de8a3a73 100644 --- a/tests/test_proxy.py +++ b/tests/test_proxy.py @@ -17,6 +17,7 @@ import pytest import responses import responses.matchers +from django.conf import settings import tests.utils from passerelle.apps.proxy.models import Resource @@ -177,3 +178,35 @@ def test_timeout(mocked_responses, app, proxy, endpoint): ) resp = app.get(endpoint + '/foo/bar', status=200) assert resp.text == 'ok' + +@pytest.fixture +def sock_9999(): + import socket + srv = socket.create_server(('localhost', 9999), reuse_port=True) + yield srv + srv.close() + +@pytest.mark.timeout(12) +def test_tmout_2(app, db, sock_9999): + proxy = tests.utils.setup_access_rights( + Resource.objects.create(slug='echo-timeout', upstream_base_url='http://127.0.0.1:9999/') + ) + endpoint = tests.utils.generic_endpoint_url('proxy', 'request', slug=proxy.slug) + + proxy.http_timeout = 2 + proxy.save() + resp = app.get(endpoint + '/foo/bar', status=500) + assert resp.json['err_class'] == 'requests.exceptions.ReadTimeout' + +@pytest.mark.timeout(settings.REQUESTS_TIMEOUT + 10) +def test_tmout_default(app, db, sock_9999): + proxy = tests.utils.setup_access_rights( + Resource.objects.create(slug='echo-timeout', upstream_base_url='http://127.0.0.1:9999/') + ) + endpoint = tests.utils.generic_endpoint_url('proxy', 'request', slug=proxy.slug) + + + proxy.http_timeout = 0 # system default + proxy.save() + resp = app.get(endpoint + '/foo/bar', status=500) + assert resp.json['err_class'] == 'requests.exceptions.ReadTimeout' diff --git a/tox.ini b/tox.ini index c3a86f9c..f11da477 100644 --- a/tox.ini +++ b/tox.ini @@ -20,6 +20,7 @@ deps = pytest-cov pytest-django pytest-xdist + pytest-timeout pytest!=6.0.0 WebTest mock<4 ```
All checks were successful
gitea/passerelle/pipeline/head This commit looks good
Details
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: entrouvert/passerelle#506
No description provided.
Delete Branch "wip/88886-proxy-timeout-0-for-system"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?