You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/05/26 20:58:24 UTC
[GitHub] [trafficserver] shinrich opened a new pull request #7885: Close connection after every bad request
shinrich opened a new pull request #7885:
URL: https://github.com/apache/trafficserver/pull/7885
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7885: Close connection after every bad request
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7885:
URL: https://github.com/apache/trafficserver/pull/7885#issuecomment-850448935
Correct, this need to close the connection after a bad request only applies to HTTP/1.1 because we are worried about messing up the transaction framing. This is not an issue for HTTP/2, so we don't need to close the HTTP/2 session just because of one bad stream on that session.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7885: Close connection after every bad request for HTTP/1.1
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7885:
URL: https://github.com/apache/trafficserver/pull/7885#issuecomment-852340528
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7885: Close connection after every bad request
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7885:
URL: https://github.com/apache/trafficserver/pull/7885#issuecomment-849138392
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich edited a comment on pull request #7885: Close connection after every bad request for HTTP/1.1
Posted by GitBox <gi...@apache.org>.
shinrich edited a comment on pull request #7885:
URL: https://github.com/apache/trafficserver/pull/7885#issuecomment-852139571
Here is the case I am testing there. Thought I had added autests, but I must not have git-added them.
```
[shinrich@atslab02 ~]$ nc -C 127.0.0.1 8080
GET /x HTTP/1.1
host : bob
HTTP/1.1 400 Invalid HTTP Request
Date: Tue, 01 Jun 2021 13:43:37 GMT
Connection: close
Server: ATS/10.0.0
Cache-Control: no-store
Content-Type: text/html
Content-Language: en
Content-Length: 219
<HTML>
<HEAD>
<TITLE>Bad Request</TITLE>
</HEAD>
<BODY BGCOLOR="white" FGCOLOR="black">
<H1>Bad Request</H1>
<HR>
<FONT FACE="Helvetica,Arial"><B>
Description: Could not process this request.
</B></FONT>
<HR>
</BODY>
GET /x HTTP/1.1
Ncat: Broken pipe.
```
Looking at the tcpdump, we see that ATS sent a FIN which nc didn't respond too. After sending the second get request, ATS sends the RESET.
I tried putting a ^V in a header name, but couldn't get a failure. Based on your output, the Connection: close is not being added, so there must be another path that the Bad Request takes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7885: Close connection after every bad request for HTTP/1.1
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7885:
URL: https://github.com/apache/trafficserver/pull/7885#issuecomment-859792223
Hmm. That is an interesting point. At this point I would not. But let me think on that a bit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7885: Close connection after every bad request for HTTP/1.1
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7885:
URL: https://github.com/apache/trafficserver/pull/7885#issuecomment-851124158
I can make another request after a bad request. Am I checking something different?
```
$ telnet localhost 8888
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
GET /x HTTP/1.1
Host: localhost
A^VA: bbb
HTTP/1.1 400 Bad Request
Date: Mon, 31 May 2021 02:25:21 GMT
Content-Type: text/html
Content-Length: 165
Age: 0
Connection: keep-alive
Via: http/1.1 traffic_server (ApacheTrafficServer/10.0.0 [c s f ])
Server: ATS/10.0.0
<html>
<head>
<title>Bad Request</title>
</head>
<body>
<h1><p>Bad Request</p></h1>
Invalid HTTP header name: 'A\x16A'
</body>
</html>
GET /x HTTP/1.1
Host: localhost
HTTP/1.1 404 NOT FOUND
Date: Mon, 31 May 2021 02:25:31 GMT
Content-Type: text/html
Content-Length: 233
Server: ATS/10.0.0
Access-Control-Allow-Origin: *
Access-Control-Allow-Credentials: true
Age: 0
Connection: keep-alive
Via: http/1.1 traffic_server (ApacheTrafficServer/10.0.0 [c s f ])
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2 Final//EN">
<title>404 Not Found</title>
<h1>Not Found</h1>
<p>The requested URL was not found on the server. If you entered the URL manually please check your spelling and try again.</p>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich edited a comment on pull request #7885: Close connection after every bad request for HTTP/1.1
Posted by GitBox <gi...@apache.org>.
shinrich edited a comment on pull request #7885:
URL: https://github.com/apache/trafficserver/pull/7885#issuecomment-852139571
Here is the case I am testing there. Thought I had added autests, but I must not have git-added them.
```
[shinrich@atslab02 ~]$ nc -C 127.0.0.1 8080
GET /x HTTP/1.1
host : bob
HTTP/1.1 400 Invalid HTTP Request
Date: Tue, 01 Jun 2021 13:43:37 GMT
Connection: close
Server: ATS/10.0.0
Cache-Control: no-store
Content-Type: text/html
Content-Language: en
Content-Length: 219
<HTML>
<HEAD>
<TITLE>Bad Request</TITLE>
</HEAD>
<BODY BGCOLOR="white" FGCOLOR="black">
<H1>Bad Request</H1>
<HR>
<FONT FACE="Helvetica,Arial"><B>
Description: Could not process this request.
</B></FONT>
<HR>
</BODY>
GET /x HTTP/1.1
Ncat: Broken pipe.
```
Looking at the tcpdump, we see that ATS sent a FIN which nc didn't respond too. After sending the second get request, ATS sends the RESET.
I tried putting a ^V in a header name, but couldn't get a failure. Based on your output, the Connection: close is not being added, so there must be another path that the Bad Request takes.
Hmm, but that one was working properly without this fix. Let me add back in the test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7885: Close connection after every bad request
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7885:
URL: https://github.com/apache/trafficserver/pull/7885#issuecomment-849197966
This is not exactly the same story, but we don't close H2 connection immediately even if there is a stream error. We have a setting for error rate threshold and only send GOAWAY frame if error rate exceeds the threshold. I personally think we can close a connection that causes an error on the first request, but some others don't want it.
https://github.com/apache/trafficserver/pull/6525
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7885: Close connection after every bad request for HTTP/1.1
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7885:
URL: https://github.com/apache/trafficserver/pull/7885#issuecomment-852139571
Here is the case I am testing there. Thought I had added autests, but I must not have git-added them.
```
[shinrich@atslab02 ~]$ nc -C 127.0.0.1 8080
GET /x HTTP/1.1
host: localhost
aa: stuff
HTTP/1.1 404 Not Found on Accelerator
Date: Tue, 01 Jun 2021 13:42:02 GMT
Connection: keep-alive
Server: ATS/10.0.0
Cache-Control: no-store
Content-Type: text/html
Content-Language: en
Content-Length: 297
<HTML>
<HEAD>
<TITLE>Not Found on Accelerator</TITLE>
</HEAD>
<BODY BGCOLOR="white" FGCOLOR="black">
<H1>Not Found on Accelerator</H1>
<HR>
<FONT FACE="Helvetica,Arial"><B>
Description: Your request on the specified host was not found.
Check the location and try again.
</B></FONT>
<HR>
</BODY>
GET /x HTTP/1.1
host : bob
HTTP/1.1 400 Invalid HTTP Request
Date: Tue, 01 Jun 2021 13:42:22 GMT
Connection: close
Server: ATS/10.0.0
Cache-Control: no-store
Content-Type: text/html
Content-Language: en
Content-Length: 219
<HTML>
<HEAD>
<TITLE>Bad Request</TITLE>
</HEAD>
<BODY BGCOLOR="white" FGCOLOR="black">
<H1>Bad Request</H1>
<HR>
<FONT FACE="Helvetica,Arial"><B>
Description: Could not process this request.
</B></FONT>
<HR>
</BODY>
GET /x HTTP/1.1
Ncat: Broken pipe.
```
Looking at the tcpdump, we see that ATS sent a FIN which nc didn't respond too. After sending the second get request, ATS sends the RESET.
I tried putting a ^V in a header name, but couldn't get a failure. Based on your output, the Connection: close is not being added, so there must be another path that the Bad Request takes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7885: Close connection after every bad request for HTTP/1.1
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7885:
URL: https://github.com/apache/trafficserver/pull/7885#issuecomment-888657768
Cherry-picked to v9.1.x branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7885: Close connection after every bad request for HTTP/1.1
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7885:
URL: https://github.com/apache/trafficserver/pull/7885#issuecomment-857384293
I finally figured out what was happening on my test. The Bad Request response was from an origin server but not ATS.
Although I think ATS itself should return 400 for my bad header test case, should we close connections if origin returns 400 as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich edited a comment on pull request #7885: Close connection after every bad request for HTTP/1.1
Posted by GitBox <gi...@apache.org>.
shinrich edited a comment on pull request #7885:
URL: https://github.com/apache/trafficserver/pull/7885#issuecomment-850448935
Correct, this need to close the connection after a bad request only applies to HTTP/1.1 because we are worried about messing up the transaction framing. This is not an issue for HTTP/2, so we don't need to close the HTTP/2 session just because of one bad stream on that session.
I believe that the do_io_close against the HTTP/2 stream will not close the HTTP/2 session, so this should all work out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich merged pull request #7885: Close connection after every bad request for HTTP/1.1
Posted by GitBox <gi...@apache.org>.
shinrich merged pull request #7885:
URL: https://github.com/apache/trafficserver/pull/7885
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org