Skip to content
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.

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

Restore isPositive check for maxHttpHeaderSize #14986

Conversation

Projects
None yet
4 participants
@bturner
Copy link

commented Oct 28, 2018

  • 1.5 and 2.0 releases used Tomcat's default max header size when the configured server.max-http-header-size was 0, but that has regressed in 2.1.0.RC1 and now the max is set to 0 when configured as 0
  • Configuring a max of 0 prevents all HTTP processing, since no headers can be set, which makes no sense

In Bitbucket Server, we've added some code around Spring Boot which facilitates adding multiple HTTP connectors, a common necessity when standing up a load balancer or reverse proxy in front of the system. The properties we support for those additional connectors are drawn from, and configured similarly to, how Spring Boot handles the primary connector.

As we build out Java 11 support, we've upgraded to Spring Boot 2.0.6 without any significant changes (aside from "standard" Spring Boot 1.5 -> 2.0 migration steps). Upgrading to 2.1.0.RC1, however, resulted in all HTTP processing failing with this error:

[INFO] 2018-10-28 13:36:33,502 ERROR [http-nio-7990-exec-3]  o.a.coyote.http11.Http11Processor Error finishing response
[INFO] org.apache.coyote.http11.HeadersTooLargeException: An attempt was made to write more data to the response headers than there was room available in the buffer. Increase maxHttpHeaderSize on the connector or write less data into the response headers.
[INFO] 	at org.apache.coyote.http11.Http11OutputBuffer.checkLengthBeforeWrite(Http11OutputBuffer.java:543)
[INFO] 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
[INFO] 	at java.lang.Thread.run(Thread.java:748)
[INFO] 	... 12 frames trimmed

I tracked this down to the introduction of DataSize and the way the TomcatWebServerFactoryCustomizer applies the maxHttpHeaderSize. In 2.0.x, the chain included a when(this::isPositive) that is no longer present. I don't see anything about this change in behavior in the various 2.1.0.M1-RC1 documentation, which makes me feel like it's a regression rather than a conscious change.

In case you're curious why we're using 0, it's to allow this in our default properties:

# Controls the max HTTP header size. The default is 0, to use Tomcat's own default (8K), and is set explicitly
# to simplify referencing it as a default for additional connectors.
server.max-http-header-size=0

server.additional-connector.1.max-http-header-size=${server.max-http-header-size}
server.additional-connector.2.max-http-header-size=${server.max-http-header-size}
server.additional-connector.3.max-http-header-size=${server.max-http-header-size}
server.additional-connector.4.max-http-header-size=${server.max-http-header-size}
server.additional-connector.5.max-http-header-size=${server.max-http-header-size}

This way, when multiple connectors are present, it's possible to apply a consistent max-http-header-size to all of them by adjusting the primary connector (and still possible to selectively override for additional connectors if necessary).

@bturner bturner force-pushed the bturner:restore-max-http-header-ispositive-check branch from 54a9134 to c53f0fd Oct 28, 2018

Bryan Turner
Restore isPositive check for maxHttpHeaderSize.
- 1.5 and 2.0 releases used Tomcat's default max header size when the
  configured server.max-http-header-size was 0, but that has regressed
  in 2.1.0.RC1 and now the max is set to 0 when configured as 0
  - Added unit tests to verify -1 and 0 use Tomcat's default 8K limit
- Configuring a max of 0 prevents all HTTP processing, since no headers
  can be set, which makes no sense

@bturner bturner force-pushed the bturner:restore-max-http-header-ispositive-check branch from c53f0fd to a022670 Oct 28, 2018

@bturner

This comment has been minimized.

Copy link
Author

commented Oct 28, 2018

Added a couple tests, and ran mvn spring-javaformat:apply to fix the formatting.

@philwebb philwebb added this to the 2.1.x milestone Oct 28, 2018

@snicoll

This comment has been minimized.

Copy link
Member

commented Oct 28, 2018

See #14234

@snicoll snicoll changed the title Restore isPositive check for maxHttpHeaderSize. Restore isPositive check for maxHttpHeaderSize Oct 28, 2018

@philwebb philwebb self-assigned this Oct 29, 2018

philwebb added a commit that referenced this pull request Oct 29, 2018

Restore max-http-header-size default value support
Fix `TomcatWebServerFactoryCustomizer` to restore Spring Boot 2.0
behavior where a negative or zero `max-http-header-size` indicates
that the server default should be used.

See gh-14986

@philwebb philwebb closed this in 1451c0c Oct 29, 2018

philwebb added a commit that referenced this pull request Oct 29, 2018

Merge pull request #14986 from bturner
* pr/14986:
  Polish "Restore max-http-header-size default value support"
  Restore max-http-header-size default value support
@philwebb

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

Thanks very much for the PR. I've merged it to master and also restored the same behavior for Jetty and Undertow.

@philwebb philwebb modified the milestones: 2.1.x, 2.1.0 Oct 29, 2018

@bturner

This comment has been minimized.

Copy link
Author

commented Oct 29, 2018

Ah, sorry, I should have thought to check Jetty and Undertow. My apologies for the incomplete change, @philwebb. Thanks for finishing it up.

@bturner bturner deleted the bturner:restore-max-http-header-ispositive-check branch Oct 29, 2018

@philwebb

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

@bturner, no problem. I nearly missed them myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.