Re: [PATCH v3 0/4] Add support for if-none-match for cache responses
On Tue, Sep 08, 2020 at 07:14:10PM +0200, William Lallemand wrote: > On Tue, Sep 08, 2020 at 06:59:07PM +0200, William Lallemand wrote: > > On Tue, Sep 08, 2020 at 05:51:31PM +0200, Willy Tarreau wrote: > > > On Tue, Sep 08, 2020 at 05:21:34PM +0200, William Lallemand wrote: > > > > Also, when reading the RFC about the 304, I notice that they impose to > > > > remove some of the entity headers in the case of the weak etag, so the > > > > output is not exactly the same as the HEAD. > > > > https://tools.ietf.org/html/rfc2616#section-10.3.5 > > > > > > Warning, 2616 is totally outdated and must really die. Please use 7234 > > > for caching, 7231 for semantics and 7230 for messaging. > > > > > > Willy > > > > Sorry, I checked on 7230, but somehow I pasted the wrong one :-) > > > > Thanks, > > > > > > I definitively checked the wrong one, as the RFC now states a SHOULD > NOT for this, so that's not a requirement: > > https://tools.ietf.org/html/rfc7232#section-4.1 Actually, among the numerous changes that happened between 2616 and 723x, an important one was the removal of the distinction between entity header, message headers, representation headers etc, which were quite confusing since plenty got added in between. The only thing that remains are the hop-by-hop headers (as opposed to end-to-end), and these are designated by being referenced in the Connection header. Willy
Re: [PATCH v3 0/4] Add support for if-none-match for cache responses
On Tue, Sep 08, 2020 at 05:48:40PM +0200, Tim Düsterhus wrote: > William, > > Unfortunately that very much sounds like it is above my "pay grade" as a > community contributor to HAProxy. I *do not* plan on implementing this, > because I don't expect this to getting this right without investing much > effort. > I understand, that allowed us to check what are the requirements for this, so this was useful! > Feel free to take the first 3 patches if you want. I believe they are > good and useful for whoever is going to implement this. I'm going to > link the mailing list thread in the issue for reference. > Okay, thanks! -- William Lallemand
Re: [PATCH v3 0/4] Add support for if-none-match for cache responses
On Tue, Sep 08, 2020 at 06:59:07PM +0200, William Lallemand wrote: > On Tue, Sep 08, 2020 at 05:51:31PM +0200, Willy Tarreau wrote: > > On Tue, Sep 08, 2020 at 05:21:34PM +0200, William Lallemand wrote: > > > Also, when reading the RFC about the 304, I notice that they impose to > > > remove some of the entity headers in the case of the weak etag, so the > > > output is not exactly the same as the HEAD. > > > https://tools.ietf.org/html/rfc2616#section-10.3.5 > > > > Warning, 2616 is totally outdated and must really die. Please use 7234 > > for caching, 7231 for semantics and 7230 for messaging. > > > > Willy > > Sorry, I checked on 7230, but somehow I pasted the wrong one :-) > > Thanks, > > I definitively checked the wrong one, as the RFC now states a SHOULD NOT for this, so that's not a requirement: https://tools.ietf.org/html/rfc7232#section-4.1 -- William Lallemand
Re: [PATCH v3 0/4] Add support for if-none-match for cache responses
On Tue, Sep 08, 2020 at 05:51:31PM +0200, Willy Tarreau wrote: > On Tue, Sep 08, 2020 at 05:21:34PM +0200, William Lallemand wrote: > > Also, when reading the RFC about the 304, I notice that they impose to > > remove some of the entity headers in the case of the weak etag, so the > > output is not exactly the same as the HEAD. > > https://tools.ietf.org/html/rfc2616#section-10.3.5 > > Warning, 2616 is totally outdated and must really die. Please use 7234 > for caching, 7231 for semantics and 7230 for messaging. > > Willy Sorry, I checked on 7230, but somehow I pasted the wrong one :-) Thanks, -- William Lallemand
Re: [PATCH v3 0/4] Add support for if-none-match for cache responses
On Tue, Sep 08, 2020 at 05:21:34PM +0200, William Lallemand wrote: > Also, when reading the RFC about the 304, I notice that they impose to > remove some of the entity headers in the case of the weak etag, so the > output is not exactly the same as the HEAD. > https://tools.ietf.org/html/rfc2616#section-10.3.5 Warning, 2616 is totally outdated and must really die. Please use 7234 for caching, 7231 for semantics and 7230 for messaging. Willy
Re: [PATCH v3 0/4] Add support for if-none-match for cache responses
William, Am 08.09.20 um 17:21 schrieb William Lallemand: >> Yes, this generally makes sense to me. Unfortunately the code frankly >> is a foreign language to me here. >> >> I have not the slightest idea what steps I would need to perform to get >> the headers of the cached response within 'http_action_req_cache_use'. >> That's the primary reason why I implemented the logic within >> 'http_cache_io_handler' and not in 'http_action_req_cache_use'. > > Indeed it's kind of complicated in the current state of the cache, > because the data are chunked in the shctx so you can't use the htx > functions to do it, so we need to dump the headers before. > >> I would also say that doing this in 'http_cache_io_handler' is >> logically the correct place, because we are already dealing with the >> response here. However I understand that needing to malloc() the >> if-none-match is non-ideal. >> >> Do you have any advice how I can efficiently retrieve the ETag header >> from the cached response in 'http_action_req_cache_use'? > > > I think it's better to handle the headers in the action, like it's done > for the store_cache where the action store the headers and the filters > stores the body. > > So what could be done is to dump the headers directly from the action, > so the applet is created only in the case there is a body to dump, that > could even improve the performances. > > It will be a requirement in the future if we want to handle properly the > Vary header because we will need to chose the right object depending on > the header before setuping the appctx. Unfortunately that very much sounds like it is above my "pay grade" as a community contributor to HAProxy. I *do not* plan on implementing this, because I don't expect this to getting this right without investing much effort. Feel free to take the first 3 patches if you want. I believe they are good and useful for whoever is going to implement this. I'm going to link the mailing list thread in the issue for reference. > Also, when reading the RFC about the 304, I notice that they impose to > remove some of the entity headers in the case of the weak etag, so the > output is not exactly the same as the HEAD. > https://tools.ietf.org/html/rfc2616#section-10.3.5 Oh, yes, indeed. For a weak ETag some headers need to be dropped, good catch. Best regards Tim Düsterhus
Re: [PATCH v3 0/4] Add support for if-none-match for cache responses
On Tue, Sep 08, 2020 at 04:11:40PM +0200, Tim Düsterhus wrote:
> William,
>
> [Did you leave out the list intentionally?]
>
Oops, no sorry, I'll bounce my previous mail on the list.
> Am 08.09.20 um 14:40 schrieb William Lallemand:
> >> diff --git a/include/haproxy/applet-t.h b/include/haproxy/applet-t.h
> >> index 60f30c56f..7cccec977 100644
> >> --- a/include/haproxy/applet-t.h
> >> +++ b/include/haproxy/applet-t.h
> >> @@ -113,6 +113,7 @@ struct appctx {
> >>unsigned int offset;/* start offset of
> >> remaining data relative to beginning of the next block */
> >>unsigned int rem_data; /* Remaining bytes for the
> >> last data block (HTX only, 0 means process next block) */
> >>struct shared_block *next; /* The next block of data
> >> to be sent for this cache entry. */
> >> + struct ist if_none_match; /* The if-none-match
> >> request header. */
> >>} cache;
> >
> > In my opinion we only need a flag here, because the validation must be
> > done near the lookup. It's not useful to do a copy of the header string.
>
> Yes, this generally makes sense to me. Unfortunately the code frankly
> is a foreign language to me here.
>
> I have not the slightest idea what steps I would need to perform to get
> the headers of the cached response within 'http_action_req_cache_use'.
> That's the primary reason why I implemented the logic within
> 'http_cache_io_handler' and not in 'http_action_req_cache_use'.
Indeed it's kind of complicated in the current state of the cache,
because the data are chunked in the shctx so you can't use the htx
functions to do it, so we need to dump the headers before.
> I would also say that doing this in 'http_cache_io_handler' is
> logically the correct place, because we are already dealing with the
> response here. However I understand that needing to malloc() the
> if-none-match is non-ideal.
>
> Do you have any advice how I can efficiently retrieve the ETag header
> from the cached response in 'http_action_req_cache_use'?
I think it's better to handle the headers in the action, like it's done
for the store_cache where the action store the headers and the filters
stores the body.
So what could be done is to dump the headers directly from the action,
so the applet is created only in the case there is a body to dump, that
could even improve the performances.
It will be a requirement in the future if we want to handle properly the
Vary header because we will need to chose the right object depending on
the header before setuping the appctx.
Also, when reading the RFC about the 304, I notice that they impose to
remove some of the entity headers in the case of the weak etag, so the
output is not exactly the same as the HEAD.
https://tools.ietf.org/html/rfc2616#section-10.3.5
Regards,
--
William Lallemand
Re: [PATCH v3 0/4] Add support for if-none-match for cache responses
William, Am 02.09.20 um 07:50 schrieb Christopher Faulet: > It seems good for me. If it's good too for William, I will merge it. > Did you have time to take a look, yet? The final patch version is the 'v3' one. Best regards Tim Düsterhus
Re: [PATCH v3 0/4] Add support for if-none-match for cache responses
Le 01/09/2020 à 18:32, Tim Duesterhus a écrit : William, Christopher, Am 01.09.20 um 10:29 schrieb Christopher Faulet: There is no applet for uncached requests. So there is no reason to call the release callback function. Looking at your 4th patch, you should manually release 'if_none_match' variable for uncached requests in the http_action_req_cache_use() function. Ah, you are correct of course. What solution would you prefer here? 1. Manually freeing in the else part of `if(res)` 2. Moving the whole If-N-M logic down into the `si_register_handler` part? I initially didn't do this, because I assumed that this was within a lock, but now I realize that there's an unlock before the `si_register_handler`. The second one is probably the best. Okay, I've adjusted this. I believe the patch is ready for a final review and merge now. Valgrind is happy and both the old and new reg-tests pass. Best regards It seems good for me. If it's good too for William, I will merge it. Thanks, -- Christopher Faulet
[PATCH v3 0/4] Add support for if-none-match for cache responses
William, Christopher, Am 01.09.20 um 10:29 schrieb Christopher Faulet: >>> There is no applet for uncached requests. So there is no reason to call >>> the release callback function. Looking at your 4th patch, you should >>> manually release 'if_none_match' variable for uncached requests in the >>> http_action_req_cache_use() function. >> >> Ah, you are correct of course. What solution would you prefer here? >> >> 1. Manually freeing in the else part of `if(res)` >> 2. Moving the whole If-N-M logic down into the `si_register_handler` >> part? I initially didn't do this, because I assumed that this was within >> a lock, but now I realize that there's an unlock before the >> `si_register_handler`. >> > > The second one is probably the best. Okay, I've adjusted this. I believe the patch is ready for a final review and merge now. Valgrind is happy and both the old and new reg-tests pass. Best regards Tim Düsterhus (4): MINOR: http: Add `enum etag_type http_get_etag_type(const struct ist)` CLEANUP: compression: Make use of http_get_etag_type() MINOR: http: Add `int etag_compare(struct ist, struct ist, int)` MEDIUM: cache: Add support for if-none-match for cache responses include/haproxy/applet-t.h| 1 + include/haproxy/http-t.h | 6 +++ include/haproxy/http.h| 23 ++ reg-tests/cache/if-none-match.vtc | 72 +++ src/cache.c | 41 -- src/flt_http_comp.c | 5 +-- src/http.c| 32 ++ 7 files changed, 173 insertions(+), 7 deletions(-) create mode 100644 reg-tests/cache/if-none-match.vtc -- 2.28.0
