Skip to content

feature: added --- sni Support TLS http2 and SNI #68

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.

Sign up for GitHub

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

Open
vislee wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feature: added --- sni Support TLS http2 and SNI #68

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d0b5b02
add feature test http2 upport sni
Sep 23, 2017
e6a2495
add instructions for SNI
Sep 23, 2017
abad786
fix style
Sep 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions lib/Test/Nginx/Socket.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2121,8 +2121,11 @@ sub gen_curl_cmd_from_req ($$) {
push @args, '-sS';
}

if (use_http2($block)) {
my $isHttp2 = use_http2($block);
if ($isHttp2 && $isHttp2 == 1) {
Copy link
Member

@agentzh agentzh Sep 23, 2017

Choose a reason for hiding this comment

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

We do not use "camel case" identifier names in this Perl project. We use "snake case" consistently.

Copy link
Author

@vislee vislee Sep 23, 2017

Choose a reason for hiding this comment

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

Sorry, I'll change it.

push @args, '--http2', '--http2-prior-knowledge';
} elsif ($isHttp2 && $isHttp2 == 2) {
push @args, '-k', '--http2', '--http2-prior-knowledge', '--resolve', "$ServerName:$ServerPortForClient:$ServerAddr",
}

if ($meth eq 'HEAD') {
Expand Down Expand Up @@ -2193,6 +2196,10 @@ sub gen_curl_cmd_from_req ($$) {
$link = "http://$server:$port$uri";
}

if (use_http2($block) == 2) {
$link = "https://$ServerName:$ServerPortForClient$uri";
Copy link
Member

@agentzh agentzh Sep 23, 2017

Choose a reason for hiding this comment

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

SSL support need proper server certificate and private keys. And it's a burden for the test writer to prepare such things themselves. We should make HTTP/2 over TLS test mode work on most of the existing test suite written for HTTP/1. So we should automatically generate such self-signed server certificate/key pair ourselves in the test scaffold.

}

push @args, $link;

return \@args;
Expand Down Expand Up @@ -3977,7 +3984,7 @@ starts. The following environment variables are supported by this module:
=head2 TEST_NGINX_USE_HTTP2

Enables the "http2" test mode by enforcing using the (plain text) HTTP/2 protocol to send the
test request.
test request. Also, you can set sni section using the TLS HTTP/2 protocol and SNI(Server Name Indication).

Under the hood, the test scaffold uses the `curl` command-line utility to do the wire communication
with the NGINX server. The `curl` utility must be recent enough to support both the C<--http2>
Expand All @@ -3991,6 +3998,10 @@ One can enable HTTP/2 mode for an individual test block by specifying the L<http

--- http2

Enable SNI mode for an individual test block by specifying the L<sni> section, as in

--- sni
Copy link
Member

@agentzh agentzh Sep 23, 2017

Choose a reason for hiding this comment

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

I'm afraid --- tls is a better name than --- sni since we are not specifying a concrete SNI name here anyway.

Copy link
Author

@vislee vislee Sep 23, 2017

Choose a reason for hiding this comment

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

Thanks for your review and comments. I'll change it.


=head2 TEST_NGINX_VERBOSE

Controls whether to output verbose debugging messages in Test::Nginx. Default to empty.
Expand Down
15 changes: 14 additions & 1 deletion lib/Test/Nginx/Util.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -942,8 +942,11 @@ _EOC_

my $listen_opts = '';

if (use_http2($block)) {
my $isHttp2 = use_http2($block);
if ($isHttp2 && $isHttp2 == 1) {
$listen_opts .= " http2";
} elsif ($isHttp2 && $isHttp2 == 2) {
$listen_opts .= " ssl http2";
}

print $out <<_EOC_;
Expand Down Expand Up @@ -2499,6 +2502,11 @@ sub use_http2 ($) {
bail_out("cannot use --- http2 with --- pipelined_requests");
}

if (defined $block->sni) {
$block->set_value("test_nginx_enabled_http2_sni", 1);
return 2;
Copy link
Member

@agentzh agentzh Sep 23, 2017

Choose a reason for hiding this comment

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

The use_http2 function returns a boolean value. It's not a good idea to make it return more complex encodings.

Copy link
Author

@vislee vislee Sep 23, 2017

Choose a reason for hiding this comment

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

Sorry, I'll change it.

}

$block->set_value("test_nginx_enabled_http2", 1);
return 1;
}
Expand Down Expand Up @@ -2527,6 +2535,11 @@ sub use_http2 ($) {
return undef;
}

if ($block->sni) {
$block->set_value("test_nginx_enabled_http2_sni", 1);
return 2;
}

$block->set_value("test_nginx_enabled_http2", 1);
return 1;
}
Expand Down