filter: Add an incomplete pluggable HTTP caching filter (CacheFilter). #7198

toddmgreer · 2019-06-07T01:05:11Z

Description: Adds CacheFilter, an HTTP filter that caches response. To support a variety of needs, storage is delegated to a plugin cache implementation, but CacheFilter handles the detailed logic of determining what can be cached, whether to serve a particular cache entry for a particular request, and validation.

Design: eCache: a multi-backend HTTP cache for Envoy

This initial submission establishes the framework for cache implementations (including a simple example plugin), and handles basic integration with Envoy as an HTTP filter. It doesn't yet handle enough of the RFCs to be used, and the tests only validate basic functionality. While there is substantial work remaining, merging this now will make it easier for development of cache plugins to happen in wile the filter side is finished.

Risk Level: Low. Since there are no (non-test) uses of this new filter, it would be difficult for it to break anything. The only changes to actually executed code are two new HeaderMap inline headers and the new static factory registration.

Testing: All components have basic unit tests. Integration tests and more unit tests will be needed before CacheFilter is ready for production use.

Docs Changes: The new config proto (envoy.config.filter.http.cache.v2alpha.Cache) has reasonable documentation, and has been added to docs/build.sh.

Release Notes: N/A

#868

…proxy#868). Signed-off-by: Todd Greer <tgreer@google.com>

…proxy#868) Signed-off-by: Todd Greer <tgreer@google.com>

Signed-off-by: Todd Greer <tgreer@google.com>

mattklein123 · 2019-06-10T21:57:31Z

@envoyproxy/maintainers any volunteers to shepherd review on this? I don't have the bandwidth to take this on.

Signed-off-by: Todd Greer <tgreer@google.com>

toddmgreer · 2019-06-11T18:36:55Z

/retest

repokitteh-read-only · 2019-06-11T18:36:58Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #7198 (comment) was created by @toddmgreer.

see: more, trace.

stale · 2019-06-18T20:48:19Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

jmarantz · 2019-06-18T20:50:48Z

/wait

jmarantz

flushing comments

source/extensions/filters/http/cache/cache_filter.cc

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

source/extensions/filters/http/cache/http_cache.cc

jmarantz

OK I think I'm done with this pass; can you address these and I'll dig deeper?

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

source/extensions/filters/http/cache/http_cache_utils.cc

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

* source/common/http/headers.h: Signed-off-by: Todd Greer <tgreer@google.com>

Signed-off-by: Todd Greer <tgreer@google.com>

… ref. Signed-off-by: Todd Greer <tgreer@google.com>

enozcan · 2020-01-25T11:15:31Z

Hi @toddmgreer, I have implemented http cache backed by Hazelcast IMDG with the completed features on the filter side (repository). I’m planning to make some changes (i.e. formatting the code style to make it consistent with Envoy cpp style, etc. ) but the caching logic will be more or less the same. What about using it as a reference implementation or the default provider? I believe implementing the filter hand in hand with a provider would make sense and once the filter is ready for production, a plugin can be provided as well.

mattklein123 · 2020-01-27T21:47:59Z

/azp run envoy-linux

azure-pipelines · 2020-01-27T21:48:11Z

Azure Pipelines successfully started running 1 pipeline(s).

toddmgreer · 2020-01-27T22:29:39Z

Hi @enozcan, that's fantastic--I'm excited to see new work building on CacheFilter! HazelcastHttpCache seems like a perfect candidate to be included with Envoy (though that will ultimately be up to Envoy's senior maintainers). I don't intend for there to be a default provider / HttpCache implementation--the configuration should specify something, but HazelcastHttpCache makes perfect sense as one of the built-in options.

Any complaints about the interfaces? Were they clear enough?

Thank you for extending Envoy/CacheFilter!

toddmgreer · 2020-01-28T19:40:32Z

/retest

repokitteh-read-only · 2020-01-28T19:40:36Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #7198 (comment) was created by @toddmgreer.

see: more, trace.

mattklein123

Thanks, left some more comments. At a high level this LGTM. TBH my advice for getting this merged at this point is to start splitting this into small PRs that we can re-review without the massive size of this PR. You could easily start with a PR for the utilities, then the cache itself, then the filter, then the simple cache implementation, etc. I think if we do that @jmarantz and I can commit to efficiently re-reviewing and we can land this? WDYT? Thank you for all your work here.

/wait

docs/root/configuration/http/http_filters/cache_filter.rst

mattklein123 · 2020-01-28T20:28:07Z

source/extensions/filters/http/cache/cache_filter.cc

+
+  CacheFilterSharedPtr self = shared_from_this();
+  ENVOY_STREAM_LOG(debug, "CacheFilter::decodeHeaders starting lookup", *decoder_callbacks_);
+  lookup_->getHeaders([self](LookupResult&& result) { onHeadersAsync(self, std::move(result)); });


Can this async lambda ever fire inline? I could imagine it can and we need to deal with this case?

mattklein123 · 2020-01-28T20:29:01Z

source/extensions/filters/http/cache/cache_filter.cc

+Http::FilterDataStatus CacheFilter::encodeData(Buffer::Instance& data, bool end_stream) {
+  if (insert_) {
+    ENVOY_STREAM_LOG(debug, "CacheFilter::encodeHeaders inserting body", *encoder_callbacks_);
+    // TODO(toddmgreer): Wait for the cache if necessary.


In addition to waiting for the cache, you will also have to apply back pressure potentially. Can you make sure this is covered by an open issue?

mattklein123 · 2020-01-28T20:30:13Z

source/extensions/filters/http/cache/cache_filter.cc

+  case CacheEntryStatus::RequiresValidation:
+  case CacheEntryStatus::FoundNotModified:
+  case CacheEntryStatus::UnsatisfiableRange:
+    ASSERT(false); // We don't yet return or support these codes.


Switch to NOT_IMPLEMENTED macro

mattklein123 · 2020-01-28T20:31:46Z

source/extensions/filters/http/cache/cache_filter.cc

+    ASSERT(false, "CacheFilter doesn't call getBody unless there's more body to get, so this is a "
+                  "bogus callback.");
+    decoder_callbacks_->resetStream();
+    return;


Please don't add error handling for cases like this. A plain ASSERT is fine before proceeding. If something can't happen I would just assert it and move on. Same below. If we want to actually defend against bad cache implementations then I would remove the ASSERT(falses) statements throughout and actually handle things, and probably have stats eventually (so add a TODO for that).

mattklein123 · 2020-01-28T20:33:44Z

source/extensions/filters/http/cache/cache_filter.cc

+void CacheFilter::onDestroy() {
+  lookup_ = nullptr;
+  insert_ = nullptr;
+}


Can you add a small comment here that we assume that destroying the handles cancels all async callbacks, etc.?

mattklein123 · 2020-01-28T20:34:48Z

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

+namespace Cache {
+
+class CacheFilterFactory
+    : public Common::FactoryBase<envoy::config::filter::http::cache::v2::CacheConfig> {


OK not sure what is going on here. I think @htuch is out this week so if this isn't burning urgent we should ask him next week before merging.

source/extensions/filters/http/cache/simple_http_cache.cc

mattklein123 · 2020-01-28T20:41:30Z

tools/spelling_dictionary.txt

+asctime
+dechunk
+dechunked
+qdtext


I agree. There is already an issue tracking this which we should just implement in the checker.

enozcan · 2020-02-07T14:07:24Z

HI @toddmgreer,

Any complaints about the interfaces? Were they clear enough?

Overall everything was fine but I wonder a few things. I followed the design doc during implementation but it seems like out of date. Some updates are not reflected on the doc. If it is planned to be maintained, I can help to make it up-to-date by suggesting editions on the doc page. Also, as stated in the previous comments, vary headers issue is a little bit complicated. A sequence diagram for it just like the existing one in the doc might be extremely helpful for providers.

Also, in source/docs/cache_filter_plugins.md it's stated that:

If you write a new cache storage implementation, please add it to the Envoy
repository if possible.

Which directory should I address when including a new plugin in Envoy repository? May be a subdirectory like filters/http/cache/hazelcast_http_cache? Also the documentation for the plugin, is source/docs/ the right place?

toddmgreer · 2020-02-07T15:41:32Z

Hi Enes, Thank you! I'm glad to hear the interfaces make sense. I hope the stale design doc didn't cause you much inconvenience--sorry about that. @mattklein123 indicated that it's preferred to include docs in the repo instead of linking to external docs, so updating should happen there. As the functionality gaps get plugged, the docs will need to be updated--I agree that diagrams from the design doc will probably need to be copied over. As you say, having a subdirectory per plugin (e.g. filters/http/cache/hazelcast_http_cache), seems like a good approach. I'm going to move SimpleHttpCache to its own dir shortly. You should be able to add HazelcastHttpCache as soon as PR #9878 lands. As you find things that need improvement, please continue to let me know, file bugs, and/or submit PRs.

…

On Fri, Feb 7, 2020 at 6:07 AM Enes Ozcan ***@***.***> wrote: HI @toddmgreer <https://github.com/toddmgreer>, Any complaints about the interfaces? Were they clear enough? Overall everything was fine but I wonder a few things. I followed the design doc <https://docs.google.com/document/d/1WPuim_GzhfdsnIj_tf-fIeutK0jO4aVQfVrLJFoLN3g/view> during implementation but it seems like out of date. Some updates are not reflected on the doc. If it is planned to be maintained, I can help to make it up-to-date by suggesting editions on the doc page. Also, as stated in the previous comments, vary headers issue is a little bit complicated. A sequence diagram for it just like the existing one in the doc might be extremely helpful for providers. Also, in source/docs/cache_filter_plugins.md it's stated that: If you write a new cache storage implementation, please add it to the Envoy repository if possible. Which directory should I address when including a new plugin in Envoy repository? May be a subdirectory like filters/http/cache/hazelcast_http_cache? Also the documentation for the plugin, is source/docs/ the right place? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7198?email_source=notifications&email_token=AFRAWPO4QWHGBAWAA4RO2PLRBVTKBA5CNFSM4HVNTKI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELDANUQ#issuecomment-583403218>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFRAWPPPZP4IIQ3LKGQR6SLRBVTKBANCNFSM4HVNTKIQ> .

toddmgreer · 2020-02-21T20:04:17Z

All of the changes in this PR have now been merged through other PRs, with the exception of support for multithreaded cache storage implementations (removed per reviewer request in #10019).

…nvoyproxy#7198 Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

toddmgreer added 7 commits June 6, 2019 16:27

Add 'age' and 'expires' inline headers to support HTTP caching (envoy…

55ea9a2

…proxy#868). Signed-off-by: Todd Greer <tgreer@google.com>

Add an incomplete pluggable HTTP caching filter (CacheFilter). (envoy…

f1a89f4

…proxy#868) Signed-off-by: Todd Greer <tgreer@google.com>

Add cache/config to BUILD file.

d5bb1c5

Signed-off-by: Todd Greer <tgreer@google.com>

Fix formatting.

72f3bea

Signed-off-by: Todd Greer <tgreer@google.com>

Fix spelling, and expand spellcheck's dictionary.

0611bed

Signed-off-by: Todd Greer <tgreer@google.com>

Rename config_lib to config.

3123971

Signed-off-by: Todd Greer <tgreer@google.com>

Add some RFC terms to spelling_dictionary.txt.

424e5bc

Signed-off-by: Todd Greer <tgreer@google.com>

jmarantz self-assigned this Jun 10, 2019

toddmgreer added 2 commits June 10, 2019 16:54

Address clang-tidy findings.

d77ff8c

Signed-off-by: Todd Greer <tgreer@google.com>

Remove cache_filter_test until it has actual tests to run.

b144193

Signed-off-by: Todd Greer <tgreer@google.com>

toddmgreer marked this pull request as ready for review June 11, 2019 19:56

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 18, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jun 18, 2019

repokitteh-read-only bot added the waiting label Jun 18, 2019

jmarantz reviewed Jun 19, 2019

View reviewed changes

Address first batch of comments. More to come.

ee2431a

* source/common/http/headers.h: Signed-off-by: Todd Greer <tgreer@google.com>

repokitteh-read-only bot removed the waiting label Jun 19, 2019

toddmgreer added 7 commits June 21, 2019 12:31

Style cleanups requested in first review, and a few more unit tests.

f432612

Signed-off-by: Todd Greer <tgreer@google.com>

Comment helper functions.

1ac7e7a

Signed-off-by: Todd Greer <tgreer@google.com>

Use absl::flat_hash_map instead of std::unordered_map.

3bb2056

Signed-off-by: Todd Greer <tgreer@google.com>

Fix formatting and spelling.

f490c86

Signed-off-by: Todd Greer <tgreer@google.com>

Add scheme checks to isCacheableRequest; misc cleanup.

bfdf8ee

Signed-off-by: Todd Greer <tgreer@google.com>

Add cache-control parsing TODO, and fix eatLeadingDuration's arg type.

1442b8c

Signed-off-by: Todd Greer <tgreer@google.com>

Fix SystemTime subtraction underflow, and improve tests.

ef53f4c

Signed-off-by: Todd Greer <tgreer@google.com>

Adapt to upstream factory changes

62d01c8

Signed-off-by: Todd Greer <tgreer@google.com>

repokitteh-read-only bot removed the waiting label Jan 24, 2020

Comment out rst docs about unenabled config fields, to avoid dangling…

c0223c3

… ref. Signed-off-by: Todd Greer <tgreer@google.com>

This was referenced Jan 25, 2020

CacheFilter: Fully parse and handle request and response cache-control #9833

Closed

CacheFilter: Support customizing whether to cache responses with no cache-control directives #9834

Open

CacheFilter: Handle backpressure from downstream #9835

Open

This was referenced Jan 28, 2020

CacheFilter: Parse and handle precondition request headers #9855

Open

CacheFilter: Refuse to cache requests with authorization headers #9857

Closed

mattklein123 requested changes Jan 28, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 28, 2020

This was referenced Jan 29, 2020

filter: Add header fields and constants for HTTP Caching Filter #868 #9865

Closed

filter: Add cache utility functions for HTTP Cache Filter #868 #9866

Merged

filter: Add HttpCache interface and helpers #9878

Merged

toddmgreer mentioned this pull request Feb 7, 2020

filter: Add CacheFilter config API #9960

Merged

toddmgreer mentioned this pull request Feb 12, 2020

filter: Add CacheFilter: a pluggable HTTP caching filter #10019

Merged

toddmgreer closed this Feb 21, 2020

enozcan mentioned this pull request Mar 26, 2020

filter: add HazelcastHttpCache #10536

Closed

yosrym93 added a commit to yosrym93/envoy that referenced this pull request Jun 15, 2020

CacheFilter bypasses cache for requests with authorization headers - e…

dcb3858

…nvoyproxy#7198 Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

yosrym93 mentioned this pull request Jun 24, 2020

CacheFilter: Upgrade GET requests whose upgrade headers are not removed should bypass the cache #11734

Closed

toddmgreer mentioned this pull request Aug 18, 2020

caching: Support out-of-thread (async) HttpCache implementations #12622

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

filter: Add an incomplete pluggable HTTP caching filter (CacheFilter). #7198

filter: Add an incomplete pluggable HTTP caching filter (CacheFilter). #7198

toddmgreer commented Jun 7, 2019

mattklein123 commented Jun 10, 2019

toddmgreer commented Jun 11, 2019

repokitteh-read-only bot commented Jun 11, 2019

stale bot commented Jun 18, 2019

jmarantz commented Jun 18, 2019

jmarantz left a comment

jmarantz left a comment

enozcan commented Jan 25, 2020

mattklein123 commented Jan 27, 2020

azure-pipelines bot commented Jan 27, 2020

toddmgreer commented Jan 27, 2020

toddmgreer commented Jan 28, 2020

repokitteh-read-only bot commented Jan 28, 2020

mattklein123 left a comment

mattklein123 Jan 28, 2020

mattklein123 Jan 28, 2020

mattklein123 Jan 28, 2020

mattklein123 Jan 28, 2020

mattklein123 Jan 28, 2020

mattklein123 Jan 28, 2020

mattklein123 Jan 28, 2020

enozcan commented Feb 7, 2020

toddmgreer commented Feb 7, 2020 via email

toddmgreer commented Feb 21, 2020

filter: Add an incomplete pluggable HTTP caching filter (CacheFilter). #7198

filter: Add an incomplete pluggable HTTP caching filter (CacheFilter). #7198

Conversation

toddmgreer commented Jun 7, 2019

mattklein123 commented Jun 10, 2019

toddmgreer commented Jun 11, 2019

repokitteh-read-only bot commented Jun 11, 2019

stale bot commented Jun 18, 2019

jmarantz commented Jun 18, 2019

jmarantz left a comment

Choose a reason for hiding this comment

jmarantz left a comment

Choose a reason for hiding this comment

enozcan commented Jan 25, 2020

mattklein123 commented Jan 27, 2020

azure-pipelines bot commented Jan 27, 2020

toddmgreer commented Jan 27, 2020

toddmgreer commented Jan 28, 2020

repokitteh-read-only bot commented Jan 28, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Jan 28, 2020

Choose a reason for hiding this comment

mattklein123 Jan 28, 2020

Choose a reason for hiding this comment

mattklein123 Jan 28, 2020

Choose a reason for hiding this comment

mattklein123 Jan 28, 2020

Choose a reason for hiding this comment

mattklein123 Jan 28, 2020

Choose a reason for hiding this comment

mattklein123 Jan 28, 2020

Choose a reason for hiding this comment

mattklein123 Jan 28, 2020

Choose a reason for hiding this comment

enozcan commented Feb 7, 2020

toddmgreer commented Feb 7, 2020 via email

toddmgreer commented Feb 21, 2020