You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by GitBox <gi...@apache.org> on 2022/04/11 06:22:46 UTC
[GitHub] [tomcat] CrazyHZM opened a new pull request, #501: Support max http request header size and max http header size configuration separation
CrazyHZM opened a new pull request, #501:
URL: https://github.com/apache/tomcat/pull/501
Mail link：https://bz.apache.org/bugzilla/show_bug.cgi?id=65866
--
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: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] CrazyHZM commented on pull request #501: Support max http request header size and max http header size configuration separation
Posted by GitBox <gi...@apache.org>.
CrazyHZM commented on PR #501:
URL: https://github.com/apache/tomcat/pull/501#issuecomment-1094939842
@rmaucher
Do i need to resubmit a clean pr?
--
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: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] rmannibucau commented on a diff in pull request #501: Support max http request header size and max http header size configuration separation
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #501:
URL: https://github.com/apache/tomcat/pull/501#discussion_r847064940
##########
java/org/apache/coyote/http11/AbstractHttp11Protocol.java:
##########
@@ -244,6 +244,34 @@ public void setMaxSavePostSize(int maxSavePostSize) {
public void setMaxHttpHeaderSize(int valueI) { maxHttpHeaderSize = valueI; }
+ /**
+ * Maximum size of the HTTP request message header.
+ */
+ private int maxHttpRequestHeaderSize = -1;
Review Comment:
Setter should stay IMHO and set both values to the exact same one *if not already set*.
Getter should get the max value since the consumer wants the max size and it is exactly the semantic of this method (= what is the max for request and response, only change is that before it was a single value, now it is really a max but a min is wrong IMHO).
So IMHO it makes perfect sense as method for frameworks and integrators to still have this, in particular when it is linked to pooling where max is more intended than min or any value under the max.
> I still insist that semantic level compatibility is more important than API level compatibility
Well there are two points:
1. API compat: IMHO it is not an option to break before next 10.2 or something like that
2. semantic level is preserved and it makes sense to have this shortcut IMHO so we can solve both at once
--
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: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] gmshake commented on a diff in pull request #501: Support max http request header size and max http header size configuration separation
Posted by GitBox <gi...@apache.org>.
gmshake commented on code in PR #501:
URL: https://github.com/apache/tomcat/pull/501#discussion_r847041315
##########
java/org/apache/coyote/http11/AbstractHttp11Protocol.java:
##########
@@ -244,6 +244,34 @@ public void setMaxSavePostSize(int maxSavePostSize) {
public void setMaxHttpHeaderSize(int valueI) { maxHttpHeaderSize = valueI; }
+ /**
+ * Maximum size of the HTTP request message header.
+ */
+ private int maxHttpRequestHeaderSize = -1;
Review Comment:
Keep `getMaxHttpHeaderSize()` only preserves API level compatibility, the semantic of `getMaxHttpHeaderSize()` has change once we introduce dedicated header settings for request and response.
To illustrate the semantic change:
```java
setMaxHttpHeaderSize(4096);
setMaxHttpRequestHeaderSize(8192);
setMaxHttpResponseHeaderSize(16384);
```
Then `getMaxHttpHeaderSize()` make no sense except the only meaning "default maxHttpHeaderSize"
Maybe it is more reasonable to deprecate `getMaxHttpHeaderSize()` and return a size that is not too bad:
```java
@Deprecated
public int getMaxHttpHeaderSize() {
return Math.min(maxHttpRequestHeaderSize, maxHttpResponseHeaderSize)
}
```
I still insist that semantic level compatibility is more important than API level compatibility. For `setMaxHttpHeaderSize()` it originally means set maximum size of the HTTP message header for both request and response.
--
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: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] rmaucher commented on pull request #501: Support max http request header size and max http header size configuration separation
Posted by GitBox <gi...@apache.org>.
rmaucher commented on PR #501:
URL: https://github.com/apache/tomcat/pull/501#issuecomment-1094622549
That seems pretty straightforward. I'm slightly skeptical about the real practical use since trying to optimize the sizes too much is risky, but no problem, this is the user's responsibility.
--
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: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] rmaucher commented on pull request #501: Support max http request header size and max http header size configuration separation
Posted by GitBox <gi...@apache.org>.
rmaucher commented on PR #501:
URL: https://github.com/apache/tomcat/pull/501#issuecomment-1094743565
Overall, I prefer the PR as it is right now, it's simpler. If the new setters are used, then the behavior could be slightly different, but since this is new API this should be fine.
--
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: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] rmaucher commented on pull request #501: Support max http request header size and max http header size configuration separation
Posted by GitBox <gi...@apache.org>.
rmaucher commented on PR #501:
URL: https://github.com/apache/tomcat/pull/501#issuecomment-1094815675
The proposed patch is still simpler and should work well, so I will merge that one.
--
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: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] CrazyHZM commented on pull request #501: Support max http request header size and max http header size configuration separation
Posted by GitBox <gi...@apache.org>.
CrazyHZM commented on PR #501:
URL: https://github.com/apache/tomcat/pull/501#issuecomment-1094598895
Please review the code first, if there is no problem, I will support this on other branches 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.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] CrazyHZM commented on a diff in pull request #501: Support max http request header size and max http header size configuration separation
Posted by GitBox <gi...@apache.org>.
CrazyHZM commented on code in PR #501:
URL: https://github.com/apache/tomcat/pull/501#discussion_r847005884
##########
java/org/apache/coyote/http11/AbstractHttp11Protocol.java:
##########
@@ -244,6 +244,34 @@ public void setMaxSavePostSize(int maxSavePostSize) {
public void setMaxHttpHeaderSize(int valueI) { maxHttpHeaderSize = valueI; }
+ /**
+ * Maximum size of the HTTP request message header.
+ */
+ private int maxHttpRequestHeaderSize = -1;
Review Comment:
I'm worried about the compatibility risk of removing getMaxHttpHeaderSize(), because this is a public API after all, I wonder if this change can be done in 10.1.x. of which I agree with the annotation Deprecated.
--
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: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] rmannibucau commented on pull request #501: Support max http request header size and max http header size configuration separation
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on PR #501:
URL: https://github.com/apache/tomcat/pull/501#issuecomment-1094636457
+1 I see two advantages:
1. enables cases where "front" tomcat has big incoming headers but "normal" forwarded headers
2. enables better web server abstractions which often have this configuration
Open question: shouldn't `setMaxHttpHeaderSize` set both request and response ones and `maxHttpHeaderSize` field be dropped for backward compat?
--
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: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] rmaucher closed pull request #501: Support max http request header size and max http header size configuration separation
Posted by GitBox <gi...@apache.org>.
rmaucher closed pull request #501: Support max http request header size and max http header size configuration separation
URL: https://github.com/apache/tomcat/pull/501
--
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: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] gmshake commented on pull request #501: Support max http request header size and max http header size configuration separation
Posted by GitBox <gi...@apache.org>.
gmshake commented on PR #501:
URL: https://github.com/apache/tomcat/pull/501#issuecomment-1094800382
> Overall, I prefer the PR as it is right now, it's simpler. If the new setters are used, then the behavior could be slightly different, but since this is new API this should be fine.
It is not complicated. I forked and edit online to demonstrate the part of change: https://github.com/gmshake/tomcat/commit/75289cf9e7a3920cc6d3ea0463db53a6ab0d090c?diff=split
--
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: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] gmshake commented on a diff in pull request #501: Support max http request header size and max http header size configuration separation
Posted by GitBox <gi...@apache.org>.
gmshake commented on code in PR #501:
URL: https://github.com/apache/tomcat/pull/501#discussion_r846991658
##########
java/org/apache/coyote/http11/AbstractHttp11Protocol.java:
##########
@@ -244,6 +244,34 @@ public void setMaxSavePostSize(int maxSavePostSize) {
public void setMaxHttpHeaderSize(int valueI) { maxHttpHeaderSize = valueI; }
+ /**
+ * Maximum size of the HTTP request message header.
+ */
+ private int maxHttpRequestHeaderSize = -1;
Review Comment:
After the change, `maxHttpHeaderSize` becomes default maximum size of the HTTP message header for both request and response.
If we no longer have consumer of `getMaxHttpHeaderSize()`, then I'd suggest to remove `maxHttpHeaderSize`, and change `setMaxHttpHeaderSize()` like this
```java
@Deprecated
public void setMaxHttpHeaderSize(int valueI) {
maxHttpRequestHeaderSize = valueI;
maxHttpResponseHeaderSize = valueI;
}
```
--
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: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] CrazyHZM commented on pull request #501: Support max http request header size and max http header size configuration separation
Posted by GitBox <gi...@apache.org>.
CrazyHZM commented on PR #501:
URL: https://github.com/apache/tomcat/pull/501#issuecomment-1094629380
> That seems pretty straightforward. I'm slightly skeptical about the real practical use since trying to optimize the sizes too much is risky, but no problem, this is the user's responsibility.
@rmaucher
I saw some discussion about this, you can refer to it:
https://github.com/spring-projects/spring-boot/issues/29382
--
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: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] gmshake commented on a diff in pull request #501: Support max http request header size and max http header size configuration separation
Posted by GitBox <gi...@apache.org>.
gmshake commented on code in PR #501:
URL: https://github.com/apache/tomcat/pull/501#discussion_r847041315
##########
java/org/apache/coyote/http11/AbstractHttp11Protocol.java:
##########
@@ -244,6 +244,34 @@ public void setMaxSavePostSize(int maxSavePostSize) {
public void setMaxHttpHeaderSize(int valueI) { maxHttpHeaderSize = valueI; }
+ /**
+ * Maximum size of the HTTP request message header.
+ */
+ private int maxHttpRequestHeaderSize = -1;
Review Comment:
Keep `getMaxHttpHeaderSize()` only preserves API level compatibility, the semantic of `getMaxHttpHeaderSize()` has change once we introduce dedicated header settings for request and response.
To illustrate the semantic change:
```java
setMaxHttpHeaderSize(4096);
setMaxHttpRequestHeaderSize(8192);
setMaxHttpResponseHeaderSize(16384);
```
Then `getMaxHttpHeaderSize()` make no sense except the only meaning "default maxHttpHeaderSize"
Maybe it is more reasonable to deprecate `getMaxHttpHeaderSize()` and return a size that is not too bad:
```java
public int getMaxHttpHeaderSize() {
return Math.min(maxHttpRequestHeaderSize, maxHttpResponseHeaderSize)
}
```
I still insist that semantic level compatibility is more important than API level compatibility. For `setMaxHttpHeaderSize()` it originally means set maximum size of the HTTP message header for both request and response.
--
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: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] CrazyHZM commented on pull request #501: Support max http request header size and max http header size configuration separation
Posted by GitBox <gi...@apache.org>.
CrazyHZM commented on PR #501:
URL: https://github.com/apache/tomcat/pull/501#issuecomment-1094916955
@rmaucher
I can patch several other branches, right?
--
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: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] gmshake commented on a diff in pull request #501: Support max http request header size and max http header size configuration separation
Posted by GitBox <gi...@apache.org>.
gmshake commented on code in PR #501:
URL: https://github.com/apache/tomcat/pull/501#discussion_r847041315
##########
java/org/apache/coyote/http11/AbstractHttp11Protocol.java:
##########
@@ -244,6 +244,34 @@ public void setMaxSavePostSize(int maxSavePostSize) {
public void setMaxHttpHeaderSize(int valueI) { maxHttpHeaderSize = valueI; }
+ /**
+ * Maximum size of the HTTP request message header.
+ */
+ private int maxHttpRequestHeaderSize = -1;
Review Comment:
Keep `getMaxHttpHeaderSize()` only preserves API level compatibility, the semantic of `getMaxHttpHeaderSize()` has change once we introduce dedicated header settings for request and response.
To illustrate the semantic change:
```java
setMaxHttpHeaderSize(4096);
setMaxHttpRequestHeaderSize(8192);
setMaxHttpResponseHeaderSize(16384);
```
Then `getMaxHttpHeaderSize()` make no sense except the only meaning "default maxHttpHeaderSize"
Maybe it is deprecate `getMaxHttpHeaderSize()` and return a size that is not too bad:
```java
public int getMaxHttpHeaderSize() {
return Math.min(maxHttpRequestHeaderSize, maxHttpResponseHeaderSize)
}
```
I still insist that semantic level compatibility is more important than API level compatibility. For `setMaxHttpHeaderSize()` it originally means set maximum size of the HTTP message header for both request and response.
--
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: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] rmaucher commented on pull request #501: Support max http request header size and max http header size configuration separation
Posted by GitBox <gi...@apache.org>.
rmaucher commented on PR #501:
URL: https://github.com/apache/tomcat/pull/501#issuecomment-1094929661
Merged with changed. I didn't understand the changes to TestHttp2Limits, HTTP/2 should use the new request header size and that should be it. Also updated docs and changelog.
--
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: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org