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 1 commit
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
Copy link
Member

Choose a reason for hiding this comment

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

Please use the following casing for the Go type/field:

Suggested change
type HttpVersion string
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 All @@ -40,6 +44,11 @@ type HTTPProxySpec struct {
// is given precedence over this field.
// +optional
IngressClassName string `json:"ingressClassName,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"`
Copy link
Member

Choose a reason for hiding this comment

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

I think this should move at least under the VirtualHost struct, since it is only applicable for root HTTPProxies that define a virtualhost. Still thinking about whether it should move under VirtualHost.TLS since it is really only relevant for TLS vhosts AFAIK. cc @sunjayBhatia and @tsaarni for any thoughts here.

Copy link
Contributor

@m-yosefpor m-yosefpor Jan 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think even if we should name it alpnProtos and not httpVersions? because it is only for the offered alpnProtos, so even if h2 is not offered in alpn proto, users can still use h2 with prior knowledge and it is still supported. also if permitInsecure is true, then h2c is supported as well. so maybe being more specific about it in the field name, helps people understand the actual behavior. WDYT? @therealak12 @skriss

Copy link
Member

Choose a reason for hiding this comment

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

+1 for moving this under VirtualHost

}

// Include describes a set of policies that can be applied to an HTTPProxy in a namespace.
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 @@ -5011,6 +5011,17 @@ spec:
spec:
description: HTTPProxySpec defines the spec of the CRD.
properties:
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
includes:
description: Includes allow for specific routing configuration to
be included from another HTTPProxy, possibly in another namespace.
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 @@ -5230,6 +5230,17 @@ spec:
spec:
description: HTTPProxySpec defines the spec of the CRD.
properties:
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
includes:
description: Includes allow for specific routing configuration to
be included from another HTTPProxy, possibly in another namespace.
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 @@ -5022,6 +5022,17 @@ spec:
spec:
description: HTTPProxySpec defines the spec of the CRD.
properties:
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
includes:
description: Includes allow for specific routing configuration to
be included from another HTTPProxy, possibly in another namespace.
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 @@ -5233,6 +5233,17 @@ spec:
spec:
description: HTTPProxySpec defines the spec of the CRD.
properties:
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
includes:
description: Includes allow for specific routing configuration to
be included from another HTTPProxy, possibly in another namespace.
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 @@ -5230,6 +5230,17 @@ spec:
spec:
description: HTTPProxySpec defines the spec of the CRD.
properties:
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
includes:
description: Includes allow for specific routing configuration to
be included from another HTTPProxy, possibly in another namespace.
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 @@ func (p *HTTPProxyProcessor) computeHTTPProxy(proxy *contour_api_v1.HTTPProxy) {
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 @@ func (p *HTTPProxyProcessor) GlobalAuthorizationContext() map[string]string {
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 {
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to reduce the duplication between this new method, cmd/contour/parseDefaultHTTPVersions and internal/envoy/v3/ProtoNamesForVersions. I'll have to look some more for specific ideas on how to refactor for reuse but if you have ideas in the meantime they are welcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do agree to move parseDefaultHTTPVersions and ProtoNamesForVersions into a util package?

proxyHttpVersions := proxy.Spec.HttpVersions
if len(proxyHttpVersions) == 0 {
return nil
}
for _, httpVersion := range proxyHttpVersions {
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 {
alpnProtos = vh.HttpVersions
Copy link
Member

Choose a reason for hiding this comment

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

In this case where the vhost specifies HTTP versions, I think we can also use the virtual host's HTTP versions for the HTTPConnectionManager's codec, above, since it's specific to the vhost.

Copy link
Member

Choose a reason for hiding this comment

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

This comment still needs to be addressed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I'll do when there's an agreement on this.

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

Expand Down
41 changes: 41 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 @@ -161,6 +161,22 @@ <h3 id="projectcontour.io/v1.HTTPProxy">HTTPProxy
is given precedence over this field.</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>
</table>
</td>
</tr>
Expand Down Expand Up @@ -1608,6 +1624,22 @@ <h3 id="projectcontour.io/v1.HTTPProxySpec">HTTPProxySpec
is given precedence over this field.</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>
<h3 id="projectcontour.io/v1.HTTPProxyStatus">HTTPProxyStatus
Expand Down Expand Up @@ -2140,6 +2172,15 @@ <h3 id="projectcontour.io/v1.HeadersPolicy">HeadersPolicy
</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.HTTPProxySpec">HTTPProxySpec</a>)
</p>
<p>
<p>HttpVersion is an alias to enforce validation</p>
</p>
<h3 id="projectcontour.io/v1.IPFilterPolicy">IPFilterPolicy
</h3>
<p>
Expand Down
Loading