HCM: finalize header after upstream is selected

[lambdai]

Commit Message:
Bring back "UPSTREAM_METADATA" evaluation in request_headers_to_add
Signed-off-by: Yuchen Dai silentdai@gmail.com

Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Fixes: #12236

[alyssawilk]

Nice catch on metadata. Unfortunately I don't think we can move this quite so easily.

If we move it to onPoolReady, I think we're going to call it multiple times for retries, and for hedging, which may be a large, and potentially undesirable change for many. If we try to hack around it with only calling finalize once, it'd still have the wrong upstream data for retries. I think we'd first need an option for if headers should be added once, or per-retry, sort out what to do for hedging. This might be easier integrated into the work being done on upstream filters?
cc @snowp and @junr03 for thoughts.

[lambdai]

Thank @alyssawilk for the feedback! I had the intuitive that it won't be this easy :)

I think there are 2 issues

1. which component should evaluate the headers?
2. Should we introduce a policy for request_header_to_add, or should we introduce another bucket for headers?
   AFAIK we have two buckets,
   a. request_header_to_add, which can be evaluated and configured by user
   b. x-envoy-XXX, e.g. x-envoy-attempt-count it's not configurable but the semantic is clear.
   How about a per_retry_headers_to_add which support evaluation and with allow policy "last_win", "append_only"?

[alyssawilk]

yeah, I'd be inclined to say we deprecate request headers to add, and replace it with a message with policy per header. That's going to need some input from the @envoyproxy/api-shepherds . Again the single-reply case is pretty easy. I think the two options you mention are fine for redirects. I hedging may get a little implementation-ugly unless we do header copies (which we may have to do) and we may want to consider internal redirects as part of the API as well (even if they're less often used cc @penguingao )

[stale]

This pull request has been automatically marked as stale because it has not had activity in the last 7 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!

[stale]

This pull request has been automatically closed because it has not had activity in the last 14 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!