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.

Sign up for GitHub

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

add per-httpproxy http-version support #5802

Closed
wants to merge 4 commits into from
Closed

add per-httpproxy http-version support #5802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
75ceb00
add per-httpproxy http-version support
therealak12 Oct 4, 2023
4a4f838
fix type names
therealak12 Jan 6, 2024
b9e13b0
move HTTPVersions under VirtualHost
therealak12 Jan 6, 2024
d5a24c2
fix type names
therealak12 Jan 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions apis/projectcontour/v1/httpproxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// HTTPVersion is an alias to enforce validation
// +kubebuilder:validation:Enum=h2;http/1.1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we already have these constants defined in the ContourConfiguration struct (in v1alpha1) and the Contour config file, let's use the same values for them here for consistency:

Suggested change
// +kubebuilder:validation:Enum=h2;http/1.1
// +kubebuilder:validation:Enum=HTTP/2;HTTP/1.1

Yes, this will then require a translation to the Envoy-accepted values, but we already have code for this in cmd/contour/parseDefaultHTTPVersions and internal/envoy/v3/ProtoNamesForVersions that we should be able to reuse, possibly with some refactoring required.

Copy link
Contributor Author

@therealak12 therealak12 Jan 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parseDefaultHTTPVersions converts from ["HTTP/1.1", "HTTP/2"] to HTTPVersionType.

The ProtoNamesForVersions function converts from HTTPVersionType to ["http/1.1", "h2"] .

Note that the desired format for Envoy configuration is ["http/1.1", "h2"]. So, why use the ["HTTP/1.1", "HTTP/2"] format at all instead of directly using ["http/1.1", "h2"]? 😅

Additionally, those functions cannot be easily imported since they're private, or importing them leads to import cycles unless we move them into a util-like package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment already answers my question. So thanks. I'll refactor.

type HTTPVersion string

// HTTPProxySpec defines the spec of the CRD.
type HTTPProxySpec struct {
// Virtualhost appears at most once. If it is present, the object is considered
Expand Down Expand Up @@ -338,6 +342,11 @@ type VirtualHost struct {
// Only one of IPAllowFilterPolicy and IPDenyFilterPolicy can be defined.
// The rules defined here may be overridden in a Route.
IPDenyFilterPolicy []IPFilterPolicy `json:"ipDenyPolicy,omitempty"`

// HTTPVersions specify the http versions to offer for this HTTPProxy.
// If empty, the DefaultHTTPVersions from v1alpha1.EnvoyConfig will be used.
// It is ignored when TCPProxy is set.
HTTPVersions []HTTPVersion `json:"httpVersions,omitempty"`
}

// JWTProvider defines how to verify JWTs on requests.
Expand Down
5 changes: 5 additions & 0 deletions apis/projectcontour/v1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions changelogs/unreleased/5802-therealak12-small.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Adds HTTPVersions field to HTTPProxy spec. The field is used to specify ALPNProtocols on the corresponding tls context.
11 changes: 11 additions & 0 deletions examples/contour/01-crds.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6933,6 +6933,17 @@ spec:
to the fqdn.
pattern: ^(\*\.)?[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
type: string
httpVersions:
description: HTTPVersions specify the http versions to offer for
this HTTPProxy. If empty, the DefaultHTTPVersions from v1alpha1.EnvoyConfig
will be used. It is ignored when TCPProxy is set.
items:
description: HTTPVersion is an alias to enforce validation
enum:
- h2
- http/1.1
type: string
type: array
ipAllowPolicy:
description: IPAllowFilterPolicy is a list of ipv4/6 filter rules
for which matching requests should be allowed. All other requests
Expand Down
11 changes: 11 additions & 0 deletions examples/render/contour-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7152,6 +7152,17 @@ spec:
to the fqdn.
pattern: ^(\*\.)?[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
type: string
httpVersions:
description: HTTPVersions specify the http versions to offer for
this HTTPProxy. If empty, the DefaultHTTPVersions from v1alpha1.EnvoyConfig
will be used. It is ignored when TCPProxy is set.
items:
description: HTTPVersion is an alias to enforce validation
enum:
- h2
- http/1.1
type: string
type: array
ipAllowPolicy:
description: IPAllowFilterPolicy is a list of ipv4/6 filter rules
for which matching requests should be allowed. All other requests
Expand Down
11 changes: 11 additions & 0 deletions examples/render/contour-gateway-provisioner.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6944,6 +6944,17 @@ spec:
to the fqdn.
pattern: ^(\*\.)?[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
type: string
httpVersions:
description: HTTPVersions specify the http versions to offer for
this HTTPProxy. If empty, the DefaultHTTPVersions from v1alpha1.EnvoyConfig
will be used. It is ignored when TCPProxy is set.
items:
description: HTTPVersion is an alias to enforce validation
enum:
- h2
- http/1.1
type: string
type: array
ipAllowPolicy:
description: IPAllowFilterPolicy is a list of ipv4/6 filter rules
for which matching requests should be allowed. All other requests
Expand Down
11 changes: 11 additions & 0 deletions examples/render/contour-gateway.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7155,6 +7155,17 @@ spec:
to the fqdn.
pattern: ^(\*\.)?[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
type: string
httpVersions:
description: HTTPVersions specify the http versions to offer for
this HTTPProxy. If empty, the DefaultHTTPVersions from v1alpha1.EnvoyConfig
will be used. It is ignored when TCPProxy is set.
items:
description: HTTPVersion is an alias to enforce validation
enum:
- h2
- http/1.1
type: string
type: array
ipAllowPolicy:
description: IPAllowFilterPolicy is a list of ipv4/6 filter rules
for which matching requests should be allowed. All other requests
Expand Down
11 changes: 11 additions & 0 deletions examples/render/contour.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7152,6 +7152,17 @@ spec:
to the fqdn.
pattern: ^(\*\.)?[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
type: string
httpVersions:
description: HTTPVersions specify the http versions to offer for
this HTTPProxy. If empty, the DefaultHTTPVersions from v1alpha1.EnvoyConfig
will be used. It is ignored when TCPProxy is set.
items:
description: HTTPVersion is an alias to enforce validation
enum:
- h2
- http/1.1
type: string
type: array
ipAllowPolicy:
description: IPAllowFilterPolicy is a list of ipv4/6 filter rules
for which matching requests should be allowed. All other requests
Expand Down
3 changes: 3 additions & 0 deletions internal/dag/dag.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -793,6 +793,9 @@ type SecureVirtualHost struct {

// JWTProviders specify how to verify JWTs.
JWTProviders []JWTProvider

// AlpnProtos specify the HTTP version to offer for this vhost
HTTPVersions []string
}

type JWTProvider struct {
Expand Down
16 changes: 16 additions & 0 deletions internal/dag/httpproxy_processor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@
svhost.Secret = sec
svhost.MinTLSVersion = minTLSVer
svhost.MaxTLSVersion = maxTLSVer
svhost.HTTPVersions = p.getSortedHTTPVersions(proxy)

// Check if FallbackCertificate && ClientValidation are both enabled in the same vhost
if tls.EnableFallbackCertificate && tls.ClientValidation != nil {
Expand Down Expand Up @@ -1473,6 +1474,21 @@
return nil
}

// getSortedHTTPVersions returns and empty slice or ["h2", "http/1.1"] or ["http/1.1"].
// This is done to conform with how envoy expects AlpnProtocols in tlsv3.CommonTlsContext.
func (p *HTTPProxyProcessor) getSortedHTTPVersions(proxy *contour_api_v1.HTTPProxy) []string {
proxyHTTPVersions := proxy.Spec.VirtualHost.HTTPVersions
if len(proxyHTTPVersions) == 0 {
return nil
}
for _, httpVersion := range proxyHTTPVersions {

Check warning on line 1484 in internal/dag/httpproxy_processor.go

View check run for this annotation

Codecov / codecov/patch

internal/dag/httpproxy_processor.go#L1484 

Added line #L1484 was not covered by tests
if httpVersion == "h2" {
return []string{"h2", "http/1.1"}
}
}
return []string{"http/1.1"}
}

// expandPrefixMatches adds new Routes to account for the difference
// between prefix replacement when matching on '/foo' and '/foo/'.
//
Expand Down
6 changes: 5 additions & 1 deletion internal/xdscache/v3/listener.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,11 @@ func (c *ListenerCache) OnChange(root *dag.DAG) {

filters = envoy_v3.Filters(cm)

alpnProtos = envoy_v3.ProtoNamesForVersions(cfg.DefaultHTTPVersions...)
if len(vh.HTTPVersions) != 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to make sure to have unit tests for this precedence behavior, i.e. when http versions are set in the virtualhost, they override the default

alpnProtos = vh.HTTPVersions
} else {
alpnProtos = envoy_v3.ProtoNamesForVersions(cfg.DefaultHTTPVersions...)
}
} else {
filters = envoy_v3.Filters(envoy_v3.TCPProxy(listener.Name, vh.TCPProxy, cfg.newSecureAccessLog()))

Expand Down
25 changes: 25 additions & 0 deletions site/content/docs/main/config/api-reference.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1842,6 +1842,15 @@ <h3 id="projectcontour.io/v1.HTTPStatusRange">HTTPStatusRange
</tr>
</tbody>
</table>
<h3 id="projectcontour.io/v1.HTTPVersion">HTTPVersion
(<code>string</code> alias)</p></h3>
<p>
(<em>Appears on:</em>
<a href="#projectcontour.io/v1.VirtualHost">VirtualHost</a>)
</p>
<p>
<p>HTTPVersion is an alias to enforce validation</p>
</p>
<h3 id="projectcontour.io/v1.HeaderHashOptions">HeaderHashOptions
</h3>
<p>
Expand Down Expand Up @@ -4859,6 +4868,22 @@ <h3 id="projectcontour.io/v1.VirtualHost">VirtualHost
The rules defined here may be overridden in a Route.</p>
</td>
</tr>
<tr>
<td style="white-space:nowrap">
<code>httpVersions</code>
<br>
<em>
<a href="#projectcontour.io/v1.HTTPVersion">
[]HTTPVersion
</a>
</em>
</td>
<td>
<p>HTTPVersions specify the http versions to offer for this HTTPProxy.
If empty, the DefaultHTTPVersions from v1alpha1.EnvoyConfig will be used.
It is ignored when TCPProxy is set.</p>
</td>
</tr>
</tbody>
</table>
<hr/>
Expand Down