diff --git a/include/proxy/PreTransactionLogData.h b/include/proxy/NonHttpSmLogData.h similarity index 61% rename from include/proxy/PreTransactionLogData.h rename to include/proxy/NonHttpSmLogData.h index 0ec03c646bb..552e09622ea 100644 --- a/include/proxy/PreTransactionLogData.h +++ b/include/proxy/NonHttpSmLogData.h @@ -1,7 +1,7 @@ /** @file - PreTransactionLogData populates LogData for requests that fail before - HttpSM creation. + NonHttpSmLogData populates LogData for access-log entries that cannot be + backed by an HttpSM. @section license License @@ -30,23 +30,32 @@ #include -/** Populate LogData for requests that never created an HttpSM. +/** Owns access-log data for entries that cannot be backed by an @c HttpSM. * - * Malformed HTTP/2 or HTTP/3 request headers can be rejected while the - * connection is still decoding and validating the stream, before the request - * progresses far enough to create an HttpSM. This class carries the - * copied request and session metadata needed to emit a best-effort - * transaction log entry for those failures. + * Normal transaction access logging is expected to use data extracted from a + * live @c HttpSM. This type is for exceptional client-facing failures that need + * transaction-log visibility but occur before an @c HttpSM exists, such as + * malformed HTTP/2 or HTTP/3 request headers rejected during protocol + * validation. It may also be used for connection-level failures, such as TLS + * handshake errors, when operators need those events in the access log. * - * This class owns its milestones, addresses, and strings because the - * originating stream is about to be destroyed. + * Because the protocol stream or connection state may be destroyed immediately + * after the failure is handled, this object owns the copied headers, + * addresses, milestones, protocol strings, and outcome fields needed by + * @c LogAccess. Fields that require an @c HttpSM, origin transaction, cache + * lookup, or server response are intentionally left unset and marshal through + * the normal default values. + * + * This path should remain narrow. If an @c HttpSM exists, prefer the standard + * @c HttpSM-backed logging path so normal transactions do not pay for extra + * copying or exceptional state. */ -class PreTransactionLogData +class NonHttpSmLogData { public: - PreTransactionLogData() = default; + NonHttpSmLogData() = default; - ~PreTransactionLogData() + ~NonHttpSmLogData() { if (owned_client_request.valid()) { owned_client_request.destroy(); diff --git a/include/proxy/ProxyTransaction.h b/include/proxy/ProxyTransaction.h index 7316be293a2..32b24c6b5bc 100644 --- a/include/proxy/ProxyTransaction.h +++ b/include/proxy/ProxyTransaction.h @@ -146,18 +146,19 @@ class ProxyTransaction : public VConnection void mark_as_tunnel_endpoint() override; - /** Emit a best-effort access log entry for a request that failed before - * HttpSM creation. + /** Emit a best-effort access log entry for a request without an HttpSM. * * Call this when a malformed request is rejected at the protocol layer * (e.g. during HTTP/2 or HTTP/3 header decoding) and no HttpSM was - * created. The method populates a PreTransactionLogData from the + * created. The method populates a NonHttpSmLogData from the * session and the partially decoded request, then invokes Log::access. + * If an HttpSM exists, callers should use the normal transaction logging + * path instead. * * @param[in] request The decoded (possibly partial) request header. * @param[in] protocol_str Protocol string for the log entry (e.g. "http/2"). */ - void log_pre_transaction_access(HTTPHdr const *request, const char *protocol_str); + void log_non_http_sm_access(HTTPHdr const *request, const char *protocol_str); /// Variables // diff --git a/include/proxy/logging/Log.h b/include/proxy/logging/Log.h index 20019bf6ce8..fffc8adf2f2 100644 --- a/include/proxy/logging/Log.h +++ b/include/proxy/logging/Log.h @@ -43,7 +43,7 @@ @section example Example usage of the API @code - // Populate a TransactionLogData, then: + // Populate a TransactionLogData source, then: LogAccess entry(data); int ret = Log::access(&entry); @endcode diff --git a/include/proxy/logging/LogAccess.h b/include/proxy/logging/LogAccess.h index e449b674b2c..8ccee12f1a5 100644 --- a/include/proxy/logging/LogAccess.h +++ b/include/proxy/logging/LogAccess.h @@ -123,8 +123,8 @@ class LogAccess * The caller retains ownership of @a data, which must outlive the * synchronous Log::access() call that marshals this entry. * - * @param[in] data Populated TransactionLogData for a completed or - * pre-transaction entry. + * @param[in] data Populated TransactionLogData for an HttpSM-backed or + * non-HttpSM entry. */ explicit LogAccess(TransactionLogData &data); diff --git a/include/proxy/logging/TransactionLogData.h b/include/proxy/logging/TransactionLogData.h index 0c1eefd53a1..8076674ed4d 100644 --- a/include/proxy/logging/TransactionLogData.h +++ b/include/proxy/logging/TransactionLogData.h @@ -31,20 +31,20 @@ #include class HttpSM; -class PreTransactionLogData; +class NonHttpSmLogData; -/** Provide access-log data from either a completed HttpSM or pre-transaction storage. +/** Provide access-log data from either a completed HttpSM or non-HttpSM storage. * * The common completed-transaction path reads directly from @c HttpSM. The - * rare pre-transaction path reads from @c PreTransactionLogData, which owns - * copied request/session state for malformed requests rejected before HttpSM - * creation. + * rare non-HttpSM path reads from @c NonHttpSmLogData, which owns copied + * request/session state for exceptional access-log entries that cannot be + * backed by an @c HttpSM. */ class TransactionLogData { public: explicit TransactionLogData(HttpSM *sm); - explicit TransactionLogData(PreTransactionLogData const &pre_data); + explicit TransactionLogData(NonHttpSmLogData const &non_http_sm_data); void *http_sm_for_plugins() const; @@ -185,7 +185,7 @@ class TransactionLogData // ===== Server response Transfer-Encoding ===== std::string_view get_server_response_transfer_encoding() const; - // ===== Fallback fields for pre-transaction logging ===== + // ===== Fallback fields for non-HttpSM logging ===== std::string_view get_method() const; std::string_view get_scheme() const; std::string_view get_client_protocol_str() const; @@ -194,8 +194,8 @@ class TransactionLogData TransactionLogData &operator=(const TransactionLogData &) = delete; private: - HttpSM *m_http_sm = nullptr; - PreTransactionLogData const *m_pre_data = nullptr; + HttpSM *m_http_sm = nullptr; + NonHttpSmLogData const *m_non_http_sm_data = nullptr; // Cached values for fields that require computation or string formatting. mutable char m_client_rx_error_code[10] = {'-', '\0'}; diff --git a/src/proxy/ProxyTransaction.cc b/src/proxy/ProxyTransaction.cc index c2a5a4584e7..a301ce20deb 100644 --- a/src/proxy/ProxyTransaction.cc +++ b/src/proxy/ProxyTransaction.cc @@ -23,7 +23,7 @@ #include "proxy/http/HttpSM.h" #include "proxy/Plugin.h" -#include "proxy/PreTransactionLogData.h" +#include "proxy/NonHttpSmLogData.h" #include "proxy/logging/LogAccess.h" #include "proxy/logging/TransactionLogData.h" #include "proxy/logging/Log.h" @@ -343,7 +343,7 @@ get_pseudo_header_value(HTTPHdr const &hdr, std::string_view name) } // end anonymous namespace void -ProxyTransaction::log_pre_transaction_access(HTTPHdr const *request, const char *protocol_str) +ProxyTransaction::log_non_http_sm_access(HTTPHdr const *request, const char *protocol_str) { if (get_sm() != nullptr) { return; @@ -358,7 +358,7 @@ ProxyTransaction::log_pre_transaction_access(HTTPHdr const *request, const char return; } - PreTransactionLogData data; + NonHttpSmLogData data; data.owned_client_request.create(HTTPType::REQUEST, request->version_get()); data.owned_client_request.copy(request); diff --git a/src/proxy/http2/Http2ConnectionState.cc b/src/proxy/http2/Http2ConnectionState.cc index 34effdf60b1..9c80aefe295 100644 --- a/src/proxy/http2/Http2ConnectionState.cc +++ b/src/proxy/http2/Http2ConnectionState.cc @@ -497,7 +497,7 @@ Http2ConnectionState::rcv_headers_frame(const Http2Frame &frame) "recv headers enhance your calm"); } else { if (!stream->trailing_header_is_possible() && !stream->is_outbound_connection()) { - stream->log_pre_transaction_access(stream->get_receive_header(), "http/2"); + stream->log_non_http_sm_access(stream->get_receive_header(), "http/2"); } return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_STREAM, Http2ErrorCode::HTTP2_ERROR_PROTOCOL_ERROR, "recv headers malformed request"); @@ -1112,7 +1112,7 @@ Http2ConnectionState::rcv_continuation_frame(const Http2Frame &frame) "continuation enhance your calm"); } else { if (!stream->trailing_header_is_possible() && !stream->is_outbound_connection()) { - stream->log_pre_transaction_access(stream->get_receive_header(), "http/2"); + stream->log_non_http_sm_access(stream->get_receive_header(), "http/2"); } return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_CONNECTION, Http2ErrorCode::HTTP2_ERROR_PROTOCOL_ERROR, "continuation malformed request"); diff --git a/src/proxy/http3/Http3HeaderVIOAdaptor.cc b/src/proxy/http3/Http3HeaderVIOAdaptor.cc index 396733d6252..9d3e256b7f6 100644 --- a/src/proxy/http3/Http3HeaderVIOAdaptor.cc +++ b/src/proxy/http3/Http3HeaderVIOAdaptor.cc @@ -110,7 +110,7 @@ Http3HeaderVIOAdaptor::_on_qpack_decode_complete() NON_TRAILER)) { Dbg(dbg_ctl_http3, "Header is invalid"); if (this->_txn != nullptr) { - this->_txn->log_pre_transaction_access(&this->_header, "http/3"); + this->_txn->log_non_http_sm_access(&this->_header, "http/3"); } return -1; } @@ -118,7 +118,7 @@ Http3HeaderVIOAdaptor::_on_qpack_decode_complete() if (res != 0) { Dbg(dbg_ctl_http3, "ParseResult::ERROR"); if (this->_txn != nullptr) { - this->_txn->log_pre_transaction_access(&this->_header, "http/3"); + this->_txn->log_non_http_sm_access(&this->_header, "http/3"); } return -1; } diff --git a/src/proxy/logging/TransactionLogData.cc b/src/proxy/logging/TransactionLogData.cc index af44b5e374b..959b406b16b 100644 --- a/src/proxy/logging/TransactionLogData.cc +++ b/src/proxy/logging/TransactionLogData.cc @@ -22,7 +22,7 @@ */ #include "proxy/logging/TransactionLogData.h" -#include "proxy/PreTransactionLogData.h" +#include "proxy/NonHttpSmLogData.h" #include "proxy/http/HttpSM.h" #include "proxy/logging/LogAccess.h" #include "proxy/hdrs/MIME.h" @@ -92,7 +92,7 @@ TransactionLogData::TransactionLogData(HttpSM *sm) : m_http_sm(sm) ink_assert(sm != nullptr); } -TransactionLogData::TransactionLogData(PreTransactionLogData const &pre_data) : m_pre_data(&pre_data) {} +TransactionLogData::TransactionLogData(NonHttpSmLogData const &non_http_sm_data) : m_non_http_sm_data(&non_http_sm_data) {} void * TransactionLogData::http_sm_for_plugins() const @@ -111,7 +111,7 @@ TransactionLogData::get_milestones() const if (likely(m_http_sm != nullptr)) { return &m_http_sm->milestones; } - return &m_pre_data->owned_milestones; + return &m_non_http_sm_data->owned_milestones; } // ===== Headers ===== @@ -127,8 +127,8 @@ TransactionLogData::get_client_request() const return nullptr; } - if (m_pre_data->owned_client_request.valid()) { - return const_cast(&m_pre_data->owned_client_request); + if (m_non_http_sm_data->owned_client_request.valid()) { + return const_cast(&m_non_http_sm_data->owned_client_request); } return nullptr; } @@ -207,7 +207,7 @@ TransactionLogData::get_client_req_url_str() const cache_url_strings(); return m_client_req_url_str; } - return m_pre_data->owned_url.empty() ? nullptr : m_pre_data->owned_url.c_str(); + return m_non_http_sm_data->owned_url.empty() ? nullptr : m_non_http_sm_data->owned_url.c_str(); } int @@ -217,7 +217,7 @@ TransactionLogData::get_client_req_url_len() const cache_url_strings(); return m_client_req_url_len; } - return static_cast(m_pre_data->owned_url.size()); + return static_cast(m_non_http_sm_data->owned_url.size()); } const char * @@ -227,7 +227,7 @@ TransactionLogData::get_client_req_url_path_str() const cache_url_strings(); return m_client_req_url_path_str; } - return m_pre_data->owned_path.empty() ? nullptr : m_pre_data->owned_path.c_str(); + return m_non_http_sm_data->owned_path.empty() ? nullptr : m_non_http_sm_data->owned_path.c_str(); } int @@ -237,7 +237,7 @@ TransactionLogData::get_client_req_url_path_len() const cache_url_strings(); return m_client_req_url_path_len; } - return static_cast(m_pre_data->owned_path.size()); + return static_cast(m_non_http_sm_data->owned_path.size()); } // ===== Proxy response content-type / reason ===== @@ -374,7 +374,7 @@ TransactionLogData::get_client_addr() const if (likely(m_http_sm != nullptr)) { return &m_http_sm->t_state.effective_client_addr.sa; } - return &m_pre_data->owned_client_addr.sa; + return &m_non_http_sm_data->owned_client_addr.sa; } sockaddr const * @@ -383,7 +383,7 @@ TransactionLogData::get_client_src_addr() const if (likely(m_http_sm != nullptr)) { return &m_http_sm->t_state.client_info.src_addr.sa; } - return &m_pre_data->owned_client_src_addr.sa; + return &m_non_http_sm_data->owned_client_src_addr.sa; } sockaddr const * @@ -392,7 +392,7 @@ TransactionLogData::get_client_dst_addr() const if (likely(m_http_sm != nullptr)) { return &m_http_sm->t_state.client_info.dst_addr.sa; } - return &m_pre_data->owned_client_dst_addr.sa; + return &m_non_http_sm_data->owned_client_dst_addr.sa; } sockaddr const * @@ -418,7 +418,7 @@ TransactionLogData::get_client_port() const } return 0; } - return m_pre_data->m_client_port; + return m_non_http_sm_data->m_client_port; } // ===== Server addressing ===== @@ -473,7 +473,7 @@ TransactionLogData::get_log_code() const if (likely(m_http_sm != nullptr)) { return m_http_sm->t_state.squid_codes.log_code; } - return m_pre_data->m_log_code; + return m_non_http_sm_data->m_log_code; } SquidSubcode @@ -491,7 +491,7 @@ TransactionLogData::get_hit_miss_code() const if (likely(m_http_sm != nullptr)) { return m_http_sm->t_state.squid_codes.hit_miss_code; } - return m_pre_data->m_hit_miss_code; + return m_non_http_sm_data->m_hit_miss_code; } SquidHierarchyCode @@ -500,7 +500,7 @@ TransactionLogData::get_hier_code() const if (likely(m_http_sm != nullptr)) { return m_http_sm->t_state.squid_codes.hier_code; } - return m_pre_data->m_hier_code; + return m_non_http_sm_data->m_hier_code; } // ===== Byte counters ===== @@ -585,7 +585,7 @@ TransactionLogData::get_connection_id() const if (likely(m_http_sm != nullptr)) { return m_http_sm->client_connection_id(); } - return m_pre_data->m_connection_id; + return m_non_http_sm_data->m_connection_id; } int @@ -594,7 +594,7 @@ TransactionLogData::get_transaction_id() const if (likely(m_http_sm != nullptr)) { return m_http_sm->client_transaction_id(); } - return m_pre_data->m_transaction_id; + return m_non_http_sm_data->m_transaction_id; } int @@ -643,7 +643,7 @@ TransactionLogData::get_client_protocol() const if (likely(m_http_sm != nullptr)) { return m_http_sm->get_user_agent().get_client_protocol(); } - return m_pre_data->owned_client_protocol_str.empty() ? nullptr : m_pre_data->owned_client_protocol_str.c_str(); + return m_non_http_sm_data->owned_client_protocol_str.empty() ? nullptr : m_non_http_sm_data->owned_client_protocol_str.c_str(); } const char * @@ -734,7 +734,7 @@ TransactionLogData::get_client_connection_is_ssl() const if (likely(m_http_sm != nullptr)) { return m_http_sm->get_user_agent().get_client_connection_is_ssl(); } - return m_pre_data->m_client_connection_is_ssl; + return m_non_http_sm_data->m_client_connection_is_ssl; } bool @@ -805,7 +805,7 @@ TransactionLogData::get_server_transact_count() const if (likely(m_http_sm != nullptr)) { return m_http_sm->server_transact_count; } - return m_pre_data->m_server_transact_count; + return m_non_http_sm_data->m_server_transact_count; } // ===== Finish status ===== @@ -1087,7 +1087,7 @@ TransactionLogData::get_server_response_transfer_encoding() const return {}; } -// ===== Fallback fields for pre-transaction logging ===== +// ===== Fallback fields for non-HttpSM logging ===== std::string_view TransactionLogData::get_method() const @@ -1095,7 +1095,7 @@ TransactionLogData::get_method() const if (likely(m_http_sm != nullptr)) { return {}; } - return m_pre_data->owned_method; + return m_non_http_sm_data->owned_method; } std::string_view @@ -1104,7 +1104,7 @@ TransactionLogData::get_scheme() const if (likely(m_http_sm != nullptr)) { return {}; } - return m_pre_data->owned_scheme; + return m_non_http_sm_data->owned_scheme; } std::string_view @@ -1113,5 +1113,5 @@ TransactionLogData::get_client_protocol_str() const if (likely(m_http_sm != nullptr)) { return {}; } - return m_pre_data->owned_client_protocol_str; + return m_non_http_sm_data->owned_client_protocol_str; } diff --git a/src/proxy/logging/unit-tests/test_LogAccess.cc b/src/proxy/logging/unit-tests/test_LogAccess.cc index aee2bd67287..94f6f8f91d6 100644 --- a/src/proxy/logging/unit-tests/test_LogAccess.cc +++ b/src/proxy/logging/unit-tests/test_LogAccess.cc @@ -23,7 +23,7 @@ #include -#include "proxy/PreTransactionLogData.h" +#include "proxy/NonHttpSmLogData.h" #include "proxy/logging/LogAccess.h" #include "proxy/logging/TransactionLogData.h" #include "tscore/ink_inet.h" @@ -101,8 +101,8 @@ set_socket_address(IpEndpoint &ep, std::string_view text) } void -populate_pre_transaction_data(PreTransactionLogData &data, std::string_view method, std::string_view scheme, - std::string_view authority, std::string_view path) +populate_non_http_sm_data(NonHttpSmLogData &data, std::string_view method, std::string_view scheme, std::string_view authority, + std::string_view path) { initialize_headers_once(); @@ -164,10 +164,10 @@ marshal_int_value(Marshal marshal) } } // namespace -TEST_CASE("LogAccess pre-transaction CONNECT fields", "[LogAccess]") +TEST_CASE("LogAccess non-HttpSM CONNECT fields", "[LogAccess]") { - PreTransactionLogData data; - populate_pre_transaction_data(data, "CONNECT", ""sv, "example.com:443", ""sv); + NonHttpSmLogData data; + populate_non_http_sm_data(data, "CONNECT", ""sv, "example.com:443", ""sv); TransactionLogData log_data(data); LogAccess access(log_data); @@ -187,8 +187,8 @@ TEST_CASE("LogAccess pre-transaction CONNECT fields", "[LogAccess]") TEST_CASE("LogAccess malformed CONNECT without authority falls back to path", "[LogAccess]") { - PreTransactionLogData data; - populate_pre_transaction_data(data, "CONNECT", "https"sv, ""sv, "/"sv); + NonHttpSmLogData data; + populate_non_http_sm_data(data, "CONNECT", "https"sv, ""sv, "/"sv); TransactionLogData log_data(data); LogAccess access(log_data); @@ -201,10 +201,10 @@ TEST_CASE("LogAccess malformed CONNECT without authority falls back to path", "[ CHECK(marshal_int_value([&](char *buf) { return access.marshal_transfer_time_ms(buf); }) == 5); } -TEST_CASE("LogAccess pre-transaction client host port is null-safe", "[LogAccess]") +TEST_CASE("LogAccess non-HttpSM client host port is null-safe", "[LogAccess]") { - PreTransactionLogData data; - populate_pre_transaction_data(data, "GET", "https"sv, "example.com", "/client-port"sv); + NonHttpSmLogData data; + populate_non_http_sm_data(data, "GET", "https"sv, "example.com", "/client-port"sv); TransactionLogData log_data(data); LogAccess access(log_data);