http2: omit server name when HTTP2 host is IP address by islandryu · Pull Request #56530 · nodejs/node

islandryu · 2025-01-09T08:41:18Z

nodejs-github-bot · 2025-01-09T08:41:24Z

Review requested:

@nodejs/http2
@nodejs/net

islandryu · 2025-01-09T08:43:34Z

+    let hostName = socket.servername;
+    if (hostName === null || hostName === false) {
+      if (socket.remoteFamily === 'IPv6') {
+        hostName = `[${socket.remoteAddress}]`;
+      } else {
+        hostName = socket.remoteAddress;
+      }


Host: the value sent in Server Name Indication (SNI) ([RFC6066],
Section 3) converted to lower case; if SNI is not present, the
remote address of the connection (i.e., the server's IP address)

According to RFC 8336, when the server name is not set, the server's address should be used instead.

codecov · 2025-01-09T10:15:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.19%. Comparing base (67b647e) to head (72eb4dc).
Report is 131 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56530      +/-   ##
==========================================
+ Coverage   88.54%   89.19%   +0.65%     
==========================================
  Files         657      662       +5     
  Lines      190689   191808    +1119     
  Branches    36602    36926     +324     
==========================================
+ Hits       168844   171089    +2245     
+ Misses      15029    13564    -1465     
- Partials     6816     7155     +339

Files with missing lines	Coverage Δ
lib/internal/http2/core.js	`95.54% <100.00%> (+<0.01%)`	⬆️

... and 140 files with indirect coverage changes

lpinca · 2025-01-13T11:43:45Z

+  })
+  );
+}, 2));
+server.on('close', common.mustCall());


Suggested change

server.on('close', common.mustCall());

lpinca · 2025-01-13T11:49:28Z

+    let data = '';
+    req.setEncoding('utf8');
+    req.on('data', (d) => data += d);
+    req.on('end', common.mustCall(() => {


Can't the requests be done in parallel? Then server can be closed with something in the listeners of 'end' events

if (++done === 2) server.close();

I've made some adjustments to process the requests in parallel.

mcollina

lgtm

nodejs-github-bot · 2025-01-13T12:37:10Z

CI: https://ci.nodejs.org/job/node-test-pull-request/64472/

ghost · 2025-01-14T04:32:11Z

+  const ipv4Url = `https://127.0.0.1:${server.address().port}`;
+  const ipv6Url = `https://[::1]:${server.address().port}`;
+  handleRequest(ipv4Url);
+  if (common.hasIPv6) handleRequest(ipv6Url);


I added this branch because the EADDRNOTAVAIL error was occurring in the tests.

Co-authored-by: Luigi Pinca <luigipinca@gmail.com>

ghost

RSLGTM

ghost · 2025-01-14T09:40:21Z

CI: https://ci.nodejs.org/job/node-test-pull-request/64500/

ghost · 2025-01-14T12:53:07Z

-      if (++done === 2) server.close();
+      if (++done === testCount) server.close();


Sorry, I missed that the number of tests changes if you don't test for IPv6.

ghost · 2025-01-14T13:02:02Z

CI: https://ci.nodejs.org/job/node-test-pull-request/64501/

ghost · 2025-01-18T19:31:27Z

Commit Queue failed

- Loading data for nodejs/node/pull/56530
✔  Done loading data for nodejs/node/pull/56530
----------------------------------- PR info ------------------------------------
Title      http2: omit server name when HTTP2 host is IP address (#56530)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     islandryu:fix/http2 -> nodejs:main
Labels     http2, needs-ci, commit-queue-squash
Commits    4
 - http2: omit server name when HTTP2 host is IP address
 - fix test
 - Update test/parallel/test-http2-ip-address-host.js
 - fix test
Committers 2
 - islandryu <shimaryuhei@gmail.com>
 - GitHub <noreply@github.com>
PR-URL: https://github.com/nodejs/node/pull/56530
Fixes: https://github.com/nodejs/node/issues/56189
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/56530
Fixes: https://github.com/nodejs/node/issues/56189
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - fix test
   ℹ  This PR was created on Thu, 09 Jan 2025 08:41:18 GMT
   ✔  Approvals: 3
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/56530#pullrequestreview-2546485278
   ✔  - Yongsheng Zhang (@ZYSzys): https://github.com/nodejs/node/pull/56530#pullrequestreview-2548929308
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/56530#pullrequestreview-2548972673
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-01-14T13:02:02Z: https://ci.nodejs.org/job/node-test-pull-request/64501/
- Querying data for job/node-test-pull-request/64501/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/12846830944

ghost · 2025-01-20T04:21:08Z

@lpinca @mcollina @ZYSzys
Commit queue is failing.
Is there anything I need to do with this PR?

ghost · 2025-01-20T14:59:30Z

Landed in fdad2fa

http2: omit server name when HTTP2 host is IP address

ed64f1c

Fixes: nodejs#56189

nodejs-github-bot added http2 Issues and PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Jan 9, 2025

islandryu commented Jan 9, 2025

View reviewed changes

lpinca reviewed Jan 13, 2025

View reviewed changes

mcollina approved these changes Jan 13, 2025

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 13, 2025

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 13, 2025

fix test

46bcf78

islandryu commented Jan 14, 2025

View reviewed changes

ZYSzys approved these changes Jan 14, 2025

View reviewed changes

lpinca reviewed Jan 14, 2025

View reviewed changes

Comment thread test/parallel/test-http2-ip-address-host.js Outdated

Update test/parallel/test-http2-ip-address-host.js

36d945b

Co-authored-by: Luigi Pinca <luigipinca@gmail.com>

lpinca approved these changes Jan 14, 2025

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 14, 2025

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 14, 2025

fix test

72eb4dc

lpinca added the commit-queue-squash PRs the Commit Queue should land as one squashed commit. label Jan 14, 2025

islandryu commented Jan 14, 2025

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 14, 2025

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 14, 2025

lpinca added the commit-queue PRs queued for automated landing through the Commit Queue. label Jan 18, 2025

nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Jan 18, 2025

jasnell closed this Jan 20, 2025

github-actions Bot mentioned this pull request Jan 28, 2025

2025-01-30, Version 23.7.0 (Current) #56800

Merged

github-actions Bot mentioned this pull request Feb 4, 2025

22.14.0 'Jod' (LTS) release proposal #56910

Merged

marco-ippolito mentioned this pull request Mar 6, 2025

v20.19.0 proposal #57349

Merged

		if (++done === 2) server.close();
		if (++done === testCount) server.close();

Uh oh!

Uh oh!

Conversation

islandryu commented Jan 9, 2025

Uh oh!

nodejs-github-bot commented Jan 9, 2025

Uh oh!

islandryu Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lpinca Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

lpinca Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

islandryu Jan 14, 2025

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jan 13, 2025

Uh oh!

ghost Jan 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jan 14, 2025

Uh oh!

ghost Jan 14, 2025

Choose a reason for hiding this comment

Uh oh!

ghost commented Jan 14, 2025

Uh oh!

ghost commented Jan 18, 2025

Uh oh!

ghost commented Jan 20, 2025

Uh oh!

ghost commented Jan 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov Bot commented Jan 9, 2025 •

edited

Loading