Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

plain configs for multiple listeners #275

Merged
merged 11 commits into from
Sep 6, 2022

Conversation

RomanBelozerov
Copy link
Contributor

@RomanBelozerov RomanBelozerov commented Aug 10, 2022

I added plain configs for multiple listeners. And also changed path for tls certificates to tmp/tempesta. I left all the tests disabled

Copy link
Contributor

@nickzaev nickzaev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the changes required multiple_listeners.test_multiple_listening passes, but two other ones fail. Do they pass for you?

@nickzaev
Copy link
Contributor

Also don't forget to delete this test suite from tests_disabled.json when it passes.

@RomanBelozerov
Copy link
Contributor Author

With the changes required multiple_listeners.test_multiple_listening passes, but two other ones fail. Do they pass for you?

No. Only `multiple_listeners.test_multiple_listening' passes for me.

@nickzaev
Copy link
Contributor

No. Only `multiple_listeners.test_multiple_listening' passes for me.

Is there an issue filed regarding those fails? Do you have any clues on why them fail?

@RomanBelozerov
Copy link
Contributor Author

No. Only `multiple_listeners.test_multiple_listening' passes for me.

Is there an issue filed regarding those fails? Do you have any clues on why them fail?

Yes, issue #1624. test_mixed failed for this reason. I have tried to understand the reason for fail of test_on_the_fly. But I don't know it yet.

Copy link
Contributor

@krizhanovsky krizhanovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to fix and unmask the tests when tempesta-tech/tempesta#1643 is merged.


listen 127.0.0.4:443 proto=h2;
listen 127.0.0.4:4433 proto=https;

Copy link
Contributor

@krizhanovsky krizhanovsky Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With tempesta-tech/tempesta#1643 tempesta-tech/tempesta#1624 is fixed, but the test must be reworked. For Tempesta FW config:

listen 127.0.0.4:443 proto=h2;
listen 127.0.0.4:4433 proto=https;

srv_group default {
	server 127.0.0.1:8000;
}

vhost tempesta-cat {
	proxy_pass default;
}

tls_match_any_server_name;
tls_certificate /root/tempesta/etc/tfw-root.crt;
tls_certificate_key /root/tempesta/etc/tfw-root.key;

cache 0;
block_action attack reply;

http_chain {
	-> tempesta-cat;
}

The tests, which must obviously pass:

# curl -Ikf --http2 https://127.0.0.4:443/
HTTP/2 200 
date: Wed, 17 Aug 2022 23:59:45 GMT
content-type: text/plain
content-length: 0
via: 2.0 tempesta_fw (Tempesta FW pre-0.7.0)
server: Tempesta FW/pre-0.7.0
# curl -Ikf --http1.1 https://127.0.0.4:4433/
HTTP/1.1 200 OK
Date: Thu, 18 Aug 2022 00:24:01 GMT
Content-Type: text/plain
Content-Length: 0
via: 1.1 tempesta_fw (Tempesta FW pre-0.7.0)
Server: Tempesta FW/pre-0.7.0

This test must not pass anymore

# curl -Ikf --http1.1 https://127.0.0.4:443/
curl: (35) error:0A000460:SSL routines::reason(1120)

and check in dmesg for [tempesta tls] Warning: [::ffff:127.0.0.1] ClientHello: cannot find matching ALPN for http/1.1.

The last test is more complicated

# curl -Ikf --http2 https://127.0.0.4:4433/
HTTP/1.1 200 OK
Date: Thu, 18 Aug 2022 00:17:13 GMT
Content-Type: text/plain
Content-Length: 0
via: 1.1 tempesta_fw (Tempesta FW pre-0.7.0)
Server: Tempesta FW/pre-0.7.0

note that the response is HTTP/1 while we have requested HTTP/2. We still can't enforce curl to use HTTP/2 only even with --http2-prior-knowledge instead of ---http2. I think we should leave the test with the comment that there is an ALPN negotiation (actually curl sends the both h2 and https in ALPN) and use the new h2 library to send strictly HTTP/2 request and make sure that it isn't accepted on the HTTPS socket.

Copy link
Contributor

@krizhanovsky krizhanovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to fix anything for now, but please in future pull requests leave more detailed commit messages. For example returned client wrk in test_on_the_fly.py doesn't explain how this fixes the reported issue.

@RomanBelozerov RomanBelozerov force-pushed the rb-238-plain-configs-for-multiple-listeners branch from c8ac99d to 94180bd Compare August 26, 2022 08:32
@RomanBelozerov
Copy link
Contributor Author

No need to fix anything for now, but please in future pull requests leave more detailed commit messages. For example returned client wrk in test_on_the_fly.py doesn't explain how this fixes the reported issue.

OK, I'll write more detailed commit messages. I rebased from master and did force push, it should work now.

…or make_curl_request method in multiple_listeners.test_mixed.py
Copy link
Contributor

@krizhanovsky krizhanovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm that all the tests from multiple_listeners now pass on my branch tempesta-tech/tempesta#1643 .

In fact, quite a good work - the test revealed relatively old race bug tempesta-tech/tempesta#1643 (comment)

@krizhanovsky krizhanovsky force-pushed the rb-238-plain-configs-for-multiple-listeners branch from 44a7ddf to 0d79730 Compare September 4, 2022 20:35
Copy link
Contributor

@nickzaev nickzaev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, all 4 tests now pass.

@RomanBelozerov RomanBelozerov merged commit bf08d9e into master Sep 6, 2022
@RomanBelozerov RomanBelozerov deleted the rb-238-plain-configs-for-multiple-listeners branch September 7, 2022 06:57
@RomanBelozerov RomanBelozerov linked an issue Sep 7, 2022 that may be closed by this pull request
@RomanBelozerov RomanBelozerov mentioned this pull request Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revert plain configs for multiple_listeners
3 participants