ext_authz: respect keep_empty_value in HeaderValueOption (envoyproxy#45103)

Author: 404SkillNotFound

changelogs/current/bug_fixes/ext_authz__keep-empty-value.rst

@@ -0,0 +1,4 @@
+Fixed a bug where ``keep_empty_value`` in ``HeaderValueOption`` was not respected by
+the ext_authz filter. Empty-valued headers are now dropped by default. This behavior
+can be reverted by setting the runtime guard
+``envoy.reloadable_features.ext_authz_respect_keep_empty_value`` to ``false``.

source/common/runtime/runtime_features.cc

@@ -50,6 +50,7 @@ RUNTIME_GUARD(envoy_reloadable_features_enable_new_dns_implementation);
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_enable_lrs_server_self_ads);
 RUNTIME_GUARD(envoy_reloadable_features_enable_new_query_param_present_match_behavior);
 RUNTIME_GUARD(envoy_reloadable_features_ext_authz_http_client_retries_respect_user_retry_on);
+RUNTIME_GUARD(envoy_reloadable_features_ext_authz_respect_keep_empty_value);
 // Ignore the automated "remove this flag" issue: we should keep this for 1 year. Confirm with
 // @yanjunxiang-google before removing.
 RUNTIME_GUARD(envoy_reloadable_features_ext_proc_fail_close_spurious_resp);

source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc

@@ -20,6 +20,11 @@ void copyHeaderFieldIntoResponse(
     ResponsePtr& response,
     const Protobuf::RepeatedPtrField<envoy::config::core::v3::HeaderValueOption>& headers) {
   for (const auto& header : headers) {
+    if (Runtime::runtimeFeatureEnabled(
+            "envoy.reloadable_features.ext_authz_respect_keep_empty_value") &&
+        header.header().value().empty() && !header.keep_empty_value()) {
+      continue;
+    }
     if (header.append().value()) {
       response->headers_to_append.emplace_back(header.header().key(), header.header().value());
     } else {
@@ -33,6 +38,11 @@ void copyOkResponseMutations(ResponsePtr& response,
   copyHeaderFieldIntoResponse(response, ok_response.headers());
 
   for (const auto& header : ok_response.response_headers_to_add()) {
+    if (Runtime::runtimeFeatureEnabled(
+            "envoy.reloadable_features.ext_authz_respect_keep_empty_value") &&
+        header.header().value().empty() && !header.keep_empty_value()) {
+      continue;
+    }
     if (header.has_append()) {
       if (header.append().value()) {
         response->response_headers_to_add.emplace_back(header.header().key(),

test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc

@@ -635,10 +635,8 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationOkUpstreamHeaderMutations) {
                      span_);
 }
 
-// TODO(https://github.com/envoyproxy/envoy/issues/45003)
-// This test ensures the default (buggy) behavior remains unchanged since
-// existing Envoy deployments may be relying on the default (buggy) behavior
-TEST_F(ExtAuthzGrpcClientTest, AuthorizationOkWithKeepEmptyValueIgnored) {
+// This test ensures keep_empty_value is respected in ext_authz responses.
+TEST_F(ExtAuthzGrpcClientTest, AuthorizationOkWithKeepEmptyValueRespected) {
   initialize();
 
   envoy::service::auth::v3::CheckResponse check_response;
@@ -654,12 +652,10 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationOkWithKeepEmptyValueIgnored) {
 )EOF",
                             check_response);
 
-  // If keep_empty_value were respected, the header "x-keep-empty" should be DROPPED
-  // because its value is empty. However, the current implementation ignores this
-  // flag and blindly adds the empty header to the headers_to_set vector.
+  // keep_empty_value is false and value is empty, so the header should be dropped.
   auto expected_authz_response = Response{
       .status = CheckStatus::OK,
-      .headers_to_set = UnsafeHeaderVector{{"x-keep-empty", ""}},
+      .headers_to_set = UnsafeHeaderVector{},
       .status_code = Http::Code::OK,
       .grpc_status = Grpc::Status::WellKnownGrpcStatus::Ok,
   };
@@ -672,8 +668,47 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationOkWithKeepEmptyValueIgnored) {
   client_->onCreateInitialMetadata(headers);
 
   EXPECT_CALL(span_, setTag(Eq("ext_authz_status"), Eq("ext_authz_ok")));
-  EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo<ResponsePtr&>(
-                                      AuthzOkResponse(expected_authz_response))));
+  EXPECT_CALL(request_callbacks_, onComplete_(testing::_))
+      .WillOnce(testing::Invoke([&expected_authz_response](ResponsePtr& response) {
+        EXPECT_TRUE(TestCommon::compareHeaderVector(response->headers_to_set,
+                                                    expected_authz_response.headers_to_set));
+      }));
+  client_->onSuccess(std::make_unique<envoy::service::auth::v3::CheckResponse>(check_response),
+                     span_);
+}
+
+TEST_F(ExtAuthzGrpcClientTest, AuthorizationOkWithKeepEmptyValueTrue) {
+  initialize();
+  envoy::service::auth::v3::CheckResponse check_response;
+  TestUtility::loadFromYaml(R"EOF(
+status:
+  code: 0
+ok_response:
+  headers:
+  - header:
+      key: x-keep-empty
+      value: ""
+    keep_empty_value: true
+)EOF",
+                            check_response);
+  // keep_empty_value is true and value is empty, so the header should be kept.
+  auto expected_authz_response = Response{
+      .status = CheckStatus::OK,
+      .headers_to_set = UnsafeHeaderVector{{"x-keep-empty", ""}},
+      .status_code = Http::Code::OK,
+      .grpc_status = Grpc::Status::WellKnownGrpcStatus::Ok,
+  };
+  envoy::service::auth::v3::CheckRequest request;
+  expectCallSend(request);
+  client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_);
+  Http::TestRequestHeaderMapImpl headers;
+  client_->onCreateInitialMetadata(headers);
+  EXPECT_CALL(span_, setTag(Eq("ext_authz_status"), Eq("ext_authz_ok")));
+  EXPECT_CALL(request_callbacks_, onComplete_(testing::_))
+      .WillOnce(testing::Invoke([&expected_authz_response](ResponsePtr& response) {
+        EXPECT_TRUE(TestCommon::compareHeaderVector(response->headers_to_set,
+                                                    expected_authz_response.headers_to_set));
+      }));
   client_->onSuccess(std::make_unique<envoy::service::auth::v3::CheckResponse>(check_response),
                      span_);
 }