Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions doc/admin-guide/plugins/header_rewrite.en.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1177,8 +1177,25 @@ set-body

set-body <text>

Sets the body to ``<text>``. Can also be used to delete a body with ``""``. This is only useful when overriding the origin status, i.e.
intercepting/pre-empting a request so that you can override the body from the body-factory with your own.
Sets the body to ``<text>``.
For internally generated/synthetic responses, ``set-body ""`` can be used to clear that replacement body.

For origin response replacement, ``set-body`` is supported at both
``READ_RESPONSE_HDR_HOOK`` and ``SEND_RESPONSE_HDR_HOOK``. Prefer
``READ_RESPONSE_HDR_HOOK`` when possible so body replacement happens before
response body tunneling starts.

.. note::

When ``set-body`` replaces an origin response body, ATS emits the replacement
through its internal error-body path. ``Content-Type`` defaults to
``text/html`` unless you override it with ``set-header Content-Type``.
``set-body ""`` clears the internal replacement body, but does not suppress an
origin response body on this hook; use a non-empty replacement value when
sanitizing origin responses.
Comment thread
bryancall marked this conversation as resolved.
The gold tests cover origin replacement for both hooks with and without an
active response transform. The transform-inactive matrix runs with HTTP cache
disabled and includes repeated-URL probes to verify deterministic replacement.

set-body-from
~~~~~~~~~~~~~
Expand Down
1 change: 1 addition & 0 deletions plugins/header_rewrite/operators.cc
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,7 @@ void
OperatorSetBody::initialize_hooks()
{
add_allowed_hook(TS_REMAP_PSEUDO_HOOK);
add_allowed_hook(TS_HTTP_READ_RESPONSE_HDR_HOOK);
add_allowed_hook(TS_HTTP_SEND_RESPONSE_HDR_HOOK);
}

Expand Down
3 changes: 3 additions & 0 deletions src/api/InkAPI.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4948,6 +4948,9 @@ TSHttpTxnErrorBodySet(TSHttpTxn txnp, char *buf, size_t buflength, char *mimetyp
s->internal_msg_buffer = buf;
s->internal_msg_buffer_size = buf ? buflength : 0;
s->internal_msg_buffer_fast_allocator_size = -1;
// TSHttpTxnErrorBodySet() and TSHttpTxnServerRequestBodySet() share the same buffer.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, and I'll start that here:

Claude has a big tendency to add comments that were part of its "thinking" phase. Explaining its reasoning for doing stuff. More often than not, I find those comments to be completely unnecessary, and often distracting, when the code itself is self explanatory. In most cases, the comments can be significantly reduced, and sometimes removed.

I'll refer to this as comment slop :).

// Switching to an error/response body override must clear the request-body mode.
s->api_server_request_body_set = false;

s->internal_msg_buffer_type = mimetype;
}
Expand Down
84 changes: 77 additions & 7 deletions src/proxy/http/HttpSM.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1687,9 +1687,37 @@ HttpSM::handle_api_return()

switch (t_state.next_action) {
case HttpTransact::StateMachineAction_t::TRANSFORM_READ: {
HttpTunnelProducer *p = setup_transfer_from_transform();
perform_transform_cache_write_action();
tunnel.tunnel_run(p);
// A plugin has installed an internal response body (TSHttpTxnErrorBodySet()).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment slop, code is self explanatory.

// In this branch we bypass transform streaming and switch to internal transfer.
if (t_state.internal_msg_buffer && !t_state.api_server_request_body_set && t_state.hdr_info.server_response.valid()) {
SMDbg(dbg_ctl_http, "plugin set internal body, bypassing response transform for internal transfer");
t_state.api_info.cache_untransformed = true;
// If a tunnel was already set up for transform I/O, shut it down before we re-route.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment slop, code says exactly the same thing as the comment.

if (tunnel.is_tunnel_active()) {
tunnel.kill_tunnel();
}
// Drop transform VC table state because this path no longer drives transform reads.
if (transform_info.entry != nullptr) {
vc_table.cleanup_entry(transform_info.entry);
transform_info.entry = nullptr;
}
transform_info.vc = nullptr;
// Some downstream paths still read client_response; seed it from transform_response when missing.
if (t_state.hdr_info.client_response.valid() == 0 && t_state.hdr_info.transform_response.valid()) {
t_state.hdr_info.client_response.create(HTTPType::RESPONSE);
t_state.hdr_info.client_response.copy(&t_state.hdr_info.transform_response);
}
// The server session is not needed for internal body transfer if it was never tunneled.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment slop.

if (server_entry != nullptr && server_entry->in_tunnel == false) {
release_server_session();
}
// Serve the plugin-provided body through the internal tunnel handler.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment slop.

setup_internal_transfer(&HttpSM::tunnel_handler);
} else {
HttpTunnelProducer *p = setup_transfer_from_transform();
perform_transform_cache_write_action();
tunnel.tunnel_run(p);
}
break;
}
case HttpTransact::StateMachineAction_t::SERVER_READ: {
Expand Down Expand Up @@ -1722,6 +1750,17 @@ HttpSM::handle_api_return()
}

setup_blind_tunnel(true, initial_data);
} else if (t_state.internal_msg_buffer && !t_state.api_server_request_body_set && t_state.hdr_info.server_response.valid() &&
plugin_tunnel == nullptr &&
(api_hooks.get(TS_HTTP_READ_RESPONSE_HDR_HOOK) != nullptr ||
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it even possible to get here, without one of these being true ? What's the intent here, that the plugin API wasn't called from somewhere else? But then we wouldn't have a server_response.valid() either.

I don't know for sure, but feels like either this check on the hook is just superfluous, or possibly should be a assert instead?

api_hooks.get(TS_HTTP_SEND_RESPONSE_HDR_HOOK) != nullptr)) {
// A plugin replaced the origin response body via TSHttpTxnErrorBodySet().
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment slop

// Serve the synthetic body before entering the response body tunnel.
SMDbg(dbg_ctl_http, "plugin set internal body, using internal transfer instead of server tunnel");
if (server_entry != nullptr && server_entry->in_tunnel == false) {
release_server_session();
}
setup_internal_transfer(&HttpSM::tunnel_handler);
} else {
HttpTunnelProducer *p = setup_server_transfer();
perform_cache_write_action();
Expand Down Expand Up @@ -7578,12 +7617,21 @@ HttpSM::setup_client_request_plugin_agents(HttpTunnelProducer *p, int num_header
inline void
HttpSM::transform_cleanup(TSHttpHookID hook, HttpTransformInfo *info)
{
// Internal-body bypass can skip transform tunnel setup, leaving no transform
// entry to clean up. In that case there is nothing safe/useful to close here.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment slop.

if (info->entry == nullptr) {
return;
}
APIHook *t_hook = api_hooks.get(hook);
if (t_hook && info->vc == nullptr) {
do {
VConnection *t_vcon = t_hook->m_cont;
t_vcon->do_io_close();
t_hook = t_hook->m_link.next;
APIHook *next = t_hook->m_link.next;
// Some transform hooks can already be detached by the time kill_this() runs.
// Guard against null continuations while still draining the remaining hooks.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first comment seems useful, the second is slop.

if (auto *t_vcon = static_cast<VConnection *>(t_hook->m_cont); t_vcon != nullptr) {
t_vcon->do_io_close();
}
t_hook = next;
} while (t_hook != nullptr);
}
Comment thread
bryancall marked this conversation as resolved.
}
Expand Down Expand Up @@ -7664,7 +7712,14 @@ HttpSM::kill_this()
// In that case, we need to manually close all the
// transforms to prevent memory leaks (INKqa06147)
if (hooks_set) {
transform_cleanup(TS_HTTP_RESPONSE_TRANSFORM_HOOK, &transform_info);
bool bypassed_response_transform =
t_state.api_info.cache_untransformed && t_state.internal_msg_buffer && !t_state.api_server_request_body_set;
// If we intentionally bypassed response transforms for internal-body
// transfer, transform_info may be partially detached; skip cleanup in this
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial comment slop, can be reduced to one line.

// specific case to avoid dereferencing stale transform state.
if (!bypassed_response_transform) {
transform_cleanup(TS_HTTP_RESPONSE_TRANSFORM_HOOK, &transform_info);
}
Comment thread
bryancall marked this conversation as resolved.
transform_cleanup(TS_HTTP_REQUEST_TRANSFORM_HOOK, &post_transform_info);
plugin_agents_cleanup();
}
Expand Down Expand Up @@ -8230,6 +8285,21 @@ HttpSM::set_next_state()
case HttpTransact::StateMachineAction_t::SERVER_READ: {
t_state.source = HttpTransact::Source_t::HTTP_ORIGIN_SERVER;

if (transform_info.vc && t_state.internal_msg_buffer && !t_state.api_server_request_body_set &&
t_state.hdr_info.server_response.valid()) {
SMDbg(dbg_ctl_http, "plugin set internal body, bypassing response transform");
t_state.api_info.cache_untransformed = true;
if (transform_info.entry != nullptr) {
vc_table.cleanup_entry(transform_info.entry);
transform_info.entry = nullptr;
}
transform_info.vc = nullptr;
if (t_state.hdr_info.client_response.valid() == 0 && t_state.hdr_info.transform_response.valid()) {
t_state.hdr_info.client_response.create(HTTPType::RESPONSE);
t_state.hdr_info.client_response.copy(&t_state.hdr_info.transform_response);
}
}

if (transform_info.vc) {
ink_assert(t_state.hdr_info.client_response.valid() == 0);
ink_assert((t_state.hdr_info.transform_response.valid() ? true : false) == true);
Expand Down
Loading