filter: Add CacheFilter: a pluggable HTTP caching filter by toddmgreer · Pull Request #10019 · envoyproxy/envoy

toddmgreer · 2020-02-12T08:38:52Z

Description: Add CacheFilter: a pluggable HTTP caching filter. Split from #7198.
Risk Level: Low: nobody uses this new filter
Testing: Unit tests. Integration tests to follow in next PR.
Docs Changes: none
Release Notes: Do we add release notes for WIP filters?
#868

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

toddmgreer · 2020-02-12T20:33:52Z

/retest

repokitteh-read-only · 2020-02-12T20:33:56Z

🤷‍♀️ nothing to rebuild.

🐱

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

see: more, trace.

mattklein123

Cool stuff. Flushing some high level comments that we should resolve first. Thank you!

/wait

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

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

toddmgreer · 2020-02-14T20:55:22Z

@mattklein123 We're almost through the code in #7198. After this PR, I have two more small ones prepped and ready to go: one for CacheFilterFactory (+ unit tests) and one for CacheIntegrationTest. I should be able to open each PR as soon as the prior one is merged.

Will you be able to continue reviewing into next week? I'm hoping to get this series fully merged as early next week as possible.

Thank you for all the reviews!

mattklein123

One high level question and needs a master merge and then I will take another pass. Thank you!

/wait

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

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

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

mattklein123

Cool stuff. A few more high level questions. Thank you!

/wait

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

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

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

…all to OnBodyAsync. Signed-off-by: Todd Greer <tgreer@google.com>

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

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

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

…do and don't respond immediately to getHeaders. Signed-off-by: Todd Greer <tgreer@google.com>

mattklein123

Thanks, LGTM other than the stop iteration issue I highlighted. I would recommend adding an integration test that will help us test various real-world permutations. I don't think it will be that much work. Thank you! Very cool stuff and excited to see this land so we can iterate on it.

/wait

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

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

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

toddmgreer · 2020-02-21T01:04:25Z

/retest

repokitteh-read-only · 2020-02-21T01:04:29Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

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

see: more, trace.

mattklein123

LGTM other than a test request. Also please merge master to fix the coverage test flake. Thank you!

/wait

mattklein123 · 2020-02-21T01:07:25Z

test/extensions/filters/http/cache/cache_filter_integration_test.cc

+
+  // Send first request, and get response from upstream.
+  {
+    IntegrationStreamDecoderPtr request = codec_client_->makeHeaderOnlyRequest(request_headers);


Can you add some integration tests with body, trailers, etc.? This would have caught the stop iteration issue I found.

I changed CacheFilter to refuse to cache requests with bodies and/or trailers, because caching GET requests with bodies would turn an ordinary request smuggling vector into cache poisoning, and because GET/HEAD requests with bodies have no meaning. If it turns out to be needed, we may want to add a config knob to allow caching them.

Added unit and integration tests for them.

…est smuggling, and the spec doesn't define any meaning for GET/HEAD requests with bodies. Signed-off-by: Todd Greer <tgreer@google.com>

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

mattklein123

Thanks, nice work. Let's ship and iterate!

toddmgreer · 2020-02-21T19:59:04Z

Woohoo! Thank you for all of your reviews!

SaveTheRbtz · 2020-03-01T01:55:09Z

JFYI: @toddmgreer I saw some flakiness on cache_filter_integration_test:

$ bazel test --runs_per_test 1000 //test/extensions/filters/http/cache:cache_filter_integration_test
...
[ RUN      ] Protocols/CacheIntegrationTest.GetRequestWithBodyAndTrailers/IPv6_HttpDownstream_HttpUpstream
[2020-02-29 23:18:30.012][21766583][critical][backtrace] [bazel-out/darwin-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:83] Caught Segmentation fault: 11, suspect faulting address 0x0
[2020-02-29 23:18:30.012][21766583][critical][backtrace] [bazel-out/darwin-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:70] Backtrace (use tools/stack_decode.py to get line numbers):
[2020-02-29 23:18:30.012][21766583][critical][backtrace] [bazel-out/darwin-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:71] Envoy version: 0/1.14.0-dev/redacted/DEBUG/BoringSSL

(sadly it doesn't print traceback on macos (even with -fno-omit-frame-pointer))

toddmgreer · 2020-03-05T23:13:08Z

@SaveTheRbtz I'm not getting any failures testing on my Linux workstation, either at 1k runs_per_test, or at 10k. I'd like to figure this out, but I need more to go on, such as a stack trace. Also, what revision were you at? If you can the crash to tell you a bit more, please file an issue.
Thanks!

Add CacheFilter: an HTTP caching filter based on the HttpCache plugin…

361326d

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

toddmgreer requested a review from jmarantz as a code owner February 12, 2020 08:38

mattklein123 self-assigned this Feb 12, 2020

yanavlasov self-assigned this Feb 12, 2020

mattklein123 requested changes Feb 13, 2020

View reviewed changes

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

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

repokitteh-read-only bot added the waiting label Feb 13, 2020

toddmgreer added 2 commits February 13, 2020 17:44

Use OnDestroy to manage async events

566e086

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

Find HttpCache implementation in factory

e57d95e

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

repokitteh-read-only bot removed the waiting label Feb 14, 2020

format fix

ecc59d9

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

mattklein123 requested changes Feb 19, 2020

View reviewed changes

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

source/extensions/filters/http/cache/http_cache.h Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Feb 19, 2020

toddmgreer added 2 commits February 18, 2020 17:15

Merge branch 'master' of github.com:envoyproxy/envoy into CacheFilter2

3434039

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

Replace HeaderMap with more specific subtype.

ce5dfad

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

repokitteh-read-only bot removed the waiting label Feb 19, 2020

mattklein123 requested changes Feb 19, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 19, 2020

toddmgreer added 3 commits February 19, 2020 13:34

Remove unused using.

0a63174

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

Fix race on decoder_callbacks_, clarify comments, and fix getBody's c…

c9423af

…all to OnBodyAsync. Signed-off-by: Todd Greer <tgreer@google.com>

Remove support for out of thread HttpCache implementations.

01543bc

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

repokitteh-read-only bot removed the waiting label Feb 19, 2020

toddmgreer added 2 commits February 19, 2020 14:48

Remove vestigial 'active' method.

18b348b

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

Delete insert_ and lookup_ in onDestroy.

a2bb906

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

mattklein123 reviewed Feb 20, 2020

View reviewed changes

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

repokitteh-read-only bot added the waiting:any label Feb 20, 2020

Remove vestigial onDestroy

d866b59

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

repokitteh-read-only bot removed the waiting:any label Feb 20, 2020

toddmgreer added 2 commits February 19, 2020 18:09

Fix error of calling continueDecoding before stopping iteration.

09c38cc

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

Test that CacheFilter can handle HttpCache implementations that both …

0112fd2

…do and don't respond immediately to getHeaders. Signed-off-by: Todd Greer <tgreer@google.com>

mattklein123 requested changes Feb 20, 2020

View reviewed changes

source/extensions/filters/http/cache/cache_filter.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Feb 20, 2020

Use StopIterationAndWatermark instead of StopIteration, per review co…

ee99204

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

repokitteh-read-only bot removed the waiting label Feb 20, 2020

Add factory and integration test.

1b9a17f

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

mattklein123 requested changes Feb 21, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 21, 2020

toddmgreer added 2 commits February 20, 2020 19:17

Refuse to cache requests with bodies, because they're used for a requ…

590a34a

…est smuggling, and the spec doesn't define any meaning for GET/HEAD requests with bodies. Signed-off-by: Todd Greer <tgreer@google.com>

Merge branch 'master' of github.com:envoyproxy/envoy into CacheFilter2

dc82ef9

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

repokitteh-read-only bot removed the waiting label Feb 21, 2020

Restore NiceMocks

b9ad06b

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

mattklein123 approved these changes Feb 21, 2020

View reviewed changes

mattklein123 merged commit f6b00e9 into envoyproxy:master Feb 21, 2020

toddmgreer deleted the CacheFilter2 branch February 21, 2020 19:59

toddmgreer mentioned this pull request Feb 21, 2020

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

Closed

enozcan mentioned this pull request Jun 5, 2020

filter: add HazelcastHttpCache #10536

Closed

Conversation

toddmgreer commented Feb 12, 2020

Uh oh!

toddmgreer commented Feb 12, 2020

Uh oh!

repokitteh-read-only bot commented Feb 12, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

toddmgreer commented Feb 14, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

toddmgreer commented Feb 21, 2020

Uh oh!

repokitteh-read-only bot commented Feb 21, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Feb 21, 2020

Choose a reason for hiding this comment

Uh oh!

toddmgreer Feb 21, 2020

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

toddmgreer commented Feb 21, 2020

Uh oh!

SaveTheRbtz commented Mar 1, 2020

Uh oh!

toddmgreer commented Mar 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants