[GitHub] [tomcat] stokito opened a new pull request #324: Change ETag format to Nginx like

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] stokito opened a new pull request #324: Change ETag format to Nginx like

GitBox

stokito opened a new pull request #324:
URL: https://github.com/apache/tomcat/pull/324


   Currently Tomcat 9 generates ETag like `W/"1047-1578315296666"` i.e. `Weak"Size-MTime in Milliseconds"`.
   This is incorrect ETag because it should be strong as for a static file i.e. octal compatibility.
   Also to make ETag working for load balancing between Tomcat and Nginx we need to change format to `"hex(MTime in seconds)-hex(Size)"` e.g. `"5e132e20-417"`.
   Nginx is not configurable but ether Tomcat is not. In the same time Nginx is more widely used while Tomcat returns incorrect ETag.
   This small PR fixes the problem.
   As a downside after update Tomcat will discard existing ETags but then they'll be updated on clients and everything will work 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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] michael-o commented on pull request #324: Change ETag format to Nginx like

GitBox

michael-o commented on pull request #324:
URL: https://github.com/apache/tomcat/pull/324#issuecomment-661045812


   I do not consider your points to be valid:
   * The format of the ETag is opaque, it is upto the caching instance to generate one
   * We cannot guarantee any strong ETags because WebResource is an interface which does not guarantee to properly return required information.


----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] stokito commented on pull request #324: Change ETag format to Nginx like

GitBox
In reply to this post by GitBox

stokito commented on pull request #324:
URL: https://github.com/apache/tomcat/pull/324#issuecomment-661083626


   > The format of the ETag is opaque, it is up to the caching instance to generate one
   
   Yes, you are right. In the same time too smart client may not send Ranges requests to download part of the file if it sees Weak ETag. This more semantical issue. But the main goal to to make ETag interoperable between different servers.
   
   > We cannot guarantee any strong ETags because WebResource is an interface which does not guarantee to properly return required information.
   
   But we can't guarantee even a Weak ETag ether. Anyway this is a default implementation and subclasses are free to override the method and return anything they need. I checked Catalina sources and there is two WebResource implementors: File and Jar.
   Both of them will 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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] michael-o commented on pull request #324: Change ETag format to Nginx like

GitBox
In reply to this post by GitBox

michael-o commented on pull request #324:
URL: https://github.com/apache/tomcat/pull/324#issuecomment-661263952


   Even if file and JAR are fine, the ETag implementation is on the `AbstractResource` and from here we cannot make any assumptions.


----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] michael-o commented on pull request #324: Change ETag format to Nginx like

GitBox
In reply to this post by GitBox

michael-o commented on pull request #324:
URL: https://github.com/apache/tomcat/pull/324#issuecomment-661265371


   Why don't you delete the ETags send from Tomcat in nginx config, use a caching module and rely on nginx ETags only?


----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] stokito commented on pull request #324: Change ETag format to Nginx like

GitBox
In reply to this post by GitBox

stokito commented on pull request #324:
URL: https://github.com/apache/tomcat/pull/324#issuecomment-661312436


   > we cannot make any assumptions.
   
   But we already did when implemented the default method :) I used Tomcat for about 10 years in different projects and once even patched it for company's internal needs but never saw the AbstractResource. Can you elaborate how can this break anything?
   The most important is to check Spring but it uses it's own hash based [ShallowEtagHeaderFilter](https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/web/filter/ShallowEtagHeaderFilter.java)
   
   > Why don't you delete the ETags send from Tomcat in nginx
   Never did that before but [here](https://stackoverflow.com/questions/33431722/in-nginx-etag-directive-doesnt-work-for-proxy-pass) is said that Nginx creates ETags only for local static files. I checked [docs](http://nginx.org/en/docs/http/ngx_http_proxy_module.html) and there is no anything about ETags.
   To create ETag from upstream Nginx needs a size (and it have it) and last mod time (can be retrieved from Last-Modifed but in my case BusyBox httpd not returns it).
   Anyway in my case some users will connect directly to upstream server.
   
   


----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] markt-asf commented on pull request #324: Change ETag format to Nginx like

GitBox
In reply to this post by GitBox

markt-asf commented on pull request #324:
URL: https://github.com/apache/tomcat/pull/324#issuecomment-661338844


   I think it is unlikely this patch will get applied as-is.
   I think a patch that made is easier to provide custom ETags (without changing the current behaviour by default) is much more likely to be accepted.
   The other option is to provide your own default servlet that provides the ETag.
   At this point a small refactoring of `DefaultServlet` so obtaining ETags goes through a single, override-able method looks like a possible solution.


----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] stokito edited a comment on pull request #324: Change ETag format to Nginx like

GitBox
In reply to this post by GitBox

stokito edited a comment on pull request #324:
URL: https://github.com/apache/tomcat/pull/324#issuecomment-661312436


   > we cannot make any assumptions.
   
   But we already did when implemented the default method :) I used Tomcat for about 10 years in different projects and once even patched it for company's internal needs but never saw the AbstractResource. Can you elaborate how can this break anything?
   The most important is to check Spring but it uses it's own hash based [ShallowEtagHeaderFilter](https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/web/filter/ShallowEtagHeaderFilter.java)
   
   > Why don't you delete the ETags send from Tomcat in nginx
   
   Never did that before but [here](https://stackoverflow.com/questions/33431722/in-nginx-etag-directive-doesnt-work-for-proxy-pass) is said that Nginx creates ETags only for local static files. I checked [docs](http://nginx.org/en/docs/http/ngx_http_proxy_module.html) and there is no anything about ETags.
   To create ETag from upstream Nginx needs a size (and it have it) and last mod time (can be retrieved from Last-Modifed but in my case BusyBox httpd not returns it).
   Anyway in my case some users will connect directly to upstream server.
   
   


----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] stokito commented on pull request #324: Change ETag format to Nginx like

GitBox
In reply to this post by GitBox

stokito commented on pull request #324:
URL: https://github.com/apache/tomcat/pull/324#issuecomment-666735779


   Well, as far I can tell it's quite safe to change the ETag generation schema. After update some servers may receive a spike because all files will be re-downloaded. Also may fail some integration tests that compares raw http response. Apache HTTPD just changed their ETag schema in v2.4 and I didn't found any mentioned problems that users had.
   
   Anyway the idea to make ETag schema configurable and make refactoring is wise. I'll try to implement this.


----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]