search

cloudflare / pingora

A library for building fast, reliable and evolvable network services.
Apache License 2.0
20.21k stars 1.1k forks source link

Empty response is cached when cached by OPTIONS method #265

Closed Object905 closed 2 weeks ago

Object905 commented 3 weeks ago

Describe the bug

I've found that my CDN (that queries pingora) sometimes responds with empty body and got cached both in cdn and pingora, which led to errors. After digging logs I've found out that HandleMiss::finish gets called with zero sized body on OPTIONS requests, so empty body was cached on OPTIONS and then served in GET

Pingora info

Pingora version: 0.0.2 Rust version: i.e. cargo 1.78.0 (54d8815d0 2024-03-26) Operating system version: e.g. Ubuntu 22.04, Debian 12.4

Expected results

OPTIONS gets replaced by GET when querying upstream with caching enabled, so full body is cached

Observed results

Empty body is cached.

drcaramelsyrup commented 3 weeks ago

Is your request_cache_filter allowing OPTIONS requests? You may only want to allow GET and HEAD requests (checking the method before calling session.cache.enable().

Object905 commented 3 weeks ago

Yes, that's eventually what I did to fix this.

Now I don't think that this behavior should be special handled by pingora, given how ProxyHttp trait is structured. Just used to nice defaults in varnish :slightly_smiling_face: I'ts my mistake that I unconditionally enabled cache on all methods (because I expected to get only GET|HEAD). You may close this, if you agree.

Weirdest thing is that my upstream responds with 400 status code for OPTIONS, and I'm only caching 200 upstream responses, but somehow responses got into HandleMiss::finish anyway. Perhaps cors preflight requests are to blame.

drcaramelsyrup commented 2 weeks ago

Glad it worked for you!

Now I don't think that this behavior should be special handled by pingora, given how ProxyHttp trait is structured

Yes, I agree. Ideally if the user chooses to cache, it would be nice if we only cached GET|HEAD by default, for now request cache filter is handling both cache enablement/selection and method filtering.

somehow responses got into HandleMiss::finish

Yeah, if you find that you're returning Uncacheable for those responses correctly in your response cache filter and still seeing this, let us know.