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 <guy.daich@sap.com>

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 <guy.daich@sap.com>

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

/retest

Signed-off-by: Guy Daich <guy.daich@sap.com>

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

/retest

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

/retest

Signed-off-by: Guy Daich <guy.daich@sap.com>

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 <guy.daich@sap.com>

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 <guy.daich@sap.com>

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 <guy.daich@sap.com> * fix lint Signed-off-by: Guy Daich <guy.daich@sap.com> * fix lint 2 Signed-off-by: Guy Daich <guy.daich@sap.com> * fix ir, coverage Signed-off-by: Guy Daich <guy.daich@sap.com> * fix lint 3 Signed-off-by: Guy Daich <guy.daich@sap.com> * open more connection in e2e Signed-off-by: Guy Daich <guy.daich@sap.com> * fix error type Signed-off-by: Guy Daich <guy.daich@sap.com> * add additional connections Signed-off-by: Guy Daich <guy.daich@sap.com> * make limit value required Signed-off-by: Guy Daich <guy.daich@sap.com> * add error-flow unit test Signed-off-by: Guy Daich <guy.daich@sap.com> * fix lint 4 Signed-off-by: Guy Daich <guy.daich@sap.com> * assert policy accepted in test Signed-off-by: Guy Daich <guy.daich@sap.com> * rename limit => connectionLimit Signed-off-by: Guy Daich <guy.daich@sap.com> --------- Signed-off-by: Guy Daich <guy.daich@sap.com> Co-authored-by: zirain <zirain2009@gmail.com> Signed-off-by: Yael Shechter <yael.shechter@sap.com>

* 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] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: Yael Shechter <yael.shechter@sap.com> * docs: fix squash link (#3013) Signed-off-by: zirain <zirain2009@gmail.com> Signed-off-by: Yael Shechter <yael.shechter@sap.com> * api: support IdleTimeout in clientTrafficPolicy Signed-off-by: Yael Shechter <yael.shechter@sap.com> * 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] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: Yael Shechter <yael.shechter@sap.com> * ci: trigger docs action (#3017) Signed-off-by: zirain <zirain2009@gmail.com> Signed-off-by: Yael Shechter <yael.shechter@sap.com> * ci: fix cherrypick permission (#2985) Signed-off-by: zirain <zirain2009@gmail.com> Co-authored-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: Yael Shechter <yael.shechter@sap.com> * fix: set path prefix for http ext auth service (#3018) Signed-off-by: huabing zhao <zhaohuabing@gmail.com> Signed-off-by: Yael Shechter <yael.shechter@sap.com> * feat: add Chinese language support for doc site (#2532) * Add Chinese support for doc site Signed-off-by: Wilson Wu <iwilsonwu@gmail.com> * Add zh i18n for menus Signed-off-by: Wilson Wu <iwilsonwu@gmail.com> * Add zh content placeholders Signed-off-by: Wilson Wu <iwilsonwu@gmail.com> * Fix about zh page Signed-off-by: Wilson Wu <iwilsonwu@gmail.com> * Apply suggestions from code review Co-authored-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: Wilson Wu <iwilsonwu@gmail.com> * docs: `%s/Application/API/g` (#2929) Signed-off-by: Wilson Wu <iwilsonwu@gmail.com> * remove old version for zh content Signed-off-by: Wilson Wu <iwilsonwu@gmail.com> * Update the latest version link Signed-off-by: Wilson Wu <iwilsonwu@gmail.com> * Fix default content dir Signed-off-by: Wilson Wu <iwilsonwu@gmail.com> * Translate landing page and fix broken link Signed-off-by: Wilson Wu <iwilsonwu@gmail.com> * Fix broken link Signed-off-by: Wilson Wu <iwilsonwu@gmail.com> * Add a contribution page Signed-off-by: Wilson Wu <iwilsonwu@gmail.com> --------- Signed-off-by: Wilson Wu <iwilsonwu@gmail.com> Co-authored-by: Huabing Zhao <zhaohuabing@gmail.com> Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com> Co-authored-by: zirain <zirain2009@gmail.com> Signed-off-by: Yael Shechter <yael.shechter@sap.com> * feat(translator): implement connection limit (#2952) * implement connection limit Signed-off-by: Guy Daich <guy.daich@sap.com> * fix lint Signed-off-by: Guy Daich <guy.daich@sap.com> * fix lint 2 Signed-off-by: Guy Daich <guy.daich@sap.com> * fix ir, coverage Signed-off-by: Guy Daich <guy.daich@sap.com> * fix lint 3 Signed-off-by: Guy Daich <guy.daich@sap.com> * open more connection in e2e Signed-off-by: Guy Daich <guy.daich@sap.com> * fix error type Signed-off-by: Guy Daich <guy.daich@sap.com> * add additional connections Signed-off-by: Guy Daich <guy.daich@sap.com> * make limit value required Signed-off-by: Guy Daich <guy.daich@sap.com> * add error-flow unit test Signed-off-by: Guy Daich <guy.daich@sap.com> * fix lint 4 Signed-off-by: Guy Daich <guy.daich@sap.com> * assert policy accepted in test Signed-off-by: Guy Daich <guy.daich@sap.com> * rename limit => connectionLimit Signed-off-by: Guy Daich <guy.daich@sap.com> --------- Signed-off-by: Guy Daich <guy.daich@sap.com> Co-authored-by: zirain <zirain2009@gmail.com> Signed-off-by: Yael Shechter <yael.shechter@sap.com> * docs: remove 0.1.0 from linkinator ignore list (#3015) Signed-off-by: yuluo <karashouk.pan@gmail.com> Co-authored-by: zirain <zirain2009@gmail.com> Signed-off-by: Yael Shechter <yael.shechter@sap.com> * disable connection limit test (#3025) Signed-off-by: huabing zhao <zhaohuabing@gmail.com> Signed-off-by: Yael Shechter <yael.shechter@sap.com> * fix: use correct connection limit attribute name in e2e, docs (#3028) fix attribute name Signed-off-by: Guy Daich <guy.daich@sap.com> Signed-off-by: Yael Shechter <yael.shechter@sap.com> * docs: document patches in envoy proxy (#3027) * docs: document patches in envoy proxy Signed-off-by: Yael Shechter <yael.shechter@sap.com> * fix linting issue Signed-off-by: Yael Shechter <yael.shechter@sap.com> * fix pr comment Signed-off-by: Yael Shechter <yael.shechter@sap.com> --------- Signed-off-by: Yael Shechter <yael.shechter@sap.com> * feat(policy): Adding BTP support for UDP/TCPRoute (#3004) * Adding BTP support for UDP/TCP Routes Signed-off-by: Alexander Volchok <alex.volchok@sap.com> * fixining lint Signed-off-by: Alexander Volchok <alex.volchok@sap.com> * fix IdleTime check Signed-off-by: Alexander Volchok <alex.volchok@sap.com> * adding missing generates Signed-off-by: Alexander Volchok <alex.volchok@sap.com> * fixing code review comments Signed-off-by: Alexander Volchok <alex.volchok@sap.com> * incresing code coverage Signed-off-by: Alexander Volchok <alex.volchok@sap.com> --------- Signed-off-by: Alexander Volchok <alex.volchok@sap.com> Co-authored-by: zirain <zirain2009@gmail.com> Co-authored-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: Yael Shechter <yael.shechter@sap.com> * 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 <zhaohuabing@gmail.com> * refactor Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix lint Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * add comments Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * remove refactor Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * remove refactor Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix gen Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix lint Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> --------- Signed-off-by: huabing zhao <zhaohuabing@gmail.com> Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: Yael Shechter <yael.shechter@sap.com> * fix pr comment Signed-off-by: Yael Shechter <yael.shechter@sap.com> * ci: grant pull-requests permission to cherrypick action (#3039) Signed-off-by: zirain <zirain2009@gmail.com> Signed-off-by: Yael Shechter <yael.shechter@sap.com> * Update ADOPTERS.md for Airspace Link (#3045) Signed-off-by: Fabrice Aneche <akhenakh@users.noreply.github.com> Co-authored-by: zirain <zirain2009@gmail.com> Signed-off-by: Yael Shechter <yael.shechter@sap.com> * 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 <kranz.jannis@googlemail.com> Co-authored-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: Yael Shechter <yael.shechter@sap.com> * run make commands Signed-off-by: Yael Shechter <yael.shechter@sap.com> * fix: allow websockets in url rewrite (#3022) allow websockets in url rewrite Signed-off-by: Jesse Haka <haka.jesse@gmail.com> Co-authored-by: zirain <zirain2009@gmail.com> Signed-off-by: Yael Shechter <yael.shechter@sap.com> --------- Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Yael Shechter <yael.shechter@sap.com> Signed-off-by: zirain <zirain2009@gmail.com> Signed-off-by: huabing zhao <zhaohuabing@gmail.com> Signed-off-by: Wilson Wu <iwilsonwu@gmail.com> Signed-off-by: Guy Daich <guy.daich@sap.com> Signed-off-by: yuluo <karashouk.pan@gmail.com> Signed-off-by: Alexander Volchok <alex.volchok@sap.com> Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: Fabrice Aneche <akhenakh@users.noreply.github.com> Signed-off-by: jaynis <kranz.jannis@googlemail.com> Signed-off-by: Jesse Haka <haka.jesse@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Huabing Zhao <zhaohuabing@gmail.com> Co-authored-by: zirain <zirain2009@gmail.com> Co-authored-by: Wilson Wu <iwilsonwu@gmail.com> Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com> Co-authored-by: Guy Daich <guy.daich@sap.com> Co-authored-by: YuLuo <karashouk.pan@gmail.com> Co-authored-by: Alex Volchok <alex.volchok@sap.com> Co-authored-by: Fabrice Aneche <akhenakh@users.noreply.github.com> Co-authored-by: jaynis <kranz.jannis@googlemail.com> Co-authored-by: Jesse Haka <haka.jesse@gmail.com>

implement connection limit

2dca063

Signed-off-by: Guy Daich <guy.daich@sap.com>

guydc added 5 commits March 18, 2024 06:09

fix lint

e812453

Signed-off-by: Guy Daich <guy.daich@sap.com>

Merge branch 'main' into impl-connection-limit

bfdc022

fix lint 2

ffae688

Signed-off-by: Guy Daich <guy.daich@sap.com>

fix ir, coverage

5191c18

Signed-off-by: Guy Daich <guy.daich@sap.com>

fix lint 3

a1e6bec

Signed-off-by: Guy Daich <guy.daich@sap.com>

open more connection in e2e

f40b55e

Signed-off-by: Guy Daich <guy.daich@sap.com>

guydc added 2 commits March 18, 2024 13:15

fix error type

965de99

Signed-off-by: Guy Daich <guy.daich@sap.com>

add additional connections

1862acc

Signed-off-by: Guy Daich <guy.daich@sap.com>

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 <guy.daich@sap.com>

Merge branch 'main' into impl-connection-limit

eda6cfb

add error-flow unit test

a57d66a

Signed-off-by: Guy Daich <guy.daich@sap.com>

fix lint 4

97580f9

Signed-off-by: Guy Daich <guy.daich@sap.com>

assert policy accepted in test

55e8421

Signed-off-by: Guy Daich <guy.daich@sap.com>

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 <guy.daich@sap.com>

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