diff --git a/source/common/grpc/BUILD b/source/common/grpc/BUILD index 29f31e66d444..e49c1c1db016 100644 --- a/source/common/grpc/BUILD +++ b/source/common/grpc/BUILD @@ -94,6 +94,7 @@ envoy_cc_library( "//source/common/common:macros", "//source/common/common:utility_lib", "//source/common/grpc:status_lib", + "//source/common/http:header_utility_lib", "//source/common/http:headers_lib", "//source/common/http:message_lib", "//source/common/http:utility_lib", diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc index 2019f54ac874..3b74a5223b96 100644 --- a/source/common/grpc/common.cc +++ b/source/common/grpc/common.cc @@ -14,6 +14,7 @@ #include "common/common/fmt.h" #include "common/common/macros.h" #include "common/common/utility.h" +#include "common/http/header_utility.h" #include "common/http/headers.h" #include "common/http/message_impl.h" #include "common/http/utility.h" @@ -37,7 +38,14 @@ bool Common::hasGrpcContentType(const Http::RequestOrResponseHeaderMap& headers) .getStringView()[Http::Headers::get().ContentTypeValues.Grpc.size()] == '+'); } -bool Common::isGrpcResponseHeader(const Http::ResponseHeaderMap& headers, bool end_stream) { +bool Common::isGrpcRequestHeaders(const Http::RequestHeaderMap& headers) { + if (!headers.Path()) { + return false; + } + return hasGrpcContentType(headers); +} + +bool Common::isGrpcResponseHeaders(const Http::ResponseHeaderMap& headers, bool end_stream) { if (end_stream) { // Trailers-only response, only grpc-status is required. return headers.GrpcStatus() != nullptr; diff --git a/source/common/grpc/common.h b/source/common/grpc/common.h index b450e7817e54..cd94fe450568 100644 --- a/source/common/grpc/common.h +++ b/source/common/grpc/common.h @@ -36,12 +36,20 @@ class Common { */ static bool hasGrpcContentType(const Http::RequestOrResponseHeaderMap& headers); + /** + * @param headers the headers to parse. + * @return bool indicating whether the header is a gRPC request header. + * Currently headers are considered gRPC request headers if they have the gRPC + * content type, and have a path header. + */ + static bool isGrpcRequestHeaders(const Http::RequestHeaderMap& headers); + /** * @param headers the headers to parse. * @param bool indicating whether the header is at end_stream. * @return bool indicating whether the header is a gRPC response header */ - static bool isGrpcResponseHeader(const Http::ResponseHeaderMap& headers, bool end_stream); + static bool isGrpcResponseHeaders(const Http::ResponseHeaderMap& headers, bool end_stream); /** * Returns the GrpcStatus code from a given set of trailers, if present. diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index cc5659da7885..b2e40ab397f6 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -133,7 +133,7 @@ void AsyncStreamImpl::sendHeaders(RequestHeaderMap& headers, bool end_stream) { is_head_request_ = true; } - is_grpc_request_ = Grpc::Common::hasGrpcContentType(headers); + is_grpc_request_ = Grpc::Common::isGrpcRequestHeaders(headers); headers.setReferenceEnvoyInternalRequest(Headers::get().EnvoyInternalRequestValues.True); if (send_xff_) { Utility::appendXff(headers, *parent_.config_.local_info_.address()); diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index d6e4a55eced7..4501cbe7fd41 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -671,7 +671,7 @@ void ConnectionManagerImpl::ActiveStream::onIdleTimeout() { } else { stream_info_.setResponseFlag(StreamInfo::ResponseFlag::StreamIdleTimeout); sendLocalReply(request_headers_ != nullptr && - Grpc::Common::hasGrpcContentType(*request_headers_), + Grpc::Common::isGrpcRequestHeaders(*request_headers_), Http::Code::RequestTimeout, "stream timeout", nullptr, state_.is_head_request_, absl::nullopt, StreamInfo::ResponseCodeDetails::get().StreamIdleTimeout); } @@ -679,7 +679,8 @@ void ConnectionManagerImpl::ActiveStream::onIdleTimeout() { void ConnectionManagerImpl::ActiveStream::onRequestTimeout() { connection_manager_.stats_.named_.downstream_rq_timeout_.inc(); - sendLocalReply(request_headers_ != nullptr && Grpc::Common::hasGrpcContentType(*request_headers_), + sendLocalReply(request_headers_ != nullptr && + Grpc::Common::isGrpcRequestHeaders(*request_headers_), Http::Code::RequestTimeout, "request timeout", nullptr, state_.is_head_request_, absl::nullopt, StreamInfo::ResponseCodeDetails::get().RequestOverallTimeout); } @@ -799,7 +800,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he // overload it is more important to avoid unnecessary allocation than to create the filters. state_.created_filter_chain_ = true; connection_manager_.stats_.named_.downstream_rq_overload_close_.inc(); - sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), + sendLocalReply(Grpc::Common::isGrpcRequestHeaders(*request_headers_), Http::Code::ServiceUnavailable, "envoy overloaded", nullptr, state_.is_head_request_, absl::nullopt, StreamInfo::ResponseCodeDetails::get().Overload); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 10bcc7522bd6..1e3705b57634 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -313,7 +313,7 @@ class ConnectionManagerImpl : Logger::Loggable, // so that we can issue gRPC local responses to gRPC requests. Filter's decodeHeaders() // called here may change the content type, so we must check it before the call. FilterHeadersStatus decodeHeaders(RequestHeaderMap& headers, bool end_stream) { - is_grpc_request_ = Grpc::Common::hasGrpcContentType(headers); + is_grpc_request_ = Grpc::Common::isGrpcRequestHeaders(headers); FilterHeadersStatus status = handle_->decodeHeaders(headers, end_stream); if (end_stream) { handle_->decodeComplete(); diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index b4a97bfa8b05..417195e1adcb 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -404,7 +404,9 @@ void ConnectionManagerUtility::mutateResponseHeaders( bool ConnectionManagerUtility::maybeNormalizePath(RequestHeaderMap& request_headers, const ConnectionManagerConfig& config) { - ASSERT(request_headers.Path()); + if (!request_headers.Path()) { + return true; // It's as valid as it is going to get. + } bool is_valid_path = true; if (config.shouldNormalizePath()) { is_valid_path = PathUtil::canonicalPath(request_headers); diff --git a/source/common/http/conn_manager_utility.h b/source/common/http/conn_manager_utility.h index 20381116162f..6bac45578eb9 100644 --- a/source/common/http/conn_manager_utility.h +++ b/source/common/http/conn_manager_utility.h @@ -63,9 +63,10 @@ class ConnectionManagerUtility { const RequestIDExtensionSharedPtr& rid_extension, const std::string& via); - // Sanitize the path in the header map if forced by config. + // Sanitize the path in the header map if the path exists and it is forced by config. // Side affect: the string view of Path header is invalidated. // Return false if error happens during the sanitization. + // Returns true if there is no path. static bool maybeNormalizePath(RequestHeaderMap& request_headers, const ConnectionManagerConfig& config); diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc index cb930929e4a0..a9a905d44340 100644 --- a/source/common/http/path_utility.cc +++ b/source/common/http/path_utility.cc @@ -29,6 +29,7 @@ absl::optional canonicalizePath(absl::string_view original_path) { /* static */ bool PathUtil::canonicalPath(RequestHeaderMap& headers) { + ASSERT(headers.Path()); const auto original_path = headers.Path()->value().getStringView(); // canonicalPath is supposed to apply on path component in URL instead of :path header const auto query_pos = original_path.find('?'); @@ -54,6 +55,7 @@ bool PathUtil::canonicalPath(RequestHeaderMap& headers) { } void PathUtil::mergeSlashes(RequestHeaderMap& headers) { + ASSERT(headers.Path()); const auto original_path = headers.Path()->value().getStringView(); // Only operate on path component in URL. const absl::string_view::size_type query_start = original_path.find('?'); diff --git a/source/common/http/path_utility.h b/source/common/http/path_utility.h index 8df1581bad6f..62be43e2e03f 100644 --- a/source/common/http/path_utility.h +++ b/source/common/http/path_utility.h @@ -14,8 +14,10 @@ class PathUtil { public: // Returns if the normalization succeeds. // If it is successful, the path header in header path will be updated with the normalized path. + // Requires the Path header be present. static bool canonicalPath(RequestHeaderMap& headers); // Merges two or more adjacent slashes in path part of URI into one. + // Requires the Path header be present. static void mergeSlashes(RequestHeaderMap& headers); // Removes the query and/or fragment string (if present) from the input path. // For example, this function returns "/data" for the input path "/data#fragment?param=value". diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 801cc0e00ad4..0287a5b6c974 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -59,6 +59,10 @@ convertInternalRedirectAction(const envoy::config::route::v3::RouteAction& route const std::string DEPRECATED_ROUTER_NAME = "envoy.router"; +const absl::string_view getPath(const Http::RequestHeaderMap& headers) { + return headers.Path() ? headers.Path()->value().getStringView() : ""; +} + } // namespace std::string SslRedirector::newPath(const Http::RequestHeaderMap& headers) const { @@ -430,12 +434,6 @@ bool RouteEntryImplBase::matchRoute(const Http::RequestHeaderMap& headers, uint64_t random_value) const { bool matches = true; - // TODO(mattklein123): Currently all match types require a path header. When we support CONNECT - // we will need to figure out how to safely relax this. - if (headers.Path() == nullptr) { - return false; - } - matches &= evaluateRuntimeMatch(random_value); if (!matches) { // No need to waste further cycles calculating a route match. @@ -443,13 +441,12 @@ bool RouteEntryImplBase::matchRoute(const Http::RequestHeaderMap& headers, } if (match_grpc_) { - matches &= Grpc::Common::hasGrpcContentType(headers); + matches &= Grpc::Common::isGrpcRequestHeaders(headers); } matches &= Http::HeaderUtility::matchHeaders(headers, config_headers_); if (!config_query_parameters_.empty()) { - Http::Utility::QueryParams query_parameters = - Http::Utility::parseQueryString(headers.Path()->value().getStringView()); + Http::Utility::QueryParams query_parameters = Http::Utility::parseQueryString(getPath(headers)); matches &= ConfigUtility::matchQueryParams(query_parameters, config_query_parameters_); } @@ -540,7 +537,7 @@ void RouteEntryImplBase::finalizePathHeader(Http::RequestHeaderMap& headers, return; } - std::string path(headers.Path()->value().getStringView()); + std::string path(getPath(headers)); if (insert_envoy_original_path) { headers.setEnvoyOriginalPath(path); } @@ -633,8 +630,7 @@ std::string RouteEntryImplBase::newPath(const Http::RequestHeaderMap& headers) c if (!path_redirect_.empty()) { final_path = path_redirect_.c_str(); } else { - ASSERT(headers.Path()); - final_path = headers.Path()->value().getStringView(); + final_path = getPath(headers); if (strip_query_) { size_t path_end = final_path.find("?"); if (path_end != absl::string_view::npos) { @@ -852,7 +848,7 @@ RouteConstSharedPtr PrefixRouteEntryImpl::matches(const Http::RequestHeaderMap& const StreamInfo::StreamInfo& stream_info, uint64_t random_value) const { if (RouteEntryImplBase::matchRoute(headers, stream_info, random_value) && - path_matcher_->match(headers.Path()->value().getStringView())) { + path_matcher_->match(getPath(headers))) { return clusterEntry(headers, random_value); } return nullptr; @@ -874,7 +870,7 @@ RouteConstSharedPtr PathRouteEntryImpl::matches(const Http::RequestHeaderMap& he const StreamInfo::StreamInfo& stream_info, uint64_t random_value) const { if (RouteEntryImplBase::matchRoute(headers, stream_info, random_value) && - path_matcher_->match(headers.Path()->value().getStringView())) { + path_matcher_->match(getPath(headers))) { return clusterEntry(headers, random_value); } @@ -902,8 +898,7 @@ RegexRouteEntryImpl::RegexRouteEntryImpl( void RegexRouteEntryImpl::rewritePathHeader(Http::RequestHeaderMap& headers, bool insert_envoy_original_path) const { - const absl::string_view path = - Http::PathUtil::removeQueryAndFragment(headers.Path()->value().getStringView()); + const absl::string_view path = Http::PathUtil::removeQueryAndFragment(getPath(headers)); // TODO(yuval-k): This ASSERT can happen if the path was changed by a filter without clearing the // route cache. We should consider if ASSERT-ing is the desired behavior in this case. ASSERT(regex_->match(path)); @@ -914,8 +909,7 @@ RouteConstSharedPtr RegexRouteEntryImpl::matches(const Http::RequestHeaderMap& h const StreamInfo::StreamInfo& stream_info, uint64_t random_value) const { if (RouteEntryImplBase::matchRoute(headers, stream_info, random_value)) { - const absl::string_view path = - Http::PathUtil::removeQueryAndFragment(headers.Path()->value().getStringView()); + const absl::string_view path = Http::PathUtil::removeQueryAndFragment(getPath(headers)); if (regex_->match(path)) { return clusterEntry(headers, random_value); } @@ -1124,6 +1118,10 @@ RouteConstSharedPtr VirtualHostImpl::getRouteFromEntries(const Http::RequestHead // Check for a route that matches the request. for (const RouteEntryImplBaseConstSharedPtr& route : routes_) { + if (!headers.Path()) { + // TODO(alyssawilk) allow specifically for kConnectMatcher routes. + return nullptr; + } RouteConstSharedPtr route_entry = route->matches(headers, stream_info, random_value); if (nullptr != route_entry) { return route_entry; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index de4278259e65..b788a691474d 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -68,6 +68,9 @@ bool convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& downstream if (internal_redirect.value().getStringView().length() == 0) { return false; } + if (!downstream_headers.Path()) { + return false; + } Http::Utility::Url absolute_url; if (!absolute_url.initialize(internal_redirect.value().getStringView(), false)) { @@ -112,6 +115,10 @@ bool convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& downstream constexpr uint64_t TimeoutPrecisionFactor = 100; +const absl::string_view getPath(const Http::RequestHeaderMap& headers) { + return headers.Path() ? headers.Path()->value().getStringView() : ""; +} + } // namespace // Express percentage as [0, TimeoutPrecisionFactor] because stats do not accept floating point @@ -379,7 +386,6 @@ void Filter::chargeUpstreamCode(Http::Code code, Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, bool end_stream) { // Do a common header check. We make sure that all outgoing requests have all HTTP/2 headers. // These get stripped by HTTP/1 codec where applicable. - ASSERT(headers.Path()); ASSERT(headers.Method()); ASSERT(headers.Host()); @@ -395,7 +401,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, : nullptr; // TODO: Maybe add a filter API for this. - grpc_request_ = Grpc::Common::hasGrpcContentType(headers); + grpc_request_ = Grpc::Common::isGrpcRequestHeaders(headers); // Only increment rq total stat if we actually decode headers here. This does not count requests // that get handled by earlier filters. @@ -410,8 +416,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, route_ = callbacks_->route(); if (!route_) { config_.stats_.no_route_.inc(); - ENVOY_STREAM_LOG(debug, "no cluster match for URL '{}'", *callbacks_, - headers.Path()->value().getStringView()); + ENVOY_STREAM_LOG(debug, "no cluster match for URL '{}'", *callbacks_, getPath(headers)); callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::NoRouteFound); callbacks_->sendLocalReply(Http::Code::NotFound, "", modify_headers, absl::nullopt, @@ -428,7 +433,10 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, direct_response->responseCode(), direct_response->responseBody(), [this, direct_response, &request_headers = headers](Http::ResponseHeaderMap& response_headers) -> void { - const auto new_path = direct_response->newPath(request_headers); + std::string new_path; + if (request_headers.Path()) { + new_path = direct_response->newPath(request_headers); + } // See https://tools.ietf.org/html/rfc7231#section-7.1.2. const auto add_location = direct_response->responseCode() == Http::Code::Created || @@ -473,7 +481,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, // Set up stat prefixes, etc. request_vcluster_ = route_entry_->virtualCluster(headers); ENVOY_STREAM_LOG(debug, "cluster '{}' match for URL '{}'", *callbacks_, - route_entry_->clusterName(), headers.Path()->value().getStringView()); + route_entry_->clusterName(), getPath(headers)); if (config_.strict_check_headers_ != nullptr) { for (const auto& header : *config_.strict_check_headers_) { diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index f3f568666f60..3933b100ad23 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -36,6 +36,9 @@ static std::string valueOrDefault(const Http::HeaderEntry* header, const char* d static std::string buildUrl(const Http::RequestHeaderMap& request_headers, const uint32_t max_path_length) { + if (!request_headers.Path()) { + return ""; + } std::string path(request_headers.EnvoyOriginalPath() ? request_headers.EnvoyOriginalPath()->value().getStringView() : request_headers.Path()->value().getStringView()); @@ -184,7 +187,7 @@ void HttpTracerUtility::finalizeDownstreamSpan(Span& span, std::string(request_headers->ClientTraceId()->value().getStringView())); } - if (Grpc::Common::hasGrpcContentType(*request_headers)) { + if (Grpc::Common::isGrpcRequestHeaders(*request_headers)) { addGrpcRequestTags(span, *request_headers); } } diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc index 5ca42738d464..16e78394248a 100644 --- a/source/common/upstream/health_checker_impl.cc +++ b/source/common/upstream/health_checker_impl.cc @@ -583,8 +583,8 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::decodeHeaders( end_stream); return; } - if (!Grpc::Common::hasGrpcContentType(*headers)) { - onRpcComplete(Grpc::Status::WellKnownGrpcStatus::Internal, "invalid gRPC content-type", false); + if (!Grpc::Common::isGrpcResponseHeaders(*headers, end_stream)) { + onRpcComplete(Grpc::Status::WellKnownGrpcStatus::Internal, "not a gRPC request", false); return; } if (end_stream) { diff --git a/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc b/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc index f10ca5c0d0b0..89e39663e3f0 100644 --- a/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc +++ b/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc @@ -298,8 +298,11 @@ void Filter::jsonizeRequest(Http::RequestHeaderMap const& headers, const Buffer: &json_req); // Wrap the Query String - for (auto&& kv_pair : Http::Utility::parseQueryString(headers.Path()->value().getStringView())) { - json_req.mutable_query_string_parameters()->insert({kv_pair.first, kv_pair.second}); + if (headers.Path()) { + for (auto&& kv_pair : + Http::Utility::parseQueryString(headers.Path()->value().getStringView())) { + json_req.mutable_query_string_parameters()->insert({kv_pair.first, kv_pair.second}); + } } // Wrap the body diff --git a/source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc b/source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc index 4dc8b216ffe9..ae5332d3504a 100644 --- a/source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc +++ b/source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc @@ -24,7 +24,7 @@ void Http1BridgeFilter::chargeStat(const Http::ResponseHeaderOrTrailerMap& heade } Http::FilterHeadersStatus Http1BridgeFilter::decodeHeaders(Http::RequestHeaderMap& headers, bool) { - const bool grpc_request = Grpc::Common::hasGrpcContentType(headers); + const bool grpc_request = Grpc::Common::isGrpcRequestHeaders(headers); if (grpc_request) { setupStatTracking(headers); } diff --git a/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc b/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc index d5191716ce59..d585f84ea0c2 100644 --- a/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc +++ b/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc @@ -84,7 +84,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, // If this is a gRPC request we: // - mark this request as being gRPC // - change the content-type to application/x-protobuf - if (Envoy::Grpc::Common::hasGrpcContentType(headers)) { + if (Envoy::Grpc::Common::isGrpcRequestHeaders(headers)) { enabled_ = true; // We keep track of the original content-type to ensure that we handle diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc index 1e0b67aa654f..1434a3a97f1e 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc @@ -259,7 +259,7 @@ ProtobufUtil::Status JsonTranscoderConfig::createTranscoder( const Http::RequestHeaderMap& headers, ZeroCopyInputStream& request_input, google::grpc::transcoding::TranscoderInputStream& response_input, std::unique_ptr& transcoder, MethodInfoSharedPtr& method_info) { - if (Grpc::Common::hasGrpcContentType(headers)) { + if (Grpc::Common::isGrpcRequestHeaders(headers)) { return ProtobufUtil::Status(Code::INVALID_ARGUMENT, "Request headers has application/grpc content-type"); } @@ -476,7 +476,7 @@ void JsonTranscoderFilter::setDecoderFilterCallbacks( Http::FilterHeadersStatus JsonTranscoderFilter::encodeHeaders(Http::ResponseHeaderMap& headers, bool end_stream) { - if (!Grpc::Common::isGrpcResponseHeader(headers, end_stream)) { + if (!Grpc::Common::isGrpcResponseHeaders(headers, end_stream)) { error_ = true; } diff --git a/source/extensions/filters/http/grpc_stats/grpc_stats_filter.cc b/source/extensions/filters/http/grpc_stats/grpc_stats_filter.cc index 7c91093d4712..8dfbc0dc0dc4 100644 --- a/source/extensions/filters/http/grpc_stats/grpc_stats_filter.cc +++ b/source/extensions/filters/http/grpc_stats/grpc_stats_filter.cc @@ -147,7 +147,7 @@ class GrpcStatsFilter : public Http::PassThroughFilter { GrpcStatsFilter(ConfigConstSharedPtr config) : config_(config) {} Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers, bool) override { - grpc_request_ = Grpc::Common::hasGrpcContentType(headers); + grpc_request_ = Grpc::Common::isGrpcRequestHeaders(headers); if (grpc_request_) { cluster_ = decoder_callbacks_->clusterInfo(); if (cluster_) { @@ -203,7 +203,7 @@ class GrpcStatsFilter : public Http::PassThroughFilter { Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap& headers, bool end_stream) override { - grpc_response_ = Grpc::Common::isGrpcResponseHeader(headers, end_stream); + grpc_response_ = Grpc::Common::isGrpcResponseHeaders(headers, end_stream); if (doStatTracking()) { config_->context_.chargeStat(*cluster_, Grpc::Context::Protocol::Grpc, request_names_, headers.GrpcStatus()); diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 7ead4cd30d55..b85e05c32962 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -39,6 +39,9 @@ const absl::flat_hash_set& GrpcWebFilter::gRpcWebContentTypes() con } bool GrpcWebFilter::isGrpcWebRequest(const Http::RequestHeaderMap& headers) { + if (!headers.Path()) { + return false; + } const Http::HeaderEntry* content_type = headers.ContentType(); if (content_type != nullptr) { return gRpcWebContentTypes().count(content_type->value().getStringView()) > 0; diff --git a/source/extensions/tracers/xray/xray_tracer_impl.cc b/source/extensions/tracers/xray/xray_tracer_impl.cc index d0ecb0684e25..50637152ead9 100644 --- a/source/extensions/tracers/xray/xray_tracer_impl.cc +++ b/source/extensions/tracers/xray/xray_tracer_impl.cc @@ -95,9 +95,11 @@ Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, } if (!should_trace.has_value()) { + const absl::string_view path = + request_headers.Path() ? request_headers.Path()->value().getStringView() : ""; const SamplingRequest request{std::string{request_headers.Host()->value().getStringView()}, std::string{request_headers.Method()->value().getStringView()}, - std::string{request_headers.Path()->value().getStringView()}}; + std::string{path}}; should_trace = sampling_strategy_->shouldTrace(request); } diff --git a/source/server/http/admin_filter.cc b/source/server/http/admin_filter.cc index 7f9cf3930974..b0565f673c5e 100644 --- a/source/server/http/admin_filter.cc +++ b/source/server/http/admin_filter.cc @@ -62,7 +62,8 @@ const Http::RequestHeaderMap& AdminFilter::getRequestHeaders() const { } void AdminFilter::onComplete() { - absl::string_view path = request_headers_->Path()->value().getStringView(); + const absl::string_view path = + request_headers_->Path() ? request_headers_->Path()->value().getStringView() : ""; ENVOY_STREAM_LOG(debug, "request complete: path: {}", *decoder_callbacks_, path); Buffer::OwnedImpl response; diff --git a/test/common/grpc/common_test.cc b/test/common/grpc/common_test.cc index 02f948b9c601..d96d07b8eacc 100644 --- a/test/common/grpc/common_test.cc +++ b/test/common/grpc/common_test.cc @@ -285,20 +285,29 @@ TEST(GrpcContextTest, HasGrpcContentType) { EXPECT_FALSE(isGrpcContentType("application/grpc-web+foo")); } +TEST(GrpcContextTest, IsGrpcRequestHeader) { + Http::TestRequestHeaderMapImpl is{ + {":method", "GET"}, {":path", "/"}, {"content-type", "application/grpc"}}; + EXPECT_TRUE(Common::isGrpcRequestHeaders(is)); + Http::TestRequestHeaderMapImpl is_not{{":method", "CONNECT"}, + {"content-type", "application/grpc"}}; + EXPECT_FALSE(Common::isGrpcRequestHeaders(is_not)); +} + TEST(GrpcContextTest, IsGrpcResponseHeader) { Http::TestResponseHeaderMapImpl grpc_status_only{{":status", "500"}, {"grpc-status", "14"}}; - EXPECT_TRUE(Common::isGrpcResponseHeader(grpc_status_only, true)); - EXPECT_FALSE(Common::isGrpcResponseHeader(grpc_status_only, false)); + EXPECT_TRUE(Common::isGrpcResponseHeaders(grpc_status_only, true)); + EXPECT_FALSE(Common::isGrpcResponseHeaders(grpc_status_only, false)); Http::TestResponseHeaderMapImpl grpc_response_header{{":status", "200"}, {"content-type", "application/grpc"}}; - EXPECT_FALSE(Common::isGrpcResponseHeader(grpc_response_header, true)); - EXPECT_TRUE(Common::isGrpcResponseHeader(grpc_response_header, false)); + EXPECT_FALSE(Common::isGrpcResponseHeaders(grpc_response_header, true)); + EXPECT_TRUE(Common::isGrpcResponseHeaders(grpc_response_header, false)); Http::TestResponseHeaderMapImpl json_response_header{{":status", "200"}, {"content-type", "application/json"}}; - EXPECT_FALSE(Common::isGrpcResponseHeader(json_response_header, true)); - EXPECT_FALSE(Common::isGrpcResponseHeader(json_response_header, false)); + EXPECT_FALSE(Common::isGrpcResponseHeaders(json_response_header, true)); + EXPECT_FALSE(Common::isGrpcResponseHeaders(json_response_header, false)); } TEST(GrpcContextTest, ValidateResponse) { diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 71e7b5a40933..9d730478ec9f 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -1409,6 +1409,16 @@ TEST_F(ConnectionManagerUtilityTest, RemovesProxyResponseHeaders) { EXPECT_FALSE(response_headers.has("proxy-connection")); } +// maybeNormalizePath() returns true with an empty path. +TEST_F(ConnectionManagerUtilityTest, SanitizeEmptyPath) { + ON_CALL(config_, shouldNormalizePath()).WillByDefault(Return(false)); + TestRequestHeaderMapImpl original_headers; + + TestRequestHeaderMapImpl header_map(original_headers); + EXPECT_TRUE(ConnectionManagerUtility::maybeNormalizePath(header_map, config_)); + EXPECT_EQ(original_headers, header_map); +} + // maybeNormalizePath() does nothing by default. TEST_F(ConnectionManagerUtilityTest, SanitizePathDefaultOff) { ON_CALL(config_, shouldNormalizePath()).WillByDefault(Return(false)); diff --git a/test/common/http/header_utility_test.cc b/test/common/http/header_utility_test.cc index 55d2056aab78..e8383aa08be1 100644 --- a/test/common/http/header_utility_test.cc +++ b/test/common/http/header_utility_test.cc @@ -469,6 +469,12 @@ TEST(HeaderIsValidTest, AuthorityIsValid) { EXPECT_FALSE(HeaderUtility::authorityIsValid("illegal{}")); } +TEST(HeaderIsValidTest, IsConnect) { + EXPECT_TRUE(HeaderUtility::isConnect(Http::TestRequestHeaderMapImpl{{":method", "CONNECT"}})); + EXPECT_FALSE(HeaderUtility::isConnect(Http::TestRequestHeaderMapImpl{{":method", "GET"}})); + EXPECT_FALSE(HeaderUtility::isConnect(Http::TestRequestHeaderMapImpl{})); +} + TEST(HeaderAddTest, HeaderAdd) { TestHeaderMapImpl headers{{"myheader1", "123value"}}; TestHeaderMapImpl headers_to_add{{"myheader2", "456value"}}; diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index fd61d51ff961..e282f6b9ae8c 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -87,6 +87,14 @@ class TestConfigImpl : public ConfigImpl { const envoy::config::route::v3::RouteConfiguration config_; }; +Http::TestRequestHeaderMapImpl genPathlessHeaders(const std::string& host, + const std::string& method) { + return Http::TestRequestHeaderMapImpl{{":authority", host}, {":method", method}, + {"x-safe", "safe"}, {"x-global-nope", "global"}, + {"x-vhost-nope", "vhost"}, {"x-route-nope", "route"}, + {"x-forwarded-proto", "http"}}; +} + Http::TestRequestHeaderMapImpl genHeaders(const std::string& host, const std::string& path, const std::string& method) { return Http::TestRequestHeaderMapImpl{{":authority", host}, {":path", path}, @@ -368,6 +376,7 @@ TEST_F(RouteMatcherTest, DEPRECATED_FEATURE_TEST(TestLegacyRoutes)) { config.route(genHeaders("bat2.com", "/foo", "GET"), 0)->routeEntry()->clusterName()); EXPECT_EQ("regex_default", config.route(genHeaders("bat2.com", " ", "GET"), 0)->routeEntry()->clusterName()); + EXPECT_TRUE(config.route(genPathlessHeaders("bat2.com", "GET"), 0) == nullptr); // Regular Expression matching with query string params EXPECT_EQ( diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index 3d05232cb56f..eaf7437d0bc4 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -158,6 +158,7 @@ TEST_F(HttpConnManFinalizerImplTest, OriginalAndLongPath) { Http::TestRequestHeaderMapImpl request_headers{{"x-request-id", "id"}, {"x-envoy-original-path", path}, {":method", "GET"}, + {":path", ""}, {"x-forwarded-proto", "http"}}; Http::TestResponseHeaderMapImpl response_headers; Http::TestResponseTrailerMapImpl response_trailers; @@ -189,8 +190,10 @@ TEST_F(HttpConnManFinalizerImplTest, NoGeneratedId) { const auto remote_address = Network::Address::InstanceConstSharedPtr{new Network::Address::Ipv4Instance(expected_ip, 0)}; - Http::TestRequestHeaderMapImpl request_headers{ - {"x-envoy-original-path", path}, {":method", "GET"}, {"x-forwarded-proto", "http"}}; + Http::TestRequestHeaderMapImpl request_headers{{":path", ""}, + {"x-envoy-original-path", path}, + {":method", "GET"}, + {"x-forwarded-proto", "http"}}; Http::TestResponseHeaderMapImpl response_headers; Http::TestResponseTrailerMapImpl response_trailers; @@ -213,6 +216,38 @@ TEST_F(HttpConnManFinalizerImplTest, NoGeneratedId) { &response_trailers, stream_info, config); } +TEST_F(HttpConnManFinalizerImplTest, Connect) { + const std::string path(300, 'a'); + const std::string path_prefix = "http://"; + const std::string expected_path(256, 'a'); + const std::string expected_ip = "10.0.0.100"; + const auto remote_address = + Network::Address::InstanceConstSharedPtr{new Network::Address::Ipv4Instance(expected_ip, 0)}; + + Http::TestRequestHeaderMapImpl request_headers{{":method", "CONNECT"}, + {"x-forwarded-proto", "http"}}; + Http::TestResponseHeaderMapImpl response_headers; + Http::TestResponseTrailerMapImpl response_trailers; + + absl::optional protocol = Http::Protocol::Http2; + EXPECT_CALL(stream_info, bytesReceived()).WillOnce(Return(10)); + EXPECT_CALL(stream_info, bytesSent()).WillOnce(Return(11)); + EXPECT_CALL(stream_info, protocol()).WillRepeatedly(ReturnPointee(&protocol)); + absl::optional response_code; + EXPECT_CALL(stream_info, responseCode()).WillRepeatedly(ReturnPointee(&response_code)); + EXPECT_CALL(stream_info, downstreamDirectRemoteAddress()) + .WillRepeatedly(ReturnPointee(&remote_address)); + + EXPECT_CALL(span, setTag(_, _)).Times(testing::AnyNumber()); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpUrl), Eq(""))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpMethod), Eq("CONNECT"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpProtocol), Eq("HTTP/2"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().PeerAddress), Eq(expected_ip))); + + HttpTracerUtility::finalizeDownstreamSpan(span, &request_headers, &response_headers, + &response_trailers, stream_info, config); +} + TEST_F(HttpConnManFinalizerImplTest, NullRequestHeadersAndNullRouteEntry) { EXPECT_CALL(stream_info, bytesReceived()).WillOnce(Return(10)); EXPECT_CALL(stream_info, bytesSent()).WillOnce(Return(11)); diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index 8828f47c62c1..817d84811061 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -114,6 +114,7 @@ class GrpcWebFilterTest : public testing::TestWithParam