bryancall commented on PR #12879: URL: https://github.com/apache/trafficserver/pull/12879#issuecomment-3892354961
Yes, I looked at `set-body-from` closely. The key difference is in how the transaction is reenabled after setting the body. `set-body-from` calls `TSHttpTxnReenable(txnp, TS_EVENT_HTTP_ERROR)`, which routes through `HandleApiErrorJump()`. That function wipes the existing client response headers (`fields_clear()`) and rebuilds the response from scratch using `build_response()`. This effectively creates a new ATS-generated response, which is why `setup_internal_transfer()` picks up the `internal_msg_buffer` -- it's already on the internal response path. `set-body`, on the other hand, reenables with `TS_EVENT_HTTP_CONTINUE`, which continues normal processing. The origin response flows through `setup_server_transfer()` / the HTTP tunnel, and `internal_msg_buffer` was simply never checked in that path -- until this PR. The ERROR approach in `set-body-from` has some trade-offs: - It wipes all origin response headers and rebuilds from scratch. This could be a feature when sanitizing responses (you don't leak origin headers), but it also means you can't selectively preserve headers you want. With the CONTINUE approach, origin headers are preserved by default and you can use `rm-header` to remove specific ones you don't want. - Status codes < 400 get clobbered to `500 INKApi Error` (there's a `TSHttpTxnStatusSet()` hack to work around this, but it only preserves codes >= 400). The existing test explicitly documents this as a bug: "*TSHttpTxnErrorBodySet will change OK status codes to 500 INKApi Error*" - It doesn't allow origin connection reuse since it doesn't drain the body Another important difference is how `set-status` composes with each operator. With this PR's CONTINUE approach, `set-status` + `set-body` works naturally -- `set-status` modifies the response header directly, `set-body` replaces the body, and `setup_internal_transfer()` sends both as-is. With `set-body-from`'s ERROR approach, `set-status` is broken regardless of operator order: - `set-status 200` before `set-body-from`: `set-body-from` reads the modified status into `http_return_code` (200), then `HandleApiErrorJump` sees 200 < 400 and builds a **500 INKApi Error** - `set-status 200` after `set-body-from`: `set-body-from` captures the original status (e.g. 403) into `http_return_code`, `set-status` modifies the header to 200, but then `HandleApiErrorJump` wipes all headers via `fields_clear()` and rebuilds from `http_return_code` (403) -- the `set-status` is completely ignored This PR takes the cleaner approach: check `internal_msg_buffer` in `handle_api_return()` before setting up the tunnel, drain the origin body when possible for connection reuse, and let the existing response headers flow through naturally. I'm planning a follow-up to standardize `set-body-from` on this same CONTINUE path, which would fix the status code bug, allow connection reuse, make `set-status` work correctly, and give users the choice of which headers to keep or remove. -- 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]
