fix: disable ALPN for non-HTTP routes #4460
Conversation
Signed-off-by: Guy Daich <[email protected]>
internal/ir/xds.go
Outdated
| @@ -355,6 +355,8 @@ type TLSConfig struct { | |||
| StatelessSessionResumption bool `json:"statelessSessionResumption,omitempty" yaml:"statelessSessionResumption,omitempty"` | |||
| // StatefulSessionResumption determines if stateful (session-id based) session resumption is enabled | |||
| StatefulSessionResumption bool `json:"statefulSessionResumption,omitempty" yaml:"statefulSessionResumption,omitempty"` | |||
| // ALPNDisabled determined if ALPN extension is disabled | |||
| ALPNDisabled bool `json:"alpnDisabled,omitempty" yaml:"alpnDisabled,omitempty"` | |||
There was a problem hiding this comment.
can we reuse ALPNProtocols ? and treat nil as a default and empty as disabled ?
There was a problem hiding this comment.
+1 on reusing ALPNProtocols instead of adding a new ALPNDisabled attribute.
Signed-off-by: Guy Daich <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4460 +/- ##
==========================================
+ Coverage 65.74% 65.79% +0.04%
==========================================
Files 210 210
Lines 31516 31526 +10
==========================================
+ Hits 20721 20742 +21
+ Misses 9598 9590 -8
+ Partials 1197 1194 -3 ☔ View full report in Codecov by Sentry. |
internal/gatewayapi/helpers.go
Outdated
|
|
||
| // explicitly disable ALPN by setting an empty list for non-HTTPS use cases | ||
| if protocol != gwapiv1.HTTPSProtocolType { | ||
| tlsListenerConfigs.ALPNProtocols = ptr.To([]string{}) |
There was a problem hiding this comment.
what's the default behivor of empty/nil?
There was a problem hiding this comment.
can you addrees it with comment.
| @@ -896,7 +896,7 @@ func buildXdsUpstreamTLSSocketWthCert(tlsConfig *ir.TLSUpstreamConfig) (*corev3. | |||
| tlsCtx.CommonTlsContext.TlsParams = tlsParams | |||
| } | |||
|
|
|||
| if len(tlsConfig.ALPNProtocols) > 0 { | |||
| if tlsConfig.ALPNProtocols != nil { | |||
There was a problem hiding this comment.
I think it would be clearer to change buildALPNProtocols to return an empty []string if tlsConfig.ALPNProtocols is nil. There's no need to check the length of the tlsConfig.ALPNProtocols array here - it's already checked in the buildALPNProtocols function.
There was a problem hiding this comment.
We have a different behavior for downstream (default: h2, http/1.1) and upstream (default: nil) tls sockets.
For upstreams, envoy manages ALPN options correctly in the connection pools that it initiates per protocol version. So envoy-gateway defaults are really not required, unless a user explicitly opts-in to override this behavior (option 2 in this link)
I think that we just don't need to invoke this function at all in case of upstream sockets.
internal/ir/xds.go
Outdated
| @@ -350,7 +350,7 @@ type TLSConfig struct { | |||
| // SignatureAlgorithms supported by this listener | |||
| SignatureAlgorithms []string `json:"signatureAlgorithms,omitempty" yaml:"signatureAlgorithms,omitempty"` | |||
| // ALPNProtocols exposed by this listener | |||
| ALPNProtocols []string `json:"alpnProtocols,omitempty" yaml:"alpnProtocols,omitempty"` | |||
| ALPNProtocols *[]string `json:"alpnProtocols,omitempty" yaml:"alpnProtocols,omitempty"` | |||
There was a problem hiding this comment.
is the ptr needed ? the zero value of a slice is nil
There was a problem hiding this comment.
Sure. If we don't use a pointer and keep omitempty, the .out files would omit alpnProtocols both for nil and empty slices, so that can make IR/XDS test reading a bit confusing. If we remove omitempty, tests are a bit inflated with alpnProtocols: null. Any preference?
There was a problem hiding this comment.
ah thanks for explaining why you chose this route :) even if .out files omit alpnProtocols, the test title (-empty-alpn) should hopefully solve the confusion imo
There was a problem hiding this comment.
ok, my bad, it's not just a readability issue. When .out files are parsed, all the omitted slices are assumed to be nil, while the IR structs that they are are compared to might be an empty slice (e.g. for TLSRoutes). The .out testdata would always be wrongly generated in this case.
So, options are:
- Don't omitempty
- Customize JSON marshaling with additional tags, methods
- Pointer to slice
I added the first option in a new commit.
Signed-off-by: Guy Daich <[email protected]>
Signed-off-by: Guy Daich <[email protected]>
Signed-off-by: Guy Daich <[email protected]>
Signed-off-by: Guy Daich <[email protected]>
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #4456