http: require stream_error_on_invalid_http_messaging to be set for every HCM #9846
Comments
|
@mattklein123 I would like to help out with this. I guess your suggested approach still applies, so I will start by: adding a Also, the original issue meant to track the deprecation of the current option in favour of the new one got closed because stale #9285. Should probably be reopened? |
|
I think the other issue, is that to have the option in the HCM, we need to have the options work in the HTTP/1 and HTTP/3 codecs the way it does for HTTP/2. Make sense? |
Yup sounds good.
Done thanks.
Right. See also #10646. cc @yanavlasov @antoniovicente |
@alyssawilk while in theory this makes sense to me, in practice I am not super confident I know where to start. Do you have any specific pointers? Or, if you already have given some thoughts to how we achieve this, could you expand on your original point? |
|
I'd suggest tackling it once codec at a time. Easiest (since there's already support) would be to do HTTP/2, where you're basically plumbing the HCM new option to be the default if there's no HTTP/2-specific option. You can add the option and hide it via annotation, so you don't have to do it all in one swoop. Next up would probably be HTTP/1.1 since it's a straight forward change. There's two places we send errors, in the codec and in the HCM. the codec closes by default (sendProtocolError) and the HCM leave it open by default even for "protocol" errors like the characters being invalid (unless saw_connection_close was true). Again you could tackle one at a time to close or not based on config. Finally for QUIC, we'd want to somewhat crib the HTTP/2 behavior for HTTP/3, which hopefully you'd have a better feel for by then (if not you can ping me and I can help out). |
|
And either way, @htuch @mattklein123 thoughts on how we can make this required, especially if v4 isn't happening for several years? |
Yes +1. I think this is what we had discussed previously. |
|
I think this should follow minor versions, to provide control planes the right versioning information. So, we introduce it now with comment and warning. I'd probably wait until the next minor version until considering setting fatal-by-default, to allow control planes to know that this is required. @markdroth @envoyproxy/api-shepherds any other thoughts on this? We're basically add a new field that will at some point become mandatory. |
|
I think this is really just a standard deprecation cycle. Deprecate old field, add new field, warn if new field is not set, then eventually make it required and delete old. |
|
Are you saying standard deprecation cycle with minor versioning? Or are you saying it's fine to make it fatal-by-default still, because it's possible to override and we'll still support it being unset indefinitely? |
I guess I'm saying it's a hybrid. We would do standard minor version deprecation on the existing field, but do warn/fatal by default on the other field (since it can be disabled), and then in the next major version we would make the new field required when the old field is removed. |
|
@alyssawilk didn't have time to do any envoy-related work over the past month, so good thing you have picked this up. I should have more time from next week though, and I would still be interested in helping you out. Is there some other task related to this area of work that I can land a hand with? |
|
Sure thing! One thing that might be nice is if we have an HTTP/1.1 protocol option akin to the HTTP/2 option - that way instead of doing the complicated dance with overriding the default and overriding the HTTP/2 option, users could just override the HTTP/1.1 option. The tricky bit is the HCM would have to pull that option from codec, so there'd be an config-API addition and an extension to the codec interface under include. |
Solving the problem where early response errors look different, and don't have HCM tracking, by sending them through the hcm. This means early responses will get all of the usual Envoy standard header additions, get accounted for in listener stats, be visible in access logs, etc. Risk Level: high (changes to HTTP early response path) Testing: new unit tests Docs Changes: n/a Release Notes: inline Runtime guard: envoy.reloadable_features.early_errors_via_hcm Fixes #8545 Part of #9846 Signed-off-by: Alyssa Wilk <[email protected]>
Solving the problem where early response errors look different, and don't have HCM tracking, by sending them through the hcm. This means early responses will get all of the usual Envoy standard header additions, get accounted for in listener stats, be visible in access logs, etc. Risk Level: high (changes to HTTP early response path) Testing: new unit tests Docs Changes: n/a Release Notes: inline Runtime guard: envoy.reloadable_features.early_errors_via_hcm Fixes envoyproxy#8545 Part of envoyproxy#9846 Signed-off-by: Alyssa Wilk <[email protected]>
This unifies HTTP/1.1 and HTTP/2 stream error on invalid messaging. Previously HTTP/1.1 defaulted permissive and HTTP/2 defaulted to strict. This defaults both to strict, resetting connections on invalid requests. This will have a major latency impact if downstream is sending a mix of valid and invalid requests over HTTP/1.1 Additional Description: This change is runtime guarded per default behavioral change rules. It can also be reverted by setting the default to permissive (for prior HTTP/1 behvior) then overriding HTTP/2 to struct (for prior HTTP/2 behavior). This works in conjunction with #11714, as the HTTP connection manager enforces the strictness, so the responses need to be sent via the HTTP connection manager to have strictness applied correctly. Risk Level: High (HCM changes) Testing: new unit tests, updated integration tests Docs Changes: n/a Release Notes: inline Runtime guard: envoy.reloadable_features.hcm_stream_error_on_invalid_message Fixes #9846 Signed-off-by: Alyssa Wilk <[email protected]>
Solving the problem where early response errors look different, and don't have HCM tracking, by sending them through the hcm. This means early responses will get all of the usual Envoy standard header additions, get accounted for in listener stats, be visible in access logs, etc. Risk Level: high (changes to HTTP early response path) Testing: new unit tests Docs Changes: n/a Release Notes: inline Runtime guard: envoy.reloadable_features.early_errors_via_hcm Fixes envoyproxy#8545 Part of envoyproxy#9846 Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: yashwant121 <[email protected]>
This unifies HTTP/1.1 and HTTP/2 stream error on invalid messaging. Previously HTTP/1.1 defaulted permissive and HTTP/2 defaulted to strict. This defaults both to strict, resetting connections on invalid requests. This will have a major latency impact if downstream is sending a mix of valid and invalid requests over HTTP/1.1 Additional Description: This change is runtime guarded per default behavioral change rules. It can also be reverted by setting the default to permissive (for prior HTTP/1 behvior) then overriding HTTP/2 to struct (for prior HTTP/2 behavior). This works in conjunction with envoyproxy#11714, as the HTTP connection manager enforces the strictness, so the responses need to be sent via the HTTP connection manager to have strictness applied correctly. Risk Level: High (HCM changes) Testing: new unit tests, updated integration tests Docs Changes: n/a Release Notes: inline Runtime guard: envoy.reloadable_features.hcm_stream_error_on_invalid_message Fixes envoyproxy#9846 Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: Kevin Baichoo <[email protected]>
This unifies HTTP/1.1 and HTTP/2 stream error on invalid messaging. Previously HTTP/1.1 defaulted permissive and HTTP/2 defaulted to strict. This defaults both to strict, resetting connections on invalid requests. This will have a major latency impact if downstream is sending a mix of valid and invalid requests over HTTP/1.1 Additional Description: This change is runtime guarded per default behavioral change rules. It can also be reverted by setting the default to permissive (for prior HTTP/1 behvior) then overriding HTTP/2 to struct (for prior HTTP/2 behavior). This works in conjunction with envoyproxy#11714, as the HTTP connection manager enforces the strictness, so the responses need to be sent via the HTTP connection manager to have strictness applied correctly. Risk Level: High (HCM changes) Testing: new unit tests, updated integration tests Docs Changes: n/a Release Notes: inline Runtime guard: envoy.reloadable_features.hcm_stream_error_on_invalid_message Fixes envoyproxy#9846 Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: scheler <[email protected]>
As documented in https://www.envoyproxy.io/docs/envoy/latest/configuration/best_practices/level_two, @alyssawilk and I have discussed forcing the user to specify an HCM configuration option that specifies how to handle stream errors, thus guiding users into selecting the correct option for their use case. I think the best way of doing this is:
The text was updated successfully, but these errors were encountered: