Roundup Tracker - Issues

Issue 2551175

classification
Make ETag content-encoding aware.
Type: behavior Severity: normal
Components: Web interface Versions: 2.2.0
process
Status: fixed fixed
:
: rouilj : marcus.priesch, rouilj
Priority: normal : Effort-Low

Created on 2021-12-01 18:06 by rouilj, last changed 2021-12-02 00:54 by rouilj.

Messages
msg7392 Author: [hidden] (rouilj) Date: 2021-12-01 18:06
When lookin at issue 2551173, it appears that the ETag header needs to be unique
by including the content-encoding. From that ticket:

====
While I was looking into this, it turns out that the ETag must be
different for different content-encodings.
https://datatracker.ietf.org/doc/html/rfc7232#section-2.3.3.  We do
explicitly set the vary header to include accept-encoding, but
apparently, that is not enough.

I am not sure how that affects the use of ETag and intermediate
caches. I suspect the easiest way to handle this is to take the ETag
and append the content-encoding. So

  ETag: 584f82231079e349031bbb853747df1c-gzip
  Content-Encoding: gzip

  ETag: 584f82231079e349031bbb853747df1c-br
  Content-Encoding: br

  ETag: 584f82231079e349031bbb853747df1c-zstd
  Content-Encoding: zstd

and then strip the suffix when comparing it server side. So as a
client you don't care about the ETag value, it still remains opaque.
But an intermediate cache will have three different copies that they
can return depending on what the accept-header is.
===

No released version of roundup does content-encoding, so this isn't a huge
issue but it should be fixed. I think it can be done in cgi/client.py::compress_encode
where the header manipulation occurs at the end. The ETag header should have been
added before this code is run.

Something like:

   current_etag = self.additional_headers['ETag']
   etag_end = current_etag.rindex('"')
   self.additional_headers['ETag'] = current_etag[:etag_end] + '-' + encoder \
       current_etag[etag_end:]

should work. Note that '-' doesn't show up in the output alphabet of hexdigest which is
used to encode the etag.

Tests will probably need to be done against the live server, but it may be possible to
piggyback of a rest test or a compression test.
msg7393 Author: [hidden] (rouilj) Date: 2021-12-01 23:19
Adding a little try/except/else block to the code above in the predicted spot
works. However the @etag value returned as part of the json payload doesn't have
the content-encoding suffix.

I claim this is not an issue as:

  the @etag value in the payload is not used by caches, proxies or other HTTP level
     transit nodes

  using the un-suffixed etag in an HTTP conditional header will (doesn't at the moment)
     still work and allow the change. This is fine as the use of etag in this case
     still works to prevent the lost update problem.

so I m not going to update the value of the payload's @etag value.
msg7394 Author: [hidden] (rouilj) Date: 2021-12-02 00:54
Fixed in changeset:   6539:f8df7fed18f6

Tests, docs updated as well.
History
Date User Action Args
2022-01-09 02:33:33rouiljlinkissue2551173 superseder
2021-12-02 00:54:32rouiljsetstatus: open -> fixed
resolution: fixed
messages: + msg7394
2021-12-01 23:19:19rouiljsetstatus: new -> open
assignee: rouilj
messages: + msg7393
nosy: + marcus.priesch
2021-12-01 18:06:56rouiljcreate