ext_proc: support HeaderAppendAction in mutation utils#37139
ext_proc: support HeaderAppendAction in mutation utils#37139agrawroh wants to merge 1 commit intoenvoyproxy:mainfrom
Conversation
agrawroh
requested review from
mattklein123,
tyxia and
yanavlasov
as code owners
agrawroh
force-pushed
the
ext_proc_ap
branch
from
fd4ccf6 to
b93e853
Compare
|
/assign @tyxia |
agrawroh
force-pushed
the
ext_proc_ap
branch
from
b93e853 to
376f420
Compare
|
/assign @yanjunxiang-google |
|
gentle ping @yanjunxiang-google /wait-any |
agrawroh
force-pushed
the
ext_proc_ap
branch
from
032e865 to
3614623
Compare
|
/retest |
Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
agrawroh
force-pushed
the
ext_proc_ap
branch
from
3614623 to
61c3066
Compare
|
/retest |
|
Haven't had chance to look into this PR yet. FYI: there is another PR trying this before: #29862. |
| should_append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, false); | ||
| check_op = (should_append && header_exists) ? CheckOperation::APPEND : CheckOperation::SET; | ||
| } else { | ||
| if (sh.has_append()) { |
There was a problem hiding this comment.
Thanks for working on this issue. If we want to support both append and append_action, can we first define the behavior. I have a few questions:
- If both append and append_action is left as default, which config to take?
- If both append and append_action are set to non-default value, what's the behavior and which config to take? Should we deem it as invalid config? Or just take the one with higher preference? Which one is preferred?
- If one is default and the other is set, this is an easy case. I think we are ignoring the default configuration, and take the one with non-default setting.
- The design should be backward compatible.
Let's add comments here to clearly document it.
There was a problem hiding this comment.
Thanks for reviewing this! Let me address your questions in order:
- We have implemented a check to prevent users from specifying both
appendandappend_action. Additionally, a behavior change notice is included to inform users that if neither option is explicitly set, the default behavior ofappend_actionwill apply, which is to append the header instead of replacing it. - Allowing both options to be set simultaneously is not permitted. If users attempt to do so, the system will throw an
InvalidArgumentError. - That’s correct! If one option is set and the other remains default, the non-default setting takes precedence. However, we would issue a deprecation warning, as
appendis being marked as deprecated. - While we aim to maintain compatibility, the design cannot be fully backward compatible. This is because there’s no way to distinguish in the proto whether
appendwas left unspecified or explicitly set tofalse. As a result, a minor behavior change is necessary. This is also noted as part of the changelog.
There was a problem hiding this comment.
Agreed with below:
- If both are set to non-default values, throw an error.
- If one is default, the other is none-default, taking the none-default value.
However, I have questions in the case both are default:
i) With the existing behavior, which only honor config "append", it will take append = false, and replace the existing headers.
ii) With your proposal, it will take append_action = APPEND_IF_EXISTS_OR_ADD, which will append.
This is not backward-compatible. Needs some more analysis on it's impact.
Another thought is if both are default, can you just take append = false, which will be backward-compatible. Then you will be able to support append-action at same time maintain backward compatibility. If in the future, there is a real need to honor append-action when both are default, you can raise a breaking-change PR then. WDYT?
There was a problem hiding this comment.
@yanjunxiang-google How should I distinguish that append_action is explicitly set to APPEND_IF_EXISTS_OR_ADD or is not present? There is no has_append_action() for enums.
There was a problem hiding this comment.
That's a good question. Let me loop in @adisuissa to see whether he has some ideas on this.
There was a problem hiding this comment.
@agrawroh Here are my previous replies and opinion about breaking change and non-breaking change
breaking change (i.e. no has function for enum): #29862 (comment)
non-breaking change idea: #29862 (comment)
I have not looked at this specific code area for a quite a while, but i just want to share them for your reference
| should_append = sh.append().value(); | ||
| check_op = (should_append && header_exists) ? CheckOperation::APPEND : CheckOperation::SET; | ||
| } else { | ||
| switch (sh.append_action()) { |
There was a problem hiding this comment.
Instead of doing this inline this function, please add two helper functions to make it easy to read, like parseAppendConfig(), parseAppendActionConfig().
There was a problem hiding this comment.
Sure, I can do that. Before I start making changes, are you okay with the proposal here?
|
/wait-any |
|
/wait-any |
|
The best practice of proto enum is having UNSPECIFIED field as default 0 to distinguish UNSET. This will resolve the difficulties of introducing HeaderAppendAction in this PR. But I am not sure if it is too late to follow this practice at this stage. I have shared some thoughts before. |
|
Assigning to Tianyu for now though it might not be reviewable yet. /wait |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
github-actions
Bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
7 participants
Description
Previously, ext_proc only supported the deprecated
appendfield inHeaderValueOptionfor header mutations, ignoring the newerappend_actionfield. This meant users could only choose between replacing a header value or appending to it, without access to more granular controls like ADD_IF_ABSENT or OVERWRITE_IF_EXISTS.In this PR, we are adding support for all
HeaderAppendActionvalues in ext_proc header mutations. Default behavior would be changed to append when no action is specified. This behavior could be temporarily reverted with runtime guardenvoy.reloadable_features.ext_proc_legacy_append. We maintain the backward compatibility with the legacyappendfield.The following actions are now supported:
Fixes #36982
Commit Message: ext_proc: support HeaderAppendAction in mutation utils
Additional Description: Add support for
HeaderAppendActionin the ext_proc header mutations.Risk Level: Low
Testing: Added unit tests covering all append_action behaviors and interaction with legacy append
Docs Changes: Added behavior change notice for the default append behavior
Release Note: Added release note about header mutation behavior change