Skip to content

Commit

Permalink
Change AccessLog::Filter::evaluate() to take HeaderMap* instead of He…
Browse files Browse the repository at this point in the history 
…aderMap&

Signed-off-by: Auni Ahsan <[email protected]>
  • Loading branch information
@auni53
auni53 committed Jun 24, 2019
1 parent dd5bb0d commit b24e25b
Show file tree
Hide file tree
Showing 4 changed files with 85 additions and 65 deletions.
8 changes: 4 additions & 4 deletions include/envoy/access_log/access_log.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ class Filter {
* Evaluate whether an access log should be written based on request and response data.
* @return TRUE if the log should be written.
*/
virtual bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap& request_headers,
const Http::HeaderMap& response_headers,
const Http::HeaderMap& response_trailers) PURE;
virtual bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap* request_headers,
const Http::HeaderMap* response_headers,
const Http::HeaderMap* response_trailers) PURE;
};

using FilterPtr = std::unique_ptr<Filter>;
Expand All @@ -86,7 +86,7 @@ class Instance {
const Http::HeaderMap* response_trailers,
const StreamInfo::StreamInfo& stream_info) {
if (filter_ &&
!filter_->evaluate(stream_info, *request_headers, *response_headers, *response_trailers)) {
!filter_->evaluate(stream_info, request_headers, response_headers, response_trailers)) {
return;
}
return log(request_headers, response_headers, response_trailers, stream_info);
Expand Down
76 changes: 48 additions & 28 deletions source/common/access_log/access_log_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,24 +83,28 @@ FilterFactory::fromProto(const envoy::config::filter::accesslog::v2::AccessLogFi
}

bool TraceableRequestFilter::evaluate(const StreamInfo::StreamInfo& info,
const Http::HeaderMap& request_headers,
const Http::HeaderMap&, const Http::HeaderMap&) {
Tracing::Decision decision = Tracing::HttpTracerUtility::isTracing(info, request_headers);
const Http::HeaderMap* request_headers,
const Http::HeaderMap*, const Http::HeaderMap*) {
static Http::HeaderMapImpl empty_headers;
if (!request_headers) {
request_headers = &empty_headers;
}
Tracing::Decision decision = Tracing::HttpTracerUtility::isTracing(info, *request_headers);

return decision.traced && decision.reason == Tracing::Reason::ServiceForced;
}

bool StatusCodeFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap&,
const Http::HeaderMap&, const Http::HeaderMap&) {
bool StatusCodeFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap*,
const Http::HeaderMap*, const Http::HeaderMap*) {
if (!info.responseCode()) {
return compareAgainstValue(0ULL);
}

return compareAgainstValue(info.responseCode().value());
}

bool DurationFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap&,
const Http::HeaderMap&, const Http::HeaderMap&) {
bool DurationFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap*,
const Http::HeaderMap*, const Http::HeaderMap*) {
absl::optional<std::chrono::nanoseconds> final = info.requestComplete();
ASSERT(final);

Expand All @@ -114,9 +118,13 @@ RuntimeFilter::RuntimeFilter(const envoy::config::filter::accesslog::v2::Runtime
percent_(config.percent_sampled()),
use_independent_randomness_(config.use_independent_randomness()) {}

bool RuntimeFilter::evaluate(const StreamInfo::StreamInfo&, const Http::HeaderMap& request_header,
const Http::HeaderMap&, const Http::HeaderMap&) {
const Http::HeaderEntry* uuid = request_header.RequestId();
bool RuntimeFilter::evaluate(const StreamInfo::StreamInfo&, const Http::HeaderMap* request_headers,
const Http::HeaderMap*, const Http::HeaderMap*) {
static Http::HeaderMapImpl empty_headers;
if (!request_headers) {
request_headers = &empty_headers;
}
const Http::HeaderEntry* uuid = request_headers->RequestId();
uint64_t random_value;
// TODO(dnoe): Migrate uuidModBy to take string_view (#6580)
if (use_independent_randomness_ || uuid == nullptr ||
Expand Down Expand Up @@ -147,9 +155,9 @@ AndFilter::AndFilter(const envoy::config::filter::accesslog::v2::AndFilter& conf
Runtime::Loader& runtime, Runtime::RandomGenerator& random)
: OperatorFilter(config.filters(), runtime, random) {}

bool OrFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap& request_headers,
const Http::HeaderMap& response_headers,
const Http::HeaderMap& response_trailers) {
bool OrFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap* request_headers,
const Http::HeaderMap* response_headers,
const Http::HeaderMap* response_trailers) {
bool result = false;
for (auto& filter : filters_) {
result |= filter->evaluate(info, request_headers, response_headers, response_trailers);
Expand All @@ -162,9 +170,9 @@ bool OrFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMa
return result;
}

bool AndFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap& request_headers,
const Http::HeaderMap& response_headers,
const Http::HeaderMap& response_trailers) {
bool AndFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap* request_headers,
const Http::HeaderMap* response_headers,
const Http::HeaderMap* response_trailers) {
bool result = true;
for (auto& filter : filters_) {
result &= filter->evaluate(info, request_headers, response_headers, response_trailers);
Expand All @@ -177,18 +185,21 @@ bool AndFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderM
return result;
}

bool NotHealthCheckFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap&,
const Http::HeaderMap&, const Http::HeaderMap&) {
bool NotHealthCheckFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap*,
const Http::HeaderMap*, const Http::HeaderMap*) {
return !info.healthCheck();
}

HeaderFilter::HeaderFilter(const envoy::config::filter::accesslog::v2::HeaderFilter& config) {
header_data_.push_back(Http::HeaderUtility::HeaderData(config.header()));
}

bool HeaderFilter::evaluate(const StreamInfo::StreamInfo&, const Http::HeaderMap& request_headers,
const Http::HeaderMap&, const Http::HeaderMap&) {
return Http::HeaderUtility::matchHeaders(request_headers, header_data_);
bool HeaderFilter::evaluate(const StreamInfo::StreamInfo&, const Http::HeaderMap* request_headers,
const Http::HeaderMap*, const Http::HeaderMap*) {
if (!request_headers) {
return false;
}
return Http::HeaderUtility::matchHeaders(*request_headers, header_data_);
}

ResponseFlagFilter::ResponseFlagFilter(
Expand All @@ -202,8 +213,8 @@ ResponseFlagFilter::ResponseFlagFilter(
}
}

bool ResponseFlagFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap&,
const Http::HeaderMap&, const Http::HeaderMap&) {
bool ResponseFlagFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap*,
const Http::HeaderMap*, const Http::HeaderMap*) {
if (configured_flags_ != 0) {
return info.intersectResponseFlags(configured_flags_);
}
Expand All @@ -219,9 +230,16 @@ GrpcStatusFilter::GrpcStatusFilter(
exclude_ = config.exclude();
}

bool GrpcStatusFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap&,
const Http::HeaderMap& response_headers,
const Http::HeaderMap& response_trailers) {
bool GrpcStatusFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap*,
const Http::HeaderMap* response_headers,
const Http::HeaderMap* response_trailers) {
static Http::HeaderMapImpl empty_headers;
if (!response_headers) {
response_headers = &empty_headers;
}
if (!response_trailers) {
response_trailers = &empty_headers;
}
// The gRPC specification does not guarantee a gRPC status code will be returned from a gRPC
// request. When it is returned, it will be in the response trailers. With that said, Envoy will
// treat a trailers-only response as a headers-only response, so we have to check the following
Expand All @@ -231,9 +249,11 @@ bool GrpcStatusFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::
// 3. Inferred from info HTTP status, if it exists.
//
// If none of those options exist, it will default to Grpc::Status::GrpcStatus::Unknown.
// absl::optional<Grpc::Status::GrpcStatus>

const std::array<absl::optional<Grpc::Status::GrpcStatus>, 3> optional_statuses = {{
{Grpc::Common::getGrpcStatus(response_trailers)},
{Grpc::Common::getGrpcStatus(response_headers)},
{Grpc::Common::getGrpcStatus(*response_trailers)},
{Grpc::Common::getGrpcStatus(*response_headers)},
{info.responseCode() ? absl::optional<Grpc::Status::GrpcStatus>(
Grpc::Utility::httpToGrpcStatus(info.responseCode().value()))
: absl::nullopt},
Expand Down
60 changes: 30 additions & 30 deletions source/common/access_log/access_log_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,9 @@ class StatusCodeFilter : public ComparisonFilter {
: ComparisonFilter(config.comparison(), runtime) {}

// AccessLog::Filter
bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap& request_headers,
const Http::HeaderMap& response_headers,
const Http::HeaderMap& response_trailers) override;
bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap* request_headers,
const Http::HeaderMap* response_headers,
const Http::HeaderMap* response_trailers) override;
};

/**
Expand All @@ -70,9 +70,9 @@ class DurationFilter : public ComparisonFilter {
: ComparisonFilter(config.comparison(), runtime) {}

// AccessLog::Filter
bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap& request_headers,
const Http::HeaderMap& response_headers,
const Http::HeaderMap& response_trailers) override;
bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap* request_headers,
const Http::HeaderMap* response_headers,
const Http::HeaderMap* response_trailers) override;
};

/**
Expand All @@ -97,9 +97,9 @@ class AndFilter : public OperatorFilter {
Runtime::RandomGenerator& random);

// AccessLog::Filter
bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap& request_headers,
const Http::HeaderMap& response_headers,
const Http::HeaderMap& response_trailers) override;
bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap* request_headers,
const Http::HeaderMap* response_headers,
const Http::HeaderMap* response_trailers) override;
};

/**
Expand All @@ -111,9 +111,9 @@ class OrFilter : public OperatorFilter {
Runtime::RandomGenerator& random);

// AccessLog::Filter
bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap& request_headers,
const Http::HeaderMap& response_headers,
const Http::HeaderMap& response_trailers) override;
bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap* request_headers,
const Http::HeaderMap* response_headers,
const Http::HeaderMap* response_trailers) override;
};

/**
Expand All @@ -124,9 +124,9 @@ class NotHealthCheckFilter : public Filter {
NotHealthCheckFilter() {}

// AccessLog::Filter
bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap& request_headers,
const Http::HeaderMap& response_headers,
const Http::HeaderMap& response_trailers) override;
bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap* request_headers,
const Http::HeaderMap* response_headers,
const Http::HeaderMap* response_trailers) override;
};

/**
Expand All @@ -135,9 +135,9 @@ class NotHealthCheckFilter : public Filter {
class TraceableRequestFilter : public Filter {
public:
// AccessLog::Filter
bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap& request_headers,
const Http::HeaderMap& response_headers,
const Http::HeaderMap& response_trailers) override;
bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap* request_headers,
const Http::HeaderMap* response_headers,
const Http::HeaderMap* response_trailers) override;
};

/**
Expand All @@ -149,9 +149,9 @@ class RuntimeFilter : public Filter {
Runtime::Loader& runtime, Runtime::RandomGenerator& random);

// AccessLog::Filter
bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap& request_headers,
const Http::HeaderMap& response_headers,
const Http::HeaderMap& response_trailers) override;
bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap* request_headers,
const Http::HeaderMap* response_headers,
const Http::HeaderMap* response_trailers) override;

private:
Runtime::Loader& runtime_;
Expand All @@ -169,9 +169,9 @@ class HeaderFilter : public Filter {
HeaderFilter(const envoy::config::filter::accesslog::v2::HeaderFilter& config);

// AccessLog::Filter
bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap& request_headers,
const Http::HeaderMap& response_headers,
const Http::HeaderMap& response_trailers) override;
bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap* request_headers,
const Http::HeaderMap* response_headers,
const Http::HeaderMap* response_trailers) override;

private:
std::vector<Http::HeaderUtility::HeaderData> header_data_;
Expand All @@ -185,9 +185,9 @@ class ResponseFlagFilter : public Filter {
ResponseFlagFilter(const envoy::config::filter::accesslog::v2::ResponseFlagFilter& config);

// AccessLog::Filter
bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap& request_headers,
const Http::HeaderMap& response_headers,
const Http::HeaderMap& response_trailers) override;
bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap* request_headers,
const Http::HeaderMap* response_headers,
const Http::HeaderMap* response_trailers) override;

private:
uint64_t configured_flags_{};
Expand All @@ -206,9 +206,9 @@ class GrpcStatusFilter : public Filter {
GrpcStatusFilter(const envoy::config::filter::accesslog::v2::GrpcStatusFilter& config);

// AccessLog::Filter
bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap& request_headers,
const Http::HeaderMap& response_headers,
const Http::HeaderMap& response_trailers) override;
bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap* request_headers,
const Http::HeaderMap* response_headers,
const Http::HeaderMap* response_trailers) override;

private:
GrpcStatusHashSet statuses_;
Expand Down
6 changes: 3 additions & 3 deletions test/mocks/access_log/mocks.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ class MockFilter : public Filter {

// AccessLog::Filter
MOCK_METHOD4(evaluate,
bool(const StreamInfo::StreamInfo& info, const Http::HeaderMap& request_headers,
const Http::HeaderMap& response_headers,
const Http::HeaderMap& response_trailers));
bool(const StreamInfo::StreamInfo& info, const Http::HeaderMap* request_headers,
const Http::HeaderMap* response_headers,
const Http::HeaderMap* response_trailers));
};

class MockAccessLogManager : public AccessLogManager {
Expand Down

0 comments on commit b24e25b

Please sign in to comment.