Adding API doc change to specify ext_authz/ext_proc ignores keep_empty_value API#45353
Adding API doc change to specify ext_authz/ext_proc ignores keep_empty_value API#45353yanjunxiang-google wants to merge 4 commits into
Conversation
keep_empty_value Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
/retest |
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
|
/retest |
|
|
||
| // Is the header value allowed to be empty? If false (default), custom headers with empty values are dropped, | ||
| // otherwise they are added. | ||
| // .. note:: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
/wait |
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
|
/coverage |
|
Coverage for this Pull Request will be rendered here: https://storage.googleapis.com/envoy-cncf-pr/45353/coverage/index.html For comparison, current coverage on https://storage.googleapis.com/envoy-cncf-postsubmit/main/coverage/index.html The coverage results are (re-)rendered each time the CI |
3 participants
Addressing: #45003
Please check: #45003 (comment) for details of this change.