pistacheio / pistache

A high-performance REST toolkit written in C++
https://pistacheio.github.io/pistache/
Apache License 2.0
3.12k stars 690 forks source link

test: listener TLS handshake timeout test #1117

Closed Tachi107 closed 1 year ago

Tachi107 commented 1 year ago

The new tls_handshake_timeout test is source-compatible with Pistache versions with and without the fix in #1115, so that it is easy to test if the bug reproduces with and without that patch applied.

Tachi107 commented 1 year ago

The test doesn't currently pass, and won't pass until my suggestion of lowering the default TLS handshake timeout is applied, which is the only way to make the test source-compatible with unpatched versions.

I can also add a test checking a non-default timeout though

kiplingw commented 1 year ago

This looks great @Tachi107. But it looks like the listener_tls_test test isn't being run in CI?

Tachi107 commented 1 year ago

Yeah, the library is built without TLS support in CI. I can add a job that compiles that variant too

Tachi107 commented 1 year ago

And autopkgtest is failing because we have a patch in the debian branch which disables GMock 1.11 because it's unavailable in old Ubuntu versions we support. I can tweak that patch as well.

kiplingw commented 1 year ago

Ok perfect. I didn't realize that we didn't test in CI without TLS. Is there any reason why we don't do this? Presumably that would exhaust more code paths.

Tachi107 commented 1 year ago

I've pushed https://github.com/pistacheio/pistache/commit/ad946ec5ed458963ba85878c19a60c4d9856a202 which should fix the autopkgtest issue

kiplingw commented 1 year ago

Looks like there's also some missing dependencies for the clang CI:

ld.lld: error: cannot open /usr/lib/llvm-14/lib/clang/14.0.6/lib/linux/libclang_rt.asan_static-x86_64.a: No such file or directory
ld.lld: error: cannot open /usr/lib/llvm-14/lib/clang/14.0.6/lib/linux/libclang_rt.profile-x86_64.a: No such file or directory
Tachi107 commented 1 year ago

Looks like there's also some missing dependencies for the clang CI

That's because of recent changes in the llvm-toolchain-14 Debian package, I'll add the required dependency on libclang-rt-dev

Edit: done

codecov-commenter commented 1 year ago

Codecov Report

Base: 78.38% // Head: 78.45% // Increases project coverage by +0.07% :tada:

Coverage data is based on head (2249d6c) compared to base (05deeb4). Patch has no changes to coverable lines.

:exclamation: Current head 2249d6c differs from pull request most recent head a8d660c. Consider uploading reports for the commit a8d660c to get more accurate results

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1117 +/- ## ========================================== + Coverage 78.38% 78.45% +0.07% ========================================== Files 52 53 +1 Lines 6698 6879 +181 ========================================== + Hits 5250 5397 +147 - Misses 1448 1482 +34 ``` | [Impacted Files](https://codecov.io/gh/pistacheio/pistache/pull/1117?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [include/pistache/transport.h](https://codecov.io/gh/pistacheio/pistache/pull/1117?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-aW5jbHVkZS9waXN0YWNoZS90cmFuc3BvcnQuaA==) | `53.75% <0.00%> (-6.25%)` | :arrow_down: | | [src/common/transport.cc](https://codecov.io/gh/pistacheio/pistache/pull/1117?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2NvbW1vbi90cmFuc3BvcnQuY2M=) | `69.09% <0.00%> (-0.14%)` | :arrow_down: | | [src/common/string\_logger.cc](https://codecov.io/gh/pistacheio/pistache/pull/1117?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2NvbW1vbi9zdHJpbmdfbG9nZ2VyLmNj) | `100.00% <0.00%> (ø)` | | | [src/common/utils.cc](https://codecov.io/gh/pistacheio/pistache/pull/1117?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2NvbW1vbi91dGlscy5jYw==) | `84.21% <0.00%> (ø)` | | | [src/common/cookie.cc](https://codecov.io/gh/pistacheio/pistache/pull/1117?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2NvbW1vbi9jb29raWUuY2M=) | `94.85% <0.00%> (+0.02%)` | :arrow_up: | | [src/common/net.cc](https://codecov.io/gh/pistacheio/pistache/pull/1117?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2NvbW1vbi9uZXQuY2M=) | `87.54% <0.00%> (+0.04%)` | :arrow_up: | | [src/common/reactor.cc](https://codecov.io/gh/pistacheio/pistache/pull/1117?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2NvbW1vbi9yZWFjdG9yLmNj) | `79.09% <0.00%> (+0.07%)` | :arrow_up: | | [src/common/stream.cc](https://codecov.io/gh/pistacheio/pistache/pull/1117?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2NvbW1vbi9zdHJlYW0uY2M=) | `84.53% <0.00%> (+0.08%)` | :arrow_up: | | [src/common/timer\_pool.cc](https://codecov.io/gh/pistacheio/pistache/pull/1117?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2NvbW1vbi90aW1lcl9wb29sLmNj) | `93.65% <0.00%> (+0.10%)` | :arrow_up: | | [src/common/http\_defs.cc](https://codecov.io/gh/pistacheio/pistache/pull/1117?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2NvbW1vbi9odHRwX2RlZnMuY2M=) | `82.05% <0.00%> (+0.15%)` | :arrow_up: | | ... and [8 more](https://codecov.io/gh/pistacheio/pistache/pull/1117?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

Tachi107 commented 1 year ago

I also took some time to fix the RHEL 8 job. The ninja binary installed with pip3 was somehow malfunctioning on that RHEL version (ninja --version reported the correct value, but then exited with status 245, never seen before), and installing the official binaries fixed the issue. Odd.

Anyway, the autopkgtest job is now correctly failing, signalling that the new test is working.

kiplingw commented 1 year ago

Great work @Tachi107. @dennisjenkins75 are you fine with this?

dennisjenkins75 commented 1 year ago

The test has a 20s pause in it? I didn't see a call to set the timeout when creating the server. Can you set the server to timeout after a shorter period, maybe 3s; and the have the test check after 5s? Waiting 20s seems like a long time dor a unit test to nap.

Otherwise, it seems fine.

Tachi107 commented 1 year ago

@dennisjenkins75 the test checks that the default TLS handshake timeout (which should be 10 seconds, but currently is 60) is less than 20 seconds.

I'll add another test that sets a custom timeout of e.g. 3 seconds and checks that it actually is three seconds.

Tachi107 commented 1 year ago

I've pushed a commit containing a test which uses the API I proposed in #1115, which obviously does not compile yet

kiplingw commented 1 year ago

Nice job @Tachi107. I'm fine with this. Feel free to merge if you're comfortable with your work as well.

kiplingw commented 1 year ago

With #1115 merged @Tachi107, are you ready to merge the unit test?

Tachi107 commented 1 year ago

Just a sec, I'd like to first rebase onto the master branch to see the tests pass

kiplingw commented 1 year ago

In case it helps, there are some build dependency problems I am seeing when I try to run locally in autopkgtest, but your changes may have already fixed this. I haven't looked too deeply into it:

...
dpkg-buildpackage: info: source package pistache
dpkg-buildpackage: info: source version 0.0.5-1
dpkg-buildpackage: info: source distribution UNRELEASED
dpkg-buildpackage: info: source changed by Kip Warner <kip@thevertigo.com>
 dpkg-source --before-build .
dpkg-buildpackage: info: host architecture amd64
dpkg-checkbuilddeps: error: Unmet build dependencies: debhelper-compat (= 13) meson (>= 0.50.0) libcurl4-openssl-dev libgmock-dev (>=v
dpkg-buildpackage: warning: build dependencies/conflicts unsatisfied; aborting
dpkg-buildpackage: warning: (Use -d flag to override.)
autopkgtest [12:06:04]:  - - - - - - - - - - running shell - - - - - - - - - -
You can now log into the VM through the serial terminal.
Depending on which terminal program you have installed, you can use one of

    ssh -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -p 10022 ubuntu@localhost
    minicom -D unix#/tmp/autopkgtest-qemu.y9mdg6p9/ttyS0
    nc -U /tmp/autopkgtest-qemu.y9mdg6p9/ttyS0
    socat - UNIX-CONNECT:/tmp/autopkgtest-qemu.y9mdg6p9/ttyS0

The tested source package is in /

Press Enter to resume running tests.

Are you able to verify on your end?

Which reminds me, we need to bump the version in debian/changelog.

Tachi107 commented 1 year ago

In case it helps, there are some build dependency problems I am seeing when I try to run locally in autopkgtest, but your changes may have already fixed this. I haven't looked too deeply into it:

dpkg-checkbuilddeps: error: Unmet build dependencies: debhelper-compat (= 13) meson (>= 0.50.0) libcurl4-openssl-dev libgmock-dev (>=v

Are you able to verify on your end?

Uhm, I'm not sure this is related to this pr. How are you running autopkgtest? What virt-server (e.g. podman, chroot, lxc...) are you using?

Which reminds me, we need to bump the version in debian/changelog.

Yeah, I wonder if we can automate it.

Tachi107 commented 1 year ago

All tests passed, merging

kiplingw commented 1 year ago

Uhm, I'm not sure this is related to this pr. How are you running autopkgtest? What virt-server (e.g. podman, chroot, lxc...) are you using?

It's probably not related. I'm running autopkgtest via QEMU with KVM enabled. Here is my configuration:

--shell-fail
--apt-upgrade
--setup-commands=/home/kip/Projects/sbuild/scripts/setup.sh
--
qemu
--ram-size=8192
--cpus=8
--show-boot
/home/kip/Projects/sbuild/images/autopkgtest-jammy-amd64.img
--qemu-options=-enable-kvm
Tachi107 commented 1 year ago

Can we continue on IRC?