Proposal: Add CachePolicy interface to allow for custom cache behavior by capoferro · Pull Request #17362 · envoyproxy/envoy

capoferro · 2021-07-15T15:51:01Z

Commit Message:
Add CachePolicy interface

Additional Description:
In order to support custom cache behavior, we need to provide an extension point that allows the results of basic cache semantics to be modified prior to acting on the calculated results.

A CachePolicy implementation contains logic that modifies how the CacheFilter interacts with an implementation of HttpCache. This includes key generation, cacheability and cache entry usability concerns.

A CachePolicy implementation does not contain:

Logic that handles encoding or decoding a request or response. That belongs in CacheFilter.
Logic specific to a cache. That belongs in HttpCache and related utils.

After we agree on the interface, I'll follow this PR with CachePolicyImpl and full integration in the CacheFilter.

Risk Level: None. Code is not in use.

Signed-off-by: Josiah Kiehl <josiah@capoferro.net>

jmarantz

This is basically an RFC?

I assume you'll modify the caching interface to use the policy in its API?

jmarantz · 2021-07-15T22:00:01Z

source/extensions/filters/http/cache/cache_policy.h

+  // createCacheKey calculates the lookup key for storing the entry in the cache.
+  virtual Key createCacheKey(const Envoy::Http::RequestHeaderMap& request_headers) PURE;
+
+  // requestCacheable modifies the cacheability of the response during


Use doxygen style comments for this interface. Based on the interface I think you mean to say:

/** * @return whether the request is cacheable. * /

Also should the method be const?

I'm a bit wary of const at the interface level, as I don't want to inadvertently restrict what a custom CachePolicy implementation can do. That said, I personally have no plans to introduce anything that modifies CachePolicy in createCacheKey, requestCacheable, responseCacheable or computeCacheEntryUsability, and could add const to all 4 of those.

What do you think? Should I add const to all of the above?

source/extensions/filters/http/cache/cache_policy.h

capoferro · 2021-07-16T13:27:27Z

This is basically an RFC?

Yeah, more or less. I want some feedback on the interface before I dive in and do the full integration. I have a version of everything implemented, across the CacheFilter, but it'll take some work to clean it up and port it to OSS Envoy, so having feedback on the interface before I do that work will make the process more efficient and make sure I'm implementing with a fully hashed out design.

Signed-off-by: Josiah Kiehl <josiah@capoferro.net>

penguingao · 2021-07-19T17:38:45Z

cc myself @penguingao

penguingao

Could you clarify the commit message a bit? I think the part that needs some clarification is "Logic that provides RFC compliance for caching semantics. That belongs in CacheFilter and related utils."

My understanding is that CachePolicy is used to override RFC7234 behavior, but how is the RFC7234 behavior implemented? I'd naively think that the easiest way to abstract this is to have an Rfc7234CachePolicy implementation, which is used by default, and custom behaviors are introduced with an override of the CachePolicy interface. This seems to not comply with the commit message.

source/extensions/filters/http/cache/cache_policy.h

capoferro · 2021-07-19T19:37:51Z

Could you clarify the commit message a bit? I think the part that needs some clarification is "Logic that provides RFC compliance for caching semantics. That belongs in CacheFilter and related utils."
My understanding is that CachePolicy is used to override RFC7234 behavior, but how is the RFC7234 behavior implemented? I'd naively think that the easiest way to abstract this is to have an Rfc7234CachePolicy implementation, which is used by default, and custom behaviors are introduced with an override of the CachePolicy interface. This seems to not comply with the commit message.

The idea is that the RFC behavior is provided in the CacheFilter and/or HttpCache base class and then the CachePolicy functions are called afterward to allow the custom, supplementary, logic to change the RFC-compliant result, if necessary.

This does mean that if the CachePolicy intends to completely replace RFC behavior, then the RFC logic being run at all will be wasteful. For example, if we have a policy that forces caching of all content, then it doesn't matter what the RFC logic decides, we're going to decide to cache it anyway, and so we may as well not run the RFC logic.

We certainly could contain all of the RFC compliant logic in an implementation of CachePolicy, I was just thinking it'd be better to have the CachePolicy be where exceptions to the rules live, rather than where the rules live.

jmarantz

Will take another look once you address @penguingao 's comments

/wait

penguingao · 2021-07-21T13:36:46Z

We certainly could contain all of the RFC compliant logic in an implementation of CachePolicy, I was just thinking it'd be better to have the CachePolicy be where exceptions to the rules live, rather than where the rules live.

I would prefer to have the option to completely replace CachePolicy implementations by an extension. Exceptions are not rare when it comes to HTTP caching. As you said, it's wasteful to make the same decision twice in many cases. With CachePolicy just making all the decisions, it provides a cleaner interface (no need to parse cache-control and pass in - each CachePolicy can just do it inside if it cares about the header), and cleaner life cycle expectations on the interface (each function just determines the behavior, instead of overriding a previous result it does not have exposure to).

Given no "default behavior", the CachePolicy extensions cannot decide what it is "modifying", meaning as soon as you have a CachePolicy extension written, it already has to make the full decision, not exceptions, because the current API does not provide the "default decision" into the methods. You could fix this by making the methods return a tuple of { bool override_decision, ResultType result} for each method, but it's rather messy.

capoferro · 2021-07-28T14:47:58Z

I would prefer to have the option to completely replace CachePolicy implementations by an extension.

Ok, this makes sense to me. I've reworded the comments to reflect this, and will keep this in mind for the full integration into CachePolicy.

Signed-off-by: Josiah Kiehl <josiah@capoferro.net>

jmarantz · 2021-08-02T12:25:23Z

@penguingao you good with this now?

penguingao · 2021-08-03T18:30:27Z

@penguingao you good with this now?

Yup, this LGTM now.

jmarantz

/assign-from @envoyproxy/senior-maintainers

source/extensions/filters/http/cache/cache_policy.h

repokitteh-read-only · 2021-08-03T19:14:07Z

@envoyproxy/senior-maintainers assignee is @zuercher

🐱

Caused by: a #17362 (review) was submitted by @jmarantz.

see: more, trace.

Signed-off-by: Josiah Kiehl <josiah@capoferro.net>

zuercher

Looks good.

zuercher · 2021-08-05T17:22:16Z

/retest

repokitteh-read-only · 2021-08-05T17:22:20Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17362 (comment) was created by @zuercher.

see: more, trace.

* main: (687 commits) ci: set build debug information from env (envoyproxy#17635) ext_authz: do the authentication even the direct response is set (envoyproxy#17546) upstream: various cleanups in connection pool code (envoyproxy#17644) owners: promote Dmitry to maintainer (envoyproxy#17642) quiche: client session supports creating bidi stream (envoyproxy#17543) Update HTTP/2 METADATA documentation. (envoyproxy#17637) ext_proc: Check validity of the :status header (envoyproxy#17596) test: add ASSERT indicating that gRPC stream has not been started yet (envoyproxy#17614) ensure that the inline cookie header will be folded correctly (envoyproxy#17560) cluster_manager: Make ClusterEntry a class instead of a struct (envoyproxy#17616) owners: make Raúl a Thrift senior extension maintainer (envoyproxy#17641) quiche: update QUICHE dependency (envoyproxy#17618) Delete mock for removed RouteEntry::perFilterConfig() method (envoyproxy#17623) REPO_LAYOUT.md: fix outdated link (envoyproxy#17626) hcm: forbid use of detection extensions with use_remote_addr/xff_num_trusted_hops (envoyproxy#17558) thrift proxy: add request shadowing support (envoyproxy#17544) ext_proc: Ensure that timer is always cancelled (envoyproxy#17569) Proposal: Add CachePolicy interface to allow for custom cache behavior (envoyproxy#17362) proto: fix verify to point at v3 only (envoyproxy#17622) api: move generic matcher proto to its own package (envoyproxy#17096) ...

* main: (687 commits) ci: set build debug information from env (envoyproxy#17635) ext_authz: do the authentication even the direct response is set (envoyproxy#17546) upstream: various cleanups in connection pool code (envoyproxy#17644) owners: promote Dmitry to maintainer (envoyproxy#17642) quiche: client session supports creating bidi stream (envoyproxy#17543) Update HTTP/2 METADATA documentation. (envoyproxy#17637) ext_proc: Check validity of the :status header (envoyproxy#17596) test: add ASSERT indicating that gRPC stream has not been started yet (envoyproxy#17614) ensure that the inline cookie header will be folded correctly (envoyproxy#17560) cluster_manager: Make ClusterEntry a class instead of a struct (envoyproxy#17616) owners: make Raúl a Thrift senior extension maintainer (envoyproxy#17641) quiche: update QUICHE dependency (envoyproxy#17618) Delete mock for removed RouteEntry::perFilterConfig() method (envoyproxy#17623) REPO_LAYOUT.md: fix outdated link (envoyproxy#17626) hcm: forbid use of detection extensions with use_remote_addr/xff_num_trusted_hops (envoyproxy#17558) thrift proxy: add request shadowing support (envoyproxy#17544) ext_proc: Ensure that timer is always cancelled (envoyproxy#17569) Proposal: Add CachePolicy interface to allow for custom cache behavior (envoyproxy#17362) proto: fix verify to point at v3 only (envoyproxy#17622) api: move generic matcher proto to its own package (envoyproxy#17096) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

envoyproxy#17362) Commit Message: Add CachePolicy interface Additional Description: In order to support custom cache behavior, we need to provide an extension point that allows the results of basic cache semantics to be modified prior to acting on the calculated results. A CachePolicy implementation contains logic that modifies how the CacheFilter interacts with an implementation of HttpCache. This includes key generation, cacheability and cache entry usability concerns. A CachePolicy implementation does not contain: Logic that handles encoding or decoding a request or response. That belongs in CacheFilter. Logic specific to a cache. That belongs in HttpCache and related utils. After we agree on the interface, I'll follow this PR with CachePolicyImpl and full integration in the CacheFilter. Risk Level: None. Code is not in use. Signed-off-by: Josiah Kiehl <josiah@capoferro.net>

Add CachePolicy interface

a7dd6fd

Signed-off-by: Josiah Kiehl <josiah@capoferro.net>

capoferro requested a review from jmarantz as a code owner July 15, 2021 15:51

capoferro added 3 commits July 15, 2021 14:18

Add CachePolicyCallbacks

193385c

Signed-off-by: Josiah Kiehl <josiah@capoferro.net>

Fix BUILD

a60636a

Signed-off-by: Josiah Kiehl <josiah@capoferro.net>

Fix format

5bf9821

Signed-off-by: Josiah Kiehl <josiah@capoferro.net>

jmarantz reviewed Jul 15, 2021

View reviewed changes

Remove Envoy:: qualifiers

2a82af3

Signed-off-by: Josiah Kiehl <josiah@capoferro.net>

snowp assigned jmarantz Jul 16, 2021

Doxygen comments

7e5c6a5

Signed-off-by: Josiah Kiehl <josiah@capoferro.net>

penguingao reviewed Jul 19, 2021

View reviewed changes

jmarantz reviewed Jul 21, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 21, 2021

CachePolicy comment updates

7e91633

Signed-off-by: Josiah Kiehl <josiah@capoferro.net>

repokitteh-read-only bot removed the waiting label Jul 28, 2021

jmarantz previously approved these changes Aug 3, 2021

View reviewed changes

source/extensions/filters/http/cache/cache_policy.h Outdated Show resolved Hide resolved

repokitteh-read-only bot assigned zuercher Aug 3, 2021

Fix parameter name

d8acb03

Signed-off-by: Josiah Kiehl <josiah@capoferro.net>

capoferro dismissed jmarantz’s stale review via d8acb03 August 4, 2021 17:42

Fix format

891e87c

Signed-off-by: Josiah Kiehl <josiah@capoferro.net>

zuercher approved these changes Aug 5, 2021

View reviewed changes

jmarantz merged commit 0075682 into envoyproxy:main Aug 8, 2021

Conversation

capoferro commented Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

jmarantz Jul 15, 2021

Choose a reason for hiding this comment

Uh oh!

capoferro Jul 16, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

capoferro commented Jul 16, 2021

Uh oh!

penguingao commented Jul 19, 2021

Uh oh!

penguingao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

capoferro commented Jul 19, 2021

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

penguingao commented Jul 21, 2021

Uh oh!

capoferro commented Jul 28, 2021

Uh oh!

jmarantz commented Aug 2, 2021

Uh oh!

penguingao commented Aug 3, 2021

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

repokitteh-read-only bot commented Aug 3, 2021

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

zuercher commented Aug 5, 2021

Uh oh!

repokitteh-read-only bot commented Aug 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

capoferro commented Jul 15, 2021 •

edited

Loading