> On 3 May 2023, at 05:26, Maxim Dounin <mdou...@mdounin.ru> wrote:
>
> Hello!
>
> On Tue, May 02, 2023 at 05:49:23PM +0400, Sergey Kandaurov wrote:
>
>>
>>> On 17 Apr 2023, at 07:31, Maxim Dounin <mdou...@mdounin.ru> wrote:
>>>
>>> # HG changeset patch
>>> # User Maxim Dounin <mdou...@mdounin.ru>
>>> # Date 1681702250 -10800
>>> # Mon Apr 17 06:30:50 2023 +0300
>>> # Node ID 6f0148ef1991d92a003c8529c8cce9a8dd49e706
>>> # Parent a01b7d84f4355073a00f43760fc512e03b4452c3
>>> Tests: removed unneeded require from proxy_ssl_keepalive.t.
>>>
>>> diff --git a/proxy_ssl_keepalive.t b/proxy_ssl_keepalive.t
>>> --- a/proxy_ssl_keepalive.t
>>> +++ b/proxy_ssl_keepalive.t
>>> @@ -22,9 +22,6 @@ use Test::Nginx;
>>> select STDERR; $| = 1;
>>> select STDOUT; $| = 1;
>>>
>>> -eval { require IO::Socket::SSL; };
>>> -plan(skip_all => 'IO::Socket::SSL not installed') if $@;
>>> -
>>> my $t = Test::Nginx->new()->has(qw/http http_ssl proxy upstream_keepalive/)
>>> ->has_daemon('openssl')->plan(3)
>>> ->write_file_expand('nginx.conf', <<'EOF');
>>
>> We can as well remove it from h2_ssl_proxy_cache.t and h2_ssl_variables.t
>> after 45c80276d691, HTTP2 package handles that for us.
>> (Same approach used for the crypto layer of HTTP/3 tests.)
>
> Both h2_ssl_proxy_cache.t and h2_ssl_variables.t actually use
> IO::Socket::SSL, even if the actual use is within
> Test::Nginx::HTTP2, and will behave incorrectly if IO::Socket::SSL
> is not available (as currently written, they won't fail, but will
> skip individual tests with incorrect reasoning). As such, they do
> need these requires (and the corresponding plan(skip_all) call if
> IO::Socket::SSL is not available).
>
I tend to disagree about reasoning: it's most specific, not incorrect.
Your mileage may vary.
HTTP/2 implementation requires to be negotiated via ALPN
(NPN was a fallback, before it was decommissioned in 1.21.4),
otherwise it continues as if it were HTTP/1.1.
So we have to check in tests that HTTP/2 was negotiated,
and as such - we can provide most specific skip reasoning.
General checking for IO::Socket:SSL, if not installed, provides
generic skip reasoning, which in fact doesn't advise that installing
an arbitrary (old enough) IO::Socket:SSL may not be enough:
$ perl h2_ssl_variables.t
1..0 # SKIP IO::Socket::SSL not installed
After removing require from h2_ssl_variables.t:
$ perl h2_ssl_variables.t
1..10
ok 1 # skip OpenSSL NPN support required
ok 2 # skip OpenSSL ALPN support required
ok 3 # skip OpenSSL NPN support required
ok 4 # skip OpenSSL ALPN support required
ok 5 # skip OpenSSL NPN support required
ok 6 # skip OpenSSL ALPN support required
ok 7 # skip OpenSSL NPN support required
ok 8 # skip OpenSSL ALPN support required
ok 9 - no alerts
ok 10 - no sanitizer errors
This is somewhat similar to IO::Uncompress::Gunzip handling.
The difference is that gunzip test is run/skipped right here,
whereas IO::Socket::SSL check and skip are moved apart.
For comparison, SSL tests in mail_imap_ssl.t indeed require
IO::Socket::SSL directly in mail_imap_ssl.t, as otherwise
it won't resolve IO::Socket::SSL::SSL_VERIFY_NONE.
BTW, it's probably time to remove NPN from tests. Removing NPN
is a good reason to review the SSL flag of HTTP2::new_socket().
It has received a limited use (cf. h2_ssl.t, h2_ssl_verify_client.t),
it may have sense to just remove it.
> In contrast, proxy_ssl_keepalive.t does not use IO::Socket::SSL at
> all. All client connections initiated by the test are plain text,
> and SSL is only used within nginx itself. As such,
> IO::Socket::SSL is not required to run the test (and hence the
> patch).
>
> --
> Maxim Dounin
> http://mdounin.ru/
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel
--
Sergey Kandaurov
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
