Skip to content

handle append_action for grpc check response #39369

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+67 −33
Draft

handle append_action for grpc check response #39369

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
4 changes: 4 additions & 0 deletions changelogs/current.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ minor_behavior_changes:
change: |
Allow getaddrinfo to be configured to run by a thread pool, controlled by :ref:`num_resolver_threads
<envoy_v3_api_field_extensions.network.dns_resolver.getaddrinfo.v3.GetAddrInfoDnsResolverConfig.num_resolver_threads>`.
- area: ext_authz
change: |
Handle ``append_action`` from :ref:`external authorization service <envoy_v3_api_msg_service.auth.v3.CheckResponse>`
that was ignored for request headers to upstream.

bug_fixes:
# *Changes expected to improve the state of the world and are unlikely to have negative effects*
Expand Down
9 changes: 6 additions & 3 deletions source/extensions/filters/common/ext_authz/ext_authz.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,12 @@ using UnsafeHeaderVector = std::vector<UnsafeHeader>;
struct Response {
// Call status.
CheckStatus status;
// A set of HTTP headers returned by the authorization server, that will be optionally appended
// to the request to the upstream server.
UnsafeHeaderVector headers_to_append{};
// A set of HTTP headers returned by the authorization server, will be optionally set
// (using "addCopy") to the request to the upstream server.
UnsafeHeaderVector headers_to_add_if_absent{};
// A set of HTTP headers returned by the authorization server, will be optionally set (using
// "setCopy") to the request to the upstream server.
UnsafeHeaderVector headers_to_overwrite_if_exists{};
// A set of HTTP headers returned by the authorization server, will be optionally set
// (using "setCopy") to the request to the upstream server.
UnsafeHeaderVector headers_to_set{};
Expand Down
26 changes: 23 additions & 3 deletions source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,30 @@ void copyHeaderFieldIntoResponse(
ResponsePtr& response,
const Protobuf::RepeatedPtrField<envoy::config::core::v3::HeaderValueOption>& headers) {
for (const auto& header : headers) {
if (header.append().value()) {
response->headers_to_append.emplace_back(header.header().key(), header.header().value());
if (header.has_append()) {
if (header.append().value()) {
response->headers_to_add.emplace_back(header.header().key(), header.header().value());
} else {
response->headers_to_set.emplace_back(header.header().key(), header.header().value());
}
} else {
response->headers_to_set.emplace_back(header.header().key(), header.header().value());
switch (header.append_action()) {
PANIC_ON_PROTO_ENUM_SENTINEL_VALUES;
case Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD:
response->headers_to_add.emplace_back(header.header().key(), header.header().value());
break;
case Router::HeaderValueOption::ADD_IF_ABSENT:
response->headers_to_add_if_absent.emplace_back(header.header().key(),
header.header().value());
break;
case Router::HeaderValueOption::OVERWRITE_IF_EXISTS:
response->headers_to_overwrite_if_exists.emplace_back(header.header().key(),
header.header().value());
break;
case Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD:
response->headers_to_set.emplace_back(header.header().key(), header.header().value());
break;
}
}
}
}
Expand Down
6 changes: 6 additions & 0 deletions source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ const Response& errorResponse() {
UnsafeHeaderVector{},
UnsafeHeaderVector{},
UnsafeHeaderVector{},
UnsafeHeaderVector{},
UnsafeHeaderVector{},
{{}},
Http::Utility::QueryParamsVector{},
{},
Expand Down Expand Up @@ -361,6 +363,8 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) {
UnsafeHeaderVector{},
UnsafeHeaderVector{},
UnsafeHeaderVector{},
UnsafeHeaderVector{},
UnsafeHeaderVector{},
std::move(headers_to_remove),
Http::Utility::QueryParamsVector{},
{},
Expand All @@ -384,6 +388,8 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) {
UnsafeHeaderVector{},
UnsafeHeaderVector{},
UnsafeHeaderVector{},
UnsafeHeaderVector{},
UnsafeHeaderVector{},
{{}},
Http::Utility::QueryParamsVector{},
{},
Expand Down
55 changes: 28 additions & 27 deletions source/extensions/filters/http/ext_authz/ext_authz.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,7 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) {
// routed. If we are changing the headers we also need to clear the route
// cache.
if (config_->clearRouteCache() &&
(!response->headers_to_set.empty() || !response->headers_to_append.empty() ||
(!response->headers_to_set.empty() || !response->headers_to_add.empty() ||
!response->headers_to_remove.empty() || !response->query_parameters_to_set.empty() ||
!response->query_parameters_to_remove.empty())) {
ENVOY_STREAM_LOG(debug, "ext_authz is clearing route cache", *decoder_callbacks_);
Expand All @@ -516,6 +516,25 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) {

ENVOY_STREAM_LOG(trace,
"ext_authz filter added header(s) to the request:", *decoder_callbacks_);
for (const auto& [key, value] : response->headers_to_add) {
CheckResult check_result = validateAndCheckDecoderHeaderMutation(
Filters::Common::MutationRules::CheckOperation::SET, key, value);
switch (check_result) {
case CheckResult::OK:
ENVOY_STREAM_LOG(trace, "'{}':'{}'", *decoder_callbacks_, key, value);
request_headers_->addCopy(Http::LowerCaseString(key), value);
break;
case CheckResult::IGNORE:
ENVOY_STREAM_LOG(trace, "Ignoring invalid header to add '{}':'{}'.", *decoder_callbacks_,
key, value);
break;
case CheckResult::FAIL:
ENVOY_STREAM_LOG(trace, "Rejecting invalid header to add '{}':'{}'.", *decoder_callbacks_,
key, value);
rejectResponse();
return;
}
}
for (const auto& [key, value] : response->headers_to_set) {
CheckResult check_result = validateAndCheckDecoderHeaderMutation(
Filters::Common::MutationRules::CheckOperation::SET, key, value);
Expand All @@ -535,7 +554,7 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) {
return;
}
}
for (const auto& [key, value] : response->headers_to_add) {
for (const auto& [key, value] : response->headers_to_add_if_absent) {
CheckResult check_result = validateAndCheckDecoderHeaderMutation(
Filters::Common::MutationRules::CheckOperation::SET, key, value);
switch (check_result) {
Expand All @@ -554,39 +573,21 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) {
return;
}
}
for (const auto& [key, value] : response->headers_to_append) {
for (const auto& [key, value] : response->headers_to_overwrite_if_exists) {
CheckResult check_result = validateAndCheckDecoderHeaderMutation(
Filters::Common::MutationRules::CheckOperation::APPEND, key, value);
Filters::Common::MutationRules::CheckOperation::SET, key, value);
switch (check_result) {
case CheckResult::OK: {
case CheckResult::OK:
ENVOY_STREAM_LOG(trace, "'{}':'{}'", *decoder_callbacks_, key, value);
Http::LowerCaseString lowercase_key(key);
const auto header_to_modify = request_headers_->get(lowercase_key);
// TODO(dio): Add a flag to allow appending non-existent headers, without setting it
// first (via `headers_to_add`). For example, given:
// 1. Original headers {"original": "true"}
// 2. Response headers from the authorization servers {{"append": "1"}, {"append":
// "2"}}
//
// Currently it is not possible to add {{"append": "1"}, {"append": "2"}} (the
// intended combined headers: {{"original": "true"}, {"append": "1"}, {"append":
// "2"}}) to the request to upstream server by only sets `headers_to_append`.
if (!header_to_modify.empty()) {
ENVOY_STREAM_LOG(trace, "'{}':'{}'", *decoder_callbacks_, key, value);
// The current behavior of appending is by combining entries with the same key,
// into one entry. The value of that combined entry is separated by ",".
// TODO(dio): Consider to use addCopy instead.
request_headers_->appendCopy(lowercase_key, value);
}
request_headers_->setCopy(Http::LowerCaseString(key), value);
break;
}
case CheckResult::IGNORE:
ENVOY_STREAM_LOG(trace, "Ignoring invalid header to append '{}':'{}'.", *decoder_callbacks_,
ENVOY_STREAM_LOG(trace, "Ignoring invalid header to add '{}':'{}'.", *decoder_callbacks_,
key, value);
break;
case CheckResult::FAIL:
ENVOY_STREAM_LOG(trace, "Rejecting invalid header to append '{}':'{}'.",
*decoder_callbacks_, key, value);
ENVOY_STREAM_LOG(trace, "Rejecting invalid header to add '{}':'{}'.", *decoder_callbacks_,
key, value);
rejectResponse();
return;
}
Expand Down