caching: Correctly calculate cached responses age

source/extensions/filters/http/cache/BUILD

@@ -24,6 +24,8 @@ envoy_cc_library(
         "//source/common/common:enum_to_int",
         "//source/common/common:logger_lib",
         "//source/common/common:macros",
+        "//source/common/common:utility_lib",
+        "//source/common/http:header_map_lib",
         "//source/common/http:headers_lib",
         "//source/common/http:utility_lib",
         "//source/extensions/filters/http/common:pass_through_filter_lib",
@@ -75,8 +77,9 @@ envoy_cc_library(
     hdrs = ["cache_headers_utils.h"],
     external_deps = ["abseil_optional"],
     deps = [
+        ":inline_headers_handles",
         "//include/envoy/common:time_interface",
-        "//include/envoy/http:header_map_interface",
+        "//source/common/http:headers_lib",
     ],
 )
 

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

@@ -1,10 +1,13 @@
 #include "extensions/filters/http/cache/cache_filter.h"
 
+#include "envoy/http/header_map.h"
+
 #include "common/common/enum_to_int.h"
 #include "common/http/headers.h"
 #include "common/http/utility.h"
 
 #include "extensions/filters/http/cache/cacheability_utils.h"
+#include "extensions/filters/http/cache/http_cache.h"
 #include "extensions/filters/http/cache/inline_headers_handles.h"
 
 #include "absl/memory/memory.h"
@@ -94,10 +97,13 @@ Http::FilterHeadersStatus CacheFilter::encodeHeaders(Http::ResponseHeaderMap& he
   // Either a cache miss or a cache entry that is no longer valid.
   // Check if the new response can be cached.
   if (request_allows_inserts_ && CacheabilityUtils::isCacheableResponse(headers)) {
-    // TODO(#12140): Add date internal header or metadata to cached responses.
     ENVOY_STREAM_LOG(debug, "CacheFilter::encodeHeaders inserting headers", *encoder_callbacks_);
     insert_ = cache_.makeInsertContext(std::move(lookup_));
+    // Add the response time internal header to be copied into the cache
+    headers.setInline(response_time_handle.handle(), date_formatter_.now(time_source_));
     insert_->insertHeaders(headers, end_stream);
+    // Then remove the internal header from the response served to the client
+    headers.removeInline(response_time_handle.handle());
   }
   return Http::FilterHeadersStatus::Continue;
 }
@@ -357,10 +363,11 @@ void CacheFilter::processSuccessfulValidation(Http::ResponseHeaderMap& response_
   response_headers.setStatus(lookup_result_->headers_->getStatusValue());
   response_headers.setContentLength(lookup_result_->headers_->getContentLengthValue());
 
-  // A cache entry was successfully validated -> encode cached body and trailers.
-  // encodeCachedResponse also adds the age header to lookup_result_
-  // so it should be called before headers are merged.
-  encodeCachedResponse();
+  // A response that has been validated should not contain an Age header as it is equivalent to a
+  // freshly served response from the origin, unless the 304 response has an Age header, which
+  // means it was served by an upstream cache.
+  // Remove any existing Age header in the cached response.
+  lookup_result_->headers_->removeInline(age_handle.handle());
 
   // Add any missing headers from the cached response to the 304 response.
   lookup_result_->headers_->iterate([&response_headers](const Http::HeaderEntry& cached_header) {
@@ -375,8 +382,15 @@ void CacheFilter::processSuccessfulValidation(Http::ResponseHeaderMap& response_
 
   if (should_update_cached_entry) {
     // TODO(yosrym93): else the cached entry should be deleted.
+    // Add the response time internal header to be copied into the cache
+    response_headers.setInline(response_time_handle.handle(), date_formatter_.now(time_source_));
     cache_.updateHeaders(*lookup_, response_headers);
+    // Then remove the internal header from the response served to the client
+    response_headers.removeInline(response_time_handle.handle());
   }
+
+  // A cache entry was successfully validated -> encode cached body and trailers.
+  encodeCachedResponse();
 }
 
 // TODO(yosrym93): Write a test that exercises this when SimpleHttpCache implements updateHeaders
@@ -414,7 +428,7 @@ void CacheFilter::injectValidationHeaders(Http::RequestHeaderMap& request_header
     absl::string_view etag = etag_header->value().getStringView();
     request_headers.setInline(if_none_match_handle.handle(), etag);
   }
-  if (CacheHeadersUtils::httpTime(last_modified_header) != SystemTime()) {
+  if (DateUtil::timePointValid(CacheHeadersUtils::httpTime(last_modified_header))) {
     // Valid Last-Modified header exists.
     absl::string_view last_modified = last_modified_header->value().getStringView();
     request_headers.setInline(if_modified_since_handle.handle(), last_modified);
@@ -433,8 +447,6 @@ void CacheFilter::encodeCachedResponse() {
 
   response_has_trailers_ = lookup_result_->has_trailers_;
   const bool end_stream = (lookup_result_->content_length_ == 0 && !response_has_trailers_);
-  // TODO(toddmgreer): Calculate age per https://httpwg.org/specs/rfc7234.html#age.calculations
-  lookup_result_->headers_->addReferenceKey(Http::Headers::get().Age, 0);
 
   // Set appropriate response flags and codes.
   Http::StreamFilterCallbacks* callbacks =

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

@@ -8,6 +8,7 @@
 #include "envoy/extensions/filters/http/cache/v3alpha/cache.pb.h"
 
 #include "common/common/logger.h"
+#include "common/common/utility.h"
 
 #include "extensions/filters/http/cache/cache_headers_utils.h"
 #include "extensions/filters/http/cache/http_cache.h"
@@ -81,6 +82,10 @@ class CacheFilter : public Http::PassThroughFilter,
   InsertContextPtr insert_;
   LookupResultPtr lookup_result_;
 
+  // Format: IMF-fixdate. As per:
+  // https://tools.ietf.org/html/rfc7231#section-7.1.1.1
+  DateFormatter date_formatter_{"%a, %d %b %Y %H:%M:%S GMT"};
+
   // Tracks what body bytes still need to be read from the cache. This is
   // currently only one Range, but will expand when full range support is added. Initialized by
   // onHeaders for Range Responses, otherwise initialized by encodeCachedResponse.

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

@@ -1,9 +1,12 @@
 #include "extensions/filters/http/cache/cache_headers_utils.h"
 
 #include <array>
+#include <chrono>
 #include <string>
 
-#include "envoy/common/time.h"
+#include "envoy/http/header_map.h"
+
+#include "extensions/filters/http/cache/inline_headers_handles.h"
 
 #include "absl/algorithm/container.h"
 #include "absl/strings/ascii.h"
@@ -143,6 +146,32 @@ SystemTime CacheHeadersUtils::httpTime(const Http::HeaderEntry* header_entry) {
   return {};
 }
 
+std::chrono::seconds
+CacheHeadersUtils::calculateAge(const Http::ResponseHeaderMap& response_headers,
+                                const SystemTime response_time, const SystemTime now) {
+  // Age headers calculations follow: https://httpwg.org/specs/rfc7234.html#age.calculations
+  const SystemTime date_value = CacheHeadersUtils::httpTime(response_headers.Date());
+
+  long age_value;
+  const absl::string_view age_header = response_headers.getInlineValue(age_handle.handle());
+  if (!absl::SimpleAtoi(age_header, &age_value)) {
+    age_value = 0;
+  }
+
+  const SystemTime::duration apparent_age =
+      std::max(SystemTime::duration(0), response_time - date_value);
+
+  // Assumption: response_delay is negligible -> correct_age_value = age_value.
+  const SystemTime::duration corrected_age_value = std::chrono::seconds(age_value);
+  const SystemTime::duration corrected_initial_age = std::max(apparent_age, corrected_age_value);
+
+  // Calculate current_age:
+  const SystemTime::duration resident_time = now - response_time;
+  const SystemTime::duration current_age = corrected_initial_age + resident_time;
+
+  return std::chrono::duration_cast<std::chrono::seconds>(current_age);
+}
+
 absl::optional<uint64_t> CacheHeadersUtils::readAndRemoveLeadingDigits(absl::string_view& str) {
   uint64_t val = 0;
   uint32_t bytes_consumed = 0;

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

@@ -1,7 +1,8 @@
 #pragma once
 
 #include "envoy/common/time.h"
-#include "envoy/http/header_map.h"
+
+#include "common/http/headers.h"
 
 #include "absl/strings/string_view.h"
 #include "absl/time/time.h"
@@ -91,6 +92,10 @@ class CacheHeadersUtils {
   // header_entry is null or malformed.
   static SystemTime httpTime(const Http::HeaderEntry* header_entry);
 
+  // Calculates the age of a cached response
+  static std::chrono::seconds calculateAge(const Http::ResponseHeaderMap& response_headers,
+                                           const SystemTime response_time, const SystemTime now);
+
   /**
    * Read a leading positive decimal integer value and advance "*str" past the
    * digits read. If overflow occurs, or no digits exist, return

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

@@ -60,14 +60,15 @@ bool CacheabilityUtils::isCacheableResponse(const Http::ResponseHeaderMap& heade
   absl::string_view cache_control = headers.getInlineValue(response_cache_control_handle.handle());
   ResponseCacheControl response_cache_control(cache_control);
 
-  // Only cache responses with explicit validation data, either:
-  //    "no-cache" cache-control directive
-  //    "max-age" or "s-maxage" cache-control directives with date header
-  //    expires header
-  const bool has_validation_data =
-      response_cache_control.must_validate_ ||
-      (headers.Date() && response_cache_control.max_age_.has_value()) ||
-      headers.get(Http::Headers::get().Expires);
+  // Only cache responses with enough data to calculate freshness lifetime as per:
+  // https://httpwg.org/specs/rfc7234.html#calculating.freshness.lifetime.
+  // Either:
+  //    "no-cache" cache-control directive (requires revalidation anyway).
+  //    "max-age" or "s-maxage" cache-control directives.
+  //    Both "Expires" and "Date" headers.
+  const bool has_validation_data = response_cache_control.must_validate_ ||
+                                   response_cache_control.max_age_.has_value() ||
+                                   (headers.Date() && headers.getInline(expires_handle.handle()));
 
   return !response_cache_control.no_store_ &&
          cacheableStatusCodes().contains((headers.getStatusValue())) && has_validation_data;

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

@@ -11,6 +11,7 @@
 #include "common/http/headers.h"
 #include "common/protobuf/utility.h"
 
+#include "extensions/filters/http/cache/cache_headers_utils.h"
 #include "extensions/filters/http/cache/inline_headers_handles.h"
 
 #include "absl/strings/str_split.h"
@@ -74,21 +75,14 @@ void LookupRequest::initializeRequestCacheControl(const Http::RequestHeaderMap&
   }
 }
 
-bool LookupRequest::requiresValidation(const Http::ResponseHeaderMap& response_headers) const {
+bool LookupRequest::requiresValidation(const Http::ResponseHeaderMap& response_headers,
+                                       SystemTime::duration response_age) const {
   // TODO(yosrym93): Store parsed response cache-control in cache instead of parsing it on every
   // lookup.
   const absl::string_view cache_control =
       response_headers.getInlineValue(response_cache_control_handle.handle());
   const ResponseCacheControl response_cache_control(cache_control);
 
-  const SystemTime response_time = CacheHeadersUtils::httpTime(response_headers.Date());
-
-  if (timestamp_ < response_time) {
-    // Response time is in the future, validate response.
-    return true;
-  }
-
-  const SystemTime::duration response_age = timestamp_ - response_time;
   const bool request_max_age_exceeded = request_cache_control_.max_age_.has_value() &&
                                         request_cache_control_.max_age_.value() < response_age;
   if (response_cache_control.must_validate_ || request_cache_control_.must_validate_ ||
@@ -99,30 +93,33 @@ bool LookupRequest::requiresValidation(const Http::ResponseHeaderMap& response_h
   }
 
   // CacheabilityUtils::isCacheableResponse(..) guarantees that any cached response satisfies this.
-  // When date metadata injection for responses with no date
-  // is implemented, this ASSERT will need to be updated.
-  ASSERT((response_headers.Date() && response_cache_control.max_age_.has_value()) ||
-             response_headers.get(Http::Headers::get().Expires),
+  ASSERT(response_cache_control.max_age_.has_value() ||
+             (response_headers.getInline(expires_handle.handle()) && response_headers.Date()),
          "Cache entry does not have valid expiration data.");
 
-  const SystemTime expiration_time =
-      response_cache_control.max_age_.has_value()
-          ? response_time + response_cache_control.max_age_.value()
-          : CacheHeadersUtils::httpTime(response_headers.get(Http::Headers::get().Expires));
+  SystemTime::duration freshness_lifetime;
+  if (response_cache_control.max_age_.has_value()) {
+    freshness_lifetime = response_cache_control.max_age_.value();
+  } else {
+    const SystemTime expires_value =
+        CacheHeadersUtils::httpTime(response_headers.getInline(expires_handle.handle()));
+    const SystemTime date_value = CacheHeadersUtils::httpTime(response_headers.Date());
+    freshness_lifetime = expires_value - date_value;
+  }
 
-  if (timestamp_ > expiration_time) {
+  if (response_age > freshness_lifetime) {
     // Response is stale, requires validation if
     // the response does not allow being served stale,
     // or the request max-stale directive does not allow it.
     const bool allowed_by_max_stale =
         request_cache_control_.max_stale_.has_value() &&
-        request_cache_control_.max_stale_.value() > timestamp_ - expiration_time;
+        request_cache_control_.max_stale_.value() > response_age - freshness_lifetime;
     return response_cache_control.no_stale_ || !allowed_by_max_stale;
   } else {
     // Response is fresh, requires validation only if there is an unsatisfied min-fresh requirement.
     const bool min_fresh_unsatisfied =
         request_cache_control_.min_fresh_.has_value() &&
-        request_cache_control_.min_fresh_.value() > expiration_time - timestamp_;
+        request_cache_control_.min_fresh_.value() > freshness_lifetime - response_age;
     return min_fresh_unsatisfied;
   }
 }
@@ -132,7 +129,23 @@ LookupResult LookupRequest::makeLookupResult(Http::ResponseHeaderMapPtr&& respon
   // TODO(toddmgreer): Implement all HTTP caching semantics.
   ASSERT(response_headers);
   LookupResult result;
-  result.cache_entry_status_ = requiresValidation(*response_headers)
+
+  // If the response_time internal header does not exist, set it to the Date header value.
+  const Http::HeaderEntry* response_time_header =
+      response_headers->getInline(response_time_handle.handle());
+  const SystemTime response_time = response_time_header
+                                       ? CacheHeadersUtils::httpTime(response_time_header)
+                                       : CacheHeadersUtils::httpTime(response_headers->Date());
+
+  // Assumption: Cache lookup time is negligible. Therefore, now == timestamp_
+  const std::chrono::seconds age =
+      CacheHeadersUtils::calculateAge(*response_headers, response_time, timestamp_);
+
+  // Add the Age header and remove the response_time header before serving the cached response.
+  response_headers->setInline(age_handle.handle(), std::to_string(age.count()));
+  response_headers->removeInline(response_time_handle.handle());
+
+  result.cache_entry_status_ = requiresValidation(*response_headers, age)
                                    ? CacheEntryStatus::RequiresValidation
                                    : CacheEntryStatus::Ok;
   result.headers_ = std::move(response_headers);

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

@@ -200,7 +200,8 @@ class LookupRequest {
 
 private:
   void initializeRequestCacheControl(const Http::RequestHeaderMap& request_headers);
-  bool requiresValidation(const Http::ResponseHeaderMap& response_headers) const;
+  bool requiresValidation(const Http::ResponseHeaderMap& response_headers,
+                          SystemTime::duration age) const;
 
   Key key_;
   std::vector<RawByteRange> request_range_spec_;

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

@@ -1,12 +1,21 @@
 #pragma once
 
+#include "envoy/http/header_map.h"
+
 #include "common/http/headers.h"
 
 namespace Envoy {
 namespace Extensions {
 namespace HttpFilters {
 namespace Cache {
 
+// An internal header that represents the time at which a cached response was received by the cache.
+// This header is added to cached responses and removed before they are served.
+// This represents "response_time" in the age header calculations at:
+// https://httpwg.org/specs/rfc7234.html#age.calculations
+inline constexpr absl::string_view ResponseTimeHeader = "Envoy-Cache-Response-Time";
+
+// Request headers inline handles
 inline Http::RegisterCustomInlineHeader<Http::CustomInlineHeaderRegistry::Type::RequestHeaders>
     authorization_handle(Http::CustomHeaders::get().Authorization);
 
@@ -41,6 +50,15 @@ inline Http::RegisterCustomInlineHeader<Http::CustomInlineHeaderRegistry::Type::
 inline Http::RegisterCustomInlineHeader<Http::CustomInlineHeaderRegistry::Type::ResponseHeaders>
     etag_handle(Http::CustomHeaders::get().Etag);
 
+inline Http::RegisterCustomInlineHeader<Http::CustomInlineHeaderRegistry::Type::ResponseHeaders>
+    age_handle(Http::Headers::get().Age);
+
+inline Http::RegisterCustomInlineHeader<Http::CustomInlineHeaderRegistry::Type::ResponseHeaders>
+    expires_handle(Http::Headers::get().Expires);
+
+inline Http::RegisterCustomInlineHeader<Http::CustomInlineHeaderRegistry::Type::ResponseHeaders>
+    response_time_handle(Http::LowerCaseString{std::string(ResponseTimeHeader)});
+
 } // namespace Cache
 } // namespace HttpFilters
 } // namespace Extensions

test/extensions/filters/http/cache/BUILD

@@ -27,6 +27,7 @@ envoy_extension_cc_test(
         "//include/envoy/http:header_map_interface",
         "//source/common/http:header_map_lib",
         "//source/extensions/filters/http/cache:cache_headers_utils_lib",
+        "//test/test_common:simulated_time_system_lib",
         "//test/test_common:utility_lib",
     ],
 )

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

@@ -88,7 +88,7 @@ TEST_P(CacheIntegrationTest, MissInsertHit) {
     EXPECT_TRUE(response_decoder->complete());
     EXPECT_THAT(response_decoder->headers(), IsSupersetOfHeaders(response_headers));
     EXPECT_EQ(response_decoder->body(), response_body);
-    EXPECT_NE(response_decoder->headers().get(Http::Headers::get().Age), nullptr);
+    EXPECT_THAT(response_decoder->headers(), HeaderHasValueRef(Http::Headers::get().Age, "10"));
     // Advance time to force a log flush.
     simTime().advanceTimeWait(std::chrono::seconds(1));
     EXPECT_THAT(waitForAccessLog(access_log_name_, 1),
@@ -164,8 +164,11 @@ TEST_P(CacheIntegrationTest, ExpiredValidated) {
     EXPECT_TRUE(response_decoder->complete());
     EXPECT_THAT(response_decoder->headers(), IsSupersetOfHeaders(response_headers));
     EXPECT_EQ(response_decoder->body(), response_body);
-    // Check that age header exists as this is a cached response
-    EXPECT_NE(response_decoder->headers().get(Http::Headers::get().Age), nullptr);
+
+    // A response that has been validated should not contain an Age header as it is equivalent to a
+    // freshly served response from the origin, unless the 304 response has an Age header, which
+    // means it was served by an upstream cache.
+    EXPECT_EQ(response_decoder->headers().get(Http::Headers::get().Age), nullptr);
 
     // Advance time to force a log flush.
     simTime().advanceTimeWait(std::chrono::seconds(1));