add per-httpproxy http-version support #5802

therealak12 · 2023-10-04T13:09:55Z

This PR adds a new field to HTTPProxy spec which specifies the HTTP versions to offer for that HTTPProxy. It's used only when spec.tls is set and spec.tcpproxy is not.

A critical use case for the field is when we're serving multiple HTTPProxies where a subset of them use the same wildcard certificate. We can disable http/2 for those utilizing this field while keeping it enabled for others.

Closes #5822

Signed-off-by: Ahmad Karimi <[email protected]>

github-actions · 2023-10-26T00:18:10Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

therealak12 · 2023-10-26T05:12:06Z

Just to make this active.

github-actions · 2023-11-10T00:18:52Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

therealak12 · 2023-11-10T09:44:05Z

Could a maintainer please add a release-note/small label to this PR? Appears I don't have access to do so.

therealak12 · 2023-11-10T19:30:14Z

/remove-lifecycle stale

github-actions · 2023-12-01T00:22:17Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

therealak12 · 2023-12-01T02:40:38Z

/remove-lifecycle stale

github-actions · 2023-12-18T00:21:01Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

therealak12 · 2023-12-18T06:13:24Z

/remove-lifecycle stale

github-actions · 2024-01-03T00:20:02Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

therealak12 · 2024-01-03T06:49:05Z

/remove-lifecycle stale

skriss

Thanks @therealak12, and apologies for the delayed review here.

In addition to the inline comments:

I'm not sure why codecov didn't run for the PR, but we'll need to ensure some test coverage for the new code paths. Either a combination of unit tests in internal/dag and internal/xdscache/v3, or a "feature test" in internal/featuretests/v3 should cover it. Let me know if you need more direction here.
It'd be good to update https://projectcontour.io/resources/faq/ (the question about Safari & 421's) to mention this

skriss · 2024-01-04T22:04:00Z

apis/projectcontour/v1/httpproxy.go

@@ -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
+type HttpVersion string


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

Suggested change

type HttpVersion string

type HTTPVersion string

skriss · 2024-01-04T22:04:43Z

apis/projectcontour/v1/httpproxy.go

@@ -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


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.

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.

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

skriss · 2024-01-04T22:07:46Z

apis/projectcontour/v1/httpproxy.go

+	// 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"`


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.

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

+1 for moving this under VirtualHost

skriss · 2024-01-04T22:12:34Z

internal/xdscache/v3/listener.go

@@ -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


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.

This comment still needs to be addressed

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

skriss · 2024-01-04T22:14:16Z

internal/dag/httpproxy_processor.go

@@ -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 {


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.

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

Signed-off-by: Ahmad Karimi <[email protected]>

codecov · 2024-01-06T11:22:17Z

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 78.78%. Comparing base (7feb49e) to head (d5a24c2).
Report is 240 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5802      +/-   ##
==========================================
+ Coverage   78.61%   78.78%   +0.17%     
==========================================
  Files         138      138              
  Lines       19231    19780     +549     
==========================================
+ Hits        15118    15584     +466     
- Misses       3824     3891      +67     
- Partials      289      305      +16

Files	Coverage Δ
internal/dag/dag.go	`98.70% <ø> (ø)`
internal/dag/httpproxy_processor.go	`91.66% <88.88%> (-0.26%)`	⬇️
internal/xdscache/v3/listener.go	`91.39% <80.00%> (-0.66%)`	⬇️

... and 49 files with indirect coverage changes

sunjayBhatia · 2024-01-17T17:54:51Z

internal/xdscache/v3/listener.go

@@ -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 {


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

github-actions · 2024-02-13T00:19:53Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

m-yosefpor · 2024-02-16T12:58:04Z

still needed

lubronzhan · 2024-02-17T05:56:50Z

Kindly ping @therealak12 Could you resolve what Sunjay mentioned. Thank you!

therealak12 · 2024-02-17T08:58:37Z

Kindly ping @therealak12 Could you resolve what Sunjay mentioned. Thank you!

Waiting for this:
#5802 (comment)

github-actions · 2024-03-13T00:40:46Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

github-actions · 2024-04-13T00:17:28Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

therealak12 force-pushed the per-httpproxy-alpn branch from 726105a to ecd020f Compare October 4, 2023 13:12

therealak12 marked this pull request as ready for review October 4, 2023 14:19

therealak12 requested a review from a team as a code owner October 4, 2023 14:19

therealak12 requested review from skriss and sunjayBhatia and removed request for a team October 4, 2023 14:19

therealak12 force-pushed the per-httpproxy-alpn branch from ecd020f to 1f19118 Compare October 4, 2023 14:21

therealak12 marked this pull request as draft October 4, 2023 14:49

add per-httpproxy http-version support

75ceb00

Signed-off-by: Ahmad Karimi <[email protected]>

therealak12 force-pushed the per-httpproxy-alpn branch from 1f19118 to 75ceb00 Compare October 4, 2023 18:34

therealak12 marked this pull request as ready for review October 7, 2023 12:52

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 26, 2023

github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 27, 2023

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 10, 2023

github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 11, 2023

skriss added the release-note/small A small change that needs one line of explanation in the release notes. label Nov 16, 2023

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 1, 2023

tsaarni removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 1, 2023

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 18, 2023

github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 19, 2023

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 3, 2024

skriss removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 4, 2024

skriss requested changes Jan 4, 2024

View reviewed changes

therealak12 added 3 commits January 6, 2024 10:08

fix type names

4a4f838

Signed-off-by: Ahmad Karimi <[email protected]>

move HTTPVersions under VirtualHost

b9e13b0

Signed-off-by: Ahmad Karimi <[email protected]>

fix type names

d5a24c2

Signed-off-by: Ahmad Karimi <[email protected]>

sunjayBhatia reviewed Jan 17, 2024

View reviewed changes

sunjayBhatia requested review from sunjayBhatia and skriss January 29, 2024 19:15

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 13, 2024

github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 20, 2024

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 13, 2024

github-actions bot closed this Apr 13, 2024

therealak12 deleted the per-httpproxy-alpn branch June 20, 2024 10:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add per-httpproxy http-version support #5802

add per-httpproxy http-version support #5802

therealak12 commented Oct 4, 2023 •

edited

Loading

github-actions bot commented Oct 26, 2023

therealak12 commented Oct 26, 2023

github-actions bot commented Nov 10, 2023

therealak12 commented Nov 10, 2023

therealak12 commented Nov 10, 2023

github-actions bot commented Dec 1, 2023

therealak12 commented Dec 1, 2023

github-actions bot commented Dec 18, 2023

therealak12 commented Dec 18, 2023

github-actions bot commented Jan 3, 2024

therealak12 commented Jan 3, 2024

skriss left a comment

skriss Jan 4, 2024

skriss Jan 4, 2024

therealak12 Jan 6, 2024 •

edited

Loading

therealak12 Jan 6, 2024

skriss Jan 4, 2024

m-yosefpor Jan 5, 2024 •

edited

Loading

sunjayBhatia Jan 5, 2024

skriss Jan 4, 2024

sunjayBhatia Jan 17, 2024

therealak12 Jan 17, 2024

skriss Jan 4, 2024

therealak12 Jan 6, 2024

codecov bot commented Jan 6, 2024 •

edited

Loading

sunjayBhatia Jan 17, 2024

github-actions bot commented Feb 13, 2024

m-yosefpor commented Feb 16, 2024

lubronzhan commented Feb 17, 2024

therealak12 commented Feb 17, 2024

github-actions bot commented Mar 13, 2024

github-actions bot commented Apr 13, 2024

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

add per-httpproxy http-version support #5802

add per-httpproxy http-version support #5802

Conversation

therealak12 commented Oct 4, 2023 • edited Loading

github-actions bot commented Oct 26, 2023

therealak12 commented Oct 26, 2023

github-actions bot commented Nov 10, 2023

therealak12 commented Nov 10, 2023

therealak12 commented Nov 10, 2023

github-actions bot commented Dec 1, 2023

therealak12 commented Dec 1, 2023

github-actions bot commented Dec 18, 2023

therealak12 commented Dec 18, 2023

github-actions bot commented Jan 3, 2024

therealak12 commented Jan 3, 2024

skriss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

therealak12 Jan 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-yosefpor Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 6, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

github-actions bot commented Feb 13, 2024

m-yosefpor commented Feb 16, 2024

lubronzhan commented Feb 17, 2024

therealak12 commented Feb 17, 2024

github-actions bot commented Mar 13, 2024

github-actions bot commented Apr 13, 2024

therealak12 commented Oct 4, 2023 •

edited

Loading

therealak12 Jan 6, 2024 •

edited

Loading

m-yosefpor Jan 5, 2024 •

edited

Loading

codecov bot commented Jan 6, 2024 •

edited

Loading