fix: Use vhost name instead of Host header in cache key calculation #2376
Conversation
MorganaFuture
force-pushed
the
morgana/fix/per-vhost-cahe
branch
from
15818fb to
e96892c
Compare
|
@krizhanovsky Could you please assign someone to review this PR? |
krizhanovsky
requested review from
krizhanovsky and
const-t
and removed request for
krizhanovsky
| .len = req->vhost->name.len | ||
| }; | ||
| req->hash ^= tfw_hash_str(&vhost_str); | ||
| } else if (!TFW_STR_EMPTY(&req->host)) { |
There was a problem hiding this comment.
Please, don't fallback to host header. vhost must be already found at this stage.
@MorganaFuture Hi! I appreciate that you wrote unit test for this fix, however every cache feature must be covered by functional tests using our testing framework. If you have experience in python could you write such test? If not please tell me I'll create an issue for testing. The test must verify that behavior from the task is not reproducible now.
| @@ -90,6 +90,7 @@ TEST_SUITE(cfg); | |||
| TEST_SUITE(tfw_str); | |||
| TEST_SUITE(mem_fast); | |||
| TEST_SUITE(http2_parser_hpack); | |||
| TEST_SUITE(http_cache_key); | |||
There was a problem hiding this comment.
Let's rename this to have common suite for all cache functions.
| TEST_SUITE(http_cache_key); | |
| TEST_SUITE(http_cache); |
| @@ -7575,7 +7575,8 @@ tfw_http_hm_srv_send(TfwServer *srv, char *data, unsigned long len) | |||
| } | |||
|
|
|||
| /** | |||
| * Calculate the key of an HTTP request by hashing URI and Host header values. | |||
| * Calculate the key of an HTTP request by hashing URI and vhost name. | |||
| * If vhost is not yet assigned, fall back to using the Host header. | |||
| */ | |||
| unsigned long | |||
| tfw_http_req_key_calc(TfwHttpReq *req) | |||
There was a problem hiding this comment.
You changed only single function, however host header used in other places where key is used. tfw_http_req_key_calc() returns key that will be used as key in TDB, but also we do store/compare string key to avoid collisions. See TFW_CACHE_REQ_KEYITER().
| /* Keys should be the same since only uri_path is used for HM requests */ | ||
| EXPECT_EQ(key1, key2); | ||
|
|
||
| //test_req_free(req); // TODO: kernel stuck here, why? |
There was a problem hiding this comment.
Please fix it or remove TODO if it already fixed.
What
Use vhost name instead of Host header in cache key calculation
Why
This change ensures proper cache operation when requests are redirected to different vhosts through HTTP chains.
Previously, the cache key was built using uri_path + host (from Host header), which caused incorrect cache behavior when a request with a Host header for one vhost was redirected to a different vhost via HTTP chains. This would lead to storing the cached response under a key that combined the URI with the original Host header, rather than the actual vhost that served the content.
With this change, the cache key is now correctly built using uri_path + vhost_name, ensuring that the cache operates properly even when HTTP chains redirect requests between different vhosts. The code falls back to using the Host header when a vhost is not yet assigned.
Links
2366