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.

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

[feature request] Fault-in-standard-out option on TE header at 1xx and 204 #13868

Closed
lambdai opened this issue Nov 2, 2020 · 17 comments · Fixed by #15880
Closed

[feature request] Fault-in-standard-out option on TE header at 1xx and 204 #13868

lambdai opened this issue Nov 2, 2020 · 17 comments · Fixed by #15880
Labels
area/http help wanted Needs help! stale stalebot believes this issue/PR has not been touched recently

Comments

@lambdai
Copy link
Contributor

lambdai commented Nov 2, 2020

#11458 and #10811 fixed the transfer-encoding: chunked behavior.

Unfortunately the runtime-key is controlling both upstream and downstream.

Zero down time cannot be achieved unless the entire Envoy are upgraded atomically.

Is it possible to provide an option to tolerate non-standard upstream response and provide standard TE?

See the ideal state here.
istio/istio#28450 (comment)

@lambdai lambdai added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Nov 2, 2020
@lambdai
Copy link
Contributor Author

lambdai commented Nov 2, 2020

CC @zuercher

@mattklein123 mattklein123 added area/http and removed enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Nov 3, 2020
@mattklein123
Copy link
Member

cc @alyssawilk to advise

@alyssawilk
Copy link
Contributor

I think it's less about upgrading atomically and more about updating the value of that flag atomically.

If atomic flag flips are problematic, I think it's plausible to break the runtime guards into upstream and downstream variants, and backport, given that Istio has done substantial work on stable releases. cc @PiotrSikora for that.

I'll definitely keep such things in mind for future data plane guards. cc @snowp @mattklein123 as the other folks who tend to handle data plane changes

@linsun
Copy link

linsun commented Dec 8, 2020

subscribe

@hxn36
Copy link

hxn36 commented Dec 11, 2020

Hi, any update on this? Will there be a feature to add those flags automatically through a configmap?

@alyssawilk
Copy link
Contributor

I don't think anyone signed up to add a second flag. cc @zuercher

@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 Jan 14, 2021
@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.

@howardjohn
Copy link
Contributor

@lambdai I think we are still interested?

@Monkeyanator
Copy link

Friendly bump on this, think we're still interested.

From here it seems like the strict_1xx_and_204_response_headers is targeted for removal in 1.18.0? IIUC, if we removed the flag altogether without adding separate flags for upstream and downstream, there would be no way for old proxies using strict_1xx_and_204_response_headers=false to stay compatible with newer proxies?

@mattklein123 mattklein123 reopened this Mar 23, 2021
@mattklein123
Copy link
Member

cc @alyssawilk wdyt?

@mattklein123 mattklein123 added the help wanted Needs help! label Mar 23, 2021
@alyssawilk
Copy link
Contributor

Yeah, I think we ought to split them up.
@lambdai as this was first reported as an issue with istio would you be willing to (and have cycles to) do the split?

@zuercher
Copy link
Member

I'll close the PR I just created (for #14650, because it referenced my PRs I thought it was just misnamed, fixed now). I'm on PTO starting tomorrow so I won't have time to look at this.

@lambdai
Copy link
Contributor Author

lambdai commented Mar 24, 2021

@tbarrella Do you have the bandwidth for this?

@tbarrella
Copy link
Contributor

I should be able to do this. I'm assuming we don't want to break what happens when strict_1xx_and_204_response_headers is true and the new flag is false, or (breakingly) change strict_1xx_and_204_response_headers to an enum. Is there any precedent for how Envoy would interpret two overlapping flags like this? i.e., with features strict_1xx_and_204_response_headers and accept_1xx_and_204_transfer_encoding_headers,

accept does nothing unless strict is true

strict true strict false
accept true receive TE send + receive TE
accept false - send + receive TE

vs.

accept only does something when strict is false (the flag name would have to be more like only_accept)

strict true strict false
accept true - receive TE
accept false - send + receive TE

vs.

strict does nothing if accept is true (the flag name would have to be more like only_accept)

strict true strict false
accept true receive TE receive TE
accept false - send + receive TE

The first option looks the most intuitive to me... @alyssawilk does that seem fine?

@alyssawilk
Copy link
Contributor

I don't think we have precedent for this, so I'm game for anything which allows istio (and others) to roll out multi-level support. Keeping prior behavior with envoy.reloadable_features.strict_1xx_and_204_response_headers true is a nice to have, but I'm totally fine with say deleting that guard and replacing it with two new ones (for upstream / downstream) (permissive/strict), whatever makes sense.

@tbarrella
Copy link
Contributor

Thank you! I think I'll split it into two distinct guards so that the behavior is less confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/http help wanted Needs help! stale stalebot believes this issue/PR has not been touched recently
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants