Skip to content
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

Jump to bottom

ext_proc does not honour HeaderAppendAction #36982

Closed
davidjumani opened this issue Nov 4, 2024 · 8 comments · May be fixed by #39066
Closed

ext_proc does not honour HeaderAppendAction #36982

davidjumani opened this issue Nov 4, 2024 · 8 comments · May be fixed by #39066
Labels
area/ext_proc bug stale stalebot believes this issue/PR has not been touched recently

Comments

@davidjumani
Copy link

Title: ext_proc does not honour HeaderAppendAction

Description:

What issue is being seen? Describe what should be happening instead of

When running envoy with the ext_proc filter and pointing to a server that returns header mutation, envoy does not honour the AppendAction field but only the deprecated Append field as mentioned in the HeaderValueOption docs.
Despite setting AppendAction to any supported value, envoy only overwrites the existing header or appends to it if append is true.

This is because the relevant code change is yet to be made in ext_proc despite the updated API
Link to the code that only checks the value of append

ext_proc server snippet :

func returnResponse(key string, value string, appendAction HeaderValueOption_HeaderAppendAction) *service_ext_proc_v3.HeadersResponse {
	return &service_ext_proc_v3.HeadersResponse{
		Response: &service_ext_proc_v3.CommonResponse{
			HeaderMutation:  &service_ext_proc_v3.HeaderMutation{
				SetHeaders: []*core_v3.HeaderValueOption{
					{
						Header: &core_v3.HeaderValue{
							Key: key, 
							RawValue: []byte(value)
						},
						AppendAction: appendAction,	
	                                        // Append: &wrapperspb.BoolValue{Value: true},
					},
				},	
			},
		},
	}
}
@davidjumani davidjumani added bug triage Issue requires triage labels Nov 4, 2024
@davidjumani davidjumani mentioned this issue Nov 4, 2024
Open
@KBaichoo KBaichoo added area/ext_proc and removed triage Issue requires triage labels Nov 5, 2024
@KBaichoo
Copy link
Contributor

KBaichoo commented Nov 5, 2024

cc @tyxia @yanjunxiang-google

@jewertow
Copy link
Contributor

jewertow commented Nov 7, 2024

I fixed the same issue in ext_authz some time ago, so I could take it if no one is working on it yet.

@tyxia
Copy link
Member

tyxia commented Nov 7, 2024

@jewertow sure please go ahead thanks

@agrawroh agrawroh mentioned this issue Nov 14, 2024
Closed
@agrawroh
Copy link
Contributor

@tyxia We need this as well and I tried taking a stab at it. Would appreciate if you could take a look at the PR.

@github-actions GitHub Actions
Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 14, 2024
@github-actions GitHub Actions
Copy link

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 21, 2024
@yanjunxiang-google
Copy link
Contributor

This issue had been there for a while and multiple folks asked ext_proc to support the append_action.

There had been several attempts which are trying to support it, but later on facing the backward compatibility issue, and have to abandon the efforts eventually.
The main reason of the backward compatibility issue is that ext_proc right now is the only one which are taking "append" default value as false, while everyone else are taking it as true. If ext_proc default value of "append" is "true", then it's easy to support append_action at same time keep it backward compatible. The logic will be:

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

With this logic, if neither append, nor append_action is encoded, it took append_action default which is append_if_exist_or_add. That will be same as append = true. Thus keep it backward compatible if ext_proc filter is changed to have default append value to be "true".

@yanjunxiang-google yanjunxiang-google mentioned this issue Apr 10, 2025
Open
@yanjunxiang-google
Copy link
Contributor

BTW, the ext_proc servers must be changed to always explicitly encode "true" or "false" in the "append" field:

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

Line 469 in 86ca8d7

google.protobuf.BoolValue append = 2
. i.e, do not rely on the Envoy ext_proc filter default value interpretation of "append" if it is not encoded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/ext_proc bug stale stalebot believes this issue/PR has not been touched recently
Projects
None yet
Milestone
No milestone
6 participants
@KBaichoo @davidjumani @agrawroh @jewertow @tyxia @yanjunxiang-google