Split VaryHeader into VaryAllowList and VaryUtils to organize vary-related logic

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

@@ -89,7 +89,7 @@ class CacheFilter : public Http::PassThroughFilter,
   // of doing it per-request. A good example of such config is found in the gzip filter:
   // source/extensions/filters/http/gzip/gzip_filter.h.
   // Stores the allow list rules that decide if a header can be varied upon.
-  VaryHeader vary_allow_list_;
+  VaryAllowList vary_allow_list_;
 
   // True if the response has trailers.
   // TODO(toddmgreer): cache trailers.

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

@@ -228,7 +228,7 @@ CacheHeadersUtils::parseCommaDelimitedHeader(const Http::HeaderMap::GetResult& e
   return values;
 }
 
-VaryHeader::VaryHeader(
+VaryAllowList::VaryAllowList(
     const Protobuf::RepeatedPtrField<envoy::type::matcher::v3::StringMatcher>& allow_list) {
 
   for (const auto& rule : allow_list) {
@@ -238,17 +238,17 @@ VaryHeader::VaryHeader(
   }
 }
 
-bool VaryHeader::allowsHeader(const absl::string_view header) const {
+bool VaryAllowList::allowsValue(const absl::string_view vary_value) const {
   for (const auto& rule : allow_list_) {
-    if (rule->match(header)) {
+    if (rule->match(vary_value)) {
       return true;
     }
   }
   return false;
 }
 
-bool VaryHeader::isAllowed(const Http::ResponseHeaderMap& headers) const {
-  if (!VaryHeader::hasVary(headers)) {
+bool VaryAllowList::allowsHeaders(const Http::ResponseHeaderMap& headers) const {
+  if (!VaryHeaderUtils::hasVary(headers)) {
     return true;
   }
 
@@ -264,7 +264,7 @@ bool VaryHeader::isAllowed(const Http::ResponseHeaderMap& headers) const {
       return false;
     }
 
-    if (allowsHeader(header)) {
+    if (allowsValue(header)) {
       valid = true;
     }
 
@@ -276,14 +276,14 @@ bool VaryHeader::isAllowed(const Http::ResponseHeaderMap& headers) const {
   return true;
 }
 
-bool VaryHeader::hasVary(const Http::ResponseHeaderMap& headers) {
+bool VaryHeaderUtils::hasVary(const Http::ResponseHeaderMap& headers) {
   // TODO(mattklein123): Support multiple vary headers and/or just make the vary header inline.
   const auto vary_header = headers.get(Http::CustomHeaders::get().Vary);
   return !vary_header.empty() && !vary_header[0]->value().empty();
 }
 
 absl::btree_set<absl::string_view>
-VaryHeader::getVaryValues(const Http::ResponseHeaderMap& headers) {
+VaryHeaderUtils::getVaryValues(const Http::ResponseHeaderMap& headers) {
   Http::HeaderMap::GetResult vary_headers = headers.get(Http::CustomHeaders::get().Vary);
   if (vary_headers.empty()) {
     return {};
@@ -306,18 +306,20 @@ constexpr absl::string_view inValueSeparator = "\r";
 }; // namespace
 
 absl::optional<std::string>
-VaryHeader::createVaryIdentifier(const absl::btree_set<absl::string_view>& vary_header_values,
-                                 const Http::RequestHeaderMap& request_headers) const {
+VaryHeaderUtils::createVaryIdentifier(const VaryAllowList& allow_list,
+                                      const absl::btree_set<absl::string_view>& vary_header_values,
+                                      const Http::RequestHeaderMap& request_headers) {
   std::string vary_identifier = "vary-id\n";
   if (vary_header_values.empty()) {
     return vary_identifier;
   }
 
-  for (const absl::string_view& header : vary_header_values) {
-    if (header.empty()) {
+  for (const absl::string_view& value : vary_header_values) {
+    if (value.empty()) {
+      // Empty headers are ignored.
       continue;
     }
-    if (!allowsHeader(header)) {
+    if (!allow_list.allowsValue(value)) {
       // The backend tried to vary on a header that we don't allow, so return
       // absl::nullopt to indicate we are unable to cache this request. This
       // also may occur if the allow list has changed since an item was cached,
@@ -334,8 +336,8 @@ VaryHeader::createVaryIdentifier(const absl::btree_set<absl::string_view>& vary_
     // be used as an inspiration for some bucketing configuration. The config
     // should enable and control the bucketing wanted.
     const auto all_values = Http::HeaderUtility::getAllOfHeaderAsString(
-        request_headers, Http::LowerCaseString(std::string(header)), inValueSeparator);
-    absl::StrAppend(&vary_identifier, header, inValueSeparator,
+        request_headers, Http::LowerCaseString(std::string(value)), inValueSeparator);
+    absl::StrAppend(&vary_identifier, value, inValueSeparator,
                     all_values.result().has_value() ? all_values.result().value() : "",
                     headerSeparator);
   }

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

@@ -122,38 +122,41 @@ class CacheHeadersUtils {
   parseCommaDelimitedHeader(const Http::HeaderMap::GetResult& entry);
 };
 
-class VaryHeader {
+class VaryAllowList {
 public:
-  // Checks if the headers contain a non-empty value in the Vary header.
-  static bool hasVary(const Http::ResponseHeaderMap& headers);
-
-  // Retrieve all the individual header values from the provided response header
-  // map across all vary header entries.
-  static absl::btree_set<absl::string_view>
-  getVaryValues(const Envoy::Http::ResponseHeaderMap& headers);
-
-  // Checks if this header is allowed to vary cache entries.
-  bool allowsHeader(const absl::string_view header) const;
-
-  // Creates a single string combining the values of the varied headers from
-  // entry_headers. Returns an absl::nullopt if no valid vary key can be created
-  // and the response should not be cached (eg. when disallowed vary headers are
-  // present in the response).
-  absl::optional<std::string>
-  createVaryIdentifier(const absl::btree_set<absl::string_view>& vary_header_values,
-                       const Envoy::Http::RequestHeaderMap& request_headers) const;
-
   // Parses the allow list from the Cache Config into the object's private allow_list_.
-  VaryHeader(const Protobuf::RepeatedPtrField<envoy::type::matcher::v3::StringMatcher>& allow_list);
+  VaryAllowList(
+      const Protobuf::RepeatedPtrField<envoy::type::matcher::v3::StringMatcher>& allow_list);
 
   // Checks if the headers contain an allowed value in the Vary header.
-  bool isAllowed(const Http::ResponseHeaderMap& headers) const;
+  bool allowsHeaders(const Http::ResponseHeaderMap& headers) const;
+
+  // Checks if this vary header value is allowed to vary cache entries.
+  bool allowsValue(const absl::string_view header) const;
 
 private:
   // Stores the matching rules that define whether a header is allowed to be varied.
   std::vector<Matchers::StringMatcherPtr> allow_list_;
 };
 
+namespace VaryHeaderUtils {
+// Checks if the headers contain a non-empty value in the Vary header.
+bool hasVary(const Http::ResponseHeaderMap& headers);
+
+// Retrieve all the individual header values from the provided response header
+// map across all vary header entries.
+absl::btree_set<absl::string_view> getVaryValues(const Envoy::Http::ResponseHeaderMap& headers);
+
+// Creates a single string combining the values of the varied headers from
+// entry_headers. Returns an absl::nullopt if no valid vary key can be created
+// and the response should not be cached (eg. when disallowed vary headers are
+// present in the response).
+absl::optional<std::string>
+createVaryIdentifier(const VaryAllowList& allow_list,
+                     const absl::btree_set<absl::string_view>& vary_header_values,
+                     const Envoy::Http::RequestHeaderMap& request_headers);
+} // namespace VaryHeaderUtils
+
 } // namespace Cache
 } // namespace HttpFilters
 } // namespace Extensions

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

@@ -57,7 +57,7 @@ bool CacheabilityUtils::canServeRequestFromCache(const Http::RequestHeaderMap& h
 }
 
 bool CacheabilityUtils::isCacheableResponse(const Http::ResponseHeaderMap& headers,
-                                            const VaryHeader& vary_allow_list) {
+                                            const VaryAllowList& vary_allow_list) {
   absl::string_view cache_control =
       headers.getInlineValue(CacheCustomHeaders::responseCacheControl());
   ResponseCacheControl response_cache_control(cache_control);
@@ -74,7 +74,7 @@ bool CacheabilityUtils::isCacheableResponse(const Http::ResponseHeaderMap& heade
 
   return !response_cache_control.no_store_ &&
          cacheableStatusCodes().contains((headers.getStatusValue())) && has_validation_data &&
-         vary_allow_list.isAllowed(headers);
+         vary_allow_list.allowsHeaders(headers);
 }
 
 } // namespace Cache

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

@@ -24,7 +24,7 @@ class CacheabilityUtils {
   // https://httpwg.org/specs/rfc7234.html#response.cacheability. Head requests are not
   // cacheable. However, this function is never called for head requests.
   static bool isCacheableResponse(const Http::ResponseHeaderMap& headers,
-                                  const VaryHeader& vary_allow_list);
+                                  const VaryAllowList& vary_allow_list);
 };
 } // namespace Cache
 } // namespace HttpFilters

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

@@ -24,7 +24,7 @@ namespace HttpFilters {
 namespace Cache {
 
 LookupRequest::LookupRequest(const Http::RequestHeaderMap& request_headers, SystemTime timestamp,
-                             const VaryHeader& vary_allow_list)
+                             const VaryAllowList& vary_allow_list)
     : request_headers_(Http::createHeaderMap<Http::RequestHeaderMapImpl>(request_headers)),
       vary_allow_list_(vary_allow_list), timestamp_(timestamp) {
   // These ASSERTs check prerequisites. A request without these headers can't be looked up in cache;

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

@@ -186,7 +186,7 @@ class LookupRequest {
 public:
   // Prereq: request_headers's Path(), Scheme(), and Host() are non-null.
   LookupRequest(const Http::RequestHeaderMap& request_headers, SystemTime timestamp,
-                const VaryHeader& vary_allow_list);
+                const VaryAllowList& vary_allow_list);
 
   const RequestCacheControl& requestCacheControl() const { return request_cache_control_; }
 
@@ -207,7 +207,7 @@ class LookupRequest {
                                 ResponseMetadata&& metadata, uint64_t content_length) const;
 
   const Http::RequestHeaderMap& requestHeaders() const { return *request_headers_; }
-  const VaryHeader& varyAllowList() const { return vary_allow_list_; }
+  const VaryAllowList& varyAllowList() const { return vary_allow_list_; }
 
 private:
   void initializeRequestCacheControl(const Http::RequestHeaderMap& request_headers);
@@ -217,7 +217,7 @@ class LookupRequest {
   Key key_;
   std::vector<RawByteRange> request_range_spec_;
   Http::RequestHeaderMapPtr request_headers_;
-  const VaryHeader& vary_allow_list_;
+  const VaryAllowList& vary_allow_list_;
   // Time when this LookupRequest was created (in response to an HTTP request).
   SystemTime timestamp_;
   RequestCacheControl request_cache_control_;

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

@@ -86,7 +86,7 @@ class SimpleInsertContext : public InsertContext {
 private:
   void commit() {
     committed_ = true;
-    if (VaryHeader::hasVary(*response_headers_)) {
+    if (VaryHeaderUtils::hasVary(*response_headers_)) {
       cache_.varyInsert(key_, std::move(response_headers_), std::move(metadata_), body_.toString(),
                         request_headers_, vary_allow_list_);
     } else {
@@ -96,7 +96,7 @@ class SimpleInsertContext : public InsertContext {
 
   Key key_;
   const Http::RequestHeaderMap& request_headers_;
-  const VaryHeader& vary_allow_list_;
+  const VaryAllowList& vary_allow_list_;
   Http::ResponseHeaderMapPtr response_headers_;
   ResponseMetadata metadata_;
   SimpleHttpCache& cache_;
@@ -124,7 +124,7 @@ SimpleHttpCache::Entry SimpleHttpCache::lookup(const LookupRequest& request) {
   }
   ASSERT(iter->second.response_headers_);
 
-  if (VaryHeader::hasVary(*iter->second.response_headers_)) {
+  if (VaryHeaderUtils::hasVary(*iter->second.response_headers_)) {
     return varyLookup(request, iter->second.response_headers_);
   } else {
     return SimpleHttpCache::Entry{
@@ -147,12 +147,12 @@ SimpleHttpCache::varyLookup(const LookupRequest& request,
   mutex_.AssertReaderHeld();
 
   absl::btree_set<absl::string_view> vary_header_values =
-      VaryHeader::getVaryValues(*response_headers);
+      VaryHeaderUtils::getVaryValues(*response_headers);
   ASSERT(!vary_header_values.empty());
 
   Key varied_request_key = request.key();
-  const absl::optional<std::string> vary_identifier =
-      request.varyAllowList().createVaryIdentifier(vary_header_values, request.requestHeaders());
+  const absl::optional<std::string> vary_identifier = VaryHeaderUtils::createVaryIdentifier(
+      request.varyAllowList(), vary_header_values, request.requestHeaders());
   if (!vary_identifier.has_value()) {
     // The vary allow list has changed and has made the vary header of this
     // cached value not cacheable.
@@ -175,17 +175,17 @@ void SimpleHttpCache::varyInsert(const Key& request_key,
                                  Http::ResponseHeaderMapPtr&& response_headers,
                                  ResponseMetadata&& metadata, std::string&& body,
                                  const Http::RequestHeaderMap& request_headers,
-                                 const VaryHeader& vary_allow_list) {
+                                 const VaryAllowList& vary_allow_list) {
   absl::WriterMutexLock lock(&mutex_);
 
   absl::btree_set<absl::string_view> vary_header_values =
-      VaryHeader::getVaryValues(*response_headers);
+      VaryHeaderUtils::getVaryValues(*response_headers);
   ASSERT(!vary_header_values.empty());
 
   // Insert the varied response.
   Key varied_request_key = request_key;
   const absl::optional<std::string> vary_identifier =
-      vary_allow_list.createVaryIdentifier(vary_header_values, request_headers);
+      VaryHeaderUtils::createVaryIdentifier(vary_allow_list, vary_header_values, request_headers);
   if (!vary_identifier.has_value()) {
     // Skip the insert if we are unable to create a vary key.
     return;

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

@@ -44,7 +44,8 @@ class SimpleHttpCache : public HttpCache {
   // Inserts a response that has been varied on certain headers.
   void varyInsert(const Key& request_key, Http::ResponseHeaderMapPtr&& response_headers,
                   ResponseMetadata&& metadata, std::string&& body,
-                  const Http::RequestHeaderMap& request_headers, const VaryHeader& vary_allow_list);
+                  const Http::RequestHeaderMap& request_headers,
+                  const VaryAllowList& vary_allow_list);
 
   absl::Mutex mutex_;
   absl::flat_hash_map<Key, Entry, MessageUtil, MessageUtil> map_ ABSL_GUARDED_BY(mutex_);