feat(translator): implement connection limit by guydc · Pull Request #2952 · envoyproxy/gateway

guydc · 2024-03-17T23:26:49Z

What this PR does / why we need it:

Implements connection limits in CTP based on API defined in api: connection limits #2709.
Simple e2e test that demonstrates connection limiting by opening connections beyond the configured limit and expecting following requests to fail
Docs

Which issue(s) this PR fixes:
Fixes #2685

Signed-off-by: Guy Daich <[email protected]>

codecov · 2024-03-17T23:30:08Z

Codecov Report

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

Project coverage is 64.55%. Comparing base (2882b7c) to head (efbcc23).

Files	Patch %	Lines
internal/ir/zz_generated.deepcopy.go	0.00%	41 Missing and 2 partials ⚠️
internal/xds/translator/listener.go	84.00%	4 Missing and 4 partials ⚠️
internal/xds/translator/translator.go	0.00%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2952      +/-   ##
==========================================
- Coverage   64.62%   64.55%   -0.07%     
==========================================
  Files         121      121              
  Lines       21135    21235     +100     
==========================================
+ Hits        13658    13709      +51     
- Misses       6627     6672      +45     
- Partials      850      854       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Guy Daich <[email protected]>

guydc · 2024-03-18T13:08:17Z

/retest

Signed-off-by: Guy Daich <[email protected]>

guydc · 2024-03-18T17:45:57Z

/retest

guydc · 2024-03-18T17:59:36Z

/retest

Signed-off-by: Guy Daich <[email protected]>

arkodg · 2024-03-18T20:36:15Z

internal/gatewayapi/clienttrafficpolicy.go

+	irConnection := &ir.Connection{}
+
+	if connection.Limit != nil {
+		hasLimit := false


so hasLimit is being used to make sure at least one of the sub fields is set ?
isn't this check already happening in CEL ?

Signed-off-by: Guy Daich <[email protected]>

guydc · 2024-03-19T20:30:47Z

/retest

arkodg · 2024-03-19T21:29:25Z

api/v1alpha1/connection_types.go

-	// +optional
 	// +kubebuilder:validation:Minimum=0
-	Value *int64 `json:"value,omitempty"`
+	Value int64 `json:"value,omitempty"`


is there a default envoy uses here ?
there maybe a valid use case here where the user wants to use the default here (not set it) and set CloseDelay instead (where the default isn't good enough)

No default is mentioned in the docs. It looks like the filter constructor requires a value to be provided: https://github.com/envoyproxy/envoy/blob/e4bd0e6f70749a50dd122d5d7006e8913bb9e84c/source/extensions/filters/network/connection_limit/connection_limit.cc#L17C24-L17C53 by the filter config. So, I think that it's legitimate to require a value if the user decides to opt-in for this feature.

Do you think EG should set a default here ?

I'd vote to leave it to users to decide what this value should be.

It might be difficult to choose a default value for max connections because it depends on things we don't know before deploying EG in production: the scale of clients, the spec of the machine the EG is running on, etc.

+1 to huabing's position. When we support instance (overload manager) and/or listener connection limits with defaults (e.g. 50k), we can validate that this limit is not higher than those limits, which will create an effective "range" for users to choose from.

guydc · 2024-03-19T23:18:37Z

/retest

guydc · 2024-03-20T10:49:21Z

/retest

guydc · 2024-03-20T13:15:59Z

/retest

Signed-off-by: Guy Daich <[email protected]>

guydc · 2024-03-25T17:57:45Z

renamed the container to "connectionLimit" based on discussion here: #2805 (comment)

zirain · 2024-03-25T18:52:48Z

api/v1alpha1/connection_types.go

@@ -9,20 +9,19 @@ import gwapiv1 "sigs.k8s.io/gateway-api/apis/v1"

 // Connection allows users to configure connection-level settings
 type Connection struct {


will this be a little odd?

connection: connectionLimit:

its to tackle the issue outlined in #2805

arkodg

LGTM thanks !

zirain · 2024-03-26T06:44:52Z

/retest

zhaohuabing

Well done! Thanks!

* implement connection limit Signed-off-by: Guy Daich <[email protected]> * fix lint Signed-off-by: Guy Daich <[email protected]> * fix lint 2 Signed-off-by: Guy Daich <[email protected]> * fix ir, coverage Signed-off-by: Guy Daich <[email protected]> * fix lint 3 Signed-off-by: Guy Daich <[email protected]> * open more connection in e2e Signed-off-by: Guy Daich <[email protected]> * fix error type Signed-off-by: Guy Daich <[email protected]> * add additional connections Signed-off-by: Guy Daich <[email protected]> * make limit value required Signed-off-by: Guy Daich <[email protected]> * add error-flow unit test Signed-off-by: Guy Daich <[email protected]> * fix lint 4 Signed-off-by: Guy Daich <[email protected]> * assert policy accepted in test Signed-off-by: Guy Daich <[email protected]> * rename limit => connectionLimit Signed-off-by: Guy Daich <[email protected]> --------- Signed-off-by: Guy Daich <[email protected]> Co-authored-by: zirain <[email protected]> Signed-off-by: Yael Shechter <[email protected]>

* build(deps): bump github.com/prometheus/common from 0.50.0 to 0.51.1 (#3010) Bumps [github.com/prometheus/common](https://github.com/prometheus/common) from 0.50.0 to 0.51.1. - [Release notes](https://github.com/prometheus/common/releases) - [Commits](prometheus/common@v0.50.0...v0.51.1) --- updated-dependencies: - dependency-name: github.com/prometheus/common dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Huabing Zhao <[email protected]> Signed-off-by: Yael Shechter <[email protected]> * docs: fix squash link (#3013) Signed-off-by: zirain <[email protected]> Signed-off-by: Yael Shechter <[email protected]> * api: support IdleTimeout in clientTrafficPolicy Signed-off-by: Yael Shechter <[email protected]> * build(deps): bump fortio.org/fortio from 1.63.4 to 1.63.5 (#3011) Bumps [fortio.org/fortio](https://github.com/fortio/fortio) from 1.63.4 to 1.63.5. - [Release notes](https://github.com/fortio/fortio/releases) - [Commits](fortio/fortio@v1.63.4...v1.63.5) --- updated-dependencies: - dependency-name: fortio.org/fortio dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Huabing Zhao <[email protected]> Signed-off-by: Yael Shechter <[email protected]> * ci: trigger docs action (#3017) Signed-off-by: zirain <[email protected]> Signed-off-by: Yael Shechter <[email protected]> * ci: fix cherrypick permission (#2985) Signed-off-by: zirain <[email protected]> Co-authored-by: Huabing Zhao <[email protected]> Signed-off-by: Yael Shechter <[email protected]> * fix: set path prefix for http ext auth service (#3018) Signed-off-by: huabing zhao <[email protected]> Signed-off-by: Yael Shechter <[email protected]> * feat: add Chinese language support for doc site (#2532) * Add Chinese support for doc site Signed-off-by: Wilson Wu <[email protected]> * Add zh i18n for menus Signed-off-by: Wilson Wu <[email protected]> * Add zh content placeholders Signed-off-by: Wilson Wu <[email protected]> * Fix about zh page Signed-off-by: Wilson Wu <[email protected]> * Apply suggestions from code review Co-authored-by: Huabing Zhao <[email protected]> Signed-off-by: Wilson Wu <[email protected]> * docs: `%s/Application/API/g` (#2929) Signed-off-by: Wilson Wu <[email protected]> * remove old version for zh content Signed-off-by: Wilson Wu <[email protected]> * Update the latest version link Signed-off-by: Wilson Wu <[email protected]> * Fix default content dir Signed-off-by: Wilson Wu <[email protected]> * Translate landing page and fix broken link Signed-off-by: Wilson Wu <[email protected]> * Fix broken link Signed-off-by: Wilson Wu <[email protected]> * Add a contribution page Signed-off-by: Wilson Wu <[email protected]> --------- Signed-off-by: Wilson Wu <[email protected]> Co-authored-by: Huabing Zhao <[email protected]> Co-authored-by: Arko Dasgupta <[email protected]> Co-authored-by: zirain <[email protected]> Signed-off-by: Yael Shechter <[email protected]> * feat(translator): implement connection limit (#2952) * implement connection limit Signed-off-by: Guy Daich <[email protected]> * fix lint Signed-off-by: Guy Daich <[email protected]> * fix lint 2 Signed-off-by: Guy Daich <[email protected]> * fix ir, coverage Signed-off-by: Guy Daich <[email protected]> * fix lint 3 Signed-off-by: Guy Daich <[email protected]> * open more connection in e2e Signed-off-by: Guy Daich <[email protected]> * fix error type Signed-off-by: Guy Daich <[email protected]> * add additional connections Signed-off-by: Guy Daich <[email protected]> * make limit value required Signed-off-by: Guy Daich <[email protected]> * add error-flow unit test Signed-off-by: Guy Daich <[email protected]> * fix lint 4 Signed-off-by: Guy Daich <[email protected]> * assert policy accepted in test Signed-off-by: Guy Daich <[email protected]> * rename limit => connectionLimit Signed-off-by: Guy Daich <[email protected]> --------- Signed-off-by: Guy Daich <[email protected]> Co-authored-by: zirain <[email protected]> Signed-off-by: Yael Shechter <[email protected]> * docs: remove 0.1.0 from linkinator ignore list (#3015) Signed-off-by: yuluo <[email protected]> Co-authored-by: zirain <[email protected]> Signed-off-by: Yael Shechter <[email protected]> * disable connection limit test (#3025) Signed-off-by: huabing zhao <[email protected]> Signed-off-by: Yael Shechter <[email protected]> * fix: use correct connection limit attribute name in e2e, docs (#3028) fix attribute name Signed-off-by: Guy Daich <[email protected]> Signed-off-by: Yael Shechter <[email protected]> * docs: document patches in envoy proxy (#3027) * docs: document patches in envoy proxy Signed-off-by: Yael Shechter <[email protected]> * fix linting issue Signed-off-by: Yael Shechter <[email protected]> * fix pr comment Signed-off-by: Yael Shechter <[email protected]> --------- Signed-off-by: Yael Shechter <[email protected]> * feat(policy): Adding BTP support for UDP/TCPRoute (#3004) * Adding BTP support for UDP/TCP Routes Signed-off-by: Alexander Volchok <[email protected]> * fixining lint Signed-off-by: Alexander Volchok <[email protected]> * fix IdleTime check Signed-off-by: Alexander Volchok <[email protected]> * adding missing generates Signed-off-by: Alexander Volchok <[email protected]> * fixing code review comments Signed-off-by: Alexander Volchok <[email protected]> * incresing code coverage Signed-off-by: Alexander Volchok <[email protected]> --------- Signed-off-by: Alexander Volchok <[email protected]> Co-authored-by: zirain <[email protected]> Co-authored-by: Huabing Zhao <[email protected]> Signed-off-by: Yael Shechter <[email protected]> * fix: add missing http filters to the http filter chain (#2970) * fix: add missing http filters to the http filter chain Signed-off-by: huabing zhao <[email protected]> * refactor Signed-off-by: huabing zhao <[email protected]> * fix lint Signed-off-by: huabing zhao <[email protected]> * add comments Signed-off-by: huabing zhao <[email protected]> * remove refactor Signed-off-by: huabing zhao <[email protected]> * remove refactor Signed-off-by: huabing zhao <[email protected]> * fix gen Signed-off-by: huabing zhao <[email protected]> * fix lint Signed-off-by: Huabing Zhao <[email protected]> --------- Signed-off-by: huabing zhao <[email protected]> Signed-off-by: Huabing Zhao <[email protected]> Signed-off-by: Yael Shechter <[email protected]> * fix pr comment Signed-off-by: Yael Shechter <[email protected]> * ci: grant pull-requests permission to cherrypick action (#3039) Signed-off-by: zirain <[email protected]> Signed-off-by: Yael Shechter <[email protected]> * Update ADOPTERS.md for Airspace Link (#3045) Signed-off-by: Fabrice Aneche <[email protected]> Co-authored-by: zirain <[email protected]> Signed-off-by: Yael Shechter <[email protected]> * feat: added option to specify resources on the OIDC security policy (#3030) added option to specify resources on the OIDC filter Signed-off-by: jaynis <[email protected]> Co-authored-by: Huabing Zhao <[email protected]> Signed-off-by: Yael Shechter <[email protected]> * run make commands Signed-off-by: Yael Shechter <[email protected]> * fix: allow websockets in url rewrite (#3022) allow websockets in url rewrite Signed-off-by: Jesse Haka <[email protected]> Co-authored-by: zirain <[email protected]> Signed-off-by: Yael Shechter <[email protected]> --------- Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Yael Shechter <[email protected]> Signed-off-by: zirain <[email protected]> Signed-off-by: huabing zhao <[email protected]> Signed-off-by: Wilson Wu <[email protected]> Signed-off-by: Guy Daich <[email protected]> Signed-off-by: yuluo <[email protected]> Signed-off-by: Alexander Volchok <[email protected]> Signed-off-by: Huabing Zhao <[email protected]> Signed-off-by: Fabrice Aneche <[email protected]> Signed-off-by: jaynis <[email protected]> Signed-off-by: Jesse Haka <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Huabing Zhao <[email protected]> Co-authored-by: zirain <[email protected]> Co-authored-by: Wilson Wu <[email protected]> Co-authored-by: Arko Dasgupta <[email protected]> Co-authored-by: Guy Daich <[email protected]> Co-authored-by: YuLuo <[email protected]> Co-authored-by: Alex Volchok <[email protected]> Co-authored-by: Fabrice Aneche <[email protected]> Co-authored-by: jaynis <[email protected]> Co-authored-by: Jesse Haka <[email protected]>

implement connection limit

2dca063

Signed-off-by: Guy Daich <[email protected]>

guydc added 5 commits March 18, 2024 06:09

fix lint

e812453

Signed-off-by: Guy Daich <[email protected]>

Merge branch 'main' into impl-connection-limit

bfdc022

fix lint 2

ffae688

Signed-off-by: Guy Daich <[email protected]>

fix ir, coverage

5191c18

Signed-off-by: Guy Daich <[email protected]>

fix lint 3

a1e6bec

Signed-off-by: Guy Daich <[email protected]>

open more connection in e2e

f40b55e

Signed-off-by: Guy Daich <[email protected]>

guydc added 2 commits March 18, 2024 13:15

fix error type

965de99

Signed-off-by: Guy Daich <[email protected]>

add additional connections

1862acc

Signed-off-by: Guy Daich <[email protected]>

guydc marked this pull request as ready for review March 18, 2024 19:45

guydc requested a review from a team as a code owner March 18, 2024 19:45

arkodg reviewed Mar 18, 2024

View reviewed changes

guydc added 5 commits March 19, 2024 05:49

make limit value required

24dd151

Signed-off-by: Guy Daich <[email protected]>

Merge branch 'main' into impl-connection-limit

eda6cfb

add error-flow unit test

a57d66a

Signed-off-by: Guy Daich <[email protected]>

fix lint 4

97580f9

Signed-off-by: Guy Daich <[email protected]>

assert policy accepted in test

55e8421

Signed-off-by: Guy Daich <[email protected]>

arkodg reviewed Mar 19, 2024

View reviewed changes

Merge branch 'main' into impl-connection-limit

aae2e55

Merge branch 'main' into impl-connection-limit

21d982a

guydc added 2 commits March 20, 2024 10:11

Merge branch 'main' into impl-connection-limit

248c9de

Merge branch 'main' into impl-connection-limit

9a059db

guydc added 2 commits March 25, 2024 12:30

Merge branch 'main' into impl-connection-limit

dd991fc

rename limit => connectionLimit

bda9c13

Signed-off-by: Guy Daich <[email protected]>

zirain reviewed Mar 25, 2024

View reviewed changes

arkodg approved these changes Mar 26, 2024

View reviewed changes

zhaohuabing approved these changes Mar 26, 2024

View reviewed changes

Merge branch 'main' into impl-connection-limit

efbcc23

zhaohuabing merged commit decd878 into envoyproxy:main Mar 26, 2024

This was referenced Mar 26, 2024

TestE2E/ConnectionLimit/Close_connections_over_limit is failing #3024

Closed

TestE2E/ConnectionLimit Close_connections_over_limit e2e test seems to constantly fail #3023

Closed

		@@ -9,20 +9,19 @@ import gwapiv1 "sigs.k8s.io/gateway-api/apis/v1"

		// Connection allows users to configure connection-level settings
		type Connection struct {

Conversation

guydc commented Mar 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

guydc commented Mar 18, 2024

Uh oh!

guydc commented Mar 18, 2024

Uh oh!

guydc commented Mar 18, 2024

Uh oh!

arkodg Mar 18, 2024

Choose a reason for hiding this comment

Uh oh!

guydc commented Mar 19, 2024

Uh oh!

arkodg Mar 19, 2024

Choose a reason for hiding this comment

Uh oh!

guydc Mar 19, 2024

Choose a reason for hiding this comment

Uh oh!

arkodg Mar 19, 2024

Choose a reason for hiding this comment

Uh oh!

zhaohuabing Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guydc Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guydc commented Mar 19, 2024

Uh oh!

guydc commented Mar 20, 2024

Uh oh!

guydc commented Mar 20, 2024

Uh oh!

guydc commented Mar 25, 2024

Uh oh!

zirain Mar 25, 2024

Choose a reason for hiding this comment

Uh oh!

arkodg Mar 26, 2024

Choose a reason for hiding this comment

Uh oh!

arkodg left a comment

Choose a reason for hiding this comment

Uh oh!

zirain commented Mar 26, 2024

Uh oh!

zhaohuabing left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

guydc commented Mar 17, 2024 •

edited

Loading

codecov bot commented Mar 17, 2024 •

edited

Loading

zhaohuabing Mar 20, 2024 •

edited

Loading

guydc Mar 20, 2024 •

edited

Loading