[PATCH 02 of 11] Tests: removed unneeded require from proxy_ssl_keepalive.t

[Sergey Kandaurov]

> On 3 May 2023, at 05:26, Maxim Dounin <mdounin at 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 <mdounin at mdounin.ru> wrote:
>>> 
>>> # HG changeset patch
>>> # User Maxim Dounin <mdounin at 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 at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

-- 
Sergey Kandaurov