ext_proc: Honor HeaderValueOption.append_action, keep append option as well for backward compatibility

[Ronnie-personal]

Commit Message:
Honor HeaderValueOption.append_action when append does not have a value.
Additional Description:
Modified mutation_utils to apply header changes based on HeaderValueOption.append_action.

[Optional Fixes #Issue] #29861

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #29862 was opened by Ronnie-personal.

see: more, trace.

[Ronnie-personal]

@gbrail @yanjunxiang-google
Could you please confirm the following code is not causing the header overwriting issue?

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

Line 68 in 7479b6c

new_header->set_value(MessageUtil::sanitizeUtf8String(e.value().getStringView()));

Initially I suspected that this line of code might be the root cause. However, after adding a test,

envoy/test/extensions/filters/http/ext_proc/filter_test.cc

Line 3207 in 16e39bd

TEST_F(HttpFilterTest, MultipleSetCookieHeaders) {

it passed successfully.

[yanjunxiang-google]

There is no issues in the ext_proc code at all as mentioned in : #29861 (comment)

You just need to set the header mutation "append" proto field in the ext_proc server header response to true to achieve your needs.

Also, the test you added in this PR is to verify Envoy ext_proc filter -> ext_proc server path. However your original issue is in the direction ext_proc server -> Envoy ext_proc filter path.

Please check below comments for more details:
#29861 (comment)

[Ronnie-personal]

/retest

[Ronnie-personal]

@mattklein123 @htuch

Can any maintainers help on the pipeline issue? The pipeline failed to execute with following error.

{"error":"invalid_grant","error_description":"Invalid JWT Signature."}, iss: azure-pipelines@envoy-ci.iam.gserviceaccount.com

[tyxia]

@mattklein123 @htuch

Can any maintainers help on the pipeline issue? The pipeline failed to execute with following error.

{"error":"invalid_grant","error_description":"Invalid JWT Signature."}, iss: azure-pipelines@envoy-ci.iam.gserviceaccount.com

#29947 Looks like there is auth issue.
cc @phlax

[phlax]

re ci failure we are following up - but there is not an imminent resolution unfortunately

[Ronnie-personal]

re ci failure we are following up - but there is not an imminent resolution unfortunately

@phlax Thanks for the prompt reply! Please let me know when I can restart the test.

[phlax]

/retest

[Ronnie-personal]

/retest

@phlax Thank you for unblocking the test, I'm able to continue with my work.

[repokitteh-read-only]

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #29862 was synchronize by Ronnie-personal.

see: more, trace.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #29862 was synchronize by Ronnie-personal.

see: more, trace.

[tyxia]

Thanks! Let's discuss about the goal of this PR first (as I asked in comment below) since i feel your initial issue with set_cookie should be resolved as long as you set HeaderValueOption.append in existing code.

[tyxia]

Let's revert this change in API proto. I didn't mean to change the API here (Sorry if my previous comment in the other thread misleads you)

Basically we don't need to impose additional restriction on it and it can just follow the general rule in header map.

[Ronnie-personal]

No problem at all, I will revert it back.

[tyxia]

Thanks @htuch !

@Ronnie-personal. As I mentioned above. we should just use addCopy to handle APPEND_IF_EXISTS_OR_ADD. ext_proc doesn't need to detect duplicate header manually as addCopy internally already handles all cases: duplicate inline and non-inline headers as well as set-cookie. This actually leads to a question for you: I am wondering what is the issue this PR aims to address? Using HeaderValueOption.append with existing code should resolve your set-cookie issue as it is using add_copy already. Or you are improving the code by handling different actions inside of HeaderAppendAction

@yanjunxiang-google Regarding your previous question about rejecting malformed mutation. I feel ext_proc doesn't need to reject malformed headers as long as low-level codec reject them. And it will be ext_proc server's responsibility of using right API. For example, single-value header like content-length should not be appended.
With that said, we should (1)make sure the codec rejection behavior and (2) have test like this example to check the bad request.

[Ronnie-personal]

Thanks! Let's discuss about the goal of this PR first (as I asked in comment below) since i feel your initial issue with set_cookie should be resolved as long as you set HeaderValueOption.append in existing code.

I agree with you and that's my understanding too.

[Ronnie-personal]

@tyxia

Can you please help to assess if this is a breaking change?

The existing test failed https://dev.azure.com/cncf/envoy/_build/results?buildId=153326&view=logs&j=8c169225-0ae8-53bd-947f-07cb81846cb5&t=d1a98671-b7ba-5fbf-f06c-ff337c010df4&l=609
=========Expected header map:========
':scheme', 'https'
':method', 'GET'
':path', '/foo/the/bar?size=123'
':authority', 'localhost:1000,localhost:1000'
':status', '418'
'content-type', 'text/plain; encoding=UTF8'
'x-append-this', '1'
'x-append-this', '2'
'x-append-this', '3'
'x-remove-and-append-this', '4'
'x-replace-this', 'nope'
'x-envoy-strange-thing', 'No'
'x-append-this', '1'
=======================is not equal to actual header map:=======================
':scheme', 'https'
':method', 'GET'
':path', '/foo/the/bar?size=123'
':authority', 'localhost:1000,localhost:1000'
':status', '418'
'content-type', 'text/plain; encoding=UTF8'
'x-append-this', '1'
'x-envoy-strange-thing', 'No'
'x-append-this', '2'
'x-append-this', '3'
'x-remove-and-append-this', '4'
'x-replace-this', 'no'
'x-replace-this', 'nope'
'x-append-this', '1'

In the current state, when 'append' is not explicitly specified with a value, header mutation processes the header using 'false' as the append option.

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

Lines 88 to 94 in f7c4aac

	// Default of "append" is "false" and mutations
	// are applied in order.
	s = mutation.add_set_headers();
	s->mutable_header()->set_key("x-replace-this");
	s->mutable_header()->set_value("nope");
	// Incomplete structures should be ignored
	mutation.add_set_headers();

In the updated code, when 'append' does not have a value, header mutation processes the header using 'append_action' option instead. The default value of 'append_action' is 0 (APPEND_IF_EXISTS_OR_ADD). As a result 'x-replace-this', 'no' is not replaced as expected.

[tyxia]

@Ronnie-personal Yea, that is a breaking change. I have some ideas (probably a bit hacky) about non-breaking approach.

But one step back, one important question I have asked here which I think is not answered yet: what is the purpose of this PR? (a)Bug fix(if so, could you elaborate the bug details) OR (b)feature enrichment (if so, do you have example use case?)
This will help us evaluate the next step/approach here

[Ronnie-personal]

@Ronnie-personal Yea, that is a breaking change. I have some ideas (probably a bit hacky) about non-breaking approach.

But one step back, one important question I have asked here which I think is not answered yet: what is the purpose of this PR? (a)Bug fix(if so, could you elaborate the bug details) OR (b)feature enrichment (if so, do you have example use case?) This will help us evaluate the next step/approach here

Sorry If I was not clear enough. I replied to your question in #29862 (comment).
The initial work was trigged by this comment #29861 (comment)

I would like to understand why initially 'HeaderValueOption.append_action' is added with with four options?

[gbrail]

I'm just replying to "what was the purpose of this PR?"

Someone changed HeaderValueOption to add a new "append_action" field, with four possible values, in addition to the existing "append" boolean option. However, when that happened, no one updated ext_proc, so it does not honor the new option. I thought it'd be confusing to ext_proc users to have this new option in the proto that ext_proc would ignore, but other code would use, and I suggested implementing it, which is what this PR is about.

I would expect (as a user of ext_proc now) that if append_option were set that it would override append, but that if append_option were unset then the append flag would work as it does today, for backward compatibility. It's not clear from the comments whether that's possible.

Also, there must be some other part of the codebase that uses this structure to modify headers -- could we find that and merge the codebases together into a library in "common" somewhere?

[Ronnie-personal]

I'm just replying to "what was the purpose of this PR?"

Someone changed HeaderValueOption to add a new "append_action" field, with four possible values, in addition to the existing "append" boolean option. However, when that happened, no one updated ext_proc, so it does not honor the new option. I thought it'd be confusing to ext_proc users to have this new option in the proto that ext_proc would ignore, but other code would use, and I suggested implementing it, which is what this PR is about.

Thanks for the clarification, and I agree with you. It's confusing to the end users if append_action is simply ignored by the ext_proc filter.

I would expect (as a user of ext_proc now) that if append_option were set that it would override append, but that if append_option were unset then the append flag would work as it does today, for backward compatibility. It's not clear from the comments whether that's possible.

Initially I implemented a runtime guard which can indicate whether to use append_option or not. But it appears that the runtime guard is not suitable for this use case, as 'append' will not be decommissioned.

Also, there must be some other part of the codebase that uses this structure to modify headers -- could we find that and merge the codebases together into a library in "common" somewhere?

I like the idea of common library.

[tyxia]

@Ronnie-personal @gbrail , The reason i asked the purpose of this PR is because I want to see if we should allow breaking change here. I think we should not break other users at current state of ext_proc unless it is for fixing existing bug. This PR is not for fixing bug in existing code. I like the idea of common library, but still IMHO it should be under the principle of non-breaking change.

Regarding non-breaking approach, here are approaches in my mind

Solution 1:
Add UNSPECIFIED field to HeaderAppendAction , as default 0 to distinguish UNSET, which is also best practice of proto enum

Solution 2:

const Protobuf::FieldDescriptor* field_descriptor =
        sh.GetDescriptor()->FindFieldByName("append_action");
if (sh.GetReflection()->HasField(sh, field_descriptor)) {
      ----> New logic for HeaderAppendAction append_action

 } else {
     -----> Old logic for google.protobuf.BoolValue append 
    append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, false);
}

One important thing: HasField will return false forAPPEND_IF_EXISTS_OR_ADD because proto3 doesn't distinguish between unset field and default 0 field. We can solve this in our context by asking new APPEND_IF_EXISTS_OR_ADD user to set boolean append flag to TRUE because those two fields basically are same and will be handled in the same way

Solution3: same philosophy as 2 but simpler :

if (sh.append_action() == ADD_IF_ABSENT) {
} else if (sh.append_action() == OVERWRITE_IF_EXISTS_OR_ADD) {
} else if (sh.append_action() == OVERWRITE_IF_EXISTS) {
} else {
   -----> APPEND_IF_EXISTS_OR_ADD is handled here and requires setting append flag to TRUE
   append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, false);
}

To summarize:
solution 2 and 3 above are not ideal and need clear documentation. I think the best solution is solution 1, but I am not sure if it is too late to change that.

Add @wbpcode who implemented HeaderAppendAction code(not API) and PR#23518 to see if he has better ideas of achieve our goal that support HeaderAppendAction without breaking boolean append users

[tyxia]

OR if other code owners think we can just simply break boolean append users and replace it with HeaderAppendAction. Then it is a different story, we can just use runtime guard to deprecate append flag and support new HeaderAppendAction.

[Ronnie-personal]

@tyxia Thank you so much for the guidance! I like all of your suggestions, especially the solution 1.

[tyxia]

@wbpcode I am wondering if you think Solution 1 that I mentioned in this comment could work? Looks like updates to this enum will only break two active users (header_parser and header_mutation).

If it doesn't work, if you have any ideas other than solution2/3, please let us know. Basically, i think we want to achieve the goal : Supporting HeaderAppendAction without breaking existing user of boolean append

[github-actions]

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]

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]

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!