Set Content-Length header in Traffic Router #4074

zrhoffman · 2019-11-04T13:53:02Z

What does this PR (Pull Request) do?

This PR fixes Traffic Router inconsistent behavior on HTTP HEAD requests #3965 by making the Traffic Router API and routing responses include a Content-Length header with every response
Exposes LocationController endpoints to HEAD requests
HEAD requests for routing responses return the same Content-Type and Content-Length as GET requests

Which Traffic Control components are affected by this PR?

Documentation
Traffic Router

What is the best way to verify this PR?

Run the following external tests:

org.apache.traffic_control.traffic_router.core.external.BufferedResponseTest
org.apache.traffic_control.traffic_router.core.external.LocationsTest

If this is a bug fix, what versions of Traffic Control are affected?

master (c65ee2e)
6.0.0

The following criteria are ALL met by this PR

This PR includes tests
This PR includes documentation
This PR includes an update to CHANGELOG.md
This PR includes any and all required license headers
This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

to leave the response uncommitted

asf-ci · 2019-11-04T13:56:10Z

Can one of the admins verify this patch?

docs/source/development/traffic_router/traffic_router_api.rst

ocket8888

Docs look fine and I glanced over the Java and everything looks alright, but somebody more familiar with TR should take a look.

mitchell852 · 2019-11-12T16:29:21Z

Docs look fine and I glanced over the Java and everything looks alright, but somebody more familiar with TR should take a look.

maybe @rawlinp or @ajschmidt can look at it.

rawlinp

Nice work! The code changes look good -- I just want to do some more testing against it before merging.

ocket8888 · 2019-12-19T15:49:25Z

Can you rebase to fix conflicts? Idk if @rawlinp ever got around to that testing, but it can't be merged in any case while it has conflicts.

zrhoffman · 2019-12-19T16:47:35Z

@ocket8888 Okay, I have rebased it. Merge conflicts are resolved.

rawlinp · 2019-12-20T17:14:40Z

add to whitelist

(this is just to tell the asf-ci bot that your PRs are ok to run through CI)

asf-ci · 2019-12-20T17:22:49Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4970/

zrhoffman · 2021-11-22T14:11:24Z

Rebased to fix merge conflicts from #6008.

As of f8dcdf2320, Traffic Router closes the connection once the response is sent, so if a user does curl -X HEAD (which would previously keep the connection alive, since curl expects to receive a request body and does not) instead of curl --head, Traffic Router won't keep the connection open for an additional 10 seconds.

Undrafted, #4074 is ready for review.

rawlinp · 2021-11-23T17:36:20Z

...rc/main/java/org/apache/traffic_control/traffic_router/core/http/BufferedResponseFilter.java

+		// sending it sometimes means we must always send it. From RFC 2616:
+		// > HTTP/1.1 applications that do not support persistent connections MUST
+		// > include the "close" connection option in every message.
+		response.addHeader(HttpHeaders.CONNECTION, "close");


This worries me, because we might be fixing the performance of invalid requests (e.g. curl -X HEAD), at the expense of valid requests. Keeping client connections alive where possible can be a good thing.

It can be, but some CDNs seem to prefer Connection: close for redirects:

[zrhoffman@computer ~]$ curl -I https://apple.com/ HTTP/1.1 301 Redirect Date: Wed, 02 Nov 2022 15:18:11 GMT Connection: close Via: http/1.1 xxxx.ts.apple.com (acdn/000.00000) Cache-Control: no-store Location: https://www.apple.com/ Content-Type: text/html Content-Language: en X-Cache: none CDNUUID: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx-xxxxxxxxxx Content-Length: 304

[zrhoffman@computer ~]$ curl -I http://fastly.com/ HTTP/1.1 301 Moved Permanently Connection: close Content-Length: 0 Retry-After: 0 Accept-Ranges: bytes Date: Wed, 02 Nov 2022 15:18:50 GMT X-Served-By: cache-xxx0000-XXX X-Cache: HIT X-Cache-Hits: 0 Cache-Control: max-age=0, private, must-revalidate Server: Artisanal bits Location: https://fastly.com/

Looks like cloudfront.com is trying to close the connection but mistyped it as Cneonction:

[zrhoffman@computer ~]$ curl -I http://cloudfront.com/ HTTP/1.1 302 Found Date: Wed, 02 Nov 2022 15:19:19 GMT Server: Server Location: http://aws.amazon.com/cloudfront Cneonction: close Content-Type: text/html; charset=iso-8859-1 ```

Does TR support HTTP/2? The spec for HTTP/2 says that clients must treat any message containing a connection-specific header as malformed. So we definitely shouldn't send that on requests for that protocol.

close is the default behavior for HTTP/1.0, so really you just want to add this header if the request is HTTP/1.1. You can check that with the request's getProtocol method, I think.

Does TR support HTTP/2?

No, and curl -X HEAD doesn't hang for HTTP/2 anyway.

As an alternative to Connection: close, we could just reduce Connector.connectionTimeout from 10 seconds to something shorter, like 3 or 5 seconds:

https://github.com/apache/trafficcontrol/blob/b24649869407d902301ce6997c629e367285b44f/traffic_router/core/src/main/conf/server.xml#L38

https://github.com/apache/trafficcontrol/blob/b24649869407d902301ce6997c629e367285b44f/traffic_router/core/src/main/conf/server.xml#L41

https://github.com/apache/trafficcontrol/blob/b24649869407d902301ce6997c629e367285b44f/traffic_router/core/src/main/conf/server.xml#L43

https://github.com/apache/trafficcontrol/blob/b24649869407d902301ce6997c629e367285b44f/traffic_router/core/src/main/conf/server.xml#L47

Ideally @rawlinp would chime in to say whether or not that's acceptable

Connection: close still kind of worries me, especially because there may be clients out there relying on high connection rates to TR, whether that is really the right approach or not, reusing connections could actually be helping TR performance. Maybe I don't fully understand the need for it here? Are HEAD requests broken without it?

Changing connectionTimeout in server.xml seems like it would be more fine-tuned via the CDN operator's ansible playbooks (or equivalent) if the CDN had issues with slow clients. I think Tomcat ships with a 20s default, and 10s seems reasonable for a TR default.

Looks like cloudfront.com is trying to close the connection but mistyped it as Cneonction:

As @rob05c pointed out to me, Cneonction is just a hack to get around Connection header differences between the origin and caches.

Maybe I don't fully understand the need for it here? Are HEAD requests broken without it?

Nope we don't need it, it would only be to appease invalid requests like curl -X HEAD. But curl -X HEAD doesn't cause a timeout anyway in HTTP/2, so there isn't a long-term reason to do it, especially with the benefits from reusing the connection like you mentioned.

Even if we did want to change any connection behavior, that belongs in a separate PR, #4074 should just be to resolve disparities between HEAD and GET.

Forgot to mention: Reverted closing the connection in 97d38f2234

rawlinp · 2021-11-23T18:00:06Z

mvn clean verify appears to pass for me on master, but in this PR I get an error:

Results :

Tests in error:
  SteeringTest.z_itemsMigrateFromSmallerToLargerBucket:327 » HttpHostConnect Con...

zrhoffman · 2022-01-14T20:17:55Z

Rebased to fix merge conflicts from #6390

.../test/java/org/apache/traffic_control/traffic_router/core/external/BufferedResponseTest.java

...ore/src/test/java/org/apache/traffic_control/traffic_router/core/external/LocationsTest.java

This reverts commit b24649869407d902301ce6997c629e367285b44f.

zrhoffman · 2022-11-04T21:54:26Z

Rebased to fix a changelog merge conflict

zrhoffman · 2022-11-07T16:14:22Z

The TR Ultimate Test Harness workflow fails on the master branch (and on #4074), fixed by #7175.

zrhoffman · 2022-11-07T18:35:38Z

Rebased onto master so tests will pass now that #7175 is merged

CHANGELOG.md

zrhoffman added 5 commits October 27, 2019 22:11

Call setHeader() + setStatus() on the response instead of sendRedirect()

bf6a0f0

to leave the response uncommitted

Add a filter that sets Content-Length header for all responses

d3cf82a

Use same program flow for GET and HEAD requests

7d54b4a

Allow HEAD requests for LocationController endpoints

db5a076

Update documentation

7d35aa4

mitchell852 added Traffic Router related to Traffic Router bug something isn't working as intended labels Nov 4, 2019

ocket8888 requested changes Nov 4, 2019

View reviewed changes

zrhoffman added 2 commits November 4, 2019 14:25

Re-add HTTP reason-phrases in documentation

0c7a32b

No need to note that API output is beautified

80e58cb

zrhoffman requested a review from ocket8888 November 4, 2019 21:07

ocket8888 approved these changes Nov 4, 2019

View reviewed changes

zrhoffman mentioned this pull request Nov 7, 2019

TR: Send reason-phrase for HTTP API responses #4096

Closed

7 tasks

rawlinp reviewed Nov 12, 2019

View reviewed changes

Add changelog entry

d341af1

zrhoffman force-pushed the set_content_length branch from ea22aff to 05da19c Compare December 19, 2019 16:40

zrhoffman force-pushed the set_content_length branch 2 times, most recently from bd2cd43 to d328946 Compare March 24, 2020 22:04

mitchell852 added the WIP "Work-in-Progress" - do not merge! (use 'draft' pull requests from now on) label Apr 21, 2020

zrhoffman marked this pull request as draft October 22, 2021 19:45

zrhoffman added 2 commits November 21, 2021 21:13

Use wrapper class instead of reflection to get Content-Length

a418140

Close the connection after sending the response

5d707dc

zrhoffman marked this pull request as ready for review November 22, 2021 06:30

zrhoffman removed the WIP "Work-in-Progress" - do not merge! (use 'draft' pull requests from now on) label Nov 22, 2021

zrhoffman force-pushed the set_content_length branch from d328946 to 666ec1b Compare November 22, 2021 14:06

rawlinp reviewed Nov 23, 2021

View reviewed changes

zrhoffman force-pushed the set_content_length branch 2 times, most recently from d78501b to 2df2416 Compare January 14, 2022 20:17

zrhoffman force-pushed the set_content_length branch from 2df2416 to b246498 Compare October 6, 2022 19:13

ocket8888 reviewed Nov 2, 2022

View reviewed changes

zrhoffman added 7 commits November 4, 2022 14:11

Use constants for headers and charsets

7bc9865

Use try block with resources

e27cf5e

Space before "to"

0f49e64

Use MatcherAssert.assertThat because Assert.assertThat is deprecated

3e21d03

Assert Content-Length header is set

093dce8

Revert "Close the connection after sending the response"

ed3b795

This reverts commit b24649869407d902301ce6997c629e367285b44f.

Move changelog entry to unreleased section

88e2490

zrhoffman force-pushed the set_content_length branch from a72b382 to 20a37e9 Compare November 4, 2022 21:54

rawlinp approved these changes Nov 7, 2022

View reviewed changes

zrhoffman force-pushed the set_content_length branch from 20a37e9 to 88e2490 Compare November 7, 2022 18:34

ocket8888 reviewed Nov 8, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Remove unused 6.1.0 changelog entry

7774d21

ocket8888 approved these changes Nov 9, 2022

View reviewed changes

ocket8888 merged commit 40971dc into apache:master Nov 9, 2022

zrhoffman deleted the set_content_length branch November 9, 2022 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set Content-Length header in Traffic Router #4074

Set Content-Length header in Traffic Router #4074

zrhoffman commented Nov 4, 2019 •

edited

Loading

asf-ci commented Nov 4, 2019

ocket8888 left a comment

mitchell852 commented Nov 12, 2019

rawlinp left a comment

ocket8888 commented Dec 19, 2019

zrhoffman commented Dec 19, 2019

rawlinp commented Dec 20, 2019 •

edited

Loading

asf-ci commented Dec 20, 2019

zrhoffman commented Nov 22, 2021

rawlinp Nov 23, 2021

zrhoffman Nov 2, 2022

ocket8888 Nov 2, 2022 •

edited

Loading

ocket8888 Nov 2, 2022

zrhoffman Nov 4, 2022

ocket8888 Nov 4, 2022

rawlinp Nov 4, 2022

zrhoffman Nov 4, 2022

zrhoffman Nov 4, 2022

rawlinp commented Nov 23, 2021

zrhoffman commented Jan 14, 2022

zrhoffman commented Nov 4, 2022

zrhoffman commented Nov 7, 2022

zrhoffman commented Nov 7, 2022

Set Content-Length header in Traffic Router #4074

Set Content-Length header in Traffic Router #4074

Conversation

zrhoffman commented Nov 4, 2019 • edited Loading

What does this PR (Pull Request) do?

Which Traffic Control components are affected by this PR?

What is the best way to verify this PR?

If this is a bug fix, what versions of Traffic Control are affected?

The following criteria are ALL met by this PR

asf-ci commented Nov 4, 2019

ocket8888 left a comment

Choose a reason for hiding this comment

mitchell852 commented Nov 12, 2019

rawlinp left a comment

Choose a reason for hiding this comment

ocket8888 commented Dec 19, 2019

zrhoffman commented Dec 19, 2019

rawlinp commented Dec 20, 2019 • edited Loading

asf-ci commented Dec 20, 2019

zrhoffman commented Nov 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocket8888 Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rawlinp commented Nov 23, 2021

zrhoffman commented Jan 14, 2022

zrhoffman commented Nov 4, 2022

zrhoffman commented Nov 7, 2022

zrhoffman commented Nov 7, 2022

zrhoffman commented Nov 4, 2019 •

edited

Loading

rawlinp commented Dec 20, 2019 •

edited

Loading

ocket8888 Nov 2, 2022 •

edited

Loading