api: Add support for enabling HTTP/1.0 and HTTP/0.9 #2534
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2534 +/- ##
==========================================
- Coverage 63.94% 63.93% -0.01%
==========================================
Files 117 117
Lines 18057 18061 +4
==========================================
+ Hits 11546 11548 +2
- Misses 5758 5761 +3
+ Partials 753 752 -1 ☔ View full report in Codecov by Sentry. |
|
/retest |
| @@ -97,6 +98,11 @@ type HTTP1Settings struct { | |||
| // By default, Envoy will lowercase all the headers. | |||
| // +optional | |||
| PreserveHeaderCase *bool `json:"preserveHeaderCase,omitempty"` | |||
| // EnableHTTP10 turns on support for HTTP/1.0 and HTTP/0.9 requests. | |||
| EnableHTTP10 *bool `json:"enableHttp10,omitempty"` | |||
There was a problem hiding this comment.
I'd prefer if this was
htp10: {}
so in the future we could opt into injecting a Host header, but id prefer if this was a bool as well and the value was derived from the listener's hostname
There was a problem hiding this comment.
I'm not sure I follow your intent here.
The Envoy Proxy documentation says that Envoy Proxy doesn't support HTTP/1.0 requests correctly iff these arrive without a Host header. That's why configuring a default host is recommended.
accept_http_10
(bool) Handle incoming HTTP/1.0 and HTTP 0.9 requests. This is off by default, and not fully standards compliant. There is support for pre-HTTP/1.1 style connect logic, dechunking, and handling lack of client host iff default_host_for_http_10 is configured.default_host_for_http_10
(string) A default host for HTTP/1.0 requests. This is highly suggested if accept_http_10 is true as Envoy does not otherwise support HTTP/1.0 without a Host header. This is a no-op if accept_http_10 is not true.
According to the Listener definition in the Gateway resource, matching with a hostname is optional. Deriving the hostname from the listener is not guaranteed to be possible, because providing a hostname is not mandatory.
Would you prefer if the API was:
spec:
http1:
enableHttp10: host-name-to-use-as-a-defaultThere was a problem hiding this comment.
imo if a user wants to set the host header for http10, and listener hostname is missing, that should be reflected in the status and policy should not be accepted
http1
http10: {}
optional opt in to set Host Header with the value begin populated from the Listener Hostname field
http1
http10:
setHostHeader: true
@envoyproxy/gateway-maintainers please weigh in
There was a problem hiding this comment.
+1 It would be hard to debug if placing hostnames in two different places and mismatch.
There was a problem hiding this comment.
Updated the proposed API.
| // If this is not set and an HTTP/1.0 request arrives without a host, then | ||
| // it will be rejected. | ||
| // +optional | ||
| SetHostHeder *bool `json:"setHostHeader,omitempty"` |
There was a problem hiding this comment.
I know I proposed set earlier, would addHostHeader be a better name ?
There was a problem hiding this comment.
Since the host header is only added if it's missing, how about useDefaultHost? That's also closer to what the Envoy Proxy configuration key is called.
There was a problem hiding this comment.
@envoyproxy/gateway-maintainers any preferences here ?
There was a problem hiding this comment.
lets go with useDefaultHost unless @zhaohuabing disagrees :)
There was a problem hiding this comment.
If addHostHeaderWhenMissing is too long, then useDefaultHost :-)
|
|
||
| // HTTP10Settings provides HTTP/1.0 configuration on the listener. | ||
| type HTTP10Settings struct { | ||
| // SetHostHeader defines if the HTTP/1.0 request is is missing the Host header, |
There was a problem hiding this comment.
| // SetHostHeader defines if the HTTP/1.0 request is is missing the Host header, | |
| // SetHostHeader defines if the HTTP/1.0 request is is missing the Host header, | |
| // SetHostHeader defines if the HTTP/1.0 request is missing the Host header, |
Signed-off-by: Lior Okman <lior.okman@sap.com>
Signed-off-by: Lior Okman <lior.okman@sap.com>
Signed-off-by: Lior Okman <lior.okman@sap.com>
What this PR does / why we need it:
This PR adds the API for enabling support for HTTP/1.0 and HTTP/0.9 from downstream clients