add `h2` static responses tests #272

nickzaev · 2022-08-09T14:00:03Z

These tests aim to ensure that Tempesta generates correct static responses in most common use-cases using h2 protocol.

nickzaev · 2022-08-09T14:45:29Z

I've encountered 2 Tempesta bugs during poking around these tests:

http_rules.test_http_tables.H2Redirects fails with

[Tue Aug  9 14:03:04 2022] [tempesta fw] Warning: request dropped: cannot find appropriate virtual host: 127.0.0.1

If you uncomment #-> redirection_chain; line in the main chain, the test passes. Just to make sure that this behavior is not caused by poorly formed configuration, I've provided 2 additional temporary tests (that will be deleted after we resolve corresponding Tempesta issues) that have pretty much the same configurations (except for the TLS part): http_rules.test_http_tables.HttpRedirects and http_rules.test_http_tables.HttpsRedirects. They both pass. Note that both of them don't have -> redirection_chain; in the main chain, so this h2 case is isolated. From my understanding, what happens is that Tempesta cannot match the request to any vhost because it simply doesn't have Host field within the request. I haven't digged deep enough into Tempesta code yet, so I added checks for:

            hdr "Authority" == "tempesta-tech.com" -> redirection_chain;
            hdr ":authority" == "tempesta-tech.com" -> redirection_chain;
            hdr "authority" == "tempesta-tech.com" -> redirection_chain;

as well in case if that would help, but it didn't — the test still fails.

cache.test_cache.H2Cache fails with:

[Tue Aug  9 14:31:49 2022] [tempesta fw] Warning: Unable to transform HTTP/1.1 data into HTTP/2 format: free space exhausted (accumulated length: 78
[Tue Aug  9 14:31:49 2022] [tempesta tls] Warning: [::ffff:127.0.0.1] Bad TLS alert

This is a known Tempesta issue (#261, #1669).
Note that:

test (cache.test_cache.H2Cache) ... Amount of requests passed to the backend: 2
FAIL

basically means that requests get to the backend and Tempesta fails to return them back to the client.
Regarding that tempesta tls warning, I'm not aware of it's meaning at the moment, but [::ffff:127.0.0.1] is definitely not a valid IPv6 address, so that might be a separate issue.

krizhanovsky · 2022-08-11T20:29:58Z

cache/test_cache.py

@@ -3,6 +3,7 @@
 from __future__ import print_function
 from testers import functional
 from helpers import chains
+from framework import tester

 __author__ = 'Tempesta Technologies, Inc.'
 __copyright__ = 'Copyright (C) 2017 Tempesta Technologies, Inc.'


Please update the copyright year

krizhanovsky · 2022-08-11T20:33:29Z

cache/test_cache.py

+
+        got_response = client.wait_for_response(timeout=5)
+
+        print(f"Amount of requests passed to the backend: {len(self.get_server('deproxy').requests)}")


It seems here should be an assertion, that backend receives only 1 request. Also too wide line

krizhanovsky · 2022-08-11T21:05:22Z

Issue (1) looks like about tempesta-tech/tempesta#1630 - could you please disable the test pointing out the issue. I think just using authority in the rules should work, but it'd be good to leave the host rule pointing out to another chain/redirection to make sure that we process Host and authority in correct way (in terms of the cited issue).

I added comment 7 about Bad TLS alert message - this is some separate issue. ::ffff:127.0.0.1 is just a IPv4-mapped IPv6 address: Tempesta FW handles all addressed in IPv6 internally and if you use IPv4, then it maps the address(es) to IPv6.

const-t · 2022-08-12T09:54:41Z

Issue (1) looks like about tempesta-tech/tempesta#1630 - could you please disable the test pointing out the issue. I think just using authority in the rules should work, but it'd be good to leave the host rule pointing out to another chain/redirection to make sure that we process Host and authority in correct way (in terms of the cited issue).

I added comment 7 about Bad TLS alert message - this is some separate issue. ::ffff:127.0.0.1 is just a IPv4-mapped IPv6 address: Tempesta FW handles all addressed in IPv6 internally and if you use IPv4, then it maps the address(es) to IPv6.

No, it's not tempesta-tech/tempesta#1630, it's about matching pseudo-headers by http tables. Now we can't match any pseudo-headers by using hdr keyword in http chain. To match :authority we can use only Host keyword in http chain block.

krizhanovsky · 2022-08-12T11:41:47Z

With the slack discussion it's worth testing configuration like:

hdr "authority" == "tempesta-tech.com" -> one_chain;
host == "tempesta-tech.com" -> some_other_chain;

const-t · 2022-08-12T10:21:10Z

http_rules/test_http_tables.py

+        }
+
+        http_chain {
+            hdr "Host" == "tempesta-tech.com" -> redirection_chain;


Host can't be matched, because you don't send it. Is it expected?

const-t · 2022-08-12T12:08:27Z

cache/test_cache.py

+    clients = [{
+        'id': 'deproxy',
+        'type': 'deproxy_h2',
+        'addr': "${server_ip}",


I believe would be better to use ${tempesta_ip}, because tempesta and backend can be on different hosts.

These tests aim to ensure that Tempesta generates correct static responses in most common use-cases using `h2` protocol.

const-t

LGTM

nickzaev force-pushed the nz-h2-static-responses branch from 556d29d to d01c099 Compare August 9, 2022 14:17

nickzaev marked this pull request as draft August 9, 2022 15:03

krizhanovsky requested review from krizhanovsky and const-t August 11, 2022 13:02

krizhanovsky reviewed Aug 11, 2022

View reviewed changes

const-t reviewed Aug 12, 2022

View reviewed changes

nickzaev force-pushed the nz-h2-static-responses branch from d01c099 to 57112cd Compare September 4, 2022 14:54

nickzaev marked this pull request as ready for review September 4, 2022 14:54

add h2 static responses tests

14d3bff

These tests aim to ensure that Tempesta generates correct static responses in most common use-cases using `h2` protocol.

nickzaev force-pushed the nz-h2-static-responses branch from 57112cd to 14d3bff Compare September 9, 2022 14:47

const-t approved these changes Sep 21, 2022

View reviewed changes

nickzaev merged commit e0c5552 into master Sep 21, 2022

nickzaev deleted the nz-h2-static-responses branch September 21, 2022 11:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `h2` static responses tests #272

add `h2` static responses tests #272

nickzaev commented Aug 9, 2022

nickzaev commented Aug 9, 2022 •

edited

Loading

krizhanovsky Aug 11, 2022

krizhanovsky Aug 11, 2022

krizhanovsky commented Aug 11, 2022 •

edited

Loading

const-t commented Aug 12, 2022

krizhanovsky commented Aug 12, 2022

const-t Aug 12, 2022

const-t Aug 12, 2022

const-t left a comment


		got_response = client.wait_for_response(timeout=5)

		print(f"Amount of requests passed to the backend: {len(self.get_server('deproxy').requests)}")

add h2 static responses tests #272

add h2 static responses tests #272

Conversation

nickzaev commented Aug 9, 2022

nickzaev commented Aug 9, 2022 • edited Loading

krizhanovsky Aug 11, 2022

Choose a reason for hiding this comment

krizhanovsky Aug 11, 2022

Choose a reason for hiding this comment

krizhanovsky commented Aug 11, 2022 • edited Loading

const-t commented Aug 12, 2022

krizhanovsky commented Aug 12, 2022

const-t Aug 12, 2022

Choose a reason for hiding this comment

const-t Aug 12, 2022

Choose a reason for hiding this comment

const-t left a comment

Choose a reason for hiding this comment

add `h2` static responses tests #272

add `h2` static responses tests #272

nickzaev commented Aug 9, 2022 •

edited

Loading

krizhanovsky commented Aug 11, 2022 •

edited

Loading