icpd opened a new issue, #13421:
URL: https://github.com/apache/trafficserver/issues/13421
## Description
`cache.config` accepts `url=` as a primary destination. A valid rule passes
parsing, but `UrlMatcher` does not add it to the URL hash table. The
configuration loads without an error, and the rule never matches a request.
The insertion code is in `UrlMatcher<Data, Result>::NewEntry()` in
`src/proxy/ControlMatcher.cc`:
```cpp
cur_d = data_array + num_el;
error = cur_d->Init(line_info);
if (error.failed()) {
url_str[num_el] = ats_strdup(pattern);
url_value[num_el] = num_el;
url_ht.emplace(url_str[num_el], url_value[num_el]);
num_el++;
}
return error;
```
`error.failed()` is `true` when initialization fails. This condition is
backwards: a successfully initialized rule skips the insertion into `url_ht`,
while the function still returns success.
## Expected behavior
When `cur_d->Init(line_info)` succeeds, ATS should add the rule to `url_ht`.
The rule's cache action should apply when a request URL exactly matches the
configured value.
## Cause
The exact URL matcher was added for TS-1280. The original code used a string
pointer for errors and inserted the rule only after successful initialization:
```cpp
errBuf = cur_d->Init(line_info);
if (errBuf == NULL) {
// Insert into url_ht.
}
```
Commit `70cc2d4ce1762b182dd52f43a6313c61772c3a8a` replaced the error string
with `config_parse_error`. That type evaluates to `true` when an error is
present, but the same change made the `UrlMatcher` condition `if (error)`,
reversing the old success check.
Commit `39f8e5c057386bffd51f5ba2284d2e715543e100` later changed the
condition to `if (error.failed())` without correcting the reversed logic.
Related commits:
- [[Original
implementation](https://github.com/apache/trafficserver/commit/2d9ad48c5075b40ebfd864b1e9ce4f097d84b85c)](https://github.com/apache/trafficserver/commit/2d9ad48c5075b40ebfd864b1e9ce4f097d84b85c)
- [[Commit that introduced the
regression](https://github.com/apache/trafficserver/commit/70cc2d4ce1762b182dd52f43a6313c61772c3a8a)](https://github.com/apache/trafficserver/commit/70cc2d4ce1762b182dd52f43a6313c61772c3a8a)
- [Conversion to
`Result`](https://github.com/apache/trafficserver/commit/39f8e5c057386bffd51f5ba2284d2e715543e100)
## Suggested fix
```diff
error = cur_d->Init(line_info);
-if (error.failed()) {
+if (!error.failed()) {
url_str[num_el] = ats_strdup(pattern);
url_value[num_el] = num_el;
url_ht.emplace(url_str[num_el], url_value[num_el]);
num_el++;
}
```
Regression tests should cover:
- A valid `url=` rule is inserted and matches the complete URL.
- A different URL does not match the rule.
- A failed `Init()` call does not insert a rule and returns a configuration
error.
- A duplicate `url=` value still returns the duplicate-rule error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
