Tests Frang #265

KonsKo · 2022-08-05T23:00:24Z

I prepared my work pass it to Nadia, because now she is gonna make Frang tests.
I upload almost everything I have except tests I was stacked with. Something was made some time ago, something not. I disabled several tests with comment Disabled for PR (them should be fixed). I repeat, I did not fix them, because task was passed to Nadia.

Also, there is some cleaning for multiple_listeners.

@ProshNad, after PR is done, take a look of tests and do everything you want.

The related issues is tempesta-tech/tempesta#673

ProshNad · 2022-08-22T12:47:09Z

It so happened that I already took a branch kk-test and fixed broken tests and added my new tests. Next time I will do everything in my branch, but this time we agreed to leave it like that and do a review in this branch.

krizhanovsky

This mostly looks like an acceptance testing, than a real functional tests. This is a security feature, so please pay much more attention to all the corner and confusing cases. Please review https://github.com/tempesta-tech/tempesta/wiki/HTTP-security and all the tests.

cert_gen.sh

createst.sh

multiple_listeners/config_for_tests_template.txt

t_frang/test_connection_rate_burst.py

t_frang/test_header_cnt.py

t_frang/test_tls_rate_burst.py

t_frang/test_request_rate_burst.py

krizhanovsky

This is a tremendous work, but there are still many issues which must be fixed.

.gitignore

t_frang/test_http_method_override_allowed.py

t_frang/test_http_trailer_split_allowed.py

t_frang/test_length.py

t_frang/test_request_rate_burst.py

nickzaev

To sum up:

multiple_listeners group of test has already been fixed in plain configs for multiple listeners #275, so you just need to rebase your branch onto master, resolving conflicts
There should be some autoformatting applied to this entire PR since changing every commented line's formatting by hand is not that productive. I suggest you to commit all your changes though before applying one, hence there will be many changes and you might not like some of them
The majority of test files contain some base class and just one class directly inherited from it, on the other hand the tests are being grouped by files (not by classes) and it is very unlikely that, say, there will be another class in t_frang/test_client_body_timeout.py any soon. So what's the point having that inheritance approach all over the place?
About a solid third of the tests code consists of almost identical Nginx configurations which either don't differ at all or just in very minor parts, so it looks more that we test Nginx rather than Tempesta :) Why not just reuse FrangTestCase everywhere possible at least just for consistency's sake?
If we actually fall for that approach of using inheritance and all, the parts of starting off backends and Tempesta should be in setUp method of some base class and not get repeated for every test as it is for now.
Constants are massively overused, indeed, what's point of having such constants as ONE?

Regarding running the tests, some of them resulted in errors because of you using non-standard path to the encryption keys:

Ran 98 tests in 307.733s

FAILED (failures=1, errors=83)

Hit me up when it gets fixed and I'll give it a try again.

nickzaev · 2022-10-09T13:03:37Z

.gitignore

@@ -2,4 +2,4 @@ tests_config.ini
 tests_resume.json
 tests_log.log
 *.pyc
-*~
+*~


These files usually get extended by piping, redirecting, etc. That is why there should be a newline character at the end, so why do you delete this one?

t_frang/frang_test_case.py

nickzaev · 2022-10-09T13:23:31Z

t_frang/test_client_body_timeout.py

+            'type': 'nginx',
+            'status_uri': 'http://${server_ip}:8000/nginx_status',
+            'config': """
+pid ${pid};


This test has the very same nginx configuration as FrangTestCase, so why it's not being reused if we already put it in a separate class and file only with that configuration?

yes you are right I will fix it

I parametrized all tests with base class - FrangTestCase

nickzaev · 2022-10-09T13:33:10Z

t_frang/test_concurrent_connections.py

+        self.assertEqual(0, len(deproxy_cl3.responses))
+
+        self.assertFalse(deproxy_cl.connection_is_closed())
+        self.assertTrue(deproxy_cl2.connection_is_closed())#all clients should be blocked here, but for some reason only one gets closed


It should probably be the second argument to self.assertTrue, not a comment, so the person who sees this failure in logs would not have to look up the test code for the meaning of it. The same goes to all similar comments.

nickzaev · 2022-10-09T13:38:31Z

t_frang/test_connection_rate_burst.py

+                connection_rate 4;
+            }
+
+            listen 127.0.0.4:8765;


IPs and other things that are subject to change should not be hardcoded, we have ${server_ip} and embedded runtime substitution with actual values for this particular purpose.

nickzaev · 2022-10-09T13:40:06Z

t_frang/test_connection_rate_burst.py

+            }
+
+            tls_match_any_server_name;
+            tls_certificate RSA/tfw-root.crt;


It should be tls_certificate ${tempesta_workdir}/tempesta.crt;

t_frang/test_host_required.py

t_frang/test_http_ct_vals.py

krizhanovsky

There are still issues in the PR, but I think there is nothing for me to review next time, so I approve the PR.

@RomanBelozerov I reassign the PR to you to make a review with appropriate adjustments, which you think are required. Also could you please address all not addressed comments by me and @nickzaev .

@pale-emperor I add you to reviewers: the main point to pay attention is that the branch is stable enough to not to break our CI.

pale-emperor · 2022-10-20T19:59:37Z

I have run the tests on CI node for several times and get stable errors in the tests listed below:

FAIL: test_h2_host_header_as_ipv6 (t_frang.test_host_required.FrangHostRequiredH2TestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tempesta/tempesta-test/t_frang/test_host_required.py", line 520, in test_h2_host_header_as_ipv6
    self._test_base_scenario(
  File "/home/tempesta/tempesta-test/t_frang/test_host_required.py", line 578, in _test_base_scenario
    self.assertEqual(
AssertionError: 0 != 1 : Frang limits warning is not shown

======================================================================
FAIL: test_h2_host_header_mismatch (t_frang.test_host_required.FrangHostRequiredH2TestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tempesta/tempesta-test/t_frang/test_host_required.py", line 504, in test_h2_host_header_mismatch
    self._test_base_scenario(
  File "/home/tempesta/tempesta-test/t_frang/test_host_required.py", line 578, in _test_base_scenario
    self.assertEqual(
AssertionError: 0 != 1 : Frang limits warning is not shown

======================================================================
FAIL: test_h2_host_header_missing (t_frang.test_host_required.FrangHostRequiredH2TestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tempesta/tempesta-test/t_frang/test_host_required.py", line 496, in test_h2_host_header_missing
    self._test_base_scenario(
  File "/home/tempesta/tempesta-test/t_frang/test_host_required.py", line 578, in _test_base_scenario
    self.assertEqual(
AssertionError: 2 != 1 : Frang limits warning is not shown

======================================================================
FAIL: test_tls_connection_rate_on_the_limit (t_frang.test_tls_rate_burst.FrangTlsRateBurstTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tempesta/tempesta-test/t_frang/test_tls_rate_burst.py", line 539, in test_tls_connection_rate_on_the_limit
    self.assertEqual(
AssertionError: 1 != 0 : Expected nums of warnings in `journalctl`: 0, but got 1

…g limits: ip_block, request_rate, request_burst; some test in work

…ate, connection_burst; concurent_connection in progress

…tests. Removed tests for two clients (duplicate logic test_ip_block.py)

Added tests with different/same ip.

Replaced curl client for h2 tests. (it does not allow to change `authority` header).

…nection_rate`, `tls_connection_burst` tests. Added tests with different connection (tls and non-tls). Separated `tls_incomplete_connection_rate` tests.

Added tests for `concurrent_connections` with clear stats clean-up.

…ection_rate_burst.py. Deproxy is slow.

RomanBelozerov · 2022-11-23T13:17:16Z

@nickzaev

multiple_listeners group of test has already been fixed in #275, so you just need to rebase your branch onto master, resolving conflicts

Done.

The majority of test files contain some base class and just one class directly inherited from it, on the other hand the tests are being grouped by files (not by classes) and it is very unlikely that, say, there will be another class in t_frang/test_client_body_timeout.py any soon. So what's the point having that inheritance approach all over the place?

If we actually fall for that approach of using inheritance and all, the parts of starting off backends and Tempesta should be in setUp method of some base class and not get repeated for every test as it is for now.

All tests are parameterized and general logic is moved to base class. Unnecessary inheritance removed.

About a solid third of the tests code consists of almost identical Nginx configurations which either don't differ at all or just in very minor parts, so it looks more that we test Nginx rather than Tempesta :) Why not just reuse FrangTestCase everywhere possible at least just for consistency's sake?

Constants are massively overused, indeed, what's point of having such constants as ONE?

Nginx replaced to deproxy. Duplicate config and single constants removed.

RomanBelozerov · 2022-11-23T13:21:44Z

To sum up:

general logic is moved to FrangTestCase;
all tests are parametrized;
all tests are checked on CI;
all conversations are fixed;
client_body_timeout and client_header_timeout tests are grouped;
http_header_chunk_cnt and http_body_chunk_cnt tests are grouped;
connection_burst and connection_rate tests are grouped;
http_host_required h2 reworked with deproxy h2;
old frang tests removed;
added tests for ip_block directive after updating documentation;
added tests default value for multiple directives;
burst tests now use curl;
removed tests for multiple clients;

This PR requires changes #360

const-t

Looks good. Can be merged after fixing CI issues and small cleanup.

t_frang/test_host_required.py

pale-emperor

Works fine, except -R 3 args problem

…port_in_uri`, `test_host_header_no_port_in_host`. Port 80 is optional, therefore we expect 200 response for these tests.

KonsKo marked this pull request as ready for review August 5, 2022 23:26

KonsKo requested a review from nickzaev August 5, 2022 23:29

KonsKo assigned ProshNad Aug 9, 2022

krizhanovsky requested review from const-t and krizhanovsky August 22, 2022 14:51

krizhanovsky requested changes Aug 25, 2022

View reviewed changes

krizhanovsky removed the request for review from const-t September 26, 2022 20:52

krizhanovsky requested changes Sep 28, 2022

View reviewed changes

nickzaev suggested changes Oct 9, 2022

View reviewed changes

krizhanovsky mentioned this pull request Oct 10, 2022

reworked cache directory with new framework #318

Merged

ProshNad requested review from krizhanovsky and nickzaev October 12, 2022 14:11

const-t reviewed Oct 13, 2022

View reviewed changes

t_frang/test_host_required.py Outdated Show resolved Hide resolved

krizhanovsky assigned RomanBelozerov Oct 13, 2022

krizhanovsky reviewed Oct 14, 2022

View reviewed changes

t_frang/test_host_required.py Outdated Show resolved Hide resolved

t_frang/test_http_ct_vals.py Outdated Show resolved Hide resolved

krizhanovsky mentioned this pull request Oct 14, 2022

Inversed logic for http_trailer_split_allowed tempesta-tech/tempesta#1725

Closed

krizhanovsky requested a review from pale-emperor October 14, 2022 23:00

krizhanovsky approved these changes Oct 14, 2022

View reviewed changes

b3b added a commit that referenced this pull request Nov 11, 2022

Logic inverted for http_trailer_split_allowed tests (#265)

64db18e

RomanBelozerov mentioned this pull request Nov 18, 2022

fix: deproxy-h2 does not receive 4xx response #356

Merged

KonsKo added 8 commits November 21, 2022 16:41

test config generator

7d5f4dd

configs moved inside py files according PR 223; create tests for fran…

0a64946

…g limits: ip_block, request_rate, request_burst; some test in work

corrected tests: request_rate, request_burst; new tests: connection_r…

37bcdb2

…ate, connection_burst; concurent_connection in progress

gitignore updated: removed execcive files

5baf7f1

added test header cnt; some changes related to work process

0e2103d

fix test header cnt

91570a6

fix test header cnt

d9e05c0

added tests - to save, in progress

da7f5af

RomanBelozerov added 10 commits November 21, 2022 16:42

parametrization of http_header_chunk_cnt and http_body_chunk_cnt …

b1ff29f

…tests. Removed tests for two clients (duplicate logic test_ip_block.py)

parametrization of http_method_override_allowed tests.

fbcc238

parametrization of request_rate and request_burst tests.

7a0eb99

Added tests with different/same ip.

parametrization of host_required tests.

09fa95f

Replaced curl client for h2 tests. (it does not allow to change `authority` header).

parametrization of connection_rate, connection_burst and `tls_con…

40aa94b

…nection_rate`, `tls_connection_burst` tests. Added tests with different connection (tls and non-tls). Separated `tls_incomplete_connection_rate` tests.

removed old config for miltiple_listeners

0753b04

parametrization of concurrent_connections tests.

f8f7ede

Added tests for `concurrent_connections` with clear stats clean-up.

updated __init__.py

64a4897

added tests for http_resp_code_block with one client.

aa14c8a

removed old tests (duplicate) and updated tests_disabled.json

3b32992

RomanBelozerov force-pushed the kk-test branch from 64db18e to 3b32992 Compare November 21, 2022 13:25

RomanBelozerov mentioned this pull request Nov 22, 2022

fix: deproxy client does not start several times on local network (temporary solution) #360

Merged

RomanBelozerov added 5 commits November 23, 2022 12:12

added timeout for frang tests. dmesg needs time to output warnings

41000d4

parametrization of http_host_required h2 tests.

b01c362

Replaced deproxy to curl for test_request_rate_burst.py and test_conn…

b933781

…ection_rate_burst.py. Deproxy is slow.

updated tests_disabled.json

2cabcb5

changed limits for burst and rate according to PR comments.

fbf7159

krizhanovsky unassigned ProshNad Nov 23, 2022

disabled http_host_required h2 tests

06d6d9b

const-t approved these changes Nov 25, 2022

View reviewed changes

t_frang/test_host_required.py Show resolved Hide resolved

pale-emperor approved these changes Nov 25, 2022

View reviewed changes

http_host_required test logic is inverted for `test_host_header_no_…

070e8f7

…port_in_uri`, `test_host_header_no_port_in_host`. Port 80 is optional, therefore we expect 200 response for these tests.

nickzaev approved these changes Nov 25, 2022

View reviewed changes

number of threads is reduced for curl. Tests sometimes fail on CI.

36bc1a2

RomanBelozerov merged commit be346f9 into master Nov 25, 2022

RomanBelozerov deleted the kk-test branch November 25, 2022 13:17

This was referenced Nov 25, 2022

Functional test for Frang tempesta-tech/tempesta#673

Closed

Limit the number of log records for security events tempesta-tech/tempesta#1749

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests Frang #265

Tests Frang #265

KonsKo commented Aug 5, 2022 •

edited by krizhanovsky

Loading

ProshNad commented Aug 22, 2022

krizhanovsky left a comment

krizhanovsky left a comment

nickzaev left a comment •

edited

Loading

nickzaev Oct 9, 2022

ProshNad Oct 11, 2022

nickzaev Oct 9, 2022

ProshNad Oct 12, 2022

RomanBelozerov Nov 23, 2022

nickzaev Oct 9, 2022

ProshNad Oct 12, 2022

nickzaev Oct 9, 2022

ProshNad Oct 12, 2022

nickzaev Oct 9, 2022

ProshNad Oct 12, 2022

krizhanovsky left a comment

pale-emperor commented Oct 20, 2022

RomanBelozerov commented Nov 23, 2022

RomanBelozerov commented Nov 23, 2022 •

edited

Loading

const-t left a comment

pale-emperor left a comment

Tests Frang #265

Tests Frang #265

Conversation

KonsKo commented Aug 5, 2022 • edited by krizhanovsky Loading

ProshNad commented Aug 22, 2022

krizhanovsky left a comment

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

nickzaev left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

pale-emperor commented Oct 20, 2022

RomanBelozerov commented Nov 23, 2022

RomanBelozerov commented Nov 23, 2022 • edited Loading

const-t left a comment

Choose a reason for hiding this comment

pale-emperor left a comment

Choose a reason for hiding this comment

KonsKo commented Aug 5, 2022 •

edited by krizhanovsky

Loading

nickzaev left a comment •

edited

Loading

RomanBelozerov commented Nov 23, 2022 •

edited

Loading