Skip to content

stale_response: handle TS API failures instead of ignoring them #13592

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bryancall wants to merge 1 commit into
base: master
Choose a base branch
from
+111 −38
Open

stale_response: handle TS API failures instead of ignoring them #13592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 26 additions & 8 deletions plugins/experimental/stale_response/CacheUpdate.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,11 @@ has_trailing_parameter(TSMBuffer hdr_url_buf, TSMLoc hdr_url_loc)
{
bool bFound = false;

TSMLoc url_loc;
TSHttpHdrUrlGet(hdr_url_buf, hdr_url_loc, &url_loc);
TSMLoc url_loc = TS_NULL_MLOC;
if (TSHttpHdrUrlGet(hdr_url_buf, hdr_url_loc, &url_loc) != TS_SUCCESS) {
SRDBG(TAG_BAD, "[%s] TSHttpHdrUrlGet failed", __FUNCTION__);
return bFound;
}
// create the new url
UrlComponents reqUrl;
reqUrl.populate(hdr_url_buf, url_loc);
Expand All @@ -144,8 +147,11 @@ has_trailing_parameter(TSMBuffer hdr_url_buf, TSMLoc hdr_url_loc)
void
add_trailing_parameter(TSMBuffer hdr_url_buf, TSMLoc hdr_url_loc)
{
TSMLoc url_loc;
TSHttpHdrUrlGet(hdr_url_buf, hdr_url_loc, &url_loc);
TSMLoc url_loc = TS_NULL_MLOC;
if (TSHttpHdrUrlGet(hdr_url_buf, hdr_url_loc, &url_loc) != TS_SUCCESS) {
SRDBG(TAG_BAD, "[%s] TSHttpHdrUrlGet failed", __FUNCTION__);
return;
}
// create the new url
UrlComponents reqUrl;
reqUrl.populate(hdr_url_buf, url_loc);
Expand All @@ -163,7 +169,11 @@ add_trailing_parameter(TSMBuffer hdr_url_buf, TSMLoc hdr_url_loc)
// parse ans set the new url
const char *start = newUrl.c_str();
const char *end = newUrl.size() + start;
TSUrlParse(hdr_url_buf, url_loc, &start, end);
if (TSUrlParse(hdr_url_buf, url_loc, &start, end) != TS_PARSE_DONE) {
SRDBG(TAG_BAD, "[%s] TSUrlParse failed for [%s]", __FUNCTION__, newUrl.c_str());
TSHandleMLocRelease(hdr_url_buf, hdr_url_loc, url_loc);
return;
}

SRDBG(TAG, "[%s] [%s]", __FUNCTION__, newQuery.c_str());
TSHandleMLocRelease(hdr_url_buf, hdr_url_loc, url_loc);
Expand All @@ -174,8 +184,11 @@ bool
strip_trailing_parameter(TSMBuffer hdr_url_buf, TSMLoc hdr_url_loc)
{
bool stripped = false;
TSMLoc url_loc;
TSHttpHdrUrlGet(hdr_url_buf, hdr_url_loc, &url_loc);
TSMLoc url_loc = TS_NULL_MLOC;
if (TSHttpHdrUrlGet(hdr_url_buf, hdr_url_loc, &url_loc) != TS_SUCCESS) {
SRDBG(TAG_BAD, "[%s] TSHttpHdrUrlGet failed", __FUNCTION__);
return stripped;
}
// create the new url
UrlComponents reqUrl;
reqUrl.populate(hdr_url_buf, url_loc);
Expand All @@ -190,7 +203,12 @@ strip_trailing_parameter(TSMBuffer hdr_url_buf, TSMLoc hdr_url_loc)
stripped = true;
}
if (stripped) {
TSUrlHttpQuerySet(hdr_url_buf, url_loc, newQuery.c_str(), newQuery.size());
// report only what actually happened: if the query could not be rewritten, nothing was stripped
if (TSUrlHttpQuerySet(hdr_url_buf, url_loc, newQuery.c_str(), newQuery.size()) != TS_SUCCESS) {
SRDBG(TAG_BAD, "[%s] TSUrlHttpQuerySet failed", __FUNCTION__);
TSError("stale_response [%s] failed to strip internal query parameter", __FUNCTION__);
stripped = false;
}
}
TSHandleMLocRelease(hdr_url_buf, hdr_url_loc, url_loc);
SRDBG(TAG, "[%s] stripped=%d [%s]", __FUNCTION__, stripped, newQuery.c_str());
Expand Down
115 changes: 85 additions & 30 deletions plugins/experimental/stale_response/stale_response.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,16 +93,34 @@ create_request_info(TSHttpTxn txnp)
TSMBuffer hdr_url_buf;
TSMLoc hdr_url_loc;

TSHttpTxnClientReqGet(txnp, &hdr_url_buf, &hdr_url_loc);
if (TSHttpTxnClientReqGet(txnp, &hdr_url_buf, &hdr_url_loc) != TS_SUCCESS) {
SRDBG(TAG_BAD, "[%s] TSHttpTxnClientReqGet failed!", __FUNCTION__);
TSfree(req_info);
return nullptr;
}

// this only seems to be correct/consistent if done in http read request header state
char *url = TSHttpTxnEffectiveUrlStringGet(txnp, &req_info->effective_url_length);
char *url = TSHttpTxnEffectiveUrlStringGet(txnp, &req_info->effective_url_length);
if (url == nullptr) {
SRDBG(TAG_BAD, "[%s] TSHttpTxnEffectiveUrlStringGet failed!", __FUNCTION__);
TSHandleMLocRelease(hdr_url_buf, TS_NULL_MLOC, hdr_url_loc);
TSfree(req_info);
return nullptr;
}
req_info->effective_url = TSstrndup(url, req_info->effective_url_length);
TSfree(url);

// copy the headers
req_info->http_hdr_buf = TSMBufferCreate();
TSHttpHdrClone(req_info->http_hdr_buf, hdr_url_buf, hdr_url_loc, &(req_info->http_hdr_loc));
req_info->http_hdr_loc = TS_NULL_MLOC;
if (TSHttpHdrClone(req_info->http_hdr_buf, hdr_url_buf, hdr_url_loc, &(req_info->http_hdr_loc)) != TS_SUCCESS) {
SRDBG(TAG_BAD, "[%s] TSHttpHdrClone failed!", __FUNCTION__);
TSHandleMLocRelease(hdr_url_buf, TS_NULL_MLOC, hdr_url_loc);
TSMBufferDestroy(req_info->http_hdr_buf);
TSfree(req_info->effective_url);
TSfree(req_info);
return nullptr;
}
// release the client request
TSHandleMLocRelease(hdr_url_buf, TS_NULL_MLOC, hdr_url_loc);

Expand Down Expand Up @@ -141,6 +159,10 @@ create_state_info(TSHttpTxn txnp, TSCont contp)
{
StateInfo *state = new StateInfo(txnp, contp);
state->req_info = create_request_info(txnp);
if (state->req_info == nullptr) {
delete state;
return nullptr;
}
return state;
}

Expand Down Expand Up @@ -345,11 +367,15 @@ void
send_stale_response(StateInfo *state)
{
// force to use age header
TSHttpTxnConfigIntSet(state->txnp, TS_CONFIG_HTTP_INSERT_AGE_IN_RESPONSE, 1);
if (TSHttpTxnConfigIntSet(state->txnp, TS_CONFIG_HTTP_INSERT_AGE_IN_RESPONSE, 1) != TS_SUCCESS) {
SRDBG(TAG_BAD, "[%s] {%u} TSHttpTxnConfigIntSet(INSERT_AGE_IN_RESPONSE) failed", __FUNCTION__, state->req_info->key_hash);
}
// add send response header hook for warning header
TSHttpTxnHookAdd(state->txnp, TS_HTTP_SEND_RESPONSE_HDR_HOOK, state->transaction_contp);
// set cache as fresh
TSHttpTxnCacheLookupStatusSet(state->txnp, TS_CACHE_LOOKUP_HIT_FRESH);
if (TSHttpTxnCacheLookupStatusSet(state->txnp, TS_CACHE_LOOKUP_HIT_FRESH) != TS_SUCCESS) {
SRDBG(TAG_BAD, "[%s] {%u} TSHttpTxnCacheLookupStatusSet failed", __FUNCTION__, state->req_info->key_hash);
}
}

/*-----------------------------------------------------------------------------------------------*/
Expand Down Expand Up @@ -677,11 +703,15 @@ fetch_finish(StateInfo *state)
// Add sie_server_intercept header.
TSMBuffer buf;
TSMLoc hdr_loc;
TSHttpTxnClientReqGet(state->txnp, &buf, &hdr_loc);
if (!add_header(buf, hdr_loc, SIE_SERVER_INTERCEPT_HEADER, HTTP_VALUE_SERVER_INTERCEPT)) {
if (TSHttpTxnClientReqGet(state->txnp, &buf, &hdr_loc) != TS_SUCCESS) {
SRDBG(TAG_BAD, "[%s] {%u} TSHttpTxnClientReqGet failed", __FUNCTION__, state->req_info->key_hash);
TSError("stale_response [%s] error inserting header %s", __FUNCTION__, SIE_SERVER_INTERCEPT_HEADER);
} else {
if (!add_header(buf, hdr_loc, SIE_SERVER_INTERCEPT_HEADER, HTTP_VALUE_SERVER_INTERCEPT)) {
TSError("stale_response [%s] error inserting header %s", __FUNCTION__, SIE_SERVER_INTERCEPT_HEADER);
}
TSHandleMLocRelease(buf, TS_NULL_MLOC, hdr_loc);
}
TSHandleMLocRelease(buf, TS_NULL_MLOC, hdr_loc);
serverInterceptSetup(state->txnp, pBody, state->plugin_config);
// This was TSHttpTxnNewCacheLookupDo -- now we just use the copy we have
}
Expand Down Expand Up @@ -958,11 +988,14 @@ transaction_handler(TSCont contp, TSEvent event, void *edata)
if (plugin_config->intercept_reroute) {
TSMBuffer buf;
TSMLoc hdr_loc;
TSHttpTxnClientReqGet(txnp, &buf, &hdr_loc);
if (strip_trailing_parameter(buf, hdr_loc)) {
SRDBG(TAG_BAD, "[%s] {%u} missed fake internal cache lookup", __FUNCTION__, state->req_info->key_hash);
if (TSHttpTxnClientReqGet(txnp, &buf, &hdr_loc) != TS_SUCCESS) {
SRDBG(TAG_BAD, "[%s] {%u} TSHttpTxnClientReqGet failed", __FUNCTION__, state->req_info->key_hash);
} else {
if (strip_trailing_parameter(buf, hdr_loc)) {
SRDBG(TAG_BAD, "[%s] {%u} missed fake internal cache lookup", __FUNCTION__, state->req_info->key_hash);
}
TSHandleMLocRelease(buf, TS_NULL_MLOC, hdr_loc);
}
TSHandleMLocRelease(buf, TS_NULL_MLOC, hdr_loc);
}

// free state - reenable - had check
Expand All @@ -983,9 +1016,12 @@ transaction_handler(TSCont contp, TSEvent event, void *edata)

case TS_EVENT_HTTP_SEND_REQUEST_HDR: {
SRDBG(TAG, "[%s]: strip_trailing_parameter", __FUNCTION__);
TSHttpTxnServerReqGet(txnp, &buf, &loc);
strip_trailing_parameter(buf, loc);
TSHandleMLocRelease(buf, TS_NULL_MLOC, loc);
if (TSHttpTxnServerReqGet(txnp, &buf, &loc) != TS_SUCCESS) {
SRDBG(TAG_BAD, "[%s] TSHttpTxnServerReqGet failed", __FUNCTION__);
} else {
strip_trailing_parameter(buf, loc);
TSHandleMLocRelease(buf, TS_NULL_MLOC, loc);
}

// reenable
TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE);
Expand All @@ -995,24 +1031,31 @@ transaction_handler(TSCont contp, TSEvent event, void *edata)

case TS_EVENT_HTTP_READ_RESPONSE_HDR:
// this should be internal request dont cache if valid sie error code -- no state variable
TSHttpTxnServerRespGet(txnp, &buf, &loc);
http_status = TSHttpHdrStatusGet(buf, loc);
if (valid_sie_status(http_status)) {
SRDBG(TAG, "[%s] Set non-cachable %d", __FUNCTION__, http_status);
TSHttpTxnServerRespNoStoreSet(txnp, 1);
if (TSHttpTxnServerRespGet(txnp, &buf, &loc) != TS_SUCCESS) {
SRDBG(TAG_BAD, "[%s] TSHttpTxnServerRespGet failed", __FUNCTION__);
} else {
http_status = TSHttpHdrStatusGet(buf, loc);
if (valid_sie_status(http_status)) {
TSHttpTxnServerRespNoStoreSet(txnp, 1);
SRDBG(TAG, "[%s] Set non-cachable %d", __FUNCTION__, http_status);
}
TSHandleMLocRelease(buf, TS_NULL_MLOC, loc);
}
TSHandleMLocRelease(buf, TS_NULL_MLOC, loc);
TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE);
break;

case TS_EVENT_HTTP_SEND_RESPONSE_HDR:
// add in the stale warning header -- no state variable
SRDBG(TAG, "[%s] set warning header", __FUNCTION__);
TSHttpTxnClientRespGet(txnp, &buf, &loc);
if (!add_header(buf, loc, TS_MIME_FIELD_WARNING, HTTP_VALUE_STALE_WARNING)) {
if (TSHttpTxnClientRespGet(txnp, &buf, &loc) != TS_SUCCESS) {
SRDBG(TAG_BAD, "[%s] TSHttpTxnClientRespGet failed", __FUNCTION__);
TSError("stale_response [%s] error inserting header %s", __FUNCTION__, TS_MIME_FIELD_WARNING);
} else {
if (!add_header(buf, loc, TS_MIME_FIELD_WARNING, HTTP_VALUE_STALE_WARNING)) {
TSError("stale_response [%s] error inserting header %s", __FUNCTION__, TS_MIME_FIELD_WARNING);
}
TSHandleMLocRelease(buf, TS_NULL_MLOC, loc);
}
TSHandleMLocRelease(buf, TS_NULL_MLOC, loc);
TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE);
break;

Expand Down Expand Up @@ -1112,7 +1155,10 @@ parse_args(int argc, char const *argv[])
char const *const log_filename =
plugin_config->log_info.filename_override.empty() ? PLUGIN_TAG : plugin_config->log_info.filename_override.c_str();
SRDBG(TAG, "[%s] Logging to %s", __FUNCTION__, log_filename);
TSTextLogObjectCreate(log_filename, TS_LOG_MODE_ADD_TIMESTAMP, &(plugin_config->log_info.object));
if (TSTextLogObjectCreate(log_filename, TS_LOG_MODE_ADD_TIMESTAMP, &(plugin_config->log_info.object)) != TS_SUCCESS) {
SRDBG(TAG_BAD, "[%s] TSTextLogObjectCreate failed for %s", __FUNCTION__, log_filename);
TSError("stale_response [%s] failed to create log object %s", __FUNCTION__, log_filename);
}
}

SRDBG(TAG, "[%s] global stale if error override = %" PRIdMAX, __FUNCTION__,
Expand All @@ -1137,6 +1183,11 @@ read_request_header_handler(TSHttpTxn const txnp, ConfigInfo *plugin_config)
TSContDataSet(transaction_contp, plugin_config);
// todo: move state create to not always happen -- issue: effective url string seems to change in dif states
StateInfo *state = create_state_info(txnp, transaction_contp);
if (state == nullptr) {
SRDBG(TAG_BAD, "[%s] create_state_info failed, skipping this transaction", __FUNCTION__);
TSContDestroy(transaction_contp);
return;
}
TSUserArgSet(txnp, plugin_config->txn_slot, state);

if (TSHttpTxnIsInternal(txnp)) {
Expand All @@ -1162,10 +1213,13 @@ read_request_header_handler(TSHttpTxn const txnp, ConfigInfo *plugin_config)
// add the async to the end so we use the fake cached response
TSMBuffer buf;
TSMLoc hdr_loc;
TSHttpTxnClientReqGet(txnp, &buf, &hdr_loc);
add_trailing_parameter(buf, hdr_loc);
TSHandleMLocRelease(buf, TS_NULL_MLOC, hdr_loc);
SRDBG(TAG, "[%s] {%u} add async parm to get fake cached item", __FUNCTION__, state->req_info->key_hash);
if (TSHttpTxnClientReqGet(txnp, &buf, &hdr_loc) != TS_SUCCESS) {
SRDBG(TAG_BAD, "[%s] {%u} TSHttpTxnClientReqGet failed", __FUNCTION__, state->req_info->key_hash);
} else {
add_trailing_parameter(buf, hdr_loc);
TSHandleMLocRelease(buf, TS_NULL_MLOC, hdr_loc);
SRDBG(TAG, "[%s] {%u} add async parm to get fake cached item", __FUNCTION__, state->req_info->key_hash);
}
}
}
}
Expand Down Expand Up @@ -1216,7 +1270,8 @@ TSPluginInit(int argc, const char *argv[])
SRDBG(TAG, "Plugin registration succeeded.");

TSMgmtString value = nullptr;
TSMgmtStringGet("proxy.config.http.server_session_sharing.pool", &value);
// a failed get leaves value as nullptr, which the check below already treats as misconfigured
static_cast<void>(TSMgmtStringGet("proxy.config.http.server_session_sharing.pool", &value));
if (nullptr == value || 0 != strcasecmp(value, "global")) {
TSError("[stale-response] Server session pool must be set to 'global'");
assert(false);
Expand Down