Ext_proc: changing the append default value to be true and protect it with a runtime #39066

yanjunxiang-google · 2025-04-10T20:33:30Z

This is the 1st step to fix: #36982.
i.e, support default append to be "true".

Once it is in, then a follow up PR will be created to support append_action.

with a runtime Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

repokitteh-read-only · 2025-04-10T20:33:41Z

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @abeyad
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #39066 was opened by yanjunxiang-google.

see: more, trace.

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google · 2025-04-10T21:13:17Z

/assign @yanavlasov @adisuissa @stevenzzzz

yanjunxiang-google · 2025-04-10T21:14:03Z

api/envoy/config/core/v3/base.proto

  // :ref:`append_action <envoy_v3_api_field_config.core.v3.HeaderValueOption.append_action>` as replacement.
-  //
-  // .. note::
-  //   The :ref:`external authorization service <envoy_v3_api_msg_service.auth.v3.CheckResponse>` and


Checking ext_authz code, it had moved to take default append as true. So, this comments is stale.

yanjunxiang-google · 2025-04-10T21:20:27Z

This PR is the 1st step to support append_action in ext_proc filter. Once this PR is in, under the default append true case(runtime flag protected), we can add below logic to support append_action with backward compatibility:

If append.has_value()
take append.value()
else
take append_action value

If default append is false, like what's in ext_proc right now, then there is no way to support append_action at same time maintain backward compatibility. This is due to default append = false is equal to append_action = OVERWRITE_IF_EXISTS_OR_ADD, however default append_action is APPEND_IF_EXISTS_OR_ADD. Thus default append false will create backward compatibility problem if neither append nor append_action is set----- with legacy code, it is OVERWRITE_IF_EXISTS_OR_ADD, with the above proposal it is APPEND_IF_EXISTS_OR_ADD

BTW, ext_proc filter is the only one in Envoy which take default append to be false. ext_authz already changed to default to be true based on this code:

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

Line 44 in 4544ca4

} else {

abeyad

/lgtm api

agrawroh · 2025-04-10T21:59:04Z

@yanjunxiang-google Is there a merit in re-opening #37139 once we merge this in?

stevenzzzz · 2025-04-10T23:42:04Z

Thanks for working on this feature!
how long would we keep the runtime flag before we deprecate the old field?

stevenzzzz

Thanks

stevenzzzz · 2025-04-10T23:30:06Z

source/extensions/filters/http/ext_proc/mutation_utils.cc

    const LowerCaseString header_name(sh.header().key());
-    const bool append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, false);
+    bool append;
+    if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.ext_proc_append_default_true")) {


move this as the default?

Keep the runtime flag as false to keep the legacy behavior for a while.

This will buy sometime for ext_proc servers to change to always explicitly encode "append" = "false" or "true", instead of leaving it un-encoded for "false" case.

Changed the behavior to have default append value as "true". This will be a breaking change.
Keep default append value to false will avoid breaking change now. But, it will be a breaking change when remove that runtime flag.

adisuissa · 2025-04-11T15:33:39Z

source/common/runtime/runtime_features.cc


 FALSE_RUNTIME_GUARD(envoy_reloadable_features_ext_proc_graceful_grpc_close);
-
+FALSE_RUNTIME_GUARD(envoy_reloadable_features_ext_proc_append_default_true);


Can you remind me the reason for this being the default value?
Runtime flags should generally be true by default. The reason to make something false by default is introducing a feature that should be gradually tested in a prod environment, and allows specific instances to enable the feature.

There are two goals:

Keep the legacy behavior for now.

When we later on remove this runtime flag, the if () case is kept, the else case is removed:

if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.ext_proc_append_default_true")) {
append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, true);
} else {
append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, false);
}

I can change the runtime to
RUNTIME_GUARD(envoy_reloadable_features_ext_proc_append_default_false);

Then the logic will be:

if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.ext_proc_append_default_false")) { append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, false); } else { append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, true); }

Then we need to add a comments that when the runtime flag is removed, the else logic need to be kept. I can make this change if it is better.

Yeah, using a negated feature sounds better to me.
Otherwise, if the flag is "false" by default, the broken users will only notice after it is deprecated and removed.
I propose using a different name for the runtime-flag, something like: ext_proc_modified_append_default_value, that is set to true by default.

Then we need to add a comments that when the runtime flag is removed, the else logic need to be kept. I can make this change if it is better.

Not sure I fully grok the intention here.
Say you have:

append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, Runtime::runtimeFeatureEnabled("envoy.reloadable_features.ext_proc_modified_append_default_value"));

And add a comment above it stating that when the runtime flag is removed, the argument should be replaced with true.

Sure, we can do that.

So, this will break those ext_proc users who did not explicitly encode append=false. I guess they can toggle the runtime flag in Envoy to false avoid the issue. At same time start make changes in the ext_proc server code.

RE default values of WKTs (taken from the API guide):

Note that changes to default values for wrapped types, e.g. google.protobuf.UInt32Value are not governed by the above policy. Any management server requiring stability across Envoy API or implementations within a major version should set explicit values for these fields.

So the change is allowed. The use of a runtime-flag in this case is actually making it nicer for users to move to the newer version.

Good to know. In that case, maybe we can remove the runtime-guard a little faster, like two months after this PR merged, if nobody reporting any issues?

I'd suggest giving the normal six-months if you want to gather more certainty.
The motivation behind the 6 months time frame was 2 version upgrades, as many users upgrade when a new release is cut.

yanjunxiang-google · 2025-04-11T16:53:38Z

@yanjunxiang-google Is there a merit in re-opening #37139 once we merge this in?

Thanks for working on #37139.

Once #39066 is landed, the plan is to having a logic like :

If append.has_value()
take append.value()
else
take append_action value

and implementing it in a common place, so the logic can be shared by every place which need to decode "append" and "append_action":

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

Line 36 in a640fa6

if (header.has_append()) {

envoy/source/common/router/header_parser.cc

Line 58 in a640fa6

if (header_value_option.has_append()) {

yanjunxiang-google · 2025-04-11T16:55:22Z

Thanks for working on this feature! how long would we keep the runtime flag before we deprecate the old field?

Normally 6 months to give enough time for ext_proc servers to migrate to explicitly encode "append" with "false" or "true".

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

…d_def_true Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

adisuissa

Small nits, but otherwise LGTM, thanks!

adisuissa · 2025-04-14T13:03:59Z

source/common/runtime/runtime_features.cc

 FALSE_RUNTIME_GUARD(envoy_reloadable_features_quic_disable_client_early_data);

 FALSE_RUNTIME_GUARD(envoy_reloadable_features_ext_proc_graceful_grpc_close);
-


nit: add empty line back to keep the next block visually separated.

yanavlasov · 2025-04-14T13:19:11Z

/wait

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

adisuissa

Thanks, added one minor request for a test.
I think this PR should be merged after the next Envoy release (probably tomorrow).

adisuissa · 2025-04-14T18:05:44Z

test/extensions/filters/http/ext_proc/mutation_utils_test.cc

  s = mutation.add_set_headers();
  s->mutable_header()->set_key(":status");
  s->mutable_header()->set_raw_value("418");
-  // Default of "append" is "false" and mutations


Please add/modify a test that shows the default behavior now is "true".

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

…d_def_true Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google · 2025-04-14T21:56:07Z

Thanks, added one minor request for a test. I think this PR should be merged after the next Envoy release (probably tomorrow).

Let me convert the PR into draft for now to avoid it being accidentally merged.

…nd error logs Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google · 2025-04-15T20:46:10Z

source/common/runtime/runtime_features.cc

 FALSE_RUNTIME_GUARD(envoy_reloadable_features_quic_disable_client_early_data);

 FALSE_RUNTIME_GUARD(envoy_reloadable_features_ext_proc_graceful_grpc_close);
+FALSE_RUNTIME_GUARD(envoy_reloadable_features_ext_proc_modified_append_default_value);


Changing the runtime default value to false based on an offline discussion about keeping the legacy behavior for now to avoid risk. This enables us to collect the data about how many ext_proc servers out there are not explicitly encoding "append".

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google · 2025-04-16T17:38:09Z

Kind ping!

agrawroh

Looks good. Thanks!

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google · 2025-04-17T17:01:56Z

There is an offline comments that the added stats for data collection purpose won't be easily removed. Converting the PR to draft for now.

github-actions · 2025-06-08T16:01:31Z

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 · 2025-06-15T20:01:24Z

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!

Ext_proc: changing the append default value to be true and protect it

a5cb948

with a runtime Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google requested review from mattklein123, tyxia and yanavlasov as code owners April 10, 2025 20:33

repokitteh-read-only bot added the api label Apr 10, 2025

repokitteh-read-only bot assigned abeyad Apr 10, 2025

yanjunxiang-google changed the title ~~Ext_proc: changing the append default value to be true and protect it~~ Ext_proc: changing the append default value to be true and protect it with a runtime Apr 10, 2025

yanjunxiang-google marked this pull request as draft April 10, 2025 20:36

change runtime guard naming

ff54a5b

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google marked this pull request as ready for review April 10, 2025 21:12

repokitteh-read-only bot assigned adisuissa, stevenzzzz and yanavlasov Apr 10, 2025

yanjunxiang-google commented Apr 10, 2025

View reviewed changes

abeyad previously approved these changes Apr 10, 2025

View reviewed changes

repokitteh-read-only bot removed the api label Apr 10, 2025

stevenzzzz approved these changes Apr 10, 2025

View reviewed changes

adisuissa reviewed Apr 11, 2025

View reviewed changes

yanjunxiang-google added 2 commits April 11, 2025 17:37

fix format error

928d3fe

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

Merge branch 'main' of https://github.com/envoyproxy/envoy into appen…

1c08bfb

…d_def_true Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google dismissed abeyad’s stale review via 1c08bfb April 11, 2025 17:42

yanjunxiang-google added 2 commits April 11, 2025 21:11

change behavior to have append default to be true

65e85bf

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

fix format

6e282a9

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

repokitteh-read-only bot added the waiting label Apr 14, 2025

fix typo

39edfdb

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

repokitteh-read-only bot removed the waiting label Apr 14, 2025

adisuissa reviewed Apr 14, 2025

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 14, 2025

fix tests

b802028

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

repokitteh-read-only bot removed the waiting label Apr 14, 2025

addessing comments

c9229ad

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

adisuissa reviewed Apr 14, 2025

View reviewed changes

yanjunxiang-google added 2 commits April 14, 2025 21:20

addressing comments

b2ed87d

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

Merge branch 'main' of https://github.com/envoyproxy/envoy into appen…

92fa007

…d_def_true Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google marked this pull request as draft April 14, 2025 21:55

yanjunxiang-google added 2 commits April 15, 2025 20:38

set the runtime to false to keep the legacy behavior. Adding stats a…

fa3a293

…nd error logs Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

fix format

2290dc5

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google commented Apr 15, 2025

View reviewed changes

yanjunxiang-google marked this pull request as ready for review April 15, 2025 20:49

merge upstream main

78f12c5

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

agrawroh previously approved these changes Apr 16, 2025

View reviewed changes

yanjunxiang-google added 2 commits April 17, 2025 14:25

adding back API doc as default append is false

17e1c46

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

fix format

6be6f99

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google marked this pull request as draft April 17, 2025 17:02

yanjunxiang-google dismissed agrawroh’s stale review via 6be6f99 May 9, 2025 15:17

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 8, 2025

github-actions bot closed this Jun 15, 2025


		FALSE_RUNTIME_GUARD(envoy_reloadable_features_ext_proc_graceful_grpc_close);

		FALSE_RUNTIME_GUARD(envoy_reloadable_features_ext_proc_append_default_true);

Ext_proc: changing the append default value to be true and protect it with a runtime #39066

Ext_proc: changing the append default value to be true and protect it with a runtime #39066

Uh oh!

Conversation

yanjunxiang-google commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Apr 10, 2025

Uh oh!

yanjunxiang-google commented Apr 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanjunxiang-google commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abeyad left a comment

Choose a reason for hiding this comment

Uh oh!

agrawroh commented Apr 10, 2025

Uh oh!

stevenzzzz commented Apr 10, 2025

Uh oh!

stevenzzzz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanjunxiang-google commented Apr 11, 2025

Uh oh!

yanjunxiang-google commented Apr 11, 2025

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanavlasov commented Apr 14, 2025

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanjunxiang-google commented Apr 14, 2025

Uh oh!

yanjunxiang-google Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanjunxiang-google commented Apr 16, 2025

Uh oh!

agrawroh left a comment

Choose a reason for hiding this comment

Uh oh!

yanjunxiang-google commented Apr 17, 2025

Uh oh!

github-actions bot commented Jun 8, 2025

yanjunxiang-google commented Apr 10, 2025 •

edited

Loading

yanjunxiang-google commented Apr 10, 2025 •

edited

Loading

yanjunxiang-google Apr 15, 2025 •

edited

Loading