Skip to content

Adding API doc change to specify ext_authz/ext_proc ignores keep_empty_value API #45353

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

Open
yanjunxiang-google wants to merge 4 commits into
base: main
Choose a base branch
from
+15 −0
Open

Adding API doc change to specify ext_authz/ext_proc ignores keep_empty_value API #45353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dae7429
Adding API doc change to specify ext_authz/ext_proc ignores
yanjunxiang-google May 29, 2026
e2d1f63
Empty-Commit
yanjunxiang-google May 29, 2026
e36570f
adding doc change in ext_authz and ext_proc proto
yanjunxiang-google Jun 1, 2026
6ebb85a
fix doc error
yanjunxiang-google Jun 2, 2026
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
5 changes: 5 additions & 0 deletions api/envoy/config/core/v3/base.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,11 @@ message HeaderValueOption {

// Is the header value allowed to be empty? If false (default), custom headers with empty values are dropped,
// otherwise they are added.
//
// .. note::
Copy link
Copy Markdown
Member

@ggreenway ggreenway May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are similar comments in api/envoy/service/auth/v3/external_auth.proto for field append. Should this doc comment go in that file instead for consistency, and better locality to the problem usage?

Copy link
Copy Markdown
Contributor

@markdroth markdroth May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, this would be better to document this where this type is used (in the ext_authz and ext_proc protos) instead of here where the type is defined.

Stepping back a bit, it's also worth asking why we have different users of this type behaving differently to begin with. If there is a common proto for a commonly used type, shouldn't there also be a single implementation of reading and writing that type that can be used anywhere the type is used, so that we don't have inconsistent behavior to begin with? (This question doesn't need to block this PR, but it is something we should probably think about going forward.)

Copy link
Copy Markdown
Contributor Author

@yanjunxiang-google yanjunxiang-google Jun 1, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stepping back a bit, it's also worth asking why we have different users of this type behaving differently to begin with

The type is added afterwards. Initially, ext_authz and router filter have different behavior for empty string in header value, with ext_authz accept it, and router_filter skip it. This type is added to control the behavior for router_filter without considering the ext_authz/ext_proc default behaviors. As this type is added as a boolean, it is very challenging to have an implementation backward compatible to both ext_authz and router_filter. If it had been a google.protobuf.BoolValue, it will be easier.

Copy link
Copy Markdown
Contributor Author

@yanjunxiang-google yanjunxiang-google Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are similar comments in api/envoy/service/auth/v3/external_auth.proto for field append. Should this doc comment go in that file instead for consistency, and better locality to the problem usage?

I added some comments in ext_authz and ext_proc proto files. I would like to also add these comments here similar to line 474 - 477.

// The :ref:`external authorization service <envoy_v3_api_msg_service.auth.v3.CheckResponse>` and
// :ref:`external processor service <envoy_v3_api_msg_service.ext_proc.v3.ProcessingResponse>`
// are ignoring this field.
bool keep_empty_value = 4;
}

Expand Down
6 changes: 6 additions & 0 deletions api/envoy/service/auth/v3/external_auth.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ message DeniedHttpResponse {
// This field allows the authorization service to send HTTP response headers
// to the downstream client. Note that the :ref:`append field in HeaderValueOption <envoy_v3_api_field_config.core.v3.HeaderValueOption.append>` defaults to
// false when used in this message.
// The :ref:`keep_empty_value field in HeaderValueOption <envoy_v3_api_field_config.core.v3.HeaderValueOption.keep_empty_value>`
// is not supported.
repeated config.core.v3.HeaderValueOption headers = 2;

// This field allows the authorization service to send a response body data
Expand All @@ -72,6 +74,8 @@ message OkHttpResponse {
// the filter will append the correspondent header value to the matched request header.
// By leaving ``append`` as false, the filter will either add a new header, or override an existing
// one if there is a match.
// The :ref:`keep_empty_value field in HeaderValueOption <envoy_v3_api_field_config.core.v3.HeaderValueOption.keep_empty_value>`
// is not supported.
repeated config.core.v3.HeaderValueOption headers = 2;

// HTTP entity headers to remove from the original request before dispatching
Expand Down Expand Up @@ -101,6 +105,8 @@ message OkHttpResponse {
// This field allows the authorization service to send HTTP response headers
// to the downstream client on success. Note that the :ref:`append field in HeaderValueOption <envoy_v3_api_field_config.core.v3.HeaderValueOption.append>`
// defaults to false when used in this message.
// The :ref:`keep_empty_value field in HeaderValueOption <envoy_v3_api_field_config.core.v3.HeaderValueOption.keep_empty_value>`
// is not supported.
repeated config.core.v3.HeaderValueOption response_headers_to_add = 6;

// This field allows the authorization service to set (and overwrite) query
Expand Down
4 changes: 4 additions & 0 deletions api/envoy/service/ext_proc/v3/external_processor.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,10 @@ message HeaderMutation {
// ``:authority``, ``:scheme``, or ``host`` headers will be ignored.
// The header value is encoded in the
// :ref:`raw_value <envoy_v3_api_field_config.core.v3.HeaderValue.raw_value>` field.
// Note that the :ref:`append field in HeaderValueOption <envoy_v3_api_field_config.core.v3.HeaderValueOption.append>`
// defaults to false when used in this message.
// The :ref:`keep_empty_value field in HeaderValueOption <envoy_v3_api_field_config.core.v3.HeaderValueOption.keep_empty_value>`
// is not supported.
repeated config.core.v3.HeaderValueOption set_headers = 1;

// Remove these HTTP headers. Attempts to remove system headers --
Expand Down
Loading