You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bneradt (via GitHub)" <gi...@apache.org> on 2025/09/06 15:35:09 UTC
[PR] plss: Plugin Status Setter [trafficserver]
bneradt opened a new pull request, #12484:
URL: https://github.com/apache/trafficserver/pull/12484
In complex setups where there are multiple plugins that can be setting an error response, it can be non-trival to determine which component intervened for any given transaction. This adds the plss, plugin status setter, log field which provides an identifying string for the plugin (or internal component, such as ip_allow) that set the transaction's response status.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] prscs: proxy response status code setter [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on code in PR #12484:
URL: https://github.com/apache/trafficserver/pull/12484#discussion_r2349991533
##########
src/api/InkAPI.cc:
##########
@@ -2948,9 +2948,21 @@ TSHttpHdrStatusSet(TSMBuffer bufp, TSMLoc obj, TSHttpStatus status)
SET_HTTP_HDR(h, bufp, obj);
ink_assert(static_cast<HdrHeapObjType>(h.m_http->m_type) == HdrHeapObjType::HTTP_HEADER);
h.status_set(static_cast<HTTPStatus>(status));
+
+ if (txnp != nullptr && !setter.empty()) {
+ sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
+ HttpSM *sm = reinterpret_cast<HttpSM *>(txnp);
+ sm->t_state.http_return_code_setter_name = setter;
+ }
Review Comment:
`TSHttpTxnStatusSet` also sets `sm->t_state.http_return_code`, but that doesn't happen here which seems like the two fields could get out of sync?
##########
src/api/InkAPI.cc:
##########
@@ -2948,9 +2948,21 @@ TSHttpHdrStatusSet(TSMBuffer bufp, TSMLoc obj, TSHttpStatus status)
SET_HTTP_HDR(h, bufp, obj);
ink_assert(static_cast<HdrHeapObjType>(h.m_http->m_type) == HdrHeapObjType::HTTP_HEADER);
h.status_set(static_cast<HTTPStatus>(status));
+
+ if (txnp != nullptr && !setter.empty()) {
+ sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
+ HttpSM *sm = reinterpret_cast<HttpSM *>(txnp);
+ sm->t_state.http_return_code_setter_name = setter;
+ }
Review Comment:
This seems out of place here. This function was just operating on its arguments, but the change to also update the transaction is only situationally relevant. Wouldn't setting the return code setter be a separate function?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] prscs: proxy response status code setter [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #12484:
URL: https://github.com/apache/trafficserver/pull/12484#discussion_r2353837052
##########
src/api/InkAPI.cc:
##########
@@ -2948,9 +2948,21 @@ TSHttpHdrStatusSet(TSMBuffer bufp, TSMLoc obj, TSHttpStatus status)
SET_HTTP_HDR(h, bufp, obj);
ink_assert(static_cast<HdrHeapObjType>(h.m_http->m_type) == HdrHeapObjType::HTTP_HEADER);
h.status_set(static_cast<HTTPStatus>(status));
+
+ if (txnp != nullptr && !setter.empty()) {
+ sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
+ HttpSM *sm = reinterpret_cast<HttpSM *>(txnp);
+ sm->t_state.http_return_code_setter_name = setter;
+ }
Review Comment:
Sounds good. Yes, it is ugly to have to pass txnp for TSHttpHdrStatusSet.
I added a commit that avoids this by associating the txn with the TSMLoc when the Req/Rsp is retrieved. That way, later, we can look it up in the associated map. How does this look?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] prscs: proxy response status code setter [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #12484:
URL: https://github.com/apache/trafficserver/pull/12484#discussion_r2342017922
##########
tests/gold_tests/pluginTest/header_rewrite/header_rewrite.test.py:
##########
@@ -27,25 +29,46 @@
Test.testName = ""
request_header = {"headers": "GET / HTTP/1.1\r\nHost: www.example.com\r\n\r\n", "timestamp": "1469733493.993", "body": ""}
-# expected response from the origin server
response_header = {"headers": "HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n", "timestamp": "1469733493.993", "body": ""}
+server.addResponse("sessionfile.log", request_header, response_header)
-# add response to the server dictionary
+request_header = {"headers": "GET /503 HTTP/1.1\r\nHost: www.example.com\r\n\r\n", "timestamp": "1469733493.993", "body": ""}
+response_header = {
+ "headers": "HTTP/1.1 503 Service Unavailable\r\nConnection: close\r\n\r\n",
+ "timestamp": "1469733493.993",
+ "body": ""
+}
server.addResponse("sessionfile.log", request_header, response_header)
+
ts.Disk.records_config.update(
{
'proxy.config.diags.debug.enabled': 1,
'proxy.config.diags.show_location': 0,
- 'proxy.config.diags.debug.tags': 'header.*',
+ 'proxy.config.diags.debug.tags': 'header|http',
Review Comment:
Nah, you don't need `.*` for debug tag logging.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] plss: Plugin Status Setter [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12484:
URL: https://github.com/apache/trafficserver/pull/12484#discussion_r2331490267
##########
tests/gold_tests/pluginTest/header_rewrite/header_rewrite.test.py:
##########
@@ -27,25 +29,46 @@
Test.testName = ""
request_header = {"headers": "GET / HTTP/1.1\r\nHost: www.example.com\r\n\r\n", "timestamp": "1469733493.993", "body": ""}
-# expected response from the origin server
response_header = {"headers": "HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n", "timestamp": "1469733493.993", "body": ""}
+server.addResponse("sessionfile.log", request_header, response_header)
-# add response to the server dictionary
+request_header = {"headers": "GET /503 HTTP/1.1\r\nHost: www.example.com\r\n\r\n", "timestamp": "1469733493.993", "body": ""}
+response_header = {
+ "headers": "HTTP/1.1 503 Service Unavailable\r\nConnection: close\r\n\r\n",
+ "timestamp": "1469733493.993",
+ "body": ""
+}
server.addResponse("sessionfile.log", request_header, response_header)
+
ts.Disk.records_config.update(
{
'proxy.config.diags.debug.enabled': 1,
'proxy.config.diags.show_location': 0,
- 'proxy.config.diags.debug.tags': 'header.*',
+ 'proxy.config.diags.debug.tags': 'header|http',
Review Comment:
The debug tags string contains 'header|http' but based on the existing pattern and other files, this should likely be 'header.*|http' to match the header rewrite plugin debug pattern.
```suggestion
'proxy.config.diags.debug.tags': 'header.*|http',
```
##########
plugins/experimental/txn_box/plugin/src/ts_util.cc:
##########
@@ -660,7 +660,7 @@ ts::HttpHeader::field_remove(swoc::TextView name)
bool
ts::HttpResponse::status_set(TSHttpStatus status) const
{
- return TS_SUCCESS == TSHttpHdrStatusSet(_buff, _loc, status);
+ return TS_SUCCESS == TSHttpHdrStatusSet(_buff, _loc, status, nullptr, "");
Review Comment:
[nitpick] The TSHttpHdrStatusSet call passes nullptr for txnp and an empty string for setter, which defeats the purpose of the status tracking feature. Consider passing the actual transaction handle and 'txn_box' as the setter name to enable proper tracking.
```suggestion
return TS_SUCCESS == TSHttpHdrStatusSet(_buff, _loc, status, _txn, "txn_box");
```
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] prscs: proxy response status code setter [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt merged PR #12484:
URL: https://github.com/apache/trafficserver/pull/12484
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] prscs: proxy response status code setter [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #12484:
URL: https://github.com/apache/trafficserver/pull/12484#issuecomment-3300179593
Thanks for the review. A quick answer to this question:
> Also, wasn't there some discussion about removing proxy from the log field name?
The concern was not with "proxy" but with my previous "plugin" terminology. "plugin" is wrong because aspects of ATS core can set the response, not just plugins. This now uses "proxy". I think that's better. "proxy" in prscs fits well with the other p* log fields which are also proxy related.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org