Skip to content

caching: Fully parse and handle request and response cache-control headers#11727

Merged
htuch merged 39 commits intoenvoyproxy:masterfrom
yosrym93:cache-control
Jul 22, 2020
Merged

caching: Fully parse and handle request and response cache-control headers#11727
htuch merged 39 commits intoenvoyproxy:masterfrom
yosrym93:cache-control

Conversation

@yosrym93
Copy link
Copy Markdown
Contributor

@yosrym93 yosrym93 commented Jun 24, 2020 •
edited
Loading

Commit Message: CacheFilter fully parses and handles request and response cache-control headers

Additional Description:
Request and response cache-control headers are fully parsed in CacheHeadersUtils. isCacheableResponse in the CacheFilter now uses parsed cache-control headers to check if a received response can be cached. HttpCache now uses parsed cache-control headers to check if a cached response requires validation before it is served. Cache entries validation is not yet implemented.
Signed-off-by: Yosry Ahmed yosryahmed@google.com

Risk Level: Low
Testing: Unit testing
Docs Changes: N/A
Release Notes: N/A
Fixes #9833

capoferro
capoferro suggested changes Jun 24, 2020
View reviewed changes
@yosrym93
Implemented CacheHeadersParses to parse cache-control request and res…
e47de63 
…ponse headers completely

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
yosrym93 added 6 commits June 24, 2020 17:32
@yosrym93
Added more test cases for ResponseCacheControl and RequestCacheContro…
06df7bb 
…l parsers

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
HttpCache now correctly interprets request and response cache-control…
fc2b5c4 
… headers to find out if a cache entry requires revalidation - revalidation not yet implemented

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
Correctly interpret "must-revalidate", "proxy-revalidate" and
5d6a3d3 
"public" response cache-control directives

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
Implemented stream operators for RequestCacheControl and ResponseCach…
cef6eb8 
…eControl structs

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
Refactored cache_headers_utils_test for style-compliance and readability
620608d 
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
Renamed CacheFilterUtils to CacheabilityUtils to avoid ambiguity in s…
7fd7fda 
…imilar class names

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93 yosrym93 changed the title Implemented CacheHeadersParses to parse cache-control request and response headers completely caching: Fully parse and handle request and response cache-control headers Jun 25, 2020
@yosrym93
isCacheableResponse now interprets cache-control, expires and status …
57c7f36 
…headers correctly according to RFC7234

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
Updated SimpleHttpCache unit tests to reflect changes in LookupReques…
1d22836 
…t's constructor

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
toddmgreer
toddmgreer suggested changes Jun 26, 2020
View reviewed changes
yosrym93 added 5 commits June 26, 2020 03:03
@yosrym93
Removed Expires from header_map.h to avoid overhead before CacheFilte…
262b2ec 
…r is production-ready

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
Updated tests after removing Expires from inline headers
24e216a 
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
Improved and added comments, removed unused includes
a4436aa 
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
isCacheableResponse now only checks the response, the caller (CacheFi…
0881953 
…lter) checks for request no-store directives

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
Minor code readability enhancements and optimizations in CacheHeaders…
0c52e05 
…Utils and HttpCache

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
Copy link
Copy Markdown
Contributor Author

yosrym93 commented Jun 26, 2020

@toddmgreer I addressed all your comments except those related to CacheHeaderUtils, waiting for further explanation on the private members comment.
Looks like this class will be heavily refactored or removed.

capoferro
capoferro reviewed Jun 26, 2020
View reviewed changes
yosrym93 added 3 commits June 26, 2020 16:35
@yosrym93
Changed the cacheable_status_codes hashset to a static member
0ce342c 
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
Fixed a logical bug in HttpCache::requiresValidation and did minor re…
f3fc8aa 
…factors to improve readability & Fixed a bug that occurred in SimpleHttpCache tests because LookupRequest did not own the RequestCacheControl object

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
Added additional test cases for invalid cache-control headers for Cac…
5ce3d5c 
…heHeaderUtils

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
capoferro
capoferro previously approved these changes Jun 26, 2020
View reviewed changes
@yosrym93
Refactored CacheHeaderUtils for better style-compliance, readability …
2b428ca 
…and minimal optimizations

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
Copy link
Copy Markdown
Contributor Author

yosrym93 commented Jun 26, 2020

@toddmgreer all comments resolved.

@yosrym93
Copy link
Copy Markdown
Contributor Author

yosrym93 commented Jun 26, 2020

/azp run envoy-presubmit

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Jun 26, 2020

Commenter does not have sufficient privileges for PR 11727 in repo envoyproxy/envoy

@yosrym93
Fixed code style compliance problems
9ea81c1 
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
Copy link
Copy Markdown
Contributor Author

yosrym93 commented Jul 16, 2020

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Jul 16, 2020

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #11727 (comment) was created by @yosrym93.

see: more, trace.

jmarantz
jmarantz reviewed Jul 16, 2020
View reviewed changes
@yosrym93
Replaced unnecessary bitfields with booleans
11c8849 
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@junr03
Copy link
Copy Markdown
Member

junr03 commented Jul 17, 2020

@yosrym93 do you mind fixing format?

@yosrym93
Copy link
Copy Markdown
Contributor Author

yosrym93 commented Jul 17, 2020

@junr03 It was the legacy codec problem, I merged master, it should be fixed.

@junr03
Copy link
Copy Markdown
Member

junr03 commented Jul 17, 2020

Thanks, @htuch do you mind doing a pass on this (saw that @jmarantz mentioned you a couple times, so thought you could do the senior maintainer pass).

jmarantz
jmarantz previously approved these changes Jul 17, 2020
View reviewed changes
@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented Jul 18, 2020

Yes, in particular I want @htuch to look at the asserts.

htuch
htuch suggested changes Jul 20, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. I'm mostly reviewing this from an Envoy style and safety perspective with the assumption that @jmarantz has the RFC familiarity.
/wait

source/extensions/filters/http/cache/cache_headers_utils.cc
// Sun, 06 Nov 1994 08:49:37 GMT ; IMF-fixdate
// Sunday, 06-Nov-94 08:49:37 GMT ; obsolete RFC 850 format
// Sun Nov 6 08:49:37 1994 ; ANSI C's asctime() format
static const char* rfc7231_date_formats[] = {"%a, %d %b %Y %H:%M:%S GMT",
Copy link
Copy Markdown
Member

@htuch htuch Jul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is GMT an example time zone or compulsory in this datetime format?

Copy link
Copy Markdown
Contributor Author

@yosrym93 yosrym93 Jul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function was originally written by @toddmgreer in source/extensions/filters/http/cache/http_cache_utils.cc , I just moved it to this file. I think he has the answer to this.

Copy link
Copy Markdown
Contributor

@toddmgreer toddmgreer Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compulsory.
See https://httpwg.org/specs/rfc7231.html#http.date, particularly the ABNF grammar.

@yosrym93
Readability improvements and clarifications
6a3a6e1 
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93 yosrym93 requested a review from htuch July 21, 2020 19:28
htuch
htuch suggested changes Jul 21, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo the nit and also bumping up coverage a bit. See https://storage.googleapis.com/envoy-pr/11727/coverage/source/extensions/filters/http/cache/cache_headers_utils.cc.gcov.html (and more generally the cache tree in https://storage.googleapis.com/envoy-pr/11727/coverage/source/extensions/filters/http/cache/index.html). Even debug lines should be exercised somehow.
/wait

@yosrym93
Copy link
Copy Markdown
Contributor Author

yosrym93 commented Jul 22, 2020 •
edited
Loading

@htuch I am fixing the const local variables now. For the coverage, the whole cache tree has low coverage before this PR (still WIP), this PR actually raises coverage by about 4-5% IIRC. I intend to start working on a separate PR to improve coverage & tests generally for this soon.

@yosrym93
Declare all constant local variables with const
bbdd024 
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93 yosrym93 requested a review from htuch July 22, 2020 00:23
htuch
htuch approved these changes Jul 22, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! Looking forward to the coverage improvements.

@htuch htuch merged commit b510dd0 into envoyproxy:master Jul 22, 2020
@yosrym93 yosrym93 deleted the cache-control branch July 22, 2020 17:43
@yosrym93 yosrym93 mentioned this pull request Jul 27, 2020
Closed
KBaichoo pushed a commit to KBaichoo/envoy that referenced this pull request Jul 30, 2020
@yosrym93 @KBaichoo
caching: Fully parse and handle request and response cache-control he…
009a503 
…aders (envoyproxy#11727)

Request and response cache-control headers are fully parsed in CacheHeadersUtils. isCacheableResponse in the CacheFilter now uses parsed cache-control headers to check if a received response can be cached. HttpCache now uses parsed cache-control headers to check if a cached response requires validation before it is served. Cache entries validation is not yet implemented.
Signed-off-by: Yosry Ahmed yosryahmed@google.com

Risk Level: Low
Testing: Unit testing
Docs Changes: N/A
Release Notes: N/A

Fixes envoyproxy#9833

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmarantz jmarantz jmarantz left review comments

+4 more reviewers

@mpwarres mpwarres mpwarres left review comments

@toddmgreer toddmgreer toddmgreer requested changes

@htuch htuch htuch approved these changes

@capoferro capoferro capoferro left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@jmarantz jmarantz

@htuch htuch

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

7 participants

@yosrym93 @jmarantz @toddmgreer @junr03 @capoferro @mpwarres @htuch