feat(translator): http2 upstream settings #3682
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3682 +/- ##
==========================================
+ Coverage 67.59% 67.61% +0.01%
==========================================
Files 183 184 +1
Lines 22446 22521 +75
==========================================
+ Hits 15173 15227 +54
- Misses 6193 6212 +19
- Partials 1080 1082 +2 ☔ View full report in Codecov by Sentry. |
guydc
force-pushed
the
btp-http2
branch
3 times, most recently
from
044255c to
f567f3c
Compare
|
/retest |
api/v1alpha1/shared_types.go
Outdated
| @@ -478,3 +479,33 @@ type BackendRef struct { | |||
| // A CIDR can be an IPv4 address range such as "192.168.1.0/24" or an IPv6 address range such as "2001:0db8:11a3:09d7::/64". | |||
| // +kubebuilder:validation:Pattern=`((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\/([0-9]+))|((([0-9a-fA-F]{1,4}:){7,7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,7}:|([0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,5}(:[0-9a-fA-F]{1,4}){1,2}|([0-9a-fA-F]{1,4}:){1,4}(:[0-9a-fA-F]{1,4}){1,3}|([0-9a-fA-F]{1,4}:){1,3}(:[0-9a-fA-F]{1,4}){1,4}|([0-9a-fA-F]{1,4}:){1,2}(:[0-9a-fA-F]{1,4}){1,5}|[0-9a-fA-F]{1,4}:((:[0-9a-fA-F]{1,4}){1,6})|:((:[0-9a-fA-F]{1,4}){1,7}|:)|fe80:(:[0-9a-fA-F]{0,4}){0,4}%[0-9a-zA-Z]{1,}|::(ffff(:0{1,4}){0,1}:){0,1}((25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])\.){3,3}(25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])|([0-9a-fA-F]{1,4}:){1,4}:((25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])\.){3,3}(25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9]))\/([0-9]+))` | |||
| type CIDR string | |||
|
|
|||
| // HTTP2Settings provides HTTP/2 configuration on the listener. | |||
There was a problem hiding this comment.
Nit:
| // HTTP2Settings provides HTTP/2 configuration on the listener. | |
| // HTTP2Settings provides HTTP/2 configuration for listeners and backends. |
| @@ -478,3 +479,33 @@ type BackendRef struct { | |||
| // A CIDR can be an IPv4 address range such as "192.168.1.0/24" or an IPv6 address range such as "2001:0db8:11a3:09d7::/64". | |||
| // +kubebuilder:validation:Pattern=`((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\/([0-9]+))|((([0-9a-fA-F]{1,4}:){7,7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,7}:|([0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,5}(:[0-9a-fA-F]{1,4}){1,2}|([0-9a-fA-F]{1,4}:){1,4}(:[0-9a-fA-F]{1,4}){1,3}|([0-9a-fA-F]{1,4}:){1,3}(:[0-9a-fA-F]{1,4}){1,4}|([0-9a-fA-F]{1,4}:){1,2}(:[0-9a-fA-F]{1,4}){1,5}|[0-9a-fA-F]{1,4}:((:[0-9a-fA-F]{1,4}){1,6})|:((:[0-9a-fA-F]{1,4}){1,7}|:)|fe80:(:[0-9a-fA-F]{0,4}){0,4}%[0-9a-zA-Z]{1,}|::(ffff(:0{1,4}){0,1}:){0,1}((25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])\.){3,3}(25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])|([0-9a-fA-F]{1,4}:){1,4}:((25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])\.){3,3}(25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9]))\/([0-9]+))` | |||
| type CIDR string | |||
|
|
|||
| // HTTP2Settings provides HTTP/2 configuration on the listener. | |||
| type HTTP2Settings struct { | |||
There was a problem hiding this comment.
Should we use the same default value for both the listeners and clusters?
There was a problem hiding this comment.
- envoy recommendations for secure-by-default are the same for window sizes.
- max concurrent streams do not necessarily require hardening in the cluster context.
- stream resetting behavior is mostly a downstream concern.
We can move some of the defaulting behavior to the API, but some will remain in the XDS translator (due to differences between clusters and listeners). So, I propose we keep it as-is for now.
There was a problem hiding this comment.
@guydc Thanks for the explanation! I'm just curious because the recommendation is for listeners. They can be the same for Clusters if there's no unintentional side effects.
Signed-off-by: Guy Daich <guy.daich@sap.com>
|
/retest |
api/v1alpha1/shared_types.go
Outdated
| // It's recommended for L2 Envoy deployments to set this value to true. | ||
| // https://www.envoyproxy.io/docs/envoy/latest/configuration/best_practices/level_two | ||
| // +optional | ||
| ResetStreamOnError *bool `json:"resetStreamOnError,omitempty"` |
There was a problem hiding this comment.
false here really means terminateConnectionOnError , and true means terminateStreamOnError
is there any other API name and value that can encapsulate this ?
There was a problem hiding this comment.
Yea, talking about connections instead of streams would be easier for users to understand and better comprehend the impact. The only downside is that this could be confusing to someone who reads the envoy docs.
Signed-off-by: Guy Daich <guy.daich@sap.com>
|
/retest |
Signed-off-by: Guy Daich <guy.daich@sap.com>
|
/retest |
Signed-off-by: Guy Daich <guy.daich@sap.com>
Signed-off-by: Guy Daich <guy.daich@sap.com>
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/retest |
What this PR does / why we need it:
override_stream_error_on_invalid_http_messageoption which is specifically relevant for L2 envoy deployments running http2: https://www.envoyproxy.io/docs/envoy/latest/configuration/best_practices/level_twoWhich issue(s) this PR fixes:
Fixes #3670, #3245
Related to #1048