Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http.send: force_cache option does not override Cache-Control: no-store from server #2841

Closed
anderseknert opened this issue Oct 29, 2020 · 3 comments · Fixed by #2853
Closed

Comments

@anderseknert
Copy link
Member

Expected Behavior

force_cache/force_cache_duration_seconds is documented as fields that "can be used to override the cache directives defined by the server".

Actual Behavior

The presence of a Cache-Control: no-store directive is still respected, and the response will not be cached.

Steps to Reproduce the Problem

OPA version 0.24.0+

Create an endpoint with a cache-control header including no-store in the response. Call it from http.send with force_cache/force_cache_duration_seconds set for forced caching. The endpoint will be called for each invocation of the policy.

@ashutosh-narkar
Copy link
Member

The initial intent of the force_cache/force_cache_duration_seconds fields was to allow the user to control the cache duration. If we want the user to have more control over other cache directives (eg. no-store), we would have to add another field like force_cache_no_store(bool) and use that to override the no-store directive. WDYT ?

@anderseknert
Copy link
Member Author

Thanks for providing some background on that @ashutosh-narkar 👍 I think I'm somewhat confused with having an option named force_cache not actually forcing responses to be cached but currently being a no-op requiring extra parameters for anything to be cached. The docs for force_cache currently states:

Cache HTTP response across OPA queries and override cache directives defined by the server.

As opposed to the cache option which caches without overriding server directives. So while a new option would solve my problem at hand I think it should be considered whether it's worth adding a fourth option for caching rather than having this behavior included in force_cache directly, as I think that would be a reasonable expectation from something named "force".

@ashutosh-narkar
Copy link
Member

That's a good point @anderseknert. I imagined force_cache being set with force_cache_duration_seconds and other parameters we introduce in the future to control more of the caching behavior. Currently we only use two caching directives in http.send, namely no-store and max-age. We allow users to override the latter with force_cache_duration_seconds and if we introduce more directives we can add new parameters to http.send. But I agree that we can have force_ cache as an implicit override of the no-store directive instead of adding a new parameter.

anderseknert added a commit to Bisnode/opa that referenced this issue Nov 3, 2020
Fixes open-policy-agent#2841

Signed-off-by: Anders Eknert <anders.eknert@bisnode.com>
patrick-east pushed a commit that referenced this issue Nov 4, 2020
Fixes #2841

Signed-off-by: Anders Eknert <anders.eknert@bisnode.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants