Long body in responses and requests #897

vladtcvs · 2018-01-26T11:45:43Z

vankoven

#619 can't be covered by a stress test, deproxy is required here. The issue is about https://tools.ietf.org/html/rfc7230#section-3.3.3 point 7.

I also don't sure that we have all points from RFC7230 Section-3.3.3 covered.

vankoven · 2018-01-26T13:34:20Z

tempesta_fw/t/functional/helpers/control.py

@@ -108,19 +108,71 @@ def set_user_agent(self, ua):
 class Wrk(Client):
    """ wrk - HTTP benchmark utility. """

+    class ScriptGenerator(object):


Why you have decided to define class inside Wrk class? It's requred only in some cases, better to move the definition into separate file like it's done for tempesta.py and nginx.py which provide advanced control options for Tempesta and Nginx classes. I admit i'm not good in python and there can be much and much better implementations of current code, but i can't agree here.

krizhanovsky · 2018-02-02T17:24:46Z

etc/tempesta_fw.conf

+block_action error reply;
+
+block_action attack reply;
+


Seems a mistaken change: the file is used by end users, so never put any testing stuff here, especially, specific for your local installation.

vankoven

I've tried to run the test and got errors like this:

======================================================================
ERROR: test_purge (cache.test_purge.TestPurge)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/cache/test_purge.py", line 40, in test_purge
    self.generic_test_routine(self.config, self.chains())
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/cache/test_purge.py", line 26, in chains
    chains.proxy(method='HEAD', uri=uri),
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/helpers/chains.py", line 241, in proxy
    return base(forward=True, *args, **kwargs)
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/helpers/chains.py", line 160, in base
    body=common_resp_body,
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/helpers/deproxy.py", line 480, in create
    return Response(msg)
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/helpers/deproxy.py", line 413, in __init__
    HttpMessage.__init__(self, *args, **kwargs)
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/helpers/deproxy.py", line 211, in __init__
    self.parse_text(message_text, body_parsing)
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/helpers/deproxy.py", line 216, in parse_text
    self.__parse(stream)
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/helpers/deproxy.py", line 223, in __parse
    self.parse_body(stream)
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/helpers/deproxy.py", line 454, in parse_body
    self.read_sized_body(stream)
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/helpers/deproxy.py", line 285, in read_sized_body
    % (size, len(self.body))))
ParseError: Wrong body size: expect 138 but got 0!

======================================================================
ERROR: test_head (cache.test_cache_methods.TestCacheMethods)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/cache/test_cache_methods.py", line 57, in test_head
    self.try_method('HEAD')
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/cache/test_cache_methods.py", line 36, in try_method
    cache_alowed=(method in self.cacheable_methods))
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/cache/test_cache_methods.py", line 30, in chain
    return chains.cache_repeated(self.messages, method=method, uri=uri)
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/helpers/chains.py", line 244, in cache_repeated
    chains = [proxy(*args, **kwargs)]
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/helpers/chains.py", line 241, in proxy
    return base(forward=True, *args, **kwargs)
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/helpers/chains.py", line 160, in base
    body=common_resp_body,
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/helpers/deproxy.py", line 480, in create
    return Response(msg)
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/helpers/deproxy.py", line 413, in __init__
    HttpMessage.__init__(self, *args, **kwargs)
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/helpers/deproxy.py", line 211, in __init__
    self.parse_text(message_text, body_parsing)
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/helpers/deproxy.py", line 216, in parse_text
    self.__parse(stream)
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/helpers/deproxy.py", line 223, in __parse
    self.parse_body(stream)
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/helpers/deproxy.py", line 454, in parse_body
    self.read_sized_body(stream)
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/helpers/deproxy.py", line 285, in read_sized_body
    % (size, len(self.body))))
ParseError: Wrong body size: expect 138 but got 0!

======================================================================
ERROR: test_scheduler (sched.test_http.HttpRules)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/sched/test_http.py", line 109, in test_scheduler
    self.routine()
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/sched/test_http.py", line 90, in routine
    tester.configure(i)
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/sched/test_http.py", line 216, in configure
    self.client.set_request(self.current_chain.request)
  File "/mnt/Storage/Projects/tempesta/tempesta/tempesta_fw/t/functional/helpers/deproxy.py", line 507, in set_request
    self.request = request_chain.request
AttributeError: 'Request' object has no attribute 'request'

Please, run all tests before requesting pull. You're changing the core of test frame work, and it's great (framework itself not very good in some places). But it's not acceptable if your changes break existing tests.

vankoven · 2018-02-16T12:23:15Z

tempesta_fw/t/functional/helpers/deproxy.py

+    def parse_body(self, stream):
+        """ RFC 7230 3.3.3 """
+        # 3.3.3 1
+        if self.method == "HEAD":


There is many cases when response may not have body: 302 responses, 200 responses to CONNECT and so on. I think that previous flag suit better in this case.

independent flag can generate inconsistency. We don't have 302 and CONNECT tests now, and when we write such tests, we should add this cases here

vankoven · 2018-02-16T13:25:02Z

tempesta_fw/t/functional/helpers/deproxy.py

@@ -491,6 +532,8 @@ def handle_read(self):
                           '<<<<<\n%s>>>>>'
                           % self.response_buffer))
            raise
+        if len(self.response_buffer) > 0:
+            raise ParseError('Garbage after response end:\n```\n%s\n```\n' % self.response_buffer)


Ok, now it's makes the job. But a either a TODO comment or rework is required here. Currently both client and response don't support message pipelining because of tests structure: messages from TempestaFW are validated one-by one (The test suite was developed before pipelining was supported). We may configure Client client with the full chain list and give it some instructions about when to use the pipelining. When the Server receives a new message it can save it to the right slot in chain list. Not all tests allow pipelining.

Now we don't have pipelined functional test. So I've added "TODO"

vankoven · 2018-02-16T13:28:00Z

tempesta_fw/t/functional/helpers/wrk.py

+    headers = []
+    body = ""
+    filename = None
+    def __luaencode(self, value):


Method can be a simple function. But why it's actually required? It does just nothing.

It should care about escaping, if we write test with " and so on in content

Now we don't have tests with content, which should be escaped. So now it does nothing

vankoven · 2018-02-16T13:36:17Z

tempesta_fw/t/functional/helpers/wrk.py

+        config.write("}\n")
+        config.write("local req\n")
+        config.write("init = function()\n")
+        config.write("    req = wrk.format(r.method, r.path, r.headers, r.body)\n")


If i'm not mistaken just setting

wrk = { method = "GET", path = "/", headers = {}, body = nil, }

will make the same job for you. In the same time wrk docs says that request() function is heavy and should be avoided:
https://github.com/wg/wrk/blob/7594a95186ebdfa7cb35477a8a811f84e2a31b62/SCRIPTING#L81-L84

fixed, but we need

wrk.method = "GET", wrk.path = ....

instead of wrk = {...}

vankoven · 2018-02-16T13:43:00Z

tempesta_fw/t/functional/long_body/test_long_request.py

+class RequestTestBase(stress.StressTest):
+    """ Test long request """
+    config = "cache 0;\n"
+    root = "/tmp/long_body/"


There is variable client.workdir in tf_cfg which points to /tmp/client by default. Use it as root dir for your directories and files.

vankoven · 2018-02-16T13:46:25Z

tempesta_fw/t/functional/long_body/test_long_request.py

+        """ Create wrk client with long request body """
+        self.generator = wrk.ScriptGenerator()
+        self.generator.set_body(body_generator.generate_body(length))
+        if not os.path.exists(self.root):


Use remote.node.mkdir() instead: first it always care about on which node should it make the directory, secondly it already throws an exception for test suit on errors.

wrk scripts copy code fixed

vladtcvs · 2018-02-18T13:47:42Z

problem with breaking other tests fixed

vankoven · 2018-03-12T21:14:30Z

tempesta_fw/t/functional/helpers/chains.py

@@ -197,6 +217,34 @@ def base(uri='/', method='GET', forward=True, date=None):
                                      server_response=backend_resp)
    return copy.copy(base_chain)

+def response_403(date=None, connection=None):


There is response_500() in the begging of the file, and response_403 here. Better to group code somehow.

vankoven · 2018-03-12T21:16:11Z

tempesta_fw/t/functional/helpers/chains.py

+    response = deproxy.Response.create(status=500, headers=headers)
+    return response
+
+def response_502():


There already is

tempesta/tempesta_fw/t/functional/helpers/chains.py

Line 28 in 3904101

def make_502_expected():

That is the difference?

it's the same. fixed

vankoven · 2018-03-12T21:20:16Z

tempesta_fw/t/functional/helpers/chains.py

+                                       date=date,
+                                       body='')
+    else:
+        resp = deproxy.Response.create(status=403,


Copy-paste. It's better to use variable to store list of headers and add Connection: header if connection is not None, then call deproxy.Response.create() only once

vankoven · 2018-03-12T21:23:30Z

tempesta_fw/t/functional/helpers/control.py

+        script_path = self.workdir + "/" + self.script
+
+        if self.copy_script:
+            local_path = ''.join([self.local_scriptdir, self.script])


Wrk is always called on the same node as test frame work. Why need to copy file? We can just reference it. Which is old behaviour.

It's for universal work with scripts from wrk/ and generated scripts

vankoven

Good to mere, but a couple of cleanups is required.

vankoven · 2018-03-15T14:53:30Z

tempesta_fw/t/functional/long_body/test_long_response.py

+
+    def create_content(self, length):
+        """ Create content file """
+        content = body_generator.generate_body(length)


It's good to test a lot of different kind of response. I think, we even need to go further: we need to prepare a back end configuration that will be capable to respond to POST, to serve a different type of resources, small and big ones. That will be a great enhancement for tests, isn't it?
Wee need to create a separate issue for that for one of the next milestones. What do you think?

I think it's a good idea to make backend more configurable

vankoven · 2018-03-15T14:54:13Z

tempesta_fw/t/functional/helpers/wrk.py

+            config += "    [\"%s\"] = \"%s\",\n" % (name, value)
+        config += "},\n"
+        config += "wrk.body = \"%s\",\n" % self.__luaencode(self.body)
+        remote.client.copy_file(filename, config)


We've discussed this line in the chat, it should be fixed.

vankoven · 2018-03-15T14:58:26Z

tempesta_fw/t/functional/helpers/deproxy.py

@@ -512,6 +552,10 @@ def handle_read(self):
                           '<<<<<\n%s>>>>>'
                           % self.response_buffer))
            raise
+        if len(self.response_buffer) > 0:


Please check with test_pairing.py tests, i expect problems here. Reach me in chat if you catch an error there, we'll discuss a solution.

generating lua config from test Add 1k and 1M test Test responses add 1M response test

expect 403 for wrong length Parsing multiple responses Expect exactly 1 parsing error check for garbage after response end Write special testers for wrong length

Add missing length tests Close connection after response w/out content-length Add tests for multiple or invalid content-length

Fix warning: /tmp/client/request_1M.lua: /tmp/client/request_1M.lua:5: unexpected symbol near '='

vladtcvs changed the title ~~Long body in responses and requests~~ WIP: Long body in responses and requests Jan 26, 2018

vladtcvs force-pushed the vlts-619 branch 3 times, most recently from 875f12d to 578fd88 Compare January 26, 2018 11:52

vladtcvs changed the title ~~WIP: Long body in responses and requests~~ Long body in responses and requests Jan 26, 2018

vankoven suggested changes Jan 26, 2018

View reviewed changes

krizhanovsky requested changes Feb 2, 2018

View reviewed changes

vladtcvs force-pushed the vlts-619 branch from 1aedaaf to 8492bce Compare February 5, 2018 14:58

vladtcvs added the ready for review label Feb 9, 2018

vankoven suggested changes Feb 16, 2018

View reviewed changes

vladtcvs force-pushed the vlts-619 branch from 8492bce to ea3403e Compare February 18, 2018 13:46

vladtcvs force-pushed the vlts-619 branch 3 times, most recently from dbe0da0 to 9d3eb96 Compare February 19, 2018 20:47

vladtcvs removed the ready for review label Mar 11, 2018

vladtcvs force-pushed the vlts-619 branch 5 times, most recently from c1cc4d9 to 485b41a Compare March 12, 2018 03:59

vladtcvs added the ready for review label Mar 12, 2018

vladtcvs force-pushed the vlts-619 branch from 485b41a to a42fcfb Compare March 12, 2018 20:43

vankoven reviewed Mar 12, 2018

View reviewed changes

vladtcvs force-pushed the vlts-619 branch 2 times, most recently from b78b362 to a707e9c Compare March 15, 2018 11:48

vankoven approved these changes Mar 15, 2018

View reviewed changes

vladtcvs force-pushed the vlts-619 branch from 87c4cb1 to ac35069 Compare March 15, 2018 21:46

vladtcvs added 2 commits March 16, 2018 00:50

Add long body tests

2c245bb

generating lua config from test Add 1k and 1M test Test responses add 1M response test

Request tests with wrong length

024bc29

expect 403 for wrong length Parsing multiple responses Expect exactly 1 parsing error check for garbage after response end Write special testers for wrong length

vladtcvs and others added 7 commits March 16, 2018 00:50

responses tests

d555dd0

Add missing length tests Close connection after response w/out content-length Add tests for multiple or invalid content-length

Fixes

2527f60

fixes after review

27e39a6

move saving config to file to test

dcbf6b5

func tests: remove extra commas unwanded by wrk

f2b6bdf

Fix warning: /tmp/client/request_1M.lua: /tmp/client/request_1M.lua:5: unexpected symbol near '='

Fixes after review

fbe7b9c

Fix handling asyncore and parsing error

3a27d27

vladtcvs force-pushed the vlts-619 branch from ac35069 to 3a27d27 Compare March 15, 2018 21:58

handle close

c5db882

vladtcvs force-pushed the vlts-619 branch from 9931eca to 1a73a15 Compare March 16, 2018 09:34

vladtcvs added 2 commits March 16, 2018 12:47

Don't measure number of connections during tests with invalid response

4c9c77a

fix TestInvalidResponse

e2a0678

vladtcvs force-pushed the vlts-619 branch from 1a73a15 to e2a0678 Compare March 16, 2018 09:47

krizhanovsky approved these changes Mar 16, 2018

View reviewed changes

vladtcvs merged commit 3e09b4f into master Mar 16, 2018

vladtcvs deleted the vlts-619 branch March 16, 2018 12:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Long body in responses and requests #897

Long body in responses and requests #897

vladtcvs commented Jan 26, 2018

vankoven left a comment

vankoven Jan 26, 2018

krizhanovsky Feb 2, 2018

vladtcvs Feb 5, 2018

vankoven left a comment

vankoven Feb 16, 2018

vladtcvs Feb 16, 2018

vankoven Feb 16, 2018

vladtcvs Feb 18, 2018

vankoven Feb 16, 2018

vladtcvs Feb 18, 2018 •

edited

Loading

vladtcvs Mar 11, 2018

vankoven Feb 16, 2018

vladtcvs Feb 18, 2018

vladtcvs Feb 18, 2018

vankoven Feb 16, 2018

vladtcvs Mar 11, 2018

vankoven Feb 16, 2018

vladtcvs Feb 18, 2018

vladtcvs commented Feb 18, 2018

vankoven Mar 12, 2018

vladtcvs Mar 15, 2018

vankoven Mar 12, 2018

vladtcvs Mar 15, 2018

vankoven Mar 12, 2018

vladtcvs Mar 15, 2018

vankoven Mar 12, 2018

vladtcvs Mar 15, 2018

vankoven left a comment

vankoven Mar 15, 2018

vladtcvs Mar 15, 2018

vankoven Mar 15, 2018

vladtcvs Mar 15, 2018

vankoven Mar 15, 2018

Long body in responses and requests #897

Long body in responses and requests #897

Conversation

vladtcvs commented Jan 26, 2018

vankoven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vankoven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladtcvs Feb 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladtcvs commented Feb 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vankoven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladtcvs Feb 18, 2018 •

edited

Loading