Support force cache even when server doesn't set the Date header #6175
Conversation
c2zwdjnlcg
force-pushed
the
force-cache-refresh
branch
2 times, most recently
from
218979d
to
9b10ddd
Compare
|
@c2zwdjnlcg thanks for looking into this. It would be great if you could clarify this behavior and point to any resources/ rfc that explain this. I would have imagined that the server should set the necessary headers. On the forced cache 304 scenario, that could be a valid issue. Some tests would be helpful for both. Thanks! |
|
Unfortunately not all servers set Date headers. The RFC says the the date header is required unless the server doesn't have a valid clock
But beyond that, it's just a fact of life that not all servers generate a date header. Either way though here the current behavior is problematic, the first time the call gets made the request is successful and cached but once it expires, it can never be refreshed and can never be cleared. My thinking is that since forced really doesn't require all these headers, ignoring these headers is the right thing to do. This was the behavior in older versions of OPA (< 0.46). Though there were other cache related bugs there as well. Where do you suggest I add tests? Is http_test.go the right place? |
topdown/http.go
Outdated
| @@ -841,7 +841,11 @@ func (c *interQueryCache) checkHTTPSendInterQueryCache() (ast.Value, error) { | |||
|
|
|||
| headers, err := parseResponseHeaders(cachedRespData.Headers) | |||
| if err != nil { | |||
| return nil, err | |||
| if forceCaching(c.forceCacheParams) { | |||
There was a problem hiding this comment.
parseResponseHeaders parses at the etag value as well for instance which is needed in the revalidation server request. So we still need that even if the parsing of the time/date related headers gives an error.
There was a problem hiding this comment.
Yeah, I'm sort of assuming here that if a server isn't going to support dates it's not going to support etags either. But maybe that's a wrong assumption. What would you suggest here? Should I split up the date and etag processing so that one can succeed without the other? Or should I just make parseResponseHeaders less strict so that it doesn't throw errors but just ignores missing or malformed headers, maybe with a log message?
There was a problem hiding this comment.
I think the only values needed are etag and last-modified. Let's see what parseResponseHeaders is doing and if the values it's parsing are actually used anywhere.
There was a problem hiding this comment.
I confirmed that parseResponseHeaders is only used with revalidateCachedResponse which only looks at etagand last-modified both of which can be parsed without potential errors. I've made that change to this PR. See what you think.
Yes. |
c2zwdjnlcg
force-pushed
the
force-cache-refresh
branch
from
9b10ddd
to
4873e6d
Compare
|
Added some tests |
c2zwdjnlcg
force-pushed
the
force-cache-refresh
branch
from
4873e6d
to
029419d
Compare
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
c2zwdjnlcg
force-pushed
the
force-cache-refresh
branch
3 times, most recently
from
be686d5
to
2f84bc3
Compare
ashutosh-narkar
force-pushed
the
force-cache-refresh
branch
from
2f84bc3
to
8e437b3
Compare
| if ast.String(tc.response).Compare(resResponse.Value) != 0 { | ||
| t.Fatalf("Expected response on query %d to be %v, got %v", i, tc.response, resResponse.String()) | ||
| } | ||
|
|
There was a problem hiding this comment.
The idea behind the logic below seems to simulate an expired cache entry and hence trigger the revalidation logic for the forced cache case. It would be be helpful to add a note about this for reference.
c2zwdjnlcg
force-pushed
the
force-cache-refresh
branch
2 times, most recently
from
66f0b55
to
7ac99e3
Compare
|
Test failure seems unrelated, but i can't reproduce locally. |
Yeah they're unrelated to your changes. |
This fixes a bug where when a upstream server doesn't set a Date header the response will be cached but then once the cache is expired it never gets refreshed because parseResponseHeaders returns a no date header error This also makes it so that the force_cache_duration_seconds is respected when an upstream returns a 304 not modified Signed-off-by: Peter <[email protected]>
ashutosh-narkar
force-pushed
the
force-cache-refresh
branch
from
7ac99e3
to
c7583d3
Compare
This fixes a bug where when a upstream server doesn't set a Date header the response will be cached but then once the cache is expired it never gets refreshed because parseResponseHeaders returns a no date header error
This also makes it so that the force_cache_duration_seconds is respected when an upstream returns a 304 not modified